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

Move role configuration into Spree::Config #2374

Merged

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Nov 11, 2017

Previously, RoleConfiguration was a singleton class, which was configured by engine initialization.

Instead, we can configure it like the rest of our configuration by moving it to AppConfiguration (and therefore Spree:Config). It is also no longer a Singleton class, which makes testing it simpler.

This deprecates RoleConfiguration.instance and RoleConfiguration.configure and has them use Spree::Config.roles instead

This uses the ClassConstantizer::Set when storing the mapping of role
names to permission sets. In development, this allows those classes to
be reloaded if changes are made to a role defined inside the
application.
Previously, RoleConfiguration was a singleton class, which was
configured by engine initialization.

Instead, we can configure it like the rest of our configuration by
moving it to AppConfiguration (and therefore Spree:Config). It is also
no longer a Singleton class, which makes testing it simpler.

This deprecates RoleConfiguration.instance and
RoleConfiguration.configure and has them use Spree::Config.roles
instead.
@jhawthorn jhawthorn requested a review from cbrunsdon November 11, 2017 02:15
@jhawthorn jhawthorn changed the title [WIP] Move role configuration into Spree::Config Move role configuration into Spree::Config Nov 14, 2017
Copy link
Contributor

@cbrunsdon cbrunsdon left a comment

Choose a reason for hiding this comment

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

Anything that kills a rails initializer from doing work is good by me, but this is also a good change!

👍

Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

LGTM, nice work!

@gmacdougall gmacdougall merged commit 4af4633 into solidusio:master Nov 22, 2017
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.

4 participants