Skip to content

Conversation

@bbearce
Copy link
Collaborator

@bbearce bbearce commented Nov 27, 2022

@ mention of reviewers

@tristan
@Didayolo

A brief description of the purpose of the changes contained in this PR.

There was no email confirmation and we don't want spam accounts to build up quickly.

Issues this PR resolves

#738

Known issues to be addressed in a separate PR

...

A checklist for hand testing

  • Sign up and check to see that a green message shows up to let you know you need to check your email.
  • Check the confirmation link shows up and click it.
  • Verify that you are brought to the login page and see a message telling you that you are all signed up and to log in.
  • Check that you can log in.

Test error case:

  • Sign up and check to see that a green message shows up to let you know you need to check your email.
  • Check the confirmation link shows up and copy most but not all of the link.
  • When navigating to this link, you should be brought to the sign-up page with red message indicating the link was corrupt. This should have deleted your user in the process and you should retry signing up.
  • You should be able to correctly sign up, click activation link, and login after this.

Any relevant files for testing

...

Misc. comments

I'm a little worried I unnecessarily added django "messages" (https://docs.djangoproject.com/en/4.1/ref/contrib/messages/) to the base.html template when I could up setup messages based on API calls (already how things are setup). The problem was I couldn't find how to catch the return response from the signup API call, which is where most messaging functionality happens. We didn't have messages setup as the code was using a javascript package called "toastr" to flash messages. I was able to use django messages (which is nice as we can use them now in the future) and wrap them in toastr so we can get the cool flashes that pop up and everything looks consistent. This is my only worry as far as implementing this correctly.

Checklist

  • Code review by me
  • Hand tested by me
  • I'm proud of my work
  • Code review by reviewer
  • Hand tested by reviewer
  • Ready to merge

@Didayolo
Copy link
Member

Didayolo commented Nov 29, 2022

I tried and it's working.

Some remarks:

  • [OK] I needed to change "http" to "https" in the confirmation link to have it working. Maybe this is due to the test environment (Tuan is updating the SSL configuration) and will work fine in production.

  • We should change "This is an official message from University of Paris Saclay" to something like "This is an automatic message from CodaBench".

  • Remove auto-login after signup

  • Automatic tests are not passing due to flake8 formatting (should be easy to fix):

    src/apps/profiles/urls_accounts.py:15:2: W292 no newline at end of file
    src/apps/profiles/tokens.py:2:11: W291 trailing whitespace
    src/apps/profiles/tokens.py:8:62: E221 multiple spaces before operator
    src/apps/profiles/tokens.py:11:1: E305 expected 2 blank lines after class or function definition, found 1
    src/apps/profiles/tokens.py:11:61: W292 no newline at end of file
    src/apps/profiles/views.py:5:1: F401 'django.contrib.auth.login' imported but unused
    src/apps/profiles/views.py:66:1: E302 expected 2 blank lines, found 1
    src/apps/profiles/views.py:70:5: E722 do not use bare 'except'
    src/apps/profiles/views.py:74:1: W293 blank line contains whitespace
    src/apps/profiles/views.py:84:1: W293 blank line contains whitespace
    src/apps/profiles/views.py:87:1: E302 expected 2 blank lines, found 1
    src/apps/profiles/views.py:103:1: E302 expected 2 blank lines, found 1
  • [OK] If I try to login without having clicked on the link, I obtain this error message. It is the right behavior that it does not work, but the message is not explicit (should be saying "please confirm your email address").

Capture d’écran 2022-11-29 à 17 30 33

@Didayolo Didayolo self-requested a review November 29, 2022 16:52
@Didayolo Didayolo merged commit 4a6ca5c into develop Nov 29, 2022
@Didayolo Didayolo deleted the email_confirmation_738 branch November 29, 2022 22:37
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