[16.0][FEAT]portal: new module to hide timesheet from portal - #1521
[16.0][FEAT]portal: new module to hide timesheet from portal#1521quirino95 wants to merge 1 commit into
Conversation
HekkiMelody
left a comment
There was a problem hiding this comment.
For adding a new module, the commit message should be
[ADD] project_portal_hide_timesheet: new module
(since the name of the module is already descriptive enough)
86eda96 to
e13b930
Compare
HekkiMelody
left a comment
There was a problem hiding this comment.
Minor formal changes but otherwise LGTM
| <?xml version="1.0" encoding="utf-8" ?> | ||
| <odoo> | ||
| <template | ||
| id="portal_home_timesheet" |
There was a problem hiding this comment.
chore:
Some formal requirements according to guidelines https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst
- File name should be <model_name>_views.xml (I think
_templates.xmlis also acceptable here since there are templates) - These two views should be split on two files since one is related to the
timesheet.sheetmodel and the other is related toproject.task - When inheriting, the
idof the new view should be the same as the original view (minus the module name), soid="portal_my_home_timesheet"andid="portal_tasks_list"
e13b930 to
e5135a4
Compare
HekkiMelody
left a comment
There was a problem hiding this comment.
Code and functional review, LGTM
|
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. |
|
@OCA/project-service-maintainers We've been using this successfully in production for a couple of months now. Could you please take a look? Thanks! |
|
Init hooks are a surprising way to modify views. It sounds dangerous. |
|
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. |
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for this contribution. The module approach is clean -- using post_init_hook/uninstall_hook to toggle existing portal views is a well-established OCA pattern, and the XML overrides are minimal and focused.
I reviewed the final commit (e5135a4) and the full review history. HekkiMelody's earlier feedback (missing sale_timesheet dep, loop-to-in operator, inactive record search, tests, XML split) has all been addressed. CI is green.
One item I'd flag:
Test coverage is vacuous (false positive)
In test_hooks.py, setUpClass fetches views with:
cls.views = cls.env["ir.ui.view"].search([("key", "in", views_to_switch)])By the time tests run, post_init_hook has already fired during module installation, so these views are active=False. Since search() applies active_test=True by default, cls.views is an empty recordset.
Both assertions then pass vacuously because all() on an empty iterable returns True:
self.assertTrue(all(not v.active for v in self.views)) # True (empty)
self.assertTrue(all(v.active for v in self.views)) # True (empty)To make the tests meaningful, the search in setUpClass should include inactive records:
cls.views = cls.env["ir.ui.view"].with_context(active_test=False).search(
[("key", "in", views_to_switch)]
)And assertions should also verify the recordset is non-empty, e.g.:
def test_post_init_hook(self):
post_init_hook(self.env.cr, self.env.registry)
self.assertTrue(self.views)
self.assertTrue(all(not v.active for v in self.views))Otherwise the hooks could be completely broken and tests would still pass.
Everything else looks good -- manifest, dependencies, license, XML structure, OCA scaffolding are all correct.
|
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. |
e5135a4 to
ff4c2b3
Compare
ff4c2b3 to
6a9e893
Compare
Thanks @dreispt for having a look! I tried using We could save one hook adding the Please let me know what you think!
@alexey-pelykh it was indeed, I fixed it, please check again. |
6a9e893 to
324158e
Compare
324158e to
07bbe00
Compare
This module hides details about timesheets and worked hours on tasks in the portal pages.