Delete trailing newlines from all files, except
tools/ci/success-http-headers.txt and tools/setup/dev-motd, where they
are significant, and static/third, where we want to stay close to
upstream.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Previous cleanups (mostly the removals of Python __future__ imports)
were done in a way that introduced leading newlines. Delete leading
newlines from all files, except static/assets/zulip-emoji/NOTICE,
which is a verbatim copy of the Apache 2.0 license.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
In the rare case that Zulip receives an email with only an HTML
format, we originally (code dating to 2013) shelled out to
html2markdown/python-html2text in order to convert the HTML into
markdown.
We long since added html2text as a reasonably managed Python
dependency of Zulip; we should just use it here.
This reverts commit f476ec7fac (#10312)
and replaces it with a proper fix using Jinja2 raw blocks.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
For storing HTTP headers as a function of fixture name, previously
we required that the fixture_to_headers method should reside in a
separate module called headers.py.
However, as in many cases, this method will only take a few lines,
we decided to move this function into the view.py file of the
integration instead of requiring a whole new file called headers.py
This commit introduces the small change in the system architecture,
migrates the GitHub integration, and updates the docs accordingly.
In the GitHub integration we established that for many integrations,
we can directly map the fixture filename to the set of required
headers and by following a simple naming convention we can greatly
ease the logic involved in fixture_to_headers method required .
So to prevent the need for duplicating the logic used by the GitHub
integration, we created a method called `get_http_headers_from_filename`
which will take the name of the HTTP header (key) and then return a
corresponding method (in a decorator-like fashion) which could then be
equated to fixture_to_headers in headers.py.
The GitHub integration was modified to use this method and the docs
were updated to suggest using this when possible.
If the event key is None, the handler content_func never gets
defined, which leads to an UnboundLocalError. This can be easily
avoided by having a dedicated function that handles the case for
when the event key is None.
If the invoice was paid then the message should simply be
"Invoice is now paid." with a link to the invoice.
Also, suppress the "status_transitions" and "payment_intent"events.
The payload for when a build is cancelled was causing an error
because the build result code mapping was missing one of the
codes. This commit also fixes a minor typo in the result codes.
This commit modifies the regex used when parsing JIRA's full links of
the form `[text|link]` so that if you have two in a message, Zulip
markup conversion doesn't think that the first link extends to the
closing `]` of the second link.
This commit also removes the conditional for when a build status
does not have a corresponding emoji. In such a case, it is better
to have no emoji than displaying some boilerplate text about no
appropriate emoji being available.
comment_created payloads may not contain the required issue data
to format a useful notification, therefore, it is better to handle
issue comments through issue_updated events (which we already do).
Fixes: #11995.
The github-services model for how GitHub would send requests to this
legacy integration is no longer available since earlier in 2019.
Removing this integration also allows us to finally remove
authenticated_api_view, the legacy authentication model from 2013 that
had been used for this integration (and other features long since
upgraded).
A few functions that were used by the Beanstalk webhook are moved into
that webhook's implementation directly.
The initial goal was to improve message formatting and punctuation
but after a closer look, I realized that a larger refactor was
worth it for clarity and redability.
Certain payloads for account updates do not include the
previous_attributes that allow us to figure out what was actually
updated. So, we just ignore just payloads.
According to GitHub's webhook docs, the scope of a membership
event can only be limited to 'teams', which holds true when a
new member is added to a team. However, we just found a payload
in our logs that indicates that when a user is removed from a
team, the scope of the membership is erroneously set to
'organization', not 'team'. This is most likely a bug on
GitHub's end because such behaviour is a direct violation of
their webhook API event specifications. We account for this
by restricting membership events to teams explicitly, at least
till GitHub's docs suggest otherwise.
This change was prompted by a possible bug on Clubhouse's end. In
general, if a branch is added, it also prompts a workflow state
transition in its primary story.
However, our webhook error logs show an erroneous payload where
the same branch is added to another story, but there is no
workflow state transition. Also, both the stories are grouped in
the same payload, which confused/broke our code. Ideally, this
shouldn't happen and is most likely a bug on Clubhouse's end.
In most cases, changes included in Clubhouse payloads never
pertain to more than one parent entity (stories) simultaneously,
and we usually operate under the assumption that the changes
included therein are related to each other in terms of their
parent object (story or epic) and not a child object (the GitHub
branch).
A check suite is a collection of check runs. We care a lot more
about the outcomes of check runs in this case because check_run
payloads are a lot more informative than check_suite payloads.
(And in any case, the check_suite events are primarily for notifying
tools like CI to run checks).
We only support notifications for events where a check run has
completed. Notifications for when a check run has been queued or
is in progress are not very informative and may be too noisy.
The payloads for this event are missing some important details
about the Project's changes, such as the name of the project,
the card's column name, etc. Without such details, the resultant
notifications would not be useful at all!
The bot's details (such as ID and name) are used to format a
connection label in Zapier's UI. This allows users to distinguish
between different Zapier-to-Zulip connections set up with different
bots.
The original commit made a number of well-meaning but suboptimal product
changes to the invoice events, such as threading them under the invoice id
rather than the customer id. It also seems to be causing 500s for
invoiceitem events, though I'm not sure why.
This reverts commit 65489b0391.
Apparently, Clubhouse has two different payloads for story label
changes, one where the label name lives inside the "references"
object, and the other where it lives inside the "actions" object.
Their webhook API is still in beta, so this could just be a bug.
Anyhow, we should support both.
For unsupported or invalid payloads, we should just raise the
UnexpectedWebhookEventType exception and let our logging system
take care of recording the payload that caused the error.
This commit improves a couple of things:
* All of the message templates are now at the top, a convention
we follow in a lot of our webhooks.
* Messages are not prefixed with any emojis. We don't do this in
any of our other webhooks. Plus, the emojis were outdated.
* Remove some superfluous code.
* Use ```quote <quote goes here> ``` style formatting for
quoted text instead of the `>` character.
This commit only adds support for the four events that have sample
payloads provided for them on the Pagerduty developer website.
Support for the remaining events will be added in subsequent
commits, as we get access to more sample payloads.
We still create a Python 2 virtualenv for thumbor but that’s
separate (/srv/zulip-thumbor-venv from
scripts/lib/create-thumbor-venv).
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
This section is largely unnecessary, doesn't convey any useful
information, and is probably a remnant from an older version of
this doc that we forgot to remove.
A key part of this is the new helper, get_user_by_delivery_email. Its
verbose name is important for clarity; it should help avoid blind
copy-pasting of get_user (which we'll also want to rename).
Unfortunately, it requires detailed understanding of the context to
figure out which one to use; each is used in about half of call sites.
Another important note is that this PR doesn't migrate get_user calls
in the tests except where not doing so would cause the tests to fail.
This probably deserves a follow-up refactor to avoid bugs here.
There are (at least) two types of objects that could be sent with a
charge.succeeded event, a Charge (e.g. for credit cards) or a Payment (if
they pay by ACH). We were handling the first but not the second.
This commit also updates the fixture for the existing charge.succeeded event
to the latest API version.
Recent changes merged in #10877 didn't handle these events
correctly. The linkified_id function breaks for the `discount`
object in the JSON payload. A cursory glance at Stripe's docs
tells me that since a discount is associated with a customer
or a coupon, it makes sense for a `discount` object to not have
an ID that can necessarily be linked to. So, we can just link
to the associated coupon instead.
There are only a handful of non-JSON webhooks that wouldn't
benefit from the notify_bot_owner_on_invalid_json feature.
Specifically, these are the webhooks where the third-party product
uses another format, whether it be HTML form-encoded, XML, or
something else.
Tweaked by tabbott to correc the list of excluded webhooks.
Previously, the Stripe webhook code was riddled with implicit
assertions that there were exactly N event types within a given
category, and we handled the last one in a final `else` clause in the
block. This was likely to cause confusing problems in the event that
we're missing an event type (or Stripe adds a new one).
We fix this by just adding a few more conditionals and raising the
standard "unexpected event type" exception for the others.
A recent change to check_send_webhook_message allows webhooks to
unescape stream names before sending a message. This commit adds
a test for the edge case where the webhook URL is escaped twice by
a third-party.
Recently, one of our users reported that a JIRA webhook was not
able to send messages to a stream with a space character in its
name. Turns out that JIRA does something weird with webhook URLs,
such that escaped space characters (%20) are escaped again, so
that when the request gets to Zulip, the double escaped %20 is
evaluated as the literal characters `%20`, and not as a space.
We fix this by unescaping the stream name on our end before
sending the message forward!
This fixes the fact that these emoji were sometimes not displaying
properly (because of changes in the emoji names used in the codebase),
while also making this integration more standard (since it was the
only one with such an aggressive use of emoji).
We recently received a bug report that implied that for certain
payloads, the `requested_reviewers` key was empty whereas a
singular `requested_reviewer` key containing one reviewer's
information was present in its stead. Naturally, this raised
some not so pretty IndexError exceptions.
After some investigation and generating a few similar payloads,
I discovered that in every case both the `requested_reviewers`
and the `requested_reviewer` keys were correctly populated, so I
had to manually edit the payload to reproduce the error on my end.
My guess is that this anomaly goes back to when GitHub's reviewer
request feature was new and didn't support requesting multiple
reviewers, and that the singular `requested_reviewer` key could
possibly just be there for backwards compatibility or might just
be mere oversight. Either way, the solution here is to look for the
plural `requested_reviewers` key, and if that is empty, fall back
to the singular `requested_reviewer` key.
This commit adds a test for the payload that is generated when
a Task is moved from one user story to another on Taiga's Sprint
Taskboard UI.
This commit also gets up this webhook's test coverage up to 100%.
I generated multiple payloads and verified that there are no
`change` event payloads that will not contain the values in
question, so it is useless to catch these KeyErrors. If there are
any anomalies still, it is better to be notified about them than
to silently ignore them.
IFTTT allows custom templating for their payloads, so the onus is
on the user to ensure that their custom templates conform to the
expectations outlined in our IFTTT webhook docs. For that reason,
these payloads weren't generated, but were manually edited.
After discovering a couple of bugs, I decided to thoroughly test
and rewrite this integration from scratch. The older code wasn't
generating coherent messages.
This also commit gets this integration up to 100% test coverage.
Test coverage was improved by removing an unused function and
removing some code (written by me) that was actually handling
Test Hook event types incorrectly.
It was a painful amount of work to generate the actual payload.
Since the only difference was a small build URL, I manually
edited the payload and used that for testing.
This commit gets our GitHub webhook up to 100% test coverage.
Some of the page build message code had insufficient test coverage.
I looked at generating the payloads that would allow me to test
the lines of code in question, but it was too much work to
generate the payloads and this seemed like a vague event anyway.
So I just rewrote the logic so that the lines missing
coverage are implicitly covered.
This is a part of our efforts to get this webhook's coverage
up to 100%.
Note that apart from just testing an uncovered line of code, this
commit also fixes a minor bug in the code for messages about issue
comment deletion and editing.
Note that Freshdesk allows custom templating for outgoing payloads
in their webhook UI. Therefore, the payloads added in this commit
did not have to be official payloads from Freshdesk.
Instead of just referring to the commit with the raw URL, we
should use the commit ID as the text of the hyperlink.
Note that in commit_status_changed type messages, the name of the
commit isn't available.
The function that generates the body of the commit_status_changed
event messages generated an invalid commit URL.
Most likely, we missed this because this event type is fairly
vague and it is possible it was never tested by users much,
if at all.
The lack of coverage was due to:
* An unused function that was never used anywhere.
* get_commit_status_changed_body was using a regex where it didn't
really need to use one. And there was an if statement that
assumed that the payload might NOT contain the URL to the commit.
However, I checked the payload and there shouldn't be any instances
where a commit event is generated but there is no URL to the commit.
* get_push_tag_body had an `else` condition that really can't happen
in any payload. I verified this by checking the BitBucket webhook
docs.
We shouldn't just ignore exceptions when encoding the incoming
auth credentials. Even if the incoming credentials are properly
encoded, it is better to know when that is the case or if
something else fails.
The TeamCity webhook plugin supports multiple payload formats that
are customized to be used by different services such as Slack,
Flowdock, etc. We don't support such payloads, so we should ignore
them and stick to parsing only the generic ones. We should also
notify that bot owner about the error.
Now reading API keys from a user is done with the get_api_key wrapper
method, rather than directly fetching it from the user object.
Also, every place where an action should be done for each API key is now
using get_all_api_keys. This method returns for the moment a single-item
list, containing the specified user's API key.
This commit is the first step towards allowing users have multiple API
keys.
This is a follow-up in response to Tim's comments on #9951.
In instances where all messages from a BitBucket integration are
grouped under one user specified topic (specified in the URL), we
should include the title of the PR in the message body, since
the availability of a user-specified topic precludes us from
including it in the topic itself (which was the default behaviour).
This is a follow-up in response to Tim's comments on #9951.
In instances where all messages from a Gogs integration are
grouped under one user specified topic (specified in the URL), we
should include the title of the PR in the message body, since
the availability of a user-specified topic precludes us from
including it in the topic itself (which was the default behaviour).
This is a follow-up in response to Tim's comments on #9951.
In instances where all messages from a GitHub integration are
grouped under one user specified topic (specified in the URL), we
should include the title of the issue/PR in the message body, since
the availability of a user-specified topic precludes us from
including it in the topic itself (which was the default behaviour).
This is a follow-up in response to Tim's comments on #9951.
In instances where all messages from a Gitlab integration are
grouped under one user specified topic (specified in the URL), we
should include the title of the issue/MR in the message body, since
the availability of a user-specified topic precludes us from
including it in the topic itself (which was the default behaviour).
We already include the issue title in the topic. But if one chooses
to group all gitlab notifications under one topic, the message body
is misleading in the sense that only the Issue ID and the description
are displayed, not the title, which isn't super helpful if the topic
doesn't tell you the title either.
I think we should err on the side of always including the title in
the main message body, which is what this commit does.
Fixes#9913.
The only changes visible at the AST level, checked using
https://github.com/asottile/astpretty, are
zerver/lib/test_fixtures.py:
'\x1b\\[(1|0)m' ↦ '\\x1b\\[(1|0)m'
'\\[[X| ]\\] (\\d+_.+)\n' ↦ '\\[[X| ]\\] (\\d+_.+)\\n'
which is fine because re treats '\\x1b' and '\\n' the same way as
'\x1b' and '\n'.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
This was technically a bug. For events that aren't unsupported
intentionally, the control should fall to the line that raises
UnknownWebhookEventType, and shouldn't be handled by anything else.
The events that are intentionally unsupported should be handled
more explicitly.
This is one of those weird webhooks where the
download-python-bindings.md macro doesn't work, because the user
only needs the bindings to run the one-time Trello script to register
the webhook and that script can be run from anywhere and doesn't need
to be "hosted" anywhere.
I spend a lot of time on this. One of our users had reported that
this webhook wasn't working at all. So I tested this with a local
ngrok instance and made sure that it was working. I also took this
opportunity to rewrite the docs for this, which were quite outdated.
With a few changes by Rishi Gupta!
This improves test coverage for a lot of our webhooks that relied
on ad-hoc methods to handle unexpected event types.
Note that I have deliberately skipped github_legacy, it isn't
advertised and is officially deprecated.
Also, I have refrained from making further changes to Trello, I
believe further improvements to test coverage should be covered
in separate per-webhook commits/PRs.
We ask our users to enable Snapshot notifications in Zulip via
Slack! But our Slack integration isn't exactly super robust and
I checked and our librato implementation isn't super smart about
handling snapshot payloads that come in via Slack.
Overall, this seems like a very poor solution, asking the user
to set up Slack in order to get the notifications in Zulip. So, I
thought we should get rid of at least the docs that suggest doing
this.
I also read librato/view.py and it wasn't clear to me how Slack
is supposed to act as an intermediate service here in a reliable
manner, which is another reason to not advertise this.
Epics are a way to further organize Pivotal Stories and are a
somewhat advanced feature that would take a significant amount of
work to properly implement. Unless we get requests for supporting
epics, I don't think we should support them.
We let Markdown increment the list step numbers, which is more
reliable than keeping track of numbered-steps manually.
Also, instead of linking to the CircleCI docs, we now have full
instructions for how to setup a webhook by modifying the circle.yml
file.
Ancient GitLab from several years ago doesn't include the
HTTP_X_GITLAB_EVENT header (and seems to have a different format), so
we should ignore its requests.
Might be good to document the version threshhold, but it's very hard
to tell from Googling what it is.
Rishi and I decided that it makes sense to get rid of the Facebook
integration for a few reasons, some of which are:
* The setup process is too complicated on Facebook's end. The users
will surely have to browse Facebook's huge API reference before even
having a vague idea of what they want.
* Slack chooses not to have a Facebook integration, but relies on
Zapier for it. Zaps that integrate with Facebook are much more
streamlined and the setup process isn't as much of a pain. Zapier's
Facebook Zaps are much more fine-tuned and there are different Zaps
for different parts of the FB API, a luxury that would likely span
2K+ lines of code on our end if we were to implement it from
scratch. So, I think we should relegate integration with Facebook to
Zapier as well!
* After thoroughly testing the setup process, we concluded that the
person who submitted the FB integration didn't really test it
thoroughly because there were some gaping holes in the docs (missing
steps, user permissions, etc.).
This one is one of the most tedious to set up and get working.
We now also rely on the Trello scripts available as part of the
`python-zulip-api/zulip` API package to make the setup process
easier.
This was a user-reported bug and a very subtle and painful one
to track down.
Previously, if payload['push']['changes'][i]['closed'] was True,
we assumed that a branch was removed. Looking at whether `closed`
was set to True or not was our way to tell whether a push removed
a branch or not.
However, this is wrong! `closed` being set to True can also mean
that the pull request associated with the branch was approved but
the branch itself was not deleted. According to the BitBucket docs,
the correct way to see if a branch is deleted is to check if `new`
is null.
This bug was leading to KeyErrors about not being able to find
the `commits` key, which shouldn't happen anymore!
webhook-errors.log file is cluttered with Stream.DoesNotExist
errors, which hides the errors that we actually need to see. So,
since check_message already sends the bot_owner a PM if the webhook
bot tries to send a message to a non-existent stream, we can ignore
such exceptions.
We filter out hidden comments out of Issue descriptions but this
breaks when description is null (which is unusual). So this commit
just checks to see if the description is None and if so, not to
filter anything out.
For a personal build, the teamcity webhook still sends a private
message using check_send_private_message since a personal build
should never trigger a public notification.
For a non-personal build, check_send_webhook_message is used,
which can either send a PM or a stream message based on whether
a stream is specified in the webhook URL or not.
We now only give users two options, to specify a stream and receive
public notifications for their goals, or to leave it out and receive
PMs and thus, keep their goals private. This simplifies the docs!
This fixes a regression in 93678e89cd
and a4979410f9, where the webhooks using
authenticated_rest_api_view were migrated to a new model that didn't
include setting a custom Client string for the webhook.
When restoring these webhooks' client strings, we also fix places
where the client string was not capitalized the same was as the
product's name.
This commit migrates all of our webhooks to use
check_send_webhook_message, except the following:
beeminder: Rishi wanted to wait on this one.
teamcity: This one is slightly more work.
yo: This one is PM-only. I am still trying to decide whether we
should have a force_private argument or something in
check_send_webhook_message.
facebook: No point in migrating this, will be removed as part of
#8433.
slack: Slightly more work too with the `channel_to_topics` feature.
Warrants a longer discussion.
These are the straightforward ones.
Note that there is a line in zerver.lib.test_classes.build_webhook_url
that lost test coverage. That's because most of our tests test using
stream messages so the webhook URLs being tested always have a query
parameter. So the line that accounts for there being no query
parameters never gets called, which is fine, but we should still
keep it.
This commit adds a generic function called check_send_webhook_message
that does the following:
* If a stream is specified in the webhook URL, it sends a stream
message, otherwise sends a PM to the owner of the bot.
* In the case of a stream message, if a custom topic is specified
in the webhook URL, it uses that topic as the subject of the
stream message.
Also, note that we need not test this anywhere except for the
helloworld webhook. Since helloworld is our default example for
webhooks, it is here to stay and it made sense that tests for a
generic function such as check_send_webhook_message be tested
with an actual generic webhook!
Fixes#8607.
Originally was going to centralize this in zerver/lib/request.pyi, but this
file is not visible at run-time, being only a stub. The matching request.py
file seemed inappropriate, as it doesn't actually use ViewFuncT.
Note that the "Save" button has no text in the Taiga webhook
setup UI. There is a small floppy disk symbol for saving which
is visible right beside the form fields. So I simply said,
"Save the form".
Rewritten in significant part by tabbott to actually be correct.
One particularly nasty thing the original webhook integration did is
do `current_time = time.time()` at the top of the `view.py` function
-- that means that code ran at import time, not runtime.
Brutus5000
Anders Kaseorg
Mateusz Mandera
Tim Abbott
Wyatt Hoodes
Hemanth V. Alluri
Eeshan Garg
Rishi Gupta
Vinit Singh
David Wood
Mayank Madan
sameerchoubey
Anders Kaseorg
Pragati Agrawal
Akshay Gupta
Steve Howell
Lenny Jagielski
Adam Birds
Yago González
MaTaM673
Aditya Bansal
Preston Hansen
neiljp (Neil Pilgrim)
Balaji2198
Ricky
Alena Volkova