Save button disabled with WAC in place: showDocumentMenu draws save button before getResourceInfo has processed wac-allow header

[snydergd]

I'm unable to use the Save button for inrupt or solidcommunity Solid Ppods (the two I've tried). It remains grayed out. It appears this is the expected behaviour when the wac-allow header doesn't indicate write access. However, the wac-allow header in my case does indicate write access for both the user and the public.

I've traced it down to one line: https://github.com/linkeddata/dokieli/blob/a1bc0805643e9cfaffafe69937f29dc58dc75f87/src/doc.js#L1336

This line gets and processes details of the current document including the wac-allow header processing and storage(getResourceSupplementalInfo). However, this getResourceInfo doesn't wait for it to finish before it's returned promise gets resolved. This means that the showDocumentMenu function that called it proceeds to draw the document menu (only happens once per page load) and when that logic tries to see if necessary authorizations are in place (accessModeAllowed), the headers key of DO.U.Resource[documentUrl] is not yet present.

Consequently, the Save button remains disabled. I'm just brand new to Dokieli, so this gave me a weird feel - I can't tell if I'm just not understanding it correctly, but it seems like this is an important piece of functionality that is not working. I'm sure in some cases the pod might give back a response quick enough that it is there in time, but it never has for me yet.

One way the problem could be resolved maybe is for the promise for getResourceSupplementalInfo to be put into a variable and then for the return of getResourceInfo to be changed to suplementalInfoPromise.then(() => /* current return value */).

I would be willing to submit a pull request if that is welcomed and there's not something obvious I'm missing (I tried to finid someone else with the same problem, but didn't have much luck). And of course I am happy to provide more information if needed.

[snydergd]

In the meantime, in case someone finds it useful, I made a userscript on Greasyfork to get around the problem by just always enabling the save button.

https://greasyfork.org/en/scripts/489401-enable-dokieli-save

[csarven]

@snydergd Meant to reply to you earlier....

Thanks so much for reporting this! I can reproduce the problem and so yes, it is a bug. I must've introduced this regression. Let's solve it.

There a couple of related concerns / issues towards resolving this. Here is some additional information that helps us / consider:

Generally the HTTP requests are put off until they are really needed. For example, dokieli tries to check the wac-allow header by making a HEAD request and this is done once the user clicks the dokieli menu. The design choice could alternatively be that HEAD is made once DOMContentLoaded. This doesn't address the core issues you've highlighted with the promise, but that it is only a workaround - with some advantages as a side-effect, i.e., having some information immediately that can be used towards the dokieli menu and elsewhere. I wouldn't mind doing this generally. HEAD is cheap and it'd be a reasonable thing to do perhaps. It was just I stuck to the principle of delaying calls as much as possible.

Another general consideration is to perhaps disable certain features and enable them asynchronously. So, in the case with "Save", it should be disabled, but once dokieli obtains the wac-allow and notes that the user/public has write, it should remove disabled="disabled" from the "Save" button.

---

Not to further confuse this issue but there is a related matter in afterSignIn and perhaps that can add to the background:

https://github.com/linkeddata/dokieli/blob/a1bc0805643e9cfaffafe69937f29dc58dc75f87/src/auth.js#L286

in that those functions are supposed to return promises but they actually aren't in a reliable way, and the the Promise.all should perhaps be Promise.allSettled. The idea with afterSignIn was that once the user signs in and the essentials are fetched, the additional information about the user can be fetched asynchronously without locking anything up. That additional information could be used later on.

You are more than welcome to work on any of these things and submit a PR. I can be available in dokieli chat on Matrix if you'd like to have quick back and forths.

I'm very interested in resolving this because I'd like to add the "Delete" feature to the menu ( https://github.com/linkeddata/dokieli/issues/391 ) and it is going to be essentially the same situation with "Save" right now.

[csarven]

I've added the Delete feature. And as expected, the issue is the same (see around https://github.com/linkeddata/dokieli/blob/64a80f77451c53ff58c4e4e422c86db25c0aa5ad/src/dokieli.js#L3784 ) where it checks for "write" access permission and doesn't find it so the button is disabled.

[snydergd]

What if getResourceInfo where to return an object containing multiple named promises. For instance, the code could be updated like this:

  var promises = [];
  var suplementalInfoPromise = Promise.reject({errorType: "Not applicable", message: "No no storeHeaders option provided, so not retrieving supplemental header information.");
  if ('storeHeaders' in options) {
    //TODO: This may need refactoring any way to avoid deleting contentType and subjectURI. It leaks the options to getResource/fetcher.
    var { storeHeaders, contentType, subjectURI, ...o } = options;
    supplementalInfoPromise = getResourceSupplementalInfo(documentURL, o) // Note: needs to update to return promise
  }

  promises.push(getGraphFromDataBlock(data, options));
  promises.push(getGraphFromData(data, options));

  return {
    resourceSupplementalInfoRetrieved: supplementalInfoPromise,
    resourceInfoRetrieved: Promise.allSettled(promises)
      .then(function(resolvedPromises){
        // ...
      })
  };

Consumers would have to switch from getResourceInfo(data, options).then(() => /* do something */) to getResourceInfo(data, options).resourceInfoRetrieved.then(() => /* do something */).

However, in the case of showDocumentMenu, the headers would be handled for updating the "disabled" field as well:

  var resourceInfo = getResourceInfo(data, options);

  resourceInfo.resourceInfoRetreived.then(function(resourceInfo){
    // ...
  });

  resourceInfo.resourceSupplementalInfoRetrieved.then(function(suplementalInfo) {
    updateSaveButtonStatus(); // function to check wac-allow header and set disabled/enabled accordingly
  }).catch(function(e) {
    if (e && "errorType" in e && e.errorType === "Not applicable") {
      return;
    } else {
      // handle the exception here as HTTP errors or other would be handled today farther down
    }
  });

updateSaveButtonStatus() could be called from within the current menu rendering as well if that makes sense, to handle local storage or similar if it should be enabled in cases without wac-allow being set.

Then a similar pattern could be performed with the delete button or others that should update a little bit after the menu displays.

I don't know if that raises concerns or not to do it that way.

[csarven]

@snydergd , I'd be grateful if you can continue to look into this and make a PR. If you can't, let me know, I will try to get it going soon. Thanks!

[csarven]

Did quite a bit of refactoring to get things under better control: https://github.com/linkeddata/dokieli/commits/main/ . And now disabled (true/false) of Save and Delete (along with other features that require Write access permission, e.g., Version, Immutable) buttons in the menu work as intended basde on the wac-allow header. Aside: I've left a TODO consideration whether that should always be the case or if there are other scenarios that may influence the button state.

Essentially getResourceSupplementalInfo returns a promise and updateFeatureStatesOfResourceInfo and updateDocumentDoButtonStates are used to track states and update the menu button state.

Generally when the menu is opened or closed, it won't continuesly do a HEAD. There is a refresh timer set ot 1 minute. If user signs in or out, HEAD is requested, and states are updated.

Closing issue. If issue persists or there is regression, please create a new issue. Thanks again for raising this!