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.

Sign up for GitHub

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

Jump to bottom

Modify adresses listing in Customer page to use Grid instead #20261

Merged
merged 6 commits into from Sep 24, 2020
Merged

Modify adresses listing in Customer page to use Grid instead #20261

merged 6 commits into from Sep 24, 2020

Conversation

sowbiba
Copy link
Contributor

@sowbiba sowbiba commented Jul 21, 2020 •
edited by matks

Questions Answers
Branch? develop
Description? Modify adresses listing in Customer page to use Grid instead. This enables use to add a confirmation modal when deleting a row from a grid.
Customers > View > Addresses.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Partially Fixes #17847
How to test? Go to Customers > View > Addresses in the BO, Try to delete a row, you will have a bootstrap modal to confirm deletion

This may have impact on automated tests @PrestaShop/qa-automation

This change is Reviewable

@sowbiba sowbiba added Improvement Type: Improvement develop Branch migration symfony migration project labels Jul 21, 2020
@sowbiba sowbiba added this to the 1.7.8.0 milestone Jul 21, 2020
@sowbiba sowbiba requested a review from a team as a code owner July 21, 2020 13:42
@sowbiba sowbiba self-assigned this Jul 21, 2020
@prestonBot prestonBot added the Waiting for wording Status: action required, waiting for wording label Jul 21, 2020
@sowbiba
Copy link
Contributor Author

sowbiba commented Jul 21, 2020

@boubkerbribri

Progi1984
Progi1984 reviewed Jul 21, 2020
View reviewed changes
Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question ;)

@sowbiba sowbiba force-pushed the delete-row-confirm-modal-customers-view-addresses branch from 1955a39 to 209a403 Compare July 21, 2020 16:03
@boubkerbribri
Copy link
Contributor

  1. @sowbiba , Yes there is an impact on automatic tests that we will fix, thanks.

  2. maybe you should fix this. There is 2 header titles on addresses grid.

image

@boubkerbribri
Copy link
Contributor

Just one more thing (maybe a regression) :
When I edit address from View customer page, browser shows addresses page instead of returning to view customer page.

@sowbiba
Copy link
Contributor Author

sowbiba commented Jul 22, 2020

  1. @sowbiba , Yes there is an impact on automatic tests that we will fix, thanks.
  2. maybe you should fix this. There is 2 header titles on addresses grid.

image

the bad news is the template is overriden by the linklist module https://github.com/PrestaShop/ps_linklist/blob/master/views/PrestaShop/Admin/Common/Grid/grid_panel.html.twig
Trying to understand why ...

@Junebyun Junebyun added Wording ✔️ Status: check done, wording approved and removed Waiting for wording Status: action required, waiting for wording labels Jul 24, 2020
@sowbiba sowbiba mentioned this pull request Jul 24, 2020
Closed
@sowbiba sowbiba force-pushed the delete-row-confirm-modal-customers-view-addresses branch from cbcf696 to 209a403 Compare July 25, 2020 14:22
@sowbiba
Copy link
Contributor Author

sowbiba commented Jul 25, 2020

For @PrestaShop/qa-automation @PrestaShop/qa-functional
This PR needs to be tested with PrestaShop/ps_linklist#101

