[MIG][14.0][mail_footer_notified_partner] Migration to v14 with renaming of module #855

cvinh · 2022-02-28T16:44:55Z

Superseeds #801

tafaRU · 2022-02-28T16:55:48Z

@cvinh thanks a lot for the PR.
Check Travis that is failing due to https://app.travis-ci.com/github/OCA/social/jobs/561474317#L1690
Also please squash the last commits, according to https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-14.0 you should only have:

[IMP] mail_footer_notified_partner: black, isort, prettier
[MIG] mail_footer_notified_partner: Migration to 14.0

oca-clabot · 2022-02-28T23:13:37Z

Hey @cvinh, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

Jairo Llopis j.llopis@grupoesoc.es (no github login found)
Denver Risser denver@compassfoundation.io (no github login found)
Jaime Arroyo jaime.arroyo@creublanca.es (no github login found)
Invitu contact@invitu.com (no github login found)
Jairo Llopis jairo.llopis@tecnativa.com (no github login found)

Appreciation of efforts,
OCA CLAbot

cvinh · 2022-02-28T23:42:54Z

@cvinh thanks a lot for the PR. Check Travis that is failing due to https://app.travis-ci.com/github/OCA/social/jobs/561474317#L1690 Also please squash the last commits, according to https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-14.0 you should only have:

[IMP] mail_footer_notified_partner: black, isort, prettier

[MIG] mail_footer_notified_partner: Migration to 14.0
ok, it's done
I prefer to keep this commit separately because it's an improvement :(f8611b7)

tafaRU · 2022-03-01T08:22:26Z

@cvinh

I prefer to keep this commit separately because it's an improvement :(f8611b7)

More than improvement it seems to me a significant change that goes against the module name itself. I would keep the module as is. At least, you could propose this change in another PR after this will be merged.
In addition, you should also avoid the following commit:

tafaRU · 2022-03-02T11:50:11Z

mail_footer_notified_partner/tests/test_mail_notification.py

