Page MenuHome

Update the views
Needs RevisionPublic

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

Details

Diff Detail

Repository
rL Looper
Branch
review2/04/views
Build Status
Buildable 7376
Build 7376: arc lint + arc unit

Event Timeline

Revert the lookup of the PaymentMethod on the Customer

I haven't finished reviewing this patch yet, but here are the first comments already.

looper/admin.py
90–92

Just return collection_method, I guess.

looper/checkout_logic.py
27

pass is not necessary here.

44

payed → paid

98–100

Nope, not automatically. We could at some point in the payment flow ask the customer "you paid with a different method, do you want to use this as method for future payments of this subscription?" and if they say yes, we change it.

137

pass is unnecessary.

168

Don't use f-strings in logging, but use %-formatting and pass subscription.pk as argument to log.debug(). That will forego the formatting when debug logging is disabled. Same for other places where f-strings are used in logging.

looper/forms.py
17

How do you now ensure that forms rendered by Django have the correct CSS classes?

41

How can users (as defined by Django's User model) even have an impact on this?

45

Eek, this should not be "hidden" inside some function. The set of mandatory payment gateways should be configured somewhere centrally.

Sybren A. Stüvel (sybren) requested changes to this revision.Mon, Mar 30, 7:04 PM
Sybren A. Stüvel (sybren) added inline comments.
looper/forms.py
88

As this returns cleaned data, maybe cleaned_plan_variation is a better name.

looper/middleware.py
111

Since lookup_country already returns Optional[…], just make it handle a None parameter. Also see my note about get_client_ip() in D7243.

looper/templates/looper/checkout/new_subscription.html
25

Why is it necessary to explicitly dispatch a change event? This should be motivated in a comment.

looper/templates/looper/form_base.html
52

In which function? It's not that hard to find, but even easier when it's just mentioned here.

looper/views/add_payment_method.py
24

I'm guessing this even handles a PaymentMethodForm. It's better to mention that explicitly, so that it shows up in search results for the form class name.

86

This is going to be tricky, because it'll ask the user whether they'll want to re-POST the request. That's not good thing to ask for, as it'll likely fail again. Setting a message in the session + returning a redirect so that a GET is performed would be better.

looper/views/checkout/existing_order.py
26

I'm still not too happy with the can_change_customer name. A single function is used for multiple goals: identifying that the user is (member of) the customer (i.e. authentication), and permissions to even vew orders (i.e. authorization). For me "can change customer" is not the same as "can pay for order".

Why not just use Django's permission model? That also supports custom permissions, so it can be extended in whatever way we like.

117–118

Same comment as in the add payment form handling

125

This situation should be logged.

134–141

This naming is a bit strange, and I'm not a fan of directly checking the class here. Usually when something is not a "success" it's a failure, but here it's "success" vs. "manual". To me it looks like it's comparing things on different abstraction levels. I think "payment complete" / "requires manual payment" would be a better way to look at things. This can then also be modeled in a function is_payment_complete() or the opposite requires_user_action(), which can then simply be called instead of using isinstance().

looper/views/checkout/new_subscription.py
28–29

There is no order given, so the comment is incorrect. It should also mention that it creates a subscription, as that's the higher abstraction of what's happening here.

looper/views/checkout/successful.py
2

"Successful" is not really a descriptive name. Earlier in the code a payment result could be "success" or "manual", but this function handles both situations. Please don't use the same terms with different semantics.

looper/views/preferred_currency.py
14

This only works for missing keys, and treats empty strings as valid. This means it can redirect to an empty URL. I would just do this handle both 'empty' and 'missing' equally:

next_url = request.POST.get('next')
if not next_url:
  …
15

This should either be an API (so code-facing) endpoint and return proper JSON (to be shown in the context of a page), or be a human-facing endpoint and return a proper page. If anything goes wrong now, the user just sees a string.

looper/views/render.py
48

Why disable recaptcha?

61

Why disable recaptcha?

66–67

DOCS! This really needs documentation. The name is so generic that it looks like it's always used when a user is not logged in. However, it is only called from the 'new subscription' view, so it is something rather specific. This should be clear from the name & docstring.

70

Docs & Naming! There are so many things that can be successful that this really needs documentation and a better name.

75

Same as above, this needs documentation.

I understand that you want to split up the rendering from the rest of the code, but separating it from the code's context does add the extra burden of making the code understandable without having to look up where and when it's called exactly.

looper/views/tokens.py
17–20

Minimize what's inside the try block.

Also same as above, only the 'missing' case is handled here, and not the 'empty' case. Something like this is better. This also separates the two things that now happen in one line (get the token from the session, and cast to the right type).

token_in_session = request.session.get(key)
if token_in_session:
    return BraintreeClientToken(cast(str, token_in_session))

Also, isn't it possible to just return cast(BraintreeClientToken, token_in_session)?

This revision now requires changes to proceed.Mon, Mar 30, 7:04 PM

Nice code, and it really helps to separate things into separate modules, the function-based views like you have now are clear & understandable. Thanks for cleaning it up so much! The nagging in the inline comments is only about (sometimes important, sure) details ;-)

Sem Mulder (SemMulder) marked 16 inline comments as done.
  • Remove the IpAddress NewType
  • Address some of the raised concerns
looper/forms.py
17

By adding them to the template.

41

