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

Display error, using inbuilt notification system, if primary email is not verified #4964

Merged

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Dec 6, 2018

Related Issue - #4025
Related PR - #4758

Closes #4025

@dojutsu-user dojutsu-user changed the title Display error, using inbuild notification system, if primary email is not verified Display error, using inbuilt notification system, if primary email is not verified Dec 6, 2018
@codecov
Copy link

@codecov codecov bot commented Dec 6, 2018

Codecov Report

No coverage uploaded for pull request base (master@f0a6474). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #4964   +/-   ##
=========================================
  Coverage          ?   76.89%           
=========================================
  Files             ?      158           
  Lines             ?    10005           
  Branches          ?     1255           
=========================================
  Hits              ?     7693           
  Misses            ?     1978           
  Partials          ?      334
Impacted Files Coverage Δ
readthedocs/projects/tasks.py 70.54% <100%> (ø)
readthedocs/projects/views/private.py 80.15% <100%> (ø)
readthedocs/projects/notifications.py 100% <100%> (ø)

Copy link
Member

@humitos humitos left a comment

I like this approach.

I left some questions and suggestions to improve it. It's close to be merged to me.

readthedocs/projects/views/private.py Outdated Show resolved Hide resolved
readthedocs/projects/notifications.py Outdated Show resolved Hide resolved
readthedocs/projects/views/private.py Outdated Show resolved Hide resolved
readthedocs/projects/views/private.py Outdated Show resolved Hide resolved
readthedocs/projects/views/private.py Outdated Show resolved Hide resolved
@@ -144,6 +145,9 @@ class TestNotification(SiteNotification):
success_level = INFO_NON_PERSISTENT

user = fixture.get(User)
# Setting the primary and verified email address of the user
email = fixture.get(EmailAddress, user=user, primary=True, verified=True)
Copy link
Member

@humitos humitos Dec 12, 2018

I don't understand how this works. Where are we testing that the user does not have a verified email and where that it does have it?

Copy link
Member Author

@dojutsu-user dojutsu-user Dec 12, 2018

I added the primary and verified email for the test user because this line
https://github.com/rtfd/readthedocs.org/blob/cd6e82118541b1612b24540efebdefb7af92827e/readthedocs/rtd_tests/tests/test_notifications.py#L155

tests that there is only one persistent message but because the email of the test user is not verified, it adds another notification (the one which is added in this PR).

@humitos humitos requested a review from Jan 9, 2019
@humitos
Copy link
Member

@humitos humitos commented Jan 9, 2019

I'd love to have this merged since we need to get there: have all the users with verified emails. I think that notify them is the first step. I'm requesting for another round of review here.

@humitos humitos force-pushed the display-error-if-email-not-verified branch from 55ee8b5 to 08b3368 Jan 9, 2019
@humitos
Copy link
Member

@humitos humitos commented Jan 9, 2019

I just tested this PR locally and pushed some changes here to make this PR mergeable.

humitos
humitos approved these changes Jan 9, 2019
Copy link
Member

@humitos humitos left a comment

Thanks for your collaboration!

I just adapt some lines to make it ready for review and merge. If nobody has a different opinion, we can merge it as it is.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 9, 2019

@humitos
Thank you for the corrections.

@humitos humitos requested a review from Jan 14, 2019
@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 14, 2019

@humitos
By mistake, I have committed the old common files which raises the merge conflict.
I tried locally, but couldn't solve the conflict. Can you help me with that.

Edit: I fixed it.

@dojutsu-user dojutsu-user force-pushed the display-error-if-email-not-verified branch from fa31d01 to feaa14b Jan 15, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 22, 2019

Looks good to me. Happy to be starting to get this resolved. 👍

@ericholscher ericholscher merged commit 94dd13f into readthedocs:master Jan 22, 2019
1 check passed
@dojutsu-user dojutsu-user deleted the display-error-if-email-not-verified branch Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants