Enable contributors to remove individual facility list items from public display #619

kellyi · 2019-06-19T17:44:16Z

Overview

This PR enables list contributors to remove list items. Removing an item will keep it on a list but suppress it from displaying on the facility details page in any way. I designed this in line with the specs detailed in #594.

Connects #594

Demo

Notes

In principle there is no reason why we cannot also remove list items with a status of "POTENTIAL_MATCH". However I did not have implement a UI for doing that, partly because the actions area for those rows is already kind of busy and partly because list items that are not yet matched don't show up on the facility details page anyway.

Testing Instructions

get branch then ./scripts/manage resetdb, ./scripts/manage processfixtures, ./scripts/server and then sign in as c6@example
in one tab, search for the facility named "Cotia". On its details page verify that there is a link to the contributor page for c6
visit the facility lists for c6 and search for "Cotia". Use the UI to remove the facility and verify that the row changes its opacity on success and that you see the "REMOVED" status.
try out the status filters and verify that "REMOVED" and the others work correctly
look at the details page for "Cotia" and very that the contributor is no longer associated with the facility
try removing some other facilities and verify that that also works. You may want to sign in as c2 or c8 and remove facilities from those lists, as you should test out that you can also remove "MATCHED" and "CONFIRMED_MATCH" list items -- and not just "NEW_FACILITY" items.

Checklist

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

jwalgran · 2019-06-24T18:05:25Z

Looking at this now.

jwalgran

Remove is working as intended and suppressing the contributor association on the details page but we need to also suppress the contributor association when searching for facilities.

Cotia still shows up on http://localhost:6543/?contributors=6

jwalgran · 2019-06-24T18:18:23Z

src/django/api/views.py

+                .filter(facility_list_item=facility_list_item) \
+                .update(is_active=False)
+
+            updated_facility_list_item = FacilityListItem \


Consider using facility_list_item.refresh_from_db() rather than a filtered get.

👍 Replaced in 7322c56

jwalgran · 2019-06-24T18:53:25Z

src/app/src/components/FacilityListItemsTable.jsx

+                                        <Typography
+                                            style={facilityListItemsTableStyles.dialogTextStyles}
+                                        >
+                                            Do you really want to remove this list item?


Consider explaining that removed facilities may still appear on the facility map, but they will not be associated with your organization.

Good idea -- I adjusted this in 6912094

jwalgran · 2019-06-24T18:59:52Z

src/app/src/components/FacilityListItemsTableRow.jsx


 import { facilityListItemStatusPropType } from '../util/propTypes';

 import { listTableCellStyles } from '../util/styles';

 import { makeFacilityDetailLink } from '../util/util';

-const propsAreEqual = (prevProps, nextProps) =>


Consider explaining in the commit message why we are removing propsAreEqual.

I'll add this when squashing the fixups. We dropped this because we're no longer using React.memo on this component.

jwalgran · 2019-06-24T19:20:45Z

src/app/src/components/FacilityListItemsTableRow.jsx

-    && isEqual(prevProps.hover, nextProps.hover)
-    && isEqual(prevProps.newFacility, nextProps.newFacility)
-    && isEqual(prevProps.oarID, nextProps.oarID);
+const makeTableRowStyles = cond([


I like the power that cond and conforms give us, but we should consider that in cases like this where we are mapping 2 values that don't have any complex interactions, a lower-order solution is more compact and arguably more easily understood.

const makeTableRowStyles = ({ handleSelectRow, isRemoved}) => { const pointerStyle = isFunction(handleSelectRow) ? { cursor: 'pointer' } : {}; const opacityStyle = isRemoved ? { opacity: '0.6' } : {}; return merge(pointerStyle, opacityStyle); }

I changed this in 90eb513 to be:

const makeTableRowStyles = ({ handleSelectRow, isRemoved }) => ({ cursor: isFunction(handleSelectRow) ? 'pointer' : 'auto', opacity: isRemoved ? '0.6' : '1.0', });

jwalgran · 2019-06-24T19:37:33Z

src/app/src/components/FacilityListItemsDetailedTableRowCell.jsx

+                    {title}
+                </span>
+                <Button
+                    variant="contained"


Consider using a stock button. The full column of blue buttons draws a lot of focus. Contrast these with the approve/reject buttons, which we absolutely need the contributor to click on. The remove buttons are an optional feature that we may want to call less attention to.

diff --git a/src/app/src/components/FacilityListItemsTableRow.jsx b/src/app/src/components/FacilityListItemsTableRow.jsx index 55936de..8d0748e 100644 --- a/src/app/src/components/FacilityListItemsTableRow.jsx +++ b/src/app/src/components/FacilityListItemsTableRow.jsx @@ -113,7 +113,6 @@ const FacilityListItemsTableRow = ({ {newFacility ? 'NEW_FACILITY' : status} </span> <Button - variant="contained" color="primary" onClick={handleRemoveItem} style={{ marginLeft: '5px', marginRight: '5px' }}

Sounds good -- I adjusted these buttons in 2a796e6

kellyi · 2019-06-24T20:48:32Z

Oof, thanks for catching the missing filter on the facilities search endpoint. I added it in 151f6a4 which will suppress matches with is_active=False from appearing in the contributors & contributor types results.

kellyi · 2019-06-24T20:48:51Z

This is ready for another look!

kellyi · 2019-06-24T22:03:24Z

Rebased this on develop

Update model & create a migration to add an `is_active` field to FacilityMatch.

- add remove endpoint for facility list item which updates `is_active` for each of the list item's facility matches to `False` - add tests for remove endpoint

- add buttons + logic for removing facility list items - style removed rows with 0.6 opacity - exclude removed list items from display on the facility list details screen - enable filtering by REMOVED status in the facility list items table - remove React.memo & propsAreEqual from FacilityListItemsTableRow & let React figure out the re-rendering conditions directly

jwalgran

Removals are working well. Thanks for the fixups. I used c8@example.com to test pending matches and it works perfectly.

kellyi · 2019-06-24T22:53:37Z

Awesome! Thanks for your help with this!

kellyi force-pushed the ki/remove-list-items branch 7 times, most recently from 5c7ca96 to d95d8eb Compare June 20, 2019 21:35

kellyi changed the title ~~WIP Ki/remove list items~~ Enable contributors to mark facility list items as "REMOVED" Jun 20, 2019

kellyi changed the title ~~Enable contributors to mark facility list items as "REMOVED"~~ Enable contributors to remove facility list items from public display Jun 20, 2019

kellyi changed the title ~~Enable contributors to remove facility list items from public display~~ Enable contributors to remove individual facility list items from public display Jun 20, 2019

kellyi requested a review from jwalgran June 20, 2019 21:43

kellyi assigned jwalgran Jun 20, 2019

kellyi marked this pull request as ready for review June 20, 2019 21:44

kellyi force-pushed the ki/remove-list-items branch from d95d8eb to 7322c56 Compare June 24, 2019 16:24

jwalgran suggested changes Jun 24, 2019

View reviewed changes

kellyi force-pushed the ki/remove-list-items branch 2 times, most recently from cadef0e to 0ccc557 Compare June 24, 2019 22:03

jwalgran self-requested a review June 24, 2019 22:05

Kelly Innes added 3 commits June 24, 2019 18:18

Add is_active field to FacilityMatch

d8b5590

Update model & create a migration to add an `is_active` field to FacilityMatch.

Add remove endpoint for facility list items

8cc24c9

- add remove endpoint for facility list item which updates `is_active` for each of the list item's facility matches to `False` - add tests for remove endpoint

kellyi force-pushed the ki/remove-list-items branch from 0ccc557 to 9103513 Compare June 24, 2019 22:19

jwalgran approved these changes Jun 24, 2019

View reviewed changes

jwalgran assigned kellyi and unassigned jwalgran Jun 24, 2019

kellyi merged commit 0fe7ea2 into develop Jun 24, 2019

kellyi deleted the ki/remove-list-items branch June 24, 2019 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable contributors to remove individual facility list items from public display #619

Enable contributors to remove individual facility list items from public display #619

kellyi commented Jun 19, 2019 •

edited

jwalgran commented Jun 24, 2019

jwalgran left a comment

jwalgran Jun 24, 2019

kellyi Jun 24, 2019

jwalgran Jun 24, 2019

kellyi Jun 24, 2019

jwalgran Jun 24, 2019

kellyi Jun 24, 2019

jwalgran Jun 24, 2019

kellyi Jun 24, 2019

jwalgran Jun 24, 2019

kellyi Jun 24, 2019

kellyi commented Jun 24, 2019

kellyi commented Jun 24, 2019

kellyi commented Jun 24, 2019

jwalgran left a comment

kellyi commented Jun 24, 2019

Enable contributors to remove individual facility list items from public display #619

Enable contributors to remove individual facility list items from public display #619

Conversation

kellyi commented Jun 19, 2019 • edited

Overview

Demo

Notes

Testing Instructions

Checklist

jwalgran commented Jun 24, 2019

jwalgran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyi commented Jun 24, 2019

kellyi commented Jun 24, 2019

kellyi commented Jun 24, 2019

jwalgran left a comment

Choose a reason for hiding this comment

kellyi commented Jun 24, 2019

kellyi commented Jun 19, 2019 •

edited