Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ResuBaka
Copy link
Collaborator

With this the controller can now:

  • add new addresses
  • delete addresses
  • get all addresses from a user

With this the controller can now:
* add new addresses
* delete addresses
* get all addresses from a user
@ResuBaka ResuBaka changed the title Feature/creating deleting user addresses WIP: Feature/creating deleting user addresses Jun 21, 2019
@ResuBaka
Copy link
Collaborator Author

When you want to merge it the restriction in the API that only two addresses are allowed needs to be removed.

Copy link

@sandermangel sandermangel left a 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.


$bAddress->setData($updatedAdress)->setIsDefaultBilling(1)->save();
$updatedBillingId = $bAddress->getId();
}
if($updatedAdress['default_shipping']) {
} elseif ($updatedAdress['default_shipping']) {

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?

Copy link
Collaborator Author

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.

$updatedAdress['parent_id'] = $customer->getId();
$updatedAdress['street'] = $addressHelper->concatStreetData($updatedAdress['street']);

$sAddress->setData($updatedAdress)->save();

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?

Copy link
Collaborator Author

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])) {

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?

Copy link
Collaborator Author

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];
Copy link

@sandermangel sandermangel Jul 2, 2019

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);

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

Copy link
Collaborator Author

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.

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?

Copy link
Collaborator Author

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;

@ResuBaka ResuBaka changed the title WIP: Feature/creating deleting user addresses Feature/creating deleting user addresses Jul 2, 2019
@ResuBaka
Copy link
Collaborator Author

@sandermangel any news on this pr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants