API Error Handling Overhaul

[cayb0rg]

This PR aims to solve how errors are both sent from FuelPHP's core and handled by the front-end (#1526)

Changes

• Adds 'status' field to Msg objects
  • post_call and get_call in fuel/app/classes/controller/api.php are responsible for handling responses for api calls. These will now recursively search for a 'status' field in the called function result and set that as the response status rather than an erroneous 200.
• Adds a global error handler to api.js. This essentially checks the status code if the response returns not OK and throws an error for individual error handlers to catch. This handler accounts for the variety of responses that can be sent from the back-end. For example:
  • If a response uses the Response class rather than the Msg class, the error will be stored in the body field and must be parsed as JSON.
  • If an error was stored in the body of a response with a 200 status code, the handler will throw an error. I tried to find all accounts of this happening, but it's possible I missed some, so I've left this check in.
• Changes all fetch calls to the api/json/___ route to use fetchGet which automatically configures the POST options and invokes the global error handler.
  • I also took the liberty of reorganizing API calls by route
• Moves all error handling from success handlers to error handlers, and adds error handlers if none were added.
  • This caused an issue with how the header detects whether a user is logged in by using user_get. The user_get function returns a 403 error if the user is not logged in. A non-logged in user would then get spammed with 403's in the console. Instead of changing user_get, which needs to return 403 in some situations, I added apiAuthorVerify to gate this API call.
  • Collaborator dialog similarly ran into issues, and these have been fixed
• Added alert dialog to several components, including My Widgets Page, Profile Page, and the Settings Page
• Updated styling for error/success feedback in the Support Dashboard to be more obvious
• Fixed an error occurring on Widget Detail Page when a slash "/" is included at the end of URL
• Updated tests
• Verified that all Msg::no_login() responses are only sent if verify_session has been called, which will logout the user and clear the session date.
• Fixes a 500 server error that occurs when a user renames a file to have the correct image extension before uploading it in the media importer.

Additional Findings

React Query has the option of setting a global error handler when initializing the QueryCache. This is only called if onError is not defined in the API call.

Potential issues

• React Query will retry 3 times if the query errors by default. It's possible to add 'retry: false' to prevent this or limit the number of retries, but I've left it without, since the user could just be having internet difficulties. Thus, if a user is logged out, it could take several seconds before the Login modal pops up, for example, and they can still interact with the interface during that time.

[clpetersonucf]

This is really great work. I only noticed a few very small things that need fixing, and one behavior associated with the user_get query that may need more investigation:

In the admin panel, copying a widget from the user panel causes the hash to update to the new instance ID, which needs to be corrected anyhow, but it appears that feeding a bad value to the user_get query (as this behavior does) causes a repeating 500 error from the server. This is different from the previous implementation, which would simply respond with an empty value and not repeat the request.

Edit: Presumably this is related to the user_get issues you described elsewhere, but while those were gated or handled, there may be additional calls being made to that endpoint that aren't yet properly updated.

[clpetersonucf]

Since it's related and was originally going to include changes to Msg::no_login() and similar API error states, I'd like to consider merging #1537 into this PR as well

Actually maybe we just merge #1537 into the dev branch first and then this branch incorporates the upstream changes (there shouldn't be much overlap)

[cayb0rg]

Great catch! The 500 error seemed to be coming from an uninitialized $results array. user_get should now respond with an empty array if the user_ids are not positive integers (such as in the case where an instance ID is passed in accidentally). user_get will respond with a 403 error if the current user is not logged in, hence the need for a gate for components like header.jsx. However, if there's no need to gate this function from non-logged in users, this 403 error can be removed along with the need to gate it with apiAuthorVerify. I think that could be a privacy risk, though.

I also fixed the issue with the copy success handler setting the hash to the new instance ID and added an optimistic update to add the new instance to the user's managed instances.

If you still get the 500 error, please let me know.

[clpetersonucf]

Updated to point to 10.2.0. I can give this a final review but there appears to be a number of merge conflicts that require resolution first.

[clpetersonucf]

A couple endpoints are throwing errors in certain situations that make sense but have been causing issues on the front end. Two I've found so far:

• widget_instance_edit_perms_verify, called when selecting a widget in My Widgets, will return a 401 if the user has insufficient edit perms - including View Scores access. React Query will continue to retry this request indefinitely, eventually erroring out the selected instance component with the Permission Denied: Failed to retrieve widget. message.
• When saving a newly created instance in the creator for the first time, widget_instance_save is called followed by question_set_get. However, question_set_get is not properly called, instead sending a pair of nulls for arguments. This call was probably unintended to begin with, but with the update the server responds with 403 and react query again enters an indefinite retry loop.

[clpetersonucf]

With additional testing, I haven't found any other error states that cause issues on the front-end, so that's all looking great.

The 403 error associated with question_set_get with new creator saves is fine - I see now the 3x retry behavior. It's unpleasant to see in the network tab, but doesn't cause any actual issues. I'd like to fix the creator to not perform that call, but I'm also happy to address that outside of this branch, since it's not really associated with these changes.

Really the only actual problem is the 401 associated with edit_perms_verify on the My Widgets page.

[cayb0rg]

widget_instance_edit_perms_verify, called when selecting a widget in My Widgets, will return a 401 if the user has insufficient edit perms - including View Scores access. React Query will continue to retry this request indefinitely, eventually erroring out the selected instance component with the Permission Denied: Failed to retrieve widget. message.

Excellent catch! That function should probably not be returning an error in the first place, since its intended function is to return true/false for whether the user has edit permissions.

I also noticed that users with View Scores access can update widgets in the creator. I could not think of a specific use case where this should be possible, so I've limited the widget_instance_update function to users with Full access only. Let me know if this is in error!

When saving a newly created instance in the creator for the first time, widget_instance_save is called followed by question_set_get. However, question_set_get is not properly called, instead sending a pair of nulls for arguments. This call was probably unintended to begin with, but with the update the server responds with 403 and react query again enters an indefinite retry loop.

I included a temporary fix that sends the state value instance.id instead of the parameter instId that comes from the hash to question_set_get for now.

Just needed to pull dev/10.2.0, as it seems you already changed it to instIdRef.current in the Save History fix PR! Sweet.

[clpetersonucf]

This is still a big, scary PR, but as best I can tell, everything is working as intended. Committing to the 10.2 dev branch for final testing.