[17.0][MIG] hr_timesheet_sheet: Migration to 17.0

[SodexisTeam]

Superseed #629

[pedrobaeza]

/ocabot migration hr_timesheet_sheet

Please check CI.

[valentinthirion]

Hi there,
What is blocking this PR to be merged?
Thanks in advance,
Best regards

[valentinthirion]

Hello,
Can this PR be merged?
Thanks in advance

[vnikolayev1]

Hey @valentinthirion @Vijaiy-Selvaraj , did not see your MR, so made my own on porting to 17
There are two ways to fix your MR:

1. Please preserve commit history following technical method explained in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-17.0. and do updates that are written in the article
   History of git blame authors should be kept - do the git format-patch - it's hard to review when git history shows that you added all code.
   We should have three commits based on doumentation instructions:
   First commit should be pre-commit fixes
   Second commit should be porting
   Third commit should be your customization

2. you can pull my code, add your customizations with third commit over my code and i review it [MIG] hr_timesheet_sheet migration to 17.0 #703

I'd rather do second option, because i've done more things on porting/fixing tests due to documentation and commits are already existing, but it's up to you.

[vnikolayev1]

@valentinthirion @Vijaiy-Selvaraj @SodexisTeam are you still interested in review? :)

[SodexisTeam]

@vnikolayev1 We took care of the improvements. Please review it.

[vnikolayev1]

@SodexisTeam Overall porting to 17 looks good after we fix this _check_can_write method , please make migration as 3 commits like in this instruction
https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-17.0.
1st commit you move v16 to v17 with format-patch and do pre-commit (you do pre-commit, than git add everything back what was changed by pre commit than push)
2nd commit should be your port to v17 (run it through pre-commit and do git add)
3rd commit should be your improvements with _check_can_write and report (run it through pre-commit and do git add)

I'll make my MR to yours after that

[SodexisTeam]

@vnikolayev1 We took care of the changes. Please review it.

[vnikolayev1]

@SodexisTeam Look good, gave another review, Let's wrap it up:

1. Can you please give steps to reproduce _check_can_write bug so I'll check it?
2. Please squash my test fix and suggestions into migration commit (run pre-commit on it)
   Let's add our companies so they will be motivated to contribute to OCA

[SodexisTeam]

@vnikolayev1 Took care of the changes.

Here is the steps to reproduce _check_can_write bug:

1. Ensure the "project_timesheet_holidays" module is installed.
2. Comment out the "_check_can_write" method in the "account_analytic_line.py" file under the "hr_timesheet_sheet" module.
3. Create a time off request for an employee with a time off type that has "Generate Timesheets" enabled, and approve it.
4. When creating a timesheet for the same employee, if the time off date falls within the timesheet date range, you will encounter a UserError stating: "You cannot modify timesheets that are linked to time off requests. Please use the Time Off application to modify your time off requests instead."

[vnikolayev1]

Thank you, will check tomorrow, found another bug on name renderingб added to suggestions:
hr_timesheet_sheet/models/hr_timesheet_sheet.py line 231

[vnikolayev1]

Hmm, i see no errors after doing this.

1. Made sure project_timesheet_holidays is installed and commented out _check_can_write function in
2. Took paid off dayoff
   
   

and took it for one day

3. added timesheet in analytic account on the same date Menu > Timesheets > All timesheets

The only value that going through this method is {'amount': -100.0} , no sheet_id, that's why i thought that this method is kind of sketchy.
Also hasattr(self, "holiday_id") will always be true because we have project_timesheet_holidays module installed
P.S. I only have our modules installed, maybe something else in your project breaks this logic?

[vnikolayev1]

@SodexisTeam any thoughts?

[SodexisTeam]

@vnikolayev1

I have attached the error simulation video for your reference.
Please ensure that the instruction to comment out the _check_can_write method in the account_analytic_line.py file under the hr_timesheet_sheet module has been followed.

Hr_Timesheet_User_Error.webm

[pedrobaeza]

Please include #712

[SodexisTeam]

@pedrobaeza

Sorry for the inconvenience caused.
We ensured that the latest commits of V16 are included in the V17 migrations.
Thanks

[pedrobaeza]

I was talking about squash administrative commits

https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests

but OK, let's merge this.

Please check the procedure about this next time, and please do commits with an individual login.

/ocabot merge nobump

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 17.0-ocabot-merge-pr-687-by-pedrobaeza-bump-nobump, awaiting test results.

[OCA-git-bot]

Congratulations, your PR was merged at 48658a1. Thanks a lot for contributing to OCA. ❤️

[odoo-sh]

@pedrobaeza sorry for the trouble and thanks for the comments. I think we are going to follow your advice and start using individual account, it seemed easier when we started. Thanks.