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

feat: verify email warning #1458

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Chisomchima
Copy link
Member

@Chisomchima Chisomchima commented Dec 16, 2024

Implements DHIS2-18376

Description

This PR introduces a new feature to prompt users to verify their email address when the "Enforce Verified Emails" setting is enabled for the organization. The feature checks if the email verification setting is enabled (enforceVerifiedEmail), and whether the user has verified their email (emailVerified). If the email is not verified, a warning will be shown at the top of the user profile page, prompting the user to verify their email. The user can still continue using the system without email verification, but the warning will remain visible until their email is verified.


Checklist

  • NoticeBox:
    The warning message is displayed inside a NoticeBox component at the top of the user profile page. It will show the following message when the user's email is not verified:
    "Your email is not verified. Please verify your email to continue using the system."

Screenshots

Screenshot 2024-12-16 at 06 54 03

@dhis2-bot
Copy link
Contributor

🚀 Deployed on https://pr-1458--dhis2-user-profile.netlify.app

const VerifyEmailWarning = () => {
const { data: userData, error: userError } =
useDataQuery(getUserProfileQuery)
const { data: systemData, error: systemError } = useDataQuery(
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 tried getting the enforceVerifiedEmail state from the systemInfo object directly from useConfig but couldn't, hence the api call.

Copy link
Member Author

@Chisomchima Chisomchima left a comment

Choose a reason for hiding this comment

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

I think this may also be added into the user profile details page as well. open to suggestions.

@Chisomchima Chisomchima changed the title Dhis2 18376/verify email warning feat: verify email warning Dec 16, 2024
@Chisomchima Chisomchima requested a review from tomzemp December 18, 2024 08:27
Copy link
Member

@tomzemp tomzemp left a comment

Choose a reason for hiding this comment

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

Hi @Chisomchima: looks pretty good.

  • The app already has some logic for retrieving systemSettings and api/me (see https://github.com/dhis2/user-profile-app/blob/master/src/AppWrapper.js). It would be better to modify/reuse those stores if possible so that we don't have to remake similar requests (also so the logic can be in one place).
  • I think you're missing one requirement from the ticket: when a system has the "enforce verified emails" setting and user has NOT verified their email*, if a user visits the user profile app, they are directed to user profile page. Right now if I just visit the main page of the app with a user with unverified email, I get directed to the viewProfile page, but I should be sent to profile page in this case, so that I see the warning message. I assume you can edit the logic in app.router.js because it's a child component of the AppWrapper, so the settings/me information should be available here. If it's not possible for some reason, I think it's probably better to change the default routing of the app
  • can you add a margin on the NoticeBox warning, so that there's not the overhang that we currently have:
image

systemData?.systemSettings?.enforceVerifiedEmail
const emailVerified = userData?.me?.emailVerified

if (userError || systemError) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this would be better as a NoticeBox if we're going to show an error about not retrieving information (e.g. like we have elsewhere in the app:

if (error) {
return (
<NoticeBox
className={styles.noticeBox}
error
title={i18n.t('Error fetching your authorities')}
>
<Button onClick={refetch}>
{i18n.t('Retry loading authorities')}
</Button>
</NoticeBox>
)
}
).

However, I think if these requests were going to fail, the app seems like it wouldn't have loaded (

(error) => {
log.error('Failed to load user settings:', error)
}
), so maybe will not be relevant after refactoring.

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.

3 participants