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

[18.0][MIG] mail_tracking: Migration to 18.0 #1

Merged
merged 106 commits into from
Dec 6, 2024

Conversation

trisdoan
Copy link
Contributor

@trisdoan trisdoan commented Nov 5, 2024

Note

Big changes in 18.0

Backend

  • access_rights_uid was removed from _search() changed in odoo/odoo@21d023e
  • _prepare_outgoing_list got extra param mail_server in odoo/odoo@97ce084
  • _message_format was converted to use store and message_format_extras was removed in odoo/odoo@52df49e. As consequences, _extras_to_store is introduced
  • tools.ustr is considered to be inefficient and flagged as deprecated in odoo/odoo@64a5724. Hence, is replaced by exception_to_unicode for the same effect.

Frontend

This changes

  • Store helper was introduced in odoo/odoo@c032ccf. As consequences, failed data is now passed through Store.

    • As a side effect, it is also easier to do the search filter_failed(thread) with Store service
  • As Odoo uses less Jquery, the module runs fine with vanilla. This impacts mail_tracking/static/src/components/message_tracking/message_tracking.esm.js

  • Took this chance to

    • replace % format with f-string

    • Improve permission constraint

      • Context: This commit porting from 12.0
      • Approach is summarized as below:
        1. When do the searching,
          - First, use the query to get result
          - Browse those id with the query + Remove any forbidden records
          - Return the accessible records in form of query, to continue the process
        2. When user read the records
          - The low-level method _check_access will discard forbidden records + raise exception
      • Added security check test
    • Resolve FIXME, which was added in OCA/social@7560443, then was resolved in odoo/odoo@7fe02c9

Result

discuss_sidebar

failed_search_panel

failed_message_review

antespi and others added 30 commits November 5, 2024 15:54
* [ADD] mail_tracking addon

* Add description icon

* Fixes remarked

* Fix Travis error

* Remarks fixed
…ent events (#82)

[IMP] mail_tracking: Speed installation time, discard concurrent events and other fixes
* Improve tests
* Show trackings even if partner removed
* Disable CSRF protection to webhooks controllers
As regular users can't access this object.
* [FIX] Only one data-odoo-tracking-email tag in each email
* [FIX] Get status even in multicompany instances
On our server,
queries based on "mail_tracking_event"."tracking_email_id" improved from 501,924 ms to 1,840 ms
queries based on "mail_tracking_email"."mail_message_id" improved from 167,436 ms to 3,223 ms

The last ones are run several times when a thread has many messages
Currently translated at 100.0% (82 of 82 strings)

Translation: social-11.0/social-11.0-mail_tracking
Translate-URL: https://translation.odoo-community.org/projects/social-11-0/social-11-0-mail_tracking/fr/
For giving more priority to other buttons like the invoices one.
Currently translated at 96.3% (79 of 82 strings)

Translation: social-11.0/social-11.0-mail_tracking
Translate-URL: https://translation.odoo-community.org/projects/social-11-0/social-11-0-mail_tracking/ca/
Or infinite recursions will happen on other `write` overwrites, like the one that happens
on `mass_mailing_partner`.
Comment on lines 78 to 80
tracking_ids = self.env["mail.tracking.email"]._search(
[("state", f"{tracking_operator}", failed_states)]
)
Copy link
Member

Choose a reason for hiding this comment

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

This seems very costly for a production database with 10 or 100 thousand of record in mail.tracking.email, it will return a lot of records.
I like better the previous implementation below,
( "mail_tracking_ids.state", "in" if value else "not in", list(self.get_failed_states()), ),

Choose a reason for hiding this comment

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

Indeed, I agree

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.

As for comment OCA/social#1476 (comment)

Look like it gets the same issue in 17.0

No, it doesn't. You can try in a runboat. If don't remember it wrong there was a bus call in python that notified the state to the client so we didn't need to reload the whole page risking to loose context like the breadcrumb path...

Copy link
Member

@mmequignon mmequignon left a comment

Choose a reason for hiding this comment

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

I agree with @TDu comment about the double search, it should be done in 1.

@trisdoan trisdoan force-pushed the 18.0-mig-mail_tracking branch from b9359cc to 3899c6c Compare November 11, 2024 08:39
@trisdoan trisdoan marked this pull request as draft November 11, 2024 08:40
@trisdoan
Copy link
Contributor Author

trisdoan commented Nov 11, 2024

DRAFT: working on reactivity of reviewed failed msg

@trisdoan trisdoan force-pushed the 18.0-mig-mail_tracking branch 2 times, most recently from cc947a6 to c37318b Compare November 12, 2024 04:16
@trisdoan trisdoan force-pushed the 18.0-mig-mail_tracking branch from c37318b to 974b492 Compare November 12, 2024 04:19
@trisdoan trisdoan marked this pull request as ready for review November 12, 2024 04:21
@trisdoan
Copy link
Contributor Author

Hello @TDu, @ivantodorovich , @mmequignon, I updated the _search_is_failed_message

@trisdoan
Copy link
Contributor Author

trisdoan commented Nov 12, 2024

Hello @chienandalu,

I just added this patch + use bus to fix the reactivity both in Discuss and Chatter, here is result:

mail_tracking_v18.webm

Yes, you'r right, it works in v17 but only in Chatter, it does not work in Discuss (I ran in gevent mode), is that the same on your local ?

mail_tracking_v17.webm

Copy link
Member

@mmequignon mmequignon left a comment

Choose a reason for hiding this comment

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

fine by me

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@hbrunn
Copy link
Member

hbrunn commented Dec 6, 2024

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 18.0-ocabot-merge-pr-1-by-hbrunn-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Dec 6, 2024
Signed-off-by hbrunn
@OCA-git-bot
Copy link
Contributor

It looks like something changed on 18.0 in the meantime.
Let me try again (no action is required from you).
Prepared branch 18.0-ocabot-merge-pr-1-by-hbrunn-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit bb6fa1f into OCA:18.0 Dec 6, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 4d83b95. Thanks a lot for contributing to OCA. ❤️

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.