Suggested removal reasons: proof-of-concept and discussion

[molenzwiebel]

So here's a large PR that is partially a rewrite of the removal reasons module and partially a proof of concept of the removal reasons. It's quite some spaghetti, so I recommend you read this first before you dive into the code (as I'll be referencing specific snippets in here).

Essentially, I'm exploring a feature that allows one to set up "premade" responses (or suggested removal reasons as they are called in this PR) to posts flagged with a certain AutoModerator report reason (although this could be extended to reports by others too). I personally mod several subreddits with AutoModerator rules that exist to catch one specific rule violation and are almost always either "approve" or "remove with this specific reason". This feature exists for those cases and essentially should help moderators simply press one button to "confirm" that the AutoModerator rule ended up being correct.

Given that removal reasons can be fairly complex (a single removal message contains a header, footer, possibly several "reasons" and each reason can also contain <input>s and <select>s), there's no ideal approach to creating these canned responses. We could just allow users to specify a text field, similar to macros, but ideally we connect it to the removal reasons system for ease of use.

Two approaches for storing a canned/suggested removal reason exist that I think are feasible. They are:

• Somehow attach IDs to each removal reason, then store the IDs of selected removal reasons and the values entered for <input>s and <select>s. Then, build the message string on removal by querying the content of the rules and substituting input values where necessary.
• Build the removal reason beforehand into a string outside of the {author} and other tokens, then only perform the last substitution on actual removal.

For this proof-of-concept, I've opted for the second approach. After some consideration however, I'm not entirely sure whether that is the best approach. Further on it will hopefully become clear why I have my doubts about this approach.

---

Let's discuss the actual proof-of-concept. First, I've created a new removal reasons module that performs essentially the same function as the current one. If you're testing this, you should probably disable the original removal reason module as they will quite definitely interfere with each other.

As part of this split/duplication, I've extracted some logic for fetching the (possibly cached) subreddit toolbox configuration and resolving the stored removal reasons (including tracking removal reasons that reference some other PR). They are TBHelpers.getSubredditConfig and TBHelpers.getRemovalReasonsStorage. These functions are simply cleanups of the existing code in removalreasons and they are of high enough quality to be added to toolbox regardless of whether the rest of this PR ever lands in the main repo. I've also added an additional function, TBHelpers.getRemovalReasonsSettings which converts the serialized removal reason settings into usable values (deserializing, adding defaults where needed). It also renames some in my opinion unclear names. This function is only used in my new (PoC) code, but I think that one should consider porting this over if you ever clean up removal reasons as it is significantly easier to work with and better represents the current legal values for removal reasons.

---

The meat of the actual addition sits in newremovalreasons.js. Before discussing the actual code, I want to explain the architecture for this reworked/PoC version of removal reasons.

We can roughly break up the removal reason process into the following distinct steps:

• User removes a post (or clicks the post-removal "Add removal reason" button)
• Toolbox loads available removal reasons
• Toolbox presents removal reason from to user, user composes message, configures reply options
• Toolbox performs removal actions, including sending the message, setting flair, locking/stickying, etc

The current removalreasons module is essentially doing all of this in a single spaghetti filled function. Parameters are passed all over the place through the DOM, some button presses are handled by waiting for a click on the entire div, etc. For my new version, I set out to untangle some of that mess (where possible).

The new module explicitly splits the steps as described above. This makes for more clear code, but additionally it allows us to present the removal reason dialog to the user without actually having something to remove. This allows us to compose suggested removal reasons but execute them later, as we will see later when discussing the config section of suggested removal reasons.

In particular, the meat of newremovalreasons.js sits in two functions: promptRemovalReason and applyRemovalResult.

promptRemovalReason
This is the function that performs step two in the steps described above. It shows a popup to the user, allowing them to compose a removal message, configure sending options, and confirm. I think a good way to get an intuitive idea of what the function does without looking through all the code is to consider its TypeScript signature:

// This is the result of `promptRemovalReason` and represents all actions that need to
// be taken as the result of options chosen by the user in the removal reasons modal.
interface ChosenRemovalOptions {
    // actions that need to be applied to the submission. undefined if this is a
    // removal acting on a comment.
    submission?: {
        // whether the submission should be locked
        lock: boolean;

        // flairs to attach to the submission, or null if no removal reasons specify any flairs
        flair: {
            text: string;
            css: string;
            templateID: string | null;
        } | null;
    };

    // presence of the comment key indicates that a comment should be left on the
    // thing to be removed. Present if the user chooses to leave a comment or to 
    // leave both a comment and a DM.
    comment?: {
        // the actual content of the message, with tokens like {author} not substituted yet
        content: string;

        // whether the reply comment should be locked
        lock: boolean;

        // whether the reply comment should be stickied
        sticky: boolean;
    }; 

    // presence of the dm key indicates that a direct message (or modmail) should
    // be sent to the user. Present if the user chooses to send a DM or to leave 
    // both a comment and a DM.
    dm?: {
        // the subject of the message, with tokens like {author} not substituted yet
        content: string;

        // the actual content of the message, with tokens like {author} not substituted yet
        content: string;

        // whether the message should be sent as a subreddit (modmail) or as a normal DM
        asSub: boolean;

        // whether the message should be automatically archived (new modmail only)
        archive: boolean;
    };

    // presence of the logSubmission key indicates that a submission should be made
    // to a given logging submission detailing the removal of the message
    logSubmission?: {
        // the subreddit to submit the removal log to
        subreddit: string;

        // the title of the log submission. {reason} is already substituted by the reason
        // given in the removal reason modal. Other tokens (e.g. {kind}) are not substituted yet
        title: boolean;
    };
}

/**
 * Ask the user for the removal reason message and sending options they'd like to apply,
 * given the set of valid removal reasons. Returns a promise that represents the chosen
 * removal options, or rejects if the user chooses to cancel.
 */
async function promptRemovalReason(
    // The removal reason settings for this subreddit. Retrieved by invoking `getRemovalReasonsSettings`.
    // Not writing out the full type definition here.
    settings: unknown,
    // The title of the removal reasons modal.
    title: string,
    // Some HTML to be shown as context at the beginning of the modal (usually something along the lines of "Removing <a href="/link/to/post">post title</a>").
    context: string,
    // Whether to show only submission removal reasons, only comment removal reasons, or both.
    show: "all" | "submission" | "comment" = "all"
): Promise<
    ChosenRemovalOptions
    | null // null if removed without leaving a reason (not the same as cancelling!)
>

The actual implementation of this function can be here. It should be noted though that it is largely copied from the original and cleaned where needed. I wouldn't go and do extended code review on it now, given that it is more the general functionality of this function that matters for the discussion in this PR (plus, I'm sure it has bugs 😄).

As you can see, invoking this function and waiting for the result gives you a fairly comprehensive object that represents exactly what actions to take. Crucially however, nothing in this result is uniquely attached to the actual thing to be removed. This means that we can store the result of promptRemovalReason and apply it later (and this is exactly what happens with suggested removal reasons).

For an example result of invoking promptRemovalReason, expand this:

Example promptRemovalReason response

{
    "submission": {
        "lock": true,
        "flair": null
    },
    "comment": {
        "content": "Hi /u/{author}. Thank you for participating in /r/leagueoflegends! However (please read this in entirety),\n\nYour post has been removed because \n*rant posts and sob stories are not allowed.*\n\n\nIf you are not familiar with the subreddit rules, you can read them [here](/r/leagueoflegends/w/subredditrules).\n\n\n---\n^(Have a question or think your post doesn't break the rules?) **[^(Message our modmail and please don't direct message)](https://reddit.com/message/compose?to=%2Fr%2Fleagueoflegends)**^.",
        "lock": true,
        "sticky": false
    },
    "dm": {
        "subject": "Your {kind} was removed from /r/{subreddit}",
        "content": "Hi /u/{author}. Thank you for participating in /r/leagueoflegends! However (please read this in entirety),\n\nYour post has been removed because \n*rant posts and sob stories are not allowed.*\n\n\nIf you are not familiar with the subreddit rules, you can read them [here](/r/leagueoflegends/w/subredditrules).\n\n\n---\n^(Have a question or think your post doesn't break the rules?) **[^(Message our modmail and please don't direct message)](https://reddit.com/message/compose?to=%2Fr%2Fleagueoflegends)**^.",
        "asSub": false,
        "archive": true
    }
}

