Fixed promise error handling

In many places where we use Promises (e.g. for making fetch requests to
our backend APIs) we were previously incorrectly handling promise
rejection. We were often doing `promise.then(fn).catch(fn)`, which is not
a great idea - then the `.catch` handler catches any errors in the
Promise AND any errors in the `.then` handler. Instead, we should prefer
to do `promise.then(resolve, reject)` - this makes the `reject` handler
narrower in it's responsibility.

The main problem with the former pattern is that the `.catch` handler
can swallow any exceptions which occur in the `.then` handler, and we
will not see them. This can lead to strange and unhelpful stack traces
(such as the react `getHostNode` error that crops up sometimes).

Anyway, this PR should fix it! I also simplified and streamlined our
`fetchJSONWithCSRF` function while I was at it.

pr #3022

static/js/actions/course_enrollments.js

@@ -28,8 +28,7 @@ export const addCourseEnrollment = (courseId: string): Dispatcher<*> => {
         dispatch(fetchCoursePrices(SETTINGS.user.username, true));
         dispatch(showEnrollPayLaterSuccessMessage(courseId));
         return Promise.resolve();
-      }).
-      catch(() => {
+      }, () => {
         dispatch(receiveAddCourseEnrollmentFailure());
         return Promise.reject();
       });

static/js/actions/course_prices.js

@@ -3,7 +3,6 @@ import type { Dispatch } from 'redux';
 
 import * as api from '../lib/api';
 import type { Dispatcher } from '../flow/reduxTypes';
-import type { CoursePrices } from '../flow/dashboardTypes';
 import { withUsername } from './util';
 
 export const REQUEST_COURSE_PRICES = 'REQUEST_COURSE_PRICES';
@@ -18,12 +17,13 @@ export const receiveCoursePricesFailure = withUsername(RECEIVE_COURSE_PRICES_FAI
 export const CLEAR_COURSE_PRICES = 'CLEAR_COURSE_PRICES';
 export const clearCoursePrices = withUsername(CLEAR_COURSE_PRICES);
 
-export function fetchCoursePrices(username: string, noSpinner: boolean = false): Dispatcher<CoursePrices> {
+export function fetchCoursePrices(username: string, noSpinner: boolean = false): Dispatcher<void> {
   return (dispatch: Dispatch) => {
     dispatch(requestCoursePrices(username, noSpinner));
     return api.getCoursePrices(username).
-      then(coursePrices => dispatch(receiveCoursePricesSuccess(username, coursePrices))).
-      catch(error => {
+      then(coursePrices => {
+        dispatch(receiveCoursePricesSuccess(username, coursePrices));
+      }, error => {
         dispatch(receiveCoursePricesFailure(username, error));
         // the exception is assumed handled and will not be propagated
       });

static/js/actions/dashboard.js

@@ -3,7 +3,6 @@ import type { Dispatch } from 'redux';
 
 import * as api from '../lib/api';
 import type { Dispatcher } from '../flow/reduxTypes';
-import type { Dashboard } from '../flow/dashboardTypes';
 import { withUsername } from './util';
 
 export const UPDATE_COURSE_STATUS = 'UPDATE_COURSE_STATUS';
@@ -24,12 +23,13 @@ export const receiveDashboardFailure = withUsername(RECEIVE_DASHBOARD_FAILURE);
 export const CLEAR_DASHBOARD = 'CLEAR_DASHBOARD';
 export const clearDashboard = withUsername(CLEAR_DASHBOARD);
 
-export function fetchDashboard(username: string, noSpinner: boolean = false): Dispatcher<Dashboard> {
+export function fetchDashboard(username: string, noSpinner: boolean = false): Dispatcher<void> {
   return (dispatch: Dispatch) => {
     dispatch(requestDashboard(username, noSpinner));
     return api.getDashboard(username).
-      then(dashboard => dispatch(receiveDashboardSuccess(username, dashboard))).
-      catch(error => {
+      then(dashboard => {
+        dispatch(receiveDashboardSuccess(username, dashboard));
+      }, error => {
         dispatch(receiveDashboardFailure(username, error));
         // the exception is assumed handled and will not be propagated
       });

static/js/actions/email.js

@@ -30,14 +30,14 @@ export function sendEmail(
   emailType: string,
   sendFunction: () => Promise<EmailSendResponse>,
   sendFunctionParams: Array<*>
-): Dispatcher<EmailSendResponse> {
+): Dispatcher<?EmailSendResponse> {
   return (dispatch: Dispatch) => {
     dispatch(initiateSendEmail(emailType));
     return sendFunction(...sendFunctionParams).
       then(response => {
         dispatch(sendEmailSuccess(emailType));
         return Promise.resolve(response);
-      }).catch(error => {
+      }, error => {
         dispatch(sendEmailFailure({type: emailType, error: error}));
       });
   };

static/js/actions/index.js

@@ -20,15 +20,15 @@ export const requestCheckout = (courseId: string) => ({
   payload: { courseId }
 });
 
-export function checkout(courseId: string): Dispatcher<CheckoutResponse> {
+export function checkout(courseId: string): Dispatcher<?CheckoutResponse> {
   return (dispatch: Dispatch) => {
     dispatch(requestCheckout(courseId));
     return api.checkout(courseId).
       then(response => {
         const {url, payload} = response;
         dispatch(receiveCheckoutSuccess(url, payload));
         return Promise.resolve(response);
-      }).catch(error => {
+      }, error => {
         dispatch(receiveCheckoutFailure(error));
       });
   };

static/js/actions/profile.js

@@ -47,12 +47,13 @@ const receivePatchUserProfileFailure = createAction(RECEIVE_PATCH_USER_PROFILE_F
   (username, errorInfo) => ({ username, errorInfo })
 );
 
-export const saveProfile = (username: string, profile: Profile): Dispatcher<Profile> => {
+export const saveProfile = (username: string, profile: Profile): Dispatcher<void> => {
   return (dispatch: Dispatch) => {
     dispatch(requestPatchUserProfile(username));
     return api.patchUserProfile(username, profile).
-      then(newProfile => dispatch(receivePatchUserProfileSuccess(username, newProfile))).
-      catch(error => {
+      then(newProfile => {
+        dispatch(receivePatchUserProfileSuccess(username, newProfile));
+      }, error => {
         dispatch(receivePatchUserProfileFailure(username, error));
         // the exception is assumed handled and will not be propagated
       });
@@ -69,12 +70,13 @@ export const updateValidationVisibility = createAction(UPDATE_VALIDATION_VISIBIL
   (username, keySet) => ({ username, keySet })
 );
 
-export function fetchUserProfile(username: string): Dispatcher<Profile> {
+export function fetchUserProfile(username: string): Dispatcher<void> {
   return (dispatch: Dispatch) => {
     dispatch(requestGetUserProfile(username));
     return api.getUserProfile(username).
-      then(json => dispatch(receiveGetUserProfileSuccess(username, json))).
-      catch(error => {
+      then(json => {
+        dispatch(receiveGetUserProfileSuccess(username, json));
+      }, error => {
         dispatch(receiveGetUserProfileFailure(username, error));
         // the exception is assumed handled and will not be propagated
       });

static/js/actions/programs.js

@@ -11,10 +11,7 @@ import { fetchDashboard } from './dashboard';
 import { fetchCoursePrices } from './course_prices';
 import { setToastMessage, setEnrollProgramDialogVisibility } from '../actions/ui';
 import type { Dispatcher } from '../flow/reduxTypes';
-import type {
-  AvailableProgram,
-  AvailablePrograms,
-} from '../flow/enrollmentTypes';
+import type { AvailableProgram } from '../flow/enrollmentTypes';
 import * as api from '../lib/api';
 
 export const SET_CURRENT_PROGRAM_ENROLLMENT = 'SET_CURRENT_PROGRAM_ENROLLMENT';
@@ -29,12 +26,13 @@ export const receiveGetProgramEnrollmentsSuccess = createAction(RECEIVE_GET_PROG
 export const RECEIVE_GET_PROGRAM_ENROLLMENTS_FAILURE = 'RECEIVE_GET_PROGRAM_ENROLLMENTS_FAILURE';
 export const receiveGetProgramEnrollmentsFailure = createAction(RECEIVE_GET_PROGRAM_ENROLLMENTS_FAILURE);
 
-export function fetchProgramEnrollments(): Dispatcher<AvailablePrograms> {
+export function fetchProgramEnrollments(): Dispatcher<void> {
   return (dispatch: Dispatch) => {
     dispatch(requestGetProgramEnrollments());
     return api.getPrograms().
-      then(enrollments => dispatch(receiveGetProgramEnrollmentsSuccess(enrollments))).
-      catch(error => {
+      then(enrollments => {
+        dispatch(receiveGetProgramEnrollmentsSuccess(enrollments));
+      }, error => {
         dispatch(receiveGetProgramEnrollmentsFailure(error));
         // the exception is assumed handled and will not be propagated
       });
@@ -50,7 +48,7 @@ export const receiveAddProgramEnrollmentSuccess = createAction(RECEIVE_ADD_PROGR
 export const RECEIVE_ADD_PROGRAM_ENROLLMENT_FAILURE = 'RECEIVE_ADD_PROGRAM_ENROLLMENT_FAILURE';
 export const receiveAddProgramEnrollmentFailure = createAction(RECEIVE_ADD_PROGRAM_ENROLLMENT_FAILURE);
 
-export const addProgramEnrollment = (programId: number): Dispatcher<AvailableProgram> => {
+export const addProgramEnrollment = (programId: number): Dispatcher<?AvailableProgram> => {
   return (dispatch: Dispatch) => {
     dispatch(requestAddProgramEnrollment(programId));
     return api.addProgramEnrollment(programId).
@@ -63,8 +61,7 @@ export const addProgramEnrollment = (programId: number): Dispatcher<AvailablePro
         dispatch(setEnrollProgramDialogVisibility(false));
         dispatch(fetchDashboard(SETTINGS.user.username));
         dispatch(fetchCoursePrices(SETTINGS.user.username));
-      }).
-      catch(error => {
+      }, error => {
         dispatch(receiveAddProgramEnrollmentFailure(error));
         dispatch(setToastMessage({
           message: "There was an error during enrollment",

static/js/containers/App_test.js

@@ -9,14 +9,17 @@ import Navbar from '../components/Navbar';
 import {
   REQUEST_DASHBOARD,
   RECEIVE_DASHBOARD_SUCCESS,
+  CLEAR_DASHBOARD,
 } from '../actions/dashboard';
 import {
   REQUEST_COURSE_PRICES,
   RECEIVE_COURSE_PRICES_SUCCESS,
+  CLEAR_COURSE_PRICES,
 } from '../actions/course_prices';
 import {
   REQUEST_FETCH_COUPONS,
   RECEIVE_FETCH_COUPONS_SUCCESS,
+  CLEAR_COUPONS,
 } from '../actions/coupons';
 import {
   REQUEST_GET_USER_PROFILE,
@@ -143,15 +146,18 @@ describe('App', function() {
       helper.programsGetStub.returns(Promise.reject("error"));
       let types = [
         REQUEST_DASHBOARD,
-        RECEIVE_DASHBOARD_SUCCESS,
         REQUEST_COURSE_PRICES,
-        RECEIVE_COURSE_PRICES_SUCCESS,
         REQUEST_FETCH_COUPONS,
-        RECEIVE_FETCH_COUPONS_SUCCESS,
         REQUEST_GET_USER_PROFILE,
-        RECEIVE_GET_USER_PROFILE_SUCCESS,
         REQUEST_GET_PROGRAM_ENROLLMENTS,
         RECEIVE_GET_PROGRAM_ENROLLMENTS_FAILURE,
+        CLEAR_DASHBOARD,
+        CLEAR_COURSE_PRICES,
+        CLEAR_COUPONS,
+        RECEIVE_DASHBOARD_SUCCESS,
+        RECEIVE_COURSE_PRICES_SUCCESS,
+        RECEIVE_FETCH_COUPONS_SUCCESS,
+        RECEIVE_GET_USER_PROFILE_SUCCESS,
       ];
       return renderComponent('/dashboard', types).then(([wrapper]) => {
         let text = wrapper.find('.page-content').text();

static/js/containers/DashboardPage.js

@@ -356,7 +356,7 @@ class DashboardPage extends React.Component {
       this.context.router.push('/dashboard/');
       // update coupon state in Redux
       dispatch(fetchCoupons());
-    }).catch(() => {
+    }, () => {
       dispatch(setToastMessage({
         title: "Coupon failed",
         message: "This coupon code is invalid or does not exist.",
@@ -407,7 +407,7 @@ class DashboardPage extends React.Component {
     ) {
       dispatch(skipFinancialAid(programId)).then(() => {
         this.setConfirmSkipDialogVisibility(false);
-      }).catch(() => {
+      }, () => {
         this.setConfirmSkipDialogVisibility(false);
         dispatch(setToastMessage({
           message: "Failed to skip financial aid.",

static/js/lib/api.js

@@ -12,6 +12,7 @@ import type { Dashboard, CoursePrices } from '../flow/dashboardTypes';
 import type { AvailableProgram, AvailablePrograms } from '../flow/enrollmentTypes';
 import type { EmailSendResponse } from '../flow/emailTypes';
 import type { PearsonSSOParameters } from '../flow/pearsonTypes';
+import { S, parseJSON, filterE } from './sanctuary';
 
 export function getCookie(name: string): string|null {
   let cookieValue = null;
@@ -84,6 +85,19 @@ const _fetchWithCSRF = (path: string, init: Object = {}): Promise<*> => {
 export { _fetchWithCSRF as fetchWithCSRF };
 import { fetchWithCSRF } from './api';
 
+// resolveEither :: Either -> Promise
+// if the Either is a Left, returns Promise.reject(val)
+// if the Either is a Right, returns Promise.resolve(val)
+// where val is the unwrapped value in the Either
+const resolveEither = S.either(
+  val => Promise.reject(val),
+  val => Promise.resolve(val)
+);
+
+const handleEmptyJSON = json => (
+  json.length === 0 ? JSON.stringify({}) : json
+);
+
 /**
  * Calls to fetch but does a few other things:
  *  - turn cookies on for this domain
@@ -94,10 +108,6 @@ import { fetchWithCSRF } from './api';
  */
 const _fetchJSONWithCSRF = (input: string, init: Object = {}, loginOnError: boolean = false): Promise<*> => {
   return fetch(input, formatJSONRequest(init)).then(response => {
-    // Not using response.json() here since it doesn't handle empty responses
-    // Also note that text is a promise here, not a string
-    let text = response.text();
-
     // For 400 and 401 errors, force login
     // the 400 error comes from edX in case there are problems with the refresh
     // token because the data stored locally is wrong and the solution is only
@@ -108,34 +118,21 @@ const _fetchJSONWithCSRF = (input: string, init: Object = {}, loginOnError: bool
       window.location = `/logout?next=${encodeURIComponent(loginRedirect)}`;
     }
 
-    // For non 2xx status codes reject the promise adding the status code
-    if (response.status < 200 || response.status >= 300) {
-      return text.then(text => {
-        return Promise.reject([text, response.status]);
-      });
-    }
-
-    return text;
-  }).then(text => {
-    if (text.length !== 0) {
-      return JSON.parse(text);
-    } else {
-      return "";
-    }
-  }).catch(([text, statusCode]) => {
-    let respJson = {};
-    if (text.length !== 0) {
-      try {
-        respJson = JSON.parse(text);
-      }
-      catch(e) {
-        // If the JSON.parse raises, it means that the backend sent a JSON invalid
-        // string, and in this context the content received is not important
-        // and can be discarded
-      }
-    }
-    respJson.errorStatusCode = statusCode;
-    return Promise.reject(respJson);
+    // Here in the .then callback we use the `parseJSON` function, which returns an Either.
+    // Left records an error parsing the JSON, and Right success. `filterE` will turn a Right
+    // into a Left based on a boolean function (similar to filtering a Maybe), and we use `bimap`
+    // to merge an error code into a Left. The `resolveEither` function above will resolve a Right
+    // and reject a Left.
+    return response.text().then(R.compose(
+      resolveEither,
+      S.bimap(
+        R.merge({ errorStatusCode: response.status }),
+        R.identity,
+      ),
+      filterE(() => response.ok),
+      parseJSON,
+      handleEmptyJSON,
+    ));
   });
 };
 

static/js/lib/api_test.js

@@ -654,7 +654,7 @@ describe('api', function() {
           method: 'PATCH',
           body: JSON.stringify(expectedJSON)
         }).then(responseBody => {
-          assert.deepEqual(responseBody, '');
+          assert.deepEqual(responseBody, {});
         });
       });
 

static/js/lib/redux_rest.js

@@ -89,7 +89,7 @@ export const deriveAction = (endpoint: Endpoint, verb: string): DerivedAction =>
         return fetchFunc(...params).then(data => {
           dispatch(successAction(data));
           return Promise.resolve(data);
-        }).catch(error => {
+        }, error => {
           dispatch(failureAction(error));
           return Promise.reject(error);
         });

static/js/lib/sanctuary.js

@@ -44,3 +44,20 @@ export const guard = (func: Function) => (...args: any) => {
 export const getm = R.curry((prop, obj) => (
   S.toMaybe(R.prop(prop, obj))
 ));
+
+// parseJSON :: String -> Either Object Object
+// A Right value indicates the JSON parsed successfully,
+// a Left value indicates the JSON was malformed (a Left contains
+// an empty object)
+export const parseJSON = S.encaseEither(() => ({}), JSON.parse);
+
+// filterE :: (Either -> Boolean) -> Either -> Either
+// filterE takes a function f and an either E(v).
+// if the Either is a Left, it returns it.
+// if the f(v) === true, it returns, E. Else,
+// if returns Left(v).
+export const filterE = R.curry((predicate, either) => S.either(
+  S.Left,
+  right => predicate(right) ? S.Right(right) : S.Left(right),
+  either
+));