Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added JS exception dialog for failed ajax calls #5273

Closed
wants to merge 2 commits into from

Conversation

PVince81
Copy link
Contributor

All failed ajax calls in the app will display an exception dialog.
This is achieved using a global listener to "ajaxError".
The dialog will show the exception returned by the server in JSON format
with a stack trace if available.

Fixes #5226

All failed ajax calls in the app will display an exception dialog.
This is achieved using a global listener to "ajaxError".
The dialog will show the exception returned by the server in JSON format
with a stack trace if available.

Fixes #5226
@PVince81
Copy link
Contributor Author

Here is how the dialog looks like:
exceptiondialog

To produce the error, I've added the following line in apps/files/ajax/delete.php:

throw new Exception('Test exception with an uninteresting message');

Then try and delete a file, the ajax request will fail and the dialog will popup.

Please review @karlitschek @DeepDiver1975 @schiesbn @jancborchardt

In the meantime I'll look into writing unit tests for the server side part.

@@ -0,0 +1,13 @@
<div id="{dialog_name}" title="{title}">
<div><span class="label">Error: </span><span class="value">{message}</span></div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing translation of Error and below Data and Stack.

@ghost
Copy link

ghost commented Oct 10, 2013

Test passed.
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/1501/

@tanghus
Copy link
Contributor

tanghus commented Oct 11, 2013

I really don't like this approach 👎

  • Uncaught Exceptions shouldn't be show to the user, but logged for the admin.
  • The app code should take care of any errors happening, and handle them appropriately. If this is wanted for the files app, it should be implemented in that app.
  • This change actually stops the app developer from making a RESTful API as AFAIK all 4XX and 5XX status codes are interpreted as errors and will be intercepted by this handler and breaking the application flow.
  • Apps using the app framework can have a middleware catching afterException and deal with any errors appropriately.

@PVince81
Copy link
Contributor Author

The reason I implemented this was because ajax calls were getting a full HTML page response with an exception stack trace embedded in it, which wasn't that easy to spot. In some cases in the files app UI the app seem to just do nothing when you tried to navigate to a subdir. Sometimes there are also exceptions happening in the background (for example exceptions during scanning) and nobody might notice as the UI doesn't show any error.

@tanghus I guess you're right, the exception handler should be implemented in the files app directly then instead of globally.

Would it still be acceptable to globally log the exception to console.error to make it a bit more visible ? AFAIK the global handler doesn't prevent/block the user's error callback code from $.ajax so RESTful should still work. (I can double check this)

@karlitschek
Copy link
Contributor

Can we make this depend on the debug config flag?

@tanghus
Copy link
Contributor

tanghus commented Oct 11, 2013

The reason I implemented this was because ajax calls were getting a full HTML page response with an exception stack trace embedded in it, which wasn't that easy to spot.

Yes, that has baffled be sometimes too.

Would it still be acceptable to globally log the exception to console.error to make it a bit more visible

I think it should be logged in the log file. Server side error should be in the server log.

Can we make this depend on the debug config flag?

The exception and where it happened should be logged with OCP\Util::ERROR and the backtrace with OCP\Util::DEBUG

@karlitschek
Copy link
Contributor

So what is the next step here? :-)

@PVince81
Copy link
Contributor Author

I don't have a good feeling about this dialog any more.
Maybe we can simply keep the exception returning part so that at least Ajax calls don't get a full page that will never be displayed.

@tanghus
Copy link
Contributor

tanghus commented Oct 22, 2013

Or error_log the exception and send a 500 header.

@PVince81
Copy link
Contributor Author

Before this PR there is only an entry added into the owncloud log, but without stack trace (I do have XDebug enabled):
{"app":"index","message":"Test exception with an uninteresting message","level":4,"time":"2013-10-22T15:03:25+00:00"}

Nothing appears in Apache's error log.

Is there an existing way how to output messages there ?

@tanghus
Copy link
Contributor

tanghus commented Oct 22, 2013

It doesn't necessarily have to be in the error log - owncloud.log is better to use, I just wasn't sure if the logging mechanism was available here.

For outputting the entire bt just use the same method as you did in the JSON response and write one enumerated log statement per row. Or use debug_backtrace.

@PVince81
Copy link
Contributor Author

I've sent another pull request for just the logging.
Closing the current one which is about the exception dialog.

@PVince81 PVince81 closed this Oct 22, 2013
@LukasReschke LukasReschke deleted the core-ajaxexceptiondialog branch June 20, 2014 18:19
@lock lock bot locked as resolved and limited conversation to collaborators Aug 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Files app ajax call exception must be shown in popup
4 participants