[MIG][11.0] donation_direct_debit #53
Conversation
tbaden
changed the title
|
@pedrobaeza is there a reason why the runbot status is skipped? |
|
Travis was not correct. Fixed in 161e5c2 Please rebase |
|
Hey @tbaden, thank you for your Pull Request. It looks like some users haven't signed our Contributor License Agreement, yet.
Appreciation of efforts, |
|
cla bot can be ignored, I somehow killed my gitconfig |
|
@pedrobaeza thanks for the support, all green |
tbaden
changed the title
| self.partner_id.customer_payment_mode_id. | ||
| payment_method_id.mandate_required | ||
| ): | ||
| mandates = self.env['account.banking.mandate'].search([ |
There was a problem hiding this comment.
I guess that taking the first record during an onchange of another field is to allow the user to select one.
In this case an order clause (for example by create_date desc) can make the result more predictable.
Also the search domains used in these use cases normally collide with restrictions on the field during the user selection. Are you we sure that mandate_id field doesn't need a domain restriction ?
I mean, when changing the partner, the first valid Mandate for that partner is selected, under certain conditions. After that the user can change it, until the state done is reached. In that moment can the user select a non-valid Mandante ? Or one of the other partner ?
If the answer is now, then it is better to move the search terms to a domain clause in the mandate_id field.
| ('partner_id', '=', self.commercial_partner_id.id), | ||
| ]) | ||
| if mandates: | ||
| self.mandate_id = mandates[0] |
There was a problem hiding this comment.
With a limit=1 clause in the previous search, explicit code for to get the first Record in the RecordSet is not needed.
| ('payment_mode_id', '=', donation.payment_mode_id.id), | ||
| ]) | ||
| msg = False | ||
| if payorders: |
There was a problem hiding this comment.
The same thing here... No need to take the first payorder, the search can have a limit=1 clause.
| an existing draft Direct Debit pay order''' | ||
| res = super(DonationDonation, self).validate() | ||
| apoo = self.env['account.payment.order'] | ||
| for donation in self: |
There was a problem hiding this comment.
Be aware that the super method works over a RecordSet and interrupt the validation process when it finds an invalid donation. That mean that the others (valids or not) would not be processed in this code.
What about to define a protected method in the donation.donation model for extension, for example _after_validate(..), and call it inside the loop of its validate()?
That would only be needed if the method should process all the valid donations and not all at once. If ALL the donations needs to be valid at once, then this way is ok.
| return res | ||
|
|
||
| def done2cancel(self): | ||
| for donation in self: |
There was a problem hiding this comment.
This code:
for donation in self:
if donation.move_id:
donation_mv_line_ids = [
l.id for l in donation.move_id.line_ids]can be optimized to:
for donation in self.filtered('move_id'):
donation_mv_line_ids = donation.mapped('move_id.line_ids').ids| "which is linked to a payment line in a " | ||
| "direct debit order. Remove it from the " | ||
| "following direct debit order: %s.") | ||
| % plines[0].order_id.name) |
There was a problem hiding this comment.
What about if there are more plines ? Those doesn't need to be removed ? If it is sure that no more plines would result from the query, then maybe just limit=1 is needed in the query.
Using limit=1 when needed also helps with performance. :-)
If there can be more than one, then an order clause would make it more predictable.
|
@yucer thanks for these suggestions. |
|
@tbaden your call: you decide if you want to make that improvements or only the migration here. |
|
@yucer I would like to make to migration first, so that people can use the module and afterwards create a PR for your improvements and maybe additional. I'll add you in the new PR if you like. |
|
Sorry, I didn't note it was a migration. |
|
@yucer does this migration get your approvement? |
Yes. I suppose for the migration is OK. And the coverage is good. |
|
This PR has the |
|
ARG, this migration didn't keep commit history!!! |
The migration is made from v10 because I figured out that commit history was lost during the v11 migration, cf OCA#53 Improve chatter message to add a link to the payment order Don't give rights on account.payment.order or account.payment.line to donation user because rights on payment orders should be very restricted. Use sudo() to create the payment order & line in the code.
The migration is made from v10 because I figured out that commit history was lost during the v11 migration, cf OCA#53 Improve chatter message to add a link to the payment order Don't give rights on account.payment.order or account.payment.line to donation user because rights on payment orders should be very restricted. Use sudo() to create the payment order & line in the code.
The migration is made from v10 because I figured out that commit history was lost during the v11 migration, cf OCA#53 Improve chatter message to add a link to the payment order Don't give rights on account.payment.order or account.payment.line to donation user because rights on payment orders should be very restricted. Use sudo() to create the payment order & line in the code.
The migration is made from v10 because I figured out that commit history was lost during the v11 migration, cf OCA#53 Improve chatter message to add a link to the payment order Don't give rights on account.payment.order or account.payment.line to donation user because rights on payment orders should be very restricted. Use sudo() to create the payment order & line in the code.
The migration is made from v10 because I figured out that commit history was lost during the v11 migration, cf OCA#53 Improve chatter message to add a link to the payment order Don't give rights on account.payment.order or account.payment.line to donation user because rights on payment orders should be very restricted. Use sudo() to create the payment order & line in the code.
No description provided.