-
-
Notifications
You must be signed in to change notification settings - Fork 785
[17.0][OU-IMP]hr: Add computation of some fields #5463
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
Conversation
|
@MiquelRForgeFlow May I ask for your opinion? 🙏 |
|
Hehe An alternative should be adding migration scripts in |
|
The problem is that the compute is failing because it has a value that doesn't exist yet |
MiquelRForgeFlow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
pedrobaeza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern on this is the increase of time on the execution, or to have different data in past leaves. Why are you recomputing the duration? Remember that you may adjust manually the duration. I don't think it should be recomputed.
|
After a full review of the error I found that the problem comes from the fact that number_of_hours was not stored in v16, but stored in v17. If we don't apply something, the function is executed no matter what, and it might raise errors if we have other modules like natural_days. Options I see:
Leaving as it is will not work if you are using an OCA module and there is no natural way to solve it. |
11a91ed to
51eb514
Compare
|
I agree on pre-adding the field, but we shouldn't trigger the regular computation, as that one computes both |
|
legit for me. I will manage it |
51eb514 to
734c69c
Compare
|
@etobella do you plan to add anything more? |
|
@pedrobaeza no, I think I made all the changes that you asked for. |
openupgrade_scripts/scripts/hr_holidays/17.0.1.6/upgrade_analysis_work.txt
Show resolved
Hide resolved
| hr_holidays / hr.leave / number_of_days (float) : now a function | ||
| hr_holidays / hr.leave / number_of_hours (float) : NEW isfunction: function, stored | ||
| # NOTHING TO DO: computed fields | ||
| # Pre-migration script and end-migration script to recompute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Pre-migration script and end-migration script to recompute | |
| # DONE: pre-migration: add column for avoiding the computation |
734c69c to
cc25748
Compare
Otherwise, an error might be rise if we have hr_holidays_natural_period installed because the option might have not been selected yet.