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

[17.0][IMP] mail_notification_custom_subject: Do not replace the subject if it is already defined + add inside replace option #1548

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

Conversation

victoralmau
Copy link
Member

@victoralmau victoralmau commented Jan 16, 2025

Changes done:

  • Do not replace the subject if it is already set.
  • Add a log when there is an Exception to know what has happened
  • Add Inside replace position

Please @pedrobaeza and @sergio-teruel can you review it?

@Tecnativa TT52259

@OCA-git-bot
Copy link
Contributor

Hi @yajo,
some modules you are maintaining are being modified, check this out!

@pedrobaeza pedrobaeza added this to the 17.0 milestone Jan 16, 2025
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

I see the subject_to_replace feature very limited, as you can only do one replacement. But I think it's not needed as well, as you can do a whole replacement and use expressions to paint that. And anyway, for the particular case we need it, we should use a stored value in the mail template (object.partner_id.comercial or object.name if I'm not wrong).

@victoralmau
Copy link
Member Author

It is important to clarify that you can set several inside template records to make several replacements if necessary.

I will change the behavior of the subject_to_replace field so that it can be an expression.

@victoralmau victoralmau force-pushed the 17.0-imp-mail_notification_custom_subject-TT52259-v2 branch from 809d101 to 9469da8 Compare January 17, 2025 07:22
Copy link
Member

@yajo yajo left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Could you please add an entry about this in the docs?

@victoralmau victoralmau force-pushed the 17.0-imp-mail_notification_custom_subject-TT52259-v2 branch from 9469da8 to 7594aa1 Compare January 17, 2025 08:31
… it is already set

If we write something from the mail thread (without subject) it is only when it should replace the subject.

TT52259
victoralmau added a commit to Tecnativa/social that referenced this pull request Jan 17, 2025
…ing the subject

If there is an error in the template we want to render the subject to be shown and acted upon.

Related to OCA#1548 (comment)

TT52259
@victoralmau victoralmau force-pushed the 17.0-imp-mail_notification_custom_subject-TT52259-v2 branch from 7594aa1 to eb7d3ac Compare January 17, 2025 08:42
Example use Case:
Our company is called: MY_COMPANY_NAME but is known as MCN,
we want to replace in some places the subject line so that it is sent with MCN

TT52259
@victoralmau victoralmau force-pushed the 17.0-imp-mail_notification_custom_subject-TT52259-v2 branch from eb7d3ac to adb7204 Compare January 17, 2025 09:17
rendered_subject_to_replace,
rendered_subject_template,
)
except Exception:
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a double except catch? I think the general one serves enough.

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.

4 participants