-
-
Notifications
You must be signed in to change notification settings - Fork 516
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
Add reporting category column and backfill for items #5034
base: main
Are you sure you want to change the base?
Add reporting category column and backfill for items #5034
Conversation
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.
One question on the enum.
app/models/item.rb
Outdated
@@ -103,6 +104,17 @@ class Item < ApplicationRecord | |||
} | |||
|
|||
before_destroy :validate_destroy, prepend: true | |||
before_create :set_reporting_category | |||
|
|||
enum :reporting_category, [ |
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.
Do we want to store the full string with spaces and capitals in the database? I feel like it'd be easier to work with if the DB value was a more "computerized" string. E.g. it makes it easier to understand the built-in methods like item.adult_incontinence?
.
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.
Yea I was trying to make the string value stored human readable, which probably isn't the best approach. I refactored it to us an enum with integer values. This means that we will have to transform the reporting categories to be human readable (eg. use titleize
to change "adult_incontinence" to "Adult Incontinence"). But this seems like a better separation of concerns. And we can easily add Spanish translations if needed.
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.
Ok I had to name the new enums a little odd. Because otherwise the old adult_incontinence
scope would clash with the adult_incontinence
class method created from the enum.
app/models/item.rb
Outdated
# TODO: The enum names below have _enum appended in order to not clash with | ||
# the scopes above. Remove _enum when the scopes above can be safely removed. | ||
enum :reporting_category, { | ||
adult_incontinence_enum: 0, |
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.
Should be straightforward to remove the "_enum"'s appended here. Because the db is storing the integer values, so we won't need to do another db migration, just change the names here.
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.
You can still store string values - you can just make them the underscores instead of the full display names.
If scopes are an issue, you can just set scopes: false
to not create the scopes. We can turn them back on later.
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.
Thanks, didn't know not creating those methods was an option. fixed in my last commit 420fb98
app/models/item.rb
Outdated
# TODO: The enum names below have _enum appended in order to not clash with | ||
# the scopes above. Remove _enum when the scopes above can be safely removed. | ||
enum :reporting_category, { | ||
adult_incontinence_enum: 0, |
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.
You can still store string values - you can just make them the underscores instead of the full display names.
If scopes are an issue, you can just set scopes: false
to not create the scopes. We can turn them back on later.
Resolves #5009
Description
Adds reporting category column and backfill the data.
Type of change
How Has This Been Tested?
Manually tested the migrations.