Fixes for custom user generator #2382
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The former usage of `Rails::Generators::Migration` forced us to write our own `next_migration_number` generator. This generator was erroneous and sometimes lead to duplicated migration versions. Using the activerecord migration generator uses its own battle proven migration number finder.
This include exists since this generator was introduced but no method is ever called on that module.
Any public method is considered a generator step. These methods are only used in other methdos or templates.
Instead of implementing our own template source path finder, we should just use the Rails' generators default way of defining the template source root path.
We used to do this before as one can see on the template that is no unused. Instead we gsub the initializer file and set the new custom user class name as the `Spree.user_class`.
mamhoff
approved these changes
Nov 17, 2017
There was a problem hiding this comment.
Fantastic work, thank you!
This also fixes the duplicate migration issue reported here: AlchemyCMS/alchemy-solidus#14 (comment)
Indeed! Nice find @mamhoff |
kennyadsl
approved these changes
Nov 18, 2017
cbrunsdon
approved these changes
Nov 22, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While building an install generator for alchemy-solidus
I came across some quirks in the current custom user generator.
First the migration generator does not use the active record migration generator but its super class.
This made is necessary to write our own
next_migration_numbergenerator. This implementation had someflaws that lead to duplicated migrations problems. We now use the active record migration generator and
do not need to ship our own custom migration number generator anymore.
Also the generator used to inject the spree initializer and set the
Spree.user_classto the generatedcustom user. Somehow this got lost. Replaced this by an gsub that ensures that the
Spree.user_classis never set twice.
This removes an unused module (
Rails::Generators::ResourceHelpers) that was used to deal with controller nameswhat we do not do anymore in this generator.
On the way I made some code cleanups.
Unfortunately it is not super easy to write generator specs as we do not have a dummy test app anymore.
We do not have tests for any generator actually. We should, but this is not purpose if this PR.
I hope the test suite of alchemy-solidus is proof enough
that this generator works. In its own generator we use the solidus custom user generator to setup the dummy app and test the integration
of Alchemy and Solidus. Would this generator fail, the builds of alchemy-solidus would fail.