Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Fix delete facility endpoint #1472

Merged
merged 1 commit into from Sep 23, 2021
Merged

Fix delete facility endpoint #1472

merged 1 commit into from Sep 23, 2021

Conversation

TaiWilkin
Copy link
Contributor

@TaiWilkin TaiWilkin commented Sep 22, 2021

Overview

When deleting a facility with multiple confirmed or automatic matches,
we attempt to split off and create a new facility from the top match.
In the case that the match was missing a location, this caused a null
location error, preventing the facility from being deleted.

Matches without locations are now filtered out when searching for valid
matches to use to create a new facility during the deletion process.

Connects Client #49

Notes

If the match without a location is the only automatic/confirmed match other than the primary facility match, no new facility will be created when deleting a facility. Potentially, we could instead send a descriptive error message to the user in this case, to allow them to choose to move the match before deleting; but this has the downside of preventing them from deleting the facility at all unless they transfer the match, forcing their hand, which seems like very undesired behavior.

See here for the Rollbar error that occurs when attempting to delete the facility indicated in the issue. It is the same error as previously occurred in the split API when users attempted to split a match with no location.

Testing Instructions

  • On develop, run ./scripts/server and identify a facility with a confirmed or automatic match in addition to the primary match.
  • Set the geocoded_point for the match's facilitylistitem to null using ./scripts/manage shell_plus.
  • Attempt to delete the facility via the facility deletion tool. It should throw an error matching the one that occurred in production, related to a null location value.
  • Checkout this branch and attempt to delete the same facility. It should delete without an issue.

Checklist

  • fixup! commits have been squashed
  • CI passes after rebase
  • CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

Copy link
Contributor

@jwalgran jwalgran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed. I agree with your assessment of the situation and think you made the right implementation choice. Thanks for including a test.

@jwalgran jwalgran assigned TaiWilkin and unassigned jwalgran Sep 23, 2021
When deleting a facility with multiple confirmed or automatic matches,
we attempt to split off and create a new facility from the top match.
In the case that the match was missing a location, this caused a null
location error, preventing the facility from being deleted.

Matches without locations are now filtered out when searching for valid
matches to use to create a new facility during the deletion process.
@TaiWilkin
Copy link
Contributor Author

Thank you for reviewing!

@TaiWilkin TaiWilkin merged commit 0559fe3 into develop Sep 23, 2021
@TaiWilkin TaiWilkin deleted the tw/fix-facility-delete branch September 23, 2021 16:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants