Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[12.0][MIG] hr_timesheet_sheet #138

Merged
merged 23 commits into from
Nov 28, 2018

Conversation

alexey-pelykh
Copy link
Contributor

No description provided.

MiquelRForgeFlow and others added 22 commits November 9, 2018 18:24
* [10.0] hr_timesheet_sheet

* [11.0][MIG] hr_timesheet_sheet

* [REMOVE] hr_timesheet.sheet.account

* [REMOVE] 'new' state

* [ADD] Tests

* [UPD] Adapt to multicompany

* [ADD] Add more tests (include multicompany tests)

* [FIX] project_task_stage_allow_timesheet: show error message only if task

* [ADD] Migration scripts to v11
Currently translated at 98.9% (88 of 89 strings)

Translation: hr-timesheet-11.0/hr-timesheet-11.0-hr_timesheet_sheet
Translate-URL: https://translation.odoo-community.org/projects/hr-timesheet-11-0/hr-timesheet-11-0-hr_timesheet_sheet/ja/
Currently translated at 100,0% (89 of 89 strings)

Translation: hr-timesheet-11.0/hr-timesheet-11.0-hr_timesheet_sheet
Translate-URL: https://translation.odoo-community.org/projects/hr-timesheet-11-0/hr-timesheet-11-0-hr_timesheet_sheet/pt_BR/
Currently translated at 100.0% (89 of 89 strings)

Translation: hr-timesheet-11.0/hr-timesheet-11.0-hr_timesheet_sheet
Translate-URL: https://translation.odoo-community.org/projects/hr-timesheet-11-0/hr-timesheet-11-0-hr_timesheet_sheet/pt_BR/
…ay into this module, which adds a configuration to select the week start day.
@alexey-pelykh alexey-pelykh force-pushed the 12.0-mig-hr_timesheet_sheet branch from 12fee67 to 4ba2fcc Compare November 9, 2018 16:38
@pedrobaeza pedrobaeza added this to the 12.0 milestone Nov 9, 2018
@OCA-git-bot OCA-git-bot mentioned this pull request Nov 9, 2018
9 tasks
Copy link
Member

@nikul-serpentcs nikul-serpentcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor Change

@JordiBForgeFlow
Copy link
Member

@alexey-pelykh why do you remove the migration scripts? They are clearly enclosed under a version number, so they are not harmful. If you migrate from v10 to v12 the scripts will be used.

@alexey-pelykh
Copy link
Contributor Author

@jbeficent only because of "Remove any possible migration script from previous version." in https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-12.0

@alexey-pelykh alexey-pelykh force-pushed the 12.0-mig-hr_timesheet_sheet branch from 4ba2fcc to 9d64767 Compare November 12, 2018 06:58
@JordiBForgeFlow
Copy link
Member

@alexey-pelykh oh well it does not make sense to hold a script for an older version if the corresponing module code might not be compatible. So I agree with the rule :)

@pedrobaeza
Copy link
Member

Jordi, they should be removed because they can interact the bad way when migrating a DB, and as OpenUpgrade requires to go version per version migrating, you don't get any advantage keeping them, only possible drawbacks.

@alexey-pelykh
Copy link
Contributor Author

thanks for clarification! btw, this PR depends on OCA/web#1101

@alexey-pelykh alexey-pelykh force-pushed the 12.0-mig-hr_timesheet_sheet branch 12 times, most recently from f16f56a to afc62bd Compare November 21, 2018 08:59
@alexey-pelykh
Copy link
Contributor Author

Summoning good people of 11.0-related PRs that are aware of the matter: @jbeficent @lreficent @mreficent @oihane @astirpe - I'd like to request a review, if you don't mind

@alexey-pelykh alexey-pelykh force-pushed the 12.0-mig-hr_timesheet_sheet branch from afc62bd to dae372e Compare November 21, 2018 09:09
@alexey-pelykh
Copy link
Contributor Author

One question to whoever may posses this knowledge:
sheet_id and sheet_id_computed - why they are separate? (@mreficent)

@MiquelRForgeFlow
Copy link
Contributor

That is what odoo had in v10 and I just maintained that.

Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alexey-pelykh
Copy link
Contributor Author

I do think that @Icallhimtest (as original author of that line) would be able to bring in some clarity, yet that's not a blocker as of now

Copy link
Member

@nikul-serpentcs nikul-serpentcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review LGTM 👍

@OCA-git-bot
Copy link
Contributor

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 alexey-pelykh force-pushed the 12.0-mig-hr_timesheet_sheet branch from dae372e to ba1b8f6 Compare November 27, 2018 04:54
@alexey-pelykh
Copy link
Contributor Author

@pedrobaeza please merge when you'll have a spare minute

@pedrobaeza pedrobaeza merged commit a5f9488 into OCA:12.0 Nov 28, 2018
@alexey-pelykh alexey-pelykh deleted the 12.0-mig-hr_timesheet_sheet branch November 28, 2018 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants