[COSMO-39] Add escalation email field for LTI-based proctoring providers. by michaelroytman · Pull Request #736 · openedx/frontend-app-authoring

michaelroytman · 2023-12-08T19:15:58Z

Jira

Description

This pull request adds a required escalation email field when the selected provider is an LTI provider. This mimics the behavior when Proctortrack is the selected provider. This commit also renames references to Proctortrack to be more generic.

Dependencies:

The changes in edx/edx-exams#221 must be merged to main or checked out if not yet merged.

Demos

Happy Path

Screen.Recording.2023-12-08.at.2.54.03.PM.mov

Escalation Email Validation

Screen.Recording.2023-12-08.at.2.56.44.PM.mov

Disabling Proctored Exams

Screen.Recording.2023-12-08.at.4.29.15.PM.mov

codecov · 2023-12-08T19:40:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (04c1427) 88.85% compared to head (9853dc8) 88.95%.
Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #736      +/-   ##
==========================================
+ Coverage   88.85%   88.95%   +0.09%     
==========================================
  Files         468      472       +4     
  Lines        7168     7340     +172     
  Branches     1539     1569      +30     
==========================================
+ Hits         6369     6529     +160     
- Misses        772      784      +12     
  Partials       27       27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

michaelroytman · 2023-12-08T19:19:12Z

+    const snakecaseDataToSave = convertObjectToSnakeCase(dataToSave, true);
    const apiClient = getAuthenticatedHttpClient();
-    return apiClient.patch(this.getExamConfigurationUrl(courseId), dataToSave);
+    return apiClient.patch(this.getExamConfigurationUrl(courseId), snakecaseDataToSave);


This was fine when the only key was provider, but the introduction of escalationEmail requires that we convert the keys to snakecase before sending the data off to the backend.

michaelroytman · 2023-12-08T19:34:29Z

+  const isLtiProvider = (provider) => (
+    ltiProctoringProviders.some(p => p.name === provider)
+  );
+
+  function getProviderDisplayLabel(provider) {
+    // if a display label exists for this provider return it
+    return ltiProctoringProviders.find(p => p.name === provider)?.verbose_name || provider;
+  }
+


There's no change to these functions. I had to move them up to make use of them earlier in the file.

