Add support for upcoming invoices w/ invoice items #320
Conversation
lskillen
changed the title
Current coverage is 100%
@@ #162-api-updates-through_2015-07-28 #320 diff @@
=================================================================
Files 23 23
Lines 1394 1475 +81
Methods 0 0
Messages 0 0
Branches 156 171 +15
=================================================================
+ Hits 1394 1475 +81
Misses 0 0
Partials 0 0
|
| if not self.pk: | ||
| # InvoiceItems need a saved invoice because they're associated via | ||
| # a RelatedManager. It might be better to make this pattern more | ||
| # generic with some sort-of _attach_objects_post_save_hook(). |
There was a problem hiding this comment.
I agree with this. Would you mind adding that post save hook? (Another PR is fine if that would cause any conflicts in the PR chain)
Adds a new _attach_objects_post_save_hook, for attaching Stripe objects to an instance only after it is saved.
| """ | ||
| instance = cls(**cls._stripe_object_to_record(data)) | ||
| instance._attach_objects_hook(cls, data) | ||
| instance.save() | ||
|
|
||
| if save: |
There was a problem hiding this comment.
Wrap these two lines in newlines please
|
No problem, changes made - The double-space thing is going to be interesting to fix, in general. Some brain rewiring will be required as I've been doing that for years and years since I was forced to. ;-) You're right though, it technically isn't correct by today's standards. I even had spaces in this paragraph before I went back and deleted them. |
| return instance | ||
|
|
||
| @classmethod | ||
| def _get_or_create_from_stripe_object(cls, data, field_name="id"): | ||
| def _get_or_create_from_stripe_object(cls, data, field_name="id", |
There was a problem hiding this comment.
That pushes it beyond 80 characters - Is that acceptable? We usually keep ours below it in line with PEP-8 (and it is likely a common theme throughout any submits I've made), but this is your project and I'll follow whatever guidelines you lay out. Could also bring it down a line instead?
def _get_or_create_from_stripe_object(
cls, data, field_name="id", refetch=True, save=True):
field = data.get(field_name)
There was a problem hiding this comment.
OK, seen your further comments later that E501 is ignored - Noted!
|
@kavdev I'm sure you're still reviewing (thanks btw, I do appreciate it), but I've added some comments as responses to yours. These are easy to miss sometimes so just pinging you here to raise awareness. Cheers! |
| if not line["id"].startswith(invoice.stripe_id): | ||
| line["id"] = "{invoice_id}-{sub_id}".format( | ||
| invoice_id=invoice.stripe_id, | ||
| sub_id=line["id"]) |
|
For future reference: Unless I've already made the change and pushed it I'll add a thumbs up to a comment just so you know I've acknowledged it. This will either mean I've made the change locally already and I am waiting to push for some reason (such as giving you a chance to make further comments) or will implement the change as soon as I can then push. Hope that somewhat helps reviews. ;) |
| save = False | ||
|
|
||
| line.setdefault("customer", invoice.customer.stripe_id) | ||
| line.setdefault("date", int(invoice.date.strftime("%s"))) |
There was a problem hiding this comment.
Is this meant to convert a datetime to a unix timestamp? If so, please use calendar.timegm(invoice.date.timetuple()) instead.
There was a problem hiding this comment.
Yep, that's right - Looks like we can borrow from Django instead though:
from django.utils import dateformat
line.setdefault("date", dateformat.format(invoice.date, 'U'))
I looked at the implementation and it's pretty similar to your suggestion with the main difference that it uses utctimetuple if the datetime is timezone aware:
def U(self):
"Seconds since the Unix epoch (January 1 1970 00:00:00 GMT)"
if isinstance(self.data, datetime.datetime) and is_aware(self.data):
return int(calendar.timegm(self.data.utctimetuple()))
else:
return int(time.mktime(self.data.timetuple()))
Thoughts?
There was a problem hiding this comment.
@lskillen: Amazing find! The calendar version is used throughout dj-stripe. I'll refactor after all of these PRs are merged.
|
@kavdev Odd, those two hyperlinks don't take me to the comment you were talking about, that's why I missed it (sorry, wasn't being purposefully ignorant!). Found it by stepping back through the comments though. I have two concerns with making it
My suggestions for the above:
I'll defer to you - Thoughts? |
|
@lskillen: Having the call as a shortcut in Customer is fine with me -- you got me on API parity :) As for the |
|
@kavdev OK, sounds good to me - Tried to make things more consistent with regards to the Stripe object data versus dj-stripe objects divide. In order to keep the API parity I have created a new |
| stripe_invoice = StripeInvoice.upcoming(**kwargs) | ||
|
|
||
| if stripe_invoice: | ||
| return cls._create_from_stripe_object(stripe_invoice, save=False) |
There was a problem hiding this comment.
Is the class check in 567-571 necessary? You could just use UpcomingInvoice on this line instead of cls. If that was put in for extendibility, I'd rather go with the class replacement settings idea from a previous conversation.
There was a problem hiding this comment.
Yes, it was (originally), but I'll remove this and the extensible support can come later.
|
@lskillen: Almost there! |
|
OK, fixes added - If it's all good I'll squash the secondary commits. |
| :rtype: ``djstripe.models.UpcomingInvoice`` | ||
| """ | ||
| try: | ||
| data = cls._api().upcoming( |
There was a problem hiding this comment.
Use upcoming_stripe_invoice here as well (instead of data) and that'll be the last fix
|
@lskillen: at over 95 comments, I believe you do, in fact, hold the record haha |
Also adds migrations (including missing migrations).
Adds a new _attach_objects_post_save_hook, for attaching Stripe objects to an instance only after it is saved.
lskillen
force-pushed
the
feature-upcoming-invoices
branch
from
1af6d21 to
ce07a63
Compare
|
@kavdev: You're a harsh task master, Alex! ;-) All sorted and squashed. |
|
Perfect, merging! |
|
@lskillen: What do you think about renaming |
|
(don't worry - I would be making the changes) |
|
@kavdev That'd be OK too (it still fits the function), although the "upcoming invoice" nomenclature appears more often than the association with preview, such as:
Part of this might be due to backwards compatibility though - I think the preview features were added over time rather than being there from the start. Might just be easier leaving it if we assume that developers are looking up the API documentation and searching for "upcoming invoice" (I know I did several times!) |
|
@lskillen: This line makes things pretty clear:
I'll keep it as |
|
@lskillen see above commit |
|
LGTM! 👍 |
Description
Adds support for Stripe's API for previewing upcoming invoices, and also adds support for subscription-based invoice items - This was a rather tricky one to implement given the ephemeral nature of the upcoming invoices. They aren't persisted to the database and their internals differ from the expected "norm" (i.e. they don't have a stripe ID).
An example of usage (as we have it in our application):
An additional
planproperty accessor has been added to invoices - As detailed in the code comments, this was necessary for us to provide a consistent view for the invoice history. Instead of using the plan from the invoice subscription, it is now possible to use the plan for the from available subscription invoice item, and this access is encapsulated in the property.The maximum length for stripe fields was also raised to 255, in accordance with the recommendations in the Stripe API Upgrades documentation. The main reason though was to make room for the longer composite key IDs used for subscription-based invoice items (necessary in order to ensure these remain unique per invoice). As per the
planchange above, this also helps to provide a consistent view for the invoice history.One odd thing to note is the use of the
QuerySetMockforUpcomingInvoice. The reasoning behind this was to ensure thatinvoice.invoiceitems()for upcoming invoices still returned a queryset-like object that supports non-mutation operations such as iteration. I made a note in the documentation not to expect it to do anything else, so it's a bit like Stripe's own docs for upcoming invoices, they are not mutable. The main downside is the added dependency, so apologies for that!Finally, I guess that the changes for
_get_or_create_from_stripe_objectmight be contentious, which were required to handle the slightly different structure for the invoice items within the upcoming invoice, and to support things like not re-fetching the data from Stripe and not saving the created object. Both of the latter could be useful for some additional optimisations later.I'll be happy to discuss any part of the PR during review!
Test Output
Ran:
python runtests.py --skip-utc