Merge pull request #78 from edx/schen/EDUCATOR-3925

fix(bug): make sure gradebook rounding handle null input

.eslintignore

@@ -1,3 +1,4 @@
 coverage/*
 dist/
 node_modules/
+src/segment.js

config/webpack.common.config.js

@@ -4,6 +4,7 @@ const path = require('path');
 
 module.exports = {
   entry: {
+    segment: path.resolve(__dirname, '../src/segment.js'),
     app: path.resolve(__dirname, '../src/index.jsx'),
   },
   output: {

config/webpack.dev.config.js

@@ -12,6 +12,7 @@ module.exports = Merge.smart(commonConfig, {
   entry: [
     // enable react's custom hot dev client so we get errors reported in the browser
     require.resolve('react-dev-utils/webpackHotDevClient'),
+    path.resolve(__dirname, '../src/segment.js'),
     path.resolve(__dirname, '../src/index.jsx'),
   ],
   module: {

documentation/decisions/0001-update-api-usage.rst

@@ -0,0 +1,46 @@
+Usage of the bulk-update API
+============================
+
+Context
+=======
+
+The LMS Grades API exposes a set of Gradebook-related endpoints:
+https://github.com/edx/edx-platform/blob/master/lms/djangoapps/grades/api/v1/gradebook_views.py
+The ``bulk-update`` endpoint defined therein allows for the creation/modification of subsection
+grades for multiple users and sections in a single request.  This allows clients of the API to limit
+the number of network requests made and to more easily manage client-side data.  Moreover,
+the course grade updates that occur during calls to this API are synchronous - the entire update operation
+is completed before a response is given to the client.
+
+For decisions made about the implementation of this API, see:
+https://github.com/edx/edx-platform/blob/master/lms/djangoapps/grades/docs/decisions/0001-gradebook-api.rst
+
+Decision
+========
+
+The Gradebook front-end will post data about a single subsection and user in a single request
+to the ``bulk-update`` API.  That is, we currently need only the "update" aspect of this
+endpoint, and not the "bulk" aspect, for satisfying the requirements of the current UX.
+
+Status
+======
+
+Accepted (circa December 2018)
+
+Consequences
+============
+
+This is a scenario in which the implementation of the API is coupled to the
+UX that depends on the API.  Because the course grade update is synchronous, it means
+the API response can contain the updated subsection and course grade data.  Because
+a response from the API contains this data, the UI can operate in a very familiar way:
+
+- A user clicks a button to submit a request with grade update data to the update endpoint.
+- On the server, the subsection and course grades are modified.
+- In the meantime, the client-side user looks at a spinner.
+- A response is returned with updated data and the spinner goes away.
+- Updated data is displayed to the user, along with a message indicative of the update.
+
+If the update becomes asynchronous, the user experience outlined above has to change.
+Because a single call to this endpoint updates grades data for only a single user,
+the endpoint does not necessarily have to utilize an asynchronous operation at this time.

package.json

@@ -25,67 +25,69 @@
   },
   "dependencies": {
     "@edx/edx-bootstrap": "^0.4.3",
-    "@edx/frontend-auth": "^1.2.1",
+    "@edx/frontend-auth": "^1.3.0",
     "@edx/frontend-component-footer": "^1.0.0",
-    "@edx/paragon": "^3.7.2",
+    "@edx/paragon": "^3.8.3",
+    "@redux-beacon/segment": "^1.0.0",
     "babel-polyfill": "^6.26.0",
-    "classnames": "^2.2.5",
-    "email-prop-type": "^1.1.5",
+    "classnames": "^2.2.6",
+    "email-prop-type": "^1.1.7",
     "font-awesome": "^4.7.0",
     "history": "^4.7.2",
-    "prop-types": "^15.5.10",
+    "prop-types": "^15.6.2",
     "query-string": "^5.1.1",
-    "react": "^16.2.0",
-    "react-dom": "^16.2.0",
-    "react-redux": "^5.0.7",
-    "react-router": "^4.2.0",
-    "react-router-dom": "^4.2.2",
+    "react": "^16.7.0",
+    "react-dom": "^16.7.0",
+    "react-redux": "^5.1.1",
+    "react-router": "^4.3.1",
+    "react-router-dom": "^4.3.1",
     "react-router-redux": "^5.0.0-alpha.9",
     "redux": "^3.7.2",
-    "redux-devtools-extension": "^2.13.2",
+    "redux-devtools-extension": "^2.13.7",
+    "redux-beacon": "^2.0.3",
     "redux-logger": "^3.0.6",
-    "redux-thunk": "^2.2.0",
-    "whatwg-fetch": "^2.0.3"
+    "redux-thunk": "^2.3.0",
+    "whatwg-fetch": "^2.0.4"
   },
   "devDependencies": {
-    "autoprefixer": "^9.4.2",
-    "axios-mock-adapter": "^1.15.0",
+    "autoprefixer": "^9.4.5",
+    "axios-mock-adapter": "^1.16.0",
     "babel-cli": "^6.26.0",
-    "babel-eslint": "^8.2.2",
-    "babel-jest": "^22.4.0",
-    "babel-loader": "^7.1.2",
+    "babel-eslint": "^8.2.6",
+    "babel-jest": "^22.4.4",
+    "babel-loader": "^7.1.5",
     "babel-plugin-transform-class-properties": "^6.24.1",
     "babel-plugin-transform-object-rest-spread": "^6.26.0",
-    "babel-preset-env": "^1.6.1",
+    "babel-preset-env": "^1.7.0",
     "babel-preset-react": "^6.24.1",
-    "codecov": "^3.0.0",
-    "css-loader": "^0.28.9",
-    "enzyme": "^3.3.0",
-    "enzyme-adapter-react-16": "^1.1.1",
-    "es-check": "^2.0.2",
-    "eslint-config-edx": "^4.0.3",
-    "fetch-mock": "^6.3.0",
+    "codecov": "^3.1.0",
+    "css-loader": "^0.28.11",
+    "enzyme": "^3.8.0",
+    "enzyme-adapter-react-16": "^1.7.1",
+    "es-check": "^2.3.0",
+    "eslint-config-edx": "^4.0.4",
+    "fetch-mock": "^6.5.2",
     "file-loader": "^1.1.9",
     "html-webpack-harddisk-plugin": "^0.2.0",
-    "html-webpack-plugin": "^3.0.3",
+    "html-webpack-plugin": "^3.2.0",
     "husky": "^0.14.3",
     "identity-obj-proxy": "^3.0.0",
     "image-webpack-loader": "^4.2.0",
-    "jest": "^22.4.0",
+    "jest": "^22.4.4",
     "mini-css-extract-plugin": "^0.4.0",
-    "node-sass": "^4.7.2",
+    "node-sass": "^4.11.0",
     "postcss-loader": "^3.0.0",
-    "react-dev-utils": "^5.0.0",
-    "react-test-renderer": "^16.2.0",
-    "redux-mock-store": "^1.5.1",
+    "react-dev-utils": "^5.0.3",
+    "react-test-renderer": "^16.7.0",
+    "redux-mock-store": "^1.5.3",
     "sass-loader": "^6.0.6",
-    "semantic-release": "^15.10.7",
-    "style-loader": "^0.20.2",
-    "travis-deploy-once": "^5.0.9",
-    "webpack": "^4.25.1",
-    "webpack-cli": "^3.1.2",
-    "webpack-dev-server": "^3.1.0",
-    "webpack-merge": "^4.1.1"
+    "semantic-release": "^15.13.3",
+    "style-loader": "^0.20.3",
+    "travis-deploy-once": "^5.0.11",
+    "webpack": "^4.28.4",
+    "webpack-cli": "^3.2.1",
+    "webpack-dev-server": "^3.1.14",
+    "webpack-merge": "^4.2.1"
   },
   "jest": {
     "setupFiles": [
@@ -106,6 +108,7 @@
     ],
     "transformIgnorePatterns": [
       "/node_modules/(?!(@edx/paragon)/).*/"
