-
Notifications
You must be signed in to change notification settings - Fork 289
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
[REM] sale_margin_percentage: remove margin_percentage field in favor of native one I#25764 #1644
[REM] sale_margin_percentage: remove margin_percentage field in favor of native one I#25764 #1644
Conversation
c58e8ea
to
55cb2f8
Compare
@isaako34 and @rolandojduartem please review. |
5734928
to
09fc66a
Compare
09fc66a
to
043dc7f
Compare
class SaleOrder(models.Model): | ||
_inherit = "sale.order" | ||
|
||
margin_percentage = fields.Float( |
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 think we need a migration script here, it is a stored field.
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.
done
There errors in the CI, is that expected? |
043dc7f
to
544e25b
Compare
@luisg123v please review. |
from odoo import api, fields, models | ||
|
||
|
||
class SaleOrderLine(models.Model): |
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.
Could you split by model on a separate commit, please?
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.
Done
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.
Commit message: it's -> its
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.
done
544e25b
to
d52aa02
Compare
To follow our good practices each model should be in its own file with the same name as the model.
… of native one The field margin_percentage is no longer needed since native module sale_margin have the field margin_percent that serve the same propose. Also the field margin_threshold must be updated since it was used as a percentage but the native field margin_percent is a ration so we must follow the same pattern.
d52aa02
to
550a57c
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.
LGTM 👍
The field margin_percentage is no longer needed since native module
sale_margin have the field margin_percent that serve the same propose.
Also the field margin_threshold must be updated since it was used as a
percentage but the native field margin_percent is a ration so we must
follow the same pattern.