This makes it much more clear that this feature does JSON encoding,
which previously was only indicated in the documentation.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Previously, the GitLab webhook code, namely the `get_objects_assignee`
method first tried to get a single assignee and if that failed then it
looks for multiple assignees and then it would return the first
assignee that it found (there's actually a code smell here - a loop
which would always return on the first iteration).
Instead, this commit will change that behavior to first check for
multiple assignees first then for a single assignee if we can't find
multiple assignees. Ultimately it will return a list of all of the
assignees (however many that might be [0, n]). This method has then
aptly been renamed to `get_assignees`.
Finally, we tweked the code using this method to always use it's
output as an "assignees" parameter to templates (there's also an
assignee parameter which we want to avoid here for consistency).
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
Any exception is an "unexpected event", which means talking about
having an "unexpected event logger" or "unexpected event exception" is
confusing. As the error message in `exceptions.py` already explains,
this is about an _unsupported_ event type.
This also switches the path that these exceptions are written to,
accordingly.
8e10ab282a moved UnexpectedWebhookEventType into
`zerver.lib.exceptions`, but left the import into
`zserver.lib.webhooks.common` so that webhooks could continue to
import the exception from there.
This clutters things and adds complexity; there is no compelling
reason that the exception's source of truth should not move alongside
all other exceptions.
Since the title of a merge request can often change, it shouldn't be a
part of the topic that we send the message to. Otherwise things would
get messy and confusing.
But at the same time we don't want to make this mandatory. So we add
a new boolean GET parameter that can toggle whether or not the topic
should include the MR title (`use_merge_request_title`).
Fixes#15951.
Signed-off-by: Hemanth V. Alluri <hdrive1999@gmail.com>
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>
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>
Apparently, the change and test I added before didn't quite cover the
corner case that was broken. This does, and exposes a second bug as
well, which we fix.
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>
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.
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.
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.
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.
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.
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.
GitLab recently changed their event name for build notifications
from "Build Hook" to "Job Hook". Instead of just supporting the
latter, we should support both just in case people are running
older versions of GitLab.
At some point, GitLab decided to change the name of the event for
CI notifications from "Build Hook" to "Job Hook" and we started
running into errors in webhook-logger.log.
Such payloads are generated when a GitLab repository has merge
request approvals enabled and a project member approves a merge
request. Approving is not the same as merging.
Payloads that don't have a payload['object_attributes']['action']
attribute are generated when GitLab sends a test payload to verify
if the webhook was set up successfully. In this case, we should
send a message notifying that the webhook was configured
successfully.
PIG208
Anders Kaseorg
Hemanth V. Alluri
Alex Vandiver
Anders Kaseorg
Johannes Hochreiter
Tim Abbott
pemontto
Tim Abbott
Anders Kaseorg
Eeshan Garg
Steve Howell
Aditya Bansal
Xavier Cooney