-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/creating deleting user addresses #41
base: master
Are you sure you want to change the base?
Feature/creating deleting user addresses #41
Conversation
With this the controller can now: * add new addresses * delete addresses * get all addresses from a user
When you want to merge it the restriction in the API that only two addresses are allowed needs to be removed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
||
$bAddress->setData($updatedAdress)->setIsDefaultBilling(1)->save(); | ||
$updatedBillingId = $bAddress->getId(); | ||
} | ||
if($updatedAdress['default_shipping']) { | ||
} elseif ($updatedAdress['default_shipping']) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now be fixed.
{ | ||
$concatStreetData = ''; | ||
|
||
if (isset($streetData[0], $streetData[1])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magento allows for 4 address lines, can we also do this here to make it compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I would also suggest working with new lines instead of spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the sql which does work for me to get all Addresses.
select * from customer_address_entity_text where attribute_id = 25;
@sandermangel any news on this pr? |
With this the controller can now: