Skip to content
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

Update admin UI for dataset config changes #2113

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Dec 22, 2022

Closes #1766

Code Changes

  • Update typescript types to include connection/dataset config types
  • Update slices to stop using deprecated endpoints and using the new endpoints as described here
  • We now have to do two requests to update the datasets associated with a connection—to upsert on datasets, then to patch the datasetconfig
  • Small fix to the demo dataset included in demo_resources
  • Cypress tests

Steps to Confirm

  • Navigate to "Privacy Requests" then "Connection Manager"
  • Either click "Create new connection" or open the ellipsis menu on an existing connection and click "Configure"
  • Open the "Dataset configuration" tab
  • Either make edits to the existing yaml, or paste in a dataset yaml (note: if copying from demo_resources, use the one on this branch, and remove the top level dataset field)
  • Save and verify the dataset updated and that when you open the connection you see the right dataset(s)

Pre-Merge Checklist

Description Of Changes

🚨 Important caveats 🚨

When editing a dataset configuration, we cannot currently handle adding or deleting a dataset in a very good way. Consider:

My mongo connection has two dataset configurations with fides_keys one and two. The first time we save this, they get saved with dataset_pairs of

[
  { fides_key: 'one', ctl_dataset_fides_key: 'one' }, 
  { fides_key: 'two', ctl_dataset_fides_key: 'two' }
]

Now to delete one of these, we go into the yaml editor and delete the dataset with a fides_key of two. In order to do this, we'd have to call a DELETE and wouldn't be able to do a normal upsert. That's not the worst, but things get hairy because the fides_keys themselves are editable. So I could change two to three, but then should we be sending a pair of {fides_key: 'two', ctl_dataset_fides_key: 'three'} or {fides_key: 'three', ctl_dataset_fides_key: 'three'} (i.e. the user cannot, in the current UI, view the config fides_key, only the ctl_dataset_fides_key)

Adding has a similar problem, where we can add a fides_key: 'three', ctl_dataset_fides_key: 'three'} but it starts to involve keeping track of before and after. Even switching order is difficult, since if you have the yaml field with datasets one and two you start off with:

[
  { fides_key: 'one', ctl_dataset_fides_key: 'one' }, 
  { fides_key: 'two', ctl_dataset_fides_key: 'two' }
]

And then let's say you switch the order of the dataset yaml blocks. Should the new pair be

[
  { fides_key: 'one', ctl_dataset_fides_key: 'two' }, 
  { fides_key: 'two', ctl_dataset_fides_key: 'one' }
]

or

[
  { fides_key: 'two', ctl_dataset_fides_key: 'two' }, 
  { fides_key: 'one', ctl_dataset_fides_key: 'one' }
]

It's not possible to tell right now since we don't actually expose fides_key in the UX (perhaps for good reason).

Because there have been other indications that we are moving away from yaml forms, and also because it seems like this form could really be a multi select on datasets instead (see Andrew's comment) of a yaml form, and also because there's already a separate dataset yaml form, I thought it didn't make sense to go down this thorny route at the moment.

I was about to go ahead and implement a multiselect for datasets, but UX started to get weird if we want to keep the yaml form as well and I think would need design help if we do want both (for example, should the yaml text editor update as the user selects different datasets?). I think the thing that might make the most sense is to get rid of the yaml form here, and just use a multiselect on datasets. Then add a link like "don't see your dataset? go to the dataset management page"

All that to say, this PR mostly gets things working as they did before, with the caveat about the dataset pairs above. If we do a multiselect only instead, then that problem should go away

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice increment that preserves the current UI while switching to new endpoints!

The current UI does have some tricky cases as you pointed out. I like your suggestion of adding a followup ticket that gets rid of the yaml form here and adds a multi-select on datasets. The danger with the current UI is that an existing customer might not realize these datasets are unified under the hood, and they might inadvertently overwrite an existing dataset.

One weird thing on the backend, a CTL Dataset can be created with a null default_organization or a null data_qualifier, but the Fideslang Dataset schema requires those fields to exist. So if a CTL Dataset doesn't have that, it will error when we try to link it to the DatasetConfig. Different ways we could go with fixing this - either we make them required for CTL Datasets, or we make them not required for Fideslang Datasets.

@allisonking
Copy link
Contributor Author

One weird thing on the backend, a CTL Dataset can be created with a null default_organization or a null data_qualifier, but the Fideslang Dataset schema requires those fields to exist. So if a CTL Dataset doesn't have that, it will error when we try to link it to the DatasetConfig. Different ways we could go with fixing this - either we make them required for CTL Datasets, or we make them not required for Fideslang Datasets.

Ah yes, I noticed that too when I was trying to fix the data qualifier stuff. I'm guessing the upsert endpoint needs to do some validation that it isn't doing... Actually, this is kind of familiar: https://github.com/ethyca/fidesplus/pull/197#pullrequestreview-1139256078

@pattisdr
Copy link
Contributor

Reviewing now @allisonking!

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding all the great cypress tests!

@allisonking allisonking merged commit 87197f7 into fidesops_1763_stop_storing_datasetconfig_dataset Dec 23, 2022
@allisonking allisonking deleted the aking/1766/ui-dataset-config branch December 23, 2022 20:53
@pattisdr pattisdr linked an issue Dec 23, 2022 that may be closed by this pull request
@pattisdr pattisdr mentioned this pull request Jan 17, 2023
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Admin UI to support new dataset config changes
2 participants