Page MenuHome

Update the models for all the new stuff
Needs ReviewPublic

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

Details

Diff Detail

Repository
rL Looper
Branch
review2/03/models
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.

looper/admin.py
204–205

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

328

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

406–407

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

looper/clock.py
102–111

This is just a dictionary lookup.

249–251

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.

341–342

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

345–348

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.

looper/gateways.py
354

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 settings.py, and not hard-coded in Looper.

looper/models.py
1120

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.

1416

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

looper/permissions.py
15–18

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.
looper/utils.py
13

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