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

Adding default_value and hidden properties to custom privacy request fields #4370

Merged
merged 7 commits into from
Nov 6, 2023

Conversation

galvana
Copy link
Contributor

@galvana galvana commented Oct 31, 2023

Description Of Changes

Adding the ability to support hidden custom privacy request fields. This requires the addition of a default value so I added two new properties default_value and hidden.

  • The default value for a hidden field won't be shown in the form, but it will be included in the privacy request payload
  • The default value for a non-hidden field will be shown in the form and can be overwritten by the user

I also limited the height of the privacy request form field and added scrolling.

Code Changes

  • Updated smoke_test.cy.ts
  • Added validation to error fields flagged as hidden without a default value
  • Updated styling on the form
  • Updated form logic to account for the new field properties

Steps to Confirm

Pre-Merge Checklist

Comment on lines +40 to +51
last_name: {
label: "Last name",
value: "",
},
color: {
label: "Color",
value: "blue",
},
tenant_id: {
label: "Tenant ID",
value: "123",
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Verifying a few scenarios with this check

  • A non-required field still contains a value, even if it's an empty string
  • A field with a default_value can be overwritten in the form
  • A hidden field with a default_value is still included with the entered form values in the request

if (field.hidden && field.default_value === undefined) {
return {
isValid: false,
message: `Default value required for hidden field '${key}' in action '${action.policy_key}'`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error message doesn't surface completely, but I see that's a known issue #3171

Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't appear to be working—I set a value to hidden without a default_value but it still says isValid = true!

I think forEach returns don't work quite as we expect them to here: https://stackoverflow.com/questions/56129153/how-to-return-a-value-from-foreach

<Text fontSize="sm" color="gray.600" mb={4}>
{action.description}
</Text>
<ModalBody maxHeight={400} overflowY="auto">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating the popup to scroll if the form body exceeds 400px

@galvana galvana marked this pull request as ready for review October 31, 2023 22:51
@galvana galvana requested a review from allisonking October 31, 2023 22:51
Copy link

cypress bot commented Oct 31, 2023

Passing run #5060 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge 8fce260 into cd37e68...
Project: fides Commit: b4fe78b644 ℹ️
Status: Passed Duration: 01:13 💡
Started: Nov 6, 2023 9:06 PM Ended: Nov 6, 2023 9:08 PM

Review all test suite changes for PR #4370 ↗︎

Copy link

codecov bot commented Oct 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cd37e68) 87.04% compared to head (8fce260) 87.04%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4370   +/-   ##
=======================================
  Coverage   87.04%   87.04%           
=======================================
  Files         328      328           
  Lines       20267    20267           
  Branches     2608     2608           
=======================================
  Hits        17642    17642           
  Misses       2162     2162           
  Partials      463      463           

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

@galvana galvana force-pushed the PROD-1250-add-hidden-fields-to-privacy-requests branch from eec6512 to f32b016 Compare November 2, 2023 02:40
Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

confirmed happy path works!
image

I don't think the validation is working quite right though

if (field.hidden && field.default_value === undefined) {
return {
isValid: false,
message: `Default value required for hidden field '${key}' in action '${action.policy_key}'`,
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't appear to be working—I set a value to hidden without a default_value but it still says isValid = true!

I think forEach returns don't work quite as we expect them to here: https://stackoverflow.com/questions/56129153/how-to-return-a-value-from-foreach

Copy link

vercel bot commented Nov 6, 2023

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 Nov 6, 2023 10:56pm

Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

🙌

@galvana galvana merged commit 2cd633c into main Nov 6, 2023
@galvana galvana deleted the PROD-1250-add-hidden-fields-to-privacy-requests branch November 6, 2023 22:57
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