Some improvements to request error handling and modal error formatting #1929

dsevillamartin · 2019-11-09T14:20:16Z

Changes proposed in this pull request:

Only show error(s) detail in debug modal, if found
Log the request URL, method, status, and response
- Only logs error(s) detail if found, else it logs the whole error object
- Logs with a group so the error data can be hidden

Reviewers should focus on:
The alert appearing was causing the modal to "close" but the background overlay remained... not sure if that's a bug or intended (?) so I closed the currently opened modal - though I don't think want to do that.

Screenshot

POST /api/flags 404

POST /flags 404

Confirmed

Frontend changes: tested on a local Flarum installation.
~~Backend changes: tests are green (run composer test).~~

luceos · 2019-11-11T12:11:19Z

Won't closing the currently active Modal cause unwanted side effects? I can imagine this would happen on log in, register etc; even so.. can't we make the error be pushed on top of the overlay somehow?

Ps, I also assume that changing these outputs will break all extensions returning custom error output?

dsevillamartin · 2019-11-11T14:03:16Z

@luceos

Won't closing the currently active Modal cause unwanted side effects?

Perhaps. The problem isn't the error alert being behind the modal, is that showing the alert results in the modal fade out animation to trigger, but the backdrop remains. Might be a separate issue though.

Ps, I also assume that changing these outputs will break all extensions returning custom error output?

I don't understand what you are saying here. The formattedError variable is only used for the error logging and for showing it in the RequestErrorModal - what extensions would be using it?

luceos · 2019-11-15T08:38:25Z

Ah your answer clarifies it. Then I have nothing to add ;)

luceos · 2019-11-18T08:22:32Z

Asked for second review from @clarkwinkelmann ; if approved feel free to merge.

clarkwinkelmann

Works great, but I have some concerns:

Why use unescape() which is deprecated ? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/unescape . Why is a decoding necessary at all ?

What is the purpose of the new this.modal.close() ? It sounds like if any error is triggered while a modal is open and the request isn't bound to that modal handler, it would close it, potentially losing data. I think core always uses the error handling inside the modal, but extension might do their own requests from the modal.

I wanted to test this with fof/terms for example. It means an error fetching the list of policies, reordering, or during saving would close the modal. But I couldn't test because of the issue I discussed on Discord where I can't install terms on my master Flarum 🤷‍♂️

dsevillamartin · 2019-11-20T21:51:12Z

@clarkwinkelmann Escape is required to transform \n into actual line breaks.

I changed the function call to decodeURI and removed the closing of the modal - not sure why, but I can no longer reproduce what I first had happen.

clarkwinkelmann · 2019-11-20T23:11:20Z

Those are great cases for inline comments I think 😄

If newlines are the only reason to use it, wouldn't .replace() work just as well ?

If we keep it like it's now I suggest adding

Escape is required to transform \n into actual line breaks

as a comment near decodeURI so we know why it was used in the future.

Otherwise approved 👍

js/src/common/Application.js

franzliedke · 2019-12-07T14:33:13Z

js/src/common/Application.js


      error.alert = new Alert({
        type: 'error',
        children,
        controls: isDebug && [
-          <Button className="Button Button--link" onclick={this.showDebug.bind(this, error)}>Debug</Button>
+          <Button className="Button Button--link" onclick={this.showDebug.bind(this, error, formattedError)}>Debug</Button>
        ]
      });

      try {
        options.errorHandler(error);
      } catch (error) {


Woah, can we rename this error variable in the catch block? I find it super confusing to follow along which object we're dealing with here. 😁

🕐 Three minutes later...

Okay, I see now, the default errorHandler implementation just re-throws the same object, which means we have an AJAX error, which makes sense.

Could we change this to check for the existence of options.errorHandler() instead? The try/catch only confuses the reader IMO.

Done. Had to remove the default errorHandler implementationa as it's not needed.

I noticed that when the error handler is set - e.g. modals using this.onerror - nothing logs to the console. I assume we want to log the error to the console regardless if the error handler is set?

Hmm, no, I think we should reserve the logging to unexpected errors - exactly the way you did it.

Really, it depends on whether we're dealing with a known or unknown error, but we have no mechanism for that yet (in custom error handlers).

js/src/common/components/RequestErrorModal.js

clarkwinkelmann · 2020-02-24T03:30:32Z

I have still not had time to test this locally, but from reading the code:

Aren't we losing the feature of the errorHandler function intentionally not handling an error ?

I'm actually using this feature in my Formulaire extension. If the errors are not validation errors for my fields, I re-throw the error, making Flarum render it in the error alert.

This PR appears to remove that feature. Now if errorHandler throws an exception, it would fail silently and not show any error alert.

I suppose any custom errorHandler could call app.alerts.show(error.alert);, but that was a nice feature, and would also show up if the error was unintentionally re-thrown from the errorHandler.

clarkwinkelmann · 2020-02-24T03:34:01Z

Would it make sense to log all errors to console, even if options.errorHandler is defined ?

We're already going to log most validation errors to the console with the current implementation I believe. We might as well log all request errors.

This would also simplify re-implementing the feature I said was removed in my previous comment as the conditions can be moved up in the code.

EDIT: I see this was discussed 10 days ago in one of the comment discussions above. If we don't want to log handled errors, maybe we should not log validation errors either ? This is kind of their expected handling if nothing more specific was defined 🤔

dsevillamartin · 2020-02-24T21:07:28Z

You make a good point. I guess there's no reason not to log e.g. validation errors - it can make debugging easier even when it's just a handled error.

The catching of errorHandler might be a mistake on my part - if we agree, I can add the try/catch back and always log the error.

franzliedke · 2020-04-17T10:31:41Z

Because we now auto-format our JS code with Prettier, this branch now has conflicts with master. Sorry about that. 😉

Please take the steps outlined in the forum to resolve the conflicts.

* Use decodeURI instead of unescape & don't close modals * Add comment * Don't use a try/catch, clean up the group log code * Remove double negative * Format; fix issues from rebasing

dsevillamartin · 2020-04-17T20:59:30Z

I rebased & squashed as I realized I hadn't done it correctly.

I also restored the errorHandler default of throwing the error, and moved back to try/catch instead of if/else.

clarkwinkelmann · 2020-04-24T23:38:29Z

Still looking good to me

askvortsov1 · 2020-05-12T16:21:54Z

This doesn't clash with any of the state changes afaik, and we might as well put it in now instead of after TS when more work will be needed to get it updated. As 3 core devs have approved and there seems to be no conflicts, merging.

luceos approved these changes Nov 15, 2019

View reviewed changes

luceos requested a review from clarkwinkelmann November 18, 2019 08:22

clarkwinkelmann requested changes Nov 19, 2019

View reviewed changes

clarkwinkelmann approved these changes Nov 27, 2019

View reviewed changes

franzliedke reviewed Dec 7, 2019

View reviewed changes

js/src/common/Application.js Outdated Show resolved Hide resolved

franzliedke reviewed Dec 7, 2019

View reviewed changes

js/src/common/Application.js Outdated Show resolved Hide resolved

franzliedke reviewed Dec 7, 2019

View reviewed changes

dsevillamartin force-pushed the ds/debug-modal-improvements branch from b5e5918 to 861d598 Compare December 7, 2019 15:50

dsevillamartin requested a review from franzliedke February 7, 2020 21:26

franzliedke reviewed Feb 14, 2020

View reviewed changes

js/src/common/components/RequestErrorModal.js Outdated Show resolved Hide resolved

Some improvements to request error handling and modal error formatting

fb43e5f

* Use decodeURI instead of unescape & don't close modals * Add comment * Don't use a try/catch, clean up the group log code * Remove double negative * Format; fix issues from rebasing

dsevillamartin force-pushed the ds/debug-modal-improvements branch from fdba98d to fb43e5f Compare April 17, 2020 20:57

askvortsov1 approved these changes Apr 25, 2020

View reviewed changes

askvortsov1 merged commit 7e661df into master May 12, 2020

askvortsov1 deleted the ds/debug-modal-improvements branch May 12, 2020 16:23

askvortsov1 added this to the 0.1.0-beta.14 milestone May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some improvements to request error handling and modal error formatting #1929

Some improvements to request error handling and modal error formatting #1929

dsevillamartin commented Nov 9, 2019 •

edited

Loading

luceos commented Nov 11, 2019

dsevillamartin commented Nov 11, 2019

luceos commented Nov 15, 2019

luceos commented Nov 18, 2019

clarkwinkelmann left a comment •

edited

Loading

dsevillamartin commented Nov 20, 2019

clarkwinkelmann commented Nov 20, 2019

franzliedke Dec 7, 2019

dsevillamartin Dec 7, 2019

franzliedke Feb 14, 2020

clarkwinkelmann commented Feb 24, 2020 •

edited

Loading

clarkwinkelmann commented Feb 24, 2020 •

edited

Loading

dsevillamartin commented Feb 24, 2020

franzliedke commented Apr 17, 2020

dsevillamartin commented Apr 17, 2020

clarkwinkelmann commented Apr 24, 2020

askvortsov1 commented May 12, 2020

Some improvements to request error handling and modal error formatting #1929

Some improvements to request error handling and modal error formatting #1929

Conversation

dsevillamartin commented Nov 9, 2019 • edited Loading

luceos commented Nov 11, 2019

dsevillamartin commented Nov 11, 2019

luceos commented Nov 15, 2019

luceos commented Nov 18, 2019

clarkwinkelmann left a comment • edited Loading

Choose a reason for hiding this comment

dsevillamartin commented Nov 20, 2019

clarkwinkelmann commented Nov 20, 2019

franzliedke Dec 7, 2019

Choose a reason for hiding this comment

dsevillamartin Dec 7, 2019

Choose a reason for hiding this comment

franzliedke Feb 14, 2020

Choose a reason for hiding this comment

clarkwinkelmann commented Feb 24, 2020 • edited Loading

clarkwinkelmann commented Feb 24, 2020 • edited Loading

dsevillamartin commented Feb 24, 2020

franzliedke commented Apr 17, 2020

dsevillamartin commented Apr 17, 2020

clarkwinkelmann commented Apr 24, 2020

askvortsov1 commented May 12, 2020

dsevillamartin commented Nov 9, 2019 •

edited

Loading

clarkwinkelmann left a comment •

edited

Loading

clarkwinkelmann commented Feb 24, 2020 •

edited

Loading

clarkwinkelmann commented Feb 24, 2020 •

edited

Loading