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

Implement a notification banner to show up on login #2842

Merged
merged 8 commits into from
Mar 15, 2023

Conversation

allisonking
Copy link
Contributor

@allisonking allisonking commented Mar 15, 2023

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

Code Changes

Steps to Confirm

  • Create a user who only has the "viewer" role via the new role assignment UI
  • Log in as that user
  • You should see the notification banner when you start the app
  • Exit out of it
  • Click around and make sure it doesn't come back
  • Log out of the app
  • Log back in and you should see the banner again
  • Log out and then log in as the root user (or a user who isn't a viewer)
  • You should not see the banner at all

Pre-Merge Checklist

Description Of Changes

I wasn't sure if we wanted to add a check against the fides version here? Or how we determine when we will remove this banner.

Screen.Recording.2023-03-15.at.12.52.11.PM.mov

@cypress
Copy link

cypress bot commented Mar 15, 2023

Passing run #834 ↗︎

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 da1ad49 into 53979ef...
Project: fides Commit: 608f460c08 ℹ️
Status: Passed Duration: 00:54 💡
Started: Mar 15, 2023 8:00 PM Ended: Mar 15, 2023 8:01 PM

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

@allisonking allisonking marked this pull request as ready for review March 15, 2023 16:58
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01 🎉

Comparison is base (d0d6b3c) 86.73% compared to head (da1ad49) 86.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2842      +/-   ##
==========================================
+ Coverage   86.73%   86.74%   +0.01%     
==========================================
  Files         295      295              
  Lines       16753    16753              
  Branches     2145     2145              
==========================================
+ Hits        14531    14533       +2     
+ Misses       1819     1818       -1     
+ Partials      403      402       -1     

see 1 file with indirect coverage changes

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

@NevilleS NevilleS left a comment

Choose a reason for hiding this comment

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

LGTM! Just a couple little nits

clients/admin-ui/src/home/HomeLayout.tsx Outdated Show resolved Hide resolved
clients/admin-ui/src/home/HomeLayout.tsx Outdated Show resolved Hide resolved
@allisonking
Copy link
Contributor Author

Just pushed one more change to make the banner only viewable to viewers. will update the steps to confirm

@allisonking allisonking merged commit a5794df into main Mar 15, 2023
@allisonking allisonking deleted the aking/fidesplus-676/access-control-warning branch March 15, 2023 20:10
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