[18.0][ADD] hr_timesheet_sheet_prefill#871
Conversation
0c8e624 to
2e33c4e
Compare
| aal = self.aal_model.with_user(self.user).create(vals) | ||
| return aal |
There was a problem hiding this comment.
| aal = self.aal_model.with_user(self.user).create(vals) | |
| return aal | |
| self.aal_model.with_user(self.user).create(vals) |
| prev_start = fields.Date.today() - relativedelta(weeks=2) | ||
| prev_end = prev_start + timedelta(days=6) | ||
| prev_sheet = self._create_sheet(prev_start, prev_end) | ||
| self._add_timesheet_line(prev_sheet, self.project_1, self.task_1) | ||
| self._add_timesheet_line(prev_sheet, self.project_2, self.task_2) | ||
| prev_sheet.action_timesheet_confirm() | ||
| self.assertEqual(prev_sheet.state, "confirm") |
There was a problem hiding this comment.
We can add a helper method for these steps.
| } | ||
| self.assertEqual(len(after), 2) | ||
| self.assertIn((self.project_1.id, self.task_1.id), after) | ||
| self.assertIn((self.project_2.id, self.task_2.id), after) |
There was a problem hiding this comment.
Please assert that unit_amount == 0 as well.
| f.employee_id = self.employee | ||
| f.date_start = date_start | ||
| f.date_end = date_end | ||
| return f.save() |
There was a problem hiding this comment.
I suppose the form is already saved at this point?
| return f.save() | |
| return f.record |
2e33c4e to
627ad8d
Compare
yostashiro
left a comment
There was a problem hiding this comment.
Code looks good. :)
627ad8d to
6987671
Compare
AungKoKoLin1997
left a comment
There was a problem hiding this comment.
Code and Functional Review: LGTM
nobuQuartile
left a comment
There was a problem hiding this comment.
Functional review: LGTM
・Prefill created empty lines for all project/task combos from the latest confirmed sheet.
・Existing combos were skipped (no duplicates).
・Errors raised when no previous sheet and nothing to create.
|
This PR has the |
6987671 to
0c8ac75
Compare
| #. odoo-python | ||
| #: code:addons/hr_timesheet_sheet_prefill/models/hr_timesheet_sheet.py:0 | ||
| msgid "No previous confirmed or approved timesheet sheet found." | ||
| msgstr "前期間の確認済みまたは承認済みのタイムシートが見つかりませんでした。" |
There was a problem hiding this comment.
| msgstr "前期間の確認済みまたは承認済みのタイムシートが見つかりませんでした。" | |
| msgstr "前期間の確認済みまたは承認済みのタイムシートレポートが見つかりませんでした。" |
| #. odoo-python | ||
| #: code:addons/hr_timesheet_sheet_prefill/models/hr_timesheet_sheet.py:0 | ||
| msgid "You can only prefill a draft timesheet sheet." | ||
| msgstr "下書き状態のタイムシートのみ仮入力できます。" |
There was a problem hiding this comment.
| msgstr "下書き状態のタイムシートのみ仮入力できます。" | |
| msgstr "ドラフトのタイムシートレポートのみ仮入力できます。" |
| #. module: hr_timesheet_sheet_prefill | ||
| #: model_terms:ir.ui.view,arch_db:hr_timesheet_sheet_prefill.hr_timesheet_sheet_form | ||
| msgid "Prefill from Previous Period" | ||
| msgstr "前期間の内容を仮入力" |
There was a problem hiding this comment.
| msgstr "前期間の内容を仮入力" | |
| msgstr "前期間から明細行を作成" |
|
@nobuQuartile Please follow up on the translation suggestions |
5dbaebb to
c861fb9
Compare
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for this addition -- the prefill pattern is a real time-saver for recurring timesheet setups, and the implementation reuses the parent module's helpers (_prepare_empty_analytic_line, _sheet_create) cleanly.
A few observations from going through the code:
Model (hr_timesheet_sheet.py)
The core logic is sound: state guard, previous sheet lookup, set-based dedup, and delegating line creation to the existing _sheet_create/_prepare_empty_analytic_line pipeline. One thing I'd flag for consideration:
- The
add_line()method in the parent module follows up_sheet_createwithself.write({"timesheet_ids": [(4, new_line.id)]}). Here that's skipped, which works because_prepare_empty_analytic_linealready setssheet_idon the line andtimesheet_idsis an inverse One2many. So functionally correct -- but if any downstream module hooks into writes ontimesheet_idsthey'd be bypassed. Not blocking, but worth being aware of.
Tests
Good coverage of the happy path and error cases. One missing scenario:
- There's no test for the state guard (
state not in ('new', 'draft')). Consider adding a test that confirms the sheet, then assertsUserErroris raised when callingaction_prefill_from_previouson a non-draft sheet.
i18n
- The
.potfile (i18n/hr_timesheet_sheet_prefill.pot) is missing. Other modules in this repo all ship one. It should be generated viaoca-gen-addon-readme/ the standard export.
Overall this is a clean, focused module. The missing .pot and the state-guard test are the main items I'd suggest addressing.
| ) | ||
| for project, task in lines_to_create: | ||
| vals = self._prepare_empty_analytic_line(project=project, task=task) | ||
| self.env["account.analytic.line"]._sheet_create(vals) |
There was a problem hiding this comment.
Nit: the parent add_line() also calls self.write({"timesheet_ids": [(4, new_line.id)]}) after _sheet_create. Here it's technically unnecessary since sheet_id is set in the vals, but if any module hooks into writes on timesheet_ids (e.g. recomputation triggers), those would be missed. Not blocking, just flagging for consistency.
| ) | ||
| self._add_timesheet_line(cur_sheet, self.project_1, self.task_1) | ||
| with self.assertRaises(UserError): | ||
| cur_sheet.action_prefill_from_previous() |
There was a problem hiding this comment.
Missing a test for the state guard -- e.g. confirm the sheet, then assert UserError on action_prefill_from_previous().
|
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. |
c861fb9 to
09ec141
Compare
|
@alexey-pelykh I updated the PR with your suggestions except from |
09ec141 to
46d1b18
Compare
|
CI is failing because of odoo issue. |
46d1b18 to
4704f03
Compare
4704f03 to
5af297f
Compare
This module adds a "Prefill from Previous Period" button to the timesheet sheet form.
When clicked, it copies the project/task combinations from the employee's most recent
confirmed or approved timesheet sheet, creating empty placeholder lines for the current
period.
Depends on #868.
@qrtl QT6381