This is a general link for logging into the billing system on behalf of
a server, but it's tied to the .contact_email and takes the user
straight to the /deactivate/ page via the next_page mechanism.
This prep commit makes 'sync_license_ledger_if_needed'
function a 'BillingSession' abstract method.
We'll override the method for RemoteServerBillingSession
in the next commit.
This prep commit extracts out the code block that determines the
last license ledger for the customer plan having automanage_licenses
set to True into a new BillingSession method named
'get_last_ledger_for_automanaged_plan_if_exists'.
We'll be using this function while implementing the
'sync_license_ledger_if_needed' method for RemoteServerBillingSession.
We already calculate the correct `billed_licenses` early in the
function, so just used that to fix the bug where a legacy server
scheduled for upgrade doesn't respect the manual license count
set by the user.
I missed a some places to check on last pass:
* For automanaged licenses when the license updates.
* When plan is changed.
* When migrating existing customers to legacy plan.
I accidentally free trials for both cloud and self hosted
enabled while testing, hence didn't catch it.
This mostly involves fixing `is_free_trial_offer_enabled` to
return the correct value and providing it the correct input.
The way the flow goes now is this:
1. The user initiaties login via "Billing" in the gear menu.
2. That takes them to `/self-hosted-billing/` (possibly with a
`next_page` param if we use that for some gear menu options).
3. The server queries the bouncer to give the user a link with a signed
access token.
4. The user is redirected to that link (on `selfhosting.zulipchat.com`).
Now we have two cases, either the user is logging in for the first time
and already did in the past.
If this is the first time, we have:
5. The user is asked to fill in their email in a form that's shown,
pre-filled with the value provided inside the signed access token.
They POST this to the next endpoint.
6. The next endpoint sends a confirmation email to that address and asks
the user to go check their email.
7. The user clicks the link in their email is taken to the
from_confirmation endpoint.
8. Their initial RemoteBillingUser is created, a new signed link like in
(3) is generated and they're transparently taken back to (4),
where now that they have a RemoteBillingUser, they're handled
just like a user who already logged in before:
If the user already logged in before, they go straight here:
9. "Confirm login" page - they're shown their information (email and
full_name), can update
their full name in the form if they want. They also accept ToS here
if necessary. They POST this form back to
the endpoint and finally have a logged in session.
10. They're redirected to billing (or `next_page`) now that they have
access.
We simply apply discount to both the plans.
Since the discount is saved in `customer.default_discount` it
will applied now to any future plans as well, even if customer
downgrades and the upgrades again.
For the last form (with Full Name and ToS consent field), this pretty
shamelessly re-uses and directly renders the
corporate/remote_realm_billing_finalize_login_confirmation.html
template. That's probably good in terms of re-use, but calls for a
clean-up commit that will generalize the name of this template and the
classes/ids in the HTML.
When a remote server uploads statistics, we update the
LicenseLedger using the audit logs uploaded.
We iterate over the RemoteRealmAuditlog data for the concerned
realm starting from the event_time of the last LicenseLedger
created for that customer and update the ledger based on each event.
If the RemoteRealmAuditLog has stale data, it means the server
stopped or never uploaded data. We raise MissingDataError in such
cases when a user action led to calculating licenses count from
stale data.
We add a 'get_remote_realm_guest_and_non_guest_count'
function that queries 'RemoteRealmAuditLog' to get
the guest and non_guest count for that remote_realm.
This function is used in 'RemoteRealmBillingSession'
to calculate the current count of billed licenses.
* For free trial, don't show number of licenses for current billing period.
* For free trial scheduled to downgrade, don't show number of
licenses for next billing period.
Adds a helper since there are only a few different parameters for
all BillingSession child clases, `build_support_url`.
Also, renames `get_support_url` to more explicitly note that it
is for realms: `get_realm_support_url`.
Use of `string_id` in the sponsorship request email content was
removed in commit d3834f8b9, but it is still used in the email
subject.
Updates the email subject to use the billing_entity_display_name,
which is still the Realm.string_id for Zulip Cloud organizations.
Sets this string as "billing_entity" in the context and subject
template.
Moves the section in support views for any current plan details
to a new template: `templates/analytics/current_plan_details.html`.
Also, updates the PlanData dataclass to have a boolean that checks
if the current plan tier is the self-hosted legacy plan.
This commit moves the 'update_license_ledger_if_needed' and its
helper function 'update_license_ledger_for_automanaged_plan'
to the 'BillingSession' abstract class.
This refactoring will help in minimizing duplicate code while
supporting both realm and remote_server customers.
Moves the 'update_license_ledger_for_manual_plan' function
to the 'BillingSession' abstract class.
This refactoring will help in minimizing duplicate code while
supporting both realm and remote_server customers.
Also adds `SWITCH_PLAN_TIER_AT_PLAN_END` for `CustomerPlan`
which will be used to mark status of remote server legacy
plans which are scheduled for an upgrade.
The logic for BillingSession.is_sponsored_or_pending would be the
same for all three child classes of BillingSession, so this should
be a method on the BillingSession abstract class.
Creates a process_support_view_request method for BillingSession
to process the various support requests that relate to the billing
system.
Moves approve_realm_sponsorship, update_realm_sponsorship_status,
and attach_discount_to_realm to this new BillingSession method.
Adds a new abstract property to BillingSession to have a string
value, billing_entity_display_name, to use for support messages
sent when these requests are processed.
The "send_invoice" and "charge_automatically" strings used by stripe
for the `collection_method` are referred to both as the "billing
method" and "billing modality" in the billing code.
Because we send this as data to stripe as either `collection_method`
or `billing_modality`, renames any references that are any form of
"billing method".
Analogical to the more complex mechanism implemented for the RemoteRealm
flow in a previous commit in
authenticated_remote_realm_management_endpoint.
As explained in the code comment, this is much easier because:
In this flow, we can only redirect to our local "legacy server flow
login" page. That means that we can do it universally whether the user
has an expired
identity_dict, or just lacks any form of authentication info at all -
there are no security concerns since this is just a local redirect.
Implements a nice redirect flow to give a good UX for users attempting
to access a remote billing page with an expired RemoteRealm session e.g.
/realm/some-uuid/sponsorship - perhaps through their browser
history or just their session expired while they were doing things in
this billing system.
The logic has a few pieces:
1. get_remote_realm_from_session, if the user doesn't have a
identity_dict will raise RemoteBillingAuthenticationError.
2. If the user has an identity_dict, but it's expired, then
get_identity_dict_from_session inside of get_remote_realm_from_session
will raise RemoteBillingIdentityExpiredError.
3. The decorator authenticated_remote_realm_management_endpoint
catches that exception and uses some general logic, described in more
detail in the comments in the code, to figure out the right URL to
redirect them to. Something like:
https://theirserver.example.com/self-hosted-billing/?next_page=...
where the next_page param is determined based on parsing request.path
to see what kind of endpoint they're trying to access.
4. The remote_server_billing_entry endpoint is tweaked to also send
its uri scheme to the bouncer, so that the bouncer can know whether
to do the redirect on http or https.
This does two important things:
1. Fix return type of get_identity_dict_from_session to correctly be
Optional[Union[RemoteBillingIdentityDict, LegacyServerIdentityDict]].
RemoteBillingIdentityDict is the type in the 8.0+ auth flow,
LegacyServerIdentityDict is the type in old servers flow, where only
the server uuid info is available.
2. The uuid key used in request.session["remote_billing_identities"]
should be explicitly namespaced depending on which flow and type
we're
dealing with - to avoid confusion in case of collisions between a
realm and server that have the same UUID. Such a situation should not
occur naturally and I haven't come up with any actual exploitation
ideas that could utilize this by manipulating your server/realm
uuids, but it's much easier to just not think about such collision
security implications by making them impossible.
Since endpoints using the
`authenticated_remote_realm_management_endpoint` decorator
want to initialize a billing session and if need be remote_realm
is accessible to via the session variable.
We pass `next` parameter with /self-hosted-billing to redirect
users to the intended page after login.
Fixed realm_uuid incorrectly required in remote_realm_upgrade_page.
Updates do_change_plan_to_new_tier in BillingSession to use an
enum for the value returned when checking for a valid change
between two plan tier types. This makes it more explicit that
the implementation for a valid upgrade in plan tier will be
different from a valid downgrade in plan tier.
We've already processed the only URL parameters we intend to support
to determine which RemoteRealm or RemoteZulipServer is involved, so
there should be nothing further to do here.
And it's cleaner to not have to write the downstream code to expect
these unnecessary parameters.
This commit refactors 'sponsorship_request' view and adds
'BillingSession.get_sponsorship_request' method.
This refactoring will help in minimizing duplicate code
while supporting both realm and remote_Server customers.
This commit refactors 'event_status' view and adds
'BillingSession.get_event_status' method.
This refactoring will help in minimizing duplicate code
while supporting both realm and remote_server customers.
Moves and generalizes `switch_realm_from_standard_to_plus_plan`
in stripe.py to be a more general function for changing a
CustomerPlan to a new and valid tier, `do_change_plan_to_new_tier`.
Adds a helper function with the previous function name to be used
for the support view and management command for changing a realm
from the Standard plan tier to the Plus plan tier.
Renames CustomerPlan.SWITCH_NOW_FROM_STANDARD_TO_PLUS to be more
generic, CustomerPlan.SWITCH_PLAN_TIER_NOW.
Because the plan tier change is immediate, moves the code to end
the current plan with the old tier and create the new plan with
the new tier from `make_end_of_cycle_updates_if_needed` to instead
be in a separate helper function, `switch_plan_tier`.
This commit moves a major portion of the 'update_plan`
view to a new shared 'BillingSession.do_update_plan' method.
This refactoring will help in minimizing duplicate code
while supporting both realm and remote_server customers.
This commit updates the 'get_initial_upgrade_context' method
to use 'get_price_per_license' for determining 'annual_price'
and 'monthly_price' based on tier and discount instead of hardcoding.
Also, removed the 'percent_off' page_params as
'get_price_per_license' already performs the price calculation
taking discount into consideration.
If the update / add card session is successful, return user to
manual license management page if user was on it before clicking
the add / update card button.
This commit moves a major portion of the 'initial_upgrade`
view to a new shared 'BillingSession.do_initial_upgrade' method.
This refactoring will help in minimizing duplicate code
while supporting both realm and remote_server customers.
These new models are incomplete and totally untested, but merging this
will provide valuable scaffolding for doing smaller PRs working on
individual gaps, and reveals a clear set of TODOs/refactoring/model
changes needed to support where want to end up.
Co-authored-by: Tim Abbott <tabbott@zulip.com>
This commit moves the main context creation part of the
'billing_home` view to a new shared
'BillingSession.get_billing_page_context' method.
This refactoring will help in minimizing duplicate code
while supporting both realm and remote_server customers.
Moves the 'make_end_of_cycle_updates_if_needed' function to
the 'BillingSession' abstract class.
This refactoring will help in minimizing duplicate code while
supporting both realm and remote_server customers.
Since the function is called from our main daily billing cron job
as well, we have changed 'RealmBillingSession' to accept 'user=None'
(our convention for automated system jobs).
There is a discrepancy between price per license shown on billing
and upgrade page for annual billing frequency due to difference
in methods used to calculate the amount.
To fix it, we use the same method on both the pages.
I didn't use the helpers.format_money directly here since that would
create a visual delay in showing the price per license which will
not be nice considering that will be the only thing on this page
being updated that way.
The stripe fixture used is same as
free_trial_upgrade_by_card--Customer.retrieve.3.json
This commit moves a major portion of the 'upgrade`
view to a new shared 'BillingSession.do_upgrade' method.
This refactoring will help in minimizing duplicate code
while supporting both realm and remote_server customers.
Moves the 'process_initial_upgrade' function to the
'BillingSession' abstract class.
This refactoring will help in minimizing duplicate code while
supporting both realm and remote_server customers.
Updates `process_initial_upgrade` to take a plan_tier parameter,
so that information can be specific to the type of BillingSession.
Note that ideally the plan tier would be passed as metadata to the
stripe.checkout.Session, but in order to do so, we need to be able
to update the generated stripe fixtures for tests. So for now, we
set the plan tier directly in the stripe event handler code.
Instead of hard coding the string for the stripe.InvoiceItem and
stripe.Invoice statement_descriptor, we use the name of the plan
that was just created.
Creates `do_change_plan_type` as a BillingSession abstract method
as an alias for updating the plan_type for a realm (and eventually
remote server).
This removes the last direct reference to a customer's realm in
`process_initial_upgrade`.
Creates `current_count_for_billed_licenses` as an abstract method
in BillingSession class, to get the latest seat count for the realm
(and eventually remote server) connected to the customer.
Moves the 'setup_upgrade_checkout_session_and_payment_intent'
function to the 'BillingSession' abstract class.
This refactoring will help in minimizing duplicate code while
supporting both realm and remote_server customers.
Instead of using the data from the stripe.SetupIntent for processing
an initial upgrade for the free trial session types, use the metadata
from the stripe.checkout.Session that was returned.
The metadata is the same for both the checkout session and the setup
intent.
According to stripe's documentation for attaching payment methods to
customers (see https://stripe.com/docs/api/payment_methods/attach),
payment methods should be attached to customers through a SetupIntent
or PaymentIntent, which is what we do as we process new customers
and accounts.
Updates create_stripe_customer so that it is clear that the payment
method should not be added when we directly create a new stripe
customer.
There is a 22 char limit on the stripe.Invoice.statement_descriptor
field. The current value for that field in this error case, "Zulip
Cloud Standard Credit" is 27 chars.
Updates the value for this error case to be "Cloud Standard Credit".
Adds `create_stripe_payment_intent` to the BillingSession abstract
class for the initial upgrade process, which is used only in
`setup_upgrade_checkout_session_and_payment_intent`.
Adds a helper dataclass for the data used to create the stripe
payment intent, StripePaymentIntentData, for the implementation of
more than one child class of BillingSession.
Also adds two abstract helper functions for getting the above stripe
payment intent data as well as updating that data for a stripe
checkout session that is associated with a payment intent:
update_data_for_checkout_session_and_payment_intent,
get_data_for_stripe_payment_intent.
Adds `create_stripe_checkout_session` to BillingSession abstract
class, which is then used in all places where a stripe.checkout.Session
and Session were created: start_retry_payment_intent_session,
start_card_update_stripe_session, and
setup_upgrade_checkout_session_and_payment_intent.
Adds a `billing_session_url` abstract property to the BillingSession
abstract class and for the RealmBillingSession child class sets that
value to `self.realm.uri`.
Moves `update_billing_method_of_current_plan` to the BillingSession
abstract class.
Adds a helper function for support views for the realm case:
`update_realm_billing_method`.
Moves `update_sponsorship_status` to BillingSession abstract class
as `update_customer_sponsorship_status`.
Updates the support views to have a helper for updating this on a
realm: `update_realm_sponsorship_status`.
Makes `approve_sponshorship` an abstract method in BillingSession
abstract base class and moves the implementation for realms to the
RealmBillingSession child class.
Adds `approve_realm_sponsorship` helper function that's used in
the support view and initiates the billing session.
Creates an enum class, AuditLogEventType, and an abstract method in
BillingSession, get_audit_log_event, so that we have an abstraction
for getting the audit log event type since it might be different for
Customer objects with a realm vs a remote_server.
This moves the logic for `attach_realm_discount`, which is used in
the support view, to be in the BillingSession class.
Updates the function name to be `attach_discount_to_customer` so
that the context is generalized vs realm specific.
Updates RealmBillingSession implementation to account for actions
that are initiated by a support admin user.
Also moves the helper function `get_discount_for_realm` that is
only used in support views to `corporate/lib/support.py`.
So that all child classes of BillingSession generate the same data
structure for customers that are created in Stripe, revise
`get_data_for_stripe_customer` to return a specific dataclass:
StripeCustomerData.
So that `update_or_create_stripe_customer` can work for Customer
objects with either a realm or remote_server, we create an abstract
base class, BillingSession, and implement a child class for the
current implementation of Customer objects with a realm.
Refactoring `update_or_create_stripe_customer` also moves
`create_stripe_customer` and `replace_payment_method` to the
BillingSession class.
In ensure_customer_does_not_have_active_plan, we were already going
through the Customer table to get/check for an active CustomerPlan.
Now we directly get/check for an active CustomerPlan with via the
Customer, which allows for reusing this function for Customer
objects without a Realm set.
Moves two functions in corporate/lib/stripe.py that are used to
get data for the main installation activity analytics page to a
separate file: corporate/lib/analytics.py.
Also, updates these functions for the possibility of realm being
None for a Customer object.
Upgrading stripe to 6.0.0 in e32366638a
breaks our Stripe integration due to API version change making us fail
to finalize creating an invoice and charge the customer.
For the upstream details see:
60ab6ac7d7/CHANGELOG.md (600---2023-08-16)
6.0.0 uses 2023-08-16 Stripe API version unless specified otherwise. We
want to use 2020-08-27.
Setting stripe.api_version in corporate/lib/stripe.py is sufficient for
it to be set everywhere else. This is supported by the fact that we also
only set stripe.api_key in that file.
Fixes two bugs involving organization with
exempt_from_license_number_check enabled:
1. If the organization had e.g. 100 users and upgraded their plan,
specifying 50 licenses, the generated LicenseLedger and thus the
corresponding invoice was still for 100 users.
2. The organization was unable to use the billing/plan endpoint (update
plan endpoint) to make their number of licenses less than the current
number of users.
Organizations with exempt_from_license_number_check are supposed to be
able to declare whatever license number they want, as this attribute
allows having pricing schemes where an organization only pays us for a
subset of their users.
This migration applies under the assumption that extra_data_json has
been populated for all existing and coming audit log entries.
- This removes the manual conversions back and forth for extra_data
throughout the codebase including the orjson.loads(), orjson.dumps(),
and str() calls.
- The custom handler used for converting Decimal is removed since
DjangoJSONEncoder handles that for extra_data.
- We remove None-checks for extra_data because it is now no longer
nullable.
- Meanwhile, we want the bouncer to support processing RealmAuditLog entries for
remote servers before and after the JSONField migration on extra_data.
- Since now extra_data should always be a dict for the newer remote
server, which is now migrated, the test cases are updated to create
RealmAuditLog objects by passing a dict for extra_data before
sending over the analytics data. Note that while JSONField allows for
non-dict values, a proper remote server always passes a dict for
extra_data.
- We still test out the legacy extra_data format because not all
remote servers have migrated to use JSONField extra_data.
This verifies that support for extra_data being a string or None has not
been dropped.
Co-authored-by: Siddharth Asthana <siddharthasthana31@gmail.com>
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Translators benefit from the extra information in the field names, and
need the reordering freedom that isn’t available with multiple
positional fields.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Note that we use the DjangoJSONEncoder so that we have builtin support
for parsing Decimal and datetime.
During this intermediate state, the migration that creates
extra_data_json field has been run. We prepare for running the backfilling
migration that populates extra_data_json from extra_data.
This change implements double-write, which is important to keep the
state of extra data consistent. For most extra_data usage, this is
handled by the overriden `save` method on `AbstractRealmAuditLog`, where
we either generates extra_data_json using orjson.loads or
ast.literal_eval.
While backfilling ensures that old realm audit log entries have
extra_data_json populated, double-write ensures that any new entries
generated will also have extra_data_json set. So that we can then safely
rename extra_data_json to extra_data while ensuring the non-nullable
invariant.
For completeness, we additionally set RealmAuditLog.NEW_VALUE for
the USER_FULL_NAME_CHANGED event. This cannot be handled with the
overridden `save`.
This addresses: https://github.com/zulip/zulip/pull/23116#discussion_r1040277795
Note that extra_data_json at this point is not used yet. So the test
cases do not need to switch to testing extra_data_json. This is later
done after we rename extra_data_json to extra_data.
Double-write for the remote server audit logs is special, because we only
get the dumped bytes from an external source. Luckily, none of the
payload carries extra_data that is not generated using orjson.dumps for
audit logs of event types in SYNC_BILLING_EVENTS. This can be verified
by looking at:
`git grep -A 6 -E "event_type=.*(USER_CREATED|USER_ACTIVATED|USER_DEACTIVATED|USER_REACTIVATED|USER_ROLE_CHANGED|REALM_DEACTIVATED|REALM_REACTIVATED)"`
Therefore, we just need to populate extra_data_json doing an
orjson.loads call after a None-check.
Co-authored-by: Zixuan James Li <p359101898@gmail.com>
exempt_from_license_number_check was initially added allowing
organizations with it enabled to invite new users above their number of
licenses.
However, an organization with this permission enabled,
cannot upgrade if they weren't on a plan already - because when choosing
Manual license management, you cannot enter a number of licenses lower
than the current seat count. However, an organization like that probably
already has some users that they get free of charge - and thus they need
to be able to enter a lower number of licenses in order to upgrade.
Updates the message sent by the notification bot when an
organization is approved for full sponsorship on Zulip
Cloud Standard to include a request to list and link to
Zulip on any acknowledgement or sponsorship pages.
Adds the count of users with the role of guest to the stats view
`page_params` via a database query. This information is then added
to the summary statistics section of the analytics page after being
formatted by `stats.js`.
Creates Bassanio as a guest user in the database for the analytics
realm.
Fixes#20162.
Our seat count calculation is different for guest user than normal users
(a number of initial guests are free, and additional marginal guests are
worth 1/5 of a seat) - so these checks we apply when a user is being
invited or signing up need to know whether it's a guest or non-guest
being added.
This is a simple generalization of get_latest_seat_count and is useful
for calculating "what will be the realm's license count if this
number of (guest) users is added?" without duplicating any of the math
logic. Will be used in the next commits.
Our billing FAQ says:
"For an organization with N other users, 5*N guest users are included at
no extra charge. After that, you will be charged at 1/5 of your regular
per-user pricing for each additional guest.".
It wasn't quite intuitive to me that
max(non_guests, math.ceil(guests / 5)) achieves that pricing, so it's
worth mentioning in a comment that it does and that that's why that
formula is used.
`extra_data` as a `TextField` expects a `str`, but we had been passing
`dict` instead. This is a temporary solution before #18391 to fix the
type annotation.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
When being called, the wrapped function is passed `PaymentIntent`
(the `content_object` of `Event`). With that, since `customer` can be
`None`, an assertion is also required.
Signed-off-by: Zixuan James Li <p359101898@gmail.com>
Now that we pass in the UserProfile.id in the metadata to Stripe's
PaymentIntent objects, we no longer need to retrieve the user via
delivery_email. It makes more sense to just fetch the user by ID
and then get the latest delivery_email directly.
Note that an update to Stripe's fixtures is not necessary here
since a previous commit already modified the metadata passed to
both stripe.Session/PaymentIntent objects.
Previously, our Stripe webhook event handler code retrieved the
user's email from Stripe using the stripe.Customer.email attribute.
This led to situations such that whenever the email that Stripe had
did not correspond to a UserProfile in Zulip, the payment flow
failed since we couldn't find a UserProfile associated with the
given email.
Now, we pass in the UserProfile.id in the metadata to Stripe's
checkout Session object, so that we can fetch the correct email
in future Stripe requests.
It is confusing to have the plan type constants not be namespaced
by the thing they represent. We already have a namespacing
convention in place for constants, so we should use it for
Realm.plan_type as well.
compute_plan_parameters assumed that the value of
tier is always CustomerPlan.STANDARD. We change that behavior
to make the function handle CustomerPlan.PLUS as well.
This upgrades the Stripe API to the most recent version. Going through
the Git history, it looks like our current API version is at 2019-03-14.
The API version should be manually changed in Stripe dashboard at the same
time as the commit is deployed in production.
Backward incompatible changes that are relevant to our codebase between
(2019-03-14, 2020-08-27].
* 2020-08-27 - The `sources` property on Customers is no longer included by
default.
* 2020-03-02 - Nothing applicable
* 2019-12-03 - The `id` field of all invoice line items have changed and are
now prefixed
with `il_`. We only rely on this while we normalize the fixtures.
* 2019-11-05 - Nothing applicable
* 2019-10-17 - The `billing` attribute on invoices, subscriptions, and
subscription schedules is renamed to`collection_method`. The invoice
change is the one that is relevant to us.
* The customer object’s `account_balance` value has been renamed to
`balance`. Only used for the stubs at the moment.
* 2019-10-08 - Nothing applicable
* 2019-09-09 - Nothing applicable
* 2019-08-14 - Nothing applicable
* 2019-05-16 - Nothing applicable
https://stripe.com/docs/upgrades
Also normalize the following IDs in stripe fixtures
* price_[A-Za-z0-9]{24}
* prod_[A-Za-z0-9]{14}
* pi_[A-Za-z0-9]{24}
* il_[A-Za-z0-9]{24}
Previously, we only downgraded and voided small organizations behind
payments only if they had an active plan.
This left us with a bunch of invoices from small realms which used to
have an active plan. It doesn't make much sense for us to get these
realms to pay the invoices so we have decided to just void them. This
commit voids the open invoices of all the small realms without an active
plan and has the last invoice open. Unlike, the realms with an active
plan we don't email them about us voiding the invoice. It's not super
obvious whether Stripe sends an email to the customer when the Invoice
is voided. But they do get the message that the invoice is voided if
they try to pay the invoice through the hosted invoice page.
An additional check for whether customer.stripe_customer_id is
None is added to the function. That check was not really required before
since all the customers with a plan also have a valid value for
stripe_customer_id. So all the calls to stripe.Invoice.list would have
non None value for customer argument.
Even though that is the case, mypy should still have complained about
the possibility of customer.stripe_customer_id being None when passed to
stripe.Invoice.list as customer paramater since mypy don't know that
customers with a plan will always have a non empty value for
stripe_customer_id. Our stripe stubs expect a non empty value for
the customer parameter of stripe.Invoice.list. This is despite the
fact that stripe.Invoice.list can actually be called with customer set
to None. This returns the invoices from the entire organization.
Though, we still decided to ensure that the value of customer should be
non empty since there is no reason for us to ever call this function
with customer set to None. You can just call the function wuthout the
customer argument instead. So this requirement of a non None customer
paramater is useful for catching bugs.
The reason mypy didn't complain was because the type of
Customer.objects.all() is Any and not QuerySet[Customer]. So mypy has no
idea that customer.stripe_customer_id can be theoratically None even
though it was not possible in this [articular case as explained before.
I verified that this was the reason mypy didn't complain by using the
reveal_type function on Customer.objects.all() and the customer object.
After the refactoring it's super to obvious to mypy that the type of the
customer is Customer since it's mentioned in the function defintion. So it
was able to complain about the possibility of customer.stripe_customer_id
being None after the refactoring.
An organization with at most 5 users that is behind on payments isn't
worth spending time on investigating the situation.
For larger organizations, we likely want somewhat different logic that
at least does not void invoices.
stripe.Invoice.list by default would only get 10 invoices at a
time. So a function like this would be really handy if we have
to go through a lot of invoices.
This also means void_all_open_invoices used to void only the last
10 invoices. The main reason we implemented this function was to
void the invoices generated by realms on free trial so I don't
think there were cases where we had to void realms with more than
10 invoices.
This also fixes a bug in void_all_open_invoices function. If a realm
with a local Customer object but without an associated stripe.Customer
is passed to void_all_open_invoices, then the function will end up
voiding the last 10 invoices created by billing system instead of voiding
no invoices at all. This is because stripe.Invoice.list(customer=None)
return last 10 invoices across all customers.
But this bug won't cauuse any issue in production since
void_all_open_invoices can be only invoked from /support page. And we
show the option to void invoices in support page only if the realm
has a paid plan. And it's not really possible for a realm to have
a paid plan without having an associated stripe_customer_id. Plus I
went through the void events in stripe stream since the PR to add
void invoices was merged and there does not seems to be any suspicious
events.
Tim Abbott
Aman Agrawal
Lauryn Menard
Mateusz Mandera
Prakhar Pratyush
Anders Kaseorg
Zixuan James Li
Zixuan Li
Zixuan James Li
Eeshan Garg
Vishnu KS