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

Remove CanCanCan custom actions aliases (cont.) #3701

Conversation

filippoliverani
Copy link
Contributor

@filippoliverani filippoliverani commented Jul 10, 2020

Description
This PR is a continuation of #3148

Current Solidus' CanCanCan custom action aliases are not needed anymore and can make maintaining permissions and upgrading to new versions of CanCanCan much harder.

This PR:

  • replaces internal usage of legacy custom aliases with CanCanCan default ones
  • deprecates legacy custom aliases usage
  • adds a compatibility layer to avoid breaking changes

The new use_custom_cancancan_actions preference can be set to false to ignore legacy custom aliases and to apply CanCanCan default aliases.
If use_custom_cancancan_actions is set to true (default value for existing stores):

  • legacy custom action alias are translated to CanCanCan default aliases
  • a deprecation warning is raised every time a legacy custom alias is used
  • a warning is raised every time :read alias is used since its behavior had been customized (:show, :to => :read) and it's different from default CanCanCan behavior (index, :show, :to => :read).

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • [ ] I have updated Guides and README accordingly to this change (if needed)
  • [ ] I have added tests to cover this change (if needed)
  • [ ] I have attached screenshots to this PR for visual changes (if needed)

@filippoliverani filippoliverani marked this pull request as draft July 10, 2020 15:40
@filippoliverani filippoliverani force-pushed the filippoliverani/remove_cancancan_customizations_cont branch 4 times, most recently from dfa025b to 7f37bed Compare July 17, 2020 13:25
@filippoliverani filippoliverani marked this pull request as ready for review July 17, 2020 14:15
@filippoliverani filippoliverani force-pushed the filippoliverani/remove_cancancan_customizations_cont branch from 7f37bed to 2f94238 Compare July 31, 2020 12:51
Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

@filippoliverani thank you 👍

Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

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

@filippoliverani thanks! Just left one small comment about a typo. I'm also wondering if we should add a deprecation warning for accessible_by calls?

core/lib/spree/app_configuration.rb Outdated Show resolved Hide resolved
@filippoliverani filippoliverani force-pushed the filippoliverani/remove_cancancan_customizations_cont branch from 2f94238 to 2bac604 Compare August 21, 2020 14:12
@filippoliverani filippoliverani force-pushed the filippoliverani/remove_cancancan_customizations_cont branch from 2bac604 to d114b81 Compare August 21, 2020 14:20
@filippoliverani
Copy link
Contributor Author

filippoliverani commented Aug 21, 2020

@filippoliverani thanks! Just left one small comment about a typo. I'm also wondering if we should add a deprecation warning for accessible_by calls?

Right, accessible_by calls using legacy aliases should be deprecated too, I'm pushing a new commit for that, thanks 👍

Legacy custom action aliases are translated to CanCanCan default aliases.
A deprecation warning is raised every time a legacy custom alias is
used.
A warning is raised every time :read alias is used since its behavior
had been customized (:show, :to => :read) and it's different from
default CanCanCan behavior (index, :show, :to => :read).
@filippoliverani filippoliverani force-pushed the filippoliverani/remove_cancancan_customizations_cont branch from d114b81 to c92e13a Compare August 21, 2020 14:46
@kennyadsl kennyadsl mentioned this pull request Aug 28, 2020
4 tasks
@kennyadsl kennyadsl added this to the 2.11 milestone Aug 28, 2020
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Great change @filippoliverani! @coorasse do you have some time to leave your opinion here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants