Repository navigation
[20.0][MIG] mail_tracking: Migration to 20.0 and rename to mail_recipient_tracking - #310
CarlosRoca13 wants to merge 118 commits into
Conversation
OCA-git-bot
added
series:20.0
mod:mail_recipient_tracking
264d75a to
c72225a
Compare
|
/ocabot migration mail_tracking |
|
There's no issue in this repo with the title 'Migration to version 20.0' and the milestone 20.0, so not possible to add the comment. |
pedrobaeza
changed the title
|
The renaming doesn't seem to be done for the whole commit history. Please check in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-20.0#technical-method-to-migrate-a-module-from-190-to-200-branch how to do it |
|
Hello guys, can you check this ? The rule probably needs to move to an _access_domain override or an ir.rule. CI doesn't catch it because the test is skipped without hr. |
c72225a to
f9be1c7
Compare
|
I think it's all solved 😄 |
| :return: number of emails sent again | ||
| """ | ||
| self.check_access("read") | ||
| messages = self.filtered("mail_tracking_needs_action") |
There was a problem hiding this comment.
| messages = self.filtered("mail_tracking_needs_action") | |
| messages = self.filtered("is_failed_message") |
action_retry_failed_mails only checks read access and then mail_tracking_needs_action. After that, _retry_failed_mails runs with sudo(). Any internal user who can read a message can call the method over RPC and resend its failed emails to the original recipients, even if they are neither the author nor a notified partner. The UI hides the Retry button from them, but the server does not enforce the same rule.
My suggestion is to filter by is_failed_message. It is computed for the current user and is true only when the user is the author or a notified partner, the message needs action and it has a failed tracking. The server now enforces the rule the UI already promises. For an uninvolved user the recordset is empty, so the call returns 0 and sends nothing.
There was a problem hiding this comment.
Good catch, thanks! Applied: action_retry_failed_mails now filters by is_failed_message. The server applies the same rule as the web client, so only the author or a notified partner can resend the failed emails.
| domain = Domain.AND( | ||
| [ | ||
| [ | ||
| ( | ||
| "mail_tracking_ids.state", | ||
| "in" if operand else "not in", | ||
| list(self.get_failed_states()), | ||
| ), | ||
| ("mail_tracking_needs_action", "=", True), | ||
| ], | ||
| is_involve, | ||
| ] | ||
| ) | ||
| return domain |
There was a problem hiding this comment.
| domain = Domain.AND( | |
| [ | |
| [ | |
| ( | |
| "mail_tracking_ids.state", | |
| "in" if operand else "not in", | |
| list(self.get_failed_states()), | |
| ), | |
| ("mail_tracking_needs_action", "=", True), | |
| ], | |
| is_involve, | |
| ] | |
| ) | |
| return domain | |
| domain = Domain.AND( | |
| [ | |
| [ | |
| ("mail_tracking_ids.state", "in", list(self.get_failed_states())), | |
| ("mail_tracking_needs_action", "=", True), | |
| ], | |
| is_involve, | |
| ] | |
| ) | |
| # The ORM normalizes "= v" / "!= v" into "in" / "not in" over a set of values | |
| wants_failed = operand if isinstance(operand, bool) else any(operand) | |
| return domain if (operator in ("in", "=")) == wants_failed else ~domain |
The ORM calls this search method with a normalized condition, never with a plain boolean: is_failed_message = False arrives as ('not in', {True}). The old code ignored operator and tested operand for truthiness. A non-empty set is always truthy, so it always built the positive domain. As a result, = True, = False, != True and != False all returned the failed messages, and the negative searches were wrong.
The fix always builds the positive domain (failed state, needs action, user involved) and negates the whole domain with ~ when the search asks for non-failed messages. This matches the computed field, which is true only when all three conditions hold together.
There was a problem hiding this comment.
Right, thanks! I checked it in _optimize_boolean_in: since 20.0 the search method always receives ("in" | "not in", [True]), so the operand was always truthy and negative searches returned the failed messages. Applied, slightly simplified since the value is always normalized.
return domain if operator == "in" else ~domain
f9be1c7 to
e65cd52
Compare
|
@cristina-hidalgo-tecnativa Can you check it again? |
cristina-hidalgo-tecnativa
left a comment
cristina-hidalgo-tecnativa
left a comment
There was a problem hiding this comment.
The Retry and Set as Reviewed actions of the messaging menu "Failed" tab are <span role="button"> elements with only a click handler. They have no tabindex, so keyboard users cannot focus them with Tab, and they do not react to Enter or Space. role="button" only changes what screen readers announce; it adds no behavior, so assistive technology announces a button that cannot be activated.
A native <button> cannot be used because the core mail.NotificationItem row is itself a <button>, and nested buttons are invalid HTML. So the pseudo-buttons are kept, with tabindex="0" and a keydown handler for Enter and Space that runs the same action as the click. preventDefault stops Space from scrolling the page, and stopPropagation keeps the key event from reaching the parent row.
e65cd52 to
2d756aa
Compare
|
Solved! 😄 |
cristina-hidalgo-tecnativa
left a comment
cristina-hidalgo-tecnativa
left a comment
There was a problem hiding this comment.
Code reviewed and tested locally and on runboat. LGTM!
Small doubt: wouldn't it be reasonable to add an else with self.skipTest(...) for when hr is not installed, so the skip is at least visible?
2d756aa to
5936326
Compare
|
Done @pedrobaeza |
|
Thanks for the migration! A question about the upgrade path for databases that already have the OCA The PR doesn't include a
Renaming the module before the upgrade (e.g. |
I'm waiting to set the rename to openupgrade when this PR has being merge OCA/OpenUpgrade#6045 |
|
@CarlosRoca13 OpenUpgrade initialization merged. |
This is dead code since the Migration to 18.0 (974b492), when the `messaging_service_patch.esm.js` was removed and replaced by a new `mail_core_common_service_patch.esm.js`
…thod The controller that called this method was removed during the Migration to 16.0, OCA/social@ef73e2d This method is a left-over, it's not used anywhere since.
There's already a collapsable panel for failed messages in the chatter. Having this other panel is totally redundant, and it was a bit glitchy too.
Before this commit, when the user clicked on the icon of a message, the following error occurred: ValueError: not enough values to unpack (expected 4, got 0) Complementary to 86e188c
…s reviewed Before these changes, when the bus event was received in tabs that did not have the message open, an error appeared because the system was not able to find the message. After these changes, if the message is not available on the screen, the process continues because synchronization will not be necessary, and therefore the error will no longer occur.
Create a dedicated company alias domain during the test setup to avoid temporary alias domains becoming referenced by aliases created by other installed modules. TT63167
Add optional mail headers expected by inherited `mail.thread` implementations, avoiding failures when modules such as `mass_mailing` are installed. TT63167
Add the optional mail headers to the second payload as well to keep the test compatible with inherited mail.thread implementations such as mass_mailing. TT63167
Currently translated at 100.0% (126 of 126 strings) Translation: mail-19.0/mail-19.0-mail_tracking Translate-URL: https://translation.odoo-community.org/projects/mail-19-0/mail-19-0-mail_tracking/sl/
`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.
res.users._init_messaging calls mail.message.get_failed_count, so every web client load (/mail/data) searches is_failed_message. That search filters on mail_tracking_needs_action, which has no index. Only a handful of messages carry the flag, but PostgreSQL starts from the failed trackings and probes mail_message by primary key once per tracking. With 122M messages and 29k failed trackings the query took 152 s cold and 5 s warm. Add a partial index on the flagged messages. It is tiny (584 kB on that database), and a write only touches it when the flag changes. The index is built once, by the first update after this change. Later updates keep it, because Odoo only rebuilds an index whose definition differs from the one stored in its comment. On a very large table that build blocks writes (13 minutes on 122M messages), so INSTALL.md explains how to create it beforehand with CREATE INDEX CONCURRENTLY and the comment the ORM stores on it.
…tracking 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`.
5936326 to
6053f32
Compare
|
The following commit authors need to sign the Contributor License Agreement:
|
|
Hi @SimoneVagile @fernandahf @trisdoan @bitroniq @nmadhoc @AreebSiddiqiIR @dhx @EgorSummer @ernestotejeda @jjscarafia @NiChrDeuse @JorgeQuinteros @bouvyd @angelJuaGar @payen000, Your earlier contributions to
Could you please sign at https://clabot.odoo-community.org/? If you have already signed, please ensure the commit email above is covered by your individual or company CLA. Thanks for helping unblock this migration! |
|
hi @weinni2000
by any chance, do you know if Odoo S.A. signed this agreement? i'm no longer an employee but i was at the time of my contributions, that would simplify things it's a bit strange to ask for the CLA now (10 years after my contribution), the fact that it prevents you from moving forward is a bit worrying 😅 (hopefully none of the other are MIA or deceased). |
|
Hi @bouvyd!
No Odoo S.A. never signed a CLA and probably never will.
Let's say we are cleaning up the stables :) So it's a bit of a rough period while we get the data straight. |
|
@sbidoul all good then, CLA signed 👍 |
carlos-lopez-tecnativa
left a comment
carlos-lopez-tecnativa
left a comment
There was a problem hiding this comment.
Just a few minor changes.
There was a problem hiding this comment.
I think this file should be dropped.
| option) where /web/databse/selector returns a list of more than one | ||
| database, then you need to add `mail_recipient_tracking` addon to wide load addons | ||
| list (by default, only `web` addon), setting `--load` option. For | ||
| example, `--load=web,mail,mail_tracking` |
There was a problem hiding this comment.
The documentation is outdated. It should be updated with the new module name.
There was a problem hiding this comment.
@carlos-lopez-tecnativa can you please handle latest comments and finish the work in absence of Charly?
cc @Tecnativa TT65067
This PR migrates
mail_tracking, which needs to be renamed because Odoo now uses the same name.ping @carlos-lopez-tecnativa @victoralmau