Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add TaxRate model #1027

Merged
merged 14 commits into from
Dec 14, 2019
Merged

Add TaxRate model #1027

merged 14 commits into from
Dec 14, 2019

Conversation

therefromhere
Copy link
Contributor

@therefromhere therefromhere commented Oct 7, 2019 •
edited
Loading

Work in progress on TaxRate model

todo:

  • Invoice.default_tax_rates
  • InvoiceItem.tax_rates
  • Invoice.total_tax_amounts
  • test update of total_tax_amounts
  • Subscription.default_tax_rates
  • SubscriptionItem.tax_rates
  • Handle / test for UpcomingInvoice

These are list of TaxRate objects or in the case of Invoice.total_tax_amounts, a list of amounts with a tax_rate id, eg:

  "total_tax_amounts": [
    {
      "amount": 412,
      "inclusive": false,
      "tax_rate": "txr_1FQrXECOCguPTL2B7vqVaWgz"
    },
    {
      "amount": 185,
      "inclusive": false,
      "tax_rate": "txr_1FQr7MCOCguPTL2B1lRnkOu2"
    }
  ],

I think it'd be sensible to add ManyToMany fields to handle these, probably with the joining table models named Djstripe... following the example of DjstripePaymentMethod.

FYI @kavdev

@codecov
Copy link

codecov bot commented Oct 12, 2019 •
edited
Loading

Codecov Report

Merging #1027 into master will increase coverage by <.01%.
The diff coverage is 97.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1027      +/-   ##
==========================================
+ Coverage   97.55%   97.56%   +<.01%     
==========================================
  Files          25       25              
  Lines        2459     2542      +83     
  Branches      223      230       +7     
==========================================
+ Hits         2399     2480      +81     
  Misses         46       46              
- Partials       14       16       +2
Impacted Files Coverage Δ
djstripe/models/__init__.py 100% <ø> (ø) ⬆️
djstripe/contrib/rest_framework/serializers.py 100% <100%> (ø) ⬆️
djstripe/models/base.py 97.02% <96.15%> (-0.1%) ⬇️
djstripe/models/billing.py 99.53% <98.24%> (-0.2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ea0953...96611c0. Read the comment docs.

@therefromhere therefromhere force-pushed the tax_rates branch 4 times, most recently from c158de1 to 4d75d0c Compare October 12, 2019 23:48
@therefromhere
Copy link
Contributor Author

therefromhere commented Oct 13, 2019 •
edited
Loading

Note that the ManyToMany fields that are just linking to TaxRate (and don't have an associated value) don't necessarily need to have an explicit joining table, but it seems sensible to be consistent, and having Djstripe in any internal models avoids the risk of clashing with new stripe objects down the line.

Also worth noting that Django's default ManyToMany models include a unique on the fks, I've intentionally not implemented because though it would be weird, there's nothing to say we can't get the same tax twice in the list.

edit: I'll implement the unique as per the django default since you sensibly do get an error from Stripe if you try to attach the same tax rate twice ("You cannot attach more than one of the same tax rate."), so I think it's safe to assume they'll be unique.

@therefromhere therefromhere force-pushed the tax_rates branch 4 times, most recently from 917ce3d to 61c50af Compare October 13, 2019 05:19
@therefromhere therefromhere marked this pull request as ready for review December 8, 2019 05:46
@therefromhere therefromhere requested a review from kavdev December 8, 2019 05:47
@therefromhere
Copy link
Contributor Author

@kavdev currently this doesn't support UpcomingInvoice - this was causing me some problems because it would mean setting (or faking) a ManyToMany field on an unsaved model.

Thoughts? (do you think not supporting tax rates on UpcomingInvoices for now acceptable?)

@therefromhere therefromhere mentioned this pull request Dec 8, 2019
Closed
3 tasks
kavdev
kavdev reviewed Dec 8, 2019
View reviewed changes
djstripe/models/billing.py Outdated Show resolved Hide resolved
@kavdev
Copy link
Member

kavdev commented Dec 8, 2019

lgtm -- solid work. I'd expect people to expect to be able to see tax rates on upcoming invoices. How much of a pain is it to implement?

@therefromhere therefromhere mentioned this pull request Dec 14, 2019
Merged
@therefromhere
Refactor UpcomingInvoice so it's no longer a subclass of Invoice
1d6124a 
This is to allow Invoice to use ManyToManyFields.
@therefromhere
Test fixes for bogus upcominginvoice relation
c1a29ff
@therefromhere
Add TaxRate model
fcee5ea 
* Add ManyToManyField for Subscription.tax_rates,
  Invoice.default_tax_rates etc.  We don't need to explicitly specify
  the joining model, but specifying the joining table name
  to match what would be generated if the joining table names
  had the prefix DjStripe, to avoid potential name clash if Stripe
  added models with these names.
@therefromhere
Test that update of invoice total_tax_rates works as expected
f0a04f9
@therefromhere
Bump migration
45d950a
@therefromhere
Updated comment, fixed typo
aecf680
@therefromhere
Add comment explaining use of db_table
f0f263f
@therefromhere
Bump migration
5570ce5
@therefromhere
Add support for default_tax_rates and total_tax_amounts to UpcomingIn…
15f94a5 
…voice
@therefromhere
Copy link
Contributor Author

I've added UpcomingInvoice.total_tax_amounts and .default_tax_rates based on the refactor from #1077, just need to add some tests.

@therefromhere therefromhere requested a review from kavdev December 14, 2019 04:52
@therefromhere
Copy link
Contributor Author

Added tests for UpcomingInvoice.total_tax_amounts and .default_tax_rates.

kavdev
kavdev approved these changes Dec 14, 2019
View reviewed changes
Copy link
Member

@kavdev kavdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌🏼

@therefromhere therefromhere merged commit adda5fb into dj-stripe:master Dec 14, 2019
@therefromhere therefromhere deleted the tax_rates branch December 14, 2019 05:53
@therefromhere therefromhere mentioned this pull request Jun 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kavdev kavdev kavdev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@therefromhere @kavdev