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

Fix all miswritten getAdminLink calls in templates #11531

Merged
merged 20 commits into from Nov 29, 2018

Conversation

jolelievre
Copy link
Contributor

@jolelievre jolelievre commented Nov 27, 2018

Questions Answers
Branch? 1.7.5.x
Description? There are a lot of templates where getAdminLink is miswritten, it uses only the controller as a parameter and appends manually the parameter afterwards. This PR fixes every templates so that they are correctly written using full getAdminLink parameters. The same work will have to be done in the controller classes. It especially fixes the edition of module position, but lots of other links have been improved as well.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #11485
How to test? The most important test is to test module position edition. But lots of other links have been modified and should be tested. The full list is after:
  • Test edit position
  • Test header:
    • Test Add quick access
    • Test Profile avatar link (right click the user avatar -> Open link) (on legacy page)
    • Test Profile (in submenu -> click on "Your profile") (on legacy page AND migrated pages)
    • Test Logout (in submenu -> click on "Logout") (on legacy page AND migrated pages)
    • Test in notifications orders when empty, one of the three random texts contains a link to "abandonned carts"
  • Test Carriers list click on the row links to the edition
  • Test Cart view
    • Link to Customer on his name
    • Link to order in "Order Information" (when order is present)
    • Link to Create order in "Order information" (when no order and cart has en customer assigned)
    • Link to Discount (when present)
  • Test Customer Service View
    • Modal to forward discussion
    • Buttons Mark as pending, Mark as handled, ...
    • Link to the customer on avatar
    • Link to the customer on his name
    • Form to reply a message
  • Test brand view
    • Test link to edit address
  • Test Order view
    • Test "View invoice", "View delivery slip" buttons
    • Test PDF documents links (delivery slip, invoice, order slip)
    • Test modal to edit shipping details
    • Test Customer service link (when messages are associated)
    • Test Previous/Next order buttons
    • In history test "Resend mail" button
    • Test change shipping address select form
  • Test Order list
    • Test PDF buttons (invoice, delievry slip)
  • Test Create order
    • Test search customer card result Details link
    • Test "Add new customer button"
    • After you choose a customer
      • In orders tab test Details link
      • Test the "Add new voucher" button
      • In addresses panel the two edit button
      • Test the "Add a new address button"
    • Finally create or use a cart, and test that the "Create the order" button works
  • Test Customers > Outstanding list invoice button
  • Test Multistore list "Click here to set a URL for this shop." link
  • Test Credit slips list -> "Download credit slip" button
  • Themes & Catalog
    • Test "Use this theme" link
    • Test "Choose layouts" button
    • Test "Reset to defaults" button
    • Test "Delete this theme" link
  • Customer addresses
    • Test import link
    • Test "Add new address" button
  • Attributes & Features
    • Test in list actions "ADD NEW ATTRIBUTES", and "IMPORT" link
    • Test in view list "ADD NEW VALUES" button and "Back to list" link
  • Shipping > Carriers
    • Test link on rows, edit links, delete links

This change is Reviewable

@prestonBot prestonBot added 1.7.5.x Branch Bug Type: Bug labels Nov 27, 2018
@jolelievre jolelievre added this to the 1.7.5.0 milestone Nov 27, 2018
'editGraft': 1
} %}
{% if selectedModule %}
{% set linkParams = liParams|merge({'show_modules': selectedModule}) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

linkParams and not LiParams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1966,7 +1967,7 @@ private function getNotificationTip($type)
array(
'[1]' => '<strong>',
'[/1]' => '</strong>',
'[2]' => '<a href="' . $this->context->link->getAdminLink('AdminCarts') . '&action=filterOnlyAbandonedCarts">',
'[2]' => '<a href="' . $this->context->link->getAdminLink('AdminCarts', true, array(), array('action' => 'filterOnlyAbandonedCarts')) . '">',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why array() and not [] like above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this class uses the array() notation everywhere so I prefer to write it this way

@Quetzacoalt91
Copy link
Member

Nothing to add regarding the PR content. However, if you had to update all URLs generated, you should probably warn the module developers to do the same thing in https://devdocs.prestashop.com/1.7/modules/core_updates/1.7.5/

If their module cannot reach specific pages of the back office and they are not aware of it, this is bad.

@jolelievre
Copy link
Contributor Author

@Quetzacoalt91 I mentioned it in the routing section https://devdocs.prestashop.com/1.7/development/architecture/migration-guide/controller-routing/#more-about-the-legacy-link-property
Do you think it's worth repeating it the the 1.7.5 changes? with a link to the routing page maybe?

@Quetzacoalt91
Copy link
Member

Nice to see you already wrote something about this. But yes, the link I gave you is mentioned in the Build article, to help contributors knowing what are the potential regression brought by 1.7.5.0. If we can add a link to the other page, that'd be great.

@jolelievre
Copy link
Contributor Author

@Quetzacoalt91 here is the update for dev doc: PrestaShop/docs#168

@Quetzacoalt91 Quetzacoalt91 added the Waiting for QA Status: action required, waiting for test feedback label Nov 28, 2018
@marionf marionf self-assigned this Nov 28, 2018
@marionf marionf added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Nov 29, 2018
@marionf marionf removed their assignment Nov 29, 2018
@Quetzacoalt91 Quetzacoalt91 merged commit bbff868 into PrestaShop:1.7.5.x Nov 29, 2018
@Quetzacoalt91
Copy link
Member

Thank you @jolelievre

@jolelievre jolelievre deleted the module-position branch July 12, 2019 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.5.x Branch Bug Type: Bug QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants