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>
Fixes this error in the dev environment:
$ ./manage.py checkconfig
Error: You must set ZULIP_ADMINISTRATOR in /etc/zulip/settings.py.
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>
datetime.timezone is available in Python ≥ 3.2. This also lets us
remove a pytz dependency from the PostgreSQL scripts.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
mock is just a backport of the standard library’s unittest.mock now.
The SAMLAuthBackendTest change is needed because
MagicMock.call_args.args wasn’t introduced until Python
3.8 (https://bugs.python.org/issue21269).
The PROVISION_VERSION bump is skipped because mock is still an
indirect dev requirement via moto.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This fixes a minor regression in a very recent
commit.
In 7ad5bea3e6 I was
a little too aggressive about deactivating users.
We do want a few users who are outside the realm,
just to prevent regressions where we fail to filter
on realm. The likelihood of such regressions are
fairly low, but it would certainly be an ugly bug.
Without this change, you could get obscure
failures when logging in as Cordelia if you
modified test data by doing something
fairly innocuous like adding a new test user.
Also the complicated query here to exclude
users was flaky, since it didn't explicitly
order by any field before doing the 'LIMIT 6'.
Part of the problem with debugging this flake
was that the failure would happen for the login,
but the data actually gets changed in `setUp,
which is easy to overlook, since it's not
explicitly invoked.
We continue to keep the seat_count set to
a constant, predictable value, since some
tests are very sensitive to having 6 users.
PIG208
Vishnu KS
Eeshan Garg
Tim Abbott
Abhijeet Prasad Bodas
Siddharth Asthana
shanukun
Mateusz Mandera
m-e-l-u-h-a-n
Alex Vandiver
Anders Kaseorg
Steve Howell
Anders Kaseorg