Remove code duplication on temp mod role revocation failure

[Zabuzard]

Overview

Sonar correctly flags this as code duplication:

We should unify this section. Maybe move it into ModerationUtils as a helper like "handle temporary moderation role revocation failure" or similar.

It is currently causing our quality gate to fail (since I made some adjustments on its strictness)

[Tais993]

I honestly disagree, just like you used to, see here.

They aren't the same thing, and nothing is wrong with handling them differently.
I rather see a way to suppress them than "fix" them.

[Zabuzard]

@Tais993 Suppress is the definitely wrong action, instead it should be acknownledged as wont-fix in sonar itself.

That said, I definitely agree to wont-fix on the other duplicate it flagged (the arg extraction of the command options). But for this one I genuinely agree with Sonar. They are identical and will stay identical. This is the failure handle for removing a temporary mod revocation role from a member.

In particular, if we introduce another temp mod action that is based on roles, it will get the identical failure handle -> it should be one place and not duplicated.

[interacsion]

I looked a bit into it, and if we want to move the method into ModerationUtils we'll have to make RevocableModerationAction public. another alternative is to make the method a static method of RevocableModerationAction.

[Tais993]

another alternative is to make the method a static method of RevocableModerationAction.

Doesn't sound like a good idea, it probably makes a lot more sense to become an utility emthod in ModerationUtils

[interacsion]

I agree, it sounds a bit sketchy, I just wanted to point it out :) the thing is, this logic is (and will probably stay) exclusive to RevocableModerationAction's implementations, as far as I can see.
so maybe putting it in a more specific scope is the right idea.

[Zabuzard]

Should probably end up as utiltiy in the temp mod package thing.

[interacsion]

In a completely new class?

[Zabuzard]

Sure, why not. Or just make it public in the moderation util. I dont really see a big issue here. Its a bit unfortunate that there is no concept of "subpackage visibility" but it is what it is. The ModUtils helper is only really meaningful for mod stuff anyways and there are already classes outside of the package using it, no? Doesnt really hurt if the subpackage mod/temp also starts using it, imo.

As in, adding a void handleTempModRoleRevocationFailure(Throwable failure) to ModerationUtils and then giving that as method reference: queue(..., ModerationUtils::handleTempModRoleRevocationFailure) or similar.

[interacsion]

only problem I see with that is that TermporaryBanAction has a slightly different logic than TemporaryMuteAction and TermoraryQuarantineAction

[Zabuzard]

Thats because it is not a "handleTempModRoleRevocationFailure" (emphasis on role for role-based). Hence it is also not a duplicate. I havent checked its failure handler, is it that similar? I didnt really intend to unify it here as well.

[interacsion]

the thing is, I got a bit confused about the method reference, as all of the three actions share one queue() call.

[interacsion]

I'll look into it.