-
Notifications
You must be signed in to change notification settings - Fork 14
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
Change handling of legacy occurrence point data #5022
Change handling of legacy occurrence point data #5022
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.
looks reasonable to me, thanks!
@@ -339,49 +339,51 @@ def data_collection_features | |||
end.compact | |||
end | |||
|
|||
# Occurrence Point Forms that are enabled for this Enrollment. | |||
# Returns array of OpenStructs, which are resolved by the HmisSchema::OccurrencePointForm GQL type. | |||
def occurrence_point_forms |
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.
This is fine; it's consistent with how the other models. But the Enrollment class is getting kind of big (550 lines). I wonder if this might be an opportunity to extract this into some kind of single-purpose service class
class Enrollment
#...
def occurrence_point_forms
OccurrencePointFormCollection.new(self).all
end
end
class OccurrencePointFormCollection
def initialize(enrollment)
@enrollment = enrollment
end
def all = active_forms + legacy_forms
def active_forms
definitions = Hmis::Form::Definition
.with_role(:OCCURRENCE_POINT)
.published_or_retired
.latest_versions
# definitions.map ...
end
def legacy_forms
#...
end
end
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 for the suggestion and snippet @ttoomey, I updated this to use a class. Both the Enrollment and Project occurrence point form function logic are moved into it
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.
This looks great, thanks for the restructure!
Merging this PR
Description
Issue: https://github.com/open-path/Green-River/issues/7236
This is walking back a previous assumption which said "you would only have one Occurrence Point form per field you're collecting". So only ONE move-in form, ONE unit assignment form, etc. If you want to customize it it can be done with custom_rule values.
Proposed approach is to only show the enabled forms, PLUS special logic to show Move-in Date, DOE, and PATH status if there is data that would otherwise be hidden.
"Bug" Reproduction:
Scenarios requiring this change:
current_unit
(https://github.com/open-path/Green-River/issues/7093)Type of change
Bug fix
Checklist before requesting review