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

Refacto ProductPreferencesController to use annotations #12076

Merged
merged 2 commits into from Jan 22, 2019

Conversation

@matks
Copy link
Contributor

matks commented Jan 8, 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? Symfony page "Configure > Shop Parameters > Product Settings" should behave like before. Access rules updated: you could access the page previously if you had read, create, update or delete rights. Now only read right grants you access to display the page.

This change is Reviewable

}
/**
* Process product preferences form.
*
* @AdminSecurity("is_granted(['update', 'create','delete'], request.get('_legacy_controller'))",
* message="You do not have permission to update this.",

This comment has been minimized.

@PierreRambaud

PierreRambaud Jan 8, 2019

Contributor

@colinegin should we update the wording to a generic one.
You do not have permission to perform this action.

This comment has been minimized.

@matks

matks Jan 8, 2019

Author Contributor

ping @LouiseBonnard too
If we add a new string into Admin.Notifications.Error then we can use it here 👍

This comment has been minimized.

@colinegin

colinegin Jan 9, 2019

I agree with you @PierreRambaud, but let's ask to Louise, she's the master !

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Jan 9, 2019

Contributor

Ahoy, sailors, captain speaking! Nice, thanks, I have listed here three possibilities:

  • You do not have permission to perform this action.
  • You need permission to perform this action.
  • You are not allowed to perform this action.

@TristanLDD, I think the second one is a bit more positive. Do you agree? @colinegin, is it possible at this point to grant the user the rights to perform it?

This comment has been minimized.

@colinegin

colinegin Jan 9, 2019

no idea... @matks @PierreRambaud ?

If yes, I prefer the 2nd option as well !

This comment has been minimized.

@PierreRambaud

PierreRambaud Jan 10, 2019

Contributor

2nd option too!

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Jan 10, 2019

Contributor

Alright, thanks for the feedback. But if we choose this wording, we need to make sure that the user can get this permission otherwise we will induce wrong behavior.

This comment has been minimized.

@LouiseBonnard

LouiseBonnard Jan 11, 2019

Contributor

Well, it seems permissions can be set in Advanced Parameters > Team to manage the 'Product Settings' page of the back office. Let's pick the second option then, @matks: You need permission to perform this action. in Admin.Notifications.Error.

This comment has been minimized.

@PierreRambaud

PierreRambaud Jan 21, 2019

Contributor

 @matks Wording needs to be updated :)

This comment has been minimized.

@matks

matks Jan 21, 2019

Author Contributor

@PierreRambaud We're considering the idea of carrying out this in another PR after having decided new global rules in #12235

@matks matks force-pushed the matks:refacto-ProductPreferencesController branch from 3df7147 to 8e28b58 Jan 8, 2019

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Jan 17, 2019

Let's pick the second option then, @matks: You need permission to perform this action. in Admin.Notifications.Error.

As I agree on this improvement, I think this should be done in another pull request (with an update on all the impacted pages).

So, I'm gonna validate this, thanks for your understanding 👍

ping @matks, would you mind to do the pull request to apply this new message to all impacted actions? or at least create a minimal issue so we don't forget it? Thanks!

@matks

This comment has been minimized.

Copy link
Contributor Author

matks commented Jan 19, 2019

PR updated for the access rules only.

@mickaelandrieu @LouiseBonnard you mean you wish to update all backoffice pages with this error message, to be used whenever someone tries to modify some settings although he does not have the rights to do it ?

@LouiseBonnard

This comment has been minimized.

Copy link
Contributor

LouiseBonnard commented Jan 21, 2019

@matks, I don't know how we can expand it throughout PrestaShop but actually, this wording appears to be better since it is more positive for the users. We could start here and apply to all situations gradually, or wait a bit and modify all occurrences in one go? You let me know what is best!

@matks

This comment has been minimized.

Copy link
Contributor Author

matks commented Jan 21, 2019

@LouiseBonnard I created an issue dedicated to the topic 😉 so we can continue the exploration there : #12235

@mbadrani mbadrani self-assigned this Jan 21, 2019

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

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

@PierreRambaud PierreRambaud merged commit 28f6eae 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

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