Page MenuHomePhabricator

Modifications done to tool form or list form are not preserved when saving fails
Open, MediumPublicBUG REPORT

Description

What I did (steps)

  1. I created a new tool entry. Added initial information, click published. It is saved.
  1. then I clicked on edit the tool, and added valid information in several fields AND added an invalid information in one field (an element which actually was in the wrong format).

In the "more information" tab, to the « for wikis » field, I tried adding « Commons ».
The right format should have been commons.wikimedia.org, but I had not realized that. So I added « Commons » in the field and « enter ».
At this point, the field « for wikis » include « commons ».

  1. then I click « Publish Changes »
  1. the page reload in « view mode » (not the « edit mode » any more). The changes have not been integrated (neither the valid, nor the invalid). At the bottom of the screen, I see a red rectangle with the following text in it "Oops! An error occurred in for_wikis: Enter a valid value conforming to the JSON Schema ».

Since the page was « reloaded » all changes I made (valid and invalid) are lost.

When I do « back » on the browser, it put back the page in edit mode, but with all previous modifications made (but not saved) gone.

This situation repeated itself several times. It happened on the « edit the tool » form and on the « edit a list «  form.
On the « edit list » form, it happened when I entered an invalid tool name. All changes were lost.
In the case of the form, the red rectangle provided this message (upon entering a wrong tool name : "Oops! An error occurred in tools: Unknown tool: bighno"

Event Timeline

bd808 triaged this task as Medium priority.May 13 2022, 12:31 AM
bd808 changed the subtype of this task from "Task" to "Bug Report".
bd808 moved this task from Backlog to Groomed/Ready on the Toolhub board.
bd808 subscribed.

Thank you for the detailed report @Anthere!

Looking at the source code in EditTool.vue I can understand the general problem. We have quite a bit of client side validation which will report errors to you in a way that they can be corrected without losing other pending changes. The lost changes issue occurs when client side validation has succeeded (or somehow been bypassed, but that's not a likely case), the edit has been submitted to the backend server, and the backend server responds with an error. This fundamentally happens because of how we are currently dealing with errors from the backend as a concern of the vuex subsystem. We catch the error from the backend in the vuex API wrapper layer and use it to generate an error alert. This ends up clearing the error state of the promise, so even if the form triggering the submit and subsequent error waits on the promise before navigating away that form does not see that the error occurred.

It is going to take some thought to redesign these forms in a way that allows us to notice and trap the error from the server, report the problem to the user, and allow the form to cancel the navigation that would normally be done on success. The most direct approach would be to put the error handling burden on each caller of the vuex API wrapper layer. The original intent of putting the error handling in the vuex actions was to make it easier to write forms. This "edge case" was not something I was really thinking of then though. I was naively assuming that we could do all the necessary validation client side, but obviously there are some places where we have either failed to do that properly, or more realistically there will always be ways that the backend can reject a submission which we cannot anticipate on the frontend. One trivial example of this would be a rejection for concurrent modification/edit conflict.

The most direct approach would be to put the error handling burden on each caller of the vuex API wrapper layer.

Fundamentally we need to be able to re-arrange the promise chain from something that today looks something like:
call backendthen store resultexcept handle errorthen go to next_page
to instead place the error handling at the end of the chain more like:
call backendthen store resultthen go to next_pageexcept handle error

A more procedural way to visualize that is:

try:
    result = call_backend()
    store( result )
    go_to( next_page )
except:
    handle_error()

We might be able to keep the error handling where it already is in the vuex actions and achieve this by allowing the calling form to pass in a "thenable" as part of the request that would do the then go to next_page action which we could stick into the promise chain ahead of our existing error handling. We might also need to be able to pass another "thenable" to be called if and only if the except handle error case is fired. That mostly depends on what would need to be done back in the caller if and when the asynchronous request hit the error state.