Bug: getting an email template by label

[mabiede]

When storing the email templates with the additional language argument, it's possible to get an error when using the EmailTemplate.get_by_label function (without passing the language argument).

It allows e.g. creating a template for multiple languages with the same label. But when reading them without the language argument you'd receive a list and not Sihl_email.Template.t option Lwt.t as it is now.

This is an edge case and could be a wanted exception. Either both (create and get) have to use the language argument or neither of them. (Sorry, I missed adding this test)

I'd suggest keeping the signature and writing a warning to the console, that the function was called without the needed language argument (and returning Lwt.return_none).

@joseferben ping

[joseferben]

@mabiede Could we somehow have a fallback mechanism, maybe a fallback language?

[aronerben]

@mabiede @joseferben I think since the users can specify their own languages, we can't generally have a fallback language. Maybe we could just return the first one that is found (from a list)?

[mabiede]

@joseferben @aronerben IMO, there doesn't need to be a fallback. Either the developer chooses to skip the optional language field or he uses it. If there is only one template for a label specified (e.g. single language) this will be returned. In case there is more than one template (same label, multiple languages), an Exception or a Log.err and None Lwt.t should be returned.

In case of a fallback, may also add an ORDER BY created_at ASC and LIMIT 1 (without being able to tell that there are other templates available for the specified key).

[aronerben]

@joseferben

@mabiede and I had another discussion. If we want to avoid throwing exceptions (which I think Sihl should avoid, or mark the function with _exn), we see two options with two suboptions:

1. The API is not changed:

   1. We add the LIMIT 1 if no language is provided (language is optional). This way, we only ever fetch one template
   2. We return the first template found*

2. The API is changed:

   1. We return a list of templates
   2. We return a result if more (or fewer) than one template is found*

* This implies adding a try with around the Caqti call.

[joseferben]

@mabiede @aronerben I like the LIMIT 1 idea if it is documented in the function docs because it does the "right thing" for probably most use cases. What do you think?

[aronerben]

@mabiede @aronerben I like the LIMIT 1 idea if it is documented in the function docs because it does the "right thing" for probably most use cases. What do you think?

I like it.