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

Refactor Shipping/ referencesController to use annotations #12098

Merged
merged 2 commits into from Jan 22, 2019

Conversation

Projects
None yet
5 participants
@matks
Copy link
Contributor

matks commented Jan 9, 2019

Questions Answers
Branch? develop
Description? Refactor Controller to use latest conventions about annotations
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? Page "Improve > Shipping > Preferences" access rules are updated (see below)

Access rules changes:

  • now (with the PR) you need read right to see the page
  • now, you need either create, update or delete right to modify the page

This change is Reviewable

@@ -42,7 +41,7 @@ class PreferencesController extends FrameworkBundleAdminController
*
* @param Request $request
*
* @Template("@PrestaShop/Admin/Improve/Shipping/Preferences/preferences.html.twig")
* @AdminSecurity("is_granted(['read', 'update', 'create', 'delete'], request.get('_legacy_controller'))")

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jan 10, 2019

Member

Isn't more logical to only ask for READ, especially if the test is: read OR update OR create OR delete?

This comment has been minimized.

@matks

matks Jan 10, 2019

Author Contributor

Bigger issue is #12109
But the short answer: on the BO, I think if you have been given the right to create something, update it or delete it, then you should be able to see it displayed.

If everybody was very rigorous about how they grant accesses, we could restrict it to "read" only. I'm handling the "write but no read" rule here 😅

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jan 15, 2019

Member

(Wrong button chosen)

I've been looking in other controller, and for instance in BackupController & EmployeeController, only the "read" permission is requested, which makes sense in my opinion.

What do you think about updating this line?

On hold by one question

@@ -42,7 +41,7 @@ class PreferencesController extends FrameworkBundleAdminController
*
* @param Request $request
*
* @Template("@PrestaShop/Admin/Improve/Shipping/Preferences/preferences.html.twig")
* @AdminSecurity("is_granted(['read', 'update', 'create', 'delete'], request.get('_legacy_controller'))")

This comment has been minimized.

@Quetzacoalt91

Quetzacoalt91 Jan 15, 2019

Member

(Wrong button chosen)

I've been looking in other controller, and for instance in BackupController & EmployeeController, only the "read" permission is requested, which makes sense in my opinion.

What do you think about updating this line?

@matks

This comment has been minimized.

Copy link
Contributor Author

matks commented Jan 19, 2019

@Quetzacoalt91 PR updated to follow access rules

Requested changes have been applied

@mbadrani mbadrani self-assigned this Jan 21, 2019

@mbadrani mbadrani added QA ✔️ and removed waiting for QA labels Jan 21, 2019

@PierreRambaud PierreRambaud merged commit 8cc9f06 into PrestaShop:develop Jan 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Jan 22, 2019

Thanks @matks

@PierreRambaud PierreRambaud added this to the 1.7.6.0 milestone Jan 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment