Naval-Base / yuudachi

Discord moderation bot
https://yuudachi.dev
GNU Affero General Public License v3.0
282 stars 66 forks source link

feat(locks): member locks #1172

Open JPBM135 opened 1 year ago

JPBM135 commented 1 year ago

Why?

Duplicated actions have been a rare but occurrent problem, this PR uses Redis to create a one-time member lock to prevent more than one mod to action a member at the same time

Fixes #1149

Const

TODO:

Note ~While this logic has been implemented in anti-raid-nuke, I'm still not sure about it~ (Removed)

vercel[bot] commented 1 year ago

@JPBM135 is attempting to deploy a commit to the discordjs Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment | Name | Status | Preview | Comments | Updated (UTC) | | :--- | :----- | :------ | :------- | :------ | | **yuudachi-report** | ⬜️ Ignored ([Inspect](https://vercel.com/discordjs/yuudachi-report/7pN5AsXafRb9UiX4ycwm4kUnokzS)) | [Visit Preview](https://yuudachi-report-git-fork-jpbm135-feat-member-locks-discordjs.vercel.app) | | Sep 17, 2024 6:26am |
JPBM135 commented 1 year ago

This looks decent, but a couple points to consider:

  1. Can we move the locking into createCase? That's really where it matters, and might cut down on code duplication.
  2. I'd really like to see locking handled with a try...finally block to ensure the lock is properly disposed once the resource is no longer needed. Unfortunately JS doesn't have a good way to do this, so I'd recommend either just always using a try...finally block or passing a callback to the locking function with the code that requires the lock; callback is probably the cleanest and most explicit about the lock boundaries.

1- The thing about moving into createCase is, it does not solve possible case duplication, since a mod can be in the confirmation screen and another one triggering the action, since all the tests and validations are made prior to confirmation there is no current way to check this

2- I couldn't find a good way to do this either, but I will deff give another look into it

JPBM135 commented 1 year ago

2. I'd really like to see locking handled with a try...finally block to ensure the lock is properly disposed once the resource is no longer needed. Unfortunately JS doesn't have a good way to do this, so I'd recommend either just always using a try...finally block or passing a callback to the locking function with the code that requires the lock; callback is probably the cleanest and most explicit about the lock boundaries.

Found a solution for this, implemented the member lock in the command handler, only using extends when needed

appellation commented 1 year ago

Found a solution for this, implemented the member lock in the command handler, only using extends when needed

Neat that's pretty clean.