By default, the Django Sentry integration provides the email address
and username of pulled from the auth layer. This is potentially PII,
and not data that we wish to store. Enable sending user data at all,
by setting `send_default_pii=True`, but strip the username and
email (which are the same, in Zulip) before sending. Users will be
identified in Sentry only by their IP address, user ID, realm, and
role.
The return type of `ugettext_lazy('...')` (aliased as `_`) is a
promise, which is only forced into a string when it is dealt with in
string context. This `django.utils.functional.lazy.__proxy__` object
is not entirely transparent, however -- it cannot be serialized by
`orjson`, and `isinstance(x, str) == False`, which can lead to
surprising action-at-a-distance.
In the two places which will serialize the role value (either into
Zulip's own error reporting queue, or Sentry's), force the return
value. Failure to do this results in errors being dropped
mostly-silently, as they cannot be serialized and enqueued by the
error reporter logger, which has no recourse but to just log a
warning; see previous commit.
When we do this forcing, explicitly override the language to be the
realm default. Failure to provide this override would translate the
role into the role in the language of the _request_, yielding varying
results.
AdminNotifyHandler is used to notify admins of errors; it is a
critical piece of logic. Failures in reporting errors will compound,
since its `except Exception` clauses cannot generate logging at the
`error` or `exception` level, as that would be recursive. It must
settle for logging at the `warning` level, and hope that admins are
vigilant to the logging there.
Increase the chances of being notified of failures in this logger, by
bubbling up those exceptions to Sentry, which is an orthogonal
reporting stack.
When user requests for a realm that doesn't exists, we raise
a InvalidSubdomainError.
This reduces our effort at repeatedly ensuring realm is valid
in request in web-public queries.
If there are unsupported keys, we still log an error,
but we now also send a message to the stream. (This
is a good tradeoff for the github webhook, since users
can just turn off notifications if they find it spammy.
Also, we intend to support "repository" soon.)
This is a bit of an experiment to see how this plays
in the field:
* will customers notice the change?
* will Sentry reports look any different?
The main thing fixed here is that we weren't turning
on our keys into a list. And then I refined the message
a bit more, including sorting the keys.
I also avoid the unnecessary "else".
The EVENT_FUNCTION_MAPPER maps a string event name
to a function handler. Before this we circumvented
mypy checks with a call to get_body_function_based_on_type,
which specified Any as the type of our event function.
Now the types are rigorous.
This change was impossible without the recent commit
to introduce the Helper class.
The Helper class will soon grow, but the immediate
problem it solves is the need to jankily inspect
the parameters of our get_*_body function.
Most of the changes were handled by an ad hoc
munge.py script.
The substantive changes were adding the Helper
class and passing it in.
And then the linter discovered a place where
the optional include_title parameter wasn't used
(which is one of the reasons to avoid the janky
inspect-signature technique).
As a side note, none of the include_title parameters
needed a default value of False, as we always passed
in an explicit value.
We test cover both sides of include_title, which
you can verify by hard coding it to either True or
False (and seeing the relevant failures), although I
suspect most individual codepaths
only test one value, based on whether "topic" is in
the fixture or not.
Finally, I know Helper is not a great name, but I
intend to evolve the class a bit before deciding
whether a more descriptive name is helpful here.
(For example, an upcoming commit will add a
log_unexpected helper method.)
We get the header_event one level up the call
stack now, too.
It's somewhat annoying that we have our own
concept of "event" here, instead of just returning
our event handlers directly, or just calling them
directly, but it's a bit non-trivial to fix that
right away.
In passing, I remove the strange OR for "ping",
which is already a key in EVENT_FUNCTION_MAPPER.
See https://github.com/zulip/zulip/issues/16258 for
possible follow up here.
We now ignore the following two new pull_request
actions (as well as the three existing ones
from before):
approved
converted_to_draft
As the issue above indicates, we may want to actually
support "approved" if we can find somebody to work
on the webhook. (And then the issue goes a little
broader than what changed here.)
We consolidate the tests and remove the fixtures, which
just have a lot of noisy fields that we ignore. Also,
pull_request__request_review_removed was named improperly.
Before this the only way we took advantage
of the summary from UnexpectedWebhookEventType
was by looking at exc_info().
Now we just explicitly add it to the log
message, which also sets us up to call
log_exception_to_webhook_logger directly
with some sort of "summary" info
when we don't actually want a real
exception (for example, we might want to
report anomalous webhook data but still
continue the transaction).
A minor change in passing is that I move
the payload parameter lexically.
It was broken by commit aaedec1fdb which
moved it to tools/i18n without adjusting its relative path references,
and it contains a sketchy injectable os.system call that I’d like to
remove.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
In 6653e19e3a we added
the convenient line to tell folks about the coverage
report. But if we failed coverage checks, we didn't
show the link.
Arguably we should just always show this, even if
tests fail, but that can also be potentially confusing.
The code to run single files was added
in c15695e514,
and it's just kinda strange code.
We already do a lot of file logic in Python
to check for line-coverage, so it's easier
to just have all the logic in Python.
This adds a new feature--you can now specify
the actual file:
./tools/test-js-with-node frontend_tests/node_tests/people.js
(This is helpful if you just want to use
shell autocomplete.)
Another minor change is that if you specify
individual files, we won't sort them. This is
important when you're trying to hunt down test
leaks.
Finally, we have a nicer message if we can't find
the file.
nyc was added in 29f04511c0
All the stuff after "&&" was actually passed to
node, because we didn't use shell=True, so the
"nyc report" command didn't run, and the ugly
finder.js code just skipped over all the final tokens.
Alex Vandiver
Anders Kaseorg
Aman Agrawal
Steve Howell