Page MenuHome

Update the documentation
ClosedPublic

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

Diff Detail

Repository
rL Looper

Event Timeline

Sybren A. Stüvel (sybren) requested changes to this revision.Fri, Mar 27, 11:39 AM

Nice docs! Really good to see things written out like this. Lots of small nags, and one really big one.

README.md
47–48

Maybe it's nice to write something about what it would entail to add another gateway, or to disable one of those two?

85–86

I wouldn't describe this as a single-sided debt. It is a contract that binds payment from a Customer to the delivery of the Product.
It can only be called a debt if the Product is delivered before the order is paid. If they stop paying, we stop delivering, but at no time is there any obligation to pay.

92–93

This isn't accurate. The payment attempt could be done by the Looper clock as well. It's always done on behalf of the Customer, though.

97

"A PaymentMethod is exactly what the name implies:" should be removed. Implications are subjective. For readers for whom this is true it's superfluous, and for readers for whom it is not true it's annoying.

101

Maybe add a bit here that the bank payment method can only exist once, as no bank account information is stored.

107

Remove the "Obviously, ordering a new subscription is an important process." for the same reason as above: it's either superfluous or annoying.

110

Add an indication as to where the customer navigates from. I'm guessing it's from a product page outside of Looper. Making this explicit gives the reader a nice indication "this is where the Customer transitions from the 'custom' pages to Looper".

111

Use 'their' as gender-neutral pronoun. "It" should be used for things, not people.

114

Remove "immediately". The previous and next steps are also done immediately, but not described as such (which is fine, and makes this sentence inconsistent with the rest).

121–122

If we previously failed to charge a Customer …

or the Payment Method in use does not allow automatic transactions, like the bank gateway, …

125

its → their

142–143

This is the first time 'managed payment' is mentioned. It's probably better to describe these collection methods first in a separate section. They are also tightly bound to the gateway chosen, as we do not allow manual payment for Braintree, and do not allow automatic payment for bank transfers.

144

I'm not sure how this happens any more ;-) I'm guessing it looks at the 'next payment date' to determine the datetime by which it should transition to Cancelled. Maybe check in the implementation and add it here?

148–149

I don't understand this reasoning. Automatically charging a credit card also requires communication with Braintree, but Looper doesn't provide a page for that (because it's a cron job). The reason that there is a page is not the communication with Braintree, but because people want to manage their payment methods.

153

its → their

156

This is a weird title. "Integrating Looper with a Django project" would be a better one, and I think this should be a 1st level section. Contrary to the previous sections it's now a very technical part of the documentation, rather than a high-level explanation of the workings.

186

They moved it to Settings cog → Business → Merchant Accounts.

239

First manage.py migrate should be run. As that should be done after updating the settings (most notably, after adding Looper to the installed apps), it's probably a good idea to mention that here again.

251

What we do or do not like to do isn't really relevant here. Probably better to just write "This section describes how to connect your actual users and products to Looper"

272

This is not a comment, just a question: do you think we should at some point introduce custom Django signals for Looper events? Or is it enough to use the standard Django signals?

276–280

It's still a bit vague to me how this works, when this function is called, and which user & customer will be passed to it. I'm guessing it's the currently logged-in unser, and the Customer they're trying to edit/view/do stuff for, but this is not described. Maybe it'll become clearer later, but if not, it could use a bit more explanation (instead of just an example that's left to the reader to extrapolate).

The "can change" in the function name is maybe the most confusing. The question that I'm guessing this function answers is "Is this User (member of) this Customer?" If that is the case, the permission to actually change that customer has little to do with it, and is actually a side-effect of this function.

Let's say the Customer is linked to an Organisation, and Looper is being used for Blender Cloud subscriptions. Does this function return True for every user that's part of that organisation, i.e. for every user who gets permission to use Blender Cloud? Or is there some way in which users can be granted Blender Cloud access without being able to modify data in Looper? Preferably only a select few people in an organisation should be allowed to manage the subscription, and the rest of the organisation should just be able to use the service.

319

Please make a distinction between a name for human consumption (which can change) and an identifier. I really don't want to have to tell Ton that if he changes the name of the 'Blender Cloud' product to 'Blender Studio' that we have to go through the source code of the Cloud and deploy at the same time he makes the change in order to avoid breaking the planet.

Add a field 'identifier' that can only be set once, when the product is created (or when it's empty, which may be easier to implement), and after that is read-only. There should always be a separation between "marketing name of the product" and "identifier used in the software".

324–325

Will those be automatically added to the admin forms as well?

This revision now requires changes to proceed.Fri, Mar 27, 11:39 AM

Revert the lookup of the PaymentMethod on the Customer

and one really big one.

Which one is the big one?

Sem Mulder (SemMulder) marked 18 inline comments as done.

Address some of the raised concerns

README.md
47–48

I am not entirely sure what exactly would be required to do so myself. So unfortunately I am currently not in a position to be able to write documentation about that.

272

In this specific case the post_save signal originated from a User being saved, so that is not a Looper event.

And I believe we already have signals for the relevant Looper events:

# Sender is the Subscription.
subscription_activated = Signal(providing_args=['old_status'])
subscription_deactivated = Signal(providing_args=['old_status'])
# Sent when next_payment < now and last_notification < next_payment:
managed_subscription_notification = Signal()

# Sender is the Order that's being processed.
automatic_payment_succesful = Signal(providing_args=['transaction'])
automatic_payment_soft_failed = Signal(providing_args=['transaction'])
automatic_payment_failed = Signal(providing_args=['transaction'])
276–280

I think were the confusion comes from is the fact that currently *every page* in Looper is used to change the Customer (e.g. managing Subscriptions, PaymentMethods, paying for Orders) and all the pages that just need to know if a User has Blender Cloud access are the responsibility of the Blender Cloud.

By design Looper doesn't care how an actual User is related to a Customer, the only thing it cares about is the question "can this User change this Customer" which is the question that is answered by this function.

So to answer your questions:

Does this function return True for every user that's part of that organisation, i.e. for every user who gets permission to use Blender Cloud?

No, it returns True iff. the User is a staff member of the Organization (i.e. allowed to change the Customer).

Or is there some way in which users can be granted Blender Cloud access without being able to modify data in Looper?

Exactly, the fact that a non-staff User is part of an Organization that has a Subscription is enough to know that a User has Blender Cloud accesss. We do not need to modify anything to be able to infer this.

select few people in an organisation should be allowed to manage the subscription

This is precisely what is defined using this function. That is, the set of Users for which the function returns True are allowed to manage the Subscription.

and the rest of the organisation should just be able to use the service.

As above, this is inferred in the Blender Cloud (i.e. not Looper) by seeing that a User is part of an Organization that has a Subscription.

319

This was only to make the example more readable. Nonetheless, I agree we should promote good practices (especially) in the examples. So I now filter on the plan_id field instead.

324–325

Nope, that is not something that Looper can do without a lot of magic programming tricks. However, the downstream project can add it to the admin however they see fit.

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Mar 31, 9:59 AM
This revision was automatically updated to reflect the committed changes.