Simplify Bulk Management Config (#200)

* refactor: simplify bulk management enabling

Formerly, a course had to have bulk management enabled and have a master's
track. This painted us into a corner where we had to create a
workaround for enabling bulk management on non-master's track courses.
Instead, this relies only on an enabled flag from edx-platform (based on
a course waffle flag) which simplifies the enabling code here.

* feat: remove unneeded bulk management allow-list

.env

@@ -30,4 +30,3 @@ ENTERPRISE_MARKETING_URL=''
 ENTERPRISE_MARKETING_UTM_SOURCE=''
 ENTERPRISE_MARKETING_UTM_CAMPAIGN=''
 ENTERPRISE_MARKETING_FOOTER_UTM_MEDIUM=''
-BULK_MANAGEMENT_SPECIAL_ACCESS_COURSE_IDS=''

.env.development

@@ -36,4 +36,3 @@ ENTERPRISE_MARKETING_URL='http://example.com'
 ENTERPRISE_MARKETING_UTM_SOURCE='example.com'
 ENTERPRISE_MARKETING_UTM_CAMPAIGN='example.com Referral'
 ENTERPRISE_MARKETING_FOOTER_UTM_MEDIUM='Footer'
-BULK_MANAGEMENT_SPECIAL_ACCESS_COURSE_IDS=''

documentation/testing/test-plan.md

@@ -68,7 +68,7 @@ Confirm the following workflows:
     - [ ] Clicking "Save Grade" applies the override, shows the successful "grade has been edited" banner and updates score in grades table (may take a few seconds).
     - [ ] Opening back up the "Edit Grades" modal shows the change as an entry in the override history table.
 
-- [ ] *Masters only*: "Bulk Management" allows overriding grades in bulk.
+- [ ] *Master's (or selectively-enabled) only*: "Bulk Management" allows overriding grades in bulk.
     - Open a non-masters-track course.
     - [ ] Verify that the "Bulk Management" tab does not appear.
     - [ ] Verify that the "Bulk Management" button does not appear.

documentation/testing/test-setup.md

@@ -14,7 +14,7 @@ Suggested resources:
 - [Adding Exercises and Tools](https://edx.readthedocs.io/projects/edx-partner-course-staff/en/latest/grading/index.html)
 - [Set the Assignment Type and Due Date for a Subsection](https://edx.readthedocs.io/projects/edx-partner-course-staff/en/latest/developing_course/course_subsections.html#set-the-assignment-type-and-due-date-for-a-subsection)
 
-## Enable Gradebook and feature toggles for course
+## Enable Gradebook for course
 
 See README.md #Quickstart for more detailed instructions.
 
@@ -25,7 +25,13 @@ As an admin user, visit Django Admin (`{lms-url}/admin`) to modify features.
     - [ ] Set name to `grades.assume_zero_grade_if_absent`, select "Active", and click "Save"
 - In Waffle_Utils > Waffle flag course overrides:
     - [ ] Add a new flag called `grades.writeable_gradebook`, select "Force On", and enable it for your course
-    - [ ] Add a new flag called `grades.bulk_management`, select "Force On", and enable it for your course
+
+## Enable Bulk Management
+
+Bulk Management is an added feature to allow modifying grades in bulk via CSV upload. Bulk Management is default enabled for Master's track courses but can be selectively enabled for other courses with a waffle flag following the steps below.
+
+- In Waffle_Utils > Waffle flag course overrides:
+    - [ ] Add a new flag called `grades.bulk_management`, select "Force On", and enable it for your course.
 
 ## Create a Master's track for testing Master's-only features
 

package-lock.json

@@ -1,6 +1,6 @@
 {
   "name": "@edx/frontend-app-gradebook",
-  "version": "1.4.43",
+  "version": "1.4.46",
   "lockfileVersion": 1,
   "requires": true,
   "dependencies": {

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@edx/frontend-app-gradebook",
-  "version": "1.4.43",
+  "version": "1.4.46",
   "description": "edx editable gradebook-ui to manipulate grade overrides on subsections",
   "repository": {
     "type": "git",

src/data/constants/filters.js

@@ -12,6 +12,7 @@ export const filters = StrictDict({
   courseGrade: 'courseGrade',
   courseGradeMax: 'courseGradeMax',
   courseGradeMin: 'courseGradeMin',
+  excludedCourseRoles: 'excludedCourseRoles',
   includeCourseRoleMembers: 'includeCourseRoleMembers',
   track: 'track',
 });

src/data/selectors/filters.js

@@ -132,6 +132,16 @@ export const selectedAssignmentId = (state) => (simpleSelectors.assignment(state
  */
 export const selectedAssignmentLabel = (state) => (simpleSelectors.assignment(state) || {}).label;
 
+/**
+ * Returns the api value for excludedCourseRoles based on the
+ * internal Bool value for includeCourseRoleMembers.
+ * @param {object} state - redux state
+ * @return {string} - '' if to be included, else 'all'
+ */
+export const excludedCourseRoles = (state) => (
+  simpleSelectors.includeCourseRoleMembers(state) ? '' : 'all'
+);
+
 export default StrictDict({
   ...simpleSelectors,
   isDefault,
@@ -143,5 +153,6 @@ export default StrictDict({
   allFilters,
   areAssignmentGradeFiltersSet,
   chooseRelevantAssignmentData,
+  excludedCourseRoles,
   getAssignmentsFromResultsSubstate,
 });

src/data/selectors/filters.test.js

@@ -139,6 +139,15 @@ describe('filters selectors', () => {
     });
   });
 
+  describe('excludedCourseRoles', () => {
+    it('returns empty string if includeCourseRoleMembers', () => {
+      expect(selectors.excludedCourseRoles({ filters: { includeCourseRoleMembers: true } })).toEqual('');
+    });
+    it('returns "all" string if not includeCourseRoleMembers', () => {
+      expect(selectors.excludedCourseRoles({ filters: { includeCourseRoleMembers: false } })).toEqual('all');
+    });
+  });
+
   describe('selectedAssignmentId', () => {
     it('gets filtered assignment ID when available', () => {
       const assignmentId = selectors.selectedAssignmentId(testState);

src/data/selectors/index.js

@@ -10,7 +10,6 @@ import cohorts from './cohorts';
 import filters from './filters';
 import grades, { minGrade, maxGrade } from './grades';
 import roles from './roles';
-import special from './special';
 import tracks from './tracks';
 
 const {
@@ -127,10 +126,7 @@ export const getHeadings = (state) => grades.headingMapper(
  * @return {string} - generated grade export url
  */
 export const gradeExportUrl = (state) => (
-  lms.urls.gradeCsvUrl({
-    ...module.lmsApiServiceArgs(state),
-    excludeCourseRoles: filters.includeCourseRoleMembers(state) ? '' : 'all',
-  })
+  lms.urls.gradeCsvUrl(module.lmsApiServiceArgs(state))
 );
 
 /**
@@ -140,9 +136,7 @@ export const gradeExportUrl = (state) => (
  * @return {string} - generated intervention export url
  */
 export const interventionExportUrl = (state) => (
-  lms.urls.interventionExportCsvUrl(
-    module.lmsApiServiceArgs(state),
-  )
+  lms.urls.interventionExportCsvUrl(module.lmsApiServiceArgs(state))
 );
 
 /**
@@ -153,6 +147,7 @@ export const interventionExportUrl = (state) => (
  */
 export const lmsApiServiceArgs = (state) => ({
   cohort: cohorts.getCohortNameById(state, filters.cohort(state)),
+  track: filters.track(state),
   assignment: filters.selectedAssignmentId(state),
   assignmentType: filters.assignmentType(state),
   assignmentGradeMin: grades.formatMinAssignmentGrade(
@@ -165,6 +160,7 @@ export const lmsApiServiceArgs = (state) => ({
   ),
   courseGradeMin: grades.formatMinCourseGrade(filters.courseGradeMin(state)),
   courseGradeMax: grades.formatMaxCourseGrade(filters.courseGradeMax(state)),
+  excludedCourseRoles: filters.excludedCourseRoles(state),
 });
 
 /**
@@ -221,15 +217,11 @@ export const shouldShowSpinner = (state) => (
 
 /**
  * showBulkManagement(state, options)
- * Returns true iff the user has special access or bulk management is configured to be available
- * and the course has a masters track.
+ * Returns true iff the course has bulk management enabled
  * @param {object} state - redux state
  * @return {bool} - should show bulk management controls?
  */
-export const showBulkManagement = (state) => (
-  special.hasSpecialBulkManagementAccess(app.courseId(state))
-  || (tracks.stateHasMastersTrack(state) && state.config.bulkManagementAvailable)
-);
+export const showBulkManagement = (state) => (state.config.bulkManagementAvailable);
 
 export default StrictDict({
   root: StrictDict({
@@ -250,6 +242,5 @@ export default StrictDict({
   filters,
   grades,
   roles,
-  special,
   tracks,
 });

src/data/selectors/index.test.js

@@ -339,24 +339,11 @@ describe('root selectors', () => {
     afterEach(() => {
       moduleSelectors.lmsApiServiceArgs = lmsApiServiceArgs;
     });
-    describe('without includeCourseRoleMembers filter', () => {
-      it('calls the API service with the right args, excluding all course roles', () => {
-        selectors.filters.includeCourseRoleMembers.mockReturnValue(undefined);
-        expect(selector(testState)).toEqual({
-          gradeCsvUrl: {
-            args: [{ lmsArgs: testState, excludeCourseRoles: 'all' }],
-          },
-        });
-      });
-    });
-    describe('with includeCourseRoleMembers filter', () => {
-      it('calls the API service with the right args, including course roles', () => {
-        selectors.filters.includeCourseRoleMembers.mockReturnValue(true);
-        expect(selector(testState)).toEqual({
-          gradeCsvUrl: {
-            args: [{ lmsArgs: testState, excludeCourseRoles: '' }],
-          },
-        });
+    it('calls the API service with the right args', () => {
+      expect(selector(testState)).toEqual({
+        gradeCsvUrl: {
+          args: [{ lmsArgs: testState }],
+        },
       });
     });
   });
@@ -381,10 +368,12 @@ describe('root selectors', () => {
       selectors.filters.selectedAssignmentId = mockFn('selectedAssignmentId');
       selectors.filters.assignmentType = mockFn('assignmentType');
       selectors.filters.cohort = mockFn('cohort');
+      selectors.filters.track = mockFn('track');
       selectors.filters.assignmentGradeMax = mockFn('assignmentGradeMax');
       selectors.filters.assignmentGradeMin = mockFn('assignmentGradeMin');
       selectors.filters.courseGradeMax = mockFn('courseGradeMax');
       selectors.filters.courseGradeMin = mockFn('courseGradeMin');
+      selectors.filters.excludedCourseRoles = mockFn('excludedCourseRoles');
       selectors.grades.formatMaxAssignmentGrade = mockMetaFn('formatMaxAssignmentGrade');
       selectors.grades.formatMinAssignmentGrade = mockMetaFn('formatMinAssignmentGrade');
       selectors.grades.formatMinCourseGrade = mockFn('formatMinCourseGrade');
@@ -392,6 +381,7 @@ describe('root selectors', () => {
       const assignmentId = { selectedAssignmentId: testState };
       expect(moduleSelectors.lmsApiServiceArgs(testState)).toEqual({
         cohort: { getCohortNameById: testState },
+        track: { track: testState },
         assignment: assignmentId,
         assignmentType: { assignmentType: testState },
         assignmentGradeMin: {
@@ -410,6 +400,7 @@ describe('root selectors', () => {
         courseGradeMax: {
           formatMaxCourseGrade: { courseGradeMax: testState },
         },
+        excludedCourseRoles: { excludedCourseRoles: testState },
       });
     });
   });
@@ -502,43 +493,13 @@ describe('root selectors', () => {
     });
   });
   describe('showBulkManagement', () => {
-    const mockAccess = (val) => {
-      selectors.special.hasSpecialBulkManagementAccess = jest.fn(() => val);
-    };
-    const mockHasMastersTrack = (val) => {
-      selectors.tracks.stateHasMastersTrack = jest.fn(() => val);
-    };
     const selector = moduleSelectors.showBulkManagement;
     const mkState = (bulkManagementAvailable) => ({ config: { bulkManagementAvailable } });
-    describe('user has special bulk management access', () => {
-      it('returns true', () => {
-        mockAccess(true);
-        mockHasMastersTrack(false);
-        expect(selector(mkState(true))).toEqual(true);
-      });
+    it('returns true when bulk management is enabled for the course', () => {
+      expect(selector(mkState(true))).toEqual(true);
     });
-    describe('user does not have special access', () => {
-      beforeEach(() => {
-        mockAccess(false);
-      });
-      describe('course has a masters track, but bulkManagement not available', () => {
-        it('returns false', () => {
-          mockHasMastersTrack(true);
-          expect(selector(mkState(false))).toEqual(false);
-        });
-      });
-      describe('course does not have a masters track, but bulkManagement available', () => {
-        it('returns false', () => {
-          mockHasMastersTrack(false);
-          expect(selector(mkState(true))).toEqual(false);
-        });
-      });
-      describe('course has a masters track, and bulkManagement is available', () => {
-        it('returns false', () => {
-          mockHasMastersTrack(true);
-          expect(selector(mkState(true))).toEqual(true);
-        });
-      });
+    it('returns false when bulk management is not enabled for the course', () => {
+      expect(selector(mkState(false))).toEqual(false);
     });
   });
 });

src/data/selectors/special.js

@@ -1,22 +0,0 @@
-import { StrictDict } from 'utils';
-
-// Certain course runs may be expressly allowed to view the
-// bulk management tools, bypassing the other checks.
-// Note that this does not affect whether or not the backend
-// LMS API will permit usage of the tool.
-
-/**
-* hasSpecialBulkManagementAccess(courseId)
-* Returns true iff the bulk management special access course ids env variable includes
-* the linked course id.
-* @param {string} courseId - linked course id
-* @param {bool} - course has special bulk management access?
-*/
-const hasSpecialBulkManagementAccess = (courseId) => {
-  const specialIdList = process.env.BULK_MANAGEMENT_SPECIAL_ACCESS_COURSE_IDS || '';
-  return specialIdList.split(',').includes(courseId);
-};
-
-export default StrictDict({
-  hasSpecialBulkManagementAccess,
-});

src/data/selectors/special.test.js

@@ -1,27 +0,0 @@
-import selectors from './special';
-
-describe('hasSpecialBulkManagementAccess', () => {
-  // Copy & restore process for testing purposes
-  const OLD_ENV = process.env;
-  const allowedCourses = ['edX/DemoX/2021T1', 'edX/DemoX/2021T2'];
-  const nonSpecialAccessCourse = 'edx/normal/course';
-
-  beforeEach(() => {
-    process.env = { ...OLD_ENV };
-  });
-
-  afterAll(() => {
-    process.env = OLD_ENV;
-  });
-
-  it('returns true if the course has special access to bulk management', () => {
-    process.env.BULK_MANAGEMENT_SPECIAL_ACCESS_COURSE_IDS = `${allowedCourses.join(',')}`;
-    const hasSpecialBulkManagementAccess = selectors.hasSpecialBulkManagementAccess(allowedCourses[0]);
-    expect(hasSpecialBulkManagementAccess).toBeTruthy();
-  });
-
-  it('returns false if the course does not have special access to bulk management', () => {
-    const hasSpecialBulkManagementAccess = selectors.hasSpecialBulkManagementAccess(nonSpecialAccessCourse);
-    expect(hasSpecialBulkManagementAccess).toBeFalsy();
-  });
-});

src/data/services/lms/urls.js

@@ -15,7 +15,7 @@ const gradebook = `${grades}gradebook/${courseId}/`;
 const bulkUpdate = `${gradebook}bulk-update`;
 const intervention = `${bulkGrades}intervention/`;
 
-const cohorts = `${baseUrl}courses/${courseId}/cohorts/`;
+const cohorts = `${baseUrl}/courses/${courseId}/cohorts/`;
 const tracks = `${enrollment}course/${courseId}?include_expired=1`;
 const bulkHistory = `${bulkGrades}history/`;