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

Users can edit adress alias during checkout #15216

Merged
merged 1 commit into from Sep 27, 2019

Conversation

@pauloffb
Copy link
Contributor

commented Aug 22, 2019

Registered users can edit adress alias during checkout. With current behaviour if they add several addresses right from the checkout they get a bunch of "My address" which can confuse users.There is an issue open for this #9913.

Questions Answers
Branch? develop
Description? When registered users add adresses from the checkout steps, they don't see the field for editing the alias for the address, and they accumulate "My address" which is the default alias. With this fix they see the alias field as an optional that they can edit or not.
Type? bug fix
Category? FO
BC breaks? Don't think so.
Deprecations? Don't think so... sorry.
Fixed ticket? Fixes #9913
How to test? Try to add addresses from the checkout phase bot as a guest and you should't see the alias field and as a registered user and you should see the alias field as an option.

This change is Reviewable

Registered users can edit adress alias during checkout. With current behaviour if they add several addresses right from the checkout they get a bunch of "My address" which can confuse users.There is an issue open for this [BOOM-3692].
@pauloffb pauloffb requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 22, 2019
@prestonBot

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2019

Hello @pauloffb!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@pauloffb pauloffb force-pushed the pauloffb:bugfix branch 2 times, most recently from 9197800 to 02e6b75 Aug 22, 2019
@matthieu-rolland

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

Thank you for your contribution @pauloffb !

@colinegin and @marionf

This PR adds the alias field when editing an address during checkout.
I ping you to know if this is really what you want, regarding this issue #9913

Capture d’écran 2019-08-22 à 17 11 31

{if $field.name eq "alias"}
{* we don't ask for alias here *}
{if $field.name eq "alias" and $customer.is_guest}
{* we don't ask for alias here if customer is not registered *}

This comment has been minimized.

Copy link
@matthieu-rolland

matthieu-rolland Aug 22, 2019

Contributor

This looks good to me, codewise, but I'll wait for the PM team's opinion before approving your PR. Thanks !

@colinegin

This comment has been minimized.

Copy link
Collaborator

commented Aug 22, 2019

Hello @matthieu-rolland and @pauloffb ,

First thanks for your contribution !
This behaviour makes sense to me but for more consistency, I think we should also add the field to the account creation form during the checkout process.
Would that be easy for you to do it @pauloffb ?

Thanks !

@pauloffb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Hi @colinegin and @matthieu-rolland,

I see this during checkout with account creation, do you see it differently or are we talking about different things? 😄

RTJDQcCyNY

Or do you mean that the field should also appear as a guest, because Prestashop also gives guest addresses the default "My Address" alias and the guest could change his mind after checking out and completes the account creation form?

@colinegin

This comment has been minimized.

Copy link
Collaborator

commented Aug 26, 2019

My bad, I thought your PR only added the alias field when editing the address and fixed the issue detailed in #9913.
What you actually do with this PR is :

  • fixing the issue related to the change in the alias name when editing the address in the checkout process
  • adding the alias field when editing the address form in the checkout
  • adding the alias field in the form during account creation in the checkout

Am I correct ? If yes, this sounds perfect to me !

@colinegin colinegin self-assigned this Aug 26, 2019
@pauloffb

This comment has been minimized.

Copy link
Contributor Author

commented Aug 27, 2019

I believe it would "fix" all those 3 things... at least making it consistent.

However one could argue if the alias field should really be required (in BO it is really required, in FO it is not, but it automatically assigns the default "My address" which is even worse).

@colinegin

This comment has been minimized.

Copy link
Collaborator

commented Aug 28, 2019

Maybe this field could be pre-filled with "My address" but letting the user change it if needed.
What do you think ?

@colinegin colinegin added PM ✔️ and removed waiting for PM labels Aug 28, 2019
Copy link
Contributor

left a comment

LGTM;

Thank you for your contribution @pauloffb 👍

@pauloffb

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

Hi @colinegin and @matthieu-rolland,

Just seen @colinegin comment...

Maybe this field could be pre-filled with "My address" but letting the user change it if needed.
What do you think ?

I think that makes sense, should I do it? Can I just make another commit to this PR?

@colinegin

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

@pauloffb what is the behaviour you chose to apply in this situation ?

@pauloffb

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

@colinegin it is explained before. The field is empty but is shown to the user, if he choose to put someting in there it will store with that alias, if the user choose not to edit, it will store with the default alias (by language: My Address, Meu endereço...)

I agree that maybe the correct behavior should be to prefill the field with the default alias. The correction can't be made on the theme templates as before, we'll have to get it from here (I guess):

public function getTemplateVariables()
{
$context = Context::getContext();
if (!$this->formFields) {
// This is usually done by fillWith but the form may be
// rendered before fillWith is called.
// I don't want to assign formFields in the constructor
// because it accesses the DB and a constructor should not
// have side effects.
$this->formFields = $this->formatter->getFormat();
}
$this->setValue('token', $this->persister->getToken());
$formFields = array_map(
function (FormField $item) {
return $item->toArray();
},
$this->formFields
);
if (empty($formFields['firstname']['value'])) {
$formFields['firstname']['value'] = $context->customer->firstname;
}
if (empty($formFields['lastname']['value'])) {
$formFields['lastname']['value'] = $context->customer->lastname;
}
return array(
'id_address' => (isset($this->address->id)) ? $this->address->id : 0,
'action' => $this->action,
'errors' => $this->getErrors(),
'formFields' => $formFields,
);
}

@Robin-Fischer-PS Robin-Fischer-PS added this to the 1.7.7.0 milestone Sep 27, 2019
@colinegin

This comment has been minimized.

Copy link
Collaborator

commented Sep 27, 2019

@pauloffb ,

no worries let's leave it like it and merge this PR :)

@matthieu-rolland matthieu-rolland merged commit 659445d into PrestaShop:develop Sep 27, 2019
3 checks passed
3 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthieu-rolland

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2019

Thank you @pauloffb !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.