Skip to content

[20.0][REVIEW] mail_recipient_tracking: migration changes from OCA #310 - #2

Open
weinni2000 wants to merge 4 commits into
20.0-review-pr310-precommitfrom
20.0-review-pr310-migration
Open

weinni2000 wants to merge 4 commits into
20.0-review-pr310-precommitfrom
20.0-review-pr310-migration

Conversation

@weinni2000

Copy link
Copy Markdown
Owner

Comparison for OCA#310 after the final pre-commit/auto-fix preparation commit through the complete migration.

Base: 561e76939d3ef1d4878794471ca20cabce4fd6b0.
Head: 59363262f7d9597cb1719341a6f8a94ee7e5ee5a.

The replayed history already uses mail_recipient_tracking (renamed to avoid the new core mail_tracking). This comparison includes three preceding 19.0 fixes for SMTP exception handling, mail_id indexing and bypass_access, plus the final 20.0 migration. Review results follow here.

skanndar and others added 4 commits October 23, 2026 08:01
`MailMail._send()` tells a successful delivery from a failed one by the
exception raised by `send_email()`, not by its return value. `mail_tracking`
catches every exception and, when a `mail.tracking.email` record exists, records
the error on the tracking record and returns normally, so core believes the
mail was delivered: the recipient is marked as reached and its notification is
written `sent` for a mail that never left, core's `except AssertionError` never
runs so a NO_VALID_RECIPIENT is never classified, and the mail keeps the
placeholder failure reason set before the sending loop.

A tracking record is created for every outgoing email, so with `mail_tracking`
installed core's whole failure-handling path is disabled for the database.

This is not a duplicate of OCA#215/OCA#212, which added `else: raise`, i.e. it only
re-raises when there is NO tracking record -- the branch that is dead in
practice. The live case is the other one.

Re-raise unconditionally. The error is still recorded on the tracking record by
the `smtp_error()` call just above, so nothing is lost; only core's contract is
restored.

Note for adopters: delivery failures that used to be silent now surface as
`exception` states on `mail.mail`/`mail.notification`, so a backlog of
previously hidden failures may become visible at once on upgrade.

Fixes OCA#253
mail_id has an ON DELETE SET NULL foreign key to mail_mail, so every
deleted mail.mail scans the whole mail_tracking_email table. Odoo 19.0
deletes up to 10,000 cancelled mails per autovacuum run. With 7M
trackings that is about 1.5 s per mail without the index and 6.5 s for
the whole batch with it.

Use a partial index: mail_id is set to NULL every time its mail.mail is
deleted, so most rows hold NULL, and the foreign key trigger only runs
"WHERE mail_id = $1" with a non-NULL value. The index stays much
smaller, as core does for mailing.trace.mail_mail_id.
mail.message.mail_tracking_ids sets bypass_search_access, so the ORM
searches mail.tracking.email with bypass_access=True. _search ignored
it and filtered every matching tracking in Python. With 7M trackings,
get_failed_count went from 5-10 s to under 1 s.
Odoo 20.0 core provides a `mail_tracking` module (technical tracking of field values), so this addon is renamed from `mail_tracking` to `mail_recipient_tracking`.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry, we are unable to review this pull request

The GitHub API does not allow us to fetch diffs exceeding 20000 lines

@weinni2000

Copy link
Copy Markdown
Owner Author

Reviewed 561e76939d3ef1d4878794471ca20cabce4fd6b0..59363262f7d9597cb1719341a6f8a94ee7e5ee5a (130 changed paths; 69 PO files). The base is the last auto-fix preparation commit. Three requested 19.0 fixes follow it before the migration, so this comparison includes those fixes and their interactions with the final migration. The history already lives under the renamed module path.

[P2] Preserve the mail_id index added immediately before the migration.

mail_recipient_tracking/models/mail_tracking_email.py:47 removes index="btree_not_null" from mail_id. This option exists in parent e6a6196f3fa5d55c30cd6656a0854833281d66b6, added by replayed commit b363deea6761e974f6c2fb7c74dd3add76b7f88f ([IMP] mail_tracking: index mail_id), but the migration overwrites it. No replacement models.Index or index setup exists in this addon.

