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

[10.0][MIG] mail_debrand #168

Merged
merged 1 commit into from
Jan 22, 2018
Merged

Conversation

pedrobaeza
Copy link
Member

There's no commits preservation, as the module doesn't serve as is on v9.

cc @Tecnativa

@dreispt
Copy link
Sponsor Member

dreispt commented May 15, 2017

Some templates also include the "Odoo" brand, such as the example below. Ideally, we should also tweak them in a mail_debrand feature:

image

dreispt
dreispt previously approved these changes May 15, 2017

* This module relies on the translation of the strings here in this module that
must match the mail notification template translation on Odoo core,
specifically the words "using" and "Odoo".
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I feel that it would be better to have these as System parameters - at least the Odoo word.
I worry the the words will creep in again in case an email is generated for a language we didn't edit the corresponding translation,.
Are you sure the translation trick also works for base english?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it can't be a parameter, as you can translate your template to any installed language, and each one should have this translated to the correct words.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I still worry about the case where no translation string is provided. Can't we use a parameter string in that case?

Copy link
Member Author

@pedrobaeza pedrobaeza May 15, 2017

Choose a reason for hiding this comment

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

No need for the parameter string. If no translation is found, the used terms are "using" and "Odoo". If you want to care about the translation, just synchronize translations in your Odoo instance, and translate the corresponding term.

Copy link
Sponsor Member

@dreispt dreispt May 16, 2017

Choose a reason for hiding this comment

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

If no translation is found, the used terms are "using" and "Odoo"

That's exactly my problem...

Copy link
Member Author

@pedrobaeza pedrobaeza May 16, 2017

Choose a reason for hiding this comment

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

But plain English works as is...

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Sorry, I don't understand what you mean.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, let me try to make you understand:

  • With English language (with no translations), the matched expression that is removed is using...<a lot of characters between>...Odoo (not the isolated words).
  • If you translate "using" and "Odoo" words to your language in both places: this module and mail template. For example, in Spanish, the matched expression will be usando...Odoo.
  • If the mail template is translated, but not this module, you just need to go to the translation and translate the words.
  • If none is translated, it will be correctly replaced.

Copy link
Sponsor Member

@dreispt dreispt May 16, 2017

Choose a reason for hiding this comment

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

OK, I think I get it now: the RE is supposed to remove the the "using Odoo" words in a translation-resistant way.
Sorry for all the trouble.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, no problem. Take also into account that you can send mails in a lot of languages inside same Odoo instance (if your customer is English, French, Portuguese...), so you need a translation mechanism, not a static parameter, for replacing this in all of them.

Copy link
Sponsor Member

@dreispt dreispt left a comment

Choose a reason for hiding this comment

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

(Question about the translation approach)

@dreispt dreispt dismissed their stale review May 15, 2017 15:23

Sorry, clicked on the worng button

@pedrobaeza
Copy link
Member Author

@dreispt please check code that is a regexp expression, so no possible to be confused.

@pedrobaeza
Copy link
Member Author

Travis error is not coming from my module and it's supposed to not fail (it's in mail_tracking). Maybe something has changed on the way Travis parses the errors?

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

tested on runbot in base language and in Spanish

@pedrobaeza pedrobaeza changed the title [MIG] mail_debrand [10.0][MIG] mail_debrand Aug 31, 2017
There's no commits preservation, as the module doesn't serve as is on v9.
@rafaelbn rafaelbn added this to the 10.0 milestone Aug 31, 2017
@pedrobaeza
Copy link
Member Author

@dreispt can you please give your blessing?

Copy link
Sponsor Member

@dreispt dreispt left a comment

Choose a reason for hiding this comment

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

LGTM

@dreispt dreispt merged commit 6181016 into OCA:10.0 Jan 22, 2018
@pedrobaeza pedrobaeza deleted the 10.0-mig-mail_debrand branch January 22, 2018 09:44
@pedrobaeza pedrobaeza mentioned this pull request Jan 22, 2018
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants