-
Notifications
You must be signed in to change notification settings - Fork 4
chore: Send email verification for signing up #1118
base: develop
Are you sure you want to change the base?
Conversation
@keshakaneria please review my PR |
vms/vms/settings.py
Outdated
EMAIL_BACKEND = 'django.core.mail.backends.smtp.EmailBackend' | ||
EMAIL_HOST = 'smtp.gmail.com' | ||
EMAIL_HOST_PASSWORD = 'Khushi17@' | ||
EMAIL_HOST_USER = 'dpskhu13108@gmail.com' |
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.
You forgot to remove your personal email ID
if DEBUG: | ||
EMAIL_HOST = config('EMAIL_HOST', default='localhost') | ||
EMAIL_PORT = config('EMAIL_PORT', default=1025, cast=int) | ||
EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend' |
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.
keep this in comments for better understanding rather than just removing it completely
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.
Remove the personal ID and keep the deleted portion of printing it in console for development purpose.
EMAIL_HOST_PASSWORD = 'your_password' | ||
EMAIL_HOST_USER = 'your_email' | ||
EMAIL_PORT = 587 | ||
EMAIL_USE_TSL = True |
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.
I don't understand this change 🤔
It's supposed to be like that so we can test easily during local dev
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.
But I don't think while pushing the changes we usually keep the email addresses and the passwords. The one who clones it, needs to add their personal ones before running this. This is what I learnt in my Internship. If I am wrong, do correct me :)
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.
Sorry for the late response but could you elaborate on that? I do not get you.
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.
Sure. What we used to do like whenever we used to push the changes in authentication part regarding login or signups, we deleted the dummy email Id used. Because after the changes are merged this email ID and password wont be valid for the other person who tries to clone and run. So kept that field empty and whenever someone tries to run, s/he must first put in their id and password in order to try the emails sent.
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.
If we are not supposed to do that way here then do let me know if I am wrong :)
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.
Oh I see. I will try to explain.
So basically, first things first, for anything that depends on an external service, it's generally much more efficient to keep it configurable, and that's exactly what's done here. If you can imagine, if for an external service like mail, we keep it hardcoded it can lead to all sorts of abuse, that is why the config()
function is used (from a 3rd party package) which basically loads the required settings from a .env
file.
Any updates? |
|
Have a look at this error. It might help you to resolve. |
@SanketDG Can you help us out how according to you explained me in above comments shall we proceed with the changes? |
@keshakaneria please help me fix it. i am not getting why is this error is coming |
Looks like this is fixable by running a command from |
@SanketDG i run this command in |
Did you try to figure this out @khushishikhu ? |
@keshakaneria yes i tried a lot but turns out getting these errors again and again. can you please help me to resolve these errors because i think if there won't be these errors the issue has been done |
I see! It will be better if we can discuss this error to solve this problem on Zulip. We'll make through it :) |
Description
changes made for verification of email when the user logged in for the first time
Fixes #1029
Type of Change:
Checklist:
Code/Quality Assurance Only