-    ]
+    ],
+    "testURL": "http://localhost"
   }
 }

src/components/Gradebook/gradebook.scss

@@ -68,6 +68,10 @@
         padding-left: 170px;
     }
 
+    .table tbody th {
+        font-weight: normal;
+    }
+
     .link-style {
         color: #0075b4;
         &:hover, &:focus {

src/components/Gradebook/index.jsx

@@ -176,7 +176,7 @@ export default class Gradebook extends React.Component {
     return 'Tracks';
   };
 
-  roundGrade = percent => parseFloat(percent.toFixed(DECIMAL_PRECISION));
+  roundGrade = percent => parseFloat((percent || 0).toFixed(DECIMAL_PRECISION));
 
   formatter = {
     percent: (entries, areGradesFrozen) => entries.map((entry) => {
@@ -244,7 +244,7 @@ export default class Gradebook extends React.Component {
               href={this.lmsInstructorDashboardUrl(this.props.match.params.courseId)}
               className="mb-3"
             >
-              {'<< Back to Dashboard'}
+              <span aria-hidden="true">{'<< '}</span> {'Back to Dashboard'}
             </a>
             <h1>Gradebook</h1>
             <h3> {this.props.match.params.courseId}</h3>
@@ -261,8 +261,8 @@ export default class Gradebook extends React.Component {
             <hr />
             <div className="d-flex justify-content-between" >
               <div>
-                <div>
-                  Score View:
+                <div role="radiogroup" aria-labelledby="score-view-group-label">
+                  <span id="score-view-group-label">Score View:</span>
                   <span>
                     <input
                       id="score-view-percent"
@@ -294,6 +294,7 @@ export default class Gradebook extends React.Component {
                     </span>
                     <InputSelect
                       name="assignment-types"
+                      ariaLabel="Assignment Types"
                       value={this.mapSelectedTrackEntry(this.props.selectedAssignmentType)}
                       options={this.mapAssignmentTypeEntries(this.props.assignmnetTypes)}
                       onChange={this.updateAssignmentTypes}
@@ -306,6 +307,7 @@ export default class Gradebook extends React.Component {
                   </span>
                   <InputSelect
                     name="Tracks"
+                    ariaLabel="Tracks"
                     disabled={this.props.tracks.length === 0}
                     value={this.mapSelectedTrackEntry(this.props.selectedTrack)}
                     options={this.mapTracksEntries(this.props.tracks)}
@@ -313,6 +315,7 @@ export default class Gradebook extends React.Component {
                   />
                   <InputSelect
                     name="Cohorts"
+                    ariaLabel="Cohorts"
                     disabled={this.props.cohorts.length === 0}
                     value={this.mapSelectedCohortEntry(this.props.selectedCohort)}
                     options={this.mapCohortsEntries(this.props.cohorts)}
@@ -347,6 +350,7 @@ export default class Gradebook extends React.Component {
                 tableSortable
                 defaultSortDirection="asc"
                 defaultSortedColumn="username"
+                rowHeaderColumnKey="username"
               />
             </div>
             {PageButtons(this.props)}

src/data/actions/roles.js

@@ -10,7 +10,11 @@ import LmsApiService from '../services/LmsApiService';
 
 const allowedRoles = ['staff', 'instructor', 'support'];
 
-const gotRoles = canUserViewGradebook => ({ type: GOT_ROLES, canUserViewGradebook });
+const gotRoles = (canUserViewGradebook, courseId) => ({
+  type: GOT_ROLES,
+  canUserViewGradebook,
+  courseId,
+});
 const errorFetchingRoles = () => ({ type: ERROR_FETCHING_ROLES });
 
 const getRoles = (courseId, urlQuery) => (
@@ -20,7 +24,7 @@ const getRoles = (courseId, urlQuery) => (
       const canUserViewGradebook = response.is_staff
                                   || (response.roles.some(role => (role.course_id === courseId)
                                       && allowedRoles.includes(role.role)));
-      dispatch(gotRoles(canUserViewGradebook));
+      dispatch(gotRoles(canUserViewGradebook, courseId));
       if (canUserViewGradebook) {
         dispatch(fetchGrades(courseId, urlQuery.cohort, urlQuery.track));
         dispatch(fetchTracks(courseId));

src/data/actions/roles.test.js

@@ -22,23 +22,23 @@ const course1Id = 'course-v1:edX+DemoX+Demo_Course';
 const course2Id = 'course-v1:edX+DemoX+Demo_Course_2';
 const rolesUrl = `${configuration.LMS_BASE_URL}/api/enrollment/v1/roles/?course_id=${encodeURIComponent(course1Id)}`;
 
-function makeRoleListObj(roles, isGlobalStaff){
+function makeRoleListObj(roles, isGlobalStaff) {
   return {
-    roles: roles,
+    roles,
     is_staff: isGlobalStaff,
-  }
+  };
 }
 function makeRoleObj(courseId, role) {
   return {
     course_id: courseId,
-    role: role,
-  }
-};
+    role,
+  };
+}
 
-const course1StaffRole = makeRoleObj(course1Id, "staff");
-const course1DummyRole = makeRoleObj(course1Id, "dummy");
-const course2StaffRole = makeRoleObj(course2Id, "staff");
-const course2DummyRole = makeRoleObj(course2Id, "dummy");
+const course1StaffRole = makeRoleObj(course1Id, 'staff');
+const course1DummyRole = makeRoleObj(course1Id, 'dummy');
+const course2StaffRole = makeRoleObj(course2Id, 'staff');
+const course2DummyRole = makeRoleObj(course2Id, 'dummy');
 const urlParams = { cohort: null, track: null };
 
 describe('actions', () => {
@@ -49,7 +49,7 @@ describe('actions', () => {
   describe('getRoles', () => {
     it('dispatches got_roles action and subsequent actions after fetching role that allows gradebook', () => {
       const expectedActions = [
-        { type: GOT_ROLES, canUserViewGradebook: true },
+        { type: GOT_ROLES, canUserViewGradebook: true, courseId: course1Id },
         { type: STARTED_FETCHING_GRADES },
         { type: STARTED_FETCHING_TRACKS },
         { type: STARTED_FETCHING_COHORTS },
@@ -57,7 +57,7 @@ describe('actions', () => {
       ];
       const store = mockStore();
       axiosMock.onGet(rolesUrl)
-      .replyOnce(200, JSON.stringify(makeRoleListObj([course1StaffRole, course2DummyRole], false)));
+        .replyOnce(200, JSON.stringify(makeRoleListObj([course1StaffRole, course2DummyRole], false)));
 
       return store.dispatch(getRoles(course1Id, urlParams)).then(() => {
         expect(store.getActions()).toEqual(expectedActions);
@@ -66,7 +66,7 @@ describe('actions', () => {
 
     it('dispatches got_roles action and other actions after fetching irrelevent roles but user is global staff', () => {
       const expectedActions = [
-        { type: GOT_ROLES, canUserViewGradebook: true },
+        { type: GOT_ROLES, canUserViewGradebook: true, courseId: course1Id },
         { type: STARTED_FETCHING_GRADES },
         { type: STARTED_FETCHING_TRACKS },
         { type: STARTED_FETCHING_COHORTS },
@@ -75,7 +75,7 @@ describe('actions', () => {
       const store = mockStore();
 
       axiosMock.onGet(rolesUrl)
-      .replyOnce(200, JSON.stringify(makeRoleListObj([course1DummyRole, course2DummyRole], true)));
+        .replyOnce(200, JSON.stringify(makeRoleListObj([course1DummyRole, course2DummyRole], true)));
 
       return store.dispatch(getRoles(course1Id, urlParams)).then(() => {
         expect(store.getActions()).toEqual(expectedActions);
@@ -84,12 +84,14 @@ describe('actions', () => {
 
     it('dispatches got_roles action and no other actions after fetching role that disallows gradebook', () => {
       const expectedActions = [
-        { type: GOT_ROLES, canUserViewGradebook: false },
+        {
+          type: GOT_ROLES, canUserViewGradebook: false, courseId: course1Id,
+        },
       ];
       const store = mockStore();
 
       axiosMock.onGet(rolesUrl)
-      .replyOnce(200, JSON.stringify(makeRoleListObj([course1DummyRole, course2StaffRole], false)));
+        .replyOnce(200, JSON.stringify(makeRoleListObj([course1DummyRole, course2StaffRole], false)));
 
       return store.dispatch(getRoles(course1Id, urlParams)).then(() => {
         expect(store.getActions()).toEqual(expectedActions);
@@ -98,12 +100,12 @@ describe('actions', () => {
 
     it('dispatches got_roles action and no other actions after fetching empty roles', () => {
       const expectedActions = [
-        { type: GOT_ROLES, canUserViewGradebook: false },
+        { type: GOT_ROLES, canUserViewGradebook: false, courseId: course1Id },
       ];
       const store = mockStore();
 
       axiosMock.onGet(rolesUrl)
-      .replyOnce(200, JSON.stringify(makeRoleListObj([], false)));
+        .replyOnce(200, JSON.stringify(makeRoleListObj([], false)));
 
       return store.dispatch(getRoles(course1Id, urlParams)).then(() => {
         expect(store.getActions()).toEqual(expectedActions);
@@ -112,7 +114,7 @@ describe('actions', () => {
 
     it('dispatches got_roles action and other actions after fetching empty roles but user is global staff', () => {
       const expectedActions = [
-        { type: GOT_ROLES, canUserViewGradebook: true },
+        { type: GOT_ROLES, canUserViewGradebook: true, courseId: course1Id },
         { type: STARTED_FETCHING_GRADES },
         { type: STARTED_FETCHING_TRACKS },
         { type: STARTED_FETCHING_COHORTS },
@@ -121,7 +123,7 @@ describe('actions', () => {
       const store = mockStore();
 
       axiosMock.onGet(rolesUrl)
-      .replyOnce(200, JSON.stringify(makeRoleListObj([], true)));
+        .replyOnce(200, JSON.stringify(makeRoleListObj([], true)));
 
       return store.dispatch(getRoles(course1Id, urlParams)).then(() => {
         expect(store.getActions()).toEqual(expectedActions);

src/data/store.js

@@ -2,14 +2,48 @@ import { applyMiddleware, createStore } from 'redux';
 import thunkMiddleware from 'redux-thunk';
 import { composeWithDevTools } from 'redux-devtools-extension/logOnlyInProduction';
 import { createLogger } from 'redux-logger';
+import { createMiddleware } from 'redux-beacon';
+import Segment, { trackEvent, trackPageView } from '@redux-beacon/segment';
+import { GOT_ROLES } from './constants/actionTypes/roles';
+import { GOT_GRADES, GRADE_UPDATE_SUCCESS, GRADE_UPDATE_FAILURE } from './constants/actionTypes/grades';
 
 import reducers from './reducers';
 
 const loggerMiddleware = createLogger();
 
+const eventsMap = {
+  [GOT_ROLES]: trackPageView(action => ({
+    page: action.courseId,
+  })),
+  [GOT_GRADES]: trackEvent(action => ({
+    name: 'Grades displayed or paginated',
+    properties: {
+      track: action.track,
+      cohort: action.cohort,
+      prev: action.prev,
+      next: action.next,
+    },
+  })),
+  [GRADE_UPDATE_SUCCESS]: trackEvent(action => ({
+    name: 'Grades Updated',
+    properties: {
+      updatedGrades: action.payload.responseData,
+    },
+  })),
+  [GRADE_UPDATE_FAILURE]: trackEvent(action => ({
+    name: 'Grades Fail to Update',
+    properties: {
+      error: action.payload.error,
+    },
+  })),
+};
+
+const segmentMiddleware = createMiddleware(eventsMap, Segment());
+
+
 const store = createStore(
   reducers,
-  composeWithDevTools(applyMiddleware(thunkMiddleware, loggerMiddleware)),
+  composeWithDevTools(applyMiddleware(thunkMiddleware, loggerMiddleware, segmentMiddleware)),
 );
 
 export default store;

src/segment.js

@@ -0,0 +1,85 @@
+// The code in this file is from Segment's website:
+// https://segment.com/docs/sources/website/analytics.js/quickstart/
+import { configuration } from './config';
+
+(function () {
+  // Create a queue, but don't obliterate an existing one!
+  const analytics = window.analytics = window.analytics || [];
+
+  // If the real analytics.js is already on the page return.
+  if (analytics.initialize) return;
+
+  // If the snippet was invoked already show an error.
+  if (analytics.invoked) {
+    if (window.console && console.error) {
+      console.error('Segment snippet included twice.');
+    }
+    return;
+  }
+
+  // Invoked flag, to make sure the snippet
+  // is never invoked twice.
+  analytics.invoked = true;
+
+  // A list of the methods in Analytics.js to stub.
+  analytics.methods = [
+    'trackSubmit',
+    'trackClick',
+    'trackLink',
+    'trackForm',
+    'pageview',
+    'identify',
+    'reset',
+    'group',
+    'track',
+    'ready',
+    'alias',
+    'debug',
+    'page',
+    'once',
+    'off',
+    'on',
+  ];
+
+  // Define a factory to create stubs. These are placeholders
+  // for methods in Analytics.js so that you never have to wait
+  // for it to load to actually record data. The `method` is
+  // stored as the first argument, so we can replay the data.
+  analytics.factory = function (method) {
+    return function () {
+      const args = Array.prototype.slice.call(arguments);
+      args.unshift(method);
+      analytics.push(args);
+      return analytics;
+    };
+  };
+
+  // For each of our methods, generate a queueing stub.
+  for (let i = 0; i < analytics.methods.length; i++) {
+    const key = analytics.methods[i];
+    analytics[key] = analytics.factory(key);
+  }
+
+  // Define a method to load Analytics.js from our CDN,
+  // and that will be sure to only ever load it once.
+  analytics.load = function (key, options) {
+    // Create an async script element based on your key.
+    const script = document.createElement('script');
+    script.type = 'text/javascript';
+    script.async = true;
+    script.src = `https://cdn.segment.com/analytics.js/v1/${
+      key}/analytics.min.js`;
+
+    // Insert our script next to the first script element.
+    const first = document.getElementsByTagName('script')[0];
+    first.parentNode.insertBefore(script, first);
+    analytics._loadOptions = options;
+  };
+
+  // Add a version to keep track of what's in the wild.
+  analytics.SNIPPET_VERSION = '4.1.0';
+
+  // Load Analytics.js with your key, which will automatically
+  // load the tools you've enabled for your account. Boosh!
+  analytics.load(configuration.SEGMENT_KEY);
+}());