From 764a1c056f1fd6ae0aef12cd81b0260b5c61d56e Mon Sep 17 00:00:00 2001 From: Carmen Bianca BAKKER Date: Fri, 28 Jun 2024 11:30:27 +0200 Subject: [PATCH 1/4] [FIX] hr_timesheet_begin_end: Test uses timesheet lines now Timesheet lines are characterised by having a project. Signed-off-by: Carmen Bianca BAKKER --- .../readme/newsfragments/692.bugfix.1.rst | 1 + hr_timesheet_begin_end/tests/test_timesheet_begin_end.py | 9 ++++----- 2 files changed, 5 insertions(+), 5 deletions(-) create mode 100644 hr_timesheet_begin_end/readme/newsfragments/692.bugfix.1.rst diff --git a/hr_timesheet_begin_end/readme/newsfragments/692.bugfix.1.rst b/hr_timesheet_begin_end/readme/newsfragments/692.bugfix.1.rst new file mode 100644 index 0000000000..056c17a06b --- /dev/null +++ b/hr_timesheet_begin_end/readme/newsfragments/692.bugfix.1.rst @@ -0,0 +1 @@ +Fixed the test to use timesheet lines instead of bare analytic lines. diff --git a/hr_timesheet_begin_end/tests/test_timesheet_begin_end.py b/hr_timesheet_begin_end/tests/test_timesheet_begin_end.py index a70b9b73b5..85ee99d01a 100644 --- a/hr_timesheet_begin_end/tests/test_timesheet_begin_end.py +++ b/hr_timesheet_begin_end/tests/test_timesheet_begin_end.py @@ -9,17 +9,16 @@ class TestBeginEnd(common.TransactionCase): def setUp(self): super(TestBeginEnd, self).setUp() self.timesheet_line_model = self.env["account.analytic.line"] - self.analytic = self.env.ref("analytic.analytic_administratif") - self.user = self.env.ref("base.user_root") + self.project = self.env.ref("project.project_project_1") + self.employee = self.env.ref("hr.employee_qdp") self.base_line = { "name": "test", "date": fields.Date.today(), "time_start": 10.0, "time_stop": 12.0, - "user_id": self.user.id, "unit_amount": 2.0, - "account_id": self.analytic.id, - "amount": -60.0, + "project_id": self.project.id, + "employee_id": self.employee.id, } def test_onchange(self): From 0985b0d3de701186b6dcdd2f5d18b70cae5205a6 Mon Sep 17 00:00:00 2001 From: Carmen Bianca BAKKER Date: Mon, 17 Jun 2024 12:10:09 +0200 Subject: [PATCH 2/4] [REF] hr_timesheet_begin_end: Make more extensible The separation of checks into separate methods is needed because I want to disable one check in another module. This makes the module more extensible. The unit_amount_from_start_stop method also makes the module more extensible. I have also moved the onchange to the top of the file, according to the OCA contribution guidelines. Signed-off-by: Carmen Bianca BAKKER --- .../models/account_analytic_line.py | 75 ++++++++++++------- .../readme/newsfragments/692.feature.1.rst | 1 + 2 files changed, 50 insertions(+), 26 deletions(-) create mode 100644 hr_timesheet_begin_end/readme/newsfragments/692.feature.1.rst diff --git a/hr_timesheet_begin_end/models/account_analytic_line.py b/hr_timesheet_begin_end/models/account_analytic_line.py index e49c22ad41..40bbbeac0f 100644 --- a/hr_timesheet_begin_end/models/account_analytic_line.py +++ b/hr_timesheet_begin_end/models/account_analytic_line.py @@ -1,5 +1,6 @@ # Copyright 2015 Camptocamp SA - Guewen Baconnier # Copyright 2017 Tecnativa, S.L. - Luis M. Ontalba +# Copyright 2024 Coop IT Easy SC - Carmen Bianca BAKKER # License AGPL-3 - See http://www.gnu.org/licenses/agpl-3.0.html from datetime import timedelta @@ -15,16 +16,14 @@ class AccountAnalyticLine(models.Model): time_start = fields.Float(string="Begin Hour") time_stop = fields.Float(string="End Hour") - @api.constrains("time_start", "time_stop", "unit_amount") - def _check_time_start_stop(self): - for line in self: - value_to_html = self.env["ir.qweb.field.float_time"].value_to_html - start = timedelta(hours=line.time_start) - stop = timedelta(hours=line.time_stop) - if stop < start: - value_to_html(line.time_start, None) - value_to_html(line.time_stop, None) + @api.onchange("time_start", "time_stop", "project_id") + def onchange_hours_start_stop(self): + self.unit_amount = self.unit_amount_from_start_stop() + def _validate_start_before_stop(self): + value_to_html = self.env["ir.qweb.field.float_time"].value_to_html + for line in self: + if line.time_stop < line.time_start: raise exceptions.ValidationError( _( "The beginning hour (%(html_start)s) must " @@ -35,7 +34,11 @@ def _check_time_start_stop(self): "html_stop": value_to_html(line.time_stop, None), } ) - hours = (stop - start).seconds / 3600 + + def _validate_unit_amount_equal_to_time_diff(self): + value_to_html = self.env["ir.qweb.field.float_time"].value_to_html + for line in self: + hours = line.unit_amount_from_start_stop() rounding = self.env.ref("uom.product_uom_hour").rounding if hours and float_compare( hours, line.unit_amount, precision_rounding=rounding @@ -50,16 +53,21 @@ def _check_time_start_stop(self): "html_hours": value_to_html(hours, None), } ) - # check if lines overlap - others = self.search( - [ - ("id", "!=", line.id), - ("employee_id", "=", line.employee_id.id), - ("date", "=", line.date), - ("time_start", "<", line.time_stop), - ("time_stop", ">", line.time_start), - ] - ) + + def _overlap_domain(self): + self.ensure_one() + return [ + ("id", "!=", self.id), + ("employee_id", "=", self.employee_id.id), + ("date", "=", self.date), + ("time_start", "<", self.time_stop), + ("time_stop", ">", self.time_start), + ] + + def _validate_no_overlap(self): + value_to_html = self.env["ir.qweb.field.float_time"].value_to_html + for line in self: + others = self.search(line._overlap_domain()) if others: message = _("Lines can't overlap:\n") message += "\n".join( @@ -74,13 +82,28 @@ def _check_time_start_stop(self): ) raise exceptions.ValidationError(message) - @api.onchange("time_start", "time_stop") - def onchange_hours_start_stop(self): - start = timedelta(hours=self.time_start) - stop = timedelta(hours=self.time_stop) + @api.constrains("time_start", "time_stop", "unit_amount") + def _check_time_start_stop(self): + lines = self.filtered(lambda line: line.project_id) + lines._validate_start_before_stop() + lines._validate_unit_amount_equal_to_time_diff() + lines._validate_no_overlap() + + @api.model + def _hours_from_start_stop(self, time_start, time_stop): + start = timedelta(hours=time_start) + stop = timedelta(hours=time_stop) if stop < start: - return - self.unit_amount = (stop - start).seconds / 3600 + # Invalid case, but return something sensible. + return 0 + return (stop - start).seconds / 3600 + + def unit_amount_from_start_stop(self): + self.ensure_one() + # Don't handle non-timesheet lines. + if not self.project_id: + return 0 + return self._hours_from_start_stop(self.time_start, self.time_stop) def merge_timesheets(self): # pragma: no cover """This method is needed in case hr_timesheet_sheet is installed""" diff --git a/hr_timesheet_begin_end/readme/newsfragments/692.feature.1.rst b/hr_timesheet_begin_end/readme/newsfragments/692.feature.1.rst new file mode 100644 index 0000000000..366ce40d00 --- /dev/null +++ b/hr_timesheet_begin_end/readme/newsfragments/692.feature.1.rst @@ -0,0 +1 @@ +Refactored the module to be more extensible. From 55c67c254f68a64a42a395a4e4d2545a36406392 Mon Sep 17 00:00:00 2001 From: Carmen Bianca BAKKER Date: Mon, 17 Jun 2024 17:21:49 +0200 Subject: [PATCH 3/4] [REF] hr_timesheet_begin_end: Refactor unit_amount to be a compute field Instead of using onchange, which is less convenient in Odoo 16. Signed-off-by: Carmen Bianca BAKKER --- .../models/account_analytic_line.py | 26 +++++++++++-- .../readme/newsfragments/692.feature.2.rst | 1 + .../tests/test_timesheet_begin_end.py | 37 +++++++++++++++---- 3 files changed, 53 insertions(+), 11 deletions(-) create mode 100644 hr_timesheet_begin_end/readme/newsfragments/692.feature.2.rst diff --git a/hr_timesheet_begin_end/models/account_analytic_line.py b/hr_timesheet_begin_end/models/account_analytic_line.py index 40bbbeac0f..70a6e4dcf8 100644 --- a/hr_timesheet_begin_end/models/account_analytic_line.py +++ b/hr_timesheet_begin_end/models/account_analytic_line.py @@ -16,9 +16,29 @@ class AccountAnalyticLine(models.Model): time_start = fields.Float(string="Begin Hour") time_stop = fields.Float(string="End Hour") - @api.onchange("time_start", "time_stop", "project_id") - def onchange_hours_start_stop(self): - self.unit_amount = self.unit_amount_from_start_stop() + # Override to be a computed field. + unit_amount = fields.Float( + compute="_compute_unit_amount", + store=True, + readonly=False, + # This default is a workaround for a bizarre situation: if a line is + # created with a time range but WITHOUT defining unit_amount, then you + # would expect unit_amount to be computed from the range. But this never + # happens, and it is instead set to default value 0. Subsequently the + # constraint _validate_unit_amount_equal_to_time_diff kicks in and + # raises an exception. + # + # By setting the default to None, the computation is correctly + # triggered. If nothing is computed, None falls back to 0. + default=None, + ) + + @api.depends("time_start", "time_stop", "project_id") + def _compute_unit_amount(self): + # Do not compute/adjust the unit_amount of non-timesheets. + lines = self.filtered(lambda line: line.project_id) + for line in lines: + line.unit_amount = line.unit_amount_from_start_stop() def _validate_start_before_stop(self): value_to_html = self.env["ir.qweb.field.float_time"].value_to_html diff --git a/hr_timesheet_begin_end/readme/newsfragments/692.feature.2.rst b/hr_timesheet_begin_end/readme/newsfragments/692.feature.2.rst new file mode 100644 index 0000000000..185296c901 --- /dev/null +++ b/hr_timesheet_begin_end/readme/newsfragments/692.feature.2.rst @@ -0,0 +1 @@ +Changed ``unit_amount`` into a computed (stored, writeable) field. diff --git a/hr_timesheet_begin_end/tests/test_timesheet_begin_end.py b/hr_timesheet_begin_end/tests/test_timesheet_begin_end.py index 85ee99d01a..7992565aea 100644 --- a/hr_timesheet_begin_end/tests/test_timesheet_begin_end.py +++ b/hr_timesheet_begin_end/tests/test_timesheet_begin_end.py @@ -21,18 +21,39 @@ def setUp(self): "employee_id": self.employee.id, } - def test_onchange(self): - line = self.timesheet_line_model.new( - {"name": "test", "time_start": 10.0, "time_stop": 12.0} - ) - line.onchange_hours_start_stop() - self.assertEqual(line.unit_amount, 2) + def test_compute_unit_amount(self): + line = self.base_line.copy() + del line["unit_amount"] + line_record = self.timesheet_line_model.create(line) + self.assertEqual(line_record.unit_amount, 2) + line_record.time_stop = 14.0 + self.assertEqual(line_record.unit_amount, 4) + + def test_compute_unit_amount_no_compute_if_no_times(self): + line = self.base_line.copy() + del line["time_start"] + del line["time_stop"] + line_record = self.timesheet_line_model.create(line) + self.assertEqual(line_record.unit_amount, 2.0) + line_record.unit_amount = 3.0 + self.assertEqual(line_record.unit_amount, 3.0) + + def test_compute_unit_amount_to_zero(self): + line = self.base_line.copy() + del line["unit_amount"] + line_record = self.timesheet_line_model.create(line) + self.assertEqual(line_record.unit_amount, 2) + line_record.write({"time_start": 0, "time_stop": 0}) + self.assertEqual(line_record.unit_amount, 0) - def test_onchange_no_update(self): + def test_compute_unit_amount_to_zero_no_record(self): + # Cannot create/save this model because it breaks a constraint, so using + # .new(). line = self.timesheet_line_model.new( {"name": "test", "time_start": 13.0, "time_stop": 12.0} ) - line.onchange_hours_start_stop() + self.assertEqual(line.unit_amount, 0) + line.time_stop = 10.0 self.assertEqual(line.unit_amount, 0) def test_check_begin_before_end(self): From b601c05f1a4b6ec0408cdddc306090b44f11ff80 Mon Sep 17 00:00:00 2001 From: Carmen Bianca BAKKER Date: Fri, 28 Jun 2024 14:45:06 +0200 Subject: [PATCH 4/4] [FIX] Add hr_timesheet_begin_end to rebel modules This is a tricky one. _Technically_ there is nothing that makes hr_timesheet_begin_end incompatible with the other modules here. However, when the project_id on a timesheet line is changed, unit_amount is rerecomputed. This has some consequences. In the sale_timesheet tests, the project_id of a timesheet line is (sometimes?) recomputed. Subsequently, unit_amount is recomputed and set to 0 when it _shouldn't_ be. Under a normal flow this wouldn't be a problem; time_start and time_stop would be populated to correctly recompute unit_amount. But in the tests, they are not. One solution is to, in addition to not recomputing the unit_amount of non-timesheet lines, not recompute the unit_amount of timesheet lines whose time_start and time_stop are both set to 00:00. However, this means that resetting these values to 00:00 does not correctly set unit_amount back to 0, which seems erroneous to me. Marking this module as a rebel module seems like the best course of action to me, even though it would be preferable to test this in conjunction with the other modules. Signed-off-by: Carmen Bianca BAKKER --- .copier-answers.yml | 3 ++- .github/workflows/test.yml | 21 ++++++++++++--------- .gitignore | 9 +++++++++ 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/.copier-answers.yml b/.copier-answers.yml index fd762c0179..e3f412721a 100644 --- a/.copier-answers.yml +++ b/.copier-answers.yml @@ -1,5 +1,5 @@ # Do NOT update manually; changes here will be overwritten by Copier -_commit: v1.21.1 +_commit: '1.23' _src_path: gh:oca/oca-addons-repo-template ci: GitHub convert_readme_fragments_to_markdown: false @@ -17,6 +17,7 @@ org_name: Odoo Community Association (OCA) org_slug: OCA rebel_module_groups: - sale_timesheet_rounded +- hr_timesheet_begin_end repo_description: 'TODO: add repo description.' repo_name: timesheet repo_slug: timesheet diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3bad198c1f..9f621e11c4 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -37,18 +37,25 @@ jobs: include: - container: ghcr.io/oca/oca-ci/py3.10-odoo16.0:latest include: "sale_timesheet_rounded" - makepot: "true" name: test with Odoo - container: ghcr.io/oca/oca-ci/py3.10-ocb16.0:latest include: "sale_timesheet_rounded" name: test with OCB + makepot: "true" - container: ghcr.io/oca/oca-ci/py3.10-odoo16.0:latest - exclude: "sale_timesheet_rounded" + include: "hr_timesheet_begin_end" + name: test with Odoo + - container: ghcr.io/oca/oca-ci/py3.10-ocb16.0:latest + include: "hr_timesheet_begin_end" + name: test with OCB makepot: "true" + - container: ghcr.io/oca/oca-ci/py3.10-odoo16.0:latest + exclude: "sale_timesheet_rounded,hr_timesheet_begin_end" name: test with Odoo - container: ghcr.io/oca/oca-ci/py3.10-ocb16.0:latest - exclude: "sale_timesheet_rounded" + exclude: "sale_timesheet_rounded,hr_timesheet_begin_end" name: test with OCB + makepot: "true" services: postgres: image: postgres:12.0 @@ -79,9 +86,5 @@ jobs: with: token: ${{ secrets.CODECOV_TOKEN }} - name: Update .pot files - run: - oca_export_and_push_pot https://x-access-token:${{ secrets.GIT_PUSH_TOKEN - }}@github.com/${{ github.repository }} - if: - ${{ matrix.makepot == 'true' && github.event_name == 'push' && - github.repository_owner == 'OCA' }} + run: oca_export_and_push_pot https://x-access-token:${{ secrets.GIT_PUSH_TOKEN }}@github.com/${{ github.repository }} + if: ${{ matrix.makepot == 'true' && github.event_name == 'push' && github.repository_owner == 'OCA' }} diff --git a/.gitignore b/.gitignore index 0090721f5d..2b045db399 100644 --- a/.gitignore +++ b/.gitignore @@ -25,6 +25,15 @@ var/ *.egg *.eggs +# Debian packages +*.deb + +# Redhat packages +*.rpm + +# MacOS packages +*.dmg + # Installer logs pip-log.txt pip-delete-this-directory.txt