Add autocomplete for name selector dropdowns by dvail · Pull Request #3676 · stackrox/stackrox

dvail · 2022-11-03T15:45:43Z

Description

This adds the ability to autocomplete text input on the dropdowns for by name rule selectors only.

Side note

As the collection form grows, there is an increasing amount of prop drilling going on. This PR passes the collection value from CollectionForm -> RuleSelector -> [ByNameSelector, ByLabelSelector] and RuleSelector -> [ByNameSelector, ByLabelSelector] -> AutocompleteSelector components now all need access to the entityType. I tried playing around with storing these values in a React context at the RuleSelector level, but wasn't convinced it was any cleaner. I also took a quick look at how to make these more composable, but that seemed to just lift the verbosity up for no apparent gain. Maybe the components are just inherently coupled, but I'm open to suggestions on how to make this component tree easier to use.

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

Go to a collection page and select "Deployment with name matching" from the rule dropdown. Focus the text input box for the name value and a list of suggestions should appear.

Begin typing text into the input. A short delay after the last keystroke the items should update to only include values that match the entered text. Additionally, the last option should be the ability to "Create", or use, the value of the entered text directly.

Clicking on one of the text suggestions should make the value of the input box match the item that was selected.

Alternatively, clicking the "Create" option will keep the user entered value of the text in the dropdown.

The same behavior should occur for both Namespace and Cluster resources. These suggestions should have the correct resource icons to match the resource types:

At this point, label key and value dropdowns should not attempt to autocomplete text. (This is TBD whether or not we will implement this for MVP.)

openshift-ci · 2022-11-03T15:45:47Z

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-11-03T15:46:00Z

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-11-04T20:34:55Z

ui/apps/platform/src/Containers/Dashboard/hooks/useRestQuery.ts

        const { request, cancel } = cancellableRequestFn();

        setError(undefined);
+        setLoading(true);


bradr5 · 2022-11-07T17:36:48Z

Would be nice if there was a way to easily compare what was in parser.ts to converter.ts from within here

bradr5 · 2022-11-07T17:45:18Z

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

-            All: {},
+            All: { type: 'All' },
            ByName: {
+                type: 'ByName',


Not asking you to change anything but was curious of your opinion on having these 3 types (All, ByName, ByLabel) defined as constants?

This is something I go back and forth on, but lately I've been just writing the string directly in cases where the code is well-typed enough that a mistyped string would result in a compile time error. This might be worth bringing up during one of the weekly calls though to see if the team has a strong preference one way or another.

My initial feeling is the the pros/cons for using a constant here are:
Pros

changing the property name only requires a change in one place

easier to Ctrl+Click to see where the relevant constants are coming from

Cons

more verbose (subjective)

changing the expected value doesn't result in compile errors at each place the string is used. I could see an argument for this being a trail of breadcrumbs to get eyes on relevant code during a refactor.

Ah - and didn't realize there were too many changes in the parser for git to detect it as a rename instead of an entirely new file... I can swap the name back if we think it is important to retain that history at this point.

TypeScript string enumeration or discriminated union seem fluent as literal strings.

If we felt the need to encapsulate, then Redux action creator functions are a precedent to adapt.

bradr5 · 2022-11-07T18:23:11Z

I can see why you avoided context here, the benefit doesn't feel like it would outweigh the complexity here.

pedrottimark · 2022-11-07T18:32:49Z

ui/apps/platform/src/Containers/Collections/RuleSelector/AutoCompleteSelect.tsx

    validated: ValidatedOptions;
    isDisabled: boolean;
+    autocompleteProvider?: (search: string) => CancellableRequest<string[]>;
+    entityType: SelectorEntityType;


If the only reason for entityType prop is for ResourceIcon element, then instead of context versus props, the question is cost in code complexity versus benefit in user experience. Besides hierarchical organization deployments, namespaces, clusters, and form element labels, it seems likely that naming conventions for the 3 entity types are quite different.

On the positive side of the coin, bravo for composition via autocompleteProvider prop.

If the only reason for entityType prop is for ResourceIcon element, then instead of context versus props, the question is cost in code complexity versus benefit in user experience. Besides hierarchical organization deployments, namespaces, clusters, and form element labels, it seems likely that naming conventions for the 3 entity types are quite different.

I'm not quite sure I understand.

The component did not need entityType prop to fetch the data, because encapsulated in autocompleteProvider function, correct?

Unless my eyes missed something in the changed lines, the reason to add entityType prop is to render different resource icon?

Maybe I misunderstood the comment in the description about prop drilling;

now all need access to the entityType

If this is an example which motivated the comment, then my counter question is instead of how to deal with increasing coupling between components, whether this change is pushing beyond a limit of how much coupling is too costly?

It looks like the component was potentially reusable to become independent of container.

Ah ok got it, yeah good point. Early on I was thinking the RuleSelector component would have been a good candidate to extract to Components/PatternFly if it had uses outside of this container. I think a lot of the tight coupling exists across everything under the ./RuleSelector directory since they were originally all one component that I split apart for readability.

I think we can make this reusable again if we need to by pulling the autocomplete function up, and maybe making the error pieces less Formik specific.

What caught my eye is asymmetry between entityType encapsulated for data but not for rendering.

If such specific rendering is impossible to avoid, then a more symmetrical solution is prop for option rendering callback function.

pedrottimark · 2022-11-07T18:37:55Z

ui/apps/platform/src/services/CollectionsService.ts

        axios
-            .get<{ values: string[] }>(`${collectionsAutocompleteUrl}?${params}`, { signal })
+            .post<{ values: string[] }>(
+                `${collectionsAutocompleteUrl}`,


Not a blocker to merge, does change in params let you replace template literal with variable?

+1 Hmm it seems like this is something prettier would automatically fix. I'll push an update.

pedrottimark · 2022-11-07T18:42:21Z

ui/apps/platform/src/Containers/Collections/RuleSelector/ByNameSelector.tsx

                                    : ValidatedOptions.default
                            }
                            isDisabled={isDisabled}
+                            autocompleteProvider={onAutocomplete}


A question for understanding, not necessarily a request for change. The difference between prop name and prop value function name.

Leftover from initial draft of code?

Intended to communicate meaning to reader?

A little bit of both, but probably more of the former. This was something in the back of my head but since you called it out I think it is worth fixing now. I'll update all to autocompleteProvider, since onAutocomplete implies the function is a callback, and this is being passed to a declarative data fetching API.

ghost · 2022-11-07T21:15:01Z

Images are ready for the commit at 9209142.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.72.x-587-g9209142d69.

openshift-ci · 2022-11-10T21:46:12Z

@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	`ca1c291`	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.

7ffc6be ROX-13368: Skip failing nongroovy tests on PG (#3721) bbdd7a0 Bump github.com/gofrs/uuid from 4.3.0+incompatible to 4.3.1+incompatible (#3642) 1f253f2 Bump github.com/google/certificate-transparency-go from 1.1.3 to 1.1.4 (#3543) d434c8d [ROX-13030] : Add delete collection API endpoint and service implementation (#3648) f062c21 Dashrews/ROX-13253 wait for central-db to come back after bounce and allow FATAL connection lost error (#3537) edc1174 CI: Fill the gaps for https://testgrid.k8s.io/ (#3715) 86d7c54 ROX-13231: use passed context when non-postgres (#3540) 9093195 Add less specific type for BE collection response string (#3728) 5abb652 Only enable ROX_OBJECT_COLLECTIONS feature flag during gke-postgres-ui-e2e job (#3727) 4f64cd1 Add centralDBOnly mode in render (#3707) d67bbe5 Dashrews/ROX-13082 UUID searcher and common updates to set allow use of postgres UUID PR 1 of 4 (#3679) 6f829d5 ROX-13259: graphInit called during init time (#3705) f3bc50d ROX-13380: Conditional rendering edges for deployments and namespaces (#3641) 3764476 ROX-12319: implement smoke test step with groovy test filter (#3220) f202fd4 ROX-11826: Disable kernel support package uploads for managed central (#3661) 61f03dc ROX-11101: Remove deprecated resources from central (#3115) e6aa6d7 ROX-11101: Restore Role permission in UI (#3428) 3203e04 ROX-11101: Remove deprecated resources (#3036) a35f41e Bump golang.org/x/sys from 0.1.0 to 0.2.0 (#3733) 4120524 Bump snakeyaml from 1.29 to 1.33 in /qa-tests-backend (#3732) e1785c0 Bump github.com/coreos/go-systemd/v22 from 22.4.0 to 22.5.0 (#3724) 870df4a Bump google.golang.org/api from 0.101.0 to 0.102.0 (#3723) 721454c Generalize User-Agent setup (#3672) 6a11bf0 Bumps collector version to 3.11.x-145-gc345f72f5e (#3736) ec5d343 [ROX-12923] Walk retries - remainder work (#3729) 40f3d43 ROX-13440: Replace ambiguous central with sensor in networkGraph integration test (#3730) 281ed22 Bump groovy-xml from 2.5.18 to 2.5.19 in /qa-tests-backend (#3741) 49d1651 Bump github.com/prometheus/client_golang from 1.13.1 to 1.14.0 (#3742) b5544aa Bump cloud.google.com/go/storage from 1.27.0 to 1.28.0 (#3743) 9c61e53 ensure CVSS is present for istio vulns (#3706) ae29d52 ROX-13452: don't always clobber scoped ctx when non-postgres (#3748) 517bf05 ROX-13261: DryRunUpdate on collection datastore (#3687) baf7654 ROX-13378: Group new resources with deprecated in UI (#3690) 569922f ROX-13421: Enable roxctl netpol generate and add tech-preview messages (#3740) 2465fc5 Dashrews/ROX-13082 UUID generator templates PR 2 of 4 (#3681) c093c68 Bump slack-api-client from 1.20.2 to 1.27.0 in /qa-tests-backend (#3752) 2c860bb Bump ubi8-micro from 8.6 to 8.7 in /operator (#3751) 80eb04c Make deploy.sh and deploy-local.sh pass shellcheck (#3582) 2182b43 Dashrews/ROX-13082 UUID test updates PR 3 of 4 (#3694) 6dc6ca5 [ROX-13403] : Fix node -> topVuln sub resolver bug when node cves is empty (#3689) 1b21361 Move integration tests for page title from general to specific containers (#3675) e1a9f31 Bump google.golang.org/api from 0.102.0 to 0.103.0 (#3773) a05ea31 Bump golang.org/x/crypto from 0.1.0 to 0.2.0 (#3772) 65ddf4f ROX-12824: Add roxctl commands to generate Central DB bundle (#3602) c3f1e2f Remove obsolete authProviders request for Integrations page (#3759) 7ccd54d Dashrews/ROX-13082 UUID protos generated PR 4 of 4 (#3698) 9ab5c8f cleanup image digest utilities (#3764) 187ed44 ROX-11931: Convert junit failure artifacts to Slack attachments (#3438) b5d8790 ROX-13432: leaning up unused code copied/pasted from topology demo (#3750) ab05bfc Refactor collection form page for better composition (#3744) c5562f7 Remove babel devDependencies in ui-components (#3761) 2b90b3a Extract collection form from drawer wrapper layout (#3745) a779fc9 [ROX-12625 + ROX-13032] : Add GetCollectionCount and UpdateCollection endpoints and services (#3749) e77f0da Upgrade cypress 11.0.0 devDependencies in ui (#3760) a3fba94 ROX-13068: Use real data for deployment details (#3688) 4c7d90e ROX-12617: Collection to search query converter (#3683) 3e98aec ROX-13067: fill out port configurations section of deployment details (#3714) a48de36 ROX-12835: Add support for NodeScanV2 to Sensor (#3533) 30c5dc7 ROX-13466: Fix deletion of groups with empty properties (#3756) 5cb2470 Add autocomplete for name selector dropdowns (#3676) b9a75ad ROX-13464 adding flows dropdown in NG (#3763) 3217a67 [ROX-13500] Perform type check for V1 CronJob (#3787) af3790d Remove bulk delete from collections table (#3776) dda123b Add more info in migration log (#3788) 179f0c9 ROX-13502: Remove the circular dependency between cluster datastore init and cscc notifier init (#3790) 029d584 Update SCANNER_VERSION (#3774) cbca57c Bump github.com/ckaznocha/protoc-gen-lint from 0.2.4 to 0.3.0 (#3783) 3613b56 Bump golang.org/x/tools from 0.2.0 to 0.3.0 (#3782) 5fc0a6a Bump github.com/google/go-containerregistry from 0.12.0 to 0.12.1 (#3781) 1d1c687 Bump controller-gen version to 0.10.0 (#3754) c3a5290 Untie documentation link from the product version (#3799) ed822aa use correct package for migration (#3784) 397a0b4 Validate that label keys are valid k8s labels and ensure correct key splitting (#3777) edd1050 Rename variable ScannerGRPCEndpoint to ScannerSlimGRPCEndpoint (#3657) 6662c9f ROX-13378: Access Control page permissions (#3720) b0e73c5 fix Operator reconciliation for external Central DB (#3796) b83bc1f ROX-13505: Fix error log scanning the postgres stat collection (#3795) ca660cb Prevent the collection being edited from displaying in its own embedded list (#3778) 3f7b3fc [ROX-13441][POSTGRES] Propagate context correctly in retries (#3793) e0cbc6f ROX-12839: Update changelog to announce removal of in-product docs (#3805) 696e8bc [ROX-12358] Follow up on vulnerability request proto change (#2851) c4b46d8 Change getCollectionCount endpoint and updateCollection request type 5f2efbc remove make proto-fmt (#3804) 0c75540 Remove os.Std* from roxctl/central (#3758) 25a90de Add ability to view embedded collections in a pop up modal (#3747) 5c1bf81 ROX-13240: fix scanner-slim updates when WebSockets are used (#3704) 1d98577 Add more context to jira notifier logging (#3812) da2fd28 ROX-13031: DryRun Collection API (#3766) 1c418d5 Test data migration code in postgres tests (#3803) ed95b37 Update UI Collection requests for BE compatibiltiy (#3762) 09cc188 ROX-11931: Fix junit-parse install in CI (#3811) d2b01e3 ROX-12814: Disable PolicyFieldsTest on openshift. (#3797) d10ce27 ROX-13345: disable 'missing required registry' aspect on openshift (#3798) 3d22396 Update collector to 3.12 (#3809) 1eb33fb ROX-13347: Modify scope queries to included quoted cluster and nameace names, to allow exact matches instead of erroneous and unintended prefix matches. (#3767) 3811a69 ROX-12621: list collection selectors api (#3806) f6d3f9d Add migration for groups with invalid values (#3789) cc21125 Bugfixes for collection autocomplete (#3816) 7623dec ROX-9350 Use fine-grained host paths for compliance mounts (#2479) b4bf5c2 Fix collector volumeMounts (#3826) 0e9be05 ROX-12953: figure out last 4 versions of sensor automatically (#3611) 459c7ae ROX-12814: Add proper todo for reenabling the test (#3817) 9ee40ff ROX-13523: add isEnabled enum to central db spec (#3815) 535bc72 Replace requestConfig with routeMatcherMap in helper functions for integration tests (#3686) 2b75b61 `gosec` G104: Add `ShouldErr(err)` that returns `err` (#3830) fb1b82f WIP: Introduce nodescan call 35f8a8f WIP: Prepare converter 716144b Moved and renamed fake nodescan tests 4748de7 Introduce real node scanner with conversion functions 5e6d9a8 wip: real scanner 0169868 wip: log results 1438d2c wip: Debug Analyze call b09894c wip: Debug Analyze call 3ceed72 wip: Update and improve debug logs d1669fd Remove copied lib, bump scanner version, add debug 14a3f73 Merge branch 'master' into mm/ROX-12967-real-nodescan fbd0450 Fix style issues 17ccb31 Debug: let both scans finish to see what they return

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

This was referenced Nov 3, 2022

Implement collection attacher component. #3599

Merged

Implement read-only view of individual collection page #3636

Merged

github-actions bot added the area/ui label Nov 3, 2022

dvail mentioned this pull request Nov 3, 2022

Add collection request generating function #3637

Merged

5 tasks

dvail mentioned this pull request Nov 3, 2022

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

Merged

5 tasks

dvail added the epic/resource-collections rox-11646 label Nov 3, 2022

dvail commented Nov 4, 2022

View reviewed changes

dvail marked this pull request as ready for review November 4, 2022 20:35

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

dvail requested review from bradr5 and pedrottimark November 4, 2022 20:35

Base automatically changed from dv/collection-request-generating-function to master November 4, 2022 21:04

bradr5 reviewed Nov 7, 2022

View reviewed changes

bradr5 approved these changes Nov 7, 2022

View reviewed changes

pedrottimark reviewed Nov 7, 2022

View reviewed changes

pedrottimark approved these changes Nov 7, 2022

View reviewed changes

dvail force-pushed the dv/ROX-13316-implement-autocomplete-for-dropdowns branch from bf0ff5d to a1a5bbd Compare November 7, 2022 20:50

dvail force-pushed the dv/ROX-13316-implement-autocomplete-for-dropdowns branch from a1a5bbd to ca1c291 Compare November 10, 2022 20:23

dvail enabled auto-merge (squash) November 10, 2022 20:24

dvail disabled auto-merge November 10, 2022 20:46

(WIP) Add autocomplete for rule selector dropdowns

41d29f3

dvail added 6 commits November 11, 2022 09:15

Correctly pass entity type

b3f60c9

remove coupling between autocompleteselector and entitytype

7b78783

PR feedback

e2664ac

Fix tests and style

6dad251

More test fixing...

2d35289

Fix intermittent test failures by disabling debounce of typeahead

9209142

dvail force-pushed the dv/ROX-13316-implement-autocomplete-for-dropdowns branch from ca1c291 to 9209142 Compare November 11, 2022 16:54

dvail merged commit 5cb2470 into master Nov 11, 2022

dvail deleted the dv/ROX-13316-implement-autocomplete-for-dropdowns branch November 11, 2022 17:40

Conversation

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

Uh oh!

Description

Side note

Checklist

Testing Performed

Uh oh!

openshift-ci bot commented Nov 3, 2022

Uh oh!

dvail commented Nov 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradr5 commented Nov 7, 2022

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 Nov 7, 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!

bradr5 commented Nov 7, 2022

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 Nov 7, 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!

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

Uh oh!

Uh oh!

openshift-ci bot commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dvail commented Nov 3, 2022 •

edited

Loading

pedrottimark Nov 7, 2022 •

edited

Loading

pedrottimark Nov 7, 2022 •

edited

Loading

ghost commented Nov 7, 2022 •

edited by ghost

Loading

openshift-ci bot commented Nov 10, 2022 •

edited

Loading