-
Notifications
You must be signed in to change notification settings - Fork 84
Improve manual task conditions pickers #7089
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
…rt-Request-Data-as-criteria-for-manual-task-conditions
…conditions-pickers
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
This reverts commit 12bb437.
Greptile OverviewGreptile SummaryThis PR improves the user experience for configuring manual task conditions by dynamically rendering appropriate input components based on the selected field type. When users select a field like Key changes:
Issues found:
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.
5 files reviewed, 1 comment
...nts/admin-ui/src/features/integrations/configure-tasks/components/ConditionValueSelector.tsx
Show resolved
Hide resolved
…into ENG-2014-Improve-conditions-pickers
| if (rawValue && typeof rawValue === "object" && "toISOString" in rawValue) { | ||
| return (rawValue as Dayjs).toISOString(); | ||
| } |
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.
This type guard is somewhat weak. The check "toISOString" in rawValue doesn't guarantee it's a Dayjs object (native Date objects also have toISOString). Consider using:
| if (rawValue && typeof rawValue === "object" && "toISOString" in rawValue) { | |
| return (rawValue as Dayjs).toISOString(); | |
| } | |
| if (dayjs.isDayjs(rawValue)) { | |
| return rawValue.toISOString(); | |
| } |
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.
Excelent suggestion, that is a much better guard! Applied
Co-authored-by: Jason Gill <jason.gill@ethyca.com>
…conditions-pickers
|
@gilluminate Thanks for the review! I've applied both your suggestions.
it is now ready for a re-review. |
gilluminate
left a comment
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.
Excellent work! Tested and works well. It's fun to use!
Co-authored-by: Jason Gill <jason.gill@ethyca.com>
Ticket ENG-2014
Description Of Changes
When setting up request fields conditions, it will now show the proper input depending on the field selected.
Some examples:
Code Changes
Steps to Confirm
Go to the conditions settings of a manual task integration like here
Click Add Condition and select "Privacy request field" in field source
Try selecting different options in the Field selector and check the value selector is correct
For a date field you should see a datetime picker
For a boolean, you should see radio buttons with True/False option
For policy name, you should see a policy selector
For location you should see a location country/city picker
For location country you should see a location country picker
For location group should show groups (and operator can only be list contains)
For location regulations should show regulation select (and operator can only be list contains)
Try adding the field and then editing, check the edit modal shows the saved value correctly.
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works