-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Added deposit for rental products #500
base: 18.0
Are you sure you want to change the base?
Conversation
aca29c1
to
9e2db7e
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.
Hi @jmra-odoo,
Great work 👏
I've made a few suggestions for your consideration.
const $depositSpan = $('#product_deposit_amount'); | ||
if (!$depositSpan.length) return; |
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.
Please avoid using jQuery.
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.
Okay, replaced with JS.
deposit_product_id = int(self.env['ir.config_parameter'].get_param('deposit_product_id')) | ||
deposit_product = self.env['product.product'].browse(deposit_product_id) | ||
if deposit_product: | ||
deposit_line = self.order_id.order_line.filtered(lambda line: line.product_id == deposit_product and line.name.endswith(f"For {self.product_id.name}")) |
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.
Do not filter by name.
Will this code still work if there are multiple records in 'self'?
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.
Filtered by linked_line_id so now no need for complex or filter by name.
No it will not work with multiple records in self as i thought there is no possibility to create multiple records but yes there can be a chance to create multiple so updated with looped approach.
|
||
def set_values(self): | ||
super().set_values() | ||
self.env['ir.config_parameter'].sudo().set_param('deposit_product_id', self.deposit_product_id.id) |
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.
Why is sudo used?
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.
By using sudo(), it gets the access rights to superuser level, temporarily bypassing regular access rules, sometimes it is possible to not have the access right to admin of configuration settings. I have seen it in codebase too.
9e2db7e
to
d981eea
Compare
if not deposit_product: | ||
raise ValidationError("Deposit product is not set in the setting.") |
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.
You can avoid this error, as it unnecessarily interrupts the flow.
With This Commit ======================================================== - Added deposit product configuration in settings. - Inherited product.template to add require_deposit & deposit_amount field. - Linked deposit product automatically to rental product in sale order line. - Synced deposit product quantity with main product in cart. - Disabled remove button and quantity changer for deposit products in cart view.
d981eea
to
054ca6f
Compare
With This Commit