Add an API parameter and UI to filter facility list items by status #507

jwalgran · 2019-05-08T17:50:35Z

Overview

Allow facility list items to be filtered by selecting one or more statuses. It is an uncommon situation in a thousand item list to only have a dozen potential matches. Having a filter makes it dramatically easier to find and act on these items.

Connects #504

Demo

Testing Instructions

Reset the database
- ./scripts/resetdb
- ./scripts/processfixtures
Log in as c8@example.com
Browse http://localhost:6543/lists/8
Filter by different statuses and verify that the list results match what is expected.
Filter by POTENTIAL_MATCH and approve one of the matches. Verify that the item disappears from the table and the item count is correctly updated.
Approve or reject the rest of the potential matches and verify that "No matching items" is displayed in the table.
Verify that clicking the "SHOW ALL ITEMS" button works.

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-05-08T19:30:47Z

I'd like to discuss our options for dealing with the situation where you have filtered the list to see only POTENTIAL_MATCHes and then you approve a match. Our current implementation updates the item in place, changing the status to MATCHED, creating an inconsistency with the current state.

The solution I am thinking about implementing is to pass the page, rowsPerPage and status query string arguments to the approve/reject endpoints and modify the view to return an entire updated page of items instead of a single item if a page query string parameter was specified. That allows us to easily update the state and keep the data in the table in sync with the filter.

Does this sound like a good plan?

kellyi · 2019-05-09T14:25:24Z

The solution I am thinking about implementing is to pass the page, rowsPerPage and status query string arguments to the approve/reject endpoints and modify the view to return an entire updated page of items instead of a single item if a page query string parameter was specified. That allows us to easily update the state and keep the data in the table in sync with the filter.

This makes sense to me. I was thinking about the general problem outside of this particular solution and came up with a similar-ish idea that put the re-fetch-page logic on the frontend.

The client "knows" in some way which filter is applied, so if the POTENTIAL_MATCH filter was applied and an accept/reject request had been completed, one of the componentDidUpdate methods could check the new items list against the previous items list to see whether it still had the same number of POTENTIAL_MATCH status facilities. If not, then it could immediately re-fetch the entire page.

The potential upside of this is that it would not require adding conditional logic to the API endpoints; a potential downside is that it'd push the re-fetch logic into a React component's componentDidUpdate method. Not sure which is preferable -- although potentially the API may have other consumers in the future outside the React client?

If we were to put the conditional logic in the API accept/reject handlers, I think that it'd make sense to have the client have to pass an additional querystring parameter along with page/rowsPerPage/status: something like returnEntireListPage=true or similar, so that the querystring params would be a little more self-documenting.

jwalgran · 2019-05-09T15:52:27Z

Detecting the change on the client side and making a follow up request is compelling.

As I was thinking about implementing that I realized that if the contributor is scrolled to the bottom of a page of 100 results when they approve a pending match, triggering a page refresh will result in scrolling them back to the top of the page. We may be able to work around it by avoiding or updating startFetchFacilityListItems so that we don't set the existing list of items to null before loading the new ones.

https://github.com/open-apparel-registry/open-apparel-registry/blob/a3f2f76d6fa2b18b53edc56f9e1911b219c8f303/src/app/src/reducers/FacilityListDetailsReducer.js#L145-L148

I think I'll move ahead with the client-side approach.

jwalgran · 2019-05-10T20:36:12Z

I implemented refreshing after approving/rejecting potential matches. Thanks for the design suggestion. It worked out well.

This is ready for review.

kellyi · 2019-05-13T14:08:22Z

src/app/src/components/FacilityListItemsTable.jsx

-        fetchListItems(listID, newPage + 1, rowsPerPage, params);
-    };
+        const params = createParamsFromQueryString(search);
+        if (params.status && params.status.includes('POTENTIAL_MATCH') && items && prevProps.items) {


Do we have POTENTIAL_MATCH set as a constant somewhere in the app? If so we could consider checking on the constant value here; if not, we could add one?

Looks like it's set on the facilityListItemStatusChoicesEnum.

Addressed in fixup ca65c8c

kellyi · 2019-05-13T14:10:37Z

src/app/src/components/FacilityListItemsTable.jsx

+            fetchListItems(listID, 1, rowsPerPage, newParams);
+        };
+
+        const handleShowAllClicked = () => {


Now that this is a class component I think it'd make sense to move these functions out of render and make them class methods. I think they could just be cut and paste, and then callers updated to call them with this.whatever.

I did some reading and discovered that defining closures inside render like this is a performance drag because since redefining these functions every time triggers unnessesary rendering. Thanks for suggestion.

Addressed in fixup 02efe99

kellyi · 2019-05-13T14:13:20Z

src/app/src/util/util.js

-        return Object.freeze({ status });
+        return Array.isArray(status)
+            ? Object.freeze({ status })
+            : Object.freeze({ status: [status] });


Consider updating the test to check for this new outcome.

Added tests in fixup b1e998a

kellyi · 2019-05-13T14:14:53Z

src/app/src/util/util.js

@@ -185,6 +185,22 @@ export const createPaginationOptionsFromQueryString = (qs) => {
    });
 };

