HIDP-280 as a user i want to be able to change my email address using

[mkirkeng-leukeleu]

• Adds a viewset at /api/email-change/
  • POST creates new email change requests and sends the appropriate emails (to the current and proposed email addresses)
  • DELETE cancels an existing email change request
• Adds a separate view at /api/email-change-confirm/
  • PUT confirms the email change request using the tokens sent in the emails from creating the email change request
  • This could, in the future, possibly also be combined into the email change viewset but that requires a bit of refactoring for forex. the get_object() method and PATCH should not be added (which is done automatically by the UpdateModelMixin).
• Adds the corresponding serializers for the viewsets/views mentioned above
  • Generally speaking put the logic from the original HIdP forms into these serializers and the HTML view logic into the API views
• Updates the mailers to allow overriding the the links sent in the emails themselves (as these should refer to a link in the application frontend not one of the HIdP HTML views.
  • Also added some settings to allow the application creator to configure these URLs as HIdP doesn't know about the URLs of frontend.
  • Also added some checks to make sure the application creator configures these settings when using the API (giving it a default will just result in the views silently sending emails with the wrong URLs).
• Added tests for the serializers and views mostly based on the tests for the existing views, but slightly changed to fit the API and the way the logic is spread across serializers and views.

[fvanderpost]

I looked at the custom url logic for the mailers and will implement similar logic in related PRs. Some thoughts:

• Would prefer argument naming like cancel_url over confirmation_url_template, template to me conveys more a multi line file instead of single string interpolation
• In line with that I would just add a check to checks.py that, for urls needing a {token} placeholder, verifies that it exists.
• Maybe move settings.EMAIL_CHANGE_CONFIRMATION_URL etc checking to the mailer itself and build URLs in that class over having it in send_mail of the API view so we can more easily reuse the logic in both the template view and the API view -> Downside is that for template view logic we specifically do not want to take the setting based URLs since we rely on our hardcoded routes to redirect to correct spot

But overal it seems like the most straightforward implementation and seems to be in line with other packages.

[mkirkeng-leukeleu]

I looked at the custom url logic for the mailers and will implement similar logic in related PRs. Some thoughts:

* Would prefer argument naming like `cancel_url` over `confirmation_url_template`, template to me conveys more a multi line file instead of single string interpolation

* In line with that I would just add a check to `checks.py` that, for urls needing a `{token}` placeholder, verifies that it exists.

Yeah I also didn't love "template" for the same reason but wanted to communicate that one is a simple url string and the other needs the interpolation/replacement field. But as long as the checks exist I'm fine with removeing the "_template"

* Maybe move `settings.EMAIL_CHANGE_CONFIRMATION_URL` etc checking to the mailer itself and build URLs in that class over having it in `send_mail` of the API view so we can more easily reuse the logic in both the template view and the API view -> Downside is that for template view logic we specifically do not want to take the setting based URLs since we rely on our hardcoded routes to redirect to correct spot

I actually would like to kind of go the other direction in future as I'd like to separate view logic from business logic more. And which url to use depending on whether the its the API or html views is view logic while the actual mail sending is business logic. At least to me.

[fvanderpost]

Not wrong to check if this is true, but different from the view based implementation. If we refactor stuff perhaps should move this check to EmailChangeTokenMixin.validate_token_data and make the mixin more general

[mkirkeng-leukeleu]

I think it's a good thing to look at this again during a future refactor.

[fvanderpost]

For consistency (and perhaps for allowing overrides) we should use setting class variables for:

  token_generator = tokens.email_change_token_generator
  email_change_request_mailer = mailers.EmailChangeRequestMailer
  proposed_email_exists_mailer = mailers.ProposedEmailExistsMailer

These things might move in a similar way to a mixin in the future as well

[mkirkeng-leukeleu]

For now I've not considered how friendly the endpoints are to subclassing/overriding, so I decided to not support that functionality either. After all the API endpoints already are structured differently compared to the views with all their mixins. So similar to this comment, I suggest looking at this again during a possible future refactor.