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

Route for individual systems #2902

Conversation

allisonking
Copy link
Contributor

Closes https://github.com/ethyca/fidesplus/issues/681

Code Changes

  • Nest the previous configure page to be configure/[id] instead. Update the code to always query for the passed in system id on load
  • Update the tests accordingly

Steps to Confirm

  • Create some systems
  • In the /systems page, click on the more icons and then "Edit"
  • You should be brought to the same page as before, but instead of /systems/configure it'll be /systems/configure/{fides_key}
  • Log out and log back in to clear the redux store (or use another browser or incognito window)
  • Go directly to /systems/configure/{fides_key} where fides_key is one you have in your system
  • You should hit the edit page for this system directly
  • Go directly to /systems/configure/{something_else} where something_else is a fides key you do not have in your system
  • You should get a message that we couldn't find that system

Pre-Merge Checklist

Description Of Changes

Screen.Recording.2023-03-22.at.4.32.58.PM.mov

Comment on lines +437 to +441
cy.fixture("systems/systems.json").then((systems) => {
cy.intercept("GET", "/api/v1/system/*", {
body: systems[0],
}).as("getFidesctlSystem");
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note that we had different fixtures for 1️⃣ getting a list of systems vs 2️⃣ getting just one system. the tests were pretty dependent on the first system from the list of systems (1️⃣ ), so I made sure to keep that around and index into it, rather than relying on 2️⃣ which had data the tests didn't expect and would've required more changes

@cypress
Copy link

cypress bot commented Mar 22, 2023

Passing run #957 ↗︎

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

Details:

Merge 253768a into 7e449f9...
Project: fides Commit: da9dc92f99 ℹ️
Status: Passed Duration: 00:36 💡
Started: Mar 23, 2023 2:59 PM Ended: Mar 23, 2023 2:59 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@codecov
Copy link

codecov bot commented Mar 22, 2023

Codecov Report

❗ No coverage uploaded for pull request base (aking/fidesplus-681/system-id-route@7e449f9). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 7f4afeb differs from pull request most recent head 253768a. Consider uploading reports for the commit 253768a to get more accurate results

Additional details and impacted files
@@                          Coverage Diff                           @@
##             aking/fidesplus-681/system-id-route    #2902   +/-   ##
======================================================================
  Coverage                                       ?   86.61%           
======================================================================
  Files                                          ?      298           
  Lines                                          ?    16783           
  Branches                                       ?     2147           
======================================================================
  Hits                                           ?    14537           
  Misses                                         ?     1835           
  Partials                                       ?      411           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@TheAndrewJackson TheAndrewJackson left a comment

Choose a reason for hiding this comment

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

LGTM 💥 I'll update fidesplus to use this soon™️

@allisonking allisonking merged commit 7db432d into aking/fidesplus-681/system-id-route Mar 23, 2023
@allisonking allisonking deleted the aking/fidesplus-681/system-id-route-2 branch March 23, 2023 15:09
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.

2 participants