-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: change email updating workflow [DHIS2-18493] #1470
base: master
Are you sure you want to change the base?
Conversation
🚀 Deployed on https://pr-1470--dhis2-user-profile.netlify.app |
@@ -37,23 +34,21 @@ export function VerifyEmail({ userEmail }) { | |||
|
|||
const emailConfigured = systemInfo?.emailConfigured | |||
|
|||
const isValidEmail = emailRegExp.test(userEmail) | |||
const isInvalidEmail = Boolean(emailValidator(userEmail)) |
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 is just updating the validator to be consistent with what we have in the email modal
src/layout/FormFields.component.js
Outdated
setUserEmail: (email) => { | ||
valueStore.state['email'] = email | ||
valueStore.state['emailUpdated'] = true | ||
valueStore.setState(valueStore.state) |
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.
initially, we'd done valueStore.setState({...valueStore.state, email})
(or similar), but then this did not actually set the new email state 🤔 (I think there's some nuances to component setState that I no longer know)
<VerifyEmailWarning | ||
config={this.context.d2} | ||
emailUpdated={ | ||
this.props?.valueStore?.state?.emailUpdated |
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.
if user updates the email, we assume by default that it is no longer verified
src/layout/ModalField.component.js
Outdated
</Button> | ||
</TooltipWrapper> | ||
</div> | ||
<Modal hide={!modalOpen} onClose={closeModal}> |
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.
Kind of minor. But I think I would've preferred this (The modal with the contents) to be it's own component. Not just for the sake of extracting this out - but it would also mean that we don't have to manually reset the state of this modal during closeModal
, if we were to conditionally render it.
I haven't done a full analysis of what it would look like, or if would be annoying to do - just an observation.
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.
I think that's fair. We just started writing it this way and then it was easier to continue on with what we had and do a manual reset of the component state. But it became a bit longer than what we had originally, so seems fair to also break it up into a separate component. I've updated to put it in its own component but within the EmailField.js file as I think it's functionally easier to read the code when this is in one file.
src/layout/ModalField.component.js
Outdated
onUpdate: PropTypes.func, | ||
} | ||
|
||
export function ModalField({ |
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.
I'm a bit confused about the naming of this component (and file). When I read "ModalField" I would expect this to be a generic field that somehow opens a modal. I would expect this to be called EmailField
.
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.
I've updated to use EmailField, and to make some of the related functions/props correspond to this update
src/layout/FormFields.component.js
Outdated
props: { userEmail: valueStore.state['email'] || '' }, | ||
component: ModalField, | ||
props: { | ||
onUpdate, |
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.
I don't think it's necessary to both pass down onUpdate
and an setUserEmail
property?
Why can't we just override onUpdate
- and omit setUserEmail
? And then set emailUpdated
within that override?
eg:
onUpdate: (
field, // Imo we should remove this so the component doesn't have to call this with the fieldName
value
) => {
onUpdate('email', value)
onUpdate('emailUpdated', true)
},
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.
Yeah. I should have cleaned that up. We had implemented the state update before we realized we needed to change the onUpdate to get the update to fire.
src/layout/FormFields.component.js
Outdated
props: { | ||
onUpdate, | ||
userEmail: valueStore.state['email'] || '', | ||
userEmailVerified: d2?.currentUser?.emailVerified, | ||
setUserEmail: (email) => { | ||
valueStore.state['email'] = email | ||
valueStore.state['emailUpdated'] = true | ||
valueStore.setState(valueStore.state) | ||
}, |
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.
props: { | |
onUpdate, | |
userEmail: valueStore.state['email'] || '', | |
userEmailVerified: d2?.currentUser?.emailVerified, | |
setUserEmail: (email) => { | |
valueStore.state['email'] = email | |
valueStore.state['emailUpdated'] = true | |
valueStore.setState(valueStore.state) | |
}, | |
props: { | |
onUpdate: ( | |
value | |
) => { | |
onUpdate('email', value) | |
onUpdate('emailUpdated', true) | |
}, | |
userEmail: valueStore.state['email'] || '', | |
userEmailVerified: d2?.currentUser?.emailVerified, |
1078394
to
a0028a1
Compare
a0028a1
to
7c54a40
Compare
Background / Notes
See ticket description.
This PR adds a modal for changing a user email (/ removing one). And does some light refactoring related to email verification button / email verification warning (e.g. moving them, clearing up some language if there is no email).
Now there's a row of buttons that let you work with your email:
(see ticket for detailed requirements)
Known issues
Unfortunately, posting ''/undefined/null to api/me (e.g. payload of {email: null}) is not allowed / does not remove value.
Additionally, api/me does not appear to support PATCH, so cannot use a remove operation.
I'm just interpreting anything an email that evaluates to
email.trim() === ''
as a non existent email, but there's room for improvement here and we would need a backend change.See also point below in testing section below about lack of rollback on failure.
Testing
There's no automated testing in this app, and I don't want to add it here 🫤.
I've done a number of manual tests changing/removing emails to make sure that things work as expected.
One weak point here is that the backend currently accepts pretty much anything you throw at it for an email, which makes it hard to test for what happens when/if the email fails to update. A general problem with the user app is that the app does not rollback the inputs when they fail to post. For consistency (🤨) I've not done anything about that in this PR, so if an email is rejected by the backend, it still updates the input field. You'd need to just try again if that happens, but that's how this app generally works 🙃.
I did not test in conjunction with verifying email as that was part of another ticket, so we will probably want to test the entire workflow manually at some point when all the user profile changes are done.