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

Add passwort reset #188

Merged
merged 10 commits into from
Nov 1, 2022
Merged

Add passwort reset #188

merged 10 commits into from
Nov 1, 2022

Conversation

4lm
Copy link

@4lm 4lm commented Oct 25, 2022

Hi @Bachibouzouk,

Please review this PR and merge it if you approve.

This PR is based on the code of the open PRs #183 and #186. Those should be merged first. Also, the failing tests should be addressed. @Bachibouzouk can you address those tests before merging? It looks like they result from your code changes in #183.

I'm a friend of merging (if possible daily), even if something still needs to be finished. If we work in a team, merging branches that sit on a shell for too long often becomes very difficult.

Please speak up; if this process of merging your and my stuff often is not an option, then we have to find a different modus operandi.

Please also note:

  • the places where I placed the password reset links (/users/login and /users/user_info) could be visually improved. Maybe a job for @bmlancien. I can write an issue and invite @bmlancien to work on it. What do you think?
  • I used the console as email backend because no email system was given (or did I miss something?). I recommend staying with this until we go production because it makes testing the functionality fast.

@Bachibouzouk
Copy link

If we work in a team

It was not the case until you came ;), the reason I was not merging to often is that I can then quickly update the app online and because I had no pressure since I was alone on the repo :P

@@ -2,13 +2,11 @@
# Copyright (C) YEAR THE PACKAGE'S COPYRIGHT HOLDER

Choose a reason for hiding this comment

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

I will rebase your PR on change/improve-user-account-flow and remove this commit, the .po file has some items that must be handled manually (because some content is stored into variables I have to add this "manually" at the end of the file). My plan to deal with translation is to focus on it at once in a dedicated PR. It is good to add the translate tags in the html though, just no need to update the .po and .mon files

Copy link
Author

@4lm 4lm Oct 27, 2022

Choose a reason for hiding this comment

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

OK, thanks for the info, I will not touch any po/mo files from now on.

@@ -25,7 +25,7 @@ jobs:
python -m pip install --upgrade pip
pip install black==19.3b pytest
pip install click==8.0.2
pip install -r app/requirements/mysql.txt
pip install -r app/requirements/postgres.txt
Copy link

@Bachibouzouk Bachibouzouk Oct 27, 2022

Choose a reason for hiding this comment

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

I had done this with mysql because for testing purposes I could not find a way to generate automatically the postgres DB, whereas it was easy with mysql

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but with #183 this fails now, because dependencies are not met. Do you need help to fix this? Shall I look into this or do you want to fix it?

Copy link
Author

Choose a reason for hiding this comment

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

Side note, I think because of #194 sqlite is used anyways.

Copy link
Author

Choose a reason for hiding this comment

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

I will try to fix this in #197

Copy link
Author

Choose a reason for hiding this comment

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

@Bachibouzouk overtakes #197 now

@@ -171,7 +171,7 @@

CRISPY_TEMPLATE_PACK = "bootstrap4"

EMAIL_BACKEND = "django.core.mail.backends.smtp.EmailBackend"
EMAIL_BACKEND = "django.core.mail.backends.console.EmailBackend"

Choose a reason for hiding this comment

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

We should use console only when debug is set to True (the credentials I used for stmp are obviously not stored in the github repo)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, your are right, I will set this PR to draft, fix this and then remove draft mode again and order an re-review.

@Bachibouzouk
Copy link

  • I can write an issue and invite @bmlancien to work on it. What do you think?

I think it is a good idea, I will talk to him as I have to meet him soon anyway :) Thanks for the suggestion!

@Bachibouzouk
Copy link

@4lm - could you explain how I can test the features locally? How do I see the email confirmation link?

@4lm
Copy link
Author

4lm commented Oct 27, 2022

If we work in a team

It was not the case until you came ;), the reason I was not merging to often is that I can then quickly update the app online and because I had no pressure since I was alone on the repo :P

Yes, thanks for the clarification. All good :D

@4lm
Copy link
Author

4lm commented Oct 27, 2022

@4lm - could you explain how I can test the features locally? How do I see the email confirmation link?

It will pop up in your console instead of being send to your inbox. This way you can also quickly invent some email addresses and test some workflows. Example log:

[27/Oct/2022 10:47:10] "GET / HTTP/1.1" 302 0
[27/Oct/2022 10:47:10] "GET /de/ HTTP/1.1" 200 7438
[27/Oct/2022 10:47:13] "GET /de/users/signup/ HTTP/1.1" 200 9835
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: Activate your account.
From: noreply@e[...]0.eu
To: a[...][email protected]
Date: Thu, 27 Oct 2022 10:47:54 -0000
Message-ID: <1[...]6@r1>

  
Hi Alexis.Michaltsis,  
Please click on the link to confirm your registration,
http://127.0.0.1:8000/de/users/activate/MQ/bdy8nu-df94d90e3433b79ab357d4d8d26a4a8e/  
If you think, it's not you, then just ignore this email.  

-------------------------------------------------------------------------------
[27/Oct/2022 10:47:54] "POST /de/users/signup/ HTTP/1.1" 302 0
[27/Oct/2022 10:47:54] "GET /de/ HTTP/1.1" 200 7939
[27/Oct/2022 10:48:09] "GET /de/users/activate/MQ/bdy8nu-df94d90e3433b79ab357d4d8d26a4a8e/ HTTP/1.1" 302 0
[27/Oct/2022 10:48:09] "GET /de/users/login/ HTTP/1.1" 200 9051

@4lm 4lm marked this pull request as draft October 27, 2022 11:09
@4lm 4lm marked this pull request as ready for review October 27, 2022 11:24
@4lm 4lm requested a review from Bachibouzouk October 27, 2022 11:25
@Bachibouzouk Bachibouzouk force-pushed the change/improve-user-account-flow branch from 80f8b74 to 550941a Compare November 1, 2022 22:36
@Bachibouzouk
Copy link

@4lm - I have rebased this PR now. For the email sending, in production I have been using (it was there already) exchangelib to be able to use our server to send emails, could you lookup projects/services.py:send_feedback_email and see how you could adapt (in a subsequent PR) the email host? I will send you in a separate email the credentials or our server

Copy link

@Bachibouzouk Bachibouzouk left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this feature @4lm !

@Bachibouzouk Bachibouzouk merged commit 72ccf0f into main Nov 1, 2022
@Bachibouzouk Bachibouzouk deleted the change/improve-user-account-flow branch November 1, 2022 22:48
@4lm
Copy link
Author

4lm commented Nov 7, 2022

@4lm - I have rebased this PR now. For the email sending, in production I have been using (it was there already) exchangelib to be able to use our server to send emails, could you lookup projects/services.py:send_feedback_email and see how you could adapt (in a subsequent PR) the email host? I will send you in a separate email the credentials or our serve

Ok, I will have a look!

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.

2 participants