odoo icon indicating copy to clipboard operation
odoo copied to clipboard

[IMP] {test_}mail, crm, website_slides: unfollow record from inbox and mail

Open pyduf opened this issue 2 years ago • 3 comments

Allow partner to unfollow a document from a follow up email or the inbox:

  • through an unsubscribe URL in the email even if not connected
  • through a link in the inbox

Details concerning the inbox implementation: When hovering on a follow up message with a related document followed by the user, the interface displays an action to unfollow it that allows the user to remove himself as follower of the document. As a result of performing that action a toast is displayed as a feedback.

Technical note: The unfollow link is displayed on a message when its property can_unfollow is true; that computed property is true when the message is a follow up and the partner still follows the record.

A message is a follow up if the notification associated is marked as a follow up which is determined when the message is sent and never change after. To know if the partner still follow the document, we have extended the mechanism that load partially the related thread of the message when loading initial messages by adding also current partner as follower if the partner follows the related document.

Necessary data being setup, the unfollow action can then use standard mechanism already developped to unfollow a document.

Nonetheless, to support unfollowing a document in the inbox no matter the current company, we have modified message_unsubscribe in mail_thread to allow internal user to unsubscribe themself without checking any rights. Indeed, some document have record rule that prevents reading it if the user is not in the right company (ex. crm.lead) and then was preventing the user to unfollow the document using that method.

Details concerning the mail implementation: It works for internal user for follow up on any document and for any partner on follow up of document tagged as authorizing being unfollowed by any partner ( slide.channel and slide.slide).

Technical note: When sending a notification by email, the recipients are splitted further depending on being follower of the related document or not. If they are followers a generic unfollow link is added in the mail.mail body_html. As a mail.mail can be used to send mail to multiple recipients the link is refined when sent to a specific recipient by setting the parameters (pids, token, model, res_id). It is also when sending the mail that we check that the user can have an unfollow link (user must be internal or the document is tagged as authorizing to be unfollowed by any partner).

Rather than splitting recipients further into different mail.mail, we could have added whether each recipients of a mail.mail is following the document or not but we didn't find proper way to do that (info on relation between mail.mail and partner ?).

An other possibility would have be to query follower when sending mail but that would defeat the strategy to do the work upstream to be able after to send the mail in batch easily.

Note also that we use special tags in the mail layout template (unfollow_placeholder_start, unfollow_placeholder_end) for 2 purposes:

  • to allow to remove that section for some recipient without rerendering the template and avoid extra queries.
  • to not include the unfollow link for partners that doesn't follow the document (it is implicit, the group of recipient not following the document are added in a mail.mail with body_html with that section removed)

Query count adjustment: Based on some test (not complete), the added query counts are due to the query of user_follower_id in message_format of mail.message. It is necessary to inform the web client which message is related to a document that the user follows in order to display or not the unfollow link and to be able to use the already developed method on the client side (which requires the mail.followers id).

Task-3061864


I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

pyduf avatar Dec 14 '22 15:12 pyduf

Pull request status dashboard

robodoo avatar Dec 14 '22 15:12 robodoo

@pyduf Hi :)

First batch of comments, good work overall :)

Not a big fan of this preprocess to add the unfollow url, but I don't see how we can do it differently :/

Thanks :)

Thanks for the PR. I followed most of your suggestions but left some question for some points.

pyduf avatar Jan 11 '23 15:01 pyduf

@pyduf Hi :)

Next batch of comments, thanks for the update / explanation on discord :)

Cheers

Thanks for the review and all your help. (waiting the runbot to see if there are still errors...)

pyduf avatar Jan 12 '23 15:01 pyduf

@pyduf Hi :)

I have a few more proposition, otherwise LGTM :)

Thanks for your work

Hello @std-odoo, thanks for your suggestions. I have updated the code with them and left some comments.

pyduf avatar Jan 16 '23 12:01 pyduf

Riviou/validation ! Interesting feature, but I fear the performance of that feature ... we already see this may add quite a lot of queries, and also as those queries probably come from mail / notification / follower tables, I really fear we add latency for this feature ...

Maybe to rethink / discuss one of those days, as the feature is great but we should probably find a less invasive way of implementing it. Don't have currently a lot of ideas, maybe we should start by checking those queries (in both mono and multi record cases) and check how we can reduce it ...

Anyway thanks for the PR, we will continue improving it !

Cheers !

Thanks for the review. I've already done the simple corrections and the change of the landing page (even if the user is connected, we display the simple "document unfollowed" page). As explained in the comments, I will try another version without the "follower_id" computed field on mail_notification as suggested. I haven't updated the query count but I will do with the new version.

pyduf avatar Feb 23 '23 12:02 pyduf

Hello @tde-banana-odoo,

Thanks for your review and useful advices. I have reimplemented the inbox version following the transition in owl and improved the code following your comments.

While reimplementing the inbox version, I have noticed that message_format was formatting message (sent through the bus) with the sender user while dispatching them to the users. So it is not possible to have partner specific information in the message throught that method. As this method is public, it’s not ideal to add all followers in the message because it’s private data. I have then added method to personalize that message but it’s add complexity… What do you think ? (more details in the commit).

pyduf avatar Apr 20 '23 14:04 pyduf

Zboing, some final comments for polishing :) (did not look at tests, will do it soon if I have some time :) )

Hello @tde-banana-odoo, thanks for the review. I have done the changes as suggested except for one for which I have left a explanation why. Note: I will do some more tests to check that the changes/rebases have no impact and let you know when it is ready.

pyduf avatar May 08 '23 13:05 pyduf

Hello, few comments as I'm passing by, but nothing too problematic!

Hello @seb-odoo, thank you for the review and advice. I have updated the code accordingly.

@tde-banana-odoo, as mentioned in the comment, to avoid computation on discuss.channel records, I have added a condition in the domain to filter them out. Is it what you had in mind ? Thanks.

pyduf avatar May 09 '23 08:05 pyduf

@robodoo override=ci/security

Reason:

  • using 'getattr' to fetch an optional class attribute. '_partner_unfollow_enabled' is used to activate the unfollow by email feature to all recipients (otherwise: internal only), which is opt-in as it generates more computation, and is used mainly for models that could lead to a spam behavior like forum or elearning due to automated emails sending (e.g. new publication)
  • using 'urllib' to analyse URLs in mail (used in tests)

tde-banana-odoo avatar May 09 '23 13:05 tde-banana-odoo

@robodoo r+ rebase-merge

tde-banana-odoo avatar May 10 '23 07:05 tde-banana-odoo

Merge method set to rebase and merge, using the PR as merge commit message.

robodoo avatar May 10 '23 07:05 robodoo