Page MenuHome

Update the models for all the new stuff
Needs ReviewPublic

Authored by Sem Mulder (SemMulder) on Thu, Mar 26, 3:43 PM.


Diff Detail

rL Looper
Build Status
Buildable 7375
Build 7375: arc lint + arc unit

Event Timeline

Sybren A. Stüvel (sybren) requested changes to this revision.Fri, Mar 27, 12:32 PM

There are multiple conceptual changes that I really don't agree with: the removal of SharedFields, and the change in how payment methods are used.


Payment & Collection methods should remain visible in the inline order list.


Why is the payment method removed here? This is important to have access to as admin.


See other comments: Payment & collection methods should remain part of the Order and be visible in the admin.


This is just a dictionary lookup.


This is a conceptual change that I was not aware of, and that I also don't agree with. A customer should be able to have different subscriptions connected to different payment methods, and know exactly which payment method is going to be used for which subscription.

This for-loop creates a transaction for every active payment method, so instead of charging once for a subscription it charges every payment method. This is unacceptable. Even for a simulated tick.

Also, since when does order.generate_transaction() actually charge a customer? And since when do we charge a customer in a simulated tick? The comment is probably misleading.


Same comment as above, I don't agree with simply iterating over the payment methods until one works.


What is the reason that these signals don't receive the transaction any more? The transaction is the thing that failed, and thus contains important information.


Not supporting manual Braintree payments was a deliberate design choice for the Development Fund. I'm assuming that this change was deliberate as well, to allow migrating existing Cloud subscribers.

To support both Cloud and DevFund in Looper, the collection methods for each gateway should be configurable via, and not hard-coded in Looper.


By removing SharedFields and adding some fields back, the Order no longer has a payment_method. This is not good, as we should be able to look at previous orders and see which payment method they were using. There was a good reason the code was keeping track of things like that, so please restore it. The concept is that an Order basically copies the information from Subscription, so that the Subscription can change without influencing any existing Orders. The code was structured to make this very easy to see & extend. Now not only is that structure gone, but there is also no comment in the Order or Subscription classes that say anything about fields being copied or that they should remain in sync between the two classes.


Motivate this choice, as right now it just won't happen (as there is no clear benefit described).


PEP 257: Multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description.

This revision now requires changes to proceed.Fri, Mar 27, 12:32 PM

Revert the lookup of the PaymentMethod on the Customer

Sem Mulder (SemMulder) marked 9 inline comments as done.Fri, Mar 27, 3:36 PM
Sybren A. Stüvel (sybren) requested changes to this revision.Mon, Mar 30, 5:34 PM
Sybren A. Stüvel (sybren) added inline comments.

This can still return IpAddress(''), or any string that is not a valid IP address. Just use str as it was used before, as currently this type annotation hides the fact that any string can be returned.

I see the static typing advantage of using Optional[IpAddress], but as the function can still return any string, including empty strings and strings that don't represent any IP address, I strongly disagree with this approach. This now invites errors like in D7244 (where it is assumed that the value returned from this function is either None and not an empty string).

This revision now requires changes to proceed.Mon, Mar 30, 5:34 PM
  • Revert "Add IpAddress newtype"
Sem Mulder (SemMulder) marked an inline comment as done.Tue, Mar 31, 10:24 AM
  • Make can_change_customer PEP257 compliant
  • Remove TODO about using Exceptions
Sem Mulder (SemMulder) marked 2 inline comments as done.Tue, Mar 31, 10:31 AM