atomiix
atomiix reviewed Jul 29, 2020
View reviewed changes
src/Core/Grid/Definition/Factory/CustomerAddressGridDefinitionFactory.php Outdated
*/
protected function getName()
{
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the name should be null? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was put there in order to avoid printing the grid name (see modifications in grid_panel.html.twig), in order to avoid rendering duplicate "Adresses" string

However I agree the PHP class (which contains the data) should, as much as possible, not be mixed with the template (which defines how it's rendered)

I would rather suggest

  • provide an option into Grid to disable the "title" name inside any Grid
  • override locally the grid to have this specific grid different from others

The idea being to keep into the View (as in MVC) whatever rendering logic we put, and keep the Grid (the Model) "consistent"

@atomiix
Copy link
Contributor

atomiix commented Jul 29, 2020

@sowbiba I think you can update GetCustomerForViewingHandler as you're getting addresses for there anymore?

Progi1984
Progi1984 previously approved these changes Aug 3, 2020
View reviewed changes
@Progi1984 Progi1984 requested a review from atomiix August 3, 2020 15:28
@matks matks changed the title Grid row delete confirmation modal - Customers > View > Addresses Modify adresses listing in Customer page to use Grid instead Aug 17, 2020
@prestonBot prestonBot added the Waiting for wording Status: action required, waiting for wording label Aug 17, 2020
@matks matks removed the Waiting for wording Status: action required, waiting for wording label Aug 17, 2020
matks
matks previously requested changes Aug 17, 2020
View reviewed changes
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sowbiba The PR is valid for with the exception of the grid name. Following @atomiix suggestion I think we should keep the data model consistent following It's just a view principle. I provided some suggestions about how to achieve that (naturally you can also suggest/implement another solution if you find something more suitable)

Do you have time to handle this small change ? Else I can do it

@sowbiba sowbiba force-pushed the delete-row-confirm-modal-customers-view-addresses branch from 209a403 to 0851849 Compare August 25, 2020 11:00
@matks
Copy link
Contributor

matks commented Sep 11, 2020

Code is valid (and approved) but sadly we got git conflicts 😞

@sowbiba sowbiba force-pushed the delete-row-confirm-modal-customers-view-addresses branch from 910f836 to f6389b6 Compare September 14, 2020 08:01
matks
matks previously approved these changes Sep 14, 2020
View reviewed changes
@matthieu-rolland
Copy link
Contributor

cs fixer is not happy, again :p

@sowbiba sowbiba dismissed stale reviews from matthieu-rolland and matks via b2ab886 September 14, 2020 09:21
@sowbiba sowbiba force-pushed the delete-row-confirm-modal-customers-view-addresses branch from b2ab886 to d4d9dd4 Compare September 14, 2020 09:21
@sowbiba sowbiba force-pushed the delete-row-confirm-modal-customers-view-addresses branch from 768d06a to 9ddb3b6 Compare September 14, 2020 11:24
@sowbiba sowbiba requested a review from matks September 14, 2020 13:33
@sowbiba sowbiba removed the Waiting for rebase Status: action required, waiting for rebase label Sep 14, 2020
@SD1982
Copy link
Contributor

SD1982 commented Sep 24, 2020

LGTM Thanks @sowbiba !!

@SD1982 SD1982 added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Sep 24, 2020
@Progi1984 Progi1984 merged commit d32dd73 into PrestaShop:develop Sep 24, 2020
@Progi1984
Copy link
Contributor

Thanks @sowbiba & @SD1982

Ping @PrestaShop/qa-automation

@boubkerbribri boubkerbribri mentioned this pull request Oct 12, 2020
Closed
@atomiix atomiix mentioned this pull request Oct 30, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthieu-rolland matthieu-rolland matthieu-rolland approved these changes

@matks matks matks approved these changes

@PierreRambaud PierreRambaud PierreRambaud left review comments

@Progi1984 Progi1984 Progi1984 left review comments

@atomiix atomiix Awaiting requested review from atomiix

Assignees

@sowbiba sowbiba

@SD1982 SD1982

Labels
develop Branch Improvement Type: Improvement migration symfony migration project QA ✔️ Status: check done, code approved Wording ✔️ Status: check done, wording approved
Projects
None yet
Milestone
1.7.8.0
Development

Successfully merging this pull request may close these issues.

Add a modal to confirm or cancel delete action for one row [1/1]
10 participants
@sowbiba @boubkerbribri @atomiix @Progi1984 @matks @matthieu-rolland @SD1982 @PierreRambaud @prestonBot @Junebyun