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.

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

Remove _ensure_full_coverage step from invoicing #35503

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nospame
Copy link
Contributor

@nospame nospame commented Dec 11, 2024

Technical Summary

SAAS-16270

In the past, a project space having no assigned subscription would mean they were on the Community plan. This meant that, when generating invoices, we would also have to check if there were periods of no subscription and count them as Community for invoicing purposes (because Community plans can still incur charges). This would happen through _ensure_full_coverage, by creating a backdated Community plan subscription for the invoicing period.

More recently, all project spaces are always assigned a subscription even when there is "no" subscription -- Community when signing up initially, or Paused if they were previously on a subscription that expired or went unpaid. This means that _ensure_full_coverage is no longer relevant and can be removed, along with tests for it. Some other invoicing tests have been tweaked to account for the change.

Safety Assurance

Safety story

I've dug a bunch into this and other subscription-related code and become confident that under no reasonable circumstances can a project space end up without a subscription these days. Changes to invoicing can be fairly high-risk, however the worst scenario here is likely that a project space which was on "no subscription" and actually should have been on "Community" does not get invoiced for charges.

There are currently ~300 "active" project spaces on production with no assigned subscription, though none have submitted any forms in the past 30 days, and it seems that none of these spaces ever had a subscription, likely indicating that subscription setup failed during project creation. The fact that they have never had a subscription also means they have never had enough use to incur charges on a Community plan (or a backdated subscription would have been created via _ensure_full_coverage), so leaving them be is probably harmless. If there is concern about these, I can put together a script to move all of these project spaces explicitly to Community.

Automated test coverage

There is good test coverage of invoicing between test_invoicing and test_invoice_factory, one case of which I actually removed because it seemed to overlap entirely between the two.

QA Plan

Not planning QA. Planning to put this on staging for a bit (through the end of the month, so the invoicing task can run as usual) and see that results look normal. All automated testing shows that it should be fine.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

Redundant between test_subscription_invoice and TestDomainInvoiceFactory::test_community_plan_generates_invoice
This should never happen in real life because all accounts are assigned a subscription, but is still a relevant test case to ensure that invoicing doesn't fail altogether
@nospame nospame marked this pull request as ready for review December 11, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant