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

Adds try/catch around notification failing with an inactive webhook for better user experience #16050

Merged
merged 4 commits into from
Jan 10, 2025

Conversation

Godmartinz
Copy link
Collaborator

@Godmartinz Godmartinz commented Jan 9, 2025

When checking in and checking out, if the webhook notification fires and there is an error, it now redirects back with an error message
image
image

image

Copy link

what-the-diff bot commented Jan 9, 2025

PR Summary

  • Enhancement of Code Readability
    The code's formatting has been improved, making it easier to read. This was done by adding spaces around parentheses in conditionals.

  • Upgraded Logging Mechanisms
    We've upgraded our logging techniques to be more suitable for error situations. Now we use Log::error rather than Log::debug.

  • In-depth Error Report for Webhook Notifications
    New detailed error messages have been added to identify issues with webhook notification failures more easily. These messages include information such as the webhook endpoint and event details.

  • Consistent Email Notifications
    We have refactored the process of sending out email notifications during check-outs and check-ins. This allows for consistent logging and error handling practices.

  • Better Error Handling
    A more streamlined error handling practice has been introduced for webhook notifications, more specifically during the check-out and check-in stages.

@Godmartinz Godmartinz requested review from marcusmoore and snipe and removed request for snipe January 9, 2025 21:00
Copy link
Owner

@snipe snipe left a comment

Choose a reason for hiding this comment

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

Can we make this a warning or info? The red error makes it look like the checkout might not have worked, even though it did. I'd say if you're going to fire two error bags, make one "success" and then the next one an info - or combine them and make it a warning.

@snipe
Copy link
Owner

snipe commented Jan 9, 2025

(I know what the messages say, but I also know what people will read.)

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

Gave this a try and it worked. Code is good but I do recommend @snipe's suggestion of changing it to a warning.

@Godmartinz
Copy link
Collaborator Author

forgot to make it a translation--1 sec

@Godmartinz
Copy link
Collaborator Author

good 👍

@snipe snipe merged commit 224f04d into snipe:develop Jan 10, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants