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

fix(data-secrecy): update fe state & address a couple bugs #82756

Merged
merged 2 commits into from
Dec 31, 2024

Conversation

iamrajjoshi
Copy link
Member

@iamrajjoshi iamrajjoshi commented Dec 31, 2024

followup of #82754.

here, i am fixing some of the bugs i found while fixing the data secrecy waiver. i added comments where relevant. after this deploys, we should be ready to GA

video demo:

Screen.Recording.2024-12-30.at.11.11.50.PM.mov

@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Dec 31, 2024

// if the user has allowed access, we need to remove the temporary access window
// only if there is an existing waiver
if (value && allowData) {
Copy link
Member Author

Choose a reason for hiding this comment

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

problem 1 - if a user has a waiver set then allows access, we should delete the waiver. this is because after a while if the user turns out data secrecy again, we don't want them to get surprised with the old waiver.

hence we delete, so when the user enables data secrecy, they start fresh and need to create a new waiver.

data: nextData,
}
);
await api.requestPromise(`/organizations/${organization.slug}/data-secrecy/`, {
Copy link
Member Author

Choose a reason for hiding this comment

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

i added 2 awaits 😵

disabled: allowAccess && !organization.access.includes('org:write'),
value: allowAccess ? '' : allowDateFormData,
// disable the field if the user has allowed access or if the user does not have org:write access
disabled: allowAccess || !organization.access.includes('org:write'),
Copy link
Member Author

Choose a reason for hiding this comment

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

this was a bug, we should disable the date input if data secrecy is disabled OR if the user doesn't have enough permissions

@@ -140,9 +172,9 @@ export default function DataSecrecy() {
<PanelBody>
{!allowAccess && (
<PanelAlert>
{hasValidTempAccess
{allowData?.accessEnd && moment().isBefore(moment(allowData.accessEnd))
Copy link
Member Author

Choose a reason for hiding this comment

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

hadValidTempAccess was a a constant which wasn't stateful, so just moved the stateful conditional here.

@iamrajjoshi iamrajjoshi self-assigned this Dec 31, 2024
@iamrajjoshi iamrajjoshi requested a review from a team December 31, 2024 16:08
@iamrajjoshi iamrajjoshi marked this pull request as ready for review December 31, 2024 16:10
Base automatically changed from raj/ds-waiver-cleanup to master December 31, 2024 16:27
@iamrajjoshi iamrajjoshi requested a review from a team as a code owner December 31, 2024 16:27
const [allowDate, setAllowDate] = useState<WaiverData>();

// state of the data secrecy waiver
const [allowData, setAllowDate] = useState<WaiverData>();
Copy link
Member

Choose a reason for hiding this comment

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

naming nit: waiver, setWaiver?

Comment on lines 71 to 82
addSuccessMessage(
t('Successfully removed temporary access window and allowed support access.')
);
// refetch to get the latest waiver data
refetch();
return;
}
addSuccessMessage(
value
? t('Successfully allowed support access.')
: t('Successfully removed support access.')
);
Copy link
Member

Choose a reason for hiding this comment

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

nit: instead of early return you can modify the success message text, add the success message outside of the if, and then you can use the refetch at the end of the function

@iamrajjoshi iamrajjoshi enabled auto-merge (squash) December 31, 2024 17:16
@iamrajjoshi iamrajjoshi merged commit cd4460f into master Dec 31, 2024
43 checks passed
@iamrajjoshi iamrajjoshi deleted the raj/ds-waiver-fix-fe branch December 31, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants