Skip to content

Conversation

@arbirali
Copy link
Contributor

@arbirali arbirali commented Nov 13, 2025

Description

This pull request removes frontend-level handling of the ProctorTrack proctoring provider from the frontend-app-authoring application.

As part of the broader ProctorTrack deprecation effort (openedx/edx-platform#36329
), the backend now provides generic proctoring configuration rules instead of vendor-specific conditions. The frontend has been updated to rely on these generic rules while still performing client-side validation.

By removing the ProctorTrack-specific logic and using the backend’s generic configuration model, this change keeps frontend-app-authoring aligned with current backend behavior and avoids relying on deprecated vendor-specific handling.

Supporting information

This change depends on backend updates described in openedx/edx-platform#36329, which provide generic proctoring configuration rules that the frontend now uses instead of ProctorTrack-specific checks.

Best Practices Checklist

Please check if your PR meets these recommendations before asking for a review:

  • Any new files are using TypeScript (.ts, .tsx).
  • Avoid propTypes and defaultProps in any new or modified code.
  • Tests should use the helpers in src/testUtils.tsx (specifically initializeMocks)
  • Do not add new fields to the Redux state/store. Use React Context to share state among multiple components.
  • Use React Query to load data from REST APIs. See any apiHooks.ts in this repo for examples.
  • All new i18n messages in messages.ts files have a description for translators to use.
  • Avoid using ../ in import paths. To import from parent folders, use @src, e.g. import { initializeMocks } from '@src/testUtils'; instead of from '../../../../testUtils'

@arbirali arbirali marked this pull request as draft November 13, 2025 11:02
@mlabeeb03
Copy link
Member

@kdmccormick Please take a look. @arbirali will update test cases shortly. Also note that we have not removed the zendesk part because it is being done here: #2517
There may be a need for rebase later, just FYI.

@kdmccormick kdmccormick self-requested a review November 13, 2025 16:26
Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple notes, but otherwise the approach looks good.

setSubmissionInProgress(false);
setCourseStartDate(settingsResponse.data.course_start_date);
setAvailableProctoringProviders(settingsResponse.data.available_proctoring_providers);
setAllowedProctoringEscalationEmail(settingsResponse.data.requires_escalation_email_providers);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, could we call it setRequiresEscalationEmailProviders ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setAllowedProctoringEscalationEmail renamed to setRequiresEscalationEmailProviders for consistency.


if (value === 'proctortrack') {
if (allowedProctoringEscalationEmail.includes(value)) {
setFormValues({ ...newFormValues, createZendeskTickets: false });
Copy link
Member

@kdmccormick kdmccormick Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
setFormValues({ ...newFormValues, createZendeskTickets: false });
setFormValues({ ...newFormValues });

Since the zendesk integration is deprecated, it's safe to just remove createZendeskTickets from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createZendeskTickets is removed now.

@arbirali arbirali force-pushed the proctortrack-depr branch 2 times, most recently from cc5c9ec to d488223 Compare November 17, 2025 11:06
@arbirali arbirali marked this pull request as ready for review November 18, 2025 12:52
@codecov
Copy link

codecov bot commented Nov 24, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.85%. Comparing base (e10ab27) to head (e439cbe).
⚠️ Report is 24 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2645      +/-   ##
==========================================
+ Coverage   94.82%   94.85%   +0.03%     
==========================================
  Files        1231     1232       +1     
  Lines       27629    27870     +241     
  Branches     6221     6304      +83     
==========================================
+ Hits        26199    26436     +237     
- Misses       1359     1363       +4     
  Partials       71       71              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @arbirali . The code looks good.

Some of the PR description is not quite right. We are still doing client-side validation. What has changed is: before, we were explicitly checking for the Proctortrak provider, but now, we are using generic proctoring configuration settings. Can you modify your PR description?

Once openedx/edx-platform#37576 has merged, then I'll test this, approve it, and merge.

@kdmccormick kdmccormick added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Nov 25, 2025
@kdmccormick
Copy link
Member

@arbirali @mlabeeb03 , could you please provide screenshots to show that you have manually verified that this change and the backend change work together? Please test all the scenarios:

  • A provider with "requires_escalation_email": True
  • A provider with "requires_escalation_email": False
  • A provider that does not specify "requires_escalation_email"(should default to False)
  • A provider with "show_review_rules": True
  • A provider with "show_review_rules": False
  • A provider that does not specify "show_review_rules"(should default to True)
  • The null provider
  • The lti_external provider
  • A course with an unrecognized provider (You make this happen by importing a course which specifies a provider that isn't installed into the system. You can create the import file by either (a) exporting it from a different instance which does have that provider, or (b) hand-editing the proctoring provider field in the .tar.gz .

Let me know if you're unsure of how to test any of those points or if you think any of them are infeasible/unnecessary to test.

@mlabeeb03
Copy link
Member

mlabeeb03 commented Nov 26, 2025

@kdmccormick

  1. A provider with "requires_escalation_email": True
Screenshot 2025-11-26 at 2 13 59 PM
  1. A provider with "requires_escalation_email": False
Screenshot 2025-11-26 at 2 13 44 PM
  1. A provider with "requires_escalation_email" not defined at all
Screenshot 2025-11-26 at 2 13 50 PM

4, 5, 6) No changes to the show_review_rules are being made in this PR.

  1. A null provider
Screenshot 2025-11-26 at 2 14 27 PM
  1. I am unable to set up an lti_provider, do you have instructions on how to do so? We only need to do a bare minimum setup that allows us the test the requires_escalation_email functionality.

  2. A course with unrecognized provider. Note that I did it be defining a recognized provider and the removing it from the proctoring backend

Screenshot 2025-11-26 at 2 13 18 PM

@kdmccormick kdmccormick self-requested a review November 26, 2025 13:54
@kdmccormick
Copy link
Member

(1-3) Great ✅

(4-6) Right, my mistake. Is it the case that show_review_rules was purely a backend change? If so, then @mlabeeb03 can you provide some testing screenshots at some point to show that the backend changes has been validated?

(7) Great ✅

(8) I'll take a look. I think 2U is well-positioned to test lti_external. I'll ping them and cc you.

(9) Great ✅

@kdmccormick kdmccormick merged commit fa562a6 into openedx:master Nov 26, 2025
8 checks passed
@kdmccormick kdmccormick changed the title refactor: Proctortrack Deprecated refactor: Replace ProctorTrack references with Generic Checks Nov 26, 2025
@mlabeeb03
Copy link
Member

@kdmccormick I tested the show_review_rules by directly looking at the API response and writing test cases. I was unable to find it on the frontend. If you can tell me where to find it that would be great.

@kdmccormick
Copy link
Member

@mlabeeb03 The review rules are configurable on the actual proctored exam subsection, rather than the course-wide proctoring settings page. Check out this doc, which mentions the Review Rules field: https://docs.openedx.org/en/latest/educators/how-tos/proctored_exams/create_proctored_exam_rpnow.html . The doc is specific to the RPNow provider, but I think the same flow should apply for any provider where show_review_rules is true. And for a provider where show_review_rules is false, the Review Rules field should be hidden.

@mlabeeb03
Copy link
Member

@kdmccormick thanks, PFA the screenshots, I've also verified the return value from the API.

  1. show_review_rules: True | Null
Screenshot 2025-11-27 at 1 03 40 PM
  1. show_review_rules: False
Screenshot 2025-11-27 at 1 14 17 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants