-
Notifications
You must be signed in to change notification settings - Fork 13
Allow users to transfer matches between facilities #1464
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.
UI for the dialog looks good.
The 3 buttons in the originating "Matches to adjust" panel are rather cramped, so I'm happy to see there'll be a follow-up card to work on that part of the UI.
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.
Clear, easy to follow implementation. My test worked and I confirmed that in addition to the new MOVE
event in the facility history of the secondary facility, the history of the target facility also includes an ASSOCIATE
event for the new match. Nice work.
I did find was appears to be a bug in the new dialog box. Each keystoke into the text box sends an API request. We should fix this before merging.
c4760b8
to
55319e1
Compare
In some cases, list items have been incorrectly matched to a facility. Currently, users will 'split' those list items away from the incorrect facility. As a second step, they sometimes merge the match to a different facility. However, when the list items are not able to be geocoded, it's impossible to use the 'split' functionality to create a new facility from the match. This adds the functionality to move or transfer a list item directly from one existing facility to another existing facility, avoiding the need to create an new, intermediate facility.
55319e1
to
64d1fd6
Compare
@jwalgran I had to rebase in the process of fixing a weird build error that was occurring; apologies for muddying the water for changes. |
@@ -1793,6 +1796,8 @@ def split(self, request, pk=None): | |||
m.facility_list_item.source.contributor.id | |||
if m.facility_list_item.source.contributor else None, | |||
'match_id': m.id, | |||
'is_geocoded': |
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.
It seems that the geocoded location wasn't being passed through the API properly. Because we do not actually need to know the location on the front end, just whether it exists, I've updated to send a boolean instead of the geocoded point.
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.
Retest successful. 👍
Thank you for reviewing! |
Overview
In some cases, list items have been incorrectly matched to a facility.
Currently, users will 'split' those list items away from the incorrect
facility. As a second step, they sometimes merge the match to a
different facility.
However, when the list items are not able to be geocoded, it's
impossible to use the 'split' functionality to create a new facility
from the match.
This adds the functionality to move or transfer a list item directly
from one existing facility to another existing facility, avoiding the
need to create a new, intermediate facility.
Connects #1459
Demo
Notes
Scott recommended using more descriptive / different titles for the buttons to clarify their purpose. I think to do this thoroughly will require a follow-up card to determine the most appropriate titles and do any refactoring / styling needed to improve the display.
Part of the facility details card was pulled out into a separate component. Initially I hope to reuse the entire component, but the card is actually the basis of the full page, and its functionality is very tied to the data flow for the facility whose matches are being adjusted. Separating the main UI of the card allowed me to reuse just that portion in the transfer modal with a different facility from the rest of the page.
Testing Instructions
Checklist
fixup!
commits have been squashed