From cc4e19cd2abee9249873f5c0507a8b2791dbcdf1 Mon Sep 17 00:00:00 2001 From: connorhaugh <49422820+connorhaugh@users.noreply.github.com> Date: Thu, 9 Jun 2022 09:41:31 -0400 Subject: [PATCH] fix: redirect to correct learning context (#83) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For https://2u-internal.atlassian.net/browse/TNL-9955 there are multiple instantiations of the text editor in different contexts, so we need to handle them upfront to not break either experience flow” or something along those lines… --- src/editors/Editor.jsx | 8 ++--- src/editors/Editor.test.jsx | 3 +- src/editors/EditorPage.jsx | 2 +- .../__snapshots__/EditorPage.test.jsx.snap | 4 +-- src/editors/data/redux/app/reducer.js | 4 +-- src/editors/data/redux/app/reducer.test.js | 2 +- src/editors/data/redux/app/selectors.js | 14 ++++---- src/editors/data/redux/app/selectors.test.js | 15 +++++--- src/editors/data/redux/thunkActions/app.js | 2 +- .../data/redux/thunkActions/requests.js | 6 ++-- .../data/redux/thunkActions/requests.test.js | 10 +++--- src/editors/data/services/cms/api.js | 16 ++++----- src/editors/data/services/cms/api.test.js | 18 +++++----- src/editors/data/services/cms/mockApi.js | 14 ++++---- src/editors/data/services/cms/urls.js | 21 ++++++++--- src/editors/data/services/cms/urls.test.js | 36 ++++++++++++++----- www/src/Gallery.jsx | 2 +- 17 files changed, 109 insertions(+), 68 deletions(-) diff --git a/src/editors/Editor.jsx b/src/editors/Editor.jsx index 4f7fac2c1..27c987aa5 100644 --- a/src/editors/Editor.jsx +++ b/src/editors/Editor.jsx @@ -9,7 +9,7 @@ import * as hooks from './hooks'; import supportedEditors from './supportedEditors'; export const Editor = ({ - courseId, + learningContextId, blockType, blockId, lmsEndpointUrl, @@ -22,7 +22,7 @@ export const Editor = ({ data: { blockId, blockType, - courseId, + learningContextId, lmsEndpointUrl, studioEndpointUrl, }, @@ -44,7 +44,7 @@ export const Editor = ({ ); }; Editor.defaultProps = { - courseId: null, + learningContextId: null, blockId: null, lmsEndpointUrl: null, studioEndpointUrl: null, @@ -52,7 +52,7 @@ Editor.defaultProps = { }; Editor.propTypes = { - courseId: PropTypes.string, + learningContextId: PropTypes.string, blockType: PropTypes.string.isRequired, blockId: PropTypes.string, lmsEndpointUrl: PropTypes.string, diff --git a/src/editors/Editor.test.jsx b/src/editors/Editor.test.jsx index 57261a7e1..e689f4665 100644 --- a/src/editors/Editor.test.jsx +++ b/src/editors/Editor.test.jsx @@ -17,13 +17,14 @@ jest.mock('./containers/ProblemEditor/ProblemEditor', () => 'ProblemEditor'); const initData = { blockId: 'block-v1:edX+DemoX+Demo_Course+type@html+block@030e35c4756a4ddc8d40b95fbbfff4d4', blockType: blockTypes.html, - courseId: 'course-v1:edX+DemoX+Demo_Course', + learningContextId: 'course-v1:edX+DemoX+Demo_Course', lmsEndpointUrl: 'evenfakerurl.com', studioEndpointUrl: 'fakeurl.com', }; const props = { initialize: jest.fn(), onClose: jest.fn().mockName('props.onClose'), + courseId: 'course-v1:edX+DemoX+Demo_Course', ...initData, }; diff --git a/src/editors/EditorPage.jsx b/src/editors/EditorPage.jsx index 1506bc573..69ad070f7 100644 --- a/src/editors/EditorPage.jsx +++ b/src/editors/EditorPage.jsx @@ -17,7 +17,7 @@ export const EditorPage = ({ ({ ...state, unitUrl: payload }), diff --git a/src/editors/data/redux/app/reducer.test.js b/src/editors/data/redux/app/reducer.test.js index ccb65f116..fd2eb4f22 100644 --- a/src/editors/data/redux/app/reducer.test.js +++ b/src/editors/data/redux/app/reducer.test.js @@ -19,7 +19,7 @@ describe('app reducer', () => { studioEndpointUrl: 'testURL', lmsEndpointUrl: 'sOmEOtherTestuRl', blockId: 'anID', - courseId: 'OTHERid', + learningContextId: 'OTHERid', blockType: 'someTYPE', }; expect(reducer( diff --git a/src/editors/data/redux/app/selectors.js b/src/editors/data/redux/app/selectors.js index 4cff5b422..50ec2370b 100644 --- a/src/editors/data/redux/app/selectors.js +++ b/src/editors/data/redux/app/selectors.js @@ -14,7 +14,7 @@ export const simpleSelectors = { blockId: mkSimpleSelector(app => app.blockId), blockType: mkSimpleSelector(app => app.blockType), blockValue: mkSimpleSelector(app => app.blockValue), - courseId: mkSimpleSelector(app => app.courseId), + learningContextId: mkSimpleSelector(app => app.learningContextId), editorInitialized: mkSimpleSelector(app => app.editorInitialized), saveResponse: mkSimpleSelector(app => app.saveResponse), lmsEndpointUrl: mkSimpleSelector(app => app.lmsEndpointUrl), @@ -24,8 +24,10 @@ export const simpleSelectors = { }; export const returnUrl = createSelector( - [module.simpleSelectors.unitUrl, module.simpleSelectors.studioEndpointUrl], - (unitUrl, studioEndpointUrl) => (unitUrl ? urls.unit({ studioEndpointUrl, unitUrl }) : ''), + [module.simpleSelectors.unitUrl, module.simpleSelectors.studioEndpointUrl, module.simpleSelectors.learningContextId], + (unitUrl, studioEndpointUrl, learningContextId) => ( + urls.returnUrl({ studioEndpointUrl, unitUrl, learningContextId }) + ), ); export const isInitialized = createSelector( @@ -57,10 +59,10 @@ export const analytics = createSelector( [ module.simpleSelectors.blockId, module.simpleSelectors.blockType, - module.simpleSelectors.courseId, + module.simpleSelectors.learningContextId, ], - (blockId, blockType, courseId) => ( - { blockId, blockType, courseId } + (blockId, blockType, learningContextId) => ( + { blockId, blockType, learningContextId } ), ); diff --git a/src/editors/data/redux/app/selectors.test.js b/src/editors/data/redux/app/selectors.test.js index 903586e0f..c025c1223 100644 --- a/src/editors/data/redux/app/selectors.test.js +++ b/src/editors/data/redux/app/selectors.test.js @@ -7,7 +7,7 @@ jest.mock('reselect', () => ({ createSelector: jest.fn((preSelectors, cb) => ({ preSelectors, cb })), })); jest.mock('../../services/cms/urls', () => ({ - unit: (args) => ({ unit: args }), + returnUrl: (args) => ({ returnUrl: args }), })); const testState = { some: 'arbitraryValue' }; @@ -39,7 +39,7 @@ describe('app selectors unit tests', () => { simpleKeys.blockTitle, simpleKeys.blockType, simpleKeys.blockValue, - simpleKeys.courseId, + simpleKeys.learningContextId, simpleKeys.editorInitialized, simpleKeys.saveResponse, simpleKeys.studioEndpointUrl, @@ -53,14 +53,19 @@ describe('app selectors unit tests', () => { expect(selectors.returnUrl.preSelectors).toEqual([ simpleSelectors.unitUrl, simpleSelectors.studioEndpointUrl, + simpleSelectors.learningContextId, ]); }); - it('returns urls.unit with the unitUrl if loaded, else an empty string', () => { + it('returns urls.returnUrl with the returnUrl', () => { const { cb } = selectors.returnUrl; const studioEndpointUrl = 'baseURL'; const unitUrl = 'some unit url'; - expect(cb(null, studioEndpointUrl)).toEqual(''); - expect(cb(unitUrl, studioEndpointUrl)).toEqual(urls.unit({ unitUrl, studioEndpointUrl })); + const learningContextId = 'some learning context'; + expect( + cb(unitUrl, studioEndpointUrl, learningContextId), + ).toEqual( + urls.returnUrl({ unitUrl, studioEndpointUrl, learningContextId }), + ); }); }); describe('isInitialized selector', () => { diff --git a/src/editors/data/redux/thunkActions/app.js b/src/editors/data/redux/thunkActions/app.js index ffc3acfcd..d30911cb4 100644 --- a/src/editors/data/redux/thunkActions/app.js +++ b/src/editors/data/redux/thunkActions/app.js @@ -20,7 +20,7 @@ export const fetchUnit = () => (dispatch) => { /** * @param {string} studioEndpointUrl * @param {string} blockId - * @param {string} courseId + * @param {string} learningContextId * @param {string} blockType */ export const initialize = (data) => (dispatch) => { diff --git a/src/editors/data/redux/thunkActions/requests.js b/src/editors/data/redux/thunkActions/requests.js index 5045e9b04..529197161 100644 --- a/src/editors/data/redux/thunkActions/requests.js +++ b/src/editors/data/redux/thunkActions/requests.js @@ -79,7 +79,7 @@ export const saveBlock = ({ content, ...rest }) => (dispatch, getState) => { promise: api.saveBlock({ blockId: selectors.app.blockId(getState()), blockType: selectors.app.blockType(getState()), - courseId: selectors.app.courseId(getState()), + learningContextId: selectors.app.learningContextId(getState()), content, studioEndpointUrl: selectors.app.studioEndpointUrl(getState()), title: selectors.app.blockTitle(getState()), @@ -91,7 +91,7 @@ export const uploadImage = ({ image, ...rest }) => (dispatch, getState) => { dispatch(module.networkRequest({ requestKey: RequestKeys.uploadImage, promise: api.uploadImage({ - courseId: selectors.app.courseId(getState()), + learningContextId: selectors.app.learningContextId(getState()), image, studioEndpointUrl: selectors.app.studioEndpointUrl(getState()), }), @@ -104,7 +104,7 @@ export const fetchImages = ({ ...rest }) => (dispatch, getState) => { requestKey: RequestKeys.fetchImages, promise: api.fetchImages({ studioEndpointUrl: selectors.app.studioEndpointUrl(getState()), - courseId: selectors.app.courseId(getState()), + learningContextId: selectors.app.learningContextId(getState()), }).then((response) => loadImages(response.data.assets)), ...rest, })); diff --git a/src/editors/data/redux/thunkActions/requests.test.js b/src/editors/data/redux/thunkActions/requests.test.js index 9faed0505..2b8b7b791 100644 --- a/src/editors/data/redux/thunkActions/requests.test.js +++ b/src/editors/data/redux/thunkActions/requests.test.js @@ -12,7 +12,7 @@ jest.mock('../app/selectors', () => ({ studioEndpointUrl: (state) => ({ studioEndpointUrl: state }), blockId: (state) => ({ blockId: state }), blockType: (state) => ({ blockType: state }), - courseId: (state) => ({ courseId: state }), + learningContextId: (state) => ({ learningContextId: state }), blockTitle: (state) => ({ title: state }), })); @@ -198,7 +198,7 @@ describe('requests thunkActions module', () => { let dispatchedAction; const expectedArgs = { studioEndpointUrl: selectors.app.studioEndpointUrl(testState), - courseId: selectors.app.courseId(testState), + learningContextId: selectors.app.learningContextId(testState), }; beforeEach(() => { fetchImages = jest.fn((args) => new Promise((resolve) => { @@ -216,7 +216,7 @@ describe('requests thunkActions module', () => { expect(dispatchedAction.networkRequest.onSuccess).toEqual(onSuccess); expect(dispatchedAction.networkRequest.onFailure).toEqual(onFailure); }); - test('api.fetchImages promise called with studioEndpointUrl and courseId', () => { + test('api.fetchImages promise called with studioEndpointUrl and learningContextId', () => { expect(fetchImages).toHaveBeenCalledWith(expectedArgs); }); test('promise is chained with api.loadImages', () => { @@ -236,7 +236,7 @@ describe('requests thunkActions module', () => { promise: api.saveBlock({ blockId: selectors.app.blockId(testState), blockType: selectors.app.blockType(testState), - courseId: selectors.app.courseId(testState), + learningContextId: selectors.app.learningContextId(testState), content, studioEndpointUrl: selectors.app.studioEndpointUrl(testState), title: selectors.app.blockTitle(testState), @@ -255,7 +255,7 @@ describe('requests thunkActions module', () => { ...fetchParams, requestKey: RequestKeys.uploadImage, promise: api.uploadImage({ - courseId: selectors.app.courseId(testState), + learningContextId: selectors.app.learningContextId(testState), image, studioEndpointUrl: selectors.app.studioEndpointUrl(testState), }), diff --git a/src/editors/data/services/cms/api.js b/src/editors/data/services/cms/api.js index 463119551..795acece2 100644 --- a/src/editors/data/services/cms/api.js +++ b/src/editors/data/services/cms/api.js @@ -11,18 +11,18 @@ export const apiMethods = { fetchByUnitId: ({ blockId, studioEndpointUrl }) => get( urls.blockAncestor({ studioEndpointUrl, blockId }), ), - fetchImages: ({ courseId, studioEndpointUrl }) => get( - urls.courseImages({ studioEndpointUrl, courseId }), + fetchImages: ({ learningContextId, studioEndpointUrl }) => get( + urls.courseImages({ studioEndpointUrl, learningContextId }), ), uploadImage: ({ - courseId, + learningContextId, studioEndpointUrl, image, }) => { const data = new FormData(); data.append('file', image); return post( - urls.courseAssets({ studioEndpointUrl, courseId }), + urls.courseAssets({ studioEndpointUrl, learningContextId }), data, ); }, @@ -30,13 +30,13 @@ export const apiMethods = { blockId, blockType, content, - courseId, + learningContextId, title, }) => { if (blockType === 'html') { return { category: blockType, - couseKey: courseId, + couseKey: learningContextId, data: content, has_changes: true, id: blockId, @@ -49,7 +49,7 @@ export const apiMethods = { blockId, blockType, content, - courseId, + learningContextId, studioEndpointUrl, title, }) => post( @@ -58,7 +58,7 @@ export const apiMethods = { blockType, content, blockId, - courseId, + learningContextId, title, }), ), diff --git a/src/editors/data/services/cms/api.test.js b/src/editors/data/services/cms/api.test.js index cbb6d441e..2d9d6868c 100644 --- a/src/editors/data/services/cms/api.test.js +++ b/src/editors/data/services/cms/api.test.js @@ -30,7 +30,7 @@ const { apiMethods } = api; const blockId = 'coursev1:2uX@4345432'; const content = 'Im baby palo santo ugh celiac fashion axe. La croix lo-fi venmo whatever. Beard man braid migas single-origin coffee forage ramps.'; -const courseId = 'demo2uX'; +const learningContextId = 'demo2uX'; const studioEndpointUrl = 'hortus.coa'; const title = 'remember this needs to go into metadata to save'; @@ -52,8 +52,8 @@ describe('cms api', () => { describe('fetchImages', () => { it('should call get with url.courseImages', () => { - apiMethods.fetchImages({ courseId, studioEndpointUrl }); - expect(get).toHaveBeenCalledWith(urls.courseImages({ studioEndpointUrl, courseId })); + apiMethods.fetchImages({ learningContextId, studioEndpointUrl }); + expect(get).toHaveBeenCalledWith(urls.courseImages({ studioEndpointUrl, learningContextId })); }); }); @@ -63,11 +63,11 @@ describe('cms api', () => { blockId, blockType: 'html', content, - courseId, + learningContextId, title, })).toEqual({ category: 'html', - couseKey: courseId, + couseKey: learningContextId, data: content, has_changes: true, id: blockId, @@ -86,7 +86,7 @@ describe('cms api', () => { blockId, blockType: 'html', content, - courseId, + learningContextId, studioEndpointUrl, title, }); @@ -96,7 +96,7 @@ describe('cms api', () => { blockType: 'html', content, blockId, - courseId, + learningContextId, title, }), ); @@ -109,12 +109,12 @@ describe('cms api', () => { const mockFormdata = new FormData(); mockFormdata.append('file', image); apiMethods.uploadImage({ - courseId, + learningContextId, studioEndpointUrl, image, }); expect(post).toHaveBeenCalledWith( - urls.courseAssets({ studioEndpointUrl, courseId }), + urls.courseAssets({ studioEndpointUrl, learningContextId }), mockFormdata, ); }); diff --git a/src/editors/data/services/cms/mockApi.js b/src/editors/data/services/cms/mockApi.js index 1de7e406d..350984bab 100644 --- a/src/editors/data/services/cms/mockApi.js +++ b/src/editors/data/services/cms/mockApi.js @@ -19,7 +19,7 @@ export const fetchByUnitId = ({ blockId, studioEndpointUrl }) => mockPromise({ data: { ancestors: [{ id: 'unitUrl' }] }, }); // eslint-disable-next-line -export const fetchImages = ({ courseId, studioEndpointUrl }) => mockPromise({ +export const fetchImages = ({ learningContextId, studioEndpointUrl }) => mockPromise({ data: { assets: [ { @@ -74,13 +74,13 @@ export const normalizeContent = ({ blockId, blockType, content, - courseId, + learningContextId, title, }) => { if (blockType === 'html') { return { category: blockType, - couseKey: courseId, + couseKey: learningContextId, data: content, has_changes: true, id: blockId, @@ -94,7 +94,7 @@ export const saveBlock = ({ blockId, blockType, content, - courseId, + learningContextId, studioEndpointUrl, title, }) => mockPromise({ @@ -103,17 +103,17 @@ export const saveBlock = ({ blockType, content, blockId, - courseId, + learningContextId, title, }), }); export const uploadImage = ({ - courseId, + learningContextId, studioEndpointUrl, // image, }) => mockPromise({ - url: urls.courseAssets({ studioEndpointUrl, courseId }), + url: urls.courseAssets({ studioEndpointUrl, learningContextId }), image: { asset: { display_name: 'journey_escape.jpg', diff --git a/src/editors/data/services/cms/urls.js b/src/editors/data/services/cms/urls.js index 0ee5750fb..6bb655088 100644 --- a/src/editors/data/services/cms/urls.js +++ b/src/editors/data/services/cms/urls.js @@ -1,7 +1,20 @@ +export const libraryV1 = ({ studioEndpointUrl, learningContextId }) => ( + `${studioEndpointUrl}/library/${learningContextId}` +); + export const unit = ({ studioEndpointUrl, unitUrl }) => ( `${studioEndpointUrl}/container/${unitUrl.data.ancestors[0].id}` ); +export const returnUrl = ({ studioEndpointUrl, unitUrl, learningContextId }) => { + if (learningContextId && learningContextId.includes('library-v1')) { + // when the learning context is a v1 library, return to the library page + return libraryV1({ studioEndpointUrl, learningContextId }); + } + // when the learning context is a course, return to the unit page + return unitUrl ? unit({ studioEndpointUrl, unitUrl }) : ''; +}; + export const block = ({ studioEndpointUrl, blockId }) => ( `${studioEndpointUrl}/xblock/${blockId}` ); @@ -10,10 +23,10 @@ export const blockAncestor = ({ studioEndpointUrl, blockId }) => ( `${block({ studioEndpointUrl, blockId })}?fields=ancestorInfo` ); -export const courseAssets = ({ studioEndpointUrl, courseId }) => ( - `${studioEndpointUrl}/assets/${courseId}/` +export const courseAssets = ({ studioEndpointUrl, learningContextId }) => ( + `${studioEndpointUrl}/assets/${learningContextId}/` ); -export const courseImages = ({ studioEndpointUrl, courseId }) => ( - `${courseAssets({ studioEndpointUrl, courseId })}?sort=uploadDate&direction=desc&asset_type=Images` +export const courseImages = ({ studioEndpointUrl, learningContextId }) => ( + `${courseAssets({ studioEndpointUrl, learningContextId })}?sort=uploadDate&direction=desc&asset_type=Images` ); diff --git a/src/editors/data/services/cms/urls.test.js b/src/editors/data/services/cms/urls.test.js index babd178b9..2e42f4a68 100644 --- a/src/editors/data/services/cms/urls.test.js +++ b/src/editors/data/services/cms/urls.test.js @@ -1,5 +1,7 @@ import { + returnUrl, unit, + libraryV1, block, blockAncestor, courseAssets, @@ -9,8 +11,10 @@ import { describe('cms url methods', () => { const studioEndpointUrl = 'urLgoeStOstudiO'; const blockId = 'blOckIDTeST123'; - const courseId = 'coUrseiD321'; - describe('unit', () => { + const learningContextId = 'lEarnIngCOntextId123'; + const courseId = 'course-v1:courseId123'; + const libraryV1Id = 'library-v1:libaryId123'; + describe('return to learning context urls', () => { const unitUrl = { data: { ancestors: [ @@ -20,6 +24,22 @@ describe('cms url methods', () => { ], }, }; + it('returns the library page when given the library', () => { + expect(returnUrl({ studioEndpointUrl, unitUrl, learningContextId: libraryV1Id })) + .toEqual(`${studioEndpointUrl}/library/${libraryV1Id}`); + }); + it('returns url with studioEndpointUrl and unitUrl', () => { + expect(returnUrl({ studioEndpointUrl, unitUrl, learningContextId: courseId })) + .toEqual(`${studioEndpointUrl}/container/${unitUrl.data.ancestors[0].id}`); + }); + it('returns empty string if no unit url', () => { + expect(returnUrl({ studioEndpointUrl, unitUrl: null, learningContextId: courseId })) + .toEqual(''); + }); + it('returns the library page when given the library', () => { + expect(libraryV1({ studioEndpointUrl, learningContextId: libraryV1Id })) + .toEqual(`${studioEndpointUrl}/library/${libraryV1Id}`); + }); it('returns url with studioEndpointUrl and unitUrl', () => { expect(unit({ studioEndpointUrl, unitUrl })) .toEqual(`${studioEndpointUrl}/container/${unitUrl.data.ancestors[0].id}`); @@ -38,15 +58,15 @@ describe('cms url methods', () => { }); }); describe('courseAssets', () => { - it('returns url with studioEndpointUrl and courseId', () => { - expect(courseAssets({ studioEndpointUrl, courseId })) - .toEqual(`${studioEndpointUrl}/assets/${courseId}/`); + it('returns url with studioEndpointUrl and learningContextId', () => { + expect(courseAssets({ studioEndpointUrl, learningContextId })) + .toEqual(`${studioEndpointUrl}/assets/${learningContextId}/`); }); }); describe('courseImages', () => { - it('returns url with studioEndpointUrl, courseId and courseAssets query', () => { - expect(courseImages({ studioEndpointUrl, courseId })) - .toEqual(`${courseAssets({ studioEndpointUrl, courseId })}?sort=uploadDate&direction=desc&asset_type=Images`); + it('returns url with studioEndpointUrl, learningContextId and courseAssets query', () => { + expect(courseImages({ studioEndpointUrl, learningContextId })) + .toEqual(`${courseAssets({ studioEndpointUrl, learningContextId })}?sort=uploadDate&direction=desc&asset_type=Images`); }); }); }); diff --git a/www/src/Gallery.jsx b/www/src/Gallery.jsx index b6c91ec2f..a529c2e44 100644 --- a/www/src/Gallery.jsx +++ b/www/src/Gallery.jsx @@ -14,7 +14,7 @@ export const EditorGallery = () => { const type = blockTypes[blockTypeKey]; return { ...obj, [type]: mockBlockIdByType(type) }; }, {}); - const courseId = 'fake-course-id'; + const courseId = 'library-v1:EDX+apmjoemaonmeonaoenan'; const studioEndpointUrl = 'fake-studio-endpoint-url'; const lmsEndpointUrl = 'https://courses.edx.org'; // this is hardcoded because that is where the image data is from. const handleChange = (e) => setBlockType(e.target.value);