#10 user address book implementation #14
Conversation
|
@SlimDeluxe Need to work on few more test cases. |
|
These should be checkboxes and not radio boxes, since they are not exclusive (default can also be a company address at the same time): The firs line is always required and should not be deletable: Please change to "Add address line" Otherwise, good job, looking good :) |
|
@SlimDeluxe got it |
database/migrations/2025_06_16_150346_create_user_addresses_table.php
Outdated
Show resolved
Hide resolved
|
@SlimDeluxe I haven't fixed this one yet |
|
Ya we can add those easily with the current setup. I will update it in
evening.
…On Fri, Jun 20, 2025, 6:20 PM Omer Sabic ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In database/migrations/2025_06_16_150346_create_user_addresses_table.php
<#14 (comment)>
:
> + $table->enum('type', ['default_address', 'company_address'])
+ ->default('default_address');
I see. How can one then fetch a default address for a user, or check if an
address is default for the user? Doesn't the json/enum complicate things in
this case? If you can add User::getDefaultAddress() and
Address::isDefault(), then I can see if it's better than a separate
column, or worse.
—
Reply to this email directly, view it on GitHub
<#14 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASFRQEXBT3ALWY5IQ3PHKNL3EP5YNAVCNFSM6AAAAAB7WVWRDOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDSNBVHE2TKNZZGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
I want to see if you can manage to add settings.
|
|
Ya almost forgot about that. Sure I will add this as well.
…On Fri, Jun 20, 2025, 6:49 PM Omer Sabic ***@***.***> wrote:
*SlimDeluxe* left a comment (DataLinx/eclipsephp-core#14)
<#14 (comment)>
1. Only one address can be set as default. Currently, you can create
multiple default ones. When a user submits a form with a default checkbox
ticked, his current default address should be unset as default. Please also
add a feature test for this.
2. Company name should be shown in the address in the table, between
the recipient name and street address.
3. VAT ID should be available as a separate column in the table.
4. I don't see this thing from the instructions:
"Address book" feature can be enabled/disabled in the system settings
I want to see if you can manage to add settings.
Please note, the setting should be added in the EclipseSettings. Don't
create a new class just for this.
5. Please add tests for this:
image.png (view on web)
<https://github.com/user-attachments/assets/5c856b8d-45d3-43b1-b5bb-de78c9aa143a>
—
Reply to this email directly, view it on GitHub
<#14 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASFRQEQXKATQGXM2E6YC2LL3EQBFHAVCNFSM6AAAAAB7WVWRDOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSOJRGU2DAMRRGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
SlimDeluxe
left a comment
SlimDeluxe
left a comment
There was a problem hiding this comment.
Requested changes are in the PR comments.
@SlimDeluxe I finished 1, 2, 3 and 4. Test part is left to be done. I will continue that on monday. By the way I wanted to ask 1 was a bit confusing to understand. Here what I implemented so if a user already has a default address and he again tried to created a default address then the type should be unset. Basically only one default address. That is what I implemented. Don't know if you wanted this or something like if he add new default address then the old default address type should be reset. |
|
Hm unfortunately I can't log in into the app with the latest changes.
Running fresh migrations and seeding, clearing cache, does not help. |
|
@SlimDeluxe Sorry about that its working for me so might be our setup thats creating a issue here. I will work on a fix for this. And maybe I should start using lando Screen.Recording.2025-06-21.at.2.54.42.PM.mov |
…inx#10-user-address-book-implementation
|
Some fixes required:
|
|
Got it that will be a quick fix
…On Tue, Jun 24, 2025, 1:32 PM Omer Sabic ***@***.***> wrote:
*SlimDeluxe* left a comment (DataLinx/eclipsephp-core#14)
<#14 (comment)>
Some fixes required:
1. When a default address exists and you edit/create another address
and want to make it the new default, nothing happens. The old one stays the
default.
2. When deleting a default address, if the user has more addresses,
the oldest one should be the new default.
—
Reply to this email directly, view it on GitHub
<#14 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASFRQESRX7BV7XOWWFOBSYL3FD7B3AVCNFSM6AAAAAB7WVWRDOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSOJZGE4TSMRTGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
@SlimDeluxe point 1. This already was done. Like when you create a new address with default check nothing will happen if there is already a default address. And I added a fix for point 2 |
…ook-implementation # Conflicts: # src/Models/User.php
|
….com:thapacodes4u/eclipsephp-core into DataLinx#10-user-address-book-implementation
@SlimDeluxe Its fixed |
There was a problem hiding this comment.
Sorry for the delay.
I've reviewed the code and tested the UI.
Here's what's still missing:
- Very important tests I requested earlier:
- Additional tests:
- only one address can be default (a new default address unsets the old one as default)
- when deleting a default address, the oldest one becomes the default
- See inline code comment below
tests/Feature/Filament/RelationManagers/AddressesRelationManagerTest.php
Show resolved
Hide resolved
3 participants
#10