-
-
Notifications
You must be signed in to change notification settings - Fork 469
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
feat: makes changes to accomodate removing seeding #4220
feat: makes changes to accomodate removing seeding #4220
Conversation
@@ -26,7 +26,7 @@ class Kit < ApplicationRecord | |||
scope :by_partner_key, ->(key) { joins(:items).where(items: { partner_key: key }) } | |||
scope :by_name, ->(name) { where("name ILIKE ?", "%#{name}%") } | |||
|
|||
validates :organization, :name, presence: true | |||
validates :name, presence: true |
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.
It belong_to
an org, validation feels unneseccary.
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.
belongs_to
used to imply optional - that switched to mandatory by default, so yep, a validation is redundant.
ef7303c
to
aa72dd5
Compare
Makes changes to factories, models, and rails helper needed to allow removal of seeding. Adds RSpec meta tag seed_items when set to false will disable seeding of items on a per test basis. ENV flag SKIP_SEED=true will prevent rspec seeding before the suite runs.
aa72dd5
to
f056d90
Compare
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.
Like the approach. I do think the comments should be stripped before merging - feel free to open some additional PRs with some function or variable renames. (You can probably put them in a single PR.)
@@ -26,7 +26,7 @@ class Kit < ApplicationRecord | |||
scope :by_partner_key, ->(key) { joins(:items).where(items: { partner_key: key }) } | |||
scope :by_name, ->(name) { where("name ILIKE ?", "%#{name}%") } | |||
|
|||
validates :organization, :name, presence: true | |||
validates :name, presence: true |
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.
belongs_to
used to imply optional - that switched to mandatory by default, so yep, a validation is redundant.
@elasticspoon all looks good to me! Some conflicts though |
@dorner fixed (failure is a flake i'm pretty sure) |
Nice! Thank you! |
Related to #4199
Makes changes to factories, models, and rails helper needed to allow removal of seeding.
Adds RSpec meta tag
seed_items
when set tofalse
will disable seeding of items on a per test basis.ENV
flagSKIP_SEED=true
will prevent rspec seeding before the suite runs.Note
I added some comments in places because I found stuff confusing. Those can be removed in the final PR but most of them felt like stuff worth bringing up.
Additional note: the speedup was actually much less than I expected, across all the models I saw only a 5s speedup, from 29s to 25s
Type of change
How Has This Been Tested?
Passes all tests