-
Notifications
You must be signed in to change notification settings - Fork 75
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
Email verification link #7117
Email verification link #7117
Conversation
app/pages/settings/email.jsx
Outdated
</a> | ||
*/} | ||
{(this.state.requestConfirmationEmailStatus === 'idle') && ( | ||
<a href="#" onClick={this.requestConfirmationEmail}> |
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 link links back to the current page. I think you meant to use a button here. In a screen reader, or even as a sighted user, I would expect the browser to navigate to a new location after clicking a link. I think the screen reader is more confusing, as this will be announced as a link, but the browser doesn't navigate anywhere after I click it (very confusing if I can't see the page.)
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.
See the WebAIM accessibility cheat sheet for more info about accessible links, but the code here would fail an accessibility audit.
By the way, 406: Not Acceptable is the response when content negotiation fails, so my first thought would be to look at the Accept headers.
|
If I go to https://pr-7117.pfe-preview.zooniverse.org/settings/email, then change my email address to a totally new email address, the page still says my email is verified. Is that a bug that I can exploit in order to get around spam blocks? I tested this by switching between an address on a personal domain that I own (which is verified), and my work address on the EDIT: when I changed my email address, the old address didn't get an email to say 'hey, someone changed your email address to a new address.' That's possibly an exploitable security hole too. Or maybe the notification emails are being a bit slow today. EDIT THE EDIT: I've now changed my email address a few times, swapping between personal and work domains. I haven't had any email notifications to say that someone else may have changed my email address, or that there's potentially suspicious activity on my account. |
Here's a weird bug (particularly if I'm using a screen reader, or colour blind, so don't see the change in colour of the input label.) When I delete my email address completely, I'm told the empty box is both valid and verified. When you're using colour like this, it's good practice to test your component in a greyscale mode, to catch accessibility failures where colour is being used to convey important information. There's also no button to submit changes. That's another potential accessibility failure. I guess that if I'm on a phone or tablet, I can discover by trial-and-error that tapping outside the box will save changes, but the UX is less than ideal. Screen.Recording.2024-06-03.at.09.52.23.mov |
aeaeca2
to
ef2da95
Compare
PR UpdateUpdate: the resend confirmation email link should now be fully functional!
StatusThis PR is ready for review. A follow up is required to address the accessibility feedback. |
Great work, Shaun! I can confirm this solution works when tested for an unconfirmed user account. I agree with Jim's comment regarding link vs. button. When "Resend Confirmation Email" appears as a clickable link -- as Sean and I recommend -- it is a bit odd to see Sorry for the indecision! @seanmiller26 -- are you OK with a button instead of a link? |
Sure, that sounds good. Let's use this style of button design - #ffffff fill with #005d69 outline, 4pt rounded corners, 16pt font. |
PR UpdateThanks for the feedback, everyone!
Design note 1: button looks like this. Font size is kept consistent with rest of text, which is... uh... 13.3333px? PFE is weird like that. Design note 2: hover & click has a different background. Design note 3: status text For further discussion, but won't be addressed in this PR:
|
4e510e3
to
c7387ee
Compare
@shaunanoordin -- I can answer this one:
No. Once verified, the account is always considered verified, which is more about confirming the validity of the account rather than the email. See zooniverse/panoptes#4268 (comment). |
c2e095e
to
b4296ef
Compare
re: email notifications -- I've opened zooniverse/panoptes#4351; that is out of scope for this PR and will be resolved separately. |
b4296ef
to
518f6a0
Compare
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.
Works as expected and code looks good!
'posting': <Translate content="emailSettings.general.emailUnverifiedRequesting" />, | ||
'success': <Translate content="emailSettings.general.emailUnverifiedSuccess" />, | ||
'error': <Translate content="emailSettings.general.emailUnverifiedError" />, | ||
}[this.state.requestConfirmationEmailStatus] || ''} |
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.
Clever!
518f6a0
to
14eb6ec
Compare
PR Overview
Staging branch URL: https://pr-7117.pfe-preview.zooniverse.org/settings/email
Follows #7083, towards #7032
This PR adds a link to the Email Settings page (under User -> Settings -> Email), which allows users to request a confirmation email from Panoptes.
New behaviour:
Testing
Status
As of commit aeaeca2, there's a problem where sending the request results in a 406 error, at least for staging.