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.

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

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

Conversation

coalest
Copy link
Collaborator

@coalest coalest commented Feb 21, 2025

Resolves #5009

Description

Adds reporting category column and backfill the data.

Type of change

  • Internal

How Has This Been Tested?

Manually tested the migrations.

@cielf cielf requested a review from dorner February 21, 2025 18:12
Copy link
Collaborator

@dorner dorner left a 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.

@@ -103,6 +104,17 @@ class Item < ApplicationRecord
}

before_destroy :validate_destroy, prepend: true
before_create :set_reporting_category

enum :reporting_category, [
Copy link
Collaborator

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?.

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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.

# 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,
Copy link
Collaborator Author

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.

Copy link
Collaborator

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: falseto not create the scopes. We can turn them back on later.

Copy link
Collaborator Author

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

@coalest coalest requested a review from dorner February 25, 2025 13:19
# 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,
Copy link
Collaborator

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: falseto not create the scopes. We can turn them back on later.

@coalest coalest requested a review from dorner February 26, 2025 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding Reporting Categories to Items
2 participants