Users here is meant to be the person integrating Looper. I now refer to the "downstream project" instead.

looper/views/checkout/existing_order.py
125

It is already logged as part of Transaction.charge.

134–141

The difficulty here is that the different types contain different data. So if we did implement it using a is_payment_complete function we would have to assert all the data we need is there.

looper/views/render.py
48

Because the old code did not check the reCAPTCHA when paying for an existing Order. I figured there was a reason for that, but, if not, we can just enable it here.

61

Because the old code did not check the reCAPTCHA when paying for an existing Order. I figured there was a reason for that, but, if not, we can just enable it here.

looper/forms.py
17

Which template? This entire class was made to have simple templates, such that {{ form }} and {{ form.field }} simply produces the correct HTML, without having to hand-write all the HTML and add CSS classes there. This follows Django's DRY principle, and makes it much easier to produce nice forms. It may have been ugly code, but it does work.

Anyway, form styling and the likes are supposed to happen at the project level, and not dictated by Looper, so it's ok to remove it from here.

looper/views/checkout/existing_order.py
26

A single function is used for multiple goals: identifying that the user is (member of) the customer (i.e. authentication), and permissions to even vew orders (i.e. authorization).

Only the latter really (e.g. for admins we could also return True even if they are not a member of the organization). Looper by design also does not care about whether a User is a part of a Customer, that is left entirely to the downstream project. Instead, Looper only cares about the following three questions: "Can this User add a new subscription for this Customer?", "Can this User pay for an existing Order for this Customer?", "Can this User add a PaymentMethod for this Customer?".

For me "can change customer" is not the same as "can pay for order".

I agree, I considered splitting up the permissions into e.g. can_pay_for_order, can_checkout_new_subscription etc. but since, in practice, they would all be equivalent I just went with one function for now. We should be able to come up with a better name than can_change_customer, though. is_authorized_for_customer maybe?

Why not just use Django's permission model? That also supports custom permissions, so it can be extended in whatever way we like.

I did look into this, but because we need per object permissions it seemed rather complicated. See:

Apparently, we should then implement a custom AuthenticationBackend. Which to me seems pretty complicated for, as far as I know, not a lot of benefits.

Sybren A. Stüvel (sybren) requested changes to this revision.Tue, Mar 31, 11:55 AM

About the note in checkout/existing_order.py lines 134-141 (the note itself seems to be gone from Phabricator, which is weird, so I just respond here).

The difficulty here is that the different types contain different data. So if we did implement it using a is_payment_complete function we would have to assert all the data we need is there.

I don't understand. I was thinking about something like this, I don't see how that's so tricky to do:

class PaymentResult(abc.ABC):
    @abc.abstractmethod
    def is_payment_complete() -> bool:
        pass

class PaymentSuccess(PaymentResult):
    def is_payment_complete() -> bool:
        return True

class ManualPayment(PaymentResult):
    def is_payment_complete() -> bool:
        return False
looper/views/preferred_currency.py
29

Having unchecked arbitrary redirects is a security leak. With this code, any user can be presented with a URL that looks like it points to https://fund.blender.org/ but actually redirects to an attacker's website. This can be used to set up a fraudulent clone of the DevFund website while using legitimate-looking URLs.

This revision now requires changes to proceed.Tue, Mar 31, 11:55 AM
looper/views/render.py
48

Paying an existing order can only be done once, so automating it with a bot wouldn't make much sense. It's a good idea to document that motivation here in a comment, though.

looper/views/checkout/existing_order.py
26

is_authorized_for_customer looks good to me.

I don't understand. I was thinking about something like this, I don't see how that's so tricky to do: ...

Including the fields we need we would get:

python
class PaymentResult(abc.ABC):
    @abc.abstractmethod
    def is_payment_complete() -> bool:
        pass

class PaymentSuccess(PaymentResult):
    def is_payment_complete() -> bool:
        return True

    def __init__(self, transaction_id: int) -> None:
        self.transaction_id = transaction_id

class ManualPayment(PaymentResult):
    def is_payment_complete() -> bool:
        return False

    def __init__(self, order_id: int, gateway_name: str) -> None:
        self.order_id = order_id
        self.gateway_name = gateway_name

Now we can check the value of is_payment_complete instead of doing the isinstance check. However, when subsequently accessing e.g. payment_result.order_id Mypy will tell us that a PaymentResult does not have a field order_id. This is because Mypy doesn't understand that if is_payment_complete() returns False it can safely assume the PaymentResult is actually a ManualPayment.

looper/forms.py
17

The templates for these forms live in looper/templates/looper/forms/ (just Ctrl + F in this Differential).

This entire class was made to have simple templates, such that {{ form }} and {{ form.field }} simply produces the correct HTML, without having to hand-write all the HTML and add CSS classes there. This follows Django's DRY principle, and makes it much easier to produce nice forms. It may have been ugly code, but it does work.

I understand that, and for AddressForms this is the way it is done. However, the other forms are very tightly bound to some JS, in which case I'd rather provide the template myself to make all IDs static and explicit.

Anyway, form styling and the likes are supposed to happen at the project level, and not dictated by Looper, so it's ok to remove it from here.

This was also one of the motivations of doing it the way I did now, since now people can just override the template and be done with it. Using the other approach we would have to find a way to let downstream projects inject things into the Form rendering pipeline within Python.