System form/vendor selector fixes

[jpople]

Closes #4413, #4415, #4416, #4417, #4418

Description Of Changes

• "Vendor" field in system form is now disabled when editing a previously-saved GVL system
• "Show suggestions" sparkle button removed; suggestions are autofilled on selecting a vendor
• "Generate data uses" sparkle button removed; data uses are generated on save (previously true for GVL systems, now true for any system with data uses in Compass)
• "Tags" field in system form now populates correctly from dictionary
• System name is now validated and checked for uniqueness before form is saved

Steps to Confirm

• Create a system with a GVL vendor, verify tags are populated and data uses are generated
• Go to "view systems" and edit same system
• "Vendor" field should be disabled
• Create a system with a non-GVL vendor (e.g. "IAB Tech Lab"), verify suggestions populate and data uses (if any) populate on save

Pre-Merge Checklist

• All CI Pipelines Succeeded
• Issue Requirements are Met
• Update CHANGELOG.md

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 13, 2023 4:51pm

[cypress]

Passing run #5224 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge 6a3332f into b0ffcf5...
Project: fides	Commit: 5ad351d0b2 ℹ️
Status: Passed	Duration: 01:20 💡
Started: Nov 13, 2023 5:06 PM	Ended: Nov 13, 2023 5:07 PM

Review all test suite changes for PR #4420 ↗︎

[adamsachs]

@jeremy Pople this is looking good from my testing locally! to me it looks like all the functionality is covered. a few notes:

• if i've selected a vendor and gotten my form auto-filled, but don't save, and then try to go back to the vendor field and re-type another vendor name, that vendor selector box seems to get pretty laggy for me. it's a bit had to pin down exactly the conditions that it starts getting laggy, but i basically am just trying to put in different vendor names repeatedly and select different vendors for autofill.
  • this could very well have to do with the fact that i'm running the "dev" admin UI (just turbo run dev from ./clients/admin-ui ) locally and who knows what state my local env is in generally. so i'd take this with a grain of salt, this may be a non-issue on production deployments
  • even if this doesn't have to do with any local considerations for me, i still think it's probably acceptable UX! but it's worth getting some more use on this to see what others think. i think it'd at least be worth creating a follow up if this behavior is indeed repeatable on deployed production instances, it definitely feels like it could get snappier
• now that we explicitly block/flag a repeated System name, and on GVL vendors we also lock this field, what it means is that we basically cannot create more than 1 system for a given GVL vendor. previously, the workaround to allow this was that the user could go to the system name field and manually edit the name to make it unique, while still keeping the system based on the same vendor.
  • i discussed briefly with @rsilvery and it seems like that's probably an OK constraint for us to enforce, but we haven't had this constraint up until this point, i.e. we did allow repeat systems for a given GVL vendor. so we should probably make a note generally that we're deciding to basically have this constraint

somewhat of an aside - just curious, what are we basing the system name uniqueness check on? we don't technically have a uniqueness constraint on system name in the application, it's on the fides_key which we happen to be generating (i think on the FE?) based on the system name. i think that is just an implementation technicality that doesn't have any significant consequences, but it's worth keeping that in mind - to think about any potential edge cases/oddities that may arise from that... 🤔

[TheAndrewJackson]

This is looking good!

Just one note, I think I may have found a bug. I'm seeing the GVL notice on the system info form for AC systems.

Is this expected or a regression? I confirmed that the lockedForGVL is being set to true but idk exactly where.

[jpople]

@adamsachs The system name uniqueness is a new constraint, yeah-- it comes from designs here. This sprint the vendor selector is going to be merged with the system name input, with the ability to split them (and, accordingly, make two systems with the same vendor but different names) being locked behind a beta flag-- see the ticket.

[jpople]

@TheAndrewJackson I'm not able to reproduce this-- did you take any other specific steps before it or anything?

[adamsachs]

@adamsachs The system name uniqueness is a new constraint, yeah-- it comes from designs here. This sprint the vendor selector is going to be merged with the system name input, with the ability to split them (and, accordingly, make two systems with the same vendor but different names) being locked behind a beta flag-- see the ticket.

OK, got it! thanks for that context. It's not a huge deal, but if we're deciding that system names should be unique, then we should probably also implement that constraint on the BE, ideally down to the DB level, to ensure consistency. but we can tackle that in a followup - i've created https://ethyca.atlassian.net/browse/PROD-1368 so that we at least do not lose track of it.

unrelated to this issue, but back to the more pressing thing on my comment - has anyone else noticed the lag/sluggishness when entering different vendor names repeatedly?

[TheAndrewJackson]

@TheAndrewJackson I'm not able to reproduce this-- did you take any other specific steps before it or anything?

@jpople I just pulled latest and the issue didn't happen. It must have been something weird on my machine

[TheAndrewJackson]

[jpople]

@adamsachs The lag is present on mine too. It was similar on the previous vendor selector as well; looking at the profiler (which, granted, I am not the best at understanding) I think it has to do with the way we have the DictSelectionInputs implemented, where each one needs to call a hook to get its new value when the suggestions status changes. It's possible that that could be simplified, especially now that the suggestions show automatically when a vendor is selected and we no longer really have to handle "hide suggestions"? I would need to spend some time looking into it, but @TheAndrewJackson might have thoughts also.

[TheAndrewJackson]

@adamsachs The lag is present on mine too. It was similar on the previous vendor selector as well; looking at the profiler (which, granted, I am not the best at understanding) I think it has to do with the way we have the DictSelectionInputs implemented, where each one needs to call a hook to get its new value when the suggestions status changes. It's possible that that could be simplified, especially now that the suggestions show automatically when a vendor is selected and we no longer really have to handle "hide suggestions"? I would need to spend some time looking into it, but @TheAndrewJackson might have thoughts also.

FWIW, fields that have been linked to dictionary suggestions have been laggy since this feature was originally introduced. @jpople is on the right track for what's causing the issue although I haven't found the root cause yet.

@adamsachs if you're comfortable with leaving the current lag in place, I think it would be worth creating a follow up issue to refactor the dictionary suggestions as Jeremy described above. I think trying to address it in this PR and make it into the release will be difficult.

[adamsachs]

@adamsachs The lag is present on mine too. It was similar on the previous vendor selector as well; looking at the profiler (which, granted, I am not the best at understanding) I think it has to do with the way we have the DictSelectionInputs implemented, where each one needs to call a hook to get its new value when the suggestions status changes. It's possible that that could be simplified, especially now that the suggestions show automatically when a vendor is selected and we no longer really have to handle "hide suggestions"? I would need to spend some time looking into it, but @TheAndrewJackson might have thoughts also.

FWIW, fields that have been linked to dictionary suggestions have been laggy since this feature was originally introduced. @jpople is on the right track for what's causing the issue although I haven't found the root cause yet.

@adamsachs if you're comfortable with leaving the current lag in place, I think it would be worth creating a follow up issue to refactor the dictionary suggestions as Jeremy described above. I think trying to address it in this PR and make it into the release will be difficult.

ok yup, thanks both for the context here. i just wanted to get that triaged and understood. especially if that's not a regression, then this likely doesn't need to be resolved before the release, i understand it's not at all a simple problem to solve.

i do think that the new behavior of adding the suggestions automatically perhaps makes this lag a bit more obvious/evident to the end user - at least that was my off-the-cuff impression. but i'm good with merging - let's go ahead and get this into the release branch and rc environment to get more eyes on it. and yeah, a follow up ticket seems like a good idea regardless @TheAndrewJackson !