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

Migration of Sell > Customers > Addresses page #15229

Merged
merged 12 commits into from Nov 14, 2019

Conversation

@RaimondasSapola
Copy link
Contributor

RaimondasSapola commented Aug 23, 2019

Questions Answers
Branch? develop
Description? Migrated Sell > Customers > Addresses page grid page, filters action and required fields form
Type? refacto
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #10557
How to test? go to Sell > Customers > Addresses. Page should be as it was before the PR (actually since the link is not enabled yet go to /sell/addresses)

This change is Reviewable

@RaimondasSapola RaimondasSapola requested a review from PrestaShop/prestashop-core-developers as a code owner Aug 23, 2019
@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Aug 23, 2019

Hello @RaimondasSapola!

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

@LouiseBonnard

This comment has been minimized.

Copy link
Contributor

LouiseBonnard commented Aug 26, 2019

Hi there, one more thing about this page... when you want to add a new address, the very first field of the form appears with the wrong UI. Can this be corrected in this PR?

Capture d’écran 2019-08-26 à 10 42 18

@RaimondasSapola

This comment has been minimized.

Copy link
Contributor Author

RaimondasSapola commented Aug 26, 2019

Wrong UI will be fixed in Sell > Customers > Addresses Create/Edit form PR

throw new InvalidAddressRequiredFieldsException(
sprintf(
'Invalid address required fields provided. Allowed fields are: %s',
implode(',', RequiredFields::ALLOWED_REQUIRED_FIELDS)

This comment has been minimized.

Copy link
@matks

matks Sep 26, 2019

Contributor

@PierreRambaud I know you've played with this part recently 😛 do you think this is a bad thing in the context of shop customization ? This prevents a module developer to add custom fields and make them required, is that an issue ?

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Sep 27, 2019

Contributor

Yes it's an issue, you'll not be able to add custom field that are only manage by hooks :/

This comment has been minimized.

Copy link
@RaimondasSapola

RaimondasSapola Sep 30, 2019

Author Contributor

There was no explicit hook for required fields so how should we implement this in migration? And is this an issue for this PR @PierreRambaud @matks ?

This comment has been minimized.

Copy link
@matks

matks Sep 30, 2019

Contributor

@RaimondasSapola At controller level, modules can add new required fields (using existing hooks). This looks like a relevant usecase so I think we need to address it 🤔

This comment has been minimized.

Copy link
@RaimondasSapola

RaimondasSapola Sep 30, 2019

Author Contributor

Ok i am a bit confused. So the problem here is that module developers cannot add new required fields after migration and we should look into implementing hooks in for requiredFields migration or is it that this validation only allows predefined required fields to be added and module developers might want to add completely new fields?

This comment has been minimized.

Copy link
@matks

matks Sep 30, 2019

Contributor

@RaimondasSapola Both 😄

So the problem here is that module developers cannot add new required fields after migration

Yes because this validation only allows predefined required fields to be added

we should look into implementing hooks in for requiredFields migration

Since this is controlled on controller level and module can override controllers using Symfony re-routing I think it's already OK but I'll check. Leave this to me 👍

module developers might want to add completely new fields?

Yes, this is a 100% relevant usecase. Module developers want to add new fields in the database and want to be able to:

  • read them
  • list them in grids
  • modify them in add/edit forms
  • make them required fields

This comment has been minimized.

Copy link
@RaimondasSapola

RaimondasSapola Oct 1, 2019

Author Contributor

So in this case validation should be removed altogether?

Copy link
Contributor

matks left a comment

@RaimondasSapola sorry for late review 😢

Thanks to @sarjon 1st review I have nothing to say about the code, it's clean
I just have 2 questions, see below 😉

@RaimondasSapola RaimondasSapola force-pushed the RaimondasSapola:M/Customers/Addresses branch from ee8902a to eff7803 Sep 30, 2019
@matks matks mentioned this pull request Oct 9, 2019
1 of 31 tasks complete
@RaimondasSapola RaimondasSapola force-pushed the RaimondasSapola:M/Customers/Addresses branch from accb0d8 to 4142eea Oct 25, 2019
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Nov 12, 2019

Hi @RaimondasSapola !

I've an issue on this PR :

When an address is soft deleted, it still appears on the list.

See screen record : Addresses ID 1 and 2 are hard deleted (since no order linked to these addresses), while ID 5 is soft deleted. After deletion, address with ID 5 is still shown on the list.

https://drive.google.com/open?id=1RpUboPhaeVUbaD_BQ29FUzuafKrx9lCT

Thanks 😄

@RaimondasSapola RaimondasSapola force-pushed the RaimondasSapola:M/Customers/Addresses branch from cdcf78f to 8175214 Nov 14, 2019
@RaimondasSapola

This comment has been minimized.

Copy link
Contributor Author

RaimondasSapola commented Nov 14, 2019

@Robin-Fischer-PS added fix :)

return;
}
//TODO add dynamic field validation

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 14, 2019

Contributor

This can't be done now? I guess it is related toRequiredFields::ALLOWED_REQUIRED_FIELDS?

This comment has been minimized.

Copy link
@RaimondasSapola

RaimondasSapola Nov 14, 2019

Author Contributor

It is decided to do in a separate PR

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 14, 2019

Contributor

Ok, I was gonna propose to add this in the issue but it's already done 😉

(new Filter('actions', SearchAndResetType::class))
->setAssociatedColumn('actions')
->setTypeOptions([
'reset_route' => 'admin_common_reset_search',

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 14, 2019

Contributor

Shouldn't you use the filterId feature rather, the filters matched by controller/action are a bit outdated since they have limitations (even though it works fine for One grid pages)

This comment has been minimized.

Copy link
@RaimondasSapola

RaimondasSapola Nov 14, 2019

Author Contributor

Yeh it's done. Sometimes it becomes difficult to follow witch method is the most recent and the one to use

This comment has been minimized.

Copy link
@jolelievre

jolelievre Nov 14, 2019

Contributor

Sure, there are many things to remember And we lack documentation as well I planned to add som about this filtering feature as soon as I have time ^^

Copy link
Contributor

jolelievre left a comment

Thank you @RaimondasSapola

@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Nov 14, 2019

Thanks @RaimondasSapola ! It's ✔️ for me :)

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Nov 14, 2019

Thank you @RaimondasSapola

@jolelievre jolelievre merged commit e136975 into PrestaShop:develop Nov 14, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@Progi1984 Progi1984 added this to the 1.7.7.0 milestone Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.