Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4657 Allow users to select the preferred webhook version #4689

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

albertisfu
Copy link
Contributor

As requested in #4657, this PR introduces changes in the webhooks UI, allowing users to select which version of the webhook to use when creating a webhook endpoint.

#4657 we planed to do:

Modify the Event Type drop down to list Seach Alert v1, Search Alert v2, etc

However, while working on this, I realized this approach could complicate code maintainability. Specifically, it would require adding new Event Type options each time a new webhook version is introduced, and adjusting code to handle multiple versions of the same event type when sending webhooks.

Instead, I propose we continue using the same event types and allow users to select the version independently.
Screenshot 2024-11-13 at 6 53 53 p m

With this change, users can create only one webhook per event_type and version combination. Since we currently have only two versions, users can create up to two webhook endpoints per event type.

The UI will display available versions based on the selected event type. If a user already has a webhook endpoint for a specific event type and version, only the remaining version will be shown for that event type:
Screenshot 2024-11-13 at 6 54 28 p m

Once created, the event type and version cannot be changed in the Edit view:
Screenshot 2024-11-13 at 6 55 35 p m

A new "Version" column has been added to the webhook endpoints list:
Screenshot 2024-11-13 at 6 55 19 p m

If a user has already created one endpoint for each event_type and version combination, an alert will appear when they try to create a new webhook:

Screenshot 2024-11-13 at 6 56 02 p m

For now, these changes are only in the UI. In #4653 and #4682, we will use the version to apply the correct serializer and version in the payload for each webhook event type. So my question is if when this is ready it's ok to merge or if we should put it behind a waffle to hide it temporarily, or wait until the other PRs are ready and merge them into this one to release everything as a unit?

@mlissner
Copy link
Member

or wait until the other PRs are ready and merge them into this one to release everything as a unit?

Seems easiest, right?

Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks about right. One little comment from me. Thank you Alberto!

cl/api/models.py Outdated
Comment on lines 20 to 21
V1 = 1, "V1"
V2 = 2, "V2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small thing, but I prefer v1 over V1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@mlissner
Copy link
Member

Process notes: I took this off your personal backlog, Alberto, since we're hoping not to use those anymore, and assigned it to you instead, since you're the one in charge of making sure it gets done. I added Eduardo as a reviewer.

Thanks all!

Copy link
Contributor

@ERosendo ERosendo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall! Here's a suggestion for improvement:

  • Let's consider placing the version dropdown before the enable checkbox in the user interface. This would make the workflow clearer for users.

    image

cl/users/api_views.py Outdated Show resolved Hide resolved
…k-version

4653 Use the selected webhook version in the webhook payload
Copy link

semgrep-app bot commented Nov 19, 2024

Semgrep found 8 direct-use-of-jinja2 findings:

Detected direct use of jinja2. If not done properly, this may bypass HTML escaping which opens up the application to cross-site scripting (XSS) vulnerabilities. Prefer using the Flask method 'render_template()' and templates with a '.html' extension in order to prevent XSS.

Ignore this finding from direct-use-of-jinja2.

@albertisfu
Copy link
Contributor Author

Thanks @ERosendo I've applied both of your suggestions. We'll merge #4705 into this one when it's ready, and then merge this one into main to deploy it to production.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

3 participants