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.

Sign up for GitHub

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

Jump to bottom

webhook subscription target URLs: Add textarea in project settings view #1030

Closed
wants to merge 1 commit into from
Closed

webhook subscription target URLs: Add textarea in project settings view #1030

wants to merge 1 commit into from
+93 −1

Conversation

jayanth-kumar-morem
Copy link
Contributor

@jayanth-kumar-morem jayanth-kumar-morem commented Dec 18, 2023 •
edited
Loading

webhooksubscriptions: Add textarea input for target URLs

  • Create a new form obj WebhookSubscriptionArea
  • Include WebhookSubscriptionArea in ProjectSettingsForm
  • Initial value is "\n" separated list of URLs retrieved using project.websubscriptions.all()
  • While submitting form
    • Check if websubscriptions input already is not None or ""
    • Delete all existing WebhookSubscription objects for the project
    • Create new WebhookSubscription objects for each URL in the input
    • Delete webhooksubscriptions key from config dict to avoid saving it in settings model

UI updates

Screen.Recording.2023-12-20.at.12.48.59.AM.mov

Fixes: #1027

@jayanth-kumar-morem jayanth-kumar-morem mentioned this pull request Dec 18, 2023
Closed
@tdruez
Copy link
Contributor

tdruez commented Dec 18, 2023

@jayanth-kumar-morem The WebhookSubscription is not related to the project inputs but rather to the Project itself.
The Webhook management does not belong to the "Inputs" related parts of the UI.

@jayanth-kumar-morem
Copy link
Contributor Author

Ack @tdruez

so, on create project page, I'll place it below the execute now check box
And on the specific project page, I'll add a modal beside, Add inputs and Add pipeline boxes

Will that be fine ?

@tdruez
Copy link
Contributor

tdruez commented Dec 18, 2023

@jayanth-kumar-morem I don't think we want to complexify the "project creation" on form with webhook management for now.
Also, a new box along inputs and pipelines on the projetc details view will not be ideal.
I think we should start with adding the Webhook management in the project settings view for now.

@jayanth-kumar-morem
webhooksubscriptions: Add textarea input for target URLs
1f7c141 
- Create a new form obj `WebhookSubscriptionArea`
- Include `WebhookSubscriptionArea` in `ProjectSettingsForm`
- Initial value is "\n" separated list of URLs retrieved using `project.websubscriptions.all()`
- While submitting form
  - Check if `websubscriptions` input already is not `None` or `""`
  - Delete all existing `WebhookSubscription` objects for the project
  - Create new `WebhookSubscription` objects for each URL in the input
  - Delete `webhooksubscriptions` key from `config` dict to avoid saving it in `settings` model

Fixes: aboutcode-org#1027
Signed-off-by: Jayanth Kumar <jknani111@gmail.com>
@jayanth-kumar-morem jayanth-kumar-morem changed the title webhook_subscription: Add input box webhook subscription target URLs: Add textarea in project settings view Dec 19, 2023
@jayanth-kumar-morem
Copy link
Contributor Author

@tdruez Requesting for code review

tdruez
tdruez requested changes Jan 30, 2024
View reviewed changes
scanpipe/forms.py
if "webhooksubscriptions" in config.keys() and config[
"webhooksubscriptions"
] not in [None, ""]:
project.webhooksubscriptions.all().delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a major caveat in this implementation.
Each time the webhook field is modified, all the WebhookSubscription instances, even the ones that have been sent are removed.
We are losing valuable data history.

@tdruez
Copy link
Contributor

tdruez commented Aug 2, 2024

The Webhook system and models were enhanced in #1338
Closing.

@tdruez tdruez closed this Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tdruez tdruez

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document and promote webhook usage
2 participants
@jayanth-kumar-morem @tdruez