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

feat: increment fails on webhook 404 #851

Merged
merged 3 commits into from
Aug 11, 2023

Conversation

lenisko
Copy link
Contributor

@lenisko lenisko commented Jul 19, 2023

Description

Make use of human.fails and increment a value upon webhook 404.

Motivation and Context

Easier to remove deleted channels but staled webhooks

How Has This Been Tested?

Production.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@jfberry
Copy link
Collaborator

jfberry commented Jul 20, 2023

If this column (which is unused as now) is to be used, surely it should also increment on bot failures (eg user permission problems) ?
Not sure how I feel about pushing db access into the queue runner but can’t immediately think of a simple alternative

@lenisko
Copy link
Contributor Author

lenisko commented Jul 20, 2023

Wasn't use case handled the other way? I believe they are getting locked after abuse. But sure, I'll take a look later.

Yeah... couldn't think about a better solution in this codebase. I was 98% sure you will mention it ,)

@jfberry
Copy link
Collaborator

jfberry commented Jul 21, 2023

I think that exception is the right place, have you seen this work?

@lenisko
Copy link
Contributor Author

lenisko commented Jul 21, 2023

  • webhook
  • discord:channel
  • discord:user

Working for those.

@jfberry
Copy link
Collaborator

jfberry commented Jul 21, 2023

I imagine it would work. It is easily testable by setting DMs to friends only then they will fail for a user

@jfberry
Copy link
Collaborator

jfberry commented Jul 21, 2023

...this would be a great addition to !userlist as well to show fail numbers (although without it resetting it would only be an indicator of a past problem I guess)

@lenisko
Copy link
Contributor Author

lenisko commented Jul 21, 2023

Tested last case on myself.

I intended to keep it for backend cleanup, cases when you see a lot of errors in log and don't want to dig manually.

I could add it in !userlist but just as you mentioned, it wouldn't be per day/week or so, but overall counter. So.. not sure there's a point 🤔

@jfberry
Copy link
Collaborator

jfberry commented Jul 21, 2023

These can be in a future PR. Would be easy to add a last_fail_date, or zero at midnight or something to make it more relevant but for now it's a good indicator at least.
I was just thinking out loud rather than these things being blockers before merge

@lenisko
Copy link
Contributor Author

lenisko commented Jul 21, 2023

:) It's ready to go then

@lenisko lenisko force-pushed the fails_incr_webhook branch from ca89749 to d892547 Compare July 22, 2023 14:06
@lenisko
Copy link
Contributor Author

lenisko commented Aug 10, 2023

:poke: @jfberry

@jfberry jfberry merged commit e4ead88 into KartulUdus:develop Aug 11, 2023
@lenisko lenisko deleted the fails_incr_webhook branch August 11, 2023 08:37
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.

2 participants