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 #1476

Closed
wants to merge 106 commits into from

Conversation

trisdoan
Copy link
Contributor

@trisdoan trisdoan commented Oct 17, 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 7560443, then was resolved in odoo/odoo@7fe02c9

Result

discuss_sidebar

failed_search_panel

failed_message_review

@trisdoan trisdoan mentioned this pull request Oct 17, 2024
26 tasks
@trisdoan trisdoan marked this pull request as draft October 17, 2024 05:09
@trisdoan
Copy link
Contributor Author

trisdoan commented Oct 17, 2024

DRAFT: working on failed test

> 2024-10-17 05:08:43,628 246 WARNING odoo odoo.addons.mail_tracking.controllers.main: MailTracking email '11' not found

@trisdoan trisdoan marked this pull request as ready for review October 17, 2024 05:24
@trisdoan trisdoan force-pushed the 18.0-mig-mail_tracking branch from df5d6c7 to 794a168 Compare October 17, 2024 05:24
@pedrobaeza
Copy link
Member

/ocabot migration mail_tracking

@OCA-git-bot OCA-git-bot added this to the 18.0 milestone Oct 17, 2024
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.

Huge work @trisdoan !

I should try with a real mail configuration. For the moment:

Installing the module in runboat doesn't load for me the demo failed messages for retry (I saw in them in your screenshot thou...)

image

@@ -29,7 +30,7 @@ class MailMessage(models.Model):
)
is_failed_message = fields.Boolean(
compute="_compute_is_failed_message",
search="_search_is_failed_message",
store=True,
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to make it stored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, this flag is used to populate the controller: /mail/failed/messages

Copy link
Member

Choose a reason for hiding this comment

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

Please don't change how it was, no stored and with search method. Or does any reason for changing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @pedrobaeza, yes I agree there is no need to store it in db. But look like the search method is not working.

I am working on it, will update when it's done

Copy link
Contributor Author

@trisdoan trisdoan Oct 23, 2024

Choose a reason for hiding this comment

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

Hi @pedrobaeza @chienandalu , I just added fixup! [MIG] mail_tracking: Migration to 18.0 (will squash after we all agree)

  • As I inspected, _search_is_failed_message was not working. Without this commit, the condition in final query is always False => return no result
  • I added a test to make sure it's covered

@trisdoan trisdoan marked this pull request as draft October 21, 2024 04:29
@trisdoan
Copy link
Contributor Author

trisdoan commented Oct 21, 2024

DRAFT: inspect _search_is_failed_message

@pedrobaeza
Copy link
Member

What do you think about #1458 (comment) ?

@trisdoan trisdoan force-pushed the 18.0-mig-mail_tracking branch 2 times, most recently from 34f41c7 to c9c296f Compare October 23, 2024 02:29
NiChrDeuse and others added 17 commits October 23, 2024 09:31
Currently translated at 82.4% (103 of 125 strings)

Translation: social-16.0/social-16.0-mail_tracking
Translate-URL: https://translation.odoo-community.org/projects/social-16-0/social-16-0-mail_tracking/pt_BR/
Currently translated at 29.8% (34 of 114 strings)

Translation: social-15.0/social-15.0-mail_tracking
Translate-URL: https://translation.odoo-community.org/projects/social-15-0/social-15-0-mail_tracking/it/
Currently translated at 100.0% (125 of 125 strings)

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

Translation: social-17.0/social-17.0-mail_tracking
Translate-URL: https://translation.odoo-community.org/projects/social-17-0/social-17-0-mail_tracking/
Currently translated at 100.0% (128 of 128 strings)

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

Translation: social-17.0/social-17.0-mail_tracking
Translate-URL: https://translation.odoo-community.org/projects/social-17-0/social-17-0-mail_tracking/
Add autovacuum to mail_tracking_email that removes old records based on new configuration variable mail_tracking_email_max_age_days.
Due to possibly a large number of records to be deleted on first run, set a default limit of 5000 per run.
@trisdoan trisdoan force-pushed the 18.0-mig-mail_tracking branch from c9c296f to 8bcec37 Compare October 23, 2024 02:36
@trisdoan trisdoan force-pushed the 18.0-mig-mail_tracking branch from 8bcec37 to a6bce24 Compare October 23, 2024 02:47
@trisdoan trisdoan marked this pull request as ready for review October 23, 2024 02:48
Copy link
Contributor

@ivantodorovich ivantodorovich left a comment

Choose a reason for hiding this comment

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

Code review. LG. Thanks!

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.

Looks very good overall. In the testing side, I'm missing reactivity for the failed message actions. An example, where the failed message card should go away (it only does when we reload the window):

Peek 25-10-2024 10-49

Also, can you squash your migration commits?

@trisdoan trisdoan force-pushed the 18.0-mig-mail_tracking branch from a6bce24 to e992629 Compare October 28, 2024 07:53
@trisdoan
Copy link
Contributor Author

trisdoan commented Oct 28, 2024

Looks very good overall. In the testing side, I'm missing reactivity for the failed message actions. An example, where the failed message card should go away (it only does when we reload the window):

Hi @chienandalu , yes you're right. It causes confusion indeed.

My approach is to reload the page: browser.location.reload();
What do you think?

Look like it gets the same issue in 17.0

@pedrobaeza
Copy link
Member

Now that OCA/mail has been created (OCA/repo-maintainer-conf#52), can you please move this PR to that repo?

@trisdoan trisdoan closed this Nov 5, 2024
@trisdoan
Copy link
Contributor Author

trisdoan commented Nov 5, 2024

Superseded by OCA/mail#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.