Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions mail_activity_board/models/mail_activity.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
# Copyright 2018 ForgeFlow S.L. <https://www.forgeflow.com>
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html).
from odoo import api, fields, models
from odoo.exceptions import AccessError


class MailActivity(models.Model):
Expand All @@ -26,10 +27,16 @@ class MailActivity(models.Model):
@api.depends("res_id", "res_model")
def _compute_related_model_instance(self):
for record in self:
ref = False
if record.res_id:
ref = f"{record.res_model},{record.res_id}"
record.related_model_instance = ref
if record.res_id and record.res_model:
try:
self.env[record.res_model].check_access_rights("read")
record.related_model_instance = (
f"{record.res_model},{record.res_id}"
)
Comment on lines +33 to +35

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

shouldn't it be enough to wrap this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's not enough, it would reintroduce the crash we are trying to fix.
The original crash didn't occur during the compute method, which didn't fail itself. The crash would simply happen a moment later: when the web client tries to render the view, it attempts to fetch the target record's display name.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

but when will the else branch of this conditional throw any of the errors you catch here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

you are right, I just moved the else branch to outside the try-catch, thanks!

except (ValueError, AccessError, KeyError):
record.related_model_instance = False
else:
record.related_model_instance = False

@api.model
def _selection_related_model_instance(self):
Expand Down
95 changes: 95 additions & 0 deletions mail_activity_board/tests/test_mail_activity_board.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,3 +214,98 @@ def test_related_model_instance(self):
self.assertEqual(self.act3.related_model_instance, self.partner_client)
self.act3.write({"res_id": False, "res_model": False})
self.assertFalse(self.act3.related_model_instance)

def test_related_model_instance_access_error(self):
"""Test that computing related_model_instance handles restricted access."""

# Ensure employee2 does not have read access to res.partner
has_access = (
self.env["ir.model.access"]
.with_user(self.employee2)
.check("res.partner", "read", raise_exception=False)
)
self.assertFalse(
has_access,
"Employee2 should not have read access to res.partner.",
)

# Create an in-memory record using .new()
# This bypasses the ir.rule that blocks reading the activity
activity = (
self.env["mail.activity"]
.with_user(self.employee2)
.new(
{
"res_model": "res.partner",
"res_id": self.partner_client.id,
}
)
)

# Trigger the compute method
activity._compute_related_model_instance()

# The access check forces it to fall back to False.
self.assertFalse(activity.related_model_instance)

def test_related_model_instance_invalid_model_valueerror(self):
"""Computing related_model_instance avoids ValueError on unallowed models."""
# Retrieve the allowed models list directly from the selection method
allowed_models = [
m[0] for m in self.env["mail.activity"]._selection_related_model_instance()
]

# Use model outside the allowed selection.
self.assertNotIn("calendar.event", allowed_models)

model_class = self.env.get("calendar.event")
self.assertIsNotNone(model_class)
self.assertFalse(model_class._abstract)
self.assertFalse(model_class._transient)

unallowed_record = model_class.search([], limit=1)
self.assertTrue(
unallowed_record,
"Test requires model to have at least one record.",
)

unallowed_model = "calendar.event"
unallowed_res_id = unallowed_record.id
self.assertTrue(
unallowed_res_id,
)

# Create an in-memory activity linked to the unallowed model
activity = self.env["mail.activity"].new(
{
"res_model": unallowed_model,
"res_id": unallowed_res_id,
}
)

# Trigger the compute method by reading the property.
result = activity.related_model_instance
self.assertFalse(
result,
"Field should compute to False for models not in the selection list.",
)

def test_related_model_instance_keyerror(self):
"""Computing related_model_instance handles non-existent models."""

# Create an in-memory activity linked to a fake model name
activity = self.env["mail.activity"].new(
{
"res_model": "some.completely.fake.model",
"res_id": 99,
}
)

# Trigger the compute method by reading the property
result = activity.related_model_instance

# Assert that the field defaults to False
self.assertFalse(
result,
"Field should compute to False for models that do not exist.",
)
Loading