Undo persistence

[MCMDEV]

Note: I made this PR mostly out of personal need and I likely don't have the time to add missing tests, fix code smells or make breaking API changes backwards compatible. I still wanted to open the PR in case someone else is interested in taking this and making it palatable for a public release.

This PR adds undo persistence. When a punishment is undone, the following information is stored:

• Undo Operator
• Undo Reason
• Undo Timestamp

A new table is added called libertybans_undone which stores relevant undo information, the simple history schema also includes undo information. There are currently breaking API changes, I made a reason and an undo operator a requirement for undoing a punishment. I'm not sure if it works completely, but I have not noticed any problems from unit/integration tests or ingame testing. I will still do more testing and fix broken functionality.

[MCMDEV]

I agree, breaking API shouldn't be done, so I un-broke the API and made the old methods all valid by method overloading with defaults, althought I deprecated the old methods. I think this is an easy low maintenance solution.

Where this might break is with the requirement for undo reasons not being required. They are not now, instead replaced with a default reason when calling the old (now deprecated) API. The unpunish command though simply uses the reason rules from the punish command, making a reason either required, substituting a default or using an empty reason. I think for command usage this is perfectly fine. Thoughts?

[MCMDEV]

Regarding the synchronization protocol: I didn't make any changes to it. From what I understand the synchronization protocol works like a push-to-pull message, sending other servers just the punishment id and type to retrieve the punishment from the database.

[A248]

The API design needs some work here. "Low maintenance" solutions are really high maintenance if not enough effort is put into the original API design. As I mentioned, I really like this PR, because it addresses a hole in the current feature set and opens up a lot of possibilities. However, that doesn't mean we can simply slap on undo information to the API.

That the undoing operator and reason are optional should be reflected in the API. The old methods do not need to be deprecated. Additionally, API users should be able to supply an undoing operator but not an undo reason. This will need to be reflected in the database schema too.

I also think that using the console operator is not an appropriate default. Users will have legacy punishments without undo information, and therefore we will need to accomodate a concept of an unknown undoing operator in the API. The relational design you have constructed is well architectured and lets us add undo information where it is available. There's no need to coax existing API usage, however, into supplying the console operator as a filler undo operator.

What I recommend for this PR's API design is to modify RevocationOrder, make it mutable, and add methods to supply the operator and undo reason. I think it is reasonable to require an operator if an undo reason is specified (this will also help with database indexing), but not vice-versa. Feel free to come up with our solutions -- my approach is only one out of many.

Here's an example of what I mean:

public interface RevocationOrder {

  // Modifies this revocation order and returns itself
  // Clears the reason if one is set
  RevocationOrder operator(Operator operator);

  // Modifies this revocation order and returns itself
  // Sets the operator and undo reason
  RevocationOrder operatorAndReason(Operator operator, String reason);

  // Clears the operator and undo reason
  RevocationOrder clearOperatorAndReason();

  // .. everything else
}

As for Punishment, I think we will need to change the method overloads to some kind of builder pattern. In my opinion, if we continue to use method overloads, the number of possibilities will grow unmanageably and make the API less ergonomic.

public interface Punishment {

  // Convenience method to keep the API approachable
  default ReactionStage<Boolean> undoPunishment() {
    return undo().undoPunishment();
  }

  UndoBuilder undo();

  // Older methods deprecated
}

public interface UndoBuilder {

  // Clears the reason if one is set
  UndoBuilder operator(Operator operator);

  // Sets the operator and undo reason
  UndoBuilder operatorAndReason(Operator operator, String reason);

  // Clears the operator and undo reason
  UndoBuilder clearOperatorAndReason();

  UndoBuilder enforcementOptions(EnforcementOptions enforcementOptions);

  ReactionStage<Boolean> undoPunishment();

  // Fetches an updated punishment object that includes undo information
  // Can be implemented by using undoPunishment() then constructing an updated punishment object
  ReactionStage<Punishment> undoAndGetPunishment();

}

Where this might break is with the requirement for undo reasons not being required. They are not now, instead replaced with a default reason when calling the old (now deprecated) API. The unpunish command though simply uses the reason rules from the punish command, making a reason either required, substituting a default or using an empty reason. I think for command usage this is perfectly fine. Thoughts?

We can't copy the logic from the punish commands because it would be a breaking change. Users will expect the old command behavior and may have automated systems in place that execute commands. Requiring undo reasons contradicts our versioning policy: https://docs.libertybans.org/#/Versioning-and-Support-Policies?id=server-side-usage

[MCMDEV]

I currently have little time to work on this, so if anybody else is interested in taking over this feature, please feel free to do so.

[MCMDEV]

I'll begin working on this again.