Conversation
etobella
force-pushed
the
17.0-share
branch
from
f5bb005 to
1959c23
Compare
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
bot
added
the
stale
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for this contribution, Enric -- the idea of readonly collaborators on shared projects is a solid addition. However, the module is missing tests entirely (codecov fails), and I have a concern about the post_init_hook record rule rewrite: the new domain restricts task visibility to edit_collaborator_ids, which would seem to exclude readonly collaborators from seeing tasks -- the opposite of what the description promises. Could you add tests covering both collaborator types and double-check the record rule logic?
| 'child_of', | ||
| [user.partner_id.commercial_partner_id.id]), | ||
| ('project_id.edit_collaborator_ids.partner_id', 'in', [user.partner_id.id]), | ||
| ] |
There was a problem hiding this comment.
This domain line restricts task visibility to partners present in edit_collaborator_ids (i.e. non-readonly collaborators). Readonly collaborators would therefore not match this rule and would not see any tasks. Shouldn't the condition use collaborator_ids (all collaborators) instead of edit_collaborator_ids here, so that readonly collaborators can at least read tasks?
| result = super()._check_project_sharing_access() | ||
| if ( | ||
| check_readonly | ||
| and isinstance(result, models.BaseModel) |
There was a problem hiding this comment.
Checking isinstance(result, models.BaseModel) and result._name == 'project.collaborator' is fragile -- the parent method's return contract could change. Consider handling unexpected return types more defensively.
|
|
||
| @api.model | ||
| def get_view(self, view_id=None, view_type="form", **options): | ||
| result = super().get_view(view_id=view_id, view_type=view_type, **options) |
There was a problem hiding this comment.
The three blocks for kanban/form/tree are nearly identical -- consider extracting a helper like _set_readonly_view(doc, tag) to reduce duplication.
| "data": [ | ||
| "views/project_collaborator.xml", | ||
| ], | ||
| "demo": [], |
There was a problem hiding this comment.
No tests directory in the module. Given that this module rewrites a security record rule and dynamically modifies views based on collaborator access level, test coverage is important. At minimum: a test for readonly collaborator seeing tasks (read) but not editing, and a test for edit collaborator retaining full access.
github-actions
bot
removed
the
stale
3 participants
This module adds a new functionality on collaborators, so we can have readonly collaborators that are able to see kanban and list views in the same way we have in internal view.