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

Migrate customer Add/Edit actions #11723

Merged
merged 51 commits into from Jan 6, 2019

Conversation

@sarjon
Copy link
Member

sarjon commented Dec 11, 2018

Questions Answers
Branch? develop
Description? Migrates Add/Edit pages for Customer
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #10560
How to test? You can access new pages from admin-dev/index.php/sell/customers, they should work the same as in legacy pages. NOTE: rebuild assets before testing.

This change is Reviewable

),
CustomerConstraintException::class => [
CustomerConstraintException::DUPLICATE_EMAIL => $this->trans(
'A registered customer account using the defined email address already exists. ',

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Dec 11, 2018

Contributor

Watch the space at the end of the string.

This comment has been minimized.

@sarjon

sarjon Dec 12, 2018

Author Member

space exists in the string. :)

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Dec 12, 2018

Contributor

Alright then, thanks!

@matks
Copy link
Contributor

matks left a comment

Some minor fixes but the overall quality of this PR is very good

And you have tests ❤️ 💚 💙 💛

@sarjon sarjon force-pushed the sarjon:migrate/customer-crud branch from c5cf6b7 to 0a8c340 Dec 12, 2018

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Dec 13, 2018

@matks i think this PR is ready for final review and QA. :)

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 13, 2018

@sarjon only #11723 (comment) to handle ;)

private function assertIsApeCode($code)
{
if (is_string($code) && empty($code)) {

This comment has been minimized.

@sarjon

sarjon Dec 13, 2018

Author Member

@matks i've made empty string as valid value for APE code, since customers in PS can be assigned with an empty APE value. do you think its ok? 🤔

This comment has been minimized.

@matks

matks Dec 14, 2018

Contributor

That I dont know. @colinegin ? 😄

@marionf marionf self-assigned this Dec 14, 2018

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Dec 14, 2018

@sarjon

  1. I have an error "birthday field invalid", while I inserted the date in the expected format dd/mm/yyyy

capture d ecran_776

capture d ecran_777

  1. I should be redirected to the customer list instead of customer page when a customer is successfully added

capture d ecran_778

  1. The password update doesn't work
    After an update, it's still the same value in database

@TristanLDD I am not sure the behavior and design of the birthday field is ok
The datepicker appears only if I click on the down arrow

capture d ecran_779

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Dec 17, 2018

hey @marionf actually, ill make Birthday input the same as it was in legacy, like this:

bday_input

however, what are the rules for birthday? can user enter just a day? or can you enter month and year but leave out day?

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Dec 17, 2018

Maybe a datepicker is ok, let's wait for @TristanLDD answer
Currently the behavior in legacy is that all fields must be completed, otherwise it's not saved.

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Dec 17, 2018

ok, but what i dont like about datepicker is that it allows you to select date in the future. :/

@TristanLDD

This comment has been minimized.

Copy link

TristanLDD commented Dec 18, 2018

@sarjon I'm ok to use the legacy behavior for birth date selection, birth date should be the only case of not using a date picker because as you said date pickers allow to set a date in the future.

Also it would be nice to add placeholders to the legacy dropdowns and fill with "Year" - "Month" " "Day" instead of "-"

@sarjon

This comment has been minimized.

Copy link
Member Author

sarjon commented Dec 19, 2018

@TristanLDD here's how it looks now:

bd

is it ok? also, is error displayed correctly?

@sarjon sarjon force-pushed the sarjon:migrate/customer-crud branch from 937e2a0 to b9f9342 Jan 4, 2019

@matks

matks approved these changes Jan 5, 2019

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 5, 2019

@sarjon I fixed the tests. Travis should be fine now. This required to modify the following expected outputs:

Can you tell me if this is intended, or you really expected a DateTime (then it is the implementation that needed a fix, not the test) ?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 6, 2019

Thanks @sarjon !

@matks matks merged commit db7fc69 into PrestaShop:develop Jan 6, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment