-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
[14.0] cooperator: Multi-company e-mail templates #73
[14.0] cooperator: Multi-company e-mail templates #73
Conversation
ba2fc00
to
9157219
Compare
cooperator/models/company.py
Outdated
cooperator_certificate_mail_template = fields.Many2one( | ||
comodel_name="mail.template", | ||
string="Certificate email template", | ||
domain="[('model', '=', 'res.partner')]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an actual domain or a string are allowed for this attribute. is there a particular reason to use the string version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I copied this from stock
;) No particular reason.
7e74c42
to
0168ce7
Compare
0168ce7
to
4331eca
Compare
In this commit, the confirmation e-mail template (chosen at random) is configured on the res.company model instead of fetched via its XMLID. In a future commit, all the other cooperator-related e-mail templates will be configured in the same way, using the common scaffolding from this commit. The commit introduces a bug/regression, which is that the confirmation e-mail won't actually be sent. This is a result of the new scaffolding. Before, this happened: 1. cooperator is marked installed. 2. The mail template data file gets sourced, creating the confirmation template's record. 3. The subscription request demo data file gets sourced, creating a subscription.request record. 4. While creating the record, an e-mail using the confirmation template gets sent. In the new logic, this happens: 1. cooperator is marked installed. 2. The mail template data file gets sourced, creating the confirmation template's record, but NOT setting it on company 1. 3. The subscription request demo data file gets sourced, creating a subscription.request record. 4. While creating the record, Odoo tries to send a mail of the template company_id.cooperator_confirmation_mail_template, but the field is empty. 5. Error, obviously, but we've commented out the erroneous code, so let's proceed. 6. As a post-init hook, a copy of the confirmation template gets set on all companies' cooperator_confirmation_mail_template field. We'll need to re-jig the logic to circumvent the error. A problem for future me. Or you, if you're reading this in the distant future. Hello. Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
…pany Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
…n company Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
…company Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
…bscription request creation This was rather bad design to start with, and caused a bug during the installation of this module (at which stage the mail templates were not yet correctly set on the companies). An additional state 'confirmed' is created. The use case is to immediately click on it after all the values are correct. Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
4331eca
to
8b38774
Compare
cooperator/models/company.py
Outdated
if copy: | ||
result = template.copy() | ||
# Set name to 'Company - Template' instead of 'Template (copy)'. | ||
result.name = "{} - {}".format(self.name, template.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also set company_id
here to the template?
Maybe we need https://github.com/OCA/multi-company/blob/14.0/mail_template_multi_company/models/mail_template.py#L11 for that.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, initially I did do that here, but the mail_template_multi_company
module is not in a usable state as far as I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about removing the default value for company_id
in this module? would it be too late in that the data would already be assigned to the 1st company?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module = cooperator
or mail_template_multi_company
?
Because there's nothing in cooperator
that we can change to fix the problem described in the linked issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was referring to cooperator
: redefining the mail.template.company_id
field without a default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mail.template
are not multi-company by default, there isn't a company_id
in the model if mail_template_multi_company
is not installed.
If we want to add a unique template for every company, we would need this module, but I understand that the scope of this PR is to add mail.template
selection for coopertor regardless if they are multi-company or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we want to fix this, the most 'correct' thing to do is to move this discussion to OCA/multi-company#483 and fix this upstream.
… from subscription request creation" This reverts commit bf5aa27. This would imply a functional change that is not wanted at the moment. Let's solve this in a different way.
…are created Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
314ca85
to
dbfff89
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, thanks for this @carmenbianca. lgtm with just some code style comments.
cooperator/models/company.py
Outdated
if copy: | ||
result = template.copy() | ||
# Set name to 'Company - Template' instead of 'Template (copy)'. | ||
result.name = "{} - {}".format(self.name, template.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was referring to cooperator
: redefining the mail.template.company_id
field without a default value.
cooperator/models/company.py
Outdated
cooperator_certificate_mail_template = fields.Many2one( | ||
comodel_name="mail.template", | ||
string="Certificate email template", | ||
domain="[('model', '=', 'res.partner')]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can assign by default the mail template of cooperator, so we don't need to select it every time we create a new company.
Also, if is not assigned, it gets the cooperator template? Should be this field required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @DaniilDigtyar.
Also: perhaps the domain should add the ('is_cooperator_template', '=', True) element (in addition to the correspondant model one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now that is assigned when creating the company.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure what the proposal is. This is the philosophy of the current state of the PR:
There exists a template (in truth, several, but let's simplify to 1) defined in the XML data. Let's call this the global template. This template is not itself used anywhere. Instead, when a company is created, a copy of the global template is created and set on the field in res.company.
The upside of this approach is that none of the non-global templates interfere with each other. The downside is that you get the following slight UX problem: When creating a company, you leave the mail template field empty. After clicking on 'save', the copy is created and the field is populated. If you now erase the contents of that field, nothing will stop you, and you will break the cooperator
module.
I have bypassed one of the UX problems by making the mail template field invisible until after clicking on 'save'. This means the user won't be confused by an empty field suddenly being populated, or feeling like they have to populate these fields during creation when in fact they needn't.
The other problem I am not certain how to work around. The field can't be mandatory, because it's empty during creation. Similar problem in re adding a constraint. Maybe you could do an onchange or write that throws an error; I'm not sure.
Your proposal, as I understand it, is to use the global by default on companies, either by making it the default value of the field, or by making it a fallback value in a helper getter method (both are fine; I'd probably prefer the former over the latter, but it's just about the same). This fixes the UX problems (although we may need to add a help text that 'empty' is a valid value for the template field), but does change the philosophy: where previously all templates were always separate, they are now identical-unless-declared-otherwise.
I'm fine with that change in philosophy, but it has the downside that a user at one company may change the template without realising that this affects all other companies.
@DaniilDigtyar @XavierBonetViura Let me know which approach you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you are right and this is correct implementation. It was a misunderstanding about how it was working because we didn't understand the assignation of the relations after the creation of the company record.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This philosophy of your implementation is the one that we need in Som Comunitats projects, because we need to have different mail.templates
for all companies, even if there are little variation between them.
But maybe for a more global need of cooperator
, it will make sense to use the global mail template as the default for all the companies and not copy them on every creation, because this will cause an extensive list of same mail.template
shared between all the companies (because there isn't a company_id
by default)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating systematically a .copy() of each email_template and assign those new templates to every new company that we create is an aproach that have some issues:
- We want to have the possibility to have it, ... but sure that not all companies will need to have it (personalized templates). Perhaps only FEW companies will want to have it.
- Caution with the templates Translation effort. I we build new templates for absolutelly each new company we will need to proceed to translate it per each company. I can be crazy to maintain.
- If we proceed to do CHANGES into the 'cooperator' templates by code (adding globally new fuctionallity),.... those improvements will not apply at all at any company because we have ALL the templates personalized.
Perhaps it will be better to have another aproach:
- have the new fields available at res_company to select personalized templates,... but allow to have it EMPTY (by default).
- If those fields are EMPLY, the code will use the 'corporate' (cooperator) base templates.
- If a company need to PERSONALYZE A TEMPLATE, allow a button at mail_template model to do a COPY of any of the coopetaror templates,... and assign company_id field to those new copied templates.
- filally at the res_company fields that point to those persolalyzed email_templates, force the domain in order to ONLY show to be selected the templates that match the field company_id (and that are cooperator ones)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@XavierBonetViura I have implemented the above proposal, where empty = default. Thank you for that proposal.
I have not presently implemented a 'copy' button. Let's come back to that later, maybe.
… otherwise specified This is a big revert of previous commits. Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
Signed-off-by: Carmen Bianca BAKKER <carmen@coopiteasy.be>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good to me
Internal task: https://gestion.coopiteasy.be/web#id=10674&action=475&active_id=470&model=project.task&view_type=form&menu_id=536
PR to have each company utilise unique mail templates to send cooperator-related e-mails. The use case is that each company can personalise their mail template to their own desires without affecting other companies.