-
-
Notifications
You must be signed in to change notification settings - Fork 374
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
Support Rails 8 authentication generator #519
base: main
Are you sure you want to change the base?
Support Rails 8 authentication generator #519
Conversation
0bf774d
to
75cf803
Compare
rspec/rspec-rails#2811 was merged so this PR is ready for review. |
75cf803
to
aa219dc
Compare
Thanks for your work on this (and rspec-rails) @jeromedalbert! I ran into these issues while spinning up a new Rails 8 app. Do we know what’s preventing it from being merged? I see one approval, but @neilvcarvalho, I see you added yourself as a reviewer recently. Thanks! |
Hi @brentdodell and @jeromedalbert! I'm going to review this PR (and other Rails 8 related work) this Friday, and possibly create a new release already. |
- This should be merged by end of week (thoughtbot/factory_bot_rails#519 (comment))
module Generators | ||
class AuthenticationGenerator < Base | ||
def create_fixture_file | ||
template 'users.rb', 'test/factories/users.rb' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think test/factories/users.rb
should be static here. It doesn't allow overriding things like the factory_bot directory, suffix, etc.
I think you'll need to include options, and build up the name dynamically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing that the model generator does is checking if you already have a factory file, such as a global factories.rb
instead of a factories
directory. If you do, it appends the factory
block to this file, instead of creating a new file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the comments made by @brentdodell and me, could you also add tests? We currently have a Cucumber feature test for that on features/generators.feature
module Generators | ||
class AuthenticationGenerator < Base | ||
def create_fixture_file | ||
template 'users.rb', 'test/factories/users.rb' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thing that the model generator does is checking if you already have a factory file, such as a global factories.rb
instead of a factories
directory. If you do, it appends the factory
block to this file, instead of creating a new file.
aa219dc
to
ed8b4cf
Compare
ed8b4cf
to
e526fcc
Compare
@neilvcarvalho rspec-rails does not currently have a released version containing the Rails 8 authentication generator; it's been on their However the new test fails on my local. The error I get seems to be EDIT: It looks like feature specs are passing in CI, so I guess it's only an issue on my local. |
e526fcc
to
ada4732
Compare
Follow-up to rspec/rspec-rails#2811.
Problem
When running
bin/rails generate authentication
for a Rails 8 app that has therspec-rails
gem, generation ends with the following error:error factory_bot [not found]
.This is because
rspec-rails
'AuthenticationGenerator
containshook_for :fixture_replacement
, which callsfactory_bot_rails
, but it errors out sincefactory_bot_rails
does not have an authentication generator.Solution
Add authentication generator that creates a user factory with the
email_address
andpassword_digest
fields. These fields come from the Rails authentication generator.