-
Notifications
You must be signed in to change notification settings - Fork 84
Adding display order to PrivacyNotice and ExperienceNotices #6939
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
Conversation
|
Deployment failed with the following error: View Documentation: https://vercel.com/docs/two-factor-authentication |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (94.44%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #6939 +/- ##
=======================================
Coverage 87.32% 87.32%
=======================================
Files 525 525
Lines 34429 34445 +16
Branches 3962 3965 +3
=======================================
+ Hits 30064 30080 +16
Misses 3501 3501
Partials 864 864 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile OverviewGreptile SummaryThis PR adds a Key Changes
Non-Breaking DesignThe implementation preserves backward compatibility:
Confidence Score: 4/5
Important Files ChangedFile Analysis
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
11 files reviewed, no comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with some comments. Don't forget the changelog entry
also codecov's complaining, let's make sure we have coverage for all added lines :)
| from fides.api.models.privacy_experience import ( | ||
| link_notices_to_experience_config, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cursor loves the local imports 😒
Co-authored-by: erosselli <67162025+erosselli@users.noreply.github.com>
Co-authored-by: erosselli <67162025+erosselli@users.noreply.github.com>
Ticket ENG-1837
Description Of Changes
Adds a
display_orderfield toExperienceNoticesandPrivacyNotice. This allows us to store notices in the order provided via the API. Additionally, thedisplay_orderfield is used to order notices on read.ExperienceNotices- Stores how the notices should be ordered within a given privacy experiencePrivacyNotices- Stores how child notices should be ordered within a parent noticeThese changes are non-breaking. If
display_orderis not available, we can still return notices. Additionally, if only some notices have an order (this shouldn't happen under normal use) notices without an order will be placed after notices with a display order. Thedisplay_orderdoes not need to be provided, and it is also not returned, it's just a backend detail. The order of the notices is derived from the list order on ingestion, and the items are ordered by display order on read.Code Changes
display_ordertoexperiencenoticesandprivacynotice.draggableto true for the notice configuration section of the privacy experience form.ScrollableListto match the height of the contents. It was hard to drag elements within this list while having to scroll to the top of the list. We need to make sure this doesn't affect other pages.Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works