Feature/creating deleting user addresses#41
Feature/creating deleting user addresses#41ResuBaka wants to merge 5 commits intoDivanteLtd:masterfrom
Conversation
With this the controller can now: * add new addresses * delete addresses * get all addresses from a user
ResuBaka
changed the title
|
When you want to merge it the restriction in the API that only two addresses are allowed needs to be removed. |
sandermangel
left a comment
sandermangel
left a comment
There was a problem hiding this comment.
Thank you for your PR!
I've added some comments. If you would like to discuss them feel free to find me on the vuestorefront slack or leave comments on this PR.
magento1-module/app/code/local/Divante/VueStorefrontBridge/controllers/UserController.php
Outdated
Show resolved
Hide resolved
magento1-module/app/code/local/Divante/VueStorefrontBridge/controllers/UserController.php
Outdated
Show resolved
Hide resolved
| $updatedBillingId = $bAddress->getId(); | ||
| } | ||
| if($updatedAdress['default_shipping']) { | ||
| } elseif ($updatedAdress['default_shipping']) { |
There was a problem hiding this comment.
An address can be default billing and default shipping at the same time. I' m not sure if this is allowed here since you are using an elseif?
There was a problem hiding this comment.
Yea you are right i need to push my latest changes here as i needed to fix a lot of logic bugs in here.
Which are all fixed now, but i have forgotten to update this PR since then.
magento1-module/app/code/local/Divante/VueStorefrontBridge/controllers/UserController.php
Show resolved
Hide resolved
| $updatedAdress['parent_id'] = $customer->getId(); | ||
| $updatedAdress['street'] = $addressHelper->concatStreetData($updatedAdress['street']); | ||
|
|
||
| $sAddress->setData($updatedAdress)->save(); |
There was a problem hiding this comment.
I'm not sure I understand this save action, isn't the address saved in previous steps in this class?
There was a problem hiding this comment.
This should now be fixed.
| { | ||
| $concatStreetData = ''; | ||
|
|
||
| if (isset($streetData[0], $streetData[1])) { |
There was a problem hiding this comment.
Magento allows for 4 address lines, can we also do this here to make it compatible?
There was a problem hiding this comment.
We can to it but as the the reference in here just shows 2. I have just Implement two.
But i think that should be done in a later PR. As this is the logic which I have mostly tested.
| $concatStreetData = ''; | ||
|
|
||
| if (isset($streetData[0], $streetData[1])) { | ||
| $concatStreetData = $streetData[0] . ' ' . $streetData[1]; |
There was a problem hiding this comment.
Magento stores address separated by a new line. This prevents false splits when a street contains a space in the name itself. Might I suggest implode("\n", $streetData); here?
| */ | ||
| public function splitStreetData(string $streetData): array | ||
| { | ||
| $streetData = preg_split('/(?=\d)/', $streetData, 2); |
There was a problem hiding this comment.
Here I would also suggest working with new lines instead of spaces
There was a problem hiding this comment.
I think that is a bit harder.
I have just checked in the database how an Address is saved by magento.
"1Street Number"
"2Street Number"
And these to are separated by now line. So it is now as easy to move to newline for it.
There was a problem hiding this comment.
Hm oko, I have to take a look at Magento 1 then. What database table & field did you check?
There was a problem hiding this comment.
This is the sql which does work for me to get all Addresses.
select * from customer_address_entity_text where attribute_id = 25;
ResuBaka
changed the title
|
@sandermangel any news on this pr? |
3 participants
With this the controller can now: