PedroBern / django-graphql-auth

Django registration and authentication with GraphQL.
https://django-graphql-auth.readthedocs.io/en/latest/
MIT License
329 stars 106 forks source link

Adding Email Context #21

Closed yanivtoledano closed 4 years ago

yanivtoledano commented 4 years ago

Partially solves #20

To add the "Password Reset" email we may need to rewrite get_email_context function altogether as it now expects an action which does not exist in case there is no action (i.e., no token required).

Possible solution would be to make the action / token generation conditional.

codecov-io commented 4 years ago

Codecov Report

Merging #21 into master will decrease coverage by 1.68%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #21      +/-   ##
==========================================
- Coverage   97.23%   95.54%   -1.69%     
==========================================
  Files          12       12              
  Lines         615      584      -31     
==========================================
- Hits          598      558      -40     
- Misses         17       26       +9     
Impacted Files Coverage Δ
graphql_auth/types.py 85.71% <0.00%> (-7.62%) :arrow_down:
graphql_auth/mixins.py 94.86% <0.00%> (-3.32%) :arrow_down:
graphql_auth/bases.py 100.00% <0.00%> (ø)
graphql_auth/relay.py 100.00% <0.00%> (ø)
graphql_auth/signals.py 100.00% <0.00%> (ø)
graphql_auth/mutations.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c377d35...dc91fcc. Read the comment docs.

PedroBern commented 4 years ago

To add the "Password Reset" email we may need to rewrite get_email_context function altogether as it now expects an action which does not exist in case there is no action (i.e., no token required).

Possible solution would be to make the action / token generation conditional.

@yanivtoledano You mean for the Password Successfully Changed (that don't exist yet)?

Everything is perfect, I didn't merge yet just because of your comment, just want to clarify :) We can work on this in a different PR. Also, I'm going to add custom templates variables in the settings to solve #22.

Just one thing, could you please add the request and time into the documentation? Here, just edit this file.

yanivtoledano commented 4 years ago

@yanivtoledano You mean for the Password Successfully Changed (that don't exist yet)?

Correct. I believe it would be nice to have the flexibility to send a Password Successfully Changed email. The way the code is currently setup however requires an action which needs a token. The Password Successfully Changed email does not need a token however as it is just informational. We should indeed take this to a different PR as it is less related to the implemented changes.

Will update the docs asap

PedroBern commented 4 years ago

Perfect :) This Password Successfully Changed email was really missing!