Request the native app to reload the direct editing view on 403 errors

[nicofrand]

📝 Summary

• Resolves: https://github.com/nextcloud/android/issues/12138

When it receives a 403 error it triggers a reload event, catchable by the android app.

Warning

I don't know how this app works/should behave in some cases, and this PR needs discussion.

1. Right now it keeps retrying the requests even though the 403 error is detected. Can I just stop the retries (they will fail anyway)? What about Direct edition outside the android app (which will not catch the reload event)?
2. The comment says "the session is invalid or the document is read only". Can I make sure the session is invalid (I should not reload the document in case it is read only I guess?)

[max-nextcloud]

Hey @nicofrand

Thanks a lot for your PR.

There are different scenarios which may cause a 403:

• Session expired or was cleaned up. In this case we would like a reload as that will cause text to reconnect and pick up the new session.
• User lost access to the document permission wise. In this case there's no point in retrying but a reload will actually show the permission error i think.
• The document is read only to begin with. This is a bit of an awkward situation right now. The user is not allowed to change the document but we would like them to be able to follow changes as they happen. We use yjs and it expects to see status updates at least from the user themself every 15 seconds. However we block such updates on the server as the user may not change the document. We should probably allow awareness changes and just block all other types of messages.

• Right now it keeps retrying the requests even though the 403 error is detected. Can I just stop the retries (they will fail anyway)?

In the case of an expired session reconnecting should fix the connection. But that's not what's happening right now i guess. Will need to investigate.

What about Direct edition outside the android app (which will not catch the reload event)?

I think displaying a message that asks for reloading the page would be a good start. We already do that in some cases.

• The comment says "the session is invalid or the document is read only". Can I make sure the session is invalid (I should not reload the document in case it is read only I guess?)

You should be able to check $editor.isEditable to rule out read only documents causing this.

[github-actions[bot]]

Hello there, Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

[nicofrand]

Hi!

Thanks for your comments.

You should be able to check $editor.isEditable to rule out read only documents causing this.

OK but the 403 occurs in the SyncService, which does not have access to $editor as far I can tell. It can just emit things. So it can't just return before throw new Error('Failed to apply steps. Retry!', { cause: err })?

I think displaying a message that asks for reloading the page would be a good start. We already do that in some cases.

Could you explain how to test a DirectEdition outside the android app? I was not able to find a way… I will display a message instead of callMobileMessage('reload') but is there an existing/proper way to do so already? Or do I need to manipulate the DOM myself? And how do I know I am not in an android app?

[max-nextcloud]

Dear @nicofrand - I'm sorry for taking so long to get back. Will prioritize this PR this week.

[juliusknorr]

@max-nextcloud As discussed in our call here is the command for testing in the browser:

curl --request POST \
  --url 'https://admin:[email protected]/ocs/v2.php/apps/files/api/v1/directEditing/open?editorId=text&creatorId=textdocument&format=json' \
  --header 'Content-Type: multipart/form-data' \
  --header 'OCS-APIRequest: true' --form path=/test.md

[max-nextcloud]

@nicofrand Again I'm sorry for taking so long. Your code changes look good and I talked through the different scenarios today witth @juliushaertl . Direct editing is also used by the desktop client and ios devices and as you said they won't support the reload message - however we can tackle that separately.

I will rebase your pull request and see if I can get the CI to work on it.

Please sign your commit off to make the DCO check pass - this means you submit your code under the same license as the rest of the text app. (The contributing guidelines have more info.)

You can do so by running git commit --amend --signoff --no-edit - this will add a signoff line to your commit message.

[nicofrand]

@nicofrand Again I'm sorry for taking so long. Your code changes look good and I talked through the different scenarios today witth @juliushaertl . Direct editing is also used by the desktop client and ios devices and as you said they won't support the reload message - however we can tackle that separately.

I will rebase your pull request and see if I can get the CI to work on it.

Please sign your commit off to make the DCO check pass - this means you submit your code under the same license as the rest of the text app. (The contributing guidelines have more info.)

You can do so by running git commit --amend --signoff --no-edit - this will add a signoff line to your commit message.

Thanks! I just signed the commit, so I had to push with --force-with-lease, which probably canceled your rebase, so I rebased it myself.

[max-nextcloud]

/backport to stable29

[max-nextcloud]

/backport to stable28

[max-nextcloud]

/backport to stable27

[max-nextcloud]

merged as the CI failures were not related.