On a fresh 20.0 installation the tracking table therefore has no index for this column. Deleting mail.mail records must locate their tracking rows for the foreign key's ON DELETE SET NULL, so mail cleanup on a large tracking history again incurs full-table scans. The migration also adds Tracking.search([("mail_id", "in", self.ids)]) in models/mail_mail.py:74, another direct consumer. This defeats the performance fix explicitly replayed into this PR. The prior commit documents measurements on millions of rows; I did not repeat that benchmark locally.

Fix: retain index="btree_not_null" on mail_id (or an equivalent partial index), and verify its presence on a fresh installation. This finding is based on the exact field declarations at parent/head and the current query paths, not a lint preference. Guideline: Fields / index selective searched fields; Performance conventions.

Other review observations:

  • Earlier reported retry authorization and negative boolean-search concerns are addressed: retries filter by the current user's is_failed_message; compute depends on uid; the search negates the complete positive domain for normalized not in conditions. Tests cover these behaviors.
  • The former custom tracking search/read checks are replaced with _access_domain, matching Odoo 20's access model. New ACL data preserves read-only portal/public access to the user's partner trackings. The independent read/access test no longer requires HR; the older HR-specific test is still conditional, as the existing reviewer noted.
  • Failed-message UI is moved to the messaging menu. Earlier keyboard concerns for Retry/Reviewed pseudo-buttons are addressed with focusability and Enter/Space handlers. Native message/thread structures, imported module paths and template references were inspected against Odoo 20 checkout f7780f35f658f84d8d3abc1b556eff11ce92ba7c.
  • SMTP failures are re-raised after tracking the error; retry generation, Cc handling, store updates, alias cache invalidation, config getters, XML/view updates and module namespace changes were reviewed. The index finding is a new regression, separate from Cristina's resolved comments.
  • The module rename still needs the appropriate pre-upgrade database rename coordination. The upstream discussion assigns that to OpenUpgrade; fresh-install CI does not prove an existing OCA mail_tracking database can upgrade safely without that mapping. This review does not validate the external upgrade mapping.

Validation:

  • All applicable local configured hooks passed, including PO checks, OCA module checks, XML/Prettier, ESLint, Ruff and both pylint hooks.
  • Upstream pre-commit, Odoo, OCB, unreleased-dependency and coverage checks all succeeded at the reviewed head.
  • Original README passes strict Docutils (halt_level=2, report_level=2); git diff --check passes; checkout is clean.
  • All current static JS import targets resolve in the addon/Odoo 20 checkout. Existing translated message text is preserved across all 69 PO files (compared by msgid and translation values); namespace/reference changes and the updated POT were inspected.
  • No local Odoo database install, browser session, upgrade test or performance benchmark was run. Runtime test evidence is upstream CI. Static checks do not catch the missing database index.

Outcome: approval withheld until the index regression is fixed.

File mapping: Python -> Imports/Naming plus ORM/Fields/Methods; controllers -> Controllers/Security; ACL additions/deletions -> Access rights/Security; XML/data/demo/views -> Views/actions/data and inheritance anchors; JS/Owl/SCSS additions, changes and removals -> web JavaScript/SCSS/Assets; test changes -> Tests; manifest -> Manifest; PO/POT -> translation/namespace validation; README/readme/generated description and removed screenshot -> documentation/Assets. Guidelines read: Module structure and file naming; Manifest; Imports; Naming and model layout; Translate only static literals; Recordsets/domains/context; Computes/onchange/constraints; Methods and extension points; Transactions and exceptions; Fields; Controllers; Access rights; Batch ORM calls; Performance conventions; Views/actions/data records; Anchor view inheritance on names; Tests; Comments; Odoo security; web Organize files by feature/Avoid getters/Patching guidance; SCSS/CSS; Assets.

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.

3 participants