-        )
-
-        self.assertTrue(rep, "message not send")
-        self.assertTrue(


I don't eliminate at all the tests. I think it's worth to keep at least the following part.

@cvinh what do you think?

@cvinh what do you think?

Yes I'm working on it

@tafaRU tests have to be rewritten, I'm not good with it, do you want to finish it ?

HaraldPanten · 2022-03-08T09:11:45Z

Maybe this one is better --> https://github.com/OCA/social/tree/12.0/mail_show_follower . Did you have a look?

cvinh · 2022-03-10T16:07:49Z

Maybe this one is better --> https://github.com/OCA/social/tree/12.0/mail_show_follower . Did you have a look?

@HaraldPanten Thanks for pointing that one

rafaelbn · 2022-03-11T13:18:05Z

/ocabot migration mail_footer_notified_partner

rafaelbn · 2022-03-15T16:06:30Z

@tafaRU please take a look: #862

cvinh · 2022-03-15T17:30:05Z

@rafaelbn @tafaRU IMHO mail_show_followers is great but it will not warn email recipients that if they reply to the catchall address, all the followers (internal or external) will ne notified by odoo... which is very dangerous, see my comment here to see why #289 (comment)

rafaelbn · 2022-03-15T18:18:00Z

This is a good point #855 (comment) what do you think @Shide @yajo @ValentinVinagre @HaraldPanten ?

HaraldPanten · 2022-03-15T22:08:36Z

This is a good point #855 (comment) what do you think @Shide @yajo @ValentinVinagre @HaraldPanten ?

Well... Normally workers shouldn't be unpolite, rude or insult anybody by using their company email address. If you do so, you know that you'll have to accept the responsibility of the consequences.

On the other hand, an option that allows users to add a "warning" to inform the recipients that replies to the catchall address will be sent to all the followers could be an improvement to mail_show_follower module.

Shide · 2022-03-16T07:55:12Z

On the other hand, an option that allows users to add a "warning" to inform the recipients that replies to the catchall address will be sent to all the followers could be an improvement to mail_show_follower module.

Agreed with that. This could come in a separate PR in order to improve the module.

Migration code LGTM, but I think module PR #862 has more options. Once this PR #862 is on v15, I will backport/migrate to v14.
From that starting point, we can think how to improve these modules. Maybe adding an option to show the CC warning on header / footer.

On the other hand:
@sbidoul can you check the runboat/build rsync ip failure? It's failing on a bunch of PRs on other repos.

Shide

LGTM

yajo

It seems to me that this module is not necessary. The one in #862 seems a better implementation IMHO, and the only extra feature that this module adds (saying: "Also notified") doesn't justify a whole new module.

The feature itself means adding 2 words to #862. I think we should close this PR and fix it in #862 instead. Then backport, as @Shide said.

ValentinVinagre · 2022-03-16T10:31:33Z

It seems to me that this module is not necessary. The one in #862 seems a better implementation IMHO, and the only extra feature that this module adds (saying: "Also notified") doesn't justify a whole new module.

The feature itself means adding 2 words to #862. I think we should close this PR and fix it in #862 instead. Then backport, as @Shide said.

I think the same as @yajo , I don't see this module as necessary having the mail_show_follower . A warning could be added to the footer as an improvement, if it is activated and thus reduce the use case to a module, but if you want to use 2 separate modules, I have no problem either.

As @HaraldPanten indicates, workers should not send emails with inappropriate words using the company email (or any other). If they do so, they must assume the consequences of their actions.

we need to save the complete list of partners because _message_notification_recipients builds recipients grouped by users groups. Thus get_additional_footer would get a partial list of recipients

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: social-12.0/social-12.0-mail_footer_notified_partner Translate-URL: https://translation.odoo-community.org/projects/social-12-0/social-12-0-mail_footer_notified_partner/

Currently translated at 50.0% (1 of 2 strings) Translation: social-12.0/social-12.0-mail_footer_notified_partner Translate-URL: https://translation.odoo-community.org/projects/social-12-0/social-12-0-mail_footer_notified_partner/fr/

… body indeed (OCA#289 (comment))

yajo · 2022-10-31T12:23:13Z

mail_body_notified_partner/tests/test_mail_notification.py

+
+
+class TestMailNotification(common.TransactionCase):
+    def test_get_signature_footer(self):


For this migration to be accepted, instead of removing the tests code and then adding it back in a new commit, you should not remove it in the previous commit. That way, you preserve the lines history.

yajo · 2022-10-31T12:23:39Z

mail_body_notified_partner/tests/test_mail_notification.py

+        self.mail_auto_delete = True
+
+    def test_get_signature_footer(self):
+        rep = self.recipient._notify(


This method doesn't seem to exist anymore in v14. You will probably need to adapt a bit this test code.

Thank you for your comment. Unfortunately this is out of my knowledge... could you please suggest the fix ?

I don't know the fix. I'd have to dig deep in the code of Odoo 16 to know that. However, how would I do it if I were you?

First thing would be to understand how unit tests work. It seems to me you never did these kind of tests, which is fine, but if you're migrating modules, this is an essential part of the migration work. Keep in mind that, if you've used modules and they work fine, it's because many others have devoted their time to write these tests. If you learn to do the same, you'll get to the next level! So, try starting by reading docs about tests. Install the module locally (I imagine you already did, but just in case), and run the tests. See the failure and fix it.

Now... how to fix it? As you can see, in v12 _notify() was declared here. However, in v13 that method no longer exists. So I'd go and review the commits (or PRs) between v12 and v13 and see which one removed that method. It should lead you to the explanation about why it was removed and what replaces that functionality.

Once you find the replacement, just adapt this call and you'll get it fixed! 😃

rafaelbn · 2022-12-02T21:22:35Z

Hello @OCA/social-maintainers , anyone with technical skills could help to @cvinh ? @sbidoul are you going to adopt this module in the future ( #855 (comment) )?

rafaelbn · 2023-06-03T17:22:38Z

Resume:

1. The main issue here is that commit history is not preserved. @sbidoul commented:

Side note: this PR seem to drop the original commit history (e.g. the initial commit of @JonathanNEMRY.

I'm a user of mail_notified_partner, and I have never used mail_show_follower. Do I understand correctly that the latter adds a cc: header to outgoing mails ? Does dis not cause issues when the recipient replies (such as double deliveries) ? Do people know why Odoo does not do that natively ?

The original PR fron @JonathanNEMRY of this module named mail_footer_notified_partners was on 2016 in this PR and all history should be preserved: ### [NEW][mail_footer_notified_partners] #37 #37

2. We should like to rename the module mail_footer_notified_partner to mail_notified_partner because adding an option to print in foooter or in header
3. mail_show_follower module make the same of this module and in some moment the functionality where duplicated in OCA
4. mail_show_follower has 3 BUGs:
- Show in emails followers that are not recipients with for example internal notes MT-579. Yo send a note internally to 4 team members and appears that is has been sent to customer too
- Don't show followers (they are) if the follower use Odoo messaging without emails
- Notify followers that have been archived in the database (so don't receive emails)
5. This module has something like 2.400 lines of code to add just in the body (header or footer) the recipients of the email. See image.

For Odoo 16 I would like a module name mail_notified_partner or mail_show_notification_partner that make the same of mail_show_follower with out BUGs. The name mail_show_follower IMHO in incorrect. Why:

We would like to show in the emails (mail_) the notifications_ids (_notification) with the partners (_partner) are in the list.

Model: mail.message

Other points of interest:

Comparision made with Basecamp or Notion done: [MIG][14.0][mail_footer_notified_partner] Migration to v14 with renaming of module #855 (comment)
[MIG][14.0][mail_footer_notified_partner] Migration to v14 with renaming of module #855 (comment)

cvinh · 2023-06-03T18:05:09Z

Thanks for this resume, it's very welcome

Don't show followers (they are) if the follower use Odoo messaging without emails

IMHO all the followers should be shown, as long as they are notified (by Odoo's messaging or classical email).
If wanted, it should be a setup choice... but it would add complexity to mail_body_notified_partner which is very simple as it is.

cvinh · 2023-06-04T17:24:03Z

This module has something like 2.400 lines of code to add just in the body (header or footer) the recipients of the email. See image.

If you talk about mail_body_notify_partner, you might not count the i18n files.
It's only a view of 29 lines of code, no python code

cvinh · 2023-06-04T17:33:20Z

We should like to rename the module mail_footer_notified_partner to mail_notified_partner because adding an option to print in foooter or in header

This could be done easily by inheriting the view and set position=after instead of position=before.
IMHO it's not worth adding a setup for that.
Actually, the module can be renamed mail_notified_partner or even better mail_show_notified_partner(s) as it is. I can modify the PR if needed.

cvinh mentioned this pull request Feb 28, 2022

14.0 mig mail footer notified partner #801

Closed

cvinh force-pushed the 14.0-mig-mail_footer_notified_partner branch from f5fcc94 to 30fc948 Compare February 28, 2022 23:13

cvinh force-pushed the 14.0-mig-mail_footer_notified_partner branch 2 times, most recently from c7cff71 to f8611b7 Compare February 28, 2022 23:41

cvinh force-pushed the 14.0-mig-mail_footer_notified_partner branch 2 times, most recently from a596f19 to d024694 Compare March 1, 2022 22:17

cvinh changed the title ~~14.0 mig mail footer notified partner~~ [MIG] 14.0 mail footer notified partner Mar 1, 2022

tafaRU suggested changes Mar 2, 2022

View reviewed changes

cvinh force-pushed the 14.0-mig-mail_footer_notified_partner branch from d024694 to 2932bb4 Compare March 10, 2022 16:04

cvinh force-pushed the 14.0-mig-mail_footer_notified_partner branch from 2932bb4 to 2e9aff2 Compare March 10, 2022 16:30

rafaelbn added the needs review label Mar 11, 2022

OCA-git-bot added this to the 14.0 milestone Mar 11, 2022

OCA-git-bot mentioned this pull request Mar 11, 2022

Migration to version 14.0 #620

Closed

37 tasks

rafaelbn added the duplicate label Mar 11, 2022

Shide approved these changes Mar 16, 2022

View reviewed changes

yajo requested changes Mar 16, 2022

View reviewed changes

rafaelbn removed the duplicate label Mar 16, 2022

oca-transbot and others added 16 commits October 28, 2022 16:22

OCA Transbot updated translations from Transifex

a1b7665

FIX mail_footer_notified_partner

4e6c3b0

we need to save the complete list of partners because _message_notification_recipients builds recipients grouped by users groups. Thus get_additional_footer would get a partial list of recipients

[UPD] Update mail_footer_notified_partner.pot

bb76a42

[MIG] mail_footer_notified_partner: Migration to 11.0

ffab2a1

[IMP] notification should be at the beginning of the body indeed

cce3e82

[FIX] fix travis

a325d63

[MIG] mail_footer_notified_partner: Migration to 12.0

b54da08

[MIG] mail_footer_notified_martner: add to use _notify() v12 method

a643875

[MIG] mail_footer_notified_partner: Fix test

4881b21

[UPD] Update mail_footer_notified_partner.pot

fab292a

[UPD] README.rst

9623543

[ADD] icon.png

df3ded4

Update translation files

c23f0f3

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: social-12.0/social-12.0-mail_footer_notified_partner Translate-URL: https://translation.odoo-community.org/projects/social-12-0/social-12-0-mail_footer_notified_partner/

Update translation files

31e0e93

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: social-12.0/social-12.0-mail_footer_notified_partner Translate-URL: https://translation.odoo-community.org/projects/social-12-0/social-12-0-mail_footer_notified_partner/

Translated using Weblate (French)

138ee84

Currently translated at 50.0% (1 of 2 strings) Translation: social-12.0/social-12.0-mail_footer_notified_partner Translate-URL: https://translation.odoo-community.org/projects/social-12-0/social-12-0-mail_footer_notified_partner/fr/

[IMP] mail_footer_notified_partner: black, isort, prettier

a561685

cvinh force-pushed the 14.0-mig-mail_footer_notified_partner branch from f10838b to ef562bc Compare October 29, 2022 02:23

cvinh added 2 commits October 28, 2022 16:28

[MIG] mail_footer_notified_partner: Migration to 14.0

d8f246b

[REN] Rename module as notification should be at the beginning of the…

772cac2

… body indeed (OCA#289 (comment))

cvinh force-pushed the 14.0-mig-mail_footer_notified_partner branch from 2279d76 to 0d84ba3 Compare October 29, 2022 02:28

[FIX] restore tests

43e0d1a

cvinh force-pushed the 14.0-mig-mail_footer_notified_partner branch from 0d84ba3 to 43e0d1a Compare October 29, 2022 02:29

yajo requested changes Oct 31, 2022

View reviewed changes

rafaelbn added the needs fixing label Jun 3, 2023

yajo mentioned this pull request Jun 20, 2024

[16.0][FIX][REF] mail_show_follower: internal notes #1391

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIG][14.0][mail_footer_notified_partner] Migration to v14 with renaming of module #855

[MIG][14.0][mail_footer_notified_partner] Migration to v14 with renaming of module #855

cvinh commented Feb 28, 2022 •

edited

Loading

tafaRU commented Feb 28, 2022 •

edited

Loading

oca-clabot commented Feb 28, 2022

cvinh commented Feb 28, 2022

tafaRU commented Mar 1, 2022

tafaRU Mar 2, 2022

tafaRU Mar 8, 2022

cvinh Mar 9, 2022

cvinh Mar 10, 2022

HaraldPanten commented Mar 8, 2022

cvinh commented Mar 10, 2022

rafaelbn commented Mar 11, 2022

rafaelbn commented Mar 15, 2022

cvinh commented Mar 15, 2022

rafaelbn commented Mar 15, 2022

HaraldPanten commented Mar 15, 2022

Shide commented Mar 16, 2022 •

edited

Loading

Shide left a comment

yajo left a comment

ValentinVinagre commented Mar 16, 2022

yajo Oct 31, 2022

yajo Oct 31, 2022

cvinh Oct 31, 2022

yajo Nov 7, 2022

rafaelbn commented Dec 2, 2022

rafaelbn commented Jun 3, 2023 •

edited

Loading

cvinh commented Jun 3, 2023 •

edited

Loading

cvinh commented Jun 4, 2023 •

edited

Loading

cvinh commented Jun 4, 2023 •

edited

Loading



		class TestMailNotification(common.TransactionCase):
		def test_get_signature_footer(self):

[MIG][14.0][mail_footer_notified_partner] Migration to v14 with renaming of module #855

Are you sure you want to change the base?

[MIG][14.0][mail_footer_notified_partner] Migration to v14 with renaming of module #855

Conversation

cvinh commented Feb 28, 2022 • edited Loading

tafaRU commented Feb 28, 2022 • edited Loading

oca-clabot commented Feb 28, 2022

cvinh commented Feb 28, 2022

tafaRU commented Mar 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaraldPanten commented Mar 8, 2022

cvinh commented Mar 10, 2022

rafaelbn commented Mar 11, 2022

rafaelbn commented Mar 15, 2022

cvinh commented Mar 15, 2022

rafaelbn commented Mar 15, 2022

HaraldPanten commented Mar 15, 2022

Shide commented Mar 16, 2022 • edited Loading

Shide left a comment

Choose a reason for hiding this comment

yajo left a comment

Choose a reason for hiding this comment

ValentinVinagre commented Mar 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelbn commented Dec 2, 2022

rafaelbn commented Jun 3, 2023 • edited Loading

cvinh commented Jun 3, 2023 • edited Loading

cvinh commented Jun 4, 2023 • edited Loading

cvinh commented Jun 4, 2023 • edited Loading

cvinh commented Feb 28, 2022 •

edited

Loading

tafaRU commented Feb 28, 2022 •

edited

Loading

Shide commented Mar 16, 2022 •

edited

Loading

rafaelbn commented Jun 3, 2023 •

edited

Loading

cvinh commented Jun 3, 2023 •

edited

Loading

cvinh commented Jun 4, 2023 •

edited

Loading

cvinh commented Jun 4, 2023 •

edited

Loading