From cdb1992ed3138969d03fed2c48917e039ad27867 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Mon, 20 Sep 2021 07:21:22 +0200 Subject: [PATCH 1/6] Use the new createAssignmentAPI if configured by the backend Create and edit assignments by first calling the API endpoint and then submit the assignment to the LMS. --- .../frontend_apps/components/FilePickerApp.js | 40 ++++++++- .../components/FilePickerFormFields.js | 5 ++ .../components/test/FilePickerApp-test.js | 88 ++++++++++++++++++- .../test/FilePickerFormFields-test.js | 17 ++++ 4 files changed, 144 insertions(+), 6 deletions(-) diff --git a/lms/static/scripts/frontend_apps/components/FilePickerApp.js b/lms/static/scripts/frontend_apps/components/FilePickerApp.js index 999d5afaab..b42ba24af3 100644 --- a/lms/static/scripts/frontend_apps/components/FilePickerApp.js +++ b/lms/static/scripts/frontend_apps/components/FilePickerApp.js @@ -8,6 +8,7 @@ import { } from 'preact/hooks'; import { Config } from '../config'; +import { apiCall } from '../utils/api'; import { truncateURL } from '../utils/format'; import ContentSelector from './ContentSelector'; @@ -64,14 +65,17 @@ function contentDescription(content) { export default function FilePickerApp({ onSubmit }) { const submitButton = useRef(/** @type {HTMLInputElement|null} */ (null)); const { + api: { authToken }, filePicker: { formAction, formFields, + createAssignmentAPI: createAssignmentAPI, canvas: { groupsEnabled: enableGroupConfig, ltiLaunchUrl }, }, } = useContext(Config); const [content, setContent] = useState(/** @type {Content|null} */ (null)); + const [extLTIAssignmentId, setExtLTIAssignmentId] = useState(null); const [groupConfig, setGroupConfig] = useState( /** @type {GroupConfig} */ ({ @@ -89,7 +93,36 @@ export default function FilePickerApp({ onSubmit }) { * render. */ const [shouldSubmit, setShouldSubmit] = useState(false); - const submit = useCallback(() => setShouldSubmit(true), []); + const submit = useCallback( + async (/** @type {Content} */ content) => { + async function createAssignment() { + const data = { + ...createAssignmentAPI.data, + content, + groupset: groupConfig.groupSet, + }; + const assignment = await apiCall({ + authToken, + path: createAssignmentAPI.path, + data, + }); + setExtLTIAssignmentId(assignment.ext_lti_assignment_id); + } + if (content && createAssignmentAPI && !extLTIAssignmentId) { + try { + await createAssignment(); + } catch (error) { + setErrorInfo({ + message: 'Creating or editing an assignment', + error: error, + }); + return; + } + } + setShouldSubmit(true); + }, + [authToken, createAssignmentAPI, extLTIAssignmentId, groupConfig.groupSet] + ); // Submit the form after a selection is made via one of the available // methods. @@ -107,7 +140,7 @@ export default function FilePickerApp({ onSubmit }) { content => { setContent(content); if (!enableGroupConfig) { - submit(); + submit(content); } }, [enableGroupConfig, submit] @@ -152,7 +185,7 @@ export default function FilePickerApp({ onSubmit }) { submit(content)} > Continue @@ -164,6 +197,7 @@ export default function FilePickerApp({ onSubmit }) { ltiLaunchURL={ltiLaunchUrl} content={content} formFields={formFields} + extLTIAssignmentId={extLTIAssignmentId} groupSet={groupConfig.useGroupSet ? groupConfig.groupSet : null} /> )} diff --git a/lms/static/scripts/frontend_apps/components/FilePickerFormFields.js b/lms/static/scripts/frontend_apps/components/FilePickerFormFields.js index f126174ca9..27624eb426 100644 --- a/lms/static/scripts/frontend_apps/components/FilePickerFormFields.js +++ b/lms/static/scripts/frontend_apps/components/FilePickerFormFields.js @@ -13,6 +13,7 @@ import { contentItemForContent } from '../utils/content-item'; * @prop {Record} formFields - Form fields provided by the backend * that should be included in the response without any changes * @prop {string|null} groupSet + * @prop {string|null} extLTIAssignmentId */ /** @@ -34,9 +35,13 @@ export default function FilePickerFormFields({ formFields, groupSet, ltiLaunchURL, + extLTIAssignmentId, }) { /** @type {Record} */ const extraParams = groupSet ? { group_set: groupSet } : {}; + if (extLTIAssignmentId) { + extraParams.ext_lti_assignment_id = extLTIAssignmentId; + } const contentItem = JSON.stringify( contentItemForContent(ltiLaunchURL, content, extraParams) ); diff --git a/lms/static/scripts/frontend_apps/components/test/FilePickerApp-test.js b/lms/static/scripts/frontend_apps/components/test/FilePickerApp-test.js index 686d5d6db3..f4f2017c6c 100644 --- a/lms/static/scripts/frontend_apps/components/test/FilePickerApp-test.js +++ b/lms/static/scripts/frontend_apps/components/test/FilePickerApp-test.js @@ -7,6 +7,7 @@ import { Config } from '../../config'; import FilePickerApp, { $imports } from '../FilePickerApp'; import { checkAccessibility } from '../../../test-util/accessibility'; import mockImportedComponents from '../../../test-util/mock-imported-components'; +import { waitFor, waitForElement } from '../../../test-util/wait'; function interact(wrapper, callback) { act(callback); @@ -31,6 +32,9 @@ describe('FilePickerApp', () => { beforeEach(() => { fakeConfig = { + api: { + authToken: 'dummyAuthToken', + }, filePicker: { formAction: 'https://www.shinylms.com/', formFields: { hidden_field: 'hidden_value' }, @@ -55,12 +59,18 @@ describe('FilePickerApp', () => { /** * Check that the expected hidden form fields were set. */ - function checkFormFields(wrapper, expectedContent, expectedGroupSet) { + function checkFormFields( + wrapper, + expectedContent, + expectedGroupSet, + expectedExtLTIAssignmentId + ) { const formFields = wrapper.find('FilePickerFormFields'); assert.deepEqual(formFields.props(), { children: [], content: expectedContent, formFields: fakeConfig.filePicker.formFields, + extLTIAssignmentId: expectedExtLTIAssignmentId, groupSet: expectedGroupSet, ltiLaunchURL: fakeConfig.filePicker.canvas.ltiLaunchUrl, }); @@ -104,6 +114,76 @@ describe('FilePickerApp', () => { }); } + context('when create assignment configuration is enabled', () => { + const authURL = 'https://testlms.hypothes.is/authorize'; + const createAssignmentPath = '/api/canvas/assignments'; + let fakeAPICall; + let fakeNewAssignment; + + beforeEach(() => { + fakeConfig.filePicker.createAssignmentAPI = { + authURL, + path: createAssignmentPath, + }; + + fakeAPICall = sinon.stub(); + fakeNewAssignment = { ext_lti_assignment_id: 10 }; + + fakeAPICall + .withArgs(sinon.match({ path: createAssignmentPath })) + .resolves(fakeNewAssignment); + + $imports.$mock({ + '../utils/api': { apiCall: fakeAPICall }, + }); + }); + + it('calls backend api when content is selected', async () => { + const onSubmit = sinon.stub().callsFake(e => e.preventDefault()); + const wrapper = renderFilePicker({ onSubmit }); + + selectContent(wrapper, 'https://example.com'); + + await waitFor(() => fakeAPICall.called); + assert.calledWith(fakeAPICall, { + authToken: 'dummyAuthToken', + path: createAssignmentPath, + data: { + content: { type: 'url', url: 'https://example.com' }, + groupset: null, + }, + }); + + await waitFor(() => onSubmit.called, 100); + + wrapper.update(); + checkFormFields( + wrapper, + { + type: 'url', + url: 'https://example.com', + }, + null /* groupSet */, + fakeNewAssignment.ext_lti_assignment_id + ); + }); + + it('shows an error if creating the assignment fails', async () => { + const error = new Error('Something happened'); + const onSubmit = sinon.stub().callsFake(e => e.preventDefault()); + fakeAPICall + .withArgs(sinon.match({ path: createAssignmentPath })) + .rejects(error); + + const wrapper = renderFilePicker({ onSubmit }); + + selectContent(wrapper, 'https://example.com'); + + const errDialog = await waitForElement(wrapper, 'ErrorDialog'); + assert.equal(errDialog.length, 1); + assert.equal(errDialog.prop('error'), error); + }); + }); context('when groups are not enabled', () => { it('submits form when content is selected', () => { const onSubmit = sinon.stub().callsFake(e => e.preventDefault()); @@ -118,7 +198,8 @@ describe('FilePickerApp', () => { type: 'url', url: 'https://example.com', }, - null /* groupSet */ + null /* groupSet */, + null /* extLTIAssignmentId */ ); }); @@ -217,7 +298,8 @@ describe('FilePickerApp', () => { type: 'url', url: 'https://example.com', }, - useGroupSet ? 'groupSet1' : null + useGroupSet ? 'groupSet1' : null, + null /* extLTIAssignmentId */ ); }); }); diff --git a/lms/static/scripts/frontend_apps/components/test/FilePickerFormFields-test.js b/lms/static/scripts/frontend_apps/components/test/FilePickerFormFields-test.js index 3b727485b2..2fe00a87a9 100644 --- a/lms/static/scripts/frontend_apps/components/test/FilePickerFormFields-test.js +++ b/lms/static/scripts/frontend_apps/components/test/FilePickerFormFields-test.js @@ -64,6 +64,23 @@ describe('FilePickerFormFields', () => { ); }); + it('adds `ext_lti_assignment_id` query param to LTI launch URL if `extLTIAssignmentId` prop is specified', () => { + const content = { type: 'url', url: 'https://example.com/' }; + const formFields = createComponent({ + content, + extLTIAssignmentId: 'EXT_LTI_ASSIGNMENT_ID', + }); + const contentItems = JSON.parse( + formFields.find('input[name="content_items"]').prop('value') + ); + assert.deepEqual( + contentItems, + contentItemForContent(launchURL, content, { + ext_lti_assignment_id: 'EXT_LTI_ASSIGNMENT_ID', + }) + ); + }); + it('renders `document_url` field for URL content', () => { const formFields = createComponent({ content: { type: 'url', url: 'https://example.com/' }, From b5c730ce5fbff779ad77049af730bbb8025c7fe3 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Mon, 1 Nov 2021 09:17:37 +0100 Subject: [PATCH 2/6] Enable canvas_db_configured_basic_lti_launch view Until canvas assignment are stored on the DB on creation/editing this view will only handle launches of already present in the DB canvas file assignments (regular and SpeedGrader launches). --- lms/views/basic_lti_launch.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lms/views/basic_lti_launch.py b/lms/views/basic_lti_launch.py index 0a2e9b3ee8..602695bfae 100644 --- a/lms/views/basic_lti_launch.py +++ b/lms/views/basic_lti_launch.py @@ -165,9 +165,8 @@ def db_configured_basic_lti_launch(self): @view_config( db_configured=True, - canvas_file=False, + legacy_speedgrader=False, request_param="ext_lti_assignment_id", - url_configured=False, ) def canvas_db_configured_basic_lti_launch(self): """Respond to a Canvas DB-configured assignment launch.""" From 09e40eb93e35fb4653180123b7114e3512673329 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Thu, 4 Nov 2021 14:46:57 +0100 Subject: [PATCH 3/6] Remove VS view predicate --- lms/views/predicates/__init__.py | 2 -- lms/views/predicates/_lti_launch.py | 9 ------- .../lms/views/predicates/__init___test.py | 2 -- .../lms/views/predicates/_lti_launch_test.py | 27 ------------------- 4 files changed, 40 deletions(-) diff --git a/lms/views/predicates/__init__.py b/lms/views/predicates/__init__.py index 6996d8aff0..de15f8e105 100644 --- a/lms/views/predicates/__init__.py +++ b/lms/views/predicates/__init__.py @@ -14,7 +14,6 @@ DBConfigured, LegacySpeedGrader, URLConfigured, - VitalSourceBook, ) @@ -25,7 +24,6 @@ def includeme(config): BrightspaceCopied, CanvasFile, URLConfigured, - VitalSourceBook, Configured, AuthorizedToConfigureAssignments, LegacySpeedGrader, diff --git a/lms/views/predicates/_lti_launch.py b/lms/views/predicates/_lti_launch.py index d26fed0e00..7c536d03c7 100644 --- a/lms/views/predicates/_lti_launch.py +++ b/lms/views/predicates/_lti_launch.py @@ -186,13 +186,6 @@ def __call__(self, context, request): return ("canvas_file" in request.params) == self.value -class VitalSourceBook(Base): - name = "vitalsource_book" - - def __call__(self, context, request): - return ("vitalsource_book" in request.params) == self.value - - class URLConfigured(Base): """ Allow invoking an LTI launch view only for URL-configured assignments. @@ -248,7 +241,6 @@ def __init__(self, value, config): self.db_configured = DBConfigured(True, config) self.blackboard_copied = BlackboardCopied(True, config) self.brightspace_copied = BrightspaceCopied(True, config) - self.vitalsource_book = VitalSourceBook(True, config) def __call__(self, context, request): configured = any( @@ -258,7 +250,6 @@ def __call__(self, context, request): self.db_configured(context, request), self.blackboard_copied(context, request), self.brightspace_copied(context, request), - self.vitalsource_book(context, request), ] ) return configured == self.value diff --git a/tests/unit/lms/views/predicates/__init___test.py b/tests/unit/lms/views/predicates/__init___test.py index 3287edb003..96ff36bb45 100644 --- a/tests/unit/lms/views/predicates/__init___test.py +++ b/tests/unit/lms/views/predicates/__init___test.py @@ -10,7 +10,6 @@ DBConfigured, LegacySpeedGrader, URLConfigured, - VitalSourceBook, ) @@ -25,7 +24,6 @@ def test_includeme_adds_the_view_predicates(): mock.call("brightspace_copied", BrightspaceCopied), mock.call("canvas_file", CanvasFile), mock.call("url_configured", URLConfigured), - mock.call("vitalsource_book", VitalSourceBook), mock.call("configured", Configured), mock.call( "authorized_to_configure_assignments", AuthorizedToConfigureAssignments diff --git a/tests/unit/lms/views/predicates/_lti_launch_test.py b/tests/unit/lms/views/predicates/_lti_launch_test.py index dcfa61e8c9..50a5807353 100644 --- a/tests/unit/lms/views/predicates/_lti_launch_test.py +++ b/tests/unit/lms/views/predicates/_lti_launch_test.py @@ -13,7 +13,6 @@ DBConfigured, LegacySpeedGrader, URLConfigured, - VitalSourceBook, ) from tests import factories @@ -151,23 +150,6 @@ def test_when_assignment_is_not_canvas_file( assert predicate(context, pyramid_request) is expected -class TestVitalSourceBook: - @pytest.mark.parametrize("value,expected", [(True, True), (False, False)]) - def test_when_assignment_is_vitalsource_book(self, value, expected): - request = DummyRequest(params={"vitalsource_book": "true"}) - predicate = VitalSourceBook(value, mock.sentinel.config) - - assert predicate(mock.sentinel.context, request) is expected - - @pytest.mark.parametrize("value,expected", [(True, False), (False, True)]) - def test_when_assignment_is_not_vitalsource_book( - self, value, expected, pyramid_request - ): - predicate = VitalSourceBook(value, mock.sentinel.config) - - assert predicate(mock.sentinel.context, pyramid_request) is expected - - class TestURLConfigured: @pytest.mark.parametrize("value,expected", [(True, True), (False, False)]) def test_when_assignment_is_url_configured( @@ -219,15 +201,6 @@ def test_when_assignment_is_db_configured( assert predicate(context, pyramid_request) is expected - @pytest.mark.parametrize("value,expected", [(True, True), (False, False)]) - def test_when_assignment_is_vitalsource_book( - self, pyramid_request, value, expected, context - ): - pyramid_request.params = {"vitalsource_book": True} - predicate = Configured(value, mock.sentinel.config) - - assert predicate(context, pyramid_request) is expected - @pytest.mark.parametrize("value,expected", [(True, False), (False, True)]) def test_when_assignment_is_unconfigured( self, assignment_service, pyramid_request, value, expected, context From cbc33f9c6656d49d1f16e189a8253678a14caf83 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Thu, 4 Nov 2021 14:48:27 +0100 Subject: [PATCH 4/6] Remove VS launch view --- lms/views/basic_lti_launch.py | 20 ----------- tests/unit/lms/views/basic_lti_launch_test.py | 33 ------------------- 2 files changed, 53 deletions(-) diff --git a/lms/views/basic_lti_launch.py b/lms/views/basic_lti_launch.py index 602695bfae..b3abc1852e 100644 --- a/lms/views/basic_lti_launch.py +++ b/lms/views/basic_lti_launch.py @@ -122,26 +122,6 @@ def legacy_canvas_file_basic_lti_launch(self): ) return self.basic_lti_launch(document_url=document_url, grading_supported=False) - @view_config(vitalsource_book=True) - def vitalsource_lti_launch(self): - """ - Respond to a VitalSource book launch. - - The book and chapter to show are configured by `book_id` and `cfi` request - parameters. VitalSource book launches involve a second LTI launch. - Hypothesis's LMS app generates the form parameters needed to load - VitalSource's book viewer using an LTI launch. The LMS frontend then - renders these parameters into a form and auto-submits the form to perform - an authenticated launch of the VS book viewer. - """ - self.sync_lti_data_to_h() - self.store_lti_data() - self.context.js_config.maybe_enable_grading() - self.context.js_config.add_vitalsource_launch_config( - self.request.params["book_id"], self.request.params.get("cfi") - ) - return {} - @view_config(db_configured=True, canvas_file=False, url_configured=False) def db_configured_basic_lti_launch(self): """ diff --git a/tests/unit/lms/views/basic_lti_launch_test.py b/tests/unit/lms/views/basic_lti_launch_test.py index 5c2ff384c9..a1c1eaefce 100644 --- a/tests/unit/lms/views/basic_lti_launch_test.py +++ b/tests/unit/lms/views/basic_lti_launch_test.py @@ -151,25 +151,6 @@ def configure_assignment_caller(context, pyramid_request): return views.configure_assignment() -def vitalsource_lti_launch_caller(context, pyramid_request): - """ - Call BasicLTILaunchViews.vitalsource_lti_launch(). - - Set up the appropriate conditions and then call - BasicLTILaunchViews.vitalsource_lti_launch(), and return whatever - BasicLTILaunchViews.vitalsource_lti_launch() returns. - """ - - # The book_id and cfi params are assumed present when vitalsource_lti_launch() - # is called. - pyramid_request.params["book_id"] = "book-id" - pyramid_request.params["cfi"] = "/abc" - - views = BasicLTILaunchViews(context, pyramid_request) - - return views.vitalsource_lti_launch() - - class TestBasicLTILaunchViewsInit: """Unit tests for BasicLTILaunchViews.__init__().""" @@ -247,7 +228,6 @@ def test_it_does_not_call_grading_info_upsert_if_canvas( brightspace_copied_basic_lti_launch_caller, url_configured_basic_lti_launch_caller, configure_assignment_caller, - vitalsource_lti_launch_caller, ] ) def view_caller(self, request): @@ -478,19 +458,6 @@ def test_it_returns_the_right_template_data(self, context, pyramid_request): assert data == {} -class TestVitalsourceLTILaunch: - def test_it_adds_vitalsource_launch_config(self, context, pyramid_request): - pyramid_request.params.update( - {"vitalsource_book": "true", "book_id": "book-id", "cfi": "/abc"} - ) - - BasicLTILaunchViews(context, pyramid_request).vitalsource_lti_launch() - - context.js_config.add_vitalsource_launch_config.assert_called_once_with( - "book-id", "/abc" - ) - - @pytest.fixture def context(pyramid_request): context = mock.create_autospec(LTILaunchResource, spec_set=True, instance=True) From 0e6ba905ab63dd9e0a618e658a120d737ea10663 Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Thu, 4 Nov 2021 15:11:47 +0100 Subject: [PATCH 5/6] Handle VS URL like other document_urls --- lms/resources/_js_config/__init__.py | 22 ++++---- lms/services/vitalsource/client.py | 20 +++++-- .../lms/resources/_js_config/__init___test.py | 54 +++++++------------ .../lms/services/vitalsource/client_test.py | 23 ++++++-- 4 files changed, 65 insertions(+), 54 deletions(-) diff --git a/lms/resources/_js_config/__init__.py b/lms/resources/_js_config/__init__.py index 60ad6b3131..62da69649b 100644 --- a/lms/resources/_js_config/__init__.py +++ b/lms/resources/_js_config/__init__.py @@ -74,24 +74,20 @@ def add_document_url(self, document_url): resource_link_id=self._request.params["resource_link_id"], ), } + elif document_url.startswith("vitalsource://"): + vitalsource_svc = self._request.find_service(name="vitalsource") + launch_url, launch_params = vitalsource_svc.get_launch_params( + document_url, self._request.lti_user + ) + self._config["vitalSource"] = { + "launchUrl": launch_url, + "launchParams": launch_params, + } else: self._config["viaUrl"] = via_url(self._request, document_url) self._add_canvas_speedgrader_settings(document_url=document_url) - def add_vitalsource_launch_config(self, book_id, cfi=None): - vitalsource_svc = self._request.find_service(name="vitalsource") - launch_url, launch_params = vitalsource_svc.get_launch_params( - book_id, cfi, self._request.lti_user - ) - self._config["vitalSource"] = { - "launchUrl": launch_url, - "launchParams": launch_params, - } - self._add_canvas_speedgrader_settings( - vitalsource_book_id=book_id, vitalsource_cfi=cfi - ) - def asdict(self): """ Return the configuration for the app's JavaScript code. diff --git a/lms/services/vitalsource/client.py b/lms/services/vitalsource/client.py index 114e357ba0..b80e6bb095 100644 --- a/lms/services/vitalsource/client.py +++ b/lms/services/vitalsource/client.py @@ -1,9 +1,17 @@ +import re + import oauthlib from oauthlib.oauth1 import SIGNATURE_HMAC_SHA1, SIGNATURE_TYPE_BODY from lms.services.exceptions import ExternalRequestError from lms.services.vitalsource._schemas import BookInfoSchema, BookTOCSchema +#: A regex for parsing the BOOK_ID and CFI parts out of one of our custom +#: vitalsource://book/bookID/BOOK_ID/cfi/CFI URLs. +DOCUMENT_URL_REGEX = re.compile( + r"vitalsource:\/\/book\/bookID\/(?P[^\/]*)\/cfi\/(?P.*)" +) + class VitalSourceService: def __init__(self, http_service, lti_launch_key, lti_launch_secret, api_key): @@ -54,7 +62,11 @@ def book_toc(self, book_id: str): return BookTOCSchema(response).parse() - def get_launch_params(self, book_id, cfi, lti_user): + @staticmethod + def parse_document_url(document_url): + return DOCUMENT_URL_REGEX.search(document_url).groupdict() + + def get_launch_params(self, document_url, lti_user): """ Return the form params needed to launch the VitalSource book viewer. @@ -64,11 +76,13 @@ def get_launch_params(self, book_id, cfi, lti_user): See https://developer.vitalsource.com/hc/en-us/articles/215612237-POST-LTI-Create-a-Bookshelf-Launch - :param book_id: The VitalSource book ID ("vbid") - :param cfi: Book location, as a Canonical Fragment Identifier, for deep linking. + :param document_url: `vitalsource://` type URL identifying the document. :param lti_user: Current LTI user information, from the LTI launch request :type lti_user: LTIUser """ + url_params = self.parse_document_url(document_url) + book_id = url_params["book_id"] + cfi = url_params["cfi"] launch_url = f"https://bc.vitalsource.com/books/{book_id}" book_location = "/cfi" + cfi diff --git a/tests/unit/lms/resources/_js_config/__init___test.py b/tests/unit/lms/resources/_js_config/__init___test.py index caf97589ab..5a5e11e4f9 100644 --- a/tests/unit/lms/resources/_js_config/__init___test.py +++ b/tests/unit/lms/resources/_js_config/__init___test.py @@ -171,6 +171,25 @@ def test_it_adds_the_viaUrl_api_config_for_Blackboard_documents(self, js_config) "path": "/api/blackboard/courses/test_course_id/via_url?document_url=blackboard%3A%2F%2Fcontent-resource%2Fxyz123", } + def test_vitalsource_sets_config( + self, js_config, vitalsource_service, pyramid_request + ): + vitalsource_url = "vitalsource://book/bookID/book-id/cfi//abc" + vitalsource_service.get_launch_params.return_value = ( + mock.sentinel.launch_url, + mock.sentinel.launch_params, + ) + + js_config.add_document_url(vitalsource_url) + + vitalsource_service.get_launch_params.assert_called_with( + vitalsource_url, pyramid_request.lti_user + ) + assert js_config.asdict()["vitalSource"] == { + "launchUrl": mock.sentinel.launch_url, + "launchParams": mock.sentinel.launch_params, + } + def test_it_adds_the_viaUrl_api_config_for_Canvas_documents( self, js_config, pyramid_request ): @@ -249,41 +268,6 @@ def test_it_doesnt_set_the_speedGrader_settings_if_theres_no_lis_outcome_service assert "speedGrader" not in js_config.asdict()["canvas"] -class TestAddVitalsourceLaunchConfig: - """Unit tests for JSConfig.add_vitalsource_launch_config().""" - - def test_it_sets_vitalsource_config( - self, js_config, pyramid_request, vitalsource_service - ): - js_config.add_vitalsource_launch_config("book-id", "/abc") - - vitalsource_service.get_launch_params.assert_called_with( - "book-id", "/abc", pyramid_request.lti_user - ) - assert js_config.asdict()["vitalSource"] == { - "launchUrl": mock.sentinel.launch_url, - "launchParams": mock.sentinel.launch_params, - } - - def test_it_sets_submission_params(self, js_config, submission_params): - js_config.add_vitalsource_launch_config("book-id", "/abc") - - assert submission_params() == Any.dict.containing( - { - "vitalsource_book_id": "book-id", - "vitalsource_cfi": "/abc", - } - ) - - @pytest.fixture - def vitalsource_service(self, vitalsource_service): - vitalsource_service.get_launch_params.return_value = ( - mock.sentinel.launch_url, - mock.sentinel.launch_params, - ) - return vitalsource_service - - class TestMaybeEnableGrading: def test_it_adds_the_grading_settings( self, js_config, grading_info_service, pyramid_request diff --git a/tests/unit/lms/services/vitalsource/client_test.py b/tests/unit/lms/services/vitalsource/client_test.py index 5f246fa636..1364158794 100644 --- a/tests/unit/lms/services/vitalsource/client_test.py +++ b/tests/unit/lms/services/vitalsource/client_test.py @@ -26,8 +26,23 @@ def test_init_raises_if_launch_credentials_invalid( with pytest.raises(ValueError, match="VitalSource credentials are missing"): VitalSourceService(http_service, lti_key, lti_secret, api_key) + @pytest.mark.parametrize( + "url,book_id,cfi", + [ + ("vitalsource://book/bookID/book-id/cfi//abc", "book-id", "/abc"), + ("vitalsource://book/bookID/book-id/cfi/abc", "book-id", "abc"), + ], + ) + def test_parse_document_url(self, svc, url, book_id, cfi): + assert svc.parse_document_url(url) == { + "book_id": book_id, + "cfi": cfi, + } + def test_it_generates_lti_launch_form_params(self, svc, lti_user): - launch_url, params = svc.get_launch_params("book-id", "/abc", lti_user) + launch_url, params = svc.get_launch_params( + "vitalsource://book/bookID/book-id/cfi//abc", lti_user + ) # Ignore OAuth signature params in this test. params = {k: v for (k, v) in params.items() if not k.startswith("oauth_")} @@ -46,7 +61,7 @@ def test_it_generates_lti_launch_form_params(self, svc, lti_user): def test_it_uses_correct_launch_key_and_secret_to_sign_params( self, svc, lti_user, OAuth1Client ): - svc.get_launch_params("book-id", "/cfi", lti_user) + svc.get_launch_params("vitalsource://book/bookID/book-id/cfi//abc", lti_user) OAuth1Client.assert_called_with( "lti_launch_key", @@ -56,7 +71,9 @@ def test_it_uses_correct_launch_key_and_secret_to_sign_params( ) def test_it_signs_lti_launch_form_params(self, svc, lti_user): - _, params = svc.get_launch_params("book-id", "/cfi", lti_user) + _, params = svc.get_launch_params( + "vitalsource://book/bookID/book-id/cfi//abc", lti_user + ) # Ignore non-OAuth signature params in this test. params = {k: v for (k, v) in params.items() if k.startswith("oauth_")} From 4422ddcc97785a8f2f848bc5b52a68692350caeb Mon Sep 17 00:00:00 2001 From: Marcos Prieto Date: Thu, 4 Nov 2021 15:57:51 +0100 Subject: [PATCH 6/6] SpeedGrader in VS launches --- .../scripts/frontend_apps/components/BasicLTILaunchApp.js | 6 +++--- lms/validation/_lti_launch_params.py | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.js b/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.js index ad547865d9..fa7fdbcfc0 100644 --- a/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.js +++ b/lms/static/scripts/frontend_apps/components/BasicLTILaunchApp.js @@ -234,8 +234,8 @@ export default function BasicLTILaunchApp() { return; } - // Don't report a submission until the URL has been successfully fetched. - if (!contentUrl) { + // Don't report a submission until we have the data needed to display the assignment content + if (!contentUrl && !vitalSourceConfig) { return; } try { @@ -251,7 +251,7 @@ export default function BasicLTILaunchApp() { // submission. handleError(e, 'error-reporting-submission', false); } - }, [authToken, canvas.speedGrader, contentUrl]); + }, [authToken, canvas.speedGrader, contentUrl, vitalSourceConfig]); useEffect(() => { reportSubmission(); diff --git a/lms/validation/_lti_launch_params.py b/lms/validation/_lti_launch_params.py index 988772daa5..e85346779e 100644 --- a/lms/validation/_lti_launch_params.py +++ b/lms/validation/_lti_launch_params.py @@ -118,6 +118,7 @@ def _decode_url(self, _data, **_kwargs): # pylint:disable=no-self-use url.lower().startswith("http%3a") or url.lower().startswith("https%3a") or url.lower().startswith("canvas%3a") + or url.lower().startswith("vitalsource%3a") ): url = unquote(url) _data["url"] = url