1358 add email verification functionalty #1442
Conversation
onadata/apps/api/permissions.py
Outdated
| return False | ||
|
|
||
| if view.action == 'send_verification_email': | ||
| if request.user.username == request.data.get('username'): |
There was a problem hiding this comment.
the equality returns a boolean, why not return that instead of writing your own returns w/bools?
There was a problem hiding this comment.
That makes sense. I'll change it.
| except RegistrationProfile.DoesNotExist: | ||
| pass | ||
| else: | ||
| response_message = "Email was NOT verified" |
| if redirect_url: | ||
| return HttpResponseRedirect(redirect_url) | ||
|
|
||
| response_message = "Email has been verified." |
ivermac
changed the title
ivermac
changed the title
|
|
||
| @action(detail=False) | ||
| def verify_email(self, request, *args, **kwargs): | ||
| webhook = settings.EMAIL_VERIFICATION_WEBHOOK |
There was a problem hiding this comment.
Could we handle these settings variable with gettattr()? Hence do nothing if the settings are not in place and not break existing installations when they update without the setting in their local_settings.py.
getattr(settings, 'EMAIL_VERIFICATION_WEBHOOK', None)Tha
There was a problem hiding this comment.
I get your concern and I had considered it while working on this feature. I need to add documentation for the this endpoints and the settings variables that need to be set. So if you checkout the if-block that follows this line, you'll notice that if ENABLE_EMAIL_VERIFICATION is not set to true, the email verification endpoints won't be accessible and a user will get a 403. This means current installations won't be affected. If you set it to true, you have to set all the other email verification settings as well for the feature to work. In my opinion, if the app fails with a 500, the dev team would know that they need to set the settings variables that are required. Returning a None if the variable does not exist won't be helpful because all those variables are required.
There was a problem hiding this comment.
I agree you should add API documentation for this endpoint.
However, on the 500 error, I would rather we define default behaviour if those values are not set and provide examples of how that can be defined as you have done on the docker settings file.
There was a problem hiding this comment.
Cool, I'll define a default behaviour.
| {% blocktrans with expiration_days=expiration_days username=username expiration_days_p=expiration_days|pluralize %} | ||
| Hi {{ username }}, | ||
|
|
||
| To verify your email please click on the link below: |
There was a problem hiding this comment.
Can you start a language discussion on this with the product team internally, I think it may need some improvement unless this is exactly the same as the default done by the registration app.
There was a problem hiding this comment.
I agree with you that this might need improvement. I'll start the discussion. The template was mainly used to check that the email functionality works.
ivermac
force-pushed
the
1358-add-email-verification-functionalty
branch
from
35590ff
to
abccef2
Compare
ivermac
changed the title
ivermac
changed the title
| pass | ||
| else: | ||
| response_message = _("Email was NOT verified") | ||
| if webhook: |
There was a problem hiding this comment.
Let's send a 204 if there's a no webhook, so the sender knows we've processed this, so add an else here
Signed-off-by: ivermac <[email protected]>
- Set is_email_verified to True if the email is valid - Return the username and the email status of the verified account Signed-off-by: ivermac <[email protected]>
Test responses and redirect urls query params Signed-off-by: ivermac <[email protected]>
Signed-off-by: ivermac <[email protected]>
- Added utility function `_get_email_data` that not only returns email_data but also asserts that keys are in emai_data object - Remove unused utility function `get_kwargs` Signed-off-by: ivermac <[email protected]>
Functions from the future library can be used in both python 2 and 3 Signed-off-by: ivermac <[email protected]>
The function sets the value of is_email_verified to either True or False Signed-off-by: ivermac <[email protected]>
Signed-off-by: ivermac <[email protected]>
Signed-off-by: ivermac <[email protected]>
Signed-off-by: ivermac <[email protected]>
Function is used to send verification email Signed-off-by: ivermac <[email protected]>
Signed-off-by: ivermac <[email protected]>
Signed-off-by: ivermac <[email protected]>
Issue will be fixed by master then I'll rebase master with this branch Signed-off-by: ivermac <[email protected]>
Updated it to commit f711368180d58eef87eda54fadfd5f8355623d52; I messed up when rebasing Signed-off-by: ivermac <[email protected]>
Both functions don't use the self object Signed-off-by: ivermac <[email protected]>
- Use mutliple args instead of one dict arg - Raise error if any of the args is missing Signed-off-by: ivermac <[email protected]>
- Replace multiple instances of enable_email_verification with getattr(settings, 'ENABLE_EMAIL_VERIFICATION', False) - Destructure email_data when passing it to send_verification_email.delay Signed-off-by: ivermac <[email protected]>
- Rename ona_verified_username and ona_verified_username to username and is_email_verified respectively - Destructure email_data when calling send_verification_email.delay Signed-off-by: ivermac <[email protected]>
Signed-off-by: ivermac <[email protected]>
Signed-off-by: ivermac <[email protected]>
Signed-off-by: ivermac <[email protected]>
ivermac
force-pushed
the
1358-add-email-verification-functionalty
branch
from
87f8405
to
662b7fe
Compare
No description provided.