Conversation
40d0383 to
6802f96
Compare
luisDIXMIT
left a comment
There was a problem hiding this comment.
Code reviewed and tested on Runboat, LGTM!
As an improvement, sub-managers could be filtered out to avoid displaying the main manager. It looks strange to me that a person can be both manager and sub-manager at the same time, although it might make sense.
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The module adds the user_ids field but the stated purpose — assigning specific access rules for sub-managers — isn't implemented yet. Without record rules referencing user_ids, the field doesn't actually restrict anything.
Also, CI tests are failing on both OCB and Odoo, and there are no tests for the module. Adding at least a basic test and the access rules would make this much more useful.
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Here is a detailed review of project_sub_manager.
Summary
The idea of assigning sub-managers to projects is useful, but the module needs several improvements before it can be merged.
Critical Issues
1. Missing explicit relation table name on Many2many field
The user_ids field does not specify a relation parameter. Odoo will auto-generate a relation table name, but if any other module also adds a user_ids Many2many to project.project, there will be a database-level conflict. OCA modules should always specify explicit relation table names for Many2many fields.
user_ids = fields.Many2many(
comodel_name="res.users",
relation="project_project_sub_manager_rel",
column1="project_id",
column2="user_id",
string="Project Sub Managers",
)2. No tests
The module has zero test coverage. OCA requires at least basic tests. Please add tests that:
- Verify the field exists and can be written to
- Verify the view changes load correctly
- If record rules are added (see point 3), test those as well
3. Module does not deliver on its stated purpose
The PR description says: "This field can be used afterwards to define specific record rules (for example, only sub_managers can create tasks in a project)"
As-is, the module only adds a field and views. Without record rules or any access control logic, the field is purely informational. Consider either:
- Adding the record rules that demonstrate the field's purpose, or
- Adjusting the description to clarify this is just a data field for other modules to consume
4. CI is failing
Both test with OCB and test with Odoo checks are failing. These need to pass before the PR can be merged.
Moderate Issues
5. Field name user_ids is too generic
The name user_ids is very generic and does not convey the field's purpose. A more descriptive name like sub_manager_ids or sub_manager_user_ids would be clearer and reduce collision risk with other modules.
6. Questionable default value
default=lambda self: self.env.user automatically adds the creating user as a sub-manager. Since the creating user is typically already set as the project manager via user_id, this creates redundancy and could be confusing. Consider removing the default.
7. Missing installable and summary in manifest
OCA convention is to explicitly set "installable": True in the manifest. Additionally, a "summary" key should be provided for the module listing.
8. Search filter label
The group-by filter has string="user" which should be more descriptive, e.g. string="Sub Manager".
Minor Issues
9. CONTRIBUTORS.md formatting
There is an extra > in CONTRIBUTORS.md:
- Cyril VINH-TUNG \<cyril@invitu.com>\>
Should be:
- Cyril VINH-TUNG <cyril@invitu.com>
The same issue appears in README.rst.
Please address the critical and moderate issues. Happy to re-review once updated!
Review posted via CorporateHub OCA review campaign
c8e0f49 to
486676b
Compare
486676b to
c700658
Compare
alexey-pelykh
left a comment
There was a problem hiding this comment.
CI is green and the explicit relation table name was added — that was the main technical concern. Clean and minimal module.
Co-Reviewed-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
|
This PR has the |
This module allows to select sub managers for projects
This field can be used afterwards to define specific record rules (for example, only sub_managers can create tasks in a project)