1473777 - use getData function in lieu of fetch directly (where possible)

Status: NEW

(Tree Management :: Treeherder: Frontend, defect, P3)

Description

[Cameron Dawson [:camd]]

Fix error handling in places where were our calls have switched to using fetch.

When using $http, you would check for errors by having something like this:

$http.get(url).then((success) => {
  ...
}, (error) => {
  ...
});

But with fetch, you don't use the second callback, you check the response for ``.ok`` like:

fetch.get(url).then((resp) => {
  if (resp.ok) {
    // success
  } else {
    // fail, throw an exception...
  }
}).catch(error => alert somehow...)

The catch gets hit directly only when you have connectivity issues and a few other circumstances.  400 and 500 response codes still go to the first call in the .then.

Comment 1

[Cameron Dawson [:camd]]

https://www.tjvantoll.com/2015/09/13/fetch-and-errors/

Comment 2

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3768

Comment 3

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/d60baca64fc737ab313eb2648bee551be24cd67a
Bug 1473777 - Fix Treestatus for unsupported repos (#3768)

Comment 5

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3944

Comment 6

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/60052faa19f25653e03fd89233a8c03060e862b0
Bug 1473777 - Fix error message if job is missing

Comment 7

[Ed Morley [:emorley]]

As part of working on this, it would be good to special-case HTTP 503s and ensure that the error message shown to the user says "Request exceeded maximum API time limit" or something along those lines - since the default Heroku page content says "app unavailable" which is misleading.

Perhaps an helper wrapper for all fetch calls would be useful?

Comment 8

[Sarah Clements [:sclements]]

As part of the perfherder conversion I moved the `getData` method (designed to return error messages in json responses or plain text) from IFV's helpers into the general `helpers/http.js` file.

export const getData = async function getData(url) {
  let failureStatus = null;
  const response = await fetch(url);

  if (!response.ok) {
    failureStatus = response.status;
  }

  if (response.headers.get('content-type') === 'text/html' && failureStatus) {
    return { data: { [failureStatus]: response.statusText }, failureStatus };
  }

  const data = await response.json();
  return { data, failureStatus };
};


But, for the case of the 503's I noticed IFV doesn't show the special message I programmed for this type of error (in a separate helper function) because it seems Heroku by default returns the "app unavailable" page unless we set a customized page to be returned instead: https://devcenter.heroku.com/articles/error-pages.

Comment 9

[Ed Morley [:emorley]]

Some ideas in:
https://www.peterbe.com/plog/displaying-fetch-errors-in-react

Comment 10

[Sarah Clements [:sclements]]

PushModel.getList is being updated via bug 1527824.