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.

Sign up for GitHub

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

Restore faker for development environments #11786

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Restore faker for development environments #11786

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
737fb6f
fix user mailer by adding faker to gemfile development, test group
eileen-nava Jan 22, 2025
647e7dd
Changelog: Internal, In-person Proofing, restore user mailer function…
eileen-nava Jan 22, 2025
bb3db5c
alphabetize gemfile development/test group
eileen-nava Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ group :development, :test do
gem 'bullet', '~> 7.0'
gem 'capybara-webmock', git: 'https://github.com/hashrocket/capybara-webmock.git', ref: 'd3f3b7c'
gem 'erb_lint', '~> 0.7.0', require: false
gem 'faker'
Copy link
Contributor

@zachmargolis zachmargolis Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

let's add an explicit require in the mailer preview too? or maybe a comment here noting why it's here?

Suggested change
gem 'faker'
gem 'faker', require: false # used in mailer previews

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the motivation making the bundle smaller, so that performance is improved? Or ensuring that faker doesn't mistakenly get removed from development in the future?
I'm not against the change, just want to be sure I understand the benefit.

Copy link
Contributor

Choose a reason for hiding this comment

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

the comment is to make sure it doesn't get removed in the future

the require: false is because we don't strictly need that in memory for our "normal" production flow so this just helps keep things small and explicit

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm misunderstanding something, moving from :test to the :development, :test group shouldn't impact production?

Copy link
Member

Choose a reason for hiding this comment

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

I think the idea is to avoid auto-requiring the gem via Bundler when running the application locally, which wouldn't have happened previously but will happen if it's moved into the development group.

identity-idp/config/application.rb

Line 23 in c1cda40

Bundler.require(*Rails.groups) 

$ rails c
[1] pry(main)> Rails.groups
=> [:default, "development"]

gem 'i18n-tasks', '~> 1.0'
gem 'knapsack'
gem 'listen'
Expand All @@ -128,7 +129,6 @@ end
group :test do
gem 'axe-core-rspec', '~> 4.2'
gem 'bundler-audit', require: false
gem 'faker'
gem 'simplecov', '~> 0.22.0', require: false
gem 'simplecov-cobertura'
gem 'simplecov_json_formatter'
Expand Down