Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add identifier to EmailTemplate #324

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jrief
Copy link
Collaborator

@jrief jrief commented Jul 23, 2020

Add unique identifier field to model EmailTemplate.
It can be used to access templates from Python code and leaves the name field as a human readable representation of that template.
This additionally simplifies the unique constraints on that model.
The idea is the same as for #320, however instead of adding label as a human readable field, it adds an identifier as an explicitly machine readable field.
In addition to this, the name and description field are now editable in all languages.

jrief added 2 commits July 23, 2020 11:49
Add unique `identifier` field to model `EmailTemplate`.
It can be used to access templates from Python code and leaves the
`name` field as a human readable representation of that template.
This additionally simplifies the unqiue constraints on that model.
@madEng84
Copy link

Hi @jrief
If that unique_together is deleted, a MultipleObjectsReturned Exception can be raised in utils.get_email_template(name, language="") function or that function should be changed in something like utils.get_email_template(identifier)
This is the reason because i didn't want to change that business logic, because I think we have to ensure retrocompatibility or we have to pass post_office to an another major version.

I think, however, that this could be the best approach.

@jrief
Copy link
Collaborator Author

jrief commented Jul 23, 2020

@madEng84 OK, valid point.

However, since after the change we will have a human readable label (name or label), and a machine readable identifier (identifier or name), we must expose these fields to the admin interface.
Any proposals on how to achieve this in a smooth way?

@madEng84
Copy link

@jrief i think there will be both, because the identifier is useful for devs (it will be obviously in the backend code) and the name is useful for staff users.

The human readable one should be a normal input text.
The machine readable one could be an input text for now, but maybe in the future it could become a Select that allows dev to customize it, not giving the possibility for staff users to change it and break everything in the basecode.

@jrief
Copy link
Collaborator Author

jrief commented Jul 24, 2020

One proposal for a smooth transition would be to rename name to identifier and NULL the fields of translated templates.
Then add a property named name returning the content of identifier. Additionally, that property will raise a deprecation warning.

However, since this is a breaking API change, consensus from @selwin is required.

@selwin
Copy link
Collaborator

selwin commented Jul 26, 2020

I'm still struggling to see why we need another field for this. We currently already have three fields:

  1. email_template.name - something short and human readable
  2. email_template.description - can be something that's more verbose and intended for humans
  3. email_template.id - this comes for free with most DBs, will never change and can be referenced from Python codes

@jrief
Copy link
Collaborator Author

jrief commented Jul 26, 2020

@selwin

I'm still struggling to see why we need another field for this.

First, I wasn't convinced about this pull-request from @madEng84 either – see my comments here.

The problem occurs, if you need a reference from your Python code, to one of the EmailTemplate objects. Field name is too descriptive and could be translated in other natural languages. The DB id-field is non-portable across instantiations. Therefore a pure identifier makes sense.
Django-CMS for instance also offers an optional identifier-field, so that Python code and Django templates can access these CMS pages directly.

@selwin
Copy link
Collaborator

selwin commented Jul 31, 2020

Ok, the "non portable across instantiations" part is fair enough. I think we can proceed with adding identifier unique field.

Please don't change the translated name bit, since it should always follow the parent template. So for example if you have a template named welcome_email, you can get the translation using:

EmailTemplate.objects.get(name='welcome_email', language=my_lang)

If you want to have a single identifier that can uniquely identify a template, we'll have the identifier field.

@jrief
Copy link
Collaborator Author

jrief commented Jul 31, 2020

Please don't change the translated name bit, since it should always follow the parent template. So for example if you have a template named welcome_email, you can get the translation using ....

but then the identifier-field is completely useless. The reason for separating name from identifier is to have a human readable name and an identifier for the code. By nature, human readable names can be in different natural languages.

So I don't understand what part of this pull request shall be changed.

@selwin
Copy link
Collaborator

selwin commented Aug 22, 2020

Sorry for the delay in replying.

To reiterate my previous comment, we currently have three fields.

  1. email_template.name - something short and human readable.
  2. email_template.description - can be something that's more verbose and intended for humans
  3. email_template.id - this comes for free with most DBs, will never change and can be referenced from Python codes

I'll try to go through your concerns and see how we can address them.

Field name is too descriptive and could be translated in other natural languages.
This field is meant to be short and not meant to be changed to other languages. We can add a unique index for name and language fields to reinforce the intention. Perhaps the documentation is also not clear about the design and it's something that we can clarify.

The upside of having a unique index for name and language is that we combine them into template's natural key so taken together, they can be used as portable identifiers across projects.

The DB id-field is non-portable across instantiations. Therefore a pure identifier makes sense. Django-CMS for instance also offers an optional identifier-field, so that Python code and Django templates can access these CMS pages directly.

Natural keys partly address this requirement. I can sympathize with wanting to have a single identifier field for convenience, for this I propose a unique field like identifier or slug.

The reason for separating name from identifier is to have a human readable name and an identifier for the code. By nature, human readable names can be in different natural languages.
We already have email_template.description for this.

@jrief
Copy link
Collaborator Author

jrief commented Sep 1, 2020

Natural keys partly address this requirement. I can sympathize with wanting to have a single identifier field for convenience, for this I propose a unique field like identifier or slug.

That's just what I did. I added an identifier field to the EmailTemplate. It is unique and shall be used to explicitly identify an EmailTemplate-object through code. Here I prefer identifier over slug, because slug in Django can have translations.

This identifier field is only set on the master (aka fallback) template. By identifying the master template, one can later determine, in which language it shall be rendered.

@michaelpoi michaelpoi deleted the add-EmailTemplate.identifier branch October 22, 2024 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants