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

[3132][ADD] account_move_mail_template_with_subject #10

Open
wants to merge 3 commits into
base: 16.0
Choose a base branch
from

Conversation

rinaldifirdaus
Copy link
Contributor

@rinaldifirdaus rinaldifirdaus commented Jan 26, 2023

3132

This module does the following:

  • Adds a new field 「Subject」in Account Move form view.
    this field will automatically filled if the invoices created from Sales Order (relate to Sale Order [Subject] field).
  • Adds an email template for sending invoices and adding account move's subject field into the email subject.

Functional test video:
https://drive.google.com/file/d/1yPu6UlEUHerZIPY9YYuUnt1EOqKignYm/view?usp=sharing

Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We originally had account_move_subject and account_move_mail_template_with_subject as separate modules. Why are they combined now? I think they should be kept separated.

Comment on lines 7 to 15
def _get_mail_template(self):
"""
:return: the correct mail template based on the current move type
"""
return (
"account_move_mail_template_with_subject.email_template_credit_note"
if all(move.move_type == "out_refund" for move in self)
else "account_move_mail_template_with_subject.email_template_send_invoice"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid complete override as a general guideline.

@AungKoKoLin1997 Please help explain how this should be handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im sorry for that, i was thinking to mix those module into one module since the account_move_subject and account_move_mail_template_with_subject using same models, account_move.

Please let me confirm, So in this case i need to split into two module as the original one ( account_move_subject and account_move_mail_template_with_subject) and also for the sale order module need to split same with the account_move. and after that add those 4 modules into fal-custom as follow:

  1. account_move_subject
  2. account_move_mail_template_with_subject
  3. sale_order_subject
  4. sale_order_mail_template_with_subject

Thank you, please correct me if im wrong.

Comment on lines 7 to 15
def _get_mail_template(self):
"""
:return: the correct mail template based on the current move type
"""
return (
"account_move_mail_template_with_subject.email_template_credit_note"
if all(move.move_type == "out_refund" for move in self)
else "account_move_mail_template_with_subject.email_template_send_invoice"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _get_mail_template(self):
"""
:return: the correct mail template based on the current move type
"""
return (
"account_move_mail_template_with_subject.email_template_credit_note"
if all(move.move_type == "out_refund" for move in self)
else "account_move_mail_template_with_subject.email_template_send_invoice"
)

I don't think you need to override this function. We can just inherit email template.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to override this function. We can just inherit email template.

@AungKoKoLin1997 I think this approach is actually an overkill (remote noupdate from the template and assign it again...). I actually do not like the idea of adding a new mail template by a module, either (whatever template we add, the user will want to change the content). Can you suggest a way to inject the subject value in to the email subject without manipulating with the template?

Copy link
Contributor Author

@rinaldifirdaus rinaldifirdaus Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AungKoKoLin1997 @yostashiro , I couldn't find any reference to inject the subject value to the email subject. Is it possible by inherit function _get_mail_template to get the email template and then replace the subject?

Copy link
Contributor

@AungKoKoLin1997 AungKoKoLin1997 Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this is a valid way or not. But we can inject like this.

def _get_mail_template(self):
     template_name = super(AccountMove, self)._get_mail_template()
     template = self.env.ref(template_name)
     template.write({'subject': "{{ object.subject or 'n/a' }}"})
     return template_name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @AungKoKoLin1997 ! its working with on the account move.
currently i am trying to apply same method to sale order as well.
`from odoo import models

class SaleOrder(models.Model):
_inherit = "sale.order"

def _find_mail_template(self):
    template_name= super(SaleOrder, self)._find_mail_template()
    template = self.env.ref(template_name)
    template.write({'subject': "{{ object.subject or 'n/a' }} - お見積りの送付 (Ref {{ object.name or 'n/a' }})"})
    return template_name`

but im getting this error.
module, name = xmlid.split('.', 1)
AttributeError: 'mail.template' object has no attribute 'split'

can you please give an advice on this @AungKoKoLin1997 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rinaldifirdaus We need to confirm this way is valid or not first.
@yostashiro How do you think that approach?

else "account_move_mail_template_with_subject.email_template_send_invoice"
)

subject = fields.Char(tracking=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field already exists in account_move_subject module. Just inherit this module in manifest.

@@ -0,0 +1,138 @@
<?xml version="1.0" encoding="utf-8" ?>
<odoo noupdate="1">
<record id="email_template_send_invoice" model="mail.template">
Copy link
Contributor

@AungKoKoLin1997 AungKoKoLin1997 Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to create new email template for replacing email subject. You can see how to inherit mail template(https://www.cybrosys.com/blog/how-to-inherit-update-an-existing-mail-template-in-odoo-15#:~:text=To%20add%20changes%20to%20the,template%20in%20the%20account%20module.)

@github-actions
Copy link

github-actions bot commented Jun 4, 2023

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale label Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants