feat: add bulk certificate exceptions CSV upload endpoint#38464
Conversation
- Add ToggleCertificateGenerationView endpoint to enable/disable certificate generation - Add CertificateExceptionsView endpoint to grant and remove certificate exceptions (allowlist) - Add CertificateInvalidationsView endpoint to invalidate and re-validate certificates - Update certificate status labels to be more user-friendly (e.g., "Received" instead of "already received") - Update certificate generation history labels (e.g., "All Learners" instead of "All learners") - Add invalidation notes to IssuedCertificateSerializer - Add certificatesEnabled flag to CourseInformationSerializerV2
… of https://github.com/WGU-Open-edX/edx-platform into wgu-jesse-stewart/instructor_dashboard_certificates_v2
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @wgu-jesse-stewart! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
mphilbrick211
added
the
mao-onboarding
mphilbrick211
added
the
needs reviewer assigned
There was a problem hiding this comment.
Thanks for the PR! The JWT test stabilization looks good. A few items on the bulk endpoint:
1. O(n²) notes lookup (bug) — The next() linear scan inside the creation loop makes the notes lookup O(n) per learner, O(n²) overall. For a bulk CSV endpoint this should be a dict lookup. See inline suggestion.
2. Missing tests (blocking) — The new BulkCertificateExceptionsView has no test coverage. At minimum: happy-path CSV, empty CSV, non-CSV file type, unresolvable learners, and partial success (mix of valid and invalid rows).
3. Duplicate CSV identifiers (nit) — If the same identifier appears twice with different notes, _resolve_learners_to_users deduplicates via dict (last mapping wins), but the next() scan picks the first match's notes. Minor inconsistency — building a notes_by_learner dict (see suggestion for number 1) also fixes this since dict assignment keeps the last value.
4. PR title — Currently the raw branch name. Consider something descriptive like "feat: add bulk certificate exceptions CSV upload endpoint".
Review assisted by Kiro
wgu-jesse-stewart
changed the title
thank you for the feedback. I have addressed those issues. |
wgu-jesse-stewart
force-pushed
the
wgu-jesse-stewart/instructor_dashboard_certificates_bulk
branch
from
dfba239 to
4e74557
Compare
wgu-taylor-payne
left a comment
wgu-taylor-payne
left a comment
There was a problem hiding this comment.
Two more nits from a closer read.
Review assisted by Kiro
| csv_reader = csv.reader(file_content.splitlines()) | ||
|
|
||
| learners_with_notes = [] | ||
| for _row_num, row in enumerate(csv_reader, start=1): |
There was a problem hiding this comment.
Nit: _row_num is unused. Either drop the enumerate or use the row number in error messages (the latter would be more useful for users debugging a bad CSV).
| for _row_num, row in enumerate(csv_reader, start=1): | |
| for row in csv_reader: |
| try: | ||
| # Read and parse CSV file | ||
| file_content = uploaded_file.read().decode('utf-8-sig') | ||
| csv_reader = csv.reader(file_content.splitlines()) | ||
|
|
||
| learners_with_notes = [] | ||
| for _row_num, row in enumerate(csv_reader, start=1): | ||
| if not row or not row[0].strip(): | ||
| continue # Skip empty rows | ||
|
|
||
| learner = row[0].strip() | ||
| notes = row[1].strip() if len(row) > 1 and row[1].strip() else '' | ||
|
|
||
| learners_with_notes.append((learner, notes)) | ||
|
|
||
| if not learners_with_notes: | ||
| return Response( | ||
| {'message': _('CSV file is empty or contains no valid entries')}, | ||
| status=status.HTTP_400_BAD_REQUEST | ||
| ) | ||
|
|
||
| # Extract learners for resolution and build a notes lookup | ||
| learners = [learner for learner, _ in learners_with_notes] | ||
| notes_by_learner = dict(learners_with_notes) | ||
|
|
||
| # Resolve all usernames/emails to users upfront | ||
| learner_to_user, user_errors = _resolve_learners_to_users(learners) | ||
| results['errors'].extend(user_errors) | ||
|
|
||
| # Validate learners for certificate exceptions | ||
| exceptions_to_create, validation_errors = _validate_learners_for_certificate_exceptions( | ||
| learner_to_user, course_key | ||
| ) | ||
| results['errors'].extend(validation_errors) | ||
|
|
||
| # Create all exceptions using the certificates API | ||
| for learner, user in exceptions_to_create: | ||
| notes = notes_by_learner.get(learner, '') | ||
|
|
||
| try: | ||
| certs_api.create_or_update_certificate_allowlist_entry(user, course_key, notes) | ||
| log.info( | ||
| "Certificate exception granted for user %s (%s) in course %s by %s via CSV upload", | ||
| user.id, learner, course_key, request.user.username | ||
| ) | ||
| results['success'].append(learner) | ||
| except Exception as exc: # pylint: disable=broad-except | ||
| log.exception( | ||
| "Error creating certificate exception for user %s in course %s", | ||
| user.id, course_key | ||
| ) | ||
| results['errors'].append({ | ||
| 'learner': learner, | ||
| 'message': str(exc) | ||
| }) | ||
|
|
||
| return Response(results, status=status.HTTP_200_OK) | ||
|
|
||
| except Exception as exc: # pylint: disable=broad-except | ||
| log.exception("Error processing CSV file for certificate exceptions") | ||
| return Response( | ||
| {'message': _('Error processing CSV file: {error}').format(error=str(exc))}, | ||
| status=status.HTTP_400_BAD_REQUEST | ||
| ) |
There was a problem hiding this comment.
The outer try/except Exception wraps the entire method body — parsing, resolution, validation, and creation — and flattens everything into a generic 400. The resolution/validation/creation steps already have their own error handling, so this only needs to cover the decode + CSV parse, which are the only parts that can fail from untrusted input.
Narrowing the scope and catching specific exceptions avoids masking real bugs as 400s:
try:
file_content = uploaded_file.read().decode('utf-8-sig')
csv_reader = list(csv.reader(file_content.splitlines()))
except (UnicodeDecodeError, csv.Error) as exc:
log.exception("Error processing CSV file for certificate exceptions")
return Response(
{'message': _('Error processing CSV file: {error}').format(error=str(exc))},
status=status.HTTP_400_BAD_REQUEST
)Then the rest of the method (from learners_with_notes = [] through the return Response(results, ...)) lives outside the try/except at the same indentation level.
openedx-webhooks
removed
the
needs reviewer assigned
wgu-jesse-stewart
deleted the
wgu-jesse-stewart/instructor_dashboard_certificates_bulk
branch
4 participants
feat: add bulk certificate exceptions endpoint and stabilize JWT tests
Description
Adds a new v2 instructor API endpoint for granting certificate exceptions in bulk via CSV upload, and refactors the JWT test module to avoid a stale-timestamp issue.
New endpoint:
BulkCertificateExceptionsViewPOST /api/instructor/v2/courses/{course_id}/certificates/exceptions/bulkAccepts a multipart form upload with a
filefield containing a CSV. Each row isusername_or_email[,notes]. The view:*.csv.utf-8-sigso Excel-exported files with a BOM work without the user having to think about it._resolve_learners_to_usersand validates them via_validate_learners_for_certificate_exceptionsbefore any writes — same helpers used by the single-learner path, so behavior stays consistent.certs_api.create_or_update_certificate_allowlist_entryfor each valid learner and aggregates results into a single{ "success": [...], "errors": [...] }payload.Errors during a single row are caught and reported in
errorsrather than aborting the batch — partial success is the expected mode for CSV imports.Permission checks reuse
permissions.InstructorPermissionandCERTIFICATE_EXCEPTION_VIEW, matching the existing single-exception endpoint.Test stabilization:
test_jwt.pyThe module previously computed
test_now = int(time())once at import and reused it across every test, along with a module-levelexpected_full_tokendict built from that timestamp. That makes the suite sensitive to import-vs-execution timing and to test reordering — and means any test that mutatesexpected_full_tokenwould leak into others.Replaced both module-level constants with two helpers:
get_test_now()— returns the current time when the test runs.get_expected_full_token(test_now)— returns a fresh expected payload bound to that timestamp.Each test now grabs its own
test_nowat the top and builds its expected payload from it. No behavior change to the JWT code itself.User roles impacted
Supporting information
_resolve_learners_to_users/_validate_learners_for_certificate_exceptionshelpers introduced earlier in the v2 redesign.Testing instructions
Bulk certificate exceptions endpoint
exceptions.csv):200withsuccesscontaining the two valid learners anderrorscontaining the unknown email with a clear message.400:filefield in the request..csvextension.403.Certificate exception granted ...info line per successful row, naming the acting instructor.JWT tests
Run the affected module:
All tests should pass. Optionally re-run several times in a row, or with
-p no:randomlyreversed (random order), to confirm there's no ordering sensitivity.Deadline
None.
Other information
BulkCertificateExceptionsViewfollows the same broad-except pattern as neighboring views inapi_v2.py(withpylint: disable=broad-except) so per-row failures don't abort the batch. Each caught exception is logged vialog.exceptionwith the user and course key.uploaded_file.read()); for very large rosters this may want a streamed parser later, but matches the existing bulk-CSV endpoints in this module.