-
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
Implement Join Household mutation #5047
base: release-151
Are you sure you want to change the base?
Conversation
This reverts commit b514eba.
* Fix spec tests * Correct spec * Fix spec * Fix spec tests for table actions * PR feedback (wip) * Use aria label directly * Remove repetition of sys tests --------- Co-authored-by: Gig <[email protected]>
enrollment.update!( | ||
household_id: receiving_household_id, | ||
relationship_to_hoh: map_enrollment_id_to_relationship[enrollment.id.to_s], | ||
) |
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.
Is it necessary to do two saves per enrollment? (update!
here and save!
below). I know assign_unit
does some validations on household membership, but I think it should work even if the household_id change hasn't been persisted yet.
enrollments.map do |enrollment| | ||
{ | ||
'enrollmentId': enrollment.id, | ||
'relationshipToHoh': enrollment.relationship_to_hoh, |
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.
we should snake case here as well. Suggested in other comment to move this to snapshot the Household if possible, since the household event table is snapshotting households
app/models/grda_warehouse/utility.rb
Outdated
@@ -200,6 +200,7 @@ def self.modifier(model) | |||
GrdaWarehouse::Hud::Client, | |||
GrdaWarehouse::Hud::Project, | |||
GrdaWarehouse::Hud::User, | |||
GrdaWarehouse::Hud::Enrollment, |
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.
was this added in order to cascade deletion of hmis_household_events when deleting an enrollment?
it's not needed for the spec test to pass locally for me
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.
Yes -- that's weird, I did need it at some point to get the spec tests to pass. but anyway, I will remove it per #5047 (comment).
conflict_scope = Hmis::Hud::Enrollment. | ||
where(personal_id: enrollment.personal_id, data_source_id: enrollment.data_source_id). | ||
with_conflicting_dates(project: enrollment.project, range: enrollment.entry_date...enrollment.exit_date) | ||
|
||
if enrollment.persisted? | ||
# If the entry date is being changed on an EXISTING enrollment, and it overlaps with another one, it should be a warning | ||
conflict_scope = conflict_scope.where.not(id: enrollment.id) | ||
return :warning if conflict_scope.any? | ||
return :warning, conflict_scope.first if conflict_scope.any? |
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 should be selected deterministically, or return all of them. Since it's unlikely to have multiple overlapping enrollments, it's probably OK to pick one -- maybe by earliest entry date, so it's consistent with the below.
# if the entry date is being set on a NEW enrollment, and the entry date is before the entry date of any conflicting enrollments, it should be a warning | ||
return :warning if enrollment.entry_date < min_conflict_date | ||
# If the entry date is being set on a NEW enrollment, | ||
conflicting_enrollment = conflict_scope.order(entry_date: :asc).first # pick the conflicting enrollment with the earliest entry date |
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.
nit:
conflicting_enrollment = conflict_scope.order(entry_date: :asc).first # pick the conflicting enrollment with the earliest entry date | |
conflicting_enrollment = conflict_scope.min_by(&:entry_date) |
@@ -34,23 +34,24 @@ def self.already_enrolled_full_message | |||
'Client has another enrollment in this project that conflicts with this entry date.' | |||
end | |||
|
|||
def self.find_conflict_severity(enrollment) | |||
def self.find_conflicting_enrollment(enrollment) |
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.
Is there test coverage for the changes here?
It looks like all the Enrollment creation tests are jammed into the submit_form_spec which is big
hmis-warehouse/drivers/hmis/spec/requests/hmis/submit_form_spec.rb
Lines 468 to 681 in a65d9cd
describe 'SubmitForm for Enrollment creation' do | |
let(:definition) { Hmis::Form::Definition.find_by(role: :ENROLLMENT) } | |
let!(:c3) { create :hmis_hud_client_complete, data_source: ds1 } | |
let(:test_input) do | |
{ | |
form_definition_id: definition.id, | |
**mock_form_values_for_definition(definition), | |
project_id: p2.id, | |
client_id: c3.id, | |
confirmed: false, | |
} | |
end | |
def merge_hud_values(input, *args) | |
input.merge(hud_values: input[:hud_values].merge(*args)) | |
end | |
def expect_error_message(input, exact: true, **expected_error) | |
response, result = post_graphql(input: { input: input }) { mutation } | |
errors = result.dig('data', 'submitForm', 'errors') | |
aggregate_failures 'checking response' do | |
expect(response.status).to eq(200), result&.inspect | |
if exact | |
expect(errors).to contain_exactly(include(expected_error.stringify_keys)) | |
else | |
expect(errors).to include(include(expected_error.stringify_keys)) | |
end | |
end | |
end | |
it 'should error if adding second HoH to existing household' do | |
input = merge_hud_values( | |
test_input, | |
'householdId' => e1.household_id, | |
) | |
expect_error_message(input, fullMessage: Hmis::Hud::Validators::EnrollmentValidator.one_hoh_full_message) | |
end | |
it 'should error if creating household without hoh' do | |
input = merge_hud_values( | |
test_input, | |
'relationshipToHoH' => Types::HmisSchema::Enums::Hud::RelationshipToHoH.key_for(2), | |
) | |
expect_error_message(input, fullMessage: Hmis::Hud::Validators::EnrollmentValidator.first_member_hoh_full_message) | |
end | |
it 'should error if client already has an open enrollment in the household' do | |
e2 = create(:hmis_hud_enrollment, data_source: ds1, project: p1, entry_date: '2000-01-01', household_id: e1.household_id) | |
input = merge_hud_values( | |
test_input.merge(client_id: e2.client.id), | |
'householdId' => e1.household_id, | |
'relationshipToHoH' => Types::HmisSchema::Enums::Hud::RelationshipToHoH.key_for(2), | |
) | |
expect_error_message(input, exact: false, fullMessage: Hmis::Hud::Validators::EnrollmentValidator.duplicate_member_full_message) | |
end | |
it 'should not error if client has a closed enrollment in the household' do | |
e2 = create(:hmis_hud_enrollment, data_source: ds1, project: p1, entry_date: '2000-01-01', household_id: e1.household_id) | |
create(:hmis_hud_exit, enrollment: e2, client: e2.client, data_source: ds1, exit_date: '2001-01-01') | |
input = merge_hud_values( | |
test_input.merge(client_id: e2.client.id), | |
'householdId' => e1.household_id, | |
'relationshipToHoH' => Types::HmisSchema::Enums::Hud::RelationshipToHoH.key_for(2), | |
) | |
response, result = post_graphql(input: { input: input }) { mutation } | |
errors = result.dig('data', 'submitForm', 'errors') | |
expect(response.status).to eq(200), result&.inspect | |
expect(errors).to be_empty | |
household_size = result.dig('data', 'submitForm', 'record', 'householdSize') | |
expect(household_size).to eq(2) # household size is 2 even though it contains 3 enrollments | |
end | |
it 'should warn if client already enrolled' do | |
input = merge_hud_values( | |
test_input.merge(client_id: e1.client.id, project_id: e1.project.id), | |
) | |
expect_error_message(input, fullMessage: Hmis::Hud::Validators::EnrollmentValidator.already_enrolled_full_message) | |
end | |
it 'should error if entry date is in the future' do | |
input = merge_hud_values( | |
test_input, | |
'entryDate' => Date.tomorrow.strftime('%Y-%m-%d'), | |
) | |
expect_error_message(input, message: Hmis::Hud::Validators::EnrollmentValidator.future_message) | |
end | |
describe 'on project that has closed' do | |
before(:each) { p2.update!(operating_start_date: today + 1.day) } | |
it 'should be invalid' do | |
input = merge_hud_values(test_input, 'entryDate' => today.to_fs(:db)) | |
expect_error_message(input, message: Hmis::Hud::Validators::BaseValidator.before_project_start_message(p2.operating_start_date)) | |
end | |
end | |
describe 'on project that has not started' do | |
before(:each) { p2.update!(operating_end_date: today - 1.day) } | |
it 'should be invalid' do | |
input = merge_hud_values(test_input, 'entryDate' => today.to_fs(:db)) | |
expect_error_message(input, message: Hmis::Hud::Validators::BaseValidator.after_project_end_message(p2.operating_end_date)) | |
end | |
end | |
end | |
describe 'SubmitForm for Enrollment updates' do | |
let(:definition) { Hmis::Form::Definition.find_by(role: :ENROLLMENT) } | |
let(:e1) { create :hmis_hud_enrollment, data_source: ds1 } | |
let(:wip_e1) { create :hmis_hud_wip_enrollment, data_source: ds1 } | |
let(:test_input) do | |
{ | |
form_definition_id: definition.id, | |
**mock_form_values_for_definition(definition), | |
project_id: p1.id, | |
client_id: c2.id, | |
confirmed: false, | |
} | |
end | |
it 'should save new enrollment as WIP' do | |
record, _errors = submit_form(test_input) | |
expect(record['inProgress']).to eq(true) | |
end | |
it 'should not change WIP status (WIP enrollment)' do | |
input = test_input.merge(record_id: wip_e1.id) | |
submit_form(input) | |
wip_e1.reload | |
expect(wip_e1.in_progress?).to eq(true) | |
end | |
it 'should not change WIP status (non-WIP enrollment)' do | |
input = test_input.merge(record_id: e1.id) | |
submit_form(input) | |
e1.reload | |
expect(e1.in_progress?).to eq(false) | |
end | |
it 're-submission should not create a second FormProcessor' do | |
input = test_input.merge(record_id: e1.id) | |
expect { submit_form(input) }.to change(Hmis::Form::FormProcessor, :count).by(1) | |
expect { submit_form(input) }.not_to change(Hmis::Form::FormProcessor, :count) | |
end | |
end | |
describe 'SubmitForm for Enrollment on project with ProjectAutoEnterConfig' do | |
let(:definition) { Hmis::Form::Definition.find_by(role: :ENROLLMENT) } | |
let!(:aec) { create :hmis_project_auto_enter_config, project: p1 } | |
let(:test_input) do | |
{ | |
form_definition_id: definition.id, | |
**mock_form_values_for_definition(definition), | |
project_id: p1.id, | |
client_id: c1.id, | |
confirmed: false, | |
} | |
end | |
it 'should save new enrollment without WIP status' do | |
record, errors = submit_form(test_input) | |
expect(errors).to be_empty | |
enrollment = Hmis::Hud::Enrollment.find_by(id: record['id']) | |
expect(enrollment).to be_present | |
expect(enrollment.in_progress?).to eq(false) | |
expect(enrollment.intake_assessment).to be_present | |
expect(enrollment.intake_assessment.assessment_date).to eq(enrollment.entry_date) | |
expect(enrollment.intake_assessment.wip).to eq(false) | |
expect(enrollment.intake_assessment.form_processor).to be_present | |
end | |
end | |
describe 'SubmitForm for Create+Enroll' do | |
let(:definition) { Hmis::Form::Definition.find_by(role: :NEW_CLIENT_ENROLLMENT) } | |
let(:test_input) do | |
{ | |
form_definition_id: definition.id, | |
**mock_form_values_for_definition(definition), | |
project_id: p2.id, | |
confirmed: true, | |
} | |
end | |
def merge_hud_values(input, *args) | |
input.merge(hud_values: input[:hud_values].merge(*args)) | |
end | |
it 'creates client and enrollment' do | |
record, errors = submit_form(test_input) | |
expect(errors).to be_empty | |
expect(record).to be_present | |
enrollment = Hmis::Hud::Enrollment.find(record['id']) | |
expect(enrollment.client).to be_present | |
expect(enrollment.client.first_name).to eq('First') | |
end | |
it 'validates client (invalid field)' do | |
input = merge_hud_values( | |
test_input, | |
'Client.dobDataQuality' => 'INVALID', | |
) | |
_, errors = submit_form(input) | |
expect(errors).to contain_exactly(include({ 'attribute' => 'dobDataQuality', 'type' => 'invalid' })) | |
end | |
it 'validates client (invalid DOB)' do | |
input = merge_hud_values( | |
test_input, | |
'Client.dob' => '2200-01-01', # future dob is not valid | |
) | |
_, errors = submit_form(input) | |
expect(errors).to contain_exactly(include({ 'attribute' => 'dob', 'type' => 'out_of_range' })) | |
end | |
end |
This seems like the right place to add assertions on the data
that is returned:
hmis-warehouse/drivers/hmis/spec/requests/hmis/submit_form_spec.rb
Lines 540 to 545 in a65d9cd
it 'should warn if client already enrolled' do | |
input = merge_hud_values( | |
test_input.merge(client_id: e1.client.id, project_id: e1.project.id), | |
) | |
expect_error_message(input, fullMessage: Hmis::Hud::Validators::EnrollmentValidator.already_enrolled_full_message) | |
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.
Looks good!! Mostly minor comments. The idea of hmis_household_event_enrollments may need some discussion, I'm not convinced that we need it. The Let me know if you have any questions on the review!
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 good! lets hold off on merging until this is targeted to a release branch, so the merge history is clearer.
create_table :hmis_household_events do |t| | ||
t.string :HouseholdID, null: false | ||
t.references :data_source, null: false | ||
t.string :event_type, null: false | ||
t.jsonb :event_details | ||
t.references :user, null: false | ||
t.timestamps | ||
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.
Tagging @eanders as an FYI on this new HMIS table which tracks changes to households.
For now, the events are "join" and "split", with the idea that we may also want to track "add" "remove" "hoh change" etc.
In the future, this could back a neat "household history" interface.
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 tag! That sounds like a wonderful plan.
Merging this PR
Description
GH issue: https://github.com/open-path/Green-River/issues/5752
Related frontend work: greenriver/hmis-frontend#1022
Summary of changes:
JoinHouseholds
mutationhmis_household_events
table and model for storing join and split eventsdata
field of the errorStill to do:
Type of change
Checklist before requesting review
//: # NOTE: system tests may fail if there is no branch on the hmis-frontend that matches the Source or Target branch of this PR. This is expected