+export const createParamsFromQueryString = (qs) => {


Consider adding a test for this new function.

Added tests in fixup b1e998a

kellyi · 2019-05-13T14:18:14Z

src/app/src/components/FacilityListItemsTable.jsx

+                            {tableRows}
+                            <ShowOnly when={listIsEmpty}>
+                                <TableRow>
+                                    <TableCell colSpan={5} style={{textAlign: 'center'}}>


Looks like civicci's seeing a linter error here resolvable by adding a space before and after textAlign: 'center'.

Addressed in fixup 0aeb695

kellyi · 2019-05-13T15:03:34Z

Question! Is it correct that selecting the MATCHED option should display both MATCHED and NEW_FACILITY status items? I know the latter is a computed status, but didn't know whether we intended for MATCHED to now be inclusive of NEW_FACILITY:

kellyi · 2019-05-13T15:05:36Z

It looks like a similar thing is happening with CONFIRMED_MATCH and NEW_FACILITY status:

kellyi · 2019-05-13T15:06:44Z

This is working as expected otherwise!

jwalgran · 2019-05-14T01:32:43Z

Thanks for the great review. I addressed all of your comments and this is ready for another look.

kellyi

👍 I tested this out again with the fixups and everything's working well!

In a list of thousands of facilities there may only be a few dozen that require the contributor to approve or reject a potential match. Adding a filter by status will allow us to display this subset of list items.

NEW_FACILITY is not a real workflow state. A `FacilityListItem` is considered a NEW_FACILITY when it has a bidirectional relationship with a `Facility`. We want to avoid showing NEW_FACILITY items when filtering by the MATCHED or CONFIRMED_MATCH statuses. To handle all of this we create a set of special case filter statements and compose them together with `status=` statements for the other statuses.

In order to page through a list of facilities filtered by status we need to include a status parameter along with the existing page number and items per page parameters. In addition to passing the new status param to the API we are also updating the pagingation UI to show the filtered count rather than the total list count

Follows the pattern of how page number and items per page changes are handled. Changing the status filter will change the number of possible pages, so we are always resetting the page number to 1.

If you have filtered the list of facility list items by `POTENTIAL_MATCH` and then confirm or reject an item we need to update the current page since the count of items in the `POTENTIAL_MATCH` status is now different. To accomplish this we convert the FacilityListItemsTable from a functional to a class component so we can add logic to `componentDidUpdate` that conditionally triggers a refresh.

Now that people are able to filter the list items, it is possible to have an empty table. Adding a "No matching items" message creates a distinction between the loading and empty states.

Creating closures inside of the `render` function can lead to extraneous rendering.

jwalgran · 2019-05-14T18:06:08Z

Thanks again for the great feedback.

jwalgran requested a review from kellyi May 8, 2019 17:50

jwalgran assigned kellyi May 8, 2019

jwalgran added the in progress label May 8, 2019

jwalgran changed the title ~~Add an API parameter and UI filter facility list items status~~ Add an API parameter and UI to filter facility list items by status May 8, 2019

jwalgran marked this pull request as ready for review May 10, 2019 20:35

kellyi reviewed May 13, 2019

View reviewed changes

jwalgran force-pushed the jcw/filter-list-items-by-status branch from bc96f9b to b1e998a Compare May 14, 2019 00:38

jwalgran requested a review from kellyi May 14, 2019 01:31

kellyi approved these changes May 14, 2019

View reviewed changes

kellyi assigned jwalgran and unassigned kellyi May 14, 2019

jwalgran force-pushed the jcw/filter-list-items-by-status branch from cf2cdcb to 913ceef Compare May 14, 2019 17:08

jwalgran added 6 commits May 14, 2019 10:30

Add filter by status to the facility list item endpoint

7c9b4c0

In a list of thousands of facilities there may only be a few dozen that require the contributor to approve or reject a potential match. Adding a filter by status will allow us to display this subset of list items.

Add a select box to facility list items to filter by status

4e3171b

Follows the pattern of how page number and items per page changes are handled. Changing the status filter will change the number of possible pages, so we are always resetting the page number to 1.

Show a message when the facility list item table is empty

cd59ae5

Now that people are able to filter the list items, it is possible to have an empty table. Adding a "No matching items" message creates a distinction between the loading and empty states.

jwalgran added 2 commits May 14, 2019 10:30

Move event handlers out of FacilityListItemsTable.render

86f9585

Creating closures inside of the `render` function can lead to extraneous rendering.

Update CHANGELOG

79cfddd

jwalgran force-pushed the jcw/filter-list-items-by-status branch from 913ceef to 79cfddd Compare May 14, 2019 17:32

jwalgran merged commit a022b81 into develop May 14, 2019

hectcastro removed the in progress label May 14, 2019

jwalgran deleted the jcw/filter-list-items-by-status branch May 14, 2019 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an API parameter and UI to filter facility list items by status #507

Add an API parameter and UI to filter facility list items by status #507

jwalgran commented May 8, 2019 •

edited

jwalgran commented May 8, 2019

kellyi commented May 9, 2019 •

edited

jwalgran commented May 9, 2019 •

edited

jwalgran commented May 10, 2019

kellyi May 13, 2019

kellyi May 13, 2019

jwalgran May 14, 2019

kellyi May 13, 2019

jwalgran May 13, 2019

jwalgran May 14, 2019

kellyi May 13, 2019 •

edited

jwalgran May 14, 2019

kellyi May 13, 2019

jwalgran May 14, 2019

kellyi May 13, 2019

jwalgran May 14, 2019

kellyi commented May 13, 2019

kellyi commented May 13, 2019

kellyi commented May 13, 2019

jwalgran commented May 14, 2019

kellyi left a comment

jwalgran commented May 14, 2019

Add an API parameter and UI to filter facility list items by status #507

Add an API parameter and UI to filter facility list items by status #507

Conversation

jwalgran commented May 8, 2019 • edited

Overview

Demo

Testing Instructions

Checklist

jwalgran commented May 8, 2019

kellyi commented May 9, 2019 • edited

jwalgran commented May 9, 2019 • edited

jwalgran commented May 10, 2019

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 May 13, 2019 • edited

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 May 13, 2019

kellyi commented May 13, 2019

kellyi commented May 13, 2019

jwalgran commented May 14, 2019

kellyi left a comment

Choose a reason for hiding this comment

jwalgran commented May 14, 2019

jwalgran commented May 8, 2019 •

edited

kellyi commented May 9, 2019 •

edited

jwalgran commented May 9, 2019 •

edited

kellyi May 13, 2019 •

edited