Skip to content

Manage emails #2898

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

Merged
merged 24 commits into from
Feb 16, 2018
Merged

Manage emails #2898

merged 24 commits into from
Feb 16, 2018

Conversation

di
Copy link
Member

@di di commented Feb 7, 2018

Fixes #2830. This PR adds the ability to add and remove emails, as well as change the primary email address. It also adds the ability to verify email addresses (and re-send verification emails).

screen shot 2018-02-06 at 6 16 42 pm

@nlhkabu I was imaginging that the "Primary", "Verified" and "Unverified" spans could be colored labels. Also imagining that the "Add" and "Save" buttons would be inline with their respective input fields.

@di di added this to the 1: Maintainer MVP milestone Feb 7, 2018
@di di force-pushed the manage-emails branch 6 times, most recently from da2f817 to 6fb784b Compare February 7, 2018 21:51
@brainwane
Copy link
Contributor

@lgh2, Dustin and I would like for you to review this PR. Please:

  • grab Dustin's branch onto your local machine
  • run the tests locally (a measure to ensure that you can do so and that there's no logistical trouble),
  • test and try to break functionality, especially edge cases
  • look at the code changes and say if there's a chunk of code that you have questions about

Edge cases Dustin would like you to investigate start with:

  • add an email, then delete it, then follow the verification link — what happens?
  • add an email, log out, then follow the verification link — what happens?
  • add an email for user A, log in as user B, follow the verification link — what happens?
  • open the management page in two separate tabs, and try to delete the same email twice — what happens?

If you think of more edge cases to test, please go ahead and try them!

@lgh2
Copy link
Contributor

lgh2 commented Feb 9, 2018

Ok, I will test it.

Copy link
Contributor

@lgh2 lgh2 left a comment

Choose a reason for hiding this comment

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

Automated tests

tests pass
linter passes

Manual tests

For the purposes of these tests, I set up 2 accounts, “Laura” and “Jane”. The email address [email protected] and [email protected] are associated with the Jane account.

I used LastPass to store the credentials for both accounts.

I was using Chrome Version 64.0.3282.119 (Official Build) beta (64-bit) on Linux Mint 18.1 (Ubuntu Xenial 16.04)

add an email, then delete it, then follow the verification link — what happens?

  • banner displays "email not found"
  • email is gone from "Add email address" box

add an email, log out, then follow the verification link — what happens?

  • redirected to login screen
  • green banner displays email address that was verified, displaying the email address (screenshot)
  • for security reasons, is it advisable to display the email to someone who is logged out?
  • email is verified when I log back in

add an email for user A, log in as user B, follow the verification link — what happens?

  • banner displays email verification message showing email, even when I am logged in to another account
  • email is verified when I log in as the original user again

open the management page in two separate tabs, and try to delete the same email twice — what happens?

  • email address not found banner is displayed in the second browser tab.
  • email is gone from both tabs.

@lgh2
Copy link
Contributor

lgh2 commented Feb 9, 2018

Here are screenshots associated with my previous comment:

add an email, log out, then follow the verification link — what happens?

This screenshot shows the verified email address shown in the banner when the user is logged out, after following the link in the verification email:

not_logged_in_email_displayed

add an email for user A, log in as user B, follow the verification link — what happens?

This screenshot shows an email associated with another user's account displayed when I am logged in, after I clicked the email verification link while logged in:

other_login_email_verified

@di
Copy link
Member Author

di commented Feb 9, 2018

@lgh2 Thanks very much for the QA! And great find. I've pushed a commit which requires that a user must be logged in before they can verify a new email address. Can you re-test the second and third use cases?

@lgh2
Copy link
Contributor

lgh2 commented Feb 12, 2018

Thank you!
Once again, I'm using Chrome Version 64.0.3282.119 (Official Build) beta (64-bit) on Linux Mint 18.1 (Ubuntu Xenial 16.04)

Here are the second and third use cases:

add an email, log out, then follow the verification link — what happens?

  • email banner is no longer displayed, I am redirected to the login page.
    no-banner-after-logout

  • when I log in again, the email is verified.

add an email for user A, log in as user B, follow the verification link — what happens?

  • When I try following the link from user A while logged in as user B, I get an "email not found" error, displayed in a banner:

email-not-found

  • When I log in to user A, the email is still unverified.

@di
Copy link
Member Author

di commented Feb 12, 2018

Great, thanks @lgh2!

@brainwane
Copy link
Contributor

If anyone feels like testing this, a few more edge cases you could check:

  • do the views give the user the appropriate error if you try to leave the "Add email address" field empty and click the Add button?
  • what does the application do if you try to put multiple email addresses in the "Add email address" field, separated with a comma?
  • does the email that we send look ok both in HTML and in plain text?
  • do we give the user good error messages for invalid email addresses? (addresses that, for instance, include spaces or don't have TLDs (example: sumanah@python))
  • does the Tab key work as you'd expect to tab through these fields/buttons?
  • what happens if you try to make up a fake verification link with random characters in it and go to that URL, while logged out and while logged in?
  • what does the application do if you try to put something like "Sumana" <[email protected]> or Sumana <[email protected]> in the "Add email address" field, separated with a comma? (When people copy and paste email addresses from email programs sometimes that's the format they end up pasting in.)
  • do we deal ok with weird but technically standards-compliant email addresses? (very long, very short, including special characters of various kinds)

@nlhkabu
Copy link
Contributor

nlhkabu commented Feb 16, 2018

Looks like my indentation is off - will fix.

@di can you please review 0990b88 to make sure I've not lost anything? Thx

@di di merged commit 16cf16f into master Feb 16, 2018
@di di deleted the manage-emails branch February 16, 2018 16:57
@waseem18
Copy link
Contributor

@brainwane I've tested the application with the said test cases and everything seems to work perfectly.

I've tested them on both Chrome as well as Firefox browsers.

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.

5 participants