Code review comment for lp:~therp-nl/banking-addons/ba7.0-MIG-payment

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

Holger, thanks for your comments. I tried to address them in my latest commits or the comments below.

> #550 570, 880 considering how often you use this selection, you should move it to its own field. This also
> simplifies adjusting the selection

I'll leave it for now as I want to refactor it later on to make the selection options more dynamic.

> #1591 check if there are line_ids

As this code path occurs after a match with a bank transaction on amount, an absence of line_ids should not occur as the payment order amount would be zero as would the transaction amount.

> #1784 why can we drop this check? shouldn't it be changed to check for transit_move_line_id?

As per l.1591, implement old style behaviour for legacy payment orders depending on the presence of transit_move_line_id.

The sent state implements the transit move. Making it go into sent_wait allows the reconciliation process to cancel and reconfirm confirmed matches. Before I introduced the latter state and modified the workflow to go from 'done' to 'sent', I could not reconfirm a match on a payment order, because then the workflow would attempt to create the transit move again. I'd think that a manual migration for wkf instances currently in the 'sent' state should be necessary, or they might get stuck at that state.

« Back to merge proposal