Skip to content

Conversation

@gwenneg
Copy link
Member

@gwenneg gwenneg commented Sep 9, 2022

▶️ Part 2 of NOTIF-512

When a webhook is automatically disabled because of 4xx or 5xx failures, an action is emitted on the platform.notifications.ingress topic with the following information:

  • bundle: console
  • application: notifications
  • event type: integration-disabled

The action also contains additional information about the issue:

  • the endpoint ID and name
  • the type of the error (client or server)
  • the HTTP status in case of client error
  • the number of errors in case of server errors

Using a default behavior group, the event type described above will be used to send an email notification to the org admins of the account that owns the disabled integration. The email body will vary depending on the type of the error.

@gwenneg gwenneg force-pushed the NOTIF-512-notif branch 10 times, most recently from fa369fa to 4b21c83 Compare September 9, 2022 20:49
@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2022

Codecov Report

Base: 60.29% // Head: 60.47% // Increases project coverage by +0.17% 🎉

Coverage data is based on head (430f16a) compared to base (2d9fa3c).
Patch coverage: 80.64% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1482      +/-   ##
============================================
+ Coverage     60.29%   60.47%   +0.17%     
- Complexity     1083     1088       +5     
============================================
  Files           222      224       +2     
  Lines          5322     5369      +47     
  Branches        520      523       +3     
============================================
+ Hits           3209     3247      +38     
- Misses         1928     1937       +9     
  Partials        185      185              
Impacted Files Coverage Δ
.../notifications/templates/ConsoleNotifications.java 0.00% <0.00%> (ø)
...otifications/events/KafkaMessageWithIdBuilder.java 83.33% <83.33%> (ø)
...d/notifications/events/FromCamelHistoryFiller.java 85.45% <100.00%> (-1.22%) ⬇️
...ifications/events/IntegrationDisabledNotifier.java 100.00% <100.00%> (ø)
...ions/processors/webhooks/WebhookTypeProcessor.java 73.73% <100.00%> (+0.54%) ⬆️
...tions/templates/EmailTemplateMigrationService.java 93.71% <100.00%> (+0.09%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gwenneg gwenneg force-pushed the NOTIF-512-notif branch 7 times, most recently from c413414 to cc30ea2 Compare September 12, 2022 08:44
@gwenneg gwenneg changed the title NOTIF-512 Notify org admins when an endpoint is disabled NOTIF-512 Notify org admins when an integration is disabled Sep 12, 2022
@gwenneg gwenneg changed the title NOTIF-512 Notify org admins when an integration is disabled NOTIF-512 Notify org admins when a webhook is disabled Sep 12, 2022
@gwenneg gwenneg marked this pull request as ready for review September 12, 2022 08:58
@gwenneg gwenneg requested a review from josejulio September 12, 2022 08:58
@gwenneg
Copy link
Member Author

gwenneg commented Sep 13, 2022

Rebased to fix conflicts.

Copy link
Member

@josejulio josejulio left a comment

Choose a reason for hiding this comment

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

lgtm

Recipient recipients = new Recipient.RecipientBuilder()
.withOnlyAdmins(true)
.withIgnoreUserPreferences(true)
.build();
Copy link
Member

Choose a reason for hiding this comment

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

This could be removed in the future, once we have something to have defaults (e.g. all admins subscribed for these notifications and a default behavior group on the account that can be removed) so they can fine-tune this setting (maybe a particular RBAC group would be the one handling these errors and no the admin itself.

.withIgnoreUserPreferences(true)
.build();

Action action = new Action.ActionBuilder()
Copy link
Member

Choose a reason for hiding this comment

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

As a side note, the suggested builder syntax has been merged, but not deployed yet!

@gwenneg gwenneg merged commit ecba690 into RedHatInsights:master Sep 13, 2022
@gwenneg gwenneg deleted the NOTIF-512-notif branch September 13, 2022 19:44
FabriciaDinizRH pushed a commit to FabriciaDinizRH/notifications-backend that referenced this pull request Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants