OCA
[MIG][14.0][mail_footer_notified_partner] Migration to v14 with renaming of module
@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
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 [email protected] (no github login found)
- Denver Risser [email protected] (no github login found)
- Jaime Arroyo [email protected] (no github login found)
- Invitu [email protected] (no github login found)
- Jairo Llopis [email protected] (no github login found)
Appreciation of efforts, OCA CLAbot
@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.0ok, it's done I prefer to keep this commit separately because it's an improvement :(https://github.com/OCA/social/pull/855/commits/f8611b7c6f9b58dbbbb9683686b805783faeaee2)
@cvinh
I prefer to keep this commit separately because it's an improvement :(https://github.com/OCA/social/commit/f8611b7c6f9b58dbbbb9683686b805783faeaee2)
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:
Maybe this one is better --> https://github.com/OCA/social/tree/12.0/mail_show_follower . Did you have a look?
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 @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 https://github.com/OCA/social/pull/289#issuecomment-401576313
This is a good point https://github.com/OCA/social/pull/855#issuecomment-1068256758 what do you think @Shide @Yajo @ValentinVinagre @HaraldPanten ?
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.
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 https://github.com/OCA/social/pull/862 has more options. Once this PR https://github.com/OCA/social/pull/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.
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.
IMO this module is valid as the other one. And is a mature one from v8. Both modules can be perfectly. We don't want to use this one so we will not install it. So simple 😄
The question for @JonathanNEMRY (original author from ACSONE) or @hbrunn is to review this PR and review if they are interested in the other one in order to reduce de maintenance as the objetive is the same. Just that.
@cvinh travis and runboat are 🔴
I'm agree with @cvinh about https://github.com/OCA/social/pull/855#issuecomment-1068256758 . This is not something about using "bad words" or unpolite (futhermore this is an opinion), this is about inform to the user (the client of the client of the client, not our client) that is she/he answer to an email that is generic (like catchall@) she/he should know who is going to reveceive the information. I really agree and we would like to make the improvement the mail_show_follower module.
IMO this module is valid as the other one. And is a mature one from v8. Both modules can be perfectly. We don't want to use this one so we will not install it. So simple 😄
The question for @JonathanNEMRY (original author from ACSONE) or @hbrunn is to review this PR and review if they are interested in the other one in order to reduce de maintenance as the objetive is the same. Just that.
@cvinh travis and runboat are 🔴
I'm agree with @cvinh about #855 (comment) . This is not something about using "bad words" or unpolite (futhermore this is an opinion), this is about inform to the user (the client of the client of the client, not our client) that is she/he answer to an email that is generic (like catchall@) she/he should know who is going to reveceive the information. I really agree and we would like to make the improvement the
mail_show_followermodule.
Yep, both modules can be useful depending on each company.
To me, mail_show_follower is more useful and it can be improved to show an optional "warning" in order to give information to the recipients about how Odoo manages the replies.
Maybe I didn't explain myself well enough. I'm not against this module, I was just giving my opinion 👍
this is about inform to the user (the client of the client of the client, not our client) that is she/he answer to an email that is generic (like catchall@) she/he should know who is going to reveceive the information
This is very important, we really had someone fired here a few years ago ... As you might know, email clients (gmail, outlook, thunderbird, zimbra...) don't show the email address but they show a name. So the user (not the Odoo user but the other one) doesn't know who he is replying to because he does not see '[email protected]' but 'John Doe'.
IMO this module is valid as the other one. [...] We don't want to use this one so we will not install it. [...] I really agree and we would like to make the improvement the
mail_show_followermodule.
Sorry @rafaelbn I don't understand your comment. So, do you mean that you want the feature or not? 🤔 And, if you want the feature, why do you think it's better to duplicate it in mail_show_follower instead of installing both modules together?
Does everybody realize that this current PR of 2400 lines only adds 2 words over what you can already find in mail_show_follower? 😅
I still don't see the point on having 2 separate modules that do the same thing, except for those 2 words in the message. Doesn't it make more sense to just add them in mail_show_follower and close this one? I must be missing something...
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 ?
Do I understand correctly that the latter adds a
cc:header to outgoing mails ?
Yes, but not in the "mail headers", but in the "header of the body of the mail". Slight but important difference!
@cvinh take care what @sbidoul said here https://github.com/OCA/social/pull/855#issuecomment-1072378496 . The original module is from v8 https://github.com/OCA/social/tree/8.0/mail_footer_notified_partners
@sbidoul in some moment in v12 we had two modules which make really "the same". mail_notified_partner and mail_show_follower
- Both add in the "body" (Odoo changed several times how messaging works) who are the "followers" of the message which means which partners are notified.
- One show it in the top of the message ("body"header)
- The other show it in the botton ("body-footer")
Does everybody realize that this current PR of 2400 lines only adds 2 words over what you can already find in mail_show_follower? 😅
This is amazing, we should maintain the less lines of code for the same functionality if possible
@cvinh take care what @sbidoul said here #855 (comment) . The original module is from v8 https://github.com/OCA/social/tree/8.0/mail_footer_notified_partners
@sbidoul in some moment in v12 we had two modules which make really "the same".
mail_notified_partnerandmail_show_follower
- Both add in the "body" (Odoo changed several times how messaging works) who are the "followers" of the message which means which partners are notified.
- One show it in the top of the message ("body"header)
- The other show it in the botton ("body-footer")
Does everybody realize that this current PR of 2400 lines only adds 2 words over what you can already find in mail_show_follower? 😅
This is amazing, we should maintain the less lines of code for the same functionality if possible
Dear all Finally I had time to test mail_show_notifier. It includes the notifications in the email body and not in the email headers as @Yajo told us. @sbidoul I was afraid of double notifications and unwanted notifications but now I'm completely ok with that module. I'm happy to close this PR Thanks to all
Hello all,
After migrating and improving mail_show_follower in v14 and v15 we realize that we don't want to inform about follower but inform about the notified partner. In fact for me is a BUG and I reported internally, for example with internal notes.
So considering that ACSONE did (as @sbidoul said https://github.com/OCA/social/pull/855#issuecomment-1072378496) the module form v8 here:
- https://github.com/OCA/social/tree/8.0/mail_footer_notified_partners
And @SimoRubi migrated to v10 (renaming) here:
- https://github.com/OCA/social/tree/10.0/mail_footer_notified_partner
We would like to adopt, migrate, improve and maintain it for v14, 15. With a change of name
mail_notified_partner (as we would choose if we print them in footer or in header)
Sorry @cvinh you were right and this module is lighter with less lines of code.
We would make a PR to you branch if you Agree, thanks.
cc @Shide @moduon MT-579
@rafaelbn thank you for reporting this. We adopted mail_show_followers and did not see the bug you pointed out. We are going to confirm and maybe go back to mail_notified_partner
We adopted mail_show_followers and did not see the bug you pointed out
Try sending an internal note. It will tell you the followers, but that doesn't mean all of them were notified.
I think we could merge this one and then make improvements without the required review ov @cvinh (just for speed and avoid innecesary noise)
OK, but we should limit it by name to the footer
- [ ] Change name to
mail_notified_partner
@rafaelbn can you help fixing tests please ? I am not good with them Also I reopened the changing name PR https://github.com/OCA/social/pull/857 (I was renaming it in mail_body_notified_partner is it ok for you ?)
- I really would like to have the approval of @JonathanNEMRY and @sbidoul .
- About the name I think is better
We would like to adopt, migrate, improve and maintain it for v14, 15. With a change of name
mail_notified_partner(as we would choose if we print them in footer or in header) https://github.com/OCA/social/pull/855#issuecomment-1102348461
mail_notified_partner looks like a nicer product 😄
We checked how Basecamp, Notion or other software solution this matter 😄
- @yajo or @Shide will help with test.
@moduon MT-579
Dear all
I am editing my answer because I was testing an old version of mail_show_folIower... Sorry for the noise...
mail_show_follower does the job for us but I am confirming the behavior @yajo https://github.com/OCA/social/pull/855#issuecomment-1103523034 If one replies to the notification, we have an error of delivery internal note's reply
Codepoint U+202C at position 4 of 'com\u202c\u202c\u202c"' not allowed |
-- | --
<br class="Apple-interchange-newline">
BTW, mail_body_notified_partner shows all the followers of the object aswell and have the same error of delivery (which is maybe related to other oca modules that we have in our infrastructure like mail_tracking)
Regarding to that, I confirm that we are going back to mail_body_notified_partner which has less lines of code and seems easier to maintain as it does not have python code.
Best regards
Dear @cvinh , thank you. I confirm you the same. We will move to mail_footer_notified_partner (I wish mail_notified_partner 😄 ).
We will work together for this scope because we had the same issues as you described.