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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seed org with available authorizations #1978

Merged
merged 2 commits into from
Oct 5, 2017

Conversation

deivid-rodriguez
Copy link
Contributor

馃帺 What? Why?

I'm doing a lot of manual testing with authorizations. Right now this is a bit tedious because the default organization is not seeded with the available handlers, so you have to go to /system, enable them, then go to admin and enable handlers for the actions, and then you can finally test the handler. This changes improve this by adding the available authorizations to the organization when seeding.

Since the "demo generator" (where the dummy authorization handler is setup) is run after seeding, I couldn't easily fix this by just adding it to seeds. So I decided to merge the demo generator with the main generator so by the time of seeding authorization handlers have been correctly setup. Instead, you can now pass --demo to the generator if you want a demo handler (authorizes ID documents ended with "X"), or the default (an example handler with documentation that by default just authorizes everything).

I think this is a good move anyways because:

  • Even if we remove the ability to run bin/rails g decidim:demo on an existing application, running it can lead to crashes if there is existing authorization data in DB.
  • I think this "demo" generator was only meant to be run on brand new applications.
  • Makes our application generator faster since we don't need to reload the application to run the demo generator.

馃搶 Related Issues

None.

馃搵 Subtasks

None.

馃摲 Screenshots (optional)

None.

馃懟 GIF

hands up

@ghost ghost assigned deivid-rodriguez Oct 4, 2017
@ghost ghost added the in-progress label Oct 4, 2017
@deivid-rodriguez deivid-rodriguez force-pushed the feature/seed_org_with_available_authorizations branch from b7b1465 to fd51e73 Compare October 4, 2017 10:46
@codecov
Copy link

codecov bot commented Oct 4, 2017

Codecov Report

Merging #1978 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1978      +/-   ##
==========================================
- Coverage   98.48%   98.47%   -0.01%     
==========================================
  Files        1145     1144       -1     
  Lines       25737    25722      -15     
==========================================
- Hits        25346    25331      -15     
  Misses        391      391

@deivid-rodriguez deivid-rodriguez force-pushed the feature/seed_org_with_available_authorizations branch from fd51e73 to bfade43 Compare October 4, 2017 10:58
mrcasals
mrcasals previously approved these changes Oct 4, 2017
Copy link
Contributor

@mrcasals mrcasals left a comment

Choose a reason for hiding this comment

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

Looks good to me! @josepjaume what do you think?

josepjaume
josepjaume previously approved these changes Oct 5, 2017
Copy link
Contributor

@josepjaume josepjaume left a comment

Choose a reason for hiding this comment

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

Looks good to me as well! Good job!

Maybe we should add some documentation about the available generators at some point? I'm sure this would have popped out before if we had tried to rationalize it on a README :)

@mrcasals
Copy link
Contributor

mrcasals commented Oct 5, 2017

Should we add a CHANGELOG entry here?

Transforming an existing application like this is very prone to errors
and not that simple. For example, if there are existing authorization
handlers already saved in DB (in either organizations or
authorizations), they would need to be migrated or we would get crashes
all around.

So the current version of this generator is only ready to be used for
direct app generator. So I merged it with the regular generator and made
it accessible by passing the --demo flag to it.
@deivid-rodriguez deivid-rodriguez force-pushed the feature/seed_org_with_available_authorizations branch from bfade43 to f399e1e Compare October 5, 2017 11:03
@mrcasals mrcasals merged commit fa23185 into master Oct 5, 2017
@mrcasals mrcasals deleted the feature/seed_org_with_available_authorizations branch October 5, 2017 11:44
@ghost ghost removed the in-progress label Oct 5, 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.

None yet

3 participants