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.

Sign up for GitHub

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

Jump to bottom

Properties page #4633

Merged
merged 5 commits into from
Feb 28, 2024
Merged

Properties page #4633

merged 5 commits into from
Feb 28, 2024

Conversation

galvana
Copy link
Contributor

@galvana galvana commented Feb 26, 2024 •
edited
Loading

Closes PROD-1661, PROD-1662, PROD-1663

Description Of Changes

Updating navbar and adding first iteration of the properties page with the ability to create a property with a name, type (Website & Other), and an auto-generated key.

Code Changes

  • Removed "Classify systems" page
  • Updated icons for Copy and Delete actions
  • Added new CustomClipboardCopyProps form component
  • New properties page and table
  • New properties.slice.ts
  • Moved EmptyTableNotice component to render inside the table
  • New property table, model, and schema
  • Cypress test for properties page

Steps to Confirm

  • Run nox -s "build(slim)" "dev(slim)" -- dev against the fidesplus branch found here https://github.com/ethyca/fidesplus/pull/1326
  • Run turbo run dev in the clients directory of fides
  • Navigate to the Properties page and verify you can create and list properties

Pre-Merge Checklist

@vercel Vercel
Copy link

vercel bot commented Feb 26, 2024 •
edited
Loading

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
fides-plus-nightly ⬜️ Ignored (Inspect) Visit Preview Feb 28, 2024 3:56pm

@galvana galvana marked this pull request as ready for review February 26, 2024 19:27
@galvana galvana mentioned this pull request Feb 26, 2024
Closed
18 tasks
adamsachs
adamsachs approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

this looks good to me, only a couple of minor points!

src/fides/api/models/property.py Show resolved Hide resolved
src/fides/api/models/property.py Show resolved Hide resolved
@cypress Cypress
Copy link

cypress bot commented Feb 27, 2024 •
edited
Loading

Passing run #6386 ↗︎

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

Details:

Merge 23bcd51 into 9cdd0c8...
Project: fides Commit: 999b359c9e ℹ️
Status: Passed Duration: 00:38 💡
Started: Feb 27, 2024 9:53 PM Ended: Feb 27, 2024 9:54 PM

Review all test suite changes for PR #4633 ↗︎

@codecov Codecov
Copy link

codecov bot commented Feb 27, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.85%. Comparing base (9cdd0c8) to head (23bcd51).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4633      +/-   ##
==========================================
+ Coverage   86.83%   86.85%   +0.02%     
==========================================
  Files         332      334       +2     
  Lines       19979    20013      +34     
  Branches     2569     2569              
==========================================
+ Hits        17349    17383      +34     
  Misses       2157     2157              
  Partials      473      473

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Kelsey-Ethyca
Kelsey-Ethyca reviewed Feb 27, 2024
View reviewed changes
clients/admin-ui/cypress/e2e/config-wizard-plus.cy.ts Outdated Show resolved Hide resolved
clients/admin-ui/src/features/common/nav/v2/BackButton.tsx Show resolved Hide resolved
clients/admin-ui/src/features/common/table/v2/FidesTable.tsx Show resolved Hide resolved
clients/admin-ui/src/features/common/nav/v2/nav-config.ts Show resolved Hide resolved
clients/admin-ui/src/features/properties/PropertyForm.tsx Show resolved Hide resolved
clients/admin-ui/src/features/properties/PropertyForm.tsx Show resolved Hide resolved
clients/admin-ui/src/features/properties/PropertiesTable.tsx Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

The designs state the row should be clickable and bring you to the edit screen but that is not the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clicking on a row now takes you to do the edit page

clients/admin-ui/src/features/properties/PropertiesTable.tsx Outdated Show resolved Hide resolved
clients/admin-ui/src/features/properties/PropertyForm.tsx Show resolved Hide resolved
Kelsey-Ethyca
Kelsey-Ethyca requested changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@Kelsey-Ethyca Kelsey-Ethyca left a comment

Choose a reason for hiding this comment

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

Left some comments following ticket AC functionality / designs. The code structure is good!

galvana
galvana commented Feb 27, 2024
View reviewed changes
Copy link
Contributor Author

@galvana galvana left a comment

Choose a reason for hiding this comment

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

Thank you @Kelsey-Ethyca and @adamsachs for the first pass! I've addressed all of your comments, please let me know what you think

clients/admin-ui/cypress/e2e/config-wizard-plus.cy.ts Outdated Show resolved Hide resolved
clients/admin-ui/src/features/common/nav/v2/nav-config.ts Show resolved Hide resolved
clients/admin-ui/src/features/common/nav/v2/nav-config.ts Outdated Show resolved Hide resolved
clients/admin-ui/src/features/properties/PropertiesTable.tsx Outdated Show resolved Hide resolved
clients/admin-ui/src/features/properties/PropertiesTable.tsx Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clicking on a row now takes you to do the edit page

clients/admin-ui/src/features/properties/PropertyForm.tsx Show resolved Hide resolved
clients/admin-ui/src/features/properties/PropertyForm.tsx Show resolved Hide resolved
src/fides/api/models/property.py Show resolved Hide resolved
clients/admin-ui/src/features/properties/PropertyForm.tsx Show resolved Hide resolved
src/fides/api/models/property.py Show resolved Hide resolved
adamsachs
adamsachs approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@adamsachs adamsachs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks good to me on the BE, thanks for making the suggested tweaks 👍

just a reminder to update the changelog (either here or on the plus PR)!

Kelsey-Ethyca
Kelsey-Ethyca approved these changes Feb 28, 2024
View reviewed changes
Copy link
Contributor

@Kelsey-Ethyca Kelsey-Ethyca left a comment

Choose a reason for hiding this comment

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

Lovely ❤️

@galvana galvana merged commit cc9409c into main Feb 28, 2024
20 checks passed
@galvana galvana deleted the PROD-1663-properties-page branch February 28, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kelsey-Ethyca Kelsey-Ethyca Kelsey-Ethyca approved these changes

@adamsachs adamsachs adamsachs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@galvana @adamsachs @Kelsey-Ethyca