promptRemovalReason's partner in crime is applyRemovalResult. It takes information on the thing we're removing (retrivable using TBCore.getApiThingInfo) alongside an ChosenRemovalOptions instance (i.e. the output of promptRemovalReason) and actually applies the removal actions such as creating a comment, flairing the post, etc.

I actually recommend you take a quick peek at that function. async/await combined with the structure of the ChosenRemovalOptions object means that the function is almost trivial and does not have to deal with complexities like "just a removal comment" vs "just a removal dm" vs "both" vs "none".

The rest of newremovalreasons largely mirrors the original, which includes setting up listeners for removal buttons, adding the post-removal "Add removal reason" button, etc. There is some additional code for actually adding the "Remove using suggested removal reason" button but we'll discuss that later.

---

Let's take a step back from the code and consider the general design of this PR and the suggested removal reasons feature now that you have an idea as to how the new removal reasons module is structured.

In this proof-of-concept, the flow essentially works as follows:

• Parse the automoderator config to extract all possible report reasons from automod.
• Allow the user to configure a specific suggested removal reason for each unique automod report reason.
• Configuring a suggested removal is done by invoking promptRemovalReason, which will present the exact same removal modal the user is used to. The result of the modal (the set of actions to take) is persisted.
• Whenever a post is encountered that has a matching report reason, a button is added that allows the user to perform the suggested removal reason.
• When this button is clicked, applyRemovalResult is called with the persisted actions configured earlier.

This video shows the entire process in action.

You can review the actual code that handles this process in other sections of the PR, but since this is more a proof of concept and not the actual final code I don't think it is too important for you to read that right now. If you do want to, here's some interesting sections:

• Parsing automod configs: TBHelpers.parseAutomodReasons
• Subreddit suggested removal reasons config: rendering the content and fetching the automod/removal settings.
• Adding support to the Toolbox event system for relaying mod reports in events, very hacky: here
• Listening to events, parsing the automoderator reason, and adding the suggested removal button if needed: here

---

Now, I want to discuss why I dislike this current implementation architecture. There's a few things here that are/could be issues.

Possible large storage consumption. Storing the result of promptRemovalReason means that the entire markdown removal reason is stored in the toolbox wiki page. If you have quite a bit of suggested removal reasons, that means you also have quite a bit of storage and duplication here.

Desync between removal reasons and suggested removal reasons is possible. Since there's nothing linking a suggested removal reason and the "original" removal reasons that were selected, suggested removal reasons can become stale when someone edits a removal reason but doesn't update all the suggested removal reasons that included said removal reason.

Subpar error handling. The original/current removalreasons implementation has error handling that provides feedback to the user about which step of the removal went wrong. If applying the flair errors, a nice message will be added to the modal and the user gets the option to retry. In the new implementation, once promptRemovalReason has returned, the modal is already destroyed. We can still handle errors and show a message, but since the modal is destroyed we have to force the user to either retry with the exact same options, or enter everything again.

Possible performance concerns. The original/current implementation specifically caches the removal modal and only clears it if multiple things are removed on a single page (such as the modqueue). In the new architecture, every invocation of promptRemovalReason is unique by design and can therefore not reuse the same modal.

Additionally, this PoC itself has some known issues:

No support for filtered automod actions. Current implementation only looks at the reports that are currently shown. filtered things do not contain a reports element and are therefore not detected.

Partial/missing support for new reddit. I've barely tested it works on old reddit.

Doesn't remove the thing until after the modal is completed. Should be as simple as programmatically pressing the "yes" button though.

Mediocre UI. Both in settings, and the "Use suggested removal option" button flow.

---

As I mentioned in the beginning, an alternative way to implement this is to instead store a list of selected removal reasons through some sort of unique ID, along the values of the <input> and <select> fields. This approach fixes some of the flaws from the architecture used in this PoC, but it comes with some other flaws on its own (cannot skip the modal entirely without rewriting removal reasons, requires a potential schema change). It would however largely allow us to avoid touching the spaghetti that is the old removal reasons.

---

I've been typing this for quite some time now, but I think this is everything we discussed in the Discord and that I've personally considered. Open to comments.

As a sidenote, if you ever consider embedding something like Vue or React for toolbox UIs, consider me on board. Creating "interactive" UIs with jQuery and direct DOM manipulation makes me want to die.

[eritbh]

I like this idea a lot, and appreciate the thought you've put into this!

I'm hesitant to completely rewrite the removal reasons module from scratch right now; I think the better way to clean it up will be to work with the existing code and revise it in chunks, so we can take time to review the individual pieces of this effort without getting lost or missing too many things. Diffs, even complicated ones, are always easier to review than entire files being rewritten from the ground up.

In my opinion, the ideal way to move forward with this feature is to break it up into two major parts. First, let's do a PR that revises the existing removal reasons code to break up the prompt and the actual action—the promptRemovalReason and applyRemovalResult functions, as you've called them—as well as any other opportunistic refactoring we can do there that will make things easier. Ideally this will introduce all the internal changes we'll need to implement the feature, but without actually implementing the feature. That way, we can determine at that point whether the new strategy introduces any regressions in existing behavior, which we can catch before introducing completely new logic. Once we've got that done and tested, we can do a second PR that uses the revised internals to implement the new feature. This should honestly be the easy part.

To respond to some of your actual code:

• I'm not a huge fan of the naming you have for the ChosenRemovalOptions model, specifically that the submission key contains actions for both submissions and comments being removed. It's not clear at a glance that comment refers not to actions taken on the comment being removed, but to the comment being left in reply to the removed item. I think an easy way to clean this up (as well a simplify some of the processing logic) would be to implement this as a model without nested keys, instead using longer, descriptive property names. This would also let you use object destructuring to set defaults for each option in the applyRemovalResult definition, which could be a bit nicer to read.

• As far as storing the settings for this goes, I definitely agree that attaching unique IDs for each removal reason and referencing that from elsewhere is the way to go. This can be done without a schema update since adding additional properties to our existing models isn't a breaking change, though we probably want to add a migration step to setting retrieval that will add unambiguous IDs to existing settings and write the result back immediately if none are already present (e.g. on first run after updating).

• Handling <input> et. al. is a bit tricky because it's not particularly well structured. This feature will add special considerations for Replace RR HTML parsing with replaced tokens #118 (which I'm honestly reconsidering the direction of anyway). The simplest way to handle this I can think of is to just not handle inputs in storage at all, and instead display a popup when clicking a suggested reason with inputs to choose which inputs are used, but that seems like a bit of a cop-out as far as the whole "one-click removal reasons" concept goes. Without any way to reliably attach identifiers to input elements in the removal reason markup, I think any solution for prefilling those fields from the sub settings window is going to be pretty brittle, so I don't have any strong preference for how that's handled in storage - maybe just an array of strings that are sequentially filled into the replaced elements? We may also want to add logic when saving removal reasons to check if the associated prefilled settings need to be updated, e.g. if the number of inputs in the template changes.

• The removal reasons UI needs a rewrite (Refactor removal reasons to use UI builders instead of custom popup #186), but I feel like it could be too messy to do a huge amount of UI refactoring in the same PR as these changes. That said, it seems like having a more modular interface for removal reasons would probably be helpful for implementing the settings UI for this feature. It might be smart to tackle that issue before anything else.