We ran into a bug in production caused by two issues:
- Some users came from orgs that didn't have a website and since
the URL field was required, they submitted invalid URLs.
- We didn't properly respond to invalid form submissions, which
led to UnboundLocalError exceptions in another part of the
code.
This commit solves this by doing the following:
- We now allow blank URLs and have a convenient placeholder text
label that tells users that they may leave the URL field blank.
- This commit refactors the code such that invalid form submissions
result in an informative error message about what exactly went
wrong.
These changes are all independent of each other; I just didn’t feel
like making dozens of commits for them.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
The locally defined NamedTuple was triggering a mypy caching bug
(https://github.com/python/mypy/issues/10913), and we don’t use the
tuple behavior anyway.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
When calling some functions or assigning values to certain attributes,
the arguments/right operand do not match the exact type that the
functions/attributes expect, and thus we fix that by converting types
beforehand.
This fixes a batch of mypy errors of the following format:
'Item "None" of "Optional[Something]" has no attribute "abc"
Since we have already been recklessly using these attritbutes
in the tests, adding assertions beforehand is justified presuming
that they oughtn't to be None.
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.
This avoid some duplicate code as well as improve the readability since
before we were checking for the expected values far away from the
definition of realm. Now we define the expected values right after the
realm definition which improves the code readability.
Also, this get removes the postfixing of realm variable names with numbers.
The postfixing is kind of mess since if we want add any new realm in between
the realms we need to renumber a lot of realm variables.
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.
This is a prep commit for the Stripe checkout migration.
The Stripe migration commit adds a lot of new view functions. Keeping
all of the views in one view file makes it super hard for readbability.
So creating a new views folder and splitting the existing view file into
two so that we minimize the changes in the big migration commit.
We are starting to run into situations where this data could be
quite useful for making future decisions, so it makes to store it
in the database, not just in an email.
This function had a confusing name, which could result in someone
using it unintentionally when they meant do_reactivate_user.
We also add docstrings for both functions.
This should have been in https://github.com/zulip/zulip/pull/18066.
The reason, the tests were not failing inspite of the params being
json encoded was because pretty much all these tests did not test
the functionality of the endpoint. Rather they were testing things
like whether the user has the right to access the endpoint and all.
So the value of the params did not matter.
The only one test which is an exception is test_replace_payment_source.
Even though a json encoded token was passed to an endpoint that
expecteda string, the test continued to work becausethe fixtures were
not updated for the test in that PR, so instead of sending an incorrect
json encoded token to stripe endpoint it was sending the correct string
token. Now that we removed the json.dumps of token, we no longer have to
update the fixtures.
I have run the tests with --generate-stripe-fixtures set to True and all
the tests are passing. Not including the fixture changes since the tests
conntinue to work the same with both the existing and new fixtures.
Billing system uses delivery_email instead of email. We used to make
the email address visible to everyone in tests which means the value
of email and delivery_email is the same. This commit disables that
so that we can distinguish between email and delivery_email in tests.
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.
This adds the is_user_active with the appropriate code for setting the
value correctly in the future. In the following commit a migration to
backfill the value for existing Subscriptions will be added.
To ensure correct user_profile.is_active handling also in tests, we
replace all direct .is_active mutation with calls to appropriate
functions.
This commit migrates some of the backend tests to use assertLogs(),
instead of mock.patch() as planned in #15331.
Tweaked by tabbott to avoid tautological assertions.
There were some tests that had mock patches for logging, although no
logging was actually happening there. This commit removes such patches
in `corporate/tests/test_stripe.py`, `zerver/tests/test_cache.py`,
`zerver/tests/test_queue_worker.py`,
and `zerver/tests/test_signup.py`.
Prefer using `assert_called_once` to protect against places where a
mock might be reused, and in so doing have been previously called,
thus making the second usage of `assert_called` not assert anything of
note.
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`.
A few major themes here:
- We remove short_name from UserProfile
and add the appropriate migration.
- We remove short_name from various
cache-related lists of fields.
- We allow import tools to continue to
write short_name to their export files,
and then we simply ignore the field
at import time.
- We change functions like do_create_user,
create_user_profile, etc.
- We keep short_name in the /json/bots
API. (It actually gets turned into
an email.)
- We don't modify our LDAP code much
here.
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>
Automatically generated by the following script, based on the output
of lint with flake8-comma:
import re
import sys
last_filename = None
last_row = None
lines = []
for msg in sys.stdin:
m = re.match(
r"\x1b\[35mflake8 \|\x1b\[0m \x1b\[1;31m(.+):(\d+):(\d+): (\w+)", msg
)
if m:
filename, row_str, col_str, err = m.groups()
row, col = int(row_str), int(col_str)
if filename == last_filename:
assert last_row != row
else:
if last_filename is not None:
with open(last_filename, "w") as f:
f.writelines(lines)
with open(filename) as f:
lines = f.readlines()
last_filename = filename
last_row = row
line = lines[row - 1]
if err in ["C812", "C815"]:
lines[row - 1] = line[: col - 1] + "," + line[col - 1 :]
elif err in ["C819"]:
assert line[col - 2] == ","
lines[row - 1] = line[: col - 2] + line[col - 1 :].lstrip(" ")
if last_filename is not None:
with open(last_filename, "w") as f:
f.writelines(lines)
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Anders Kaseorg
Eeshan Garg
Mateusz Mandera
PIG208
Vishnu KS
Tim Abbott
Abhijeet Prasad Bodas
Siddharth Asthana
shanukun
m-e-l-u-h-a-n
Alex Vandiver
Steve Howell
Anders Kaseorg