Implement collection attacher component. by dvail · Pull Request #3599 · stackrox/stackrox

dvail · 2022-10-27T17:34:32Z

Description

Adds a UI component that allows users to attach and detach "embedded" collections from a collection configuration.

When the initial collection data is loaded, a secondary request will fetch metadata for all of the embedded collections of the initial collection. These embedded collections will make up the attached list in the widget, and are not paginated.

Once this data is loaded and the form is rendered, the collection attacher component will attempt to fetch a page of detached collections (everything else in the system). Since this secondary request may include collections that are already part of the attached list, we need to filter the results client side. If this filtering results in less than half a page load, we fetch an additional page worth of data. The idea being we want to prevent situations where "View more" returns a full page sometimes, and then very few another time (possibly even zero), which could lead to looking like buggy behavior.

When the user enters a search term, the locally cached list of detached collections is cleared out and the page counter is reset. This will require refetching the detached collections, now filtered by search term. This could cause some wasted requests, but the alternative of keeping the cache and filtering client side had larger problems. If we don't clear cache and all data has not yet been loaded, there would be two ways to handle the follow up "view more" requests:

Pass the search term along with the request. This would have good performance, but would leave holes in the local cache data if the user then removed the search term. We would need to clear the local cache anyway or be unable to present the full data set to the user.
Don't pass the search term along with the request, and filter the results client side. This would build up the local cache cleanly and show the user the full, accurate set of data. The downside here is that a search term with few, or zero, matches would create a string of requests that load all of the collections from the server. Worst case would be multiple sequential requests with a loading spinner that eventually resulted in no new data.

I've documented some potential workarounds and optimizations for this in the code that we can do as follow ups if needed.

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

Visit the collections page, and select a collection that has embedded collection data:

Clicking the "Attach" button should remove items from the bottom list and place them in the top list:

Clicking the "Detach" button should move items from the top list to the bottom list:

Entering a search value should filter the top and bottom lists by collection name, case insensitive:

Clearing the filter should return the list to its previous state:

To demo the "View more" behavior, I manually changed the pageSize and minimum threshold to be 1.

If there are more results on the server, a "View more" button should be displayed.

Clicking this button should load another page of results:

Continuously clicking this button will load more and more results from the server.

Note that when we get to the fifth item, two network requests are sent instead of one. This is because the first request contains only "Notable deployments", which is already present in the attached list. Since this would result in no new items in the detached collections list, another request is sent.

When all results have been loaded by the server, the view more button disappears.

Test that layout is usable on small screens:

openshift-ci · 2022-10-27T17:34:36Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

dvail · 2022-10-27T17:35:15Z

Current dependencies on/for this PR:

master
- PR Implement collection attacher component. #3599 👈
  - PR Implement read-only view of individual collection page #3636
    - PR Add collection request generating function #3637
      - PR Add autocomplete for name selector dropdowns #3676
      - PR Implement create/clone/edit functionality for collection form #3644

This comment was auto-generated by Graphite.

dvail · 2022-10-28T12:41:27Z

ui/apps/platform/src/Containers/Collections/CollectionsTable.tsx

        () =>
            debounce(
-                (value: string) => setSearchFilter({ Collection: value }),
+                (value: string) => setSearchFilter({ 'Collection Name': value }),


This is an unrelated change, but not worth a separate PR.

The BE team is using Collection Name instead of Collection for this search label.

dvail · 2022-10-28T12:42:46Z

ui/apps/platform/src/utils/searchUtils.ts

    sortOption: ApiSortOption,
-    page: number,
-    pageSize: number
+    page?: number,


Most backend APIs can support un-paginated requests, so we don't need to enforce that these values are set.

In this specific use case, we need to request all attached collections by id without pagination.

ghost · 2022-10-28T16:47:24Z

Images are ready for the commit at 462c127.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.72.x-453-g462c127658.

pedrottimark · 2022-10-28T18:29:02Z

ui/apps/platform/src/Containers/Collections/CollectionAttacher.tsx

+                    className="pf-u-align-self-flex-start"
+                    variant="secondary"
+                    onClick={() => fetchMore(search)}
+                    isLoading={fetchMoreLoading}


Most boolean state variables have is or occasionally has prefix:

isLoading={isFetchMoreLoading}

A grammatical alternative in this case which I mention, not necessarily recommend: isFetchingMore

I think the grammatical alternative reads better in this case, updated the prop name.

pedrottimark · 2022-10-28T18:42:39Z

ui/apps/platform/src/Components/PatternFly/BacklogListSelector.tsx

+            }
+        >
+            {items.length > 0 ? (
+                <TableComposable aria-label={label} variant={TableVariant.compact}>


Can you inspect table cells to see why centered alignment in responsive view?

Here is an example from /main/access-control/permission-sets for comparison:

pedrottimark · 2022-10-28T18:47:51Z

ui/apps/platform/src/Components/PatternFly/BacklogListSelector.tsx

+                    <Badge className="pf-u-ml-sm" isRead>
+                        {items.length}
+                    </Badge>


Thoughts about badge, which I was happy to see after designs had moved away from it for a while:

Attached collections: Does search filter apply to this table? Length seems meaningful only if not filtered.

Detached collections: Length seems meaningful only if !hasMore and maybe only without search filter.

Agreed that the badge has limited utility when the full dataset is not present, and the one case where it would be meaningful (unfiltered attached collections) is likely going to be a small enough number that the badge is fairly unimportant.

I left the ability to show the badge in the underlying component though, since if we get reuse out of it there is the possibility we use it against data that is complete and unfilterable.

dvail · 2022-10-28T19:34:59Z

ui/apps/platform/src/Containers/Collections/types.ts

    return isByLabelField(selector.field);
 }

+export type CollectionSlim = { id: string; name: string; description: string };


Note to self - we can just use the full type since we are querying the server for the full Collection object here.

Replace CollectionSlim with Collection

Instead of Collection, I just passed CollectionResponse[] directly since the parsing of the response into a stricter data type is not needed for this piece of the UI.

pedrottimark · 2022-10-31T20:09:06Z

ui/apps/platform/src/Containers/Collections/CollectionAttacher.tsx

+            <Button variant="link" className="pf-u-pl-0" isInline>
+                {name}
+            </Button>


Asking to confirm my understanding or misunderstanding: Button element

in form: will have onClick callback to view collection in modal?

in modal: will have component={LinkShim} href={href} props to view collection in page?
If yes, then are target="_blank" rel="noopener noreferrer" props also needed?

Yes that is exactly correct. In a follow up I'll likely move the CollectionAttacher.tsx#selectorListCells up to CollectionFormPage and pass through as props CollectionFormPage -> CollectionForm -> CollectionAttacher to accommodate the two use cases for the main page table and the modal table.

pedrottimark · 2022-10-31T20:15:18Z

ui/apps/platform/src/Containers/Collections/CollectionAttacher.tsx

+                aria-label="Search by name"
+                placeholder="Search by name"


Although I see Search Google and Gmail, Filter seems more common for placeholder in UI.

Fair, there are multiple occurrences of Filter in similar contexts but this looks to be the first case where we use Search. This is kind of a hybrid case, as we are both searching and filtering the two lists with a single input, but I'll change for the sake of consistency.

pedrottimark · 2022-10-31T20:18:42Z

ui/apps/platform/src/Containers/Collections/CollectionAttacher.tsx

+                selectedOptions={selectedOptions}
+                deselectedOptions={deselectedOptions}
+                onSelectItem={({ id }) => attach(id)}
+                onDeselectItem={({ id }) => detach(id)}


An observation, not a change request, that change in terminology from attach/detach to select/deselect is what I would expect for a reusable component, what do you think are pro and con:

pro: how likely to reuse in a context where application level is other than attach/detach

con: slight substitution by reader and possible effect on Find in Files

Hmm this is a good question. My gut feeling is that the default terminology for using this in other contexts would be "select/deselect". This component is similar to the dual list selector PF component which uses terms that are fairly close. I think the attach/detach are specific, but not exclusive, to the semantics behind Collections.

That said it doesn't feel great having a disconnect between the props and the only use case. (The useSelectToggle() hook suffers from this mismatch as well.) I'm OK with changing the terminology or leaving as-is. Do you think people are more likely to Find in Files or Ctrl+Click to navigate around this code in the future?

Your choice. You caught me: I often use Find in Files by mental muscle memory from less capable editors.

pedrottimark · 2022-10-31T20:19:48Z

ui/apps/platform/src/Containers/Collections/CollectionAttacher.tsx

+                selectButtonText="Attach"
+                deselectButtonText="Detach"
+            />
+            {fetchMoreError && (


First impression gave me a much needed smile: fetch more error, please ;)

Hahah now I can't read it any other way! Seriously though, do you think there is enough chance for confusion that we should change this to fetchingError?

It just hit me funny at that moment. In first reading a few days ago, it seemed nicely consistent.

pedrottimark

Approving and am available as needed if there is anything more to discuss.

openshift-ci · 2022-11-01T14:06:50Z

@dvail: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/gke-postgres-ui-e2e-tests	`462c127`	link	false	`/test gke-postgres-ui-e2e-tests`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/work-in-progress label Oct 27, 2022

github-actions bot added the area/ui label Oct 27, 2022

dvail force-pushed the dv/ROX-12597-implement-attach-collections-table branch 3 times, most recently from 68f4a49 to e3de69a Compare October 28, 2022 12:39

dvail commented Oct 28, 2022

View reviewed changes

dvail force-pushed the dv/ROX-12597-implement-attach-collections-table branch from e3de69a to d6ee87a Compare October 28, 2022 13:04

dvail marked this pull request as ready for review October 28, 2022 16:15

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 28, 2022

dvail force-pushed the dv/ROX-12597-implement-attach-collections-table branch 2 times, most recently from c89253d to 9ea40fb Compare October 28, 2022 17:40

dvail requested review from bradr5 and pedrottimark October 28, 2022 17:40

pedrottimark reviewed Oct 28, 2022

View reviewed changes

dvail commented Oct 28, 2022

View reviewed changes

dvail added the epic/resource-collections rox-11646 label Oct 31, 2022

dvail added 5 commits October 31, 2022 08:05

Implement collection attacher component.

b540de0

Replace CollectionSlim with direct reference to CollectionResponse type

6bebffe

Make badge rendering false by default in backlog list selector component

dfa136b

Rename of boolean prop

25da4a1

Fix text centering on table buttons

2f9f80b

dvail force-pushed the dv/ROX-12597-implement-attach-collections-table branch from 9ea40fb to 2f9f80b Compare October 31, 2022 16:28

This was referenced Oct 31, 2022

Implement read-only view of individual collection page #3636

Merged

Add collection request generating function #3637

Merged

pedrottimark reviewed Oct 31, 2022

View reviewed changes

pedrottimark approved these changes Oct 31, 2022

View reviewed changes

Swap search for filter

462c127

dvail merged commit 7a59767 into master Nov 1, 2022

dvail deleted the dv/ROX-12597-implement-attach-collections-table branch November 1, 2022 14:18

This was referenced Nov 1, 2022

Implement create/clone/edit functionality for collection form #3644

Merged

Add autocomplete for name selector dropdowns #3676

Merged

ivan-degtiarenko pushed a commit that referenced this pull request Nov 5, 2022

Implement collection attacher component. (#3599)

84b4d62

Conversation

dvail commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

openshift-ci bot commented Oct 27, 2022

Uh oh!

dvail commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 28, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedrottimark Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvail Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvail Oct 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrottimark Oct 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrottimark left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dvail commented Oct 27, 2022 •

edited

Loading

dvail commented Oct 27, 2022 •

edited

Loading

ghost commented Oct 28, 2022 •

edited by ghost

Loading

pedrottimark Oct 28, 2022 •

edited

Loading

dvail Oct 31, 2022 •

edited

Loading

dvail Oct 28, 2022 •

edited

Loading

pedrottimark Oct 31, 2022 •

edited

Loading