Able to show different errors on storage exceptions

[jmaciasportela]

Right now files_external is able to manage only one kind of error from backend storages based on the backend status (0 is OK, 1 is NOK), but it would be good to increase the error detail to show different messages to the user.

On Windows network drive app as example:

• Connection error due network problems
• Timeout error (slow network, etc...)
• Authentication error
  • If the mount is global/system, uses global credentials and the user is not admin, message to contact with owncloud admin is shown.
  • If the mount is global/system, uses global credentials and the user is admin, show a dialog to change global credentials with a warning.
  • If the mount is global/system, uses custom credentials and the user is not admin, message to contact with owncloud admin is shown.
  • If the mount is global/system, uses global credentials and the user is admin, show a dialog to enter new custom credentials.
  • If the mount is global/system and uses user credentials, dialog is shown to enter user credentials
  • If the mount is global/system and uses login credentials, message with info about the error is shown
  • If the mount is personal and uses personal credentials or custom credentials, dialog is shown to enter new credentials

So I open this issue to know your opinion and to know if is possible to have those error granularity

cc @PVince81 @icewind1991 @DeepDiver1975 @Xenopathic

[PVince81]

@jmaciasportela mentionned a discussion with @Xenopathic where instead of showing a dialog we would redirect to the admin page or personal page. I think this is only valid if the personal page always provides the proper fields for the user to enter the new credentials. Since we already have code for the dialog it is probably fine to just reuse it.

Regarding error codes, it should be possible to catch exceptions in the "test()" method and then maybe return different error codes instead of a boolean. Or instead of a boolean, throw the exception and forward it to the JS side, but then it would require parsing the exception name.

What do you guys think ? @icewind1991 @Xenopathic

[RobinMcCorkell]

We already have support for multiple failure types, currently we only distinguish general failures/indeterminate errors (red square/yellow blob in the GUI respectively), so we just need to extend that. Once all the storages in files_external have code for converting library exceptions to oC exceptions, everything will work seamlessly. There's a ticket for that around somewhere...

[PVince81]

Setting to 9.0 to look into this

[PVince81]

@jmaciasportela the code is here https://github.com/owncloud/core/blob/v8.2.1/apps/files_external/lib/config.php#L261

We need to introduce new STATUS_XXXX constants and set them from the catch block depending on the exception type.

The code that calls this function is here: https://github.com/owncloud/core/blob/v8.2.1/apps/files_external/controller/storagescontroller.php#L233 so you'll see the new status code value already in the response.

@jmaciasportela can you make the required change ? (either in #20569 (comment) or in a separate PR)

[jmaciasportela]

@PVince81 Do you want to reuse \OCP\Files\StorageNotAvailableException with different error codes and messages or is necessary to create new exceptions for each one?

Which errors do we want to be able to capture?

• Bad Credentials
• Imcomplete config
• Timeout
• Network error

[PVince81]

Maybe create subclasses of StorageNotAvailableException for these ?

@icewind1991 @Xenopathic do you agree ?

I think timeout and network error can be merged into a single one ? (or keep StorageNotAvailable for that one)

[jmaciasportela]

@PVince81 #20729

[jmaciasportela]

Every storage can implement their own test method to detect availability and status.

[jmaciasportela]

@PVince81 I saw your comment with delay, sorry PR is already ready, comment on it.

[PVince81]

That's fine, your approach might be acceptable already.

[RobinMcCorkell]

@PVince81 @jmaciasportela Ah, I read the PR first before the comment above (wrt. #20729 (comment)): yes, it'd be better to create separate exceptions that derive from StorageNotAvailableException, and get caught as appropriate.

[PVince81]

@jmaciasportela can we close this ticket as done and tick the matching checkboxes ? I see that #20729 is merged.

Or is there anything left to do ?

[jmaciasportela]

@PVince81 yes we can close it.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.