[11.0][IMP] hr_timesheet_sheet: fix amount calculation issues + code simplification #207

astirpe · 2019-03-13T14:53:48Z

This PR fixes the amount calculation issues (see #206) regarding the decrease of the amount of hours from the summary tab.
The idea is that decreasing the amount from the summary tab should not directly decrease the amounts of time for existing projects and/or tasks, to avoid potential loose of relevant information. In case it's needed, it's up to the end user to manually adjust the amounts or merge lines in the details tab.
This is basically achieved by removing the def _diff_amount_timesheets() method.

The proposed solution also causes a code reduction (simplified code) including the following:

Removed count_timesheets field
Simplified code for def onchange_unit_amount()
Makes use of field sheet.timesheet_ids in place of self.env['account.analytic.line'].search()

This PR implicitly allows negative amounts: two lines are removed from def onchange_unit_amount(), the same way as it is done in #201.

This PR also fixes method def _default_employee() in a multicompany environment, filtering the default employee by company.

Fixes #198

Fixes #209

MiquelRForgeFlow

This PR implicitly includes a fix for the negative amounts. The solution is a bit alternative to #201.

That's not a fix for the negative amounts. It's just the removal of the non negative amounts condition as in #201. And then, why do you propose an alternative? What's wrong with the #201? Now, will reviewers need to choose between two PRs? I don't understand. I think you could propose here only the other changes (the ones solving your issue #206) instead of doing also commits that enter in conflict with #201. If you want changes, then request changes in my PR, but don't open alternative PRs regarding a same topic (negative amounts). Also, I could be happy to include the solution of your issue in #201, it's easy.

hr_timesheet_sheet/models/hr_timesheet_sheet.py

astirpe · 2019-03-13T16:07:31Z

@mreficent While simplifying that method, I just deleted the same 2 lines you already deleted in your PR. That was necessary because of the design of this PR, the reason is already explained above: this PR fixes the decreasing of the amounts, and decreasing by setting negative amounts is also part of the cases fixed by this PR. Nobody is complaining about your PR #201 which remains valid.

MiquelRForgeFlow · 2019-03-13T17:03:19Z

@astirpe ok, I will check

astirpe · 2019-03-14T13:02:08Z

Commit b5d1a7a should fix the issue mentioned in #207 (comment)

hr_timesheet_sheet/models/hr_timesheet_sheet.py

nikul-serpentcs

Minor change

hr_timesheet_sheet/__manifest__.py

hr_timesheet_sheet/models/hr_timesheet_sheet.py

MiquelRForgeFlow

Review completed. Good changes. 👍

But please, answer my question above

MiquelRForgeFlow · 2019-03-15T13:38:37Z

@alexey-pelykh could you review this?

astirpe · 2019-03-19T08:41:56Z

Commit [FIX] Allow to delete all lines + better tab sync contains a fix for issue #198. The fix is better than the one proposed in #203.

The proposed solution consists in splitting and fixing the onchange method of the date fields and the timesheet_ids.

As a consequence of fixing the onchange methods, another wrong behavior is fixed: when deleting lines in the Details tab, the Summary tab is automatically updated (it was not updated before this commit).

Supersedes #203.

hr_timesheet_sheet/models/hr_timesheet_sheet.py

astirpe · 2019-03-19T10:49:38Z

Commit 455d280 fixes the onchange of field employee_id (fixes #209).

The proposed fix restores the state 'New' for the timesheet sheet, like it was in standard Odoo 10.0 (see line).

Restoring the state 'New' is necessary to impede the onchange of date_start, date_end and employee_id being triggered when the status is "Draft". In facts performing the onchange when the status is "Draft" will cause the timesheet lines being deleted from database (this must be avoided!).

astirpe · 2019-03-19T13:38:07Z

Squashed and rebased

alexey-pelykh

Did the code review, yet no functional test yet

hr_timesheet_sheet/models/hr_employee.py

hr_timesheet_sheet/models/hr_timesheet_sheet.py

alexey-pelykh

Code LGTM, yet I'm already scared how this going to be ported to v12

alexey-pelykh · 2019-03-20T09:07:04Z

@astirpe please squash the code review commit as fixup into other commit

astirpe · 2019-03-20T09:37:14Z

@alexey-pelykh I can do the porting to V12, I think I'm now confident enough with this module.

alexey-pelykh · 2019-03-20T09:39:04Z

@astirpe That would be much appreciated!

[IMP] Fix amount calculation issues [IMP] Better warning message when onchange_unit_amount() fails [IMP] Performance [FIX] Allow to delete all lines + better tab sync [IMP] Coverage [REM] Remove useless lines of code [IMP] Code simplification [IMP] Code review

[FIX] Make employee_id readonly when timesheet sheet is approved [FIX] Onchange employee_id

astirpe · 2019-03-20T11:40:42Z

Porting to V12: #211

astirpe · 2019-03-21T08:43:51Z

I need also 412a801 to make customization easier

astirpe · 2019-03-21T08:53:43Z

And 4dc6093 makes the list view more readable.

astirpe · 2019-03-21T10:49:45Z

Commit b2c5b30 simplifies the code and I think also with performance benefits

hr_timesheet_sheet/models/hr_timesheet_sheet.py

astirpe · 2019-03-22T09:03:22Z

Commit 66a2455 fixes the following issue.

Be sure that admin user belongs to company: YourCompany.
Create CompanyB.
Create UserB, belonging to CompanyB.
Create EmployeeB, belonging to CompanyB and linked to UserB.
Login as admin and create a new timesheet sheet. Do not save yet.
Change employee to EmployeeB.

Company is not updated: it stays YourCompany (not ok).
It is not possible to change the Company because it's a readonly field.
Save. Error is displayed: "The Company in the Timesheet Sheet and in the Employee must be the same".

alexey-pelykh · 2019-03-22T11:35:26Z

@astirpe do you plan other changes in this PR?

astirpe · 2019-03-22T11:41:11Z

@alexey-pelykh No. I would be happy to stop it here. 😉

alexey-pelykh · 2019-03-22T12:12:54Z

@nikul-serpentcs please re-review

nikul-serpentcs

Only Code review LGTM 👍

OCA-git-bot · 2019-03-25T05:00:02Z

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

alexey-pelykh · 2019-03-25T05:53:03Z

@pedrobaeza please take a look at this PR

pedrobaeza · 2019-03-25T08:43:09Z

Merging blindly on my side due to previous approvals.

astirpe mentioned this pull request Mar 13, 2019

[11.0][FIX] hr_timesheet_sheet: deleting all lines #203

Closed

MiquelRForgeFlow suggested changes Mar 13, 2019

View reviewed changes

hr_timesheet_sheet/models/hr_timesheet_sheet.py Outdated Show resolved Hide resolved

MiquelRForgeFlow reviewed Mar 14, 2019

View reviewed changes

hr_timesheet_sheet/models/hr_timesheet_sheet.py Outdated Show resolved Hide resolved

nikul-serpentcs suggested changes Mar 15, 2019

View reviewed changes

hr_timesheet_sheet/__manifest__.py Show resolved Hide resolved

MiquelRForgeFlow reviewed Mar 15, 2019

View reviewed changes

hr_timesheet_sheet/models/hr_timesheet_sheet.py Outdated Show resolved Hide resolved

MiquelRForgeFlow approved these changes Mar 15, 2019

View reviewed changes

MiquelRForgeFlow mentioned this pull request Mar 18, 2019

[11.0][ADD] hr_timesheet_sheet_no_negative #201

Closed

1 task

MiquelRForgeFlow reviewed Mar 19, 2019

View reviewed changes

hr_timesheet_sheet/models/hr_timesheet_sheet.py Outdated Show resolved Hide resolved

hr_timesheet_sheet/models/hr_timesheet_sheet.py Show resolved Hide resolved

MiquelRForgeFlow approved these changes Mar 19, 2019

View reviewed changes

alexey-pelykh requested changes Mar 20, 2019

View reviewed changes

alexey-pelykh approved these changes Mar 20, 2019

View reviewed changes

astirpe added 2 commits March 20, 2019 10:54

[FIX] Fix employee_id related stuff

050ed4d

[FIX] Make employee_id readonly when timesheet sheet is approved [FIX] Onchange employee_id

astirpe mentioned this pull request Mar 20, 2019

[12.0][IMP] hr_timesheet_sheet: fix amount calculation issues + code simplification #211

Merged

[IMP] Ease customizations of message_subscribe_users

412a801

[IMP] List view easier to read

4dc6093

MiquelRForgeFlow reviewed Mar 21, 2019

View reviewed changes

hr_timesheet_sheet/models/hr_timesheet_sheet.py Outdated Show resolved Hide resolved

astirpe added 2 commits March 22, 2019 08:52

[IMP] Extra code simplification

4addcc3

[FIX] Update company when onchange employee_id

66a2455

astirpe mentioned this pull request Mar 22, 2019

[11.0][WIP] hr_timesheet_sheet: allow multicompany timesheet sheet #216

Closed

5 tasks

MiquelRForgeFlow approved these changes Mar 22, 2019

View reviewed changes

nikul-serpentcs approved these changes Mar 25, 2019

View reviewed changes

OCA-git-bot added approved ready to merge labels Mar 25, 2019

pedrobaeza merged commit bbace75 into OCA:11.0 Mar 25, 2019

astirpe deleted the 11_fix_hr_timesheet_sheet2 branch March 25, 2019 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[11.0][IMP] hr_timesheet_sheet: fix amount calculation issues + code simplification #207

[11.0][IMP] hr_timesheet_sheet: fix amount calculation issues + code simplification #207

astirpe commented Mar 13, 2019 •

edited

Loading

MiquelRForgeFlow left a comment

astirpe commented Mar 13, 2019

MiquelRForgeFlow commented Mar 13, 2019

astirpe commented Mar 14, 2019

nikul-serpentcs left a comment

MiquelRForgeFlow left a comment

MiquelRForgeFlow commented Mar 15, 2019

astirpe commented Mar 19, 2019 •

edited

Loading

astirpe commented Mar 19, 2019 •

edited

Loading

astirpe commented Mar 19, 2019

alexey-pelykh left a comment

alexey-pelykh left a comment

alexey-pelykh commented Mar 20, 2019

astirpe commented Mar 20, 2019

alexey-pelykh commented Mar 20, 2019

astirpe commented Mar 20, 2019

astirpe commented Mar 21, 2019

astirpe commented Mar 21, 2019

astirpe commented Mar 21, 2019

astirpe commented Mar 22, 2019

alexey-pelykh commented Mar 22, 2019

astirpe commented Mar 22, 2019

alexey-pelykh commented Mar 22, 2019

nikul-serpentcs left a comment

OCA-git-bot commented Mar 25, 2019

alexey-pelykh commented Mar 25, 2019

pedrobaeza commented Mar 25, 2019

[11.0][IMP] hr_timesheet_sheet: fix amount calculation issues + code simplification #207

[11.0][IMP] hr_timesheet_sheet: fix amount calculation issues + code simplification #207

Conversation

astirpe commented Mar 13, 2019 • edited Loading

MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

astirpe commented Mar 13, 2019

MiquelRForgeFlow commented Mar 13, 2019

astirpe commented Mar 14, 2019

nikul-serpentcs left a comment

Choose a reason for hiding this comment

MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

MiquelRForgeFlow commented Mar 15, 2019

astirpe commented Mar 19, 2019 • edited Loading

astirpe commented Mar 19, 2019 • edited Loading

astirpe commented Mar 19, 2019

alexey-pelykh left a comment

Choose a reason for hiding this comment

alexey-pelykh left a comment

Choose a reason for hiding this comment

alexey-pelykh commented Mar 20, 2019

astirpe commented Mar 20, 2019

alexey-pelykh commented Mar 20, 2019

astirpe commented Mar 20, 2019

astirpe commented Mar 21, 2019

astirpe commented Mar 21, 2019

astirpe commented Mar 21, 2019

astirpe commented Mar 22, 2019

alexey-pelykh commented Mar 22, 2019

astirpe commented Mar 22, 2019

alexey-pelykh commented Mar 22, 2019

nikul-serpentcs left a comment

Choose a reason for hiding this comment

OCA-git-bot commented Mar 25, 2019

alexey-pelykh commented Mar 25, 2019

pedrobaeza commented Mar 25, 2019

astirpe commented Mar 13, 2019 •

edited

Loading

astirpe commented Mar 19, 2019 •

edited

Loading

astirpe commented Mar 19, 2019 •

edited

Loading