michaelroytman · 2023-12-08T19:35:06Z

      if (value === 'proctortrack') {
        setFormValues({ ...newFormValues, createZendeskTickets: false });
-        setShowProctortrackEscalationEmail(true);
+        setShowEscalationEmail(true);


I thought this if...else if... else would be clearer if it enumerated the various cases more explicitly.

michaelroytman · 2023-12-08T19:40:42Z

@@ -113,17 +120,21 @@ const ProctoringSettings = ({ intl, onClose }) => {
    }

    if (formValues.proctoringProvider === 'proctortrack') {


I'm not 100% sure whether I'm supposed to post this information back to Studio if the provider is LTI-based. I think no, because LTI-based providers are managed by edx-exams and the source of truth for LTI-based providers is edx-exams, so Studio doesn't need this information. Let me know if you disagree.

Yeah this is a little weird since this does exist in both places. I think this make sense tho, trying to pass this back to platform probably creates more problems with no clear benefit.

michaelroytman · 2023-12-08T19:42:26Z

-      if (formValues.proctortrackEscalationEmail === '') {
-        const errorMessage = intl.formatMessage(messages['authoring.proctoring.escalationemail.error.blank']);
+      if (formValues.escalationEmail === '') {
+        const errorMessage = intl.formatMessage(messages['authoring.proctoring.escalationemail.error.blank'], { proctoringProviderName: getProviderDisplayLabel(formValues.proctoringProvider) });


I've changed the message to no longer hardcode "Proctortrack", so we must send the name of the provider now.

michaelroytman · 2023-12-08T19:46:25Z

          }
+
+          const isProctortrack = selectedProvider === 'proctortrack';
+          const ltiProviderSelected = proctoringProvidersLti.some(p => p.name === selectedProvider);


Because setting state is not synchronous, ltiProctoringProviders is not hydrated with the results of the API response yet and is still [], so calling isLtiProvider returns False incorrectly. That's why I have to duplicate the logic that's in isLtiProvider here.

I thought about making is LtiProvider functional to accept (provider, ltiProviders) as arguments, but it made the function seem a little pointless.

michaelroytman · 2023-12-08T21:37:56Z

-      await waitFor(() => {
-        screen.getByDisplayValue('proctortrack');
+    proctoringProvidersRequiringEscalationEmail.forEach(provider => {
+      it(`Creates an alert when no proctoring escalation email is provided with ${provider} selected`, async () => {


I moved the majority of tests that are marked as deleted below into this for loop to test both Proctortrack and LTI-based providers.

The one difference is I corrected the test name Has no error when invalid proctoring escalation email is provided with proctoring disabled to Has no error when empty proctoring escalation email is provided with proctoring disabled.

ahh good idea 👍

michaelroytman · 2023-12-08T21:40:52Z

+        expect(document.activeElement).toEqual(escalationEmailError);
+      });
+
+      it('Has no error when empty proctoring escalation email is provided with proctoring disabled', async () => {


This works on the edx-exams side because of the way I wrote the serializer, but I just want to call out that this allows saving the following state to edx-exams: {'provider': 'test-lti', 'escalation_email': None}, which is, technically not valid. I need to allow this on the backend because edx-exams doesn't know if proctored exams are enabled or not via the configs endpoint.

I don't really understand why we require a valid email when disabling proctoring but yet allow the empty string as a valid option. But that's the existing behavior.

Just wanted to call this out!

michaelroytman · 2023-12-08T21:41:33Z

    it('Successfully updates exam configuration and studio provider is set to "lti_external" for lti providers', async () => {
      await act(async () => render(intlWrapper(<IntlProctoredExamSettings {...defaultProps} />)));
-      // Make a change to the provider to proctortrack and set the email
+      // Make a change to the provider to test_lti and set the email


This looks like some copypasta, and the email was never set, so I've fixed that.

zacharis278

LGTM 👍

these tests did not look fun to refactor for something seemingly so small

zacharis278 · 2023-12-11T19:18:15Z

@@ -113,17 +120,21 @@ const ProctoringSettings = ({ intl, onClose }) => {
    }

    if (formValues.proctoringProvider === 'proctortrack') {


Yeah this is a little weird since this does exist in both places. I think this make sense tho, trying to pass this back to platform probably creates more problems with no clear benefit.

zacharis278 · 2023-12-11T19:21:52Z

-      await waitFor(() => {
-        screen.getByDisplayValue('proctortrack');
+    proctoringProvidersRequiringEscalationEmail.forEach(provider => {
+      it(`Creates an alert when no proctoring escalation email is provided with ${provider} selected`, async () => {


ahh good idea 👍

This commit adds an escalation email field for LTI-based proctoring providers to the Proctoring modal on the Pages & Resources page. This field behaves identically to the Proctortrack escalation email.

…viders (#736)" This reverts commit 0f483dc.

Internal issue: https://2u-internal.atlassian.net/servicedesk/customer/portal/9/CR-6328?created=true Reverted 6 merged PRs due to problems. scroll was not working on editors potential problems with editor content loading ------------------------------------------------------ * Revert "fix(deps): update dependency @edx/frontend-lib-content-components to v1.177.4 (#742)" This reverts commit cc40e9d. * Revert "feat: add escalation email field for LTI-based proctoring providers (#736)" This reverts commit 0f483dc. * Revert "fix: video downloads (#728)" This reverts commit c5abd21. * Revert "fix: import api to chunk file (#734)" This reverts commit 6f7a992. * Revert "feat: Taxonomy delete dialog (#684)" This reverts commit 1eff489. * Revert "fix(deps): update dependency @edx/frontend-lib-content-components to v1.177.1 (#727)" This reverts commit dcabb77.

michaelroytman force-pushed the michaelroytman/COSMO-39-escalalation-email branch 4 times, most recently from 215e753 to f2ae17d Compare December 8, 2023 19:39

michaelroytman force-pushed the michaelroytman/COSMO-39-escalalation-email branch 2 times, most recently from 041e209 to 2197231 Compare December 8, 2023 21:33

michaelroytman commented Dec 8, 2023

View reviewed changes

zacharis278 approved these changes Dec 11, 2023

View reviewed changes

feat: add escalation email field for LTI-based proctoring providers

9853dc8

This commit adds an escalation email field for LTI-based proctoring providers to the Proctoring modal on the Pages & Resources page. This field behaves identically to the Proctortrack escalation email.

michaelroytman force-pushed the michaelroytman/COSMO-39-escalalation-email branch from 2197231 to 9853dc8 Compare December 12, 2023 14:10

michaelroytman merged commit 0f483dc into master Dec 12, 2023

michaelroytman deleted the michaelroytman/COSMO-39-escalalation-email branch December 12, 2023 19:28

jesperhodge added a commit that referenced this pull request Dec 12, 2023

Revert "feat: add escalation email field for LTI-based proctoring pro…

6dd110a

…viders (#736)" This reverts commit 0f483dc.

kenclary mentioned this pull request Jan 9, 2024

fix: configure webpack to fix paid powerpaste plugin #783

Merged

		@@ -113,17 +120,21 @@ const ProctoringSettings = ({ intl, onClose }) => {
		}

		if (formValues.proctoringProvider === 'proctortrack') {

Uh oh!

Conversation

michaelroytman commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jira

Description

Demos

Uh oh!

codecov Bot commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zacharis278 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

michaelroytman commented Dec 8, 2023 •

edited

Loading

codecov Bot commented Dec 8, 2023 •

edited

Loading