Page MenuHomePhabricator

Structured tasks should never make an edit when the user rejects the recommendation
Open, LowPublicBUG REPORT


The GrowthExperiment structured task workflow is built on top of VisualEditor, and as a hack that saves a bunch of code duplication, we make VE save the page both when the user accepts a recommendation and when they reject it (or rejects every recommendation, in the case of Add Link). On rejection, this should result in a null edit, but sometimes it doesn't because Parsoid introduces small semantically-neutral changes to wikitext (a "dirty diff"). Some examples:

We should prevent the saving of such "fake" edits, either by undoing a hack and having a rejection process that does not rely on saving, or by preventing the save (without breaking the workflow too much - at the very least, we need the recommendation to be invalidated and removed from the queue, otherwise the user will just receive it again).

(Dirty diffs can also happen when accepting recommendations, but the only effect then is the edit becoming more confusing, so these have less negative impact; they are also harder to detect, and more disruptive to prevent, so it's less clear if it's worth doing something about them.)

Event Timeline

Not sure if it's relevant - I ran into just one case: A rejected link still resulted in a saved edit. The issue is not reproducible though.

Note: Currently (wmf.15`) after a user has rejected all recommended links, a user will still be presented with "Save your changes" dialog. Add image tasks do not have it. Should the Publish dialog be dropped for Add link too?

either by undoing a hack and having a rejection process that does not rely on saving

Currently saving on rejection is used to:

  • Treat rejecting and accepting recommendations similarly from a workflow perspective (even though only one of them should result in an edit, they both mean the user "submitted" the task, so we want similar UX).
    • Show the save dialog, or something looking like the save dialog, on rejection for Add Link (we don't show it for Add Image as there is a dedicated reject button, wile for Add Link the user reviews multiple links and it's a rejection if some of them were rejected and none of them were accepted).
    • Exit VE, update suggested edit session state, and trigger the post-edit dialog after rejection (I guess exiting VE is not strictly necessary, but it makes the UX more consistent, and the post-edit dialog behavior more predictable from our perspective). We already do this since the postEdit hook doesn't trigger on a null edit, but we'd have to reorganize the code a bit.
  • Create a new rejection API that takes task information (like the image name) and triggers the structured task type's submit handler (which creates a Special:Log entry, for Add Image an EventGate event, and does various invalidation like removing the CirrusSearch tag, resetting the user's task cache, adding to the Add Image temporary exclusion list, deleting from DB cache in the case of Add Link).
  • Adjust instrumentation.
  • Increment the grafana tasktype save counter (now done by the VE postsave hook).

So rewriting it would not be simple. OTOH we could probably remove a bunch of hacks (like StructuredTaskArticleTarget.getSaveOptions).

or by preventing the save

Some options here:

  • Update VisualEditorApiVisualEditorEditPreSave so it can change the wikitext, and update the wikitext to be the wikitext stored in the revision table (or provide some other means by which the hook can signal to the VE API that it should not make an edit but pretend it did). This is a huge hack, and fragile. If I were the maintainer of the VE API code, I wouldn't want it.
  • Use the VisualEditorApiVisualEditorEditPreSave hook to return an error if the received wikitext is not the same as the stored wikitext. This can fail in some extremely fringe cases (when the pre-save transform isn't idempotent) but in practice it's fine. Since this will make the visualeditoredit API return an error, we'd need special handling for that on the frontend (if we care; since this will be rarely needed, and a confusing error message is less problematic than a confusing edit, we might not).
  • Use a lower-level edit hook which allows changing the content, such as ParserPreSaveTransformComplete. There is no clean way to identify GrowthExperiments edits at this point, but we can rely on global state. In terms of capabilities, this is the best option (allows forcing a null edit, instead of a failed edit) but feels super hacky. Would probably need to be re-done after parser unification.
  • Use a lower-level save hook with access to the edit summary (e.g. MultiContentSave) which is a relatively clean way to detect structured edits. These hooks can only prevent the edit, not turn it into a null edit, though.
Etonkovidova lowered the priority of this task from High to Low.Oct 20 2023, 12:32 AM

The issue is not straightforwardly reproducible (doesn't mean that there are no cases where dummy edits are possible for structured tasks). Due to low user impact, I'm lowering the severity of the issue.