Classify Dataset UX Updates

[SteveDMurphy]

Closes fidesplus#404

Code Changes

• Return only top 1 suggestion instead of top n in data table
• rename button to state approve classifications
• Update dataset classify heading text
• Update to only approve collection at one time vs entire dataset
• Update from returning the first classification to ensure returning the top 1 in the same way the dropdown does
• Create a common function to get the topClassification
• Exclude Identifiability from rendering as part of the Dataset view (Closes fidesplus#513)

Steps to Confirm

• Start up fidesplus in the background
• Navigate to the admin-ui and start up the frontend using npm run dev
• Classify the fides db using postgresql+psycopg2://postgres:fides@fidesplus-db:5432/fides (it's actually a bit collection-heavy so feel free to use a smaller one you can find in 1Password such as this)
• Click into the dataset and run through each collection. On the last collection, validate the status is updated

Pre-Merge Checklist

• All CI Pipelines Succeeded
• Documentation:
  • documentation complete, PR opened in fidesdocs
  • documentation issue created in fidesdocs
• Issue Requirements are Met
• Relevant Follow-Up Issues Created
• Update CHANGELOG.md

Description Of Changes

Currently, these changes feel like a lot is left to be desired as far as the UX for classification. The approve classification button is not informative and should likely change on click or load the next collection to be completed.

[codeclimate]

Code Climate has analyzed commit 0218970 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

View more on Code Climate.

[SteveDMurphy]

As an early criticism, check out this Loom -> https://www.loom.com/share/963dbf44bd044668aa57da96875badb6

It feels like the UX doesn't seem to quite align with what we want here, some more work around the button to approve classifications is likely required. I'd love to see something around loading the next collection to be reviewed as well 🤷🏽

[allisonking]

I think the UX is currently weird because toasts seem not to be working even on main. will investigate further, but that's not your PR's fault!

[allisonking]

looks great!! just requesting changes to the tests, and also a column thing

[SteveDMurphy]

Not sure but I may have introduced a new issue somewhere causing an empty state seizure 🤔 found when retesting https://www.loom.com/share/5b57f5b506cb445b9fa0766b2e640409

[allisonking]

Not sure but I may have introduced a new issue somewhere causing an empty state seizure 🤔 found when retesting

Hmm what steps did you take to get here? I've seen this once before but I think it's because my backend wasn't working properly. I'll try to repro on main

[SteveDMurphy]

Not sure but I may have introduced a new issue somewhere causing an empty state seizure 🤔 found when retesting

Hmm what steps did you take to get here? I've seen this once before but I think it's because my backend wasn't working properly. I'll try to repro on main

It looks like it only happens when using npm run cy:start - I noticed the classify dataset button was missing as well so thinking maybe something wrong with backend too. Flipped back to npm run dev since Cypress seemed to be fine and no more stuttering / classify dataset button visible again

[allisonking]

Not sure but I may have introduced a new issue somewhere causing an empty state seizure 🤔 found when retesting

Hmm what steps did you take to get here? I've seen this once before but I think it's because my backend wasn't working properly. I'll try to repro on main

It looks like it only happens when using npm run cy:start - I noticed the classify dataset button was missing as well so thinking maybe something wrong with backend too. Flipped back to npm run dev since Cypress seemed to be fine and no more stuttering / classify dataset button visible again

Ohhhh okay yeah npm run cy:start should only be used against cypress (which I'm only just realizing now too!). This is because we set a NODE_ENV=test there, and in our next config we don't reroute backend requests if we are in the test environment (since these cypress tests should work without a backend (they're all stubbed))

I believe to run a built js, you can npm run build and then npm run start

[allisonking]

yay nice job Steve!! I left one suggestion, though it might make sense to make a separate ticket for it, if we want to do it at all

[TheAndrewJackson]

Great job! I like your use of reduce

[SteveDMurphy]

Not sure but I may have introduced a new issue somewhere causing an empty state seizure 🤔 found when retesting

Hmm what steps did you take to get here? I've seen this once before but I think it's because my backend wasn't working properly. I'll try to repro on main

It looks like it only happens when using npm run cy:start - I noticed the classify dataset button was missing as well so thinking maybe something wrong with backend too. Flipped back to npm run dev since Cypress seemed to be fine and no more stuttering / classify dataset button visible again

Ohhhh okay yeah npm run cy:start should only be used against cypress (which I'm only just realizing now too!). This is because we set a NODE_ENV=test there, and in our next config we don't reroute backend requests if we are in the test environment (since these cypress tests should work without a backend (they're all stubbed))

I believe to run a built js, you can npm run build and then npm run start

Ahh, interesting! Thank you once again for the detailed explainer 🙌🏽 makes sense now and I'm not afraid that I did something wonky somewhere 😂