From 7e9261e30f679d38e6a157d3414e72e21c29046d Mon Sep 17 00:00:00 2001 From: Awais Ansari <79941147+awais-ansari@users.noreply.github.com> Date: Tue, 18 Jan 2022 17:16:00 +0500 Subject: [PATCH] =?UTF-8?q?fix:=20divide=20discussions=20by=20cohort=20tog?= =?UTF-8?q?gle=20should=20not=20divide=20course-wid=E2=80=A6=20(#233)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: divide discussions by cohort toggle should not divide course-wide discussions * test: update failed test cases --- .../apps/legacy/LegacyConfigForm.test.jsx | 36 +++++++++++-------- .../discussions/data/api.js | 15 ++++---- .../discussions/data/redux.test.js | 19 ++++++---- .../discussions/factories/mockApiResponses.js | 6 +--- 4 files changed, 43 insertions(+), 33 deletions(-) diff --git a/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.test.jsx b/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.test.jsx index 972f6aaa7..8816729ae 100644 --- a/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.test.jsx +++ b/src/pages-and-resources/discussions/app-config-form/apps/legacy/LegacyConfigForm.test.jsx @@ -29,23 +29,21 @@ import { selectApp } from '../../../data/slice'; import { DivisionSchemes } from '../../../../../data/constants'; const courseId = 'course-v1:edX+TestX+Test_Course'; -const defaultAppConfig = { +const defaultAppConfig = (divideDiscussionIds = []) => ({ id: 'legacy', divideByCohorts: false, divideCourseTopicsByCohorts: false, + alwaysDivideInlineDiscussions: false, discussionTopics: [ { name: 'Edx', id: '13f106c6-6735-4e84-b097-0456cff55960' }, { name: 'General', id: 'course' }, ], - divideDiscussionIds: [ - '13f106c6-6735-4e84-b097-0456cff55960', - 'course', - ], + divideDiscussionIds, allowAnonymousPosts: false, allowAnonymousPostsPeers: false, allowDivisionByUnit: false, blackoutDates: [], -}; +}); describe('LegacyConfigForm', () => { let axiosMock; let store; @@ -111,8 +109,8 @@ describe('LegacyConfigForm', () => { // any of the form inputs, this exact object shape is returned back to us, so we're reusing // it here. It's not supposed to be 'the same object', it just happens to be. { - ...defaultAppConfig, - divideByCohorts: true, + ...defaultAppConfig(), + divideByCohorts: false, divisionScheme: DivisionSchemes.COHORT, }, ); @@ -121,12 +119,14 @@ describe('LegacyConfigForm', () => { test('default field states are correct, including removal of folded sub-fields', async () => { await mockStore({ ...legacyApiResponse, plugin_configuration: { divided_course_wide_discussions: [] } }); createComponent(); + const { divideDiscussionIds } = defaultAppConfig(['13f106c6-6735-4e84-b097-0456cff55960', 'course']); + // DivisionByGroupFields expect(container.querySelector('#divideByCohorts')).toBeInTheDocument(); expect(container.querySelector('#divideByCohorts')).not.toBeChecked(); expect(container.querySelector('#divideCourseTopicsByCohorts')).not.toBeInTheDocument(); - defaultAppConfig.divideDiscussionIds.forEach(id => expect( + divideDiscussionIds.forEach(id => expect( container.querySelector(`#checkbox-${id}`), ).not.toBeInTheDocument()); @@ -144,9 +144,15 @@ describe('LegacyConfigForm', () => { test('folded sub-fields are in the DOM when parents are enabled', async () => { await mockStore({ ...legacyApiResponse, - plugin_configuration: { ...legacyApiResponse.plugin_configuration, allow_anonymous: true }, + plugin_configuration: { + ...legacyApiResponse.plugin_configuration, + allow_anonymous: true, + always_divide_inline_discussions: true, + divided_course_wide_discussions: [], + }, }); createComponent(); + const { divideDiscussionIds } = defaultAppConfig(['13f106c6-6735-4e84-b097-0456cff55960', 'course']); // DivisionByGroupFields expect(container.querySelector('#divideByCohorts')).toBeInTheDocument(); @@ -158,7 +164,7 @@ describe('LegacyConfigForm', () => { container.querySelector('#divideCourseTopicsByCohorts'), ).not.toBeChecked(); - defaultAppConfig.divideDiscussionIds.forEach(id => expect( + divideDiscussionIds.forEach(id => expect( container.querySelector(`#checkbox-${id}`), ).not.toBeInTheDocument()); @@ -179,11 +185,13 @@ describe('LegacyConfigForm', () => { ...legacyApiResponse, plugin_configuration: { ...legacyApiResponse.plugin_configuration, - divided_course_wide_discussions: ['13f106c6-6735-4e84-b097-0456cff55960', - 'course', 'test-topic'], + allow_anonymous: true, + always_divide_inline_discussions: true, + divided_course_wide_discussions: ['13f106c6-6735-4e84-b097-0456cff55960', 'course'], }, }); createComponent(); + const { divideDiscussionIds } = defaultAppConfig(['13f106c6-6735-4e84-b097-0456cff55960', 'course']); // DivisionByGroupFields expect(container.querySelector('#divideByCohorts')).toBeInTheDocument(); @@ -191,7 +199,7 @@ describe('LegacyConfigForm', () => { expect(container.querySelector('#divideCourseTopicsByCohorts')).toBeInTheDocument(); expect(container.querySelector('#divideCourseTopicsByCohorts')).toBeChecked(); - defaultAppConfig.divideDiscussionIds.forEach(id => { + divideDiscussionIds.forEach(id => { expect(container.querySelector(`#checkbox-${id}`)).toBeInTheDocument(); expect(container.querySelector(`#checkbox-${id}`)).toBeChecked(); }); diff --git a/src/pages-and-resources/discussions/data/api.js b/src/pages-and-resources/discussions/data/api.js index 1860f637e..a0c85988f 100644 --- a/src/pages-and-resources/discussions/data/api.js +++ b/src/pages-and-resources/discussions/data/api.js @@ -57,17 +57,16 @@ function normalizePluginConfig(data) { if (!data || Object.keys(data).length < 1) { return {}; } - const discussionDividedTopicsCount = _.size(data.divided_course_wide_discussions); - const discussionTopicsCount = _.size(data.discussion_topics); - const enableDivideCourseTopicsByCohorts = Boolean(discussionDividedTopicsCount - && (discussionDividedTopicsCount !== discussionTopicsCount)); + const enableDivideByCohorts = data.always_divide_inline_discussions && data.division_scheme === 'cohort'; + const enableDivideCourseTopicsByCohorts = enableDivideByCohorts && data.divided_course_wide_discussions.length > 0; return { allowAnonymousPosts: data.allow_anonymous, allowAnonymousPostsPeers: data.allow_anonymous_to_peers, divisionScheme: data.division_scheme, + alwaysDivideInlineDiscussions: data.always_divide_inline_discussions, blackoutDates: normalizeBlackoutDates(data.discussion_blackouts), allowDivisionByUnit: false, - divideByCohorts: discussionDividedTopicsCount > 0, + divideByCohorts: enableDivideByCohorts, divideCourseTopicsByCohorts: enableDivideCourseTopicsByCohorts, }; } @@ -180,6 +179,7 @@ function denormalizeData(courseId, appId, data) { } if ('divideByCohorts' in data) { pluginConfiguration.division_scheme = data.divideByCohorts ? DivisionSchemes.COHORT : DivisionSchemes.NONE; + pluginConfiguration.always_divide_inline_discussions = data.divideByCohorts; } if (data.blackoutDates?.length) { pluginConfiguration.discussion_blackouts = data.blackoutDates.map((blackoutDates) => ( @@ -195,8 +195,9 @@ function denormalizeData(courseId, appId, data) { return newTopics; }, {}); } - if (data.divideDiscussionIds) { - pluginConfiguration.divided_course_wide_discussions = data.divideDiscussionIds; + if ('divideCourseTopicsByCohorts' in data) { + pluginConfiguration.divided_course_wide_discussions = data.divideCourseTopicsByCohorts + ? data.divideDiscussionIds : []; } const ltiConfiguration = {}; diff --git a/src/pages-and-resources/discussions/data/redux.test.js b/src/pages-and-resources/discussions/data/redux.test.js index c55fe5690..f02b801e1 100644 --- a/src/pages-and-resources/discussions/data/redux.test.js +++ b/src/pages-and-resources/discussions/data/redux.test.js @@ -227,7 +227,7 @@ describe('Data layer integration tests', () => { saveStatus: SAVED, hasValidationError: false, discussionTopicIds, - divideDiscussionIds, + divideDiscussionIds: [], }); expect(store.getState().models.apps.legacy).toEqual(legacyApp); expect(store.getState().models.apps.piazza).toEqual(piazzaApp); @@ -240,7 +240,8 @@ describe('Data layer integration tests', () => { // TODO: Note! As of this writing, all the data below this line is NOT returned in the API // but we add it in during normalization. divisionScheme: DivisionSchemes.COHORT, - divideByCohorts: true, + divideByCohorts: false, + alwaysDivideInlineDiscussions: false, allowDivisionByUnit: false, divideCourseTopicsByCohorts: false, }); @@ -388,8 +389,9 @@ describe('Data layer integration tests', () => { plugin_configuration: { allow_anonymous: true, allow_anonymous_to_peers: true, + always_divide_inline_discussions: true, discussion_blackouts: [], - division_scheme: DivisionSchemes.NONE, + division_scheme: DivisionSchemes.COHORT, discussion_topics: { Edx: { id: '13f106c6-6735-4e84-b097-0456cff55960' }, General: { id: 'course' }, @@ -406,6 +408,7 @@ describe('Data layer integration tests', () => { plugin_configuration: { allow_anonymous: true, allow_anonymous_to_peers: true, + always_divide_inline_discussions: true, discussion_blackouts: [], division_scheme: DivisionSchemes.COHORT, discussion_topics: { @@ -431,9 +434,10 @@ describe('Data layer integration tests', () => { blackoutDates: [], // TODO: Note! As of this writing, all the data below this line is NOT returned in the API // but we technically send it to the thunk, so here it is. - divideByCohorts: false, - allowDivisionByUnit: true, - divideCourseTopicsByCohorts: false, + divideByCohorts: true, + allowDivisionsByUnit: true, + alwaysDivideInlineDiscussions: true, + divideCourseTopicsByCohorts: true, divideDiscussionIds, discussionTopics: [ { name: 'Edx', id: '13f106c6-6735-4e84-b097-0456cff55960' }, @@ -462,13 +466,14 @@ describe('Data layer integration tests', () => { // These three fields should be updated. allowAnonymousPosts: true, allowAnonymousPostsPeers: true, + alwaysDivideInlineDiscussions: true, blackoutDates: [], // TODO: Note! The values we tried to save were ignored, this test reflects what currently // happens, but NOT what we want to have happen! divideByCohorts: true, divisionScheme: DivisionSchemes.COHORT, allowDivisionByUnit: false, - divideCourseTopicsByCohorts: false, + divideCourseTopicsByCohorts: true, }); }); }); diff --git a/src/pages-and-resources/discussions/factories/mockApiResponses.js b/src/pages-and-resources/discussions/factories/mockApiResponses.js index 7d46caf53..06a1a9f9d 100644 --- a/src/pages-and-resources/discussions/factories/mockApiResponses.js +++ b/src/pages-and-resources/discussions/factories/mockApiResponses.js @@ -99,11 +99,7 @@ export const generateLegacyApiResponse = () => ({ Edx: { id: '13f106c6-6735-4e84-b097-0456cff55960' }, General: { id: 'course' }, }, - divided_course_wide_discussions: [ - '13f106c6-6735-4e84-b097-0456cff55960', - 'course', - ], - divided_inline_discussions: [], + divided_course_wide_discussions: [], division_scheme: DivisionSchemes.COHORT, // Note, this gets stringified when normalized into the app, but the API returns it as an // actual array. Argh.