Log entries for email sending

[ronjakrg]

Fixes #2509

[richardebeling]

You can keep this as a list comprehension, they support nested-ifs:

return {
    field_name: list(_field_actions_for_field(model._meta.get_field(field_name), actions))
    for for field_name, actions in self.data.items()
    if field_name in model_field_names
}

looks more complex at first, but makes it clear that all we're doing is creating a dictionary and returning that, so as a reader, you don't have think about so much control flow and state manipulation.

[ronjakrg]

Thanks, nice to know! I'll change this like you suggested

[richardebeling]

Two thoughts about the interface:

1. The function intentionally modifies body_params. To make this clear at the call site, I would explicitly return body_params again (even though we just modify the given dict and return it again)

2. Naming this one is hard, it does three things: 1. prepare the user for a login URL, 2. determine if a separate login-url mail needs to be sent, and 3. update the body params for the template to render appropriately. Typically, I consider check to be a smelly name, because it's a word that says nothing, but I don't really have a much better suggestion here, but I don't know if splitting it up further helps

send_separate_login_url can be computed directly as

send_separate_login_url = user.needs_login_key and cc_addresses

maybe the other part should really be

body_params |= user.get_email_template_login_url_context()

and the code here should decide whether that gets added to the context or not, based on send_separate_login_url? All a bit tricky, it is all connected, so maybe it makes sense to keep it all in one place...

[ronjakrg]

@richardebeling If I understand the logic correctly here, I cannot work with send_separate_login_url within the new method because

• ensure_valid_login_key() happens if user.needs_login_key (and doesn't care what's in cc_addresses)
• but setting body_params["login_url"] = self.login_url only happens if user.needs_login_key and not cc_addresses
  But I agree with you that naming the thing check wasn't the best idea ... I would suggest the following:

In UserProfile

def get_email_template_login_url_context(
    self,
    body_params: dict[str, Any],
    cc_addresses: list[str | Any],
) -> dict:
    body_params["login_url"] = ""
    if self.needs_login_key:
        self.ensure_valid_login_key()
        if not cc_addresses:
            body_params["login_url"] = self.login_url
    return body_params

In EmailTemplate

send_separate_login_url = user.needs_login_key and bool(cc_addresses)
user.get_email_template_login_url_context(
    body_params=body_params,
    cc_addresses=cc_addresses,
)

[richardebeling]

Looks good and sounds like what I had in mind, if we keep in mind the point of "using" the returned dict, i.e.

body_params = user.get_email_template_login_url_context(
    body_params=body_params,
    cc_addresses=cc_addresses,
)

Thanks for working on this :)