[16.0[IMP]account_commission: add payment date limit on settle commissions#588
Conversation
|
Hi @pedrobaeza, |
matteotognini
force-pushed
the
16.0-imp-account_commission
branch
from
ba276da to
50f6a32
Compare
matteotognini
changed the title
| """ | ||
| self.ensure_one() | ||
| payment_based_commission = ( | ||
| True if self.commission_id.invoice_state == "paid" else False |
There was a problem hiding this comment.
nitpick: this expression could be simplified
| True if self.commission_id.invoice_state == "paid" else False | |
| self.commission_id.invoice_state == "paid" |
matteotognini
force-pushed
the
16.0-imp-account_commission
branch
from
50f6a32 to
8a9338b
Compare
| context_date_payment = self.env.context.copy() | ||
| context_date_payment["date_payment_to"] = self.date_payment_to | ||
| self.env.context = context_date_payment |
There was a problem hiding this comment.
question: aren't these operations equivalent to
self.env.context["date_payment_to"] = self.date_payment_to?
Or am I missing something?
There was a problem hiding this comment.
Hi @PicchiSeba, self.env.context is a frozendict, which is an immutable dictionary. Because of this, I couldn't add self.date_payment_to directly as you suggested.
There was a problem hiding this comment.
Please use with_context decorator instead of a hard replacement of this type, as it's not guaranteed that the internal implementation will work with this kind of operations.
|
@pedrobaeza What do you think? is it possible to merge? |
| context_date_payment = self.env.context.copy() | ||
| context_date_payment["date_payment_to"] = self.date_payment_to | ||
| self.env.context = context_date_payment |
There was a problem hiding this comment.
Please use with_context decorator instead of a hard replacement of this type, as it's not guaranteed that the internal implementation will work with this kind of operations.
matteotognini
force-pushed
the
16.0-imp-account_commission
branch
from
4ad98ba to
d5c0a62
Compare
Shouldn't we prefer this approach instead of |
|
No, |
|
|
||
| def action_settle(self): | ||
| self = self.with_context(date_payment_to=self.date_payment_to) | ||
| return super().action_settle() |
There was a problem hiding this comment.
Is this covered by a test?
My doubt is that self gets reassigned locally. I'm not sure that super() w/o arguments uses local self. So I'm not sure it works.
I'd suggest the explicit syntax:
return super(CommissionMakeSettle, self).action_settle()
to make sure that it's the modified self that gets used.
There was a problem hiding this comment.
Hi @TheMule71, thanks for your review. I've just added a coverage test and it seems to work properly as it is now. Do you think it's better to make the syntax more explicit, as you mentioned? Thanks!
matteotognini
force-pushed
the
16.0-imp-account_commission
branch
from
d5c0a62 to
3a9c534
Compare
|
@pedrobaeza merge? |
|
This is adding a bit of complication on the module for a specific case, but it's not too much overhead and it's tested, so let's continue. /ocabot merge minor |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at de7708e. Thanks a lot for contributing to OCA. ❤️ |
6 participants
Porting #441