-
-
Notifications
You must be signed in to change notification settings - Fork 667
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][IMP] hr_attendance_rfid: Add record rule #833
Conversation
1b9f9f4
to
847ee13
Compare
@HviorForgeFlow @JordiBForgeFlow |
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.
Looks great for me
@jarroyomorales ready to move forward |
This PR has the |
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.
Code Reviewed.
hr_attendance_rfid/data/ir_rule.xml
Outdated
@@ -0,0 +1,15 @@ | |||
<odoo> |
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.
<odoo> | |
<odoo noupdate="1"> |
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.
If we are adding the noupdate, we should need to add a migration script that will create the rule
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.
Really? even if that ir.model.data
record is not already set on the system? OUCH
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.
Or the forcecreate, but I would opt for the migration
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.
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.
I don't know about migration script.
if you add noupdate
in code, Will it cause problems?
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.
Something as hard as this:
https://github.com/OCA/community-data-files/blob/12.0/account_payment_unece/migrations/12.0.1.0.0/post-migration.py#L8 wink
I think this is the best option, better than the force_create
847ee13
to
c30afa2
Compare
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.
Nice! Thank you!
You also need to upgrade manually the manifest version to 12.0.1.0.2 and change the name of the migration folder to the same version (12.0.1.0.2). Then this will be merged with the nobump option
c30afa2
to
e791486
Compare
I was just getting ready to comment and ask if I should manually update the manifest or if the merge bot was going to be told what to do! Now I have updated and manually bumped the manifest to 12.0.2.0.0 from 12.0.1.0.0 |
@brian10048 Yes, now we have to agree on the version number. It is not a very huge change so maybe instead of 12.0.2.0.0 we could do 12.0.1.1.0. @Saran440 What do you think? |
I agree with @jarroyomorales |
e791486
to
e759ec5
Compare
@Saran440 @jarroyomorales - Version changes made as requested |
@OCA/human-resources-maintainers I think this is ready to merge 😉 |
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
Congratulations, your PR was merged at bab439d. Thanks a lot for contributing to OCA. ❤️ |
@HviorForgeFlow