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.
django.utils.translation.ugettext is a deprecated alias of
django.utils.translation.gettext as of Django 3.0, and will be removed
in Django 4.0.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Problems with the card itself should not be logged as errors -- while
perhaps notable in aggregate, they are not worthy of being logged to
Sentry, for instance.
Downgrade these to `info`; continue to log other problems at the
`error` level. This updates tests for this change, and in so doing
corrects a test that does not do its job, due to a missing
`reset_mock`.
There are three functional side effects:
• Correct an insignificant but mathematically offensive bias toward
repeated characters in generate_api_key introduced in commit
47b4283c4b4c70ecde4d3c8de871c90ee2506d87; its entropy is increased
from 190.52864 bits to 190.53428 bits.
• Use the base32 alphabet in confirmation.models.generate_key; its
entropy is reduced from 124.07820 bits to the documented 120 bits, but
now it uses 1 syscall instead of 24.
• Use the base32 alphabet in get_bigbluebutton_url; its entropy is
reduced from 51.69925 bits to 50 bits, but now it uses 1 syscall
instead of 10.
(The base32 alphabet is A-Z 2-7. We could probably replace all of
these with plain secrets.token_urlsafe, since I expect most callers
can handle the full urlsafe_b64 alphabet A-Z a-z 0-9 - _ without
problems.)
Signed-off-by: Anders Kaseorg <anders@zulip.com>
There seems to have been a confusion between two different uses of the
word “optional”:
• An optional parameter may be omitted and replaced with a default
value.
• An Optional type has None as a possible value.
Sometimes an optional parameter has a default value of None, or None
is otherwise a meaningful value to provide, in which case it makes
sense for the optional parameter to have an Optional type. But in
other cases, optional parameters should not have Optional type. Fix
them.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Fixes#2665.
Regenerated by tabbott with `lint --fix` after a rebase and change in
parameters.
Note from tabbott: In a few cases, this converts technical debt in the
form of unsorted imports into different technical debt in the form of
our largest files having very long, ugly import sequences at the
start. I expect this change will increase pressure for us to split
those files, which isn't a bad thing.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Generated by pyupgrade --py36-plus --keep-percent-format, but with the
NamedTuple changes reverted (see commit
ba7906a3c6, #15132).
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Only CustomerPlan.ACTIVE is being used in the code / in production, so don't
need a db migration to move the other CustomerPlan.status constants around.
Previously, when users got a "payment failed" email from Stripe (e.g. if
their card failed on renewal), they would enter in a new card on
/billing#payment-method, and wouldn't find out if the card worked till
Stripe retried the payment 4 days later.
This is a major rewrite of the billing system. It moves subscription
information off of stripe Subscriptions and into a local CustomerPlan
table.
To keep this manageable, it leaves several things unimplemented
(downgrading, etc), and a variety of other TODOs in the code. There are also
some known regressions, e.g. error-handling on /upgrade is broken.
A lot of the seemingly unrelated test fixture changes are because we're
removing a query to stripe in the upgrade path, in cases when the user's
realm has an existing Customer object.
This used to be a more likely codepath, before we introduced
Customer.has_billing_relationship. It is no longer literally impossible to
hit this race condition, so I'm not deleting the code, but it's unlikely
enough that it's not worth figuring out how to test it.
Already better tested by the upgrade and downgrade tests using mock_stripe.
Note that the line that was removed is actually not possible to reach, since
canceled subscriptions aren't shown on the Customer object.
Also fixes a bug in process_initial_upgrade. If you have a card on file
(e.g. from a previous subscription), and try to upgrade by billing by
invoice, neither the if nor the elif condition applies.
There is no attribute called plan_interval. The reason
this was missed by mypy is moost likely due to us using
Any instead of Subscription in extract_current_subscription.
This will make it easier to mock the calls in our new stripe mocking
framework. I believe the two forms are equivalent, assuming the Stripe
Python bindings aren't doing anything crazy. And if not, well hopefully our
new testing framework will catch it :).
[Idea originally from Vishnu KS.]
Mateusz Mandera
Zixuan James Li
Anders Kaseorg
Zixuan Li
Lauryn Menard
Zixuan James Li
Aman Agrawal
Eeshan Garg
Vishnu KS
Siddharth Asthana
Alex Vandiver
Anders Kaseorg
Stefan Weil
Tim Abbott
Rishi Gupta
Anders Kaseorg