-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Connector dataset dropdown #2162
Connector dataset dropdown #2162
Conversation
Failing test seems unrelated? |
yep this is unrelated @allisonking, I saw this on my last |
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.
This feels a little buggy to me in places. This branch isn't up-to-date with unified-fides-resources
that has the improved upsert validation, but that is handy here so it's easy to see where we might add better error handling:
- Start from scratch, delete datasetconfigs, connectionconfigs, and ctl_datasets. Refresh. It makes sense that there's no dropdown to connect my first connection config to an existing dataset, but the save button stacked on the cancel button looks off
error_handling_patch_datasetconfig.mov
- Editing an existing dataset that was good to something invalid (add bad data category) also puts the UI in a weird state
- When saving existing dataset yaml you've added a helpful confirmation that you're about to overwrite a dataset. This is not necessarily true if the user is also editing the
fides_key
.
Below, the only edit I made was to changepostgres_example_test_dataset
fides_key toupdated_fides_key
. This will create a newctl_dataset
titledupdated_fides_key
, and link it to the DatasetConfig.
- When configuring an existing dataset in the connector workflow, it took me awhile to figure out how to edit directly. I needed to unselect the current dataset.
Thanks for the thorough testing @pattisdr ! 🙏
2 and 3. I'm having trouble reproducing these—I just merged in
|
clients/admin-ui/src/features/datastore-connections/add-connection/forms/YamlEditorForm.tsx
Outdated
Show resolved
Hide resolved
OK after some more digging I found what bad request is causing this: it's if a To reproduce, you could force the backend to throw an error, for example:
This mimics what would happen if a user had a bad dataset already saved. |
clients/admin-ui/src/features/datastore-connections/add-connection/forms/YamlEditorForm.tsx
Outdated
Show resolved
Hide resolved
Thanks for such thorough testing! Should be ready for another review now 🤞 |
wonderful, testing now! |
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.
OK this UI is feeling pretty good now and holding up under a lot of scenarios I'm testing. Thanks for your nice changes here! There's just a couple of things I saw on this last round but I think it's about ready to go.
clients/admin-ui/src/features/datastore-connections/add-connection/DatasetConfiguration.tsx
Show resolved
Hide resolved
clients/admin-ui/src/features/datastore-connections/add-connection/forms/YamlEditorForm.tsx
Outdated
Show resolved
Hide resolved
@allisonking one more thing I just thought of I forgot to test: creating saas connectors. One big difference with saas connectors is that we create them from a template. After the user enters their connector parameters and clicks save, the connection config, dataset config, and dataset are all created in the background for them. There's a small thing where it's hard to save the dataset when you create multiple saas connectors.
Screen.Recording.2023-01-12.at.12.35.55.PM.mov |
This allows us to invalidate the cache on datasets from within the datastore connection slice.
Oh man, that's a good catch I never would have looked for. This should fix it. The change really should have only been the one line (to add "Datasets" to the list of resources in the cache to be invalidated when a new saas connector is created), but due to some tech debt I had to combine the relevant redux slices to use the same base API first. |
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.
Alright this looks really good to me @allisonking thanks for your hard work here 🏆 Failing test is unrelated like we discussed and I'll look into this after you merge this branch into unified-fides-resources
.
Closes #2114
Code Changes
Steps to Confirm
nox -s test_env
Pre-Merge Checklist
CHANGELOG.md
Description Of Changes
Using the dropdown
Screen.Recording.2023-01-10.at.7.01.39.PM.mov
Using the yaml editor
Screen.Recording.2023-01-10.at.7.04.22.PM.mov