[18.0][MIG] hr_timesheet_task_required: Migration to 18.0 #730
Conversation
Currently translated at 100.0% (9 of 9 strings) Translation: timesheet-14.0/timesheet-14.0-hr_timesheet_task_required Translate-URL: https://translation.odoo-community.org/projects/timesheet-14-0/timesheet-14-0-hr_timesheet_task_required/nl_NL/
Currently translated at 66.6% (6 of 9 strings) Translation: timesheet-14.0/timesheet-14.0-hr_timesheet_task_required Translate-URL: https://translation.odoo-community.org/projects/timesheet-14-0/timesheet-14-0-hr_timesheet_task_required/fr/
Currently translated at 100.0% (12 of 12 strings) Translation: timesheet-15.0/timesheet-15.0-hr_timesheet_task_required Translate-URL: https://translation.odoo-community.org/projects/timesheet-15-0/timesheet-15-0-hr_timesheet_task_required/fr/
Currently translated at 100.0% (10 of 10 strings) Translation: timesheet-16.0/timesheet-16.0-hr_timesheet_task_required Translate-URL: https://translation.odoo-community.org/projects/timesheet-16-0/timesheet-16-0-hr_timesheet_task_required/it/
Currently translated at 100.0% (10 of 10 strings) Translation: timesheet-16.0/timesheet-16.0-hr_timesheet_task_required Translate-URL: https://translation.odoo-community.org/projects/timesheet-16-0/timesheet-16-0-hr_timesheet_task_required/es/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: timesheet-17.0/timesheet-17.0-hr_timesheet_task_required Translate-URL: https://translation.odoo-community.org/projects/timesheet-17-0/timesheet-17-0-hr_timesheet_task_required/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: timesheet-17.0/timesheet-17.0-hr_timesheet_task_required Translate-URL: https://translation.odoo-community.org/projects/timesheet-17-0/timesheet-17-0-hr_timesheet_task_required/
There was a problem hiding this comment.
The module works as expected.
- Commit history is not squashed for commits like oca-ci, ...
- There should be 2 commits.
2.1. [IMP] hr_timesheet_task_required: pre-commit auto fixes
This commit should contain the yellow maked changes.
2.2. [MIG] hr_timesheet_task_required: Migration to 18.0
All the other changes should be in this commit. - The UI has some issues, see comment at the xml file
| <field name="arch" type="xml"> | ||
| <xpath expr="//block[@name='project_time']" position="inside"> | ||
| <setting | ||
| id="is_timesheet_task_required_setting" | ||
| help="Require task selection on each timesheet entry by default" | ||
| > | ||
| <field name="is_timesheet_task_required" /> | ||
| </setting> | ||
| </xpath> | ||
| </field> |
There was a problem hiding this comment.
I think the Settings UI is not correct:
in version 17 it looked like this:
| <field name="task_id" position="before"> | ||
| <field name="is_task_required" invisible="1" /> | ||
| </field> |
There was a problem hiding this comment.
I think this can be removed because of:
Missing fields used in domains and other attributes are added to the view as invisible automatically. If you encounter fields in views with invisible="True", they can probably be removed.
| <field name="task_id" position="before"> | ||
| <field name="is_task_required" column_invisible="1" /> | ||
| </field> |
There was a problem hiding this comment.
please check if this can be removed as well.
| <field name="allow_timesheets" position="after"> | ||
| <field | ||
| name="is_timesheet_task_required" | ||
| invisible="allow_timesheets == False" |
There was a problem hiding this comment.
change to
invisible="not allow_timesheets"
| <setting | ||
| id="timesheet_task_settings" | ||
| help="Set tasks on timesheet as a mandatory field" | ||
| invisible="allow_timesheets == False" |
| def _check_timesheet_task(self): | ||
| for line in self: | ||
| if line.is_task_required and not line.task_id: | ||
| raise ValidationError(_("You must specify a task for timesheet lines.")) |
There was a problem hiding this comment.
I think this should be changed?
You can replace the call to translate a string ( _ ) with self.env._ for getting some performance improvement in some cases.
| is_timesheet_task_required = fields.Boolean( | ||
| string="Require Tasks on Timesheets", | ||
| related="company_id.is_timesheet_task_required", | ||
| readonly=False, |
| _inherit = "account.analytic.line" | ||
|
|
||
| is_task_required = fields.Boolean( | ||
| string="Is Task Required", related="project_id.is_timesheet_task_required" |
There was a problem hiding this comment.
This is the default string value and can be removed?
HeliconiaSolutions
force-pushed
the
18.0-mig-hr_timesheet_task_required
branch
from
427cc3e to
14664ac
Compare
Hi @CRogos , Thank you for the review. As mentioned, I have cross-checked the migration of the same module in a fresh environment using the steps below: git checkout -b 18.0-mig-hr_timesheet_task_required origin/18.0 During these steps, I did not encounter any changes related to the pre-commit hooks, so the migration commit is straightforward. As a result, this PR does not include any pre-commit fixes. Please let me know if I am missing any steps or information. Your advice would be highly appreciated. |
|
Hi @CRogos , I have taken care of the other suggested changes and updated the PR accordingly. Whenever you have a chance, please review them as well. Thank you for your time and guidance! |
I had a quick look on other PR and you are right. I would have expected some of the index.html changes (like gray/grey) in this commit, but this isn't in the other PR as well. The commits here marked should be squashed. The rest LGTM, even though I am not sure how to trigger the Validation Error message? |
No description provided.