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`.
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>
The exception trace only goes from where the exception was thrown up
to where the `logging.exception` call is; any context as to where
_that_ was called from is lost, unless `stack_info` is passed as well.
Having the stack is particularly useful for Sentry exceptions, which
gain the full stack trace.
Add `stack_info=True` on all `logging.exception` calls with a
non-trivial stack; we omit `wsgi.py`. Adjusts tests to match.
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.
These fixtures were added in 4aa2ac1b52.
The fixture name mentions renewal as the test function name. But we
don't have any function called test_renewal in test_stripe file. This
likely means the fixtures were accidentally added. Also, deleting all
fixtures and running --generate-stripe-fixtures don't result in these
fixtures getting generated as well.
If make_end_of_cycle_updates_if_needed returns None it means the current
plan has been ended. So we should change the value of variable plan_name
only if last_ledger_entry is not None.
Generated by `pyupgrade --py3-plus --keep-percent-format` on all our
Python code except `zthumbor` and `zulip-ec2-configure-interfaces`,
followed by manual indentation fixes.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This will otherwise throw error when the billing page is accessed
after a plan is expired. A proper followup would be to not show
licenses, licenses_used etc if the current plan is on Limited.
We try to use the correct variation of `email`
or `delivery_email`, even though in some
databases they are the same.
(To find the differences, I temporarily hacked
populate_db to use different values for email
and delivery_email, and reduced email visibility
in the zulip realm to admins only.)
In places where we want the "normal" realm
behavior of showing emails (and having `email`
be the same as `delivery_email`), we use
the new `reset_emails_in_zulip_realm` helper.
A couple random things:
- I fixed any error messages that were leaking
the wrong email
- a test that claimed to rely on the order
of emails no longer does (we sort user_ids
instead)
- we now use user_ids in some place where we used
to use emails
- for IRC mirrors I just punted and used
`reset_emails_in_zulip_realm` in most places
- for MIT-related tests, I didn't fix email
vs. delivery_email unless it was obvious
I also explicitly reset the realm to a "normal"
realm for a couple tests that I frankly just didn't
have the energy to debug. (Also, we do want some
coverage on the normal case, even though it is
"easier" for tests to pass if you mix up `email`
and `delivery_email`.)
In particular, I just reset data for the analytics
and corporate tests.
We now have this API...
If you really just need to log in
and not do anything with the actual
user:
self.login('hamlet')
If you're gonna use the user in the
rest of the test:
hamlet = self.example_user('hamlet')
self.login_user(hamlet)
If you are specifically testing
email/password logins (used only in 4 places):
self.login_by_email(email, password)
And for failures uses this (used twice):
self.assert_login_failure(email)
This specifically fixes an issue where a customer chose monthly payment
plan instead of annual, getting stuck with the higher monthly payments.
With this change, the Customer plan can be set to ENDED and they can go
through /upgrade/ again and choose annual payments.
A proper follow-up should be to make it possible to change between
monthly/annual on the /billing/ page.
Django 2.2.x is the next LTS release after Django 1.11.x; I expect
we'll be on it for a while, as Django 3.x won't have an LTS release
series out for a while.
Because of upstream API changes in Django, this commit includes
several changes beyond requirements and:
* urls: django.urls.resolvers.RegexURLPattern has been replaced by
django.urls.resolvers.URLPattern; affects OpenAPI code and related
features which re-parse Django's internals.
https://code.djangoproject.com/ticket/28593
* test_runner: Change number to suffix. Django changed the name in this
ticket: https://code.djangoproject.com/ticket/28578
* Delete now-unnecessary SameSite cookie code (it's now the default).
* forms: urlsafe_base64_encode returns string in Django 2.2.
https://docs.djangoproject.com/en/2.2/ref/utils/#django.utils.http.urlsafe_base64_encode
* upload: Django's File.size property replaces _get_size().
https://docs.djangoproject.com/en/2.2/_modules/django/core/files/base/
* process_queue: Migrate to new autoreload API.
* test_messages: Add an extra query caused by .refresh_from_db() losing
the .select_related() on the Realm object.
* session: Sync SessionHostDomainMiddleware with Django 2.2.
There's a lot more we can do to take advantage of the new release;
this is tracked in #11341.
Many changes by Tim Abbott, Umair Waheed, and Mateusz Mandera squashed
are squashed into this commit.
Fixes#10835.
This is a preparatory commit for using isort for sorting all of our
imports, merging changes to files where we can easily review the
changes as something we're happy with.
These are also files with relatively little active development, which
means we don't expect much merge conflict risk from these changes.
Then, find and fix a predictable number of previous misuses.
With a small change by tabbott to preserve backwards compatibility for
sending `yes` for the `forged` field.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
MigrationsTestCase is intentionally omitted from this, since migrations
tests are different in their nature and so whatever setUp()
ZulipTestCase may do in the future, MigrationsTestCase may not
necessarily want to replicate.
This sidesteps tricky escaping issues, and will make it easier to
build a strict Content-Security-Policy.
Signed-off-by: Anders Kaseorg <anders@zulipchat.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.
The fixture changes are because self.upgrade formerly used to cause a page load
of /billing, which in turn calls Customer.retrieve.
If we ran the full test suite with GENERATE_STRIPE_FIXTURES=True, we would
likely see several more Customer.retrieve.N.json's being deleted. But
keeping them there for now to keep the diff small.
f52e9d1 ended up not going far enough. Keeping f52e9d1 in place in case we
ever want to go back to that sort of solution.
Also removes the keep argument from test_billing_quantity_changes_end_to_end,
since that test is actually testing the arguments to
stripe.Subscription.save(), not what is returned by Stripe.
Reran every test with GENERATE_STRIPE_FIXTURES = True, which also caused a
few fixtures to get updates unrelated to these changes (likely due to API
updates that hadn't been previously applied).
When we started the billing system we started by following conventions used
in the Stripe documentation, but in hindsight it makes more sense to follow
conventions used in the Zulip codebase.
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.
Was hoping to do this after adding timestamp normalization to
normalize_fixture_data, which would have turned this into a <10 line
diff. There is a potentially material change in this API upgrade though
(around how invoices are handled), so just doing it now.
Without the cast mypy raises the following error:
Incompatible return value type (got "Callable[..., Any]",
expected "CallableT")
This is a known issue: https://github.com/python/mypy/issues/1927
This makes a few other changes to the fixtures as well. Most are from API
updates, though I'm not sure why "Zulip Cloud Standard" got changed to
"Zulip Cloud Premium".
[Substantial edits by Rishi Gupta]
This means you'll need access to our Stripe API key to add new fixtures.
Will be undone eventually, but having this in place will make it easier to
finish the mock.patch to mock_stripe migration.
The actual implementation of the change will be a cron job that runs once a
day and generates invoices for anyone with an account_balance > 0.
There are currently no tests for that part of the flow, so no tests had to
change.
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.]
This will improve both the maintainability and accuracy of the fixture
data. It also makes it less scary to upgrade Stripe API versions.
[With significant changes by Rishi Gupta.]
While we already don't link to /terms anywhere on the site, they can still be
accessed if you navigate to /terms directly. Now, those routes will only be
exported on the Zulip.com service.
We should ideally provide a mechanism for deployments to specify their own
terms without modifying source code; in the interim, sites that have already
customised the provided Zulip.com terms can simply carry a patch reverting this
commit.
The corporate "app" is not a full-fledged Django app, but it has
a urls.py and a templates directory. This commit creates the app
and moves the jobs pages into it. Localserver deployments will
not see any of the corporate code.
(imported from commit 35889c3cf92329258c30741fdfa564769a4fac1a)
shanukun
Alex Vandiver
Mateusz Mandera
Anders Kaseorg
m-e-l-u-h-a-n
Vishnu KS
Clara Dantas
Steve Howell
Anders Kaseorg
wowol
Stefan Weil
Steve Howell
Mateusz Mandera
rht
Tim Abbott
Rishi Gupta
Wyatt Hoodes
Greg Price
Anders Kaseorg
K.Kanakhin
Umair Khan
acrefoot
Luke Faraone
Steve Howell