[CarlosRoca13]

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

[pedrobaeza]

/ocabot migration mail_tracking

[OCA-git-bot]

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]

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

[pedrobaeza]

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.

@sbidoul @simahawk do you know why this is not linked?

[rodrigoluizgenz]

Hello guys, can you check this ?
Tested on 20.0 localhost database with hr installed: test_access_tracking_email fails (AccessError not raised).
In 20.0, BaseModel.check_access()/has_access() rely only on _access_domain() + the access cache and no longer call _check_access(), so the override never runs on read() (searches are still filtered by _search).

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.

[CarlosRoca13]

I think it's all solved 😄

[cristina-hidalgo-tecnativa]

Suggested change

	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.

[CarlosRoca13]

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.

[cristina-hidalgo-tecnativa]

Suggested change

	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.

[CarlosRoca13]

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

[CarlosRoca13]

@cristina-hidalgo-tecnativa Can you check it again?

[cristina-hidalgo-tecnativa]

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.

[CarlosRoca13]

Solved! 😄

[cristina-hidalgo-tecnativa]

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?

[pedrobaeza]

Please include #298 and #297 before migration commit.

[CarlosRoca13]

Done @pedrobaeza

[rodrigoluizgenz]

Thanks for the migration! A question about the upgrade path for databases that already have the OCA mail_tracking installed.

The PR doesn't include a migrations/ folder or a module-rename step. If a 19.0 database arrives at 20.0 with mail_tracking still in ir_module_module, that row is taken over by the new core mail_tracking ("Discuss Tracking"). When the core module updates, ir.model.data._process_end unlinks every mail_tracking.* xmlid it didn't load, which includes all of the OCA metadata:

• mail.tracking.email / mail.tracking.event: the ir.model rows are removed, but the tables survive because _drop_table only acts on models in the registry. They are orphaned until mail_recipient_tracking is installed and picks them up again (same _name).
• Stored fields the OCA module adds to core models: ir.model.fields.unlink() → _drop_column drops the column, because those models are in the registry. That means res_partner.email_bounced, mail_message.email_to / email_cc / mail_tracking_needs_action and res_company.mail_tracking_show_aliases would be lost.

Renaming the module before the upgrade (e.g. openupgradelib.update_module_names(cr, [("mail_tracking", "mail_recipient_tracking")], merge_modules=False)) avoids this. Is the plan to handle it through an OpenUpgrade apriori.py renamed_modules entry? A note in the README (INSTALL/ROADMAP) for users migrating with the Odoo upgrade service would also help, since they'd need to run the rename themselves.

[CarlosRoca13]

Thanks for the migration! A question about the upgrade path for databases that already have the OCA mail_tracking installed.

The PR doesn't include a migrations/ folder or a module-rename step. If a 19.0 database arrives at 20.0 with mail_tracking still in ir_module_module, that row is taken over by the new core mail_tracking ("Discuss Tracking"). When the core module updates, ir.model.data._process_end unlinks every mail_tracking.* xmlid it didn't load, which includes all of the OCA metadata:

• mail.tracking.email / mail.tracking.event: the ir.model rows are removed, but the tables survive because _drop_table only acts on models in the registry. They are orphaned until mail_recipient_tracking is installed and picks them up again (same _name).
• Stored fields the OCA module adds to core models: ir.model.fields.unlink() → _drop_column drops the column, because those models are in the registry. That means res_partner.email_bounced, mail_message.email_to / email_cc / mail_tracking_needs_action and res_company.mail_tracking_show_aliases would be lost.

Renaming the module before the upgrade (e.g. openupgradelib.update_module_names(cr, [("mail_tracking", "mail_recipient_tracking")], merge_modules=False)) avoids this. Is the plan to handle it through an OpenUpgrade apriori.py renamed_modules entry? A note in the README (INSTALL/ROADMAP) for users migrating with the Odoo upgrade service would also help, since they'd need to run the rename themselves.

I'm waiting to set the rename to openupgrade when this PR has being merge OCA/OpenUpgrade#6045

[pedrobaeza]

@CarlosRoca13 OpenUpgrade initialization merged.