diff --git a/openedx/features/edly/management/commands/export_tenant_reports_csv.py b/openedx/features/edly/management/commands/export_tenant_reports_csv.py new file mode 100644 index 000000000000..37b263c2d231 --- /dev/null +++ b/openedx/features/edly/management/commands/export_tenant_reports_csv.py @@ -0,0 +1,1818 @@ +""" +Export one Edly tenant's off-boarding data as human-readable CSVs, built by +calling the LMS's own per-course instructor-report generators directly. + +This supersedes the earlier `export_learner_data.py` JSON-restore-bundle +command (removed from this branch). A review of that JSON bundle +(`pullrequestreview-4805493785`) argued the actual off-boarding need is a +CSV a non-technical operator can open, not a machine-shaped restore +bundle -- this command is that redirect, and the JSON command's +tenant/user/course-resolution patterns (referenced throughout this file +and its tests) were carried forward from it before it was deleted. + +Execution mechanism: direct in-process calls, not real Celery tasks +===================================================================== +`CourseGradeReport.generate`, `ProblemResponses.generate`, +`upload_students_csv`, `upload_may_enroll_csv`, and `upload_ora2_data` +(`lms/djangoapps/instructor_task/tasks_helper/{grades,enrollments,misc}.py`) +are the exact functions the LMS's own "Instructor > Data Download" UI +submits as Celery tasks. This command calls them directly, in-process, +instead of submitting real Celery tasks -- avoiding shared-queue contention +with live student-facing work and the stuck-task-reservation risk a real +submission would carry if this command's run were interrupted (this +codebase ships a `fail_old_tasks` cleanup command precisely because that +happens to real InstructorTask rows). + +This works because every one of these functions eventually calls +`TaskProgress.update_task_state` (`tasks_helper/runner.py`), which calls +`_get_current_task().update_state(...)`. `_get_current_task()` +(`runner.py`) is a one-line wrapper around `celery.current_task` whose own +docstring says it exists because "it doesn't seem to work to mock +current_task directly" -- a deliberately-provided test seam, not an +undocumented hack, and it's exactly what this codebase's own +`test_tasks_helper.py` already patches to call these functions outside +Celery. `_celery_free_context` below does the same thing. + +Output capture: `upload_csv_to_report_store` is imported as a separate name +binding into three different modules (`grades.py`, `enrollments.py`, +`misc.py`), so `_capture_csv_uploads` patches all three individually rather +than the shared `tasks_helper/utils.py` original -- patching only the +original would leave these three modules still calling the *real*, +unpatched function via their own imported name. Capturing in-process this +way also means the file-correlation problem a real Celery submission would +face (`ReportStore` has no task-id-keyed lookup, only "most recent file in +this course's folder") never arises: we get exactly the rows this run +produced, no storage round-trip, no race window. + +A regression test (`test_export_tenant_reports_csv.py`) asserts these exact +patch targets (`runner._get_current_task`, and each module's +`upload_csv_to_report_store` binding) still exist, so a future upstream +refactor of `tasks_helper/` fails loudly there instead of silently breaking +this command. + +Per-function call signatures (confirmed by reading each -- one real gotcha) +============================================================================= +- `CourseGradeReport.generate(None, None, course_id, {}, 'grades')` -- + `_task_input` unused. +- `ProblemResponses.generate(None, None, course_id, task_input, 'responses')` + -- requires `task_input['problem_locations']` (a *string*: `generate` + calls `.split(',')` on it) and `task_input['user_id']`. This command + passes the course's own root usage key + (`str(modulestore().make_course_usage_key(course_id))`) so the whole + course tree is walked rather than one problem -- confirmed against real + source, not inferred: `test_tasks_helper.py`'s own + `TestProblemResponsesReport.test_success` passes + `{'problem_locations': str(self.course.location), 'user_id': ...}` into + this exact function, and `self.course.location` is the course's root + usage key. `MAX_PROBLEM_RESPONSES_COUNT` (a hard 5000-response-per-course + cap by default) is left untouched unless an operator explicitly opts in + via `--max-problem-responses` (an integer, or the literal `unlimited`) -- + see `_override_max_problem_responses_limit` -- with a loud log line, then + restored. This is opt-in, not automatic: lifting the only bound on this + report's memory footprint just because problem_responses was requested + is a deliberate operator decision, not a default. +- `upload_students_csv(None, None, course_id, feature_list, 'features')` -- + **`task_input` here is the bare feature list itself**, not a dict + (confirmed from `instructor_task/api.py`'s + `submit_calculate_students_features_csv`, which passes `task_input = + features` verbatim -- not the test file, which isn't ground truth for + this signature quirk). +- `upload_may_enroll_csv(None, None, course_id, {'features': feature_list}, + 'may_enroll')` -- **`task_input` is a dict** here, `.get('features')`. +- `upload_ora2_data(None, None, course_id, {}, 'ora2')` -- `_task_input` + unused. On internal failure it returns the plain string `'failed'` + (`UPDATE_STATUS_FAILED`) instead of raising -- this command branches on + that return value, not a dict/exception. + +Two structural scoping fixes (both confirmed via `export_learner_data.py`'s +own docstring, which already hit and fixed these once for the JSON export) +============================================================================= +1. Tenant membership (`EdlyMultiSiteAccess`), not course-org filtering, is + the identity boundary. Every report's rows are post-filtered to the + tenant's member set before being written to any sink -- each report + exposes a different identity column (Student ID for grades, id for + profiles, username for problem_responses, an anonymized-id/username + match for ora2 -- see below). `may_enroll_info` cannot be filtered this + way at all (see its own section below) and is excluded by default. +2. The course set is the union of org-filtered courses AND courses any + member is actually enrolled in (`_get_tenant_course_ids`) -- org + filtering alone misses a member's enrollment in a course outside the + tenant's own orgs (cross-listed courses, legacy enrollments, etc.). + Unlike `export_learner_data.py`'s `_get_course_ids` (which stringifies + its output for JSON/display purposes only), this command's course-id + resolver returns real `CourseKey` objects -- the five report generators + below all take an actual `CourseKey`, not a string (confirmed: every + test in `test_tasks_helper.py` passes `self.course.id`, a `CourseLocator`, + not `str(self.course.id)`). + +Output / merge design +========================= +Report headers are not stable across courses (grade-report headers vary by +each course's own graded-assignment/cohort/team structure; problem-response +columns vary by problem type). Each course's raw `(header, rows)` is +written untouched (full column fidelity, no forced alignment) to +`per_course/__.csv` -- filtered to tenant membership, +never unfiltered, even at this per-course level. A fixed, known-safe +column-name allowlist is additionally pulled into tenant-wide summary +files: +- `grades_summary.csv`: course_id + the identity/enrollment/certificate + columns from `GRADES_SUMMARY_COLUMNS` (excludes per-course-varying + assignment/cohort/team columns, which stay in the per-course file only). +- `learner_profile.csv`: one row per learner, deduped on `id` across every + course (profile fields are learner-constants, not course-scoped). +- `course_enrollments.csv`: the course-varying profile columns + (`enrollment_mode`, `verification_status`, `cohort`, `team`) as separate + rows with `course_id` attached -- these three files all come from a + single `upload_students_csv` call per course (see `GENERATOR_KEY_FOR_REPORT`). +- `problem_responses.csv` / `ora2_responses.csv`: learner-response + granularity, concatenated with `course_id` prepended, no dedup. Because + their per-course headers are genuinely not a fixed, predetermined + constant (problem_responses' columns are xblock-dependent; ora2's exact + schema is unverified in this checkout -- see below), these two sinks + necessarily buffer every row in memory across the whole run and write + once at the end (`_DictCsvSink`), unlike the fixed-header sinks above + (`_CsvSink`), which stream one course at a time. This is bounded by the + tenant's total response volume, not the OOM-class risk of accumulating + multiple courses' *raw, pre-filtered* generator output simultaneously -- + the actual per-course generator calls still run, filter, and write their + per-course file one course at a time; only the small merged summary rows + are held past that point. +- `may_enroll_info.csv` (opt-in only, see below): a fixed, constant header + (this command controls the requested feature list), so it streams like + `grades_summary.csv`. +- `per_course/__grades_errors.csv` (only written when + `CourseGradeReport._upload` actually captured a `grade_report_err` + upload): the students `CourseGradeFactory` failed to grade for that + course, tenant-filtered the same as every other sink here. The grades + status in the manifest also records two counts, which can legitimately + differ: `failed` is `CourseGradeReport`'s own course-wide failure count + (`context.task_progress.failed`), `failed_rows_exported` is how many of + those rows are this tenant's and were actually written to the file above. + +`MANIFEST.json['summary_files']` counts are read from each sink BEFORE that +sink's own `close()` runs (so a `close()` failure never costs an operator a +count that was already safely knowable) -- read this alongside +`summary_file_errors`, which is keyed by the same `.csv` and only +present for a sink whose `close()` itself raised. A count in `summary_files` +for such a sink describes what it was *holding*, not necessarily what +successfully landed on disk; check `summary_file_errors` for that filename +before trusting the file is complete. + +A failure to write MANIFEST.json, and a failure in any sink's own `close()` +(which can lose an entire summary CSV, not just this audit trail), are both +only ever swallowed (as a raised CommandError, that is -- see below for what +still reaches stderr regardless) when doing so would mask an original +in-flight exception (KeyboardInterrupt, MemoryError, ...); on an otherwise +successful run there is nothing to mask, so either failure is surfaced as a +CommandError instead of a clean exit -- a clean exit 0 that silently cost +every caveat, per-course status, and count documented above (a lost +MANIFEST.json), or a whole missing summary CSV (a lost sink), is worse than a +loud failure. The sink close loop always runs BEFORE the MANIFEST.json write +attempt, so `summary_file_errors` above is on disk in the written manifest +whenever that write itself succeeds; if both the manifest write AND a sink +close() fail on the same run, the MANIFEST.json write failure takes priority +for which exception actually propagates -- only one can. That does mean +`summary_file_errors` itself never reaches disk in that case (MANIFEST.json +never landed), but it is NOT lost entirely: the sink-failure stderr message +below is written unconditionally whenever `summary_file_errors` is +non-empty -- BEFORE either raise is decided, not after -- so which CSV(s) +were lost is still named on stderr, the operator's only remaining channel +for it once the manifest write itself has failed. + +Per-course report failures (a `_run_*` method returning `{'status': 'error', +...}` for one course, without raising -- see "per-course orchestration" +below) are isolated from the run as a whole by design: a broken course must +not abort a run covering many courses. But that isolation must not let the +run's own top-level summary claim a plain, unconditional success either. Note +`manifest['courses_completed']` above answers a DIFFERENT question -- "was +this course actually attempted" (excludes only `--skip-course` entries) -- +not "did it succeed"; a course with every report erroring still appears +there, since it genuinely was processed. Read it alongside +`manifest['courses_with_errors']` below for the latter. + +`manifest['courses_with_errors']` (computed in the finally block, before +`_write_manifest` runs) lists every course with at least one report that +lost something -- either a report's own `status` is `'error'`, or a +nominally-`'success'` report recorded a secondary sub-artifact failure (only +`_run_grades`'s `grades_errors_export_error` does this today: the per-course +grades data itself was fine, but that course's own `grades_errors.csv` +never landed). `'skipped'` (only `_run_ora2`'s unverifiable-identity-column +outcome, see below) does NOT count as an error here -- it is a deliberate, +already-documented abstention from writing an unsafe unfiltered file, not a +failure to write something that should have succeeded. `--skip-course` +entries are never counted either (they were never actually processed). When +`courses_with_errors` is non-empty, `manifest['status']` is downgraded from +`'complete'` to `'complete_with_errors'` (an `'incomplete'` run -- one that +was itself interrupted -- is left as `'incomplete'`, a strictly worse signal +already), and the final stdout banner is a `self.style.WARNING(...)` naming +the affected-course count and pointing at `MANIFEST.json['courses']`, +instead of the plain unconditional success line. + +A SEPARATE, weaker rollup, `manifest['courses_needing_review']`, catches two +outcomes that are NOT failures -- nothing raised, no report's own `status` is +`'error'`, so they do NOT touch `manifest['status']` or count toward +`courses_with_errors` above -- but still leave real data missing or suspect +for that course: a report carrying `_empty_filter_warning`'s `warning` key +(every row for that report was filtered out -- usually a sign the identity +column didn't actually match, not that the course genuinely has zero tenant +rows), and `_run_ora2`'s `'skipped'` outcome (that course's ora2 data is +absent from this export by deliberate design, but still absent). The final +stdout banner is qualified by EITHER rollup being non-empty, not just +`courses_with_errors` -- an operator must not have to know to go dig through +every per-course entry in `MANIFEST.json['courses']` to discover that a +course's data was quietly filtered to nothing or skipped outright. + +`_open_sinks` itself raising partway through (e.g. a third sink's file +creation fails after two already succeeded) already surfaces loudly -- that +exception propagates all the way out of `handle()`, an already-loud failure, +not a silent one. But the sinks it had already created before that point +must not become orphans of it: `_open_sinks` mutates the SAME `sinks` dict +`handle()` already holds (rather than only building and returning a fresh +one), so those earlier sinks stay reachable to the finally block below even +though `_open_sinks` itself never returns normally -- they still get +`close()`d and still get counted in `summary_files`, instead of leaking their +file descriptors and vanishing from the audit trail of a run that already +failed. + +Known limit inherited from the reused generator code, not fixable here: +`CourseGradeReport._generate` materializes a full course's enrollment batch +in memory regardless of caller. For a tenant with one very large course, +this is a real per-course memory ceiling this command's own streaming +discipline cannot fully paper over. + +`learner_profile.csv` and `grades_summary.csv` can disagree on membership +============================================================================= +`learner_profile.csv` (via `upload_students_csv` -> `enrolled_students_features`, +`instructor_analytics/basic.py`) filters to `courseenrollment__is_active=1`. +`grades_summary.csv` (via `CourseGradeReport`, which calls +`users_enrolled_in(..., include_inactive=True)`) does not. A learner who +unenrolled from a course can therefore have a `grades_summary.csv` row with +no matching `learner_profile.csv` row for that same learner -- this is +inherited from the two upstream generators' own differing definitions of +"enrolled", not a bug in this command's merge logic, and is not resolved +here (an operator joining these two files by learner id should expect it). + +CSV formula injection +========================= +All three CSV writers here (`_CsvSink`, `_DictCsvSink`, and +`_write_course_csv`'s raw writer) escape any header or data cell whose +string value starts with `=`, `+`, `-`, or `@` by prefixing it with a +single quote (`_escape_csv_formula`) -- `QUOTE_ALL` alone does not stop a +spreadsheet application from *evaluating* such a cell as a formula, and +learner-controlled fields (name, goals, mailing_address, free-text ORA/ +problem-response answers) flow through every sink here. Headers are +included, not just data, because a per-course header is not always a +fixed constant this command controls (e.g. `CourseGradeReport`'s per- +assignment/experiment-partition names are course-author-supplied). + +The `may_enroll_info` gap +============================ +`may_enroll_info` reports on `CourseEnrollmentAllowed` rows: pending, +not-yet-registered invitees identified only by email, with no user account +and therefore no `EdlyMultiSiteAccess` row to filter against by definition. +It is excluded from the default `--reports` list; requesting it via +`--reports may_enroll` produces a file covering *every* pending invite for +the resolved courses, not just this tenant's -- this caveat is recorded in +the manifest and printed as a warning at runtime, not just documented here. + +The ora2 identity-column gap -- CONFIRMED against the real schema, content +check kept as a safety net rather than the sole mechanism +============================================================================= +`openassessment`/`edx-ora2` is pinned in `requirements/edx/base.txt` as an +editable VCS install (`edly-io/edx-ora2@develop-koa`) and was NOT vendored +in the checkout this command was originally authored against, so +`OraAggregateData.collect_ora2_data`'s exact column layout could not be +read directly at the time. This has since been confirmed against the real +`edly-io/edx-ora2@develop-koa` schema: the real identity columns are +`Anonymized Student ID` (always present) and `Username` (present when +username-in-report is enabled) -- the earlier concern that a scorer column +could false-positive-match does not apply. This closes what was +previously an open, unverified gap. + +`_detect_ora2_identity_column` still verifies a column *by its actual +content* against two known identity spaces this command DOES control: +every `student_anonymoususerid.anonymous_user_id` and every enrolled +username for that course -- kept intentionally as a sanity-check/safety +net rather than switched to a name-based lookup, since a content check +degrades safely (skips the course) if a future ora2 schema change ever +renames or removes these columns, where a bare name lookup would not. A +column qualifies only if every non-empty value in it is contained in one +of those sets. If detection finds no qualifying column, that course's +ora2 output is skipped entirely (not written unfiltered) and recorded in +the manifest as `skipped: identity column unverified`, with a loud log +line pointing at `--ora2-identity-column` as the override. + +ORA2 file attachments are not exported +========================================== +Only the free-text response fields `OraAggregateData.collect_ora2_data` +returns are exported to `ora2_responses.csv` / `per_course/*__ora2.csv`. +Submission **file attachments** (`upload_ora2_submission_files`, +`tasks_helper/misc.py`) are a separate upload this command does not call +and are never included in this export. Flagged here as a product +question, not silently assumed acceptable: confirm this is fine for the +off-boarding use case before relying on this command as a complete ORA2 +data export. + +Secrets/PII default policy +============================== +None of these five report functions leak actual secrets by construction -- +they build rows from curated column allowlists, not `SELECT *`. The one +real risk: the profile feature `meta` is a raw JSON blob on +`auth_userprofile` that arbitrary installed apps can stash anything into -- +excluded from `DEFAULT_PROFILE_FEATURES` and from any `--include-fields` +override unless `--allow-meta-field` is also passed. Standard PII (email, +mailing address, DOB) is expected and wanted for a human-readable +off-boarding roster per the redirect decision and is not stripped by +default. Not resolved here, flagged as a product question: ORA2/ +problem-response CSVs contain free-text learner-submitted answers, where +PII could appear inside the answer content itself (e.g. a name typed into +an essay) -- no column-level filter catches that. + +Read-replica routing -- this command's own queries only, by design +======================================================================== +This command's own direct queries (tenant/org/course/user resolution, +ORA2 identity-column detection) route through `.using(read_replica_or_default())` +(`common.djangoapps.util.query`, the same pattern `cache_programs.py` uses) +so a large tenant export doesn't add read load to the primary database. + +The five upstream instructor-report generators themselves are deliberately +NOT forced onto the replica -- this is a known, permanent limitation, not +an oversight. `CourseGradeReport.generate` has a confirmed read-then-write +pattern (a grade-cache miss triggers an in-request recompute-and-persist), +which makes a replica-lag-sensitive read there actively unsafe (a stale +read could recompute and persist a grade from lagging data); the other +four generators are unverified for the same pattern and are left on +whatever routing they already use rather than guessing. + +Consequence -- this command's own queries can be staler than the generator +output they filter. The generators read the primary; membership/identity +resolution here reads the replica. Under replica lag: a recently added +tenant member is silently omitted from every filtered file, and +`_detect_ora2_identity_column`'s all-values-must-match check can fail on a +single unreplicated anon id, skipping that course's ora2 output entirely +(recorded as 'identity column unverified'). For an off-boarding export, +either run against a quiesced tenant or accept this window. + +Usage (--output-dir is required -- see add_arguments; there is no safe default): + python manage.py export_tenant_reports_csv --dry-run --output-dir /path/to/dir + python manage.py export_tenant_reports_csv --output-dir /path/to/dir + python manage.py export_tenant_reports_csv --output-dir /path/to/dir --reports grades,profiles + python manage.py export_tenant_reports_csv --output-dir /path/to/dir --as-user staff_user \ + --reports problem_responses +""" + +import csv +import getpass +import json +import logging +import os +import re +from contextlib import contextmanager +from unittest.mock import Mock, patch + +from django.conf import settings +from django.contrib.auth import get_user_model +from django.core.management.base import BaseCommand, CommandError +from django.db.models import Q +from django.utils import timezone +from six import text_type + +from common.djangoapps.student.models import AnonymousUserId, CourseEnrollment +from common.djangoapps.util.query import read_replica_or_default +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview +from openedx.core.djangoapps.site_configuration.models import SiteConfiguration +from openedx.features.edly.models import EdlyMultiSiteAccess, EdlySubOrganization +from xmodule.modulestore.django import modulestore + +from lms.djangoapps.instructor_task.tasks_helper import enrollments as enrollments_module +from lms.djangoapps.instructor_task.tasks_helper import grades as grades_module +from lms.djangoapps.instructor_task.tasks_helper import misc as misc_module +from lms.djangoapps.instructor_task.tasks_helper import runner as runner_module +from lms.djangoapps.instructor_task.tasks_helper.utils import UPDATE_STATUS_FAILED + +logger = logging.getLogger(__name__) + +# --reports vocabulary. 'may_enroll' is deliberately excluded from +# DEFAULT_REPORTS -- see the module docstring's "may_enroll_info gap". +REPORT_CHOICES = ('grades', 'profiles', 'enrollments', 'problem_responses', 'ora2', 'may_enroll') +DEFAULT_REPORTS = ('grades', 'profiles', 'enrollments', 'problem_responses', 'ora2') + +# 'profiles' and 'enrollments' are two different tenant-wide *outputs* built +# from the same single `upload_students_csv` call per course -- see the +# module docstring's merge design. +GENERATOR_KEY_FOR_REPORT = { + 'grades': 'grades', + 'profiles': 'student_features', + 'enrollments': 'student_features', + 'problem_responses': 'problem_responses', + 'ora2': 'ora2', + 'may_enroll': 'may_enroll', +} + +# Profile-constant columns (learner-level, not course-scoped) -- 'meta' is +# deliberately absent (see module docstring); 'id' is forced into every +# resolved feature list regardless of overrides, since it's both the +# membership-filter column and the learner_profile.csv dedup key. +DEFAULT_PROFILE_FEATURES = ( + 'id', 'username', 'name', 'email', 'language', 'location', 'year_of_birth', + 'gender', 'level_of_education', 'mailing_address', 'goals', 'last_login', 'date_joined', +) +# Course-varying profile columns -- routed to course_enrollments.csv instead +# of learner_profile.csv (see module docstring). +COURSE_VARYING_PROFILE_FEATURES = ('enrollment_mode', 'verification_status', 'cohort', 'team') + +# Fixed default feature request for --reports may_enroll -- CourseEnrollmentAllowed's +# own schema (email/auto_enroll/created), not learner PII beyond an email address. +MAY_ENROLL_DEFAULT_FEATURES = ('email', 'auto_enroll', 'created') + +# Fixed subset of CourseGradeReport's header that's stable across every +# course (the rest of that header -- per-assignment/cohort/team columns -- +# varies by course and stays in the per-course file only). 'Grade' sits at +# the same position CourseGradeReport._grades_header always emits it at, +# right after the three identity columns -- omitting it here previously +# meant grades_summary.csv had no grade in it at all. +GRADES_SUMMARY_COLUMNS = [ + 'Student ID', 'Email', 'Username', 'Grade', 'Enrollment Track', 'Verification Status', + 'Certificate Eligible', 'Certificate Delivered', 'Certificate Type', 'Enrollment Status', +] + +MANIFEST_FILENAME = 'MANIFEST.json' + + +@contextmanager +def _celery_free_context(): + """ + Patch the one sanctioned test seam (see module docstring) so the five + report-generator functions' internal `TaskProgress.update_task_state` + calls work outside a real Celery worker. + """ + with patch.object(runner_module, '_get_current_task', return_value=Mock(update_state=Mock())): + yield + + +@contextmanager +def _capture_csv_uploads(buffer): + """ + Patch `upload_csv_to_report_store` in all three modules that import + their own name binding of it (see module docstring), appending every + call made during the `with` block to `buffer` instead of touching + `ReportStore`/S3 at all. + """ + def _capture(rows, csv_name, course_id, timestamp, config_name='GRADES_DOWNLOAD'): + buffer.append({'csv_name': csv_name, 'course_id': course_id, 'rows': list(rows)}) + return csv_name + + with patch.object(grades_module, 'upload_csv_to_report_store', _capture), \ + patch.object(enrollments_module, 'upload_csv_to_report_store', _capture), \ + patch.object(misc_module, 'upload_csv_to_report_store', _capture): + yield + + +@contextmanager +def _override_max_problem_responses_limit(override_value): + """ + Temporarily override FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] (a hard + 5000-responses-per-course default) to `override_value` (None means "no + cap") for the duration of a run -- restoring the original value + afterward even if the run raises. Opt-in only, via --max-problem- + responses (see add_arguments/handle): this is NOT applied automatically + just because problem_responses was requested -- lifting the only bound + on that report's memory footprint is a deliberate operator decision, + not a default. + """ + original = settings.FEATURES.get('MAX_PROBLEM_RESPONSES_COUNT') + logger.warning( + "export_tenant_reports_csv: overriding FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] " + "to %r for this run (was %r) -- requested via --max-problem-responses.", + override_value, original, + ) + settings.FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] = override_value + try: + yield + finally: + settings.FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] = original + + +@contextmanager +def _null_context(): + """ + No-op context manager for when the --max-problem-responses override + isn't requested (or doesn't apply) and + `_override_max_problem_responses_limit` shouldn't run at all. + """ + yield + + +def _safe_course_id(course_id): + """ + Sanitize a course id for use in a filename -- same characters + `ProblemResponses._generate_upload_file_name` strips. + """ + return re.sub(r'[:/+]', '_', text_type(course_id)) + + +# Characters a spreadsheet application (Excel, Sheets, etc.) will interpret +# as a formula trigger if they lead a cell's content. +_CSV_FORMULA_INJECTION_PREFIXES = ('=', '+', '-', '@') + + +def _escape_csv_formula(value): + """ + Prefix a string value with a single quote if it starts with a formula + trigger character -- QUOTE_ALL alone does not stop a spreadsheet app + from *evaluating* such a cell (CSV formula injection), and learner- + controlled fields (name, goals, mailing_address, free-text ORA/ + problem-response answers) flow through every sink here unescaped + otherwise. Applied to every header AND data cell at each of this + file's three write points (`_CsvSink`, `_DictCsvSink`, and + `_write_course_csv`'s raw writer) -- not just data rows, since a + per-course header can itself be course-author/xblock-supplied (e.g. + CourseGradeReport's per-assignment/experiment-partition names), not a + fixed constant this command controls. The leading apostrophe is the + standard mitigation (it tells the spreadsheet app "treat this as text, + not a formula") and is invisible once opened in Excel/Sheets; the one + visible side effect is that a stringified value starting with '-' + (e.g. a negative number rendered as text) also gets the prefix -- an + accepted trade-off, not a data-fidelity bug. + """ + if isinstance(value, text_type) and value.startswith(_CSV_FORMULA_INJECTION_PREFIXES): + return u"'" + value + return value + + +class _CsvSink(object): + """ + A tenant-wide summary CSV whose column set is a fixed, predetermined + constant -- written once, then appended to one course at a time as the + run progresses (see module docstring: this is the streaming half of + the merge design, as opposed to `_DictCsvSink` below). + """ + + def __init__(self, path): + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + self._file = os.fdopen(fd, 'w', newline='', encoding='utf-8') + self._writer = csv.writer(self._file, dialect='excel', quotechar='"', quoting=csv.QUOTE_ALL) + self._header_written = False + self.row_count = 0 + + def write_header(self, header): + if not self._header_written: + self._writer.writerow([_escape_csv_formula(value) for value in header]) + self._header_written = True + + def write_rows(self, rows): + for row in rows: + self._writer.writerow([_escape_csv_formula(value) for value in row]) + self.row_count += 1 + self._file.flush() + + def close(self): + self._file.close() + + +class _DictCsvSink(object): + """ + A tenant-wide summary sink for report types whose per-course header is + NOT a predetermined constant (problem_responses' columns depend on + each course's xblock types; ora2's exact schema is unverified in this + checkout -- see module docstring). A flat CSV needs one fixed column + set decided before its header row is written, and that set can only be + known once every course's header has been seen -- so, unlike + `_CsvSink`, this buffers every row (as a dict, keyed by that course's + own header) in memory across the whole run and writes once at `close`. + Bounded by this tenant's total response/assessment volume -- not the + OOM-class risk `_CsvSink`'s immediate-flush avoids, which is about + accumulating multiple courses' *raw, pre-filtered* generator output + simultaneously. The actual per-course generator call, filter, and + per-course file write still happen one course at a time; only these + already-filtered, tenant-scoped summary rows are held past that point. + """ + + def __init__(self, path): + self._path = path + self._fieldnames = [] + self._rows = [] + + def add_rows(self, course_id, header, rows): + for column in ['course_id'] + list(header): + if column not in self._fieldnames: + self._fieldnames.append(column) + course_id_str = text_type(course_id) + for row in rows: + entry = dict(zip(header, row)) + entry['course_id'] = course_id_str + self._rows.append(entry) + + @property + def row_count(self): + return len(self._rows) + + def close(self): + fd = os.open(self._path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, 'w', newline='', encoding='utf-8') as output_file: + writer = csv.DictWriter( + output_file, fieldnames=self._fieldnames, restval='', extrasaction='ignore', + dialect='excel', quotechar='"', quoting=csv.QUOTE_ALL, + ) + # Not writer.writeheader() -- these fieldnames come from each + # course's own report header (xblock/ora2 column names), not a + # fixed constant this command controls, so they need the same + # formula-injection escaping as any other cell (see + # _escape_csv_formula). writerow() on a {name: escaped_name} + # dict writes the header row in fieldname order same as + # writeheader() would. + writer.writerow({name: _escape_csv_formula(name) for name in self._fieldnames}) + for entry in self._rows: + writer.writerow({key: _escape_csv_formula(value) for key, value in entry.items()}) + + +class Command(BaseCommand): + """ + Export one Edly tenant's off-boarding data as human-readable, tenant- + membership-filtered CSVs (grades, profiles/enrollments, problem + responses, ORA2 responses, and optionally may-enroll invites), by + calling the LMS's own per-course instructor-report generators directly + in-process -- see module docstring for the full design and known gaps. + """ + help = ( + "Export one Edly tenant's data as human-readable CSV reports for off-boarding, " + "reusing the LMS's own instructor-report generators (grades/profiles/enrollments/" + "problem_responses/ora2, plus optional may_enroll)." + ) + + def add_arguments(self, parser): + """ + Add command line arguments. + """ + parser.add_argument('slug', help='EdlySubOrganization slug identifying the tenant to export.') + parser.add_argument( + '--output-dir', + required=True, + help='Directory to write into. Required -- there is no safe default: MEDIA_ROOT is ' + 'served publicly by nginx in this deployment, so a tenant\'s learner-PII export ' + 'must never land there by default. The operator must consciously choose a private ' + 'destination every run.', + ) + parser.add_argument( + '--reports', + default=','.join(DEFAULT_REPORTS), + help='Comma-separated report types to generate. Choices: {0}. ' + 'Default excludes may_enroll -- see module docstring.'.format(', '.join(REPORT_CHOICES)), + ) + parser.add_argument( + '--as-user', + default=None, + help='Username or email of the acting staff identity. Required if problem_responses is ' + 'requested (drives its get_course_blocks() access resolution); also recorded in the ' + 'manifest as the run operator when given.', + ) + parser.add_argument( + '--include-fields', + default=None, + help='Comma-separated override of the default profile feature allowlist. ' + "'meta' requires --allow-meta-field to also be passed.", + ) + parser.add_argument( + '--allow-meta-field', + action='store_true', + help="Confirm that --include-fields is intentionally requesting the raw 'meta' JSON blob.", + ) + parser.add_argument( + '--skip-course', + action='append', + default=[], + help='Course id to skip (repeatable escape hatch).', + ) + parser.add_argument( + '--ora2-identity-column', + default=None, + help='Column name in the ora2 report known (from a real devstack) to hold an anonymized ' + 'student id or username -- bypasses the runtime content-based detection (see module ' + 'docstring\'s ora2 gap).', + ) + parser.add_argument( + '--max-problem-responses', + default=None, + help="Opt-in override for FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] (a hard 5000-response-" + 'per-course default) for the duration of a run that requests problem_responses. Not ' + "applied automatically just because problem_responses was requested -- pass an integer " + "cap, or the literal 'unlimited' to remove the cap entirely (the old automatic-lift " + 'behavior). Default: leave the platform-configured cap untouched.', + ) + parser.add_argument( + '--dry-run', + action='store_true', + help='Resolve + print the (course x report-type) matrix and resolved member/course counts. ' + 'Generates nothing.', + ) + + def handle(self, *args, **options): + """ + Resolve the tenant's members and courses, then generate the requested reports. + """ + slug = options['slug'] + dry_run = options['dry_run'] + reports = self._parse_reports(options['reports']) + skip_courses = set(options.get('skip_course') or []) + include_fields = self._resolve_include_fields(options.get('include_fields'), options.get('allow_meta_field')) + ora2_identity_column = options.get('ora2_identity_column') + max_problem_responses = self._resolve_max_problem_responses(options.get('max_problem_responses')) + + self._print_header(slug, reports, dry_run) + + sub_org = self._get_sub_org(slug) + orgs = self._resolve_orgs(sub_org) + user_ids = self._get_user_ids(sub_org) + course_ids = self._get_tenant_course_ids(orgs, user_ids) + + self.stdout.write(u"Orgs: {0}".format(orgs)) + self.stdout.write(u"Courses resolved (org-filtered union member-enrolled): {0}".format(len(course_ids))) + self.stdout.write(u"Members resolved: {0}".format(len(user_ids))) + self.stdout.write(u"Reports requested: {0}".format(', '.join(reports))) + + if not user_ids: + self.stdout.write(self.style.WARNING("No users found for this tenant -- nothing to export.")) + return + + if 'may_enroll' in reports: + self.stdout.write(self.style.WARNING( + "may_enroll requested -- these rows cannot be tenant-membership-filtered " + "(CourseEnrollmentAllowed rows are pre-registration invites with no user account); " + "the output will include every pending invite for these courses, not just this tenant's." + )) + + # Dry-run never calls a report generator and never needs an operator identity, so it must + # return before the --as-user/problem_responses staff-check gate below -- that gate exists to + # protect the real problem_responses generator run, not to block a preview of what would happen. + if dry_run: + self._dry_run(course_ids, reports) + return + + operator_user = None + if options.get('as_user'): + operator_user = self._resolve_operator(options['as_user']) + if 'problem_responses' in reports: + if operator_user is None: + raise CommandError("--as-user is required when --reports includes problem_responses.") + if not operator_user.is_staff: + raise CommandError( + u"--as-user '{0}' is not a staff user -- ProblemResponses._build_student_data's " + u"get_course_blocks() call would silently prune the block tree to what that user " + u"can see, undercounting problem_responses for a non-staff operator. Pass a staff " + u"account via --as-user.".format(operator_user.username) + ) + + output_dir = self._resolve_output_dir(options['output_dir']) + per_course_dir = os.path.join(output_dir, 'per_course') + self._make_private_dir(output_dir) + self._make_private_dir(per_course_dir) + + tenant_usernames = set( + get_user_model().objects.using(read_replica_or_default()) + .filter(id__in=user_ids).values_list('username', flat=True) + ) + feature_list = self._build_feature_list(include_fields, reports) + + manifest = { + 'slug': slug, + 'operator': operator_user.username if operator_user else getpass.getuser(), + 'generated_at': timezone.now().isoformat(), + 'reports_requested': list(reports), + 'overrides': { + 'include_fields': include_fields, + 'allow_meta_field': bool(options.get('allow_meta_field')), + # None when --max-problem-responses wasn't passed (the cap is opt-in, not + # automatic just because problem_responses was requested -- see add_arguments). + 'max_problem_responses_override': max_problem_responses, + 'ora2_identity_column_override': ora2_identity_column, + }, + 'courses': {}, + 'summary_files': {}, + 'known_gaps': {}, + } + # known_gaps entries are NOT gated on which reports were requested as a whole -- + # the active/inactive enrollment-membership mismatch between learner_profile.csv + # and grades_summary.csv (see module docstring) affects any plain --reports + # grades,profiles run, not just when ora2 is requested -- previously this caveat + # was only ever surfaced when ora2 was in the requested reports, so an operator + # running grades+profiles alone never saw it in MANIFEST.json at all, only in the + # module docstring. + if 'grades' in reports and ('profiles' in reports or 'enrollments' in reports): + manifest['known_gaps']['enrollment_membership_mismatch'] = ( + "learner_profile.csv (upload_students_csv -> enrolled_students_features) filters to " + "courseenrollment__is_active=1; grades_summary.csv (CourseGradeReport -> " + "users_enrolled_in(include_inactive=True)) does not. A learner who unenrolled can have " + "a grades_summary.csv row with no matching learner_profile.csv row -- inherited from the " + "two upstream generators' differing definitions of 'enrolled' (see module docstring)." + ) + if 'ora2' in reports: + manifest['known_gaps'].update({ + 'ora2_identity_column_detection': ( + "The ora2 identity column ('Anonymized Student ID', or 'Username' when enabled) " + "is detected at runtime by content against known identity spaces, kept " + "intentionally as a safety net rather than a bare name lookup -- courses where " + "detection fails are recorded below as 'skipped: identity column unverified', not " + "silently included unfiltered (see module docstring's ora2 identity-column note)." + ), + 'ora2_file_attachments_not_exported': ( + "Only free-text ORA responses are exported -- submission file attachments " + "(upload_ora2_submission_files) are never included (see module docstring)." + ), + }) + + seen_learner_ids = set() + if 'problem_responses' in reports and max_problem_responses is not None: + resolved_cap = None if max_problem_responses == 'unlimited' else max_problem_responses + max_problem_responses_ctx = _override_max_problem_responses_limit(resolved_cap) + else: + max_problem_responses_ctx = _null_context() + + # Wrapped in try/finally (not just a bare loop) so a Ctrl-C (KeyboardInterrupt) or + # an in-process MemoryError partway through a multi-course run still closes every + # open sink -- note an external OOM-kill is SIGKILL and no finally block can survive + # that; only an in-process KeyboardInterrupt/MemoryError are actually caught here. + # _DictCsvSink (problem_responses/ora2) buffers entirely in memory and only touches + # disk in close(), so without this, an interrupted run previously lost those two + # files entirely and left no manifest at all. 'status'/'courses_completed' below + # let an operator tell an interrupted run's output apart from a complete one. + # run_succeeded tracks whether control reaches the finally block WITHOUT an + # exception in flight -- the finally block's own _write_manifest guard below + # needs this to tell "nothing to mask, surface a manifest-write failure loudly" + # apart from "an original exception is already propagating, don't mask it". + # sinks is pre-initialized to {} and _open_sinks() itself is called as the FIRST + # statement inside this try (not before it, as it previously was) -- so that if + # opening a sink raises partway through (e.g. a third sink's file creation fails + # after two already succeeded), this finally block still runs and MANIFEST.json + # still gets written recording an incomplete run, instead of the whole try/finally + # never even being entered. The same pre-initialized `sinks` dict is also passed + # INTO _open_sinks (mutated in place, see its own docstring) rather than only + # using whatever it returns -- so a sink or two already created before a LATER + # sink's creation raises still land in this same `sinks` dict, and still get + # closed/counted below, instead of being orphaned along with the exception. + sinks = {} + run_succeeded = False + try: + sinks = self._open_sinks(output_dir, reports, feature_list, sinks) + with max_problem_responses_ctx: + for course_id in sorted(course_ids, key=text_type): + course_key_str = text_type(course_id) + if course_key_str in skip_courses: + manifest['courses'][course_key_str] = {'_skipped': '--skip-course'} + self.stdout.write(u" {0}: skipped (--skip-course)".format(course_key_str)) + continue + status = self._process_course( + course_id, reports, user_ids, tenant_usernames, feature_list, operator_user, + ora2_identity_column, per_course_dir, sinks, seen_learner_ids, + ) + manifest['courses'][course_key_str] = status + self.stdout.write(u" {0}: {1}".format(course_key_str, status)) + manifest['status'] = 'complete' + run_succeeded = True + finally: + # Only reached with run_succeeded still False if the loop (or the + # max_problem_responses_ctx it's nested in) exited via an exception -- + # including KeyboardInterrupt/SystemExit, which `except Exception` inside + # each _run_* method does NOT catch. + manifest.setdefault('status', 'incomplete') + manifest['courses_completed'] = [ + course_key_str for course_key_str, course_status in manifest['courses'].items() + if '_skipped' not in course_status + ] + # Per-course report failures (a _run_* method returning {'status': 'error', ...} + # for one course, without raising) are correctly isolated from the run as a + # whole -- a broken course must not abort a run covering many courses -- but + # that isolation must not let the run's OWN top-level summary claim a plain, + # unconditional success when courses actually lost data. Computed here, BEFORE + # _write_manifest below, so it's on disk in the written manifest. See module + # docstring for what counts as an error here (and why 'skipped' does not). + manifest['courses_with_errors'] = sorted( + course_key_str for course_key_str, course_status in manifest['courses'].items() + if self._course_has_report_error(course_status) + ) + if manifest['courses_with_errors'] and manifest['status'] == 'complete': + # Only downgrade a plain 'complete' -- an already-'incomplete' run (this + # run was itself interrupted) is a strictly worse, more specific signal + # and must not be overwritten by this weaker one. + manifest['status'] = 'complete_with_errors' + # A SEPARATE, weaker rollup from courses_with_errors above -- nothing here + # actually failed (an all-rows-filtered warning, or ora2's deliberate + # identity-column-unverified skip), so this does NOT touch manifest['status'] + # -- but real data is still missing/suspect for that course, and an operator + # must not have to know to go dig through every per-course entry in + # manifest['courses'] to find that out. See _course_needs_review's docstring. + manifest['courses_needing_review'] = sorted( + course_key_str for course_key_str, course_status in manifest['courses'].items() + if self._course_needs_review(course_status) + ) + # Each sink's own close() is real work that can itself fail -- + # _DictCsvSink.close() opens the output file and encodes/writes every + # buffered row, and _CsvSink.close() flushes+closes an open fd. Without + # isolating each one, a single sink failing to flush would abort this + # loop before the remaining sinks close or the manifest below gets + # written -- exactly the failure this finally block exists to prevent. + for name, sink in sinks.items(): + try: + # Read the count BEFORE close() -- it's already known (a plain int + # attribute on _CsvSink, len(self._rows) on _DictCsvSink; neither can + # raise today) -- for _DictCsvSink specifically it's the size of the + # buffer close() is about to write, so a close() failure shouldn't + # cost the operator this number too. Reading it inside this try (not + # just before it) is defensive for a hypothetical future sink type + # whose row_count computation could itself raise. + manifest['summary_files'][name + '.csv'] = sink.row_count + sink.close() + except Exception as exc: # pylint: disable=broad-except + # Known, deliberate trade-off, not addressed here: a KeyboardInterrupt + # raised specifically during THIS sink's close() is a BaseException, not + # an Exception, so it escapes this guard entirely -- the remaining sinks + # never get a chance to close and the manifest write below is skipped. + logger.exception("export_tenant_reports_csv: failed to close sink %s", name) + manifest.setdefault('summary_file_errors', {})[name + '.csv'] = text_type(exc) + + # Computed here, right after the sink-close loop above (so it's fully + # populated), rather than after the manifest-write attempt below -- the + # manifest-write-failure message just below needs to already know whether a + # sink-failure stderr message will actually follow it, to avoid pointing an + # operator "(see below)" at nothing. + sink_errors = manifest.get('summary_file_errors') or {} + + # manifest_write_error is captured rather than raised immediately so the + # raise-or-swallow decision below can weigh it together with any sink close() + # failure recorded above (already fully populated, see sink_errors just + # above) -- so _write_manifest always runs BEFORE either raise below, + # landing summary_file_errors on disk first whenever the write succeeds. + manifest_write_error = None + try: + self._write_manifest(output_dir, manifest) + except Exception as exc: # pylint: disable=broad-except + manifest_write_error = exc + logger.exception( + "export_tenant_reports_csv: failed to write MANIFEST.json to %s", output_dir, + ) + self.stderr.write(self.style.ERROR( + u"MANIFEST.json could not be written to {0}: {1} -- this run has no audit " + u"trail (report caveats, skipped-course records, and summary counts are all " + u"lost); any sink failure is reported on stderr only, not in the missing " + u"manifest{2}.".format( + output_dir, exc, + # Only promise a following stderr message if one will actually be + # written just below -- an empty sink_errors means it won't be. + u" (see below)" if sink_errors else u"", + ) + )) + + # Emitted unconditionally whenever a sink failed to close -- NOT gated on + # run_succeeded below, and written BEFORE either raise is decided. A stderr + # write cannot mask a propagating exception (only the raises below need that + # gate), and this is the operator's ONLY remaining channel for which CSV(s) + # were lost once the manifest write has ALSO failed (see module docstring): + # the priority raise below can only let ONE exception through, so without + # this, naming the lost sink here would otherwise be unreachable on that path. + if sink_errors: + manifest_pointer = ( + u" -- see MANIFEST.json['summary_file_errors']." if manifest_write_error is None + # Don't point an operator at a MANIFEST.json that was never written. + else u" -- MANIFEST.json was not written this run (see above), so this " + u"is the only record of it." + ) + self.stderr.write(self.style.ERROR( + u"These summary CSVs failed to flush/close and are missing or truncated " + u"on disk: {0}{1}".format(u', '.join(sorted(sink_errors)), manifest_pointer) + )) + + if run_succeeded: + # Nothing is propagating out of this finally block, so nothing can be + # masked -- an operator must not get a clean exit 0 either for an export + # with no audit trail or for one that silently lost a whole summary CSV. + if manifest_write_error is not None: + # Manifest failure takes priority when both fail -- only one exception + # can propagate, and this one also means summary_file_errors itself + # never made it to disk (MANIFEST.json never landed) -- but the sink + # name(s) it held were already put on stderr above regardless. + raise CommandError( + u"Export failed to write MANIFEST.json: {0}".format(manifest_write_error) + ) + if sink_errors: + raise CommandError( + u"Export completed but {0} summary CSV(s) failed to write: {1}".format( + len(sink_errors), u', '.join(sorted(sink_errors)) + ) + ) + # Otherwise an original exception (KeyboardInterrupt, MemoryError, a plain + # Exception from _process_course, ...) is already propagating out of this + # finally block -- swallow these secondary failures so they can't replace that + # original, the operator's only signal for why the run actually stopped. + + courses_with_errors = manifest.get('courses_with_errors') or [] + courses_needing_review = manifest.get('courses_needing_review') or [] + self.stdout.write("\n" + "=" * 72) + if courses_with_errors or courses_needing_review: + # Nothing above raised (run_succeeded, no manifest-write/sink-close failure), + # but either rollup being non-empty means this run must not read as a plain, + # unconditional success -- see module docstring's courses_with_errors / + # courses_needing_review sections. Composed rather than two separate + # `if`/`elif` banners so a run hitting BOTH still gets a single message + # naming both counts, not just whichever branch happened to come first. + parts = [] + if courses_with_errors: + parts.append(u"{0} course(s) had at least one report error".format(len(courses_with_errors))) + if courses_needing_review: + parts.append(u"{0} course(s) have a warning or skipped report worth reviewing".format( + len(courses_needing_review) + )) + self.stdout.write(self.style.WARNING( + u"Tenant CSV export for {0} finished, but {1} -- see MANIFEST.json['courses'] " + u"in {2} for detail.".format(slug, u' and '.join(parts), output_dir) + )) + else: + self.stdout.write(u"Exported tenant CSV reports for {0} to {1}".format(slug, output_dir)) + self.stdout.write("=" * 72) + + # ------------------------------------------------------------------ tenant scoping + # _get_sub_org / _resolve_orgs / _get_user_ids reuse export_learner_data.py's pattern + # verbatim (see module docstring). _get_tenant_course_ids is the amended course-set + # union (structural fix #2) -- NOT export_learner_data.py's _get_course_ids, whose + # stringified output is display/JSON-only; the report generators below need real + # CourseKey objects. + + def _get_sub_org(self, slug): + """ + Resolve the EdlySubOrganization for the given slug. + """ + try: + return EdlySubOrganization.objects.using(read_replica_or_default()).get(slug=slug) + except EdlySubOrganization.DoesNotExist: + raise CommandError(u"No EdlySubOrganization found for slug '{0}'.".format(slug)) + + def _resolve_orgs(self, sub_org): + """ + Resolve the tenant's course_org_filter (used only to seed the + org-filtered half of `_get_tenant_course_ids`'s union). + """ + orgs = [] + try: + site_configuration = sub_org.lms_site.configuration + except SiteConfiguration.DoesNotExist: + site_configuration = None + + if site_configuration: + course_org_filter = site_configuration.get_value('course_org_filter', []) + if course_org_filter: + orgs = course_org_filter if isinstance(course_org_filter, list) else [course_org_filter] + + if not orgs: + orgs = sub_org.get_edx_organizations + + return list(orgs) + + def _get_user_ids(self, sub_org): + """ + Resolve the tenant's user ids strictly via EdlyMultiSiteAccess + membership -- the sole tenant boundary this command recognizes. + """ + return set( + EdlyMultiSiteAccess.objects.using(read_replica_or_default()) + .filter(sub_org=sub_org).values_list('user_id', flat=True) + ) + + def _get_tenant_course_ids(self, orgs, user_ids): + """ + Amended course-set union (structural fix #2, see module docstring): + org-filtered courses alone miss a member's enrollments in courses + outside the tenant's own org list. Returns real CourseKey objects. + """ + org_course_ids = set( + CourseOverview.objects.using(read_replica_or_default()) + .filter(org__in=orgs).values_list('id', flat=True) + ) + member_course_ids = set( + CourseEnrollment.objects.using(read_replica_or_default()) + .filter(user_id__in=user_ids, is_active=True) + .values_list('course_id', flat=True).distinct() + ) + return org_course_ids | member_course_ids + + def _resolve_operator(self, identifier): + """ + Resolve --as-user by username or email. + """ + User = get_user_model() + try: + return User.objects.using(read_replica_or_default()).get(Q(username=identifier) | Q(email=identifier)) + except User.DoesNotExist: + raise CommandError(u"--as-user '{0}' does not match any user (by username or email).".format(identifier)) + except User.MultipleObjectsReturned: + raise CommandError(u"--as-user '{0}' matches multiple users -- be more specific.".format(identifier)) + + # ------------------------------------------------------------------ argument resolution + + def _parse_reports(self, reports_arg): + """ + Parse + validate --reports against REPORT_CHOICES. + """ + requested = [r.strip() for r in reports_arg.split(',') if r.strip()] + unknown = set(requested) - set(REPORT_CHOICES) + if unknown: + raise CommandError(u"Unknown --reports value(s): {0}. Choose from: {1}".format( + ', '.join(sorted(unknown)), ', '.join(REPORT_CHOICES) + )) + return requested + + def _resolve_include_fields(self, include_fields_arg, allow_meta_field): + """ + Parse --include-fields, guard 'meta' behind --allow-meta-field, and + force 'id' into the list regardless of the override (see module + docstring -- it's the membership-filter/dedup key, not optional). + """ + fields = ( + [f.strip() for f in include_fields_arg.split(',') if f.strip()] + if include_fields_arg else list(DEFAULT_PROFILE_FEATURES) + ) + if 'meta' in fields and not allow_meta_field: + raise CommandError( + "--include-fields requested 'meta' -- pass --allow-meta-field to confirm this is " + "intended (see module docstring's secrets/PII policy)." + ) + if 'id' not in fields: + fields = ['id'] + fields + return fields + + def _resolve_max_problem_responses(self, max_problem_responses_arg): + """ + Parse --max-problem-responses: None if not passed (leave the + platform's configured cap untouched -- see + _override_max_problem_responses_limit / handle), the literal + 'unlimited' (explicit opt-in to the old automatic-lift behavior), + or an integer override. + """ + if max_problem_responses_arg is None: + return None + if max_problem_responses_arg == 'unlimited': + return 'unlimited' + try: + return int(max_problem_responses_arg) + except ValueError: + raise CommandError( + u"--max-problem-responses must be an integer or 'unlimited', got '{0}'.".format( + max_problem_responses_arg + ) + ) + + def _build_feature_list(self, include_fields, reports): + """ + The feature list actually requested from upload_students_csv: the + resolved --include-fields, plus the course-varying columns if + 'enrollments' was requested. + """ + feature_list = list(include_fields) + if 'enrollments' in reports: + for feature in COURSE_VARYING_PROFILE_FEATURES: + if feature not in feature_list: + feature_list.append(feature) + return feature_list + + # ------------------------------------------------------------------ output plumbing + + def _resolve_output_dir(self, output_dir): + """ + Return `--output-dir` verbatim -- it's a required argument (see add_arguments) precisely + because there's no safe default to fall back to here. This used to fall back to + EDM_EXPORT_DIR (never defined anywhere in this codebase's settings) or, failing that, a + directory under MEDIA_ROOT when --output-dir was omitted -- but MEDIA_ROOT is served + publicly by nginx at /media/ in this Koa deployment, so that fallback silently put a + tenant's full learner-PII export under the public web root by default. Making --output-dir + required removes the unsafe default outright instead of trying to guess a safer one. + """ + return output_dir + + def _make_private_dir(self, path): + """ + Create `path` (and any missing parents) and force 0700 perms on it + -- these directories hold PII (grades, profiles, free-text ORA + responses). Only chmods a directory this call actually created: + `--output-dir` can point at a path that already existed before this + run (e.g. MEDIA_ROOT itself), and unconditionally chmod'ing it would + silently lock that shared directory down for every other consumer. + """ + if not os.path.exists(path): + os.makedirs(path, mode=0o700) + os.chmod(path, 0o700) + + def _open_sinks(self, output_dir, reports, feature_list, sinks=None): + """ + Open the tenant-wide summary sinks for the requested reports and + write their headers up front where the header is a fixed constant + (see module docstring: _CsvSink vs _DictCsvSink). + + Mutates (and returns) `sinks` in place rather than only building and + returning a fresh dict -- `handle()` passes in the same dict it + already holds so that if creating a LATER sink here raises (e.g. a + third sink's file creation fails after two already succeeded), the + EARLIER sinks already added above stay visible to the caller even + though this call itself never returns normally (see module + docstring). `sinks=None` (every other/existing caller, including + this file's own tests) still gets a fresh dict, same as before. + """ + sinks = {} if sinks is None else sinks + if 'grades' in reports: + sinks['grades_summary'] = _CsvSink(os.path.join(output_dir, 'grades_summary.csv')) + sinks['grades_summary'].write_header(['course_id'] + GRADES_SUMMARY_COLUMNS) + if 'profiles' in reports: + constant_columns = [c for c in feature_list if c not in COURSE_VARYING_PROFILE_FEATURES] + sinks['learner_profile'] = _CsvSink(os.path.join(output_dir, 'learner_profile.csv')) + sinks['learner_profile'].write_header(constant_columns) + if 'enrollments' in reports: + course_varying_present = [c for c in COURSE_VARYING_PROFILE_FEATURES if c in feature_list] + sinks['course_enrollments'] = _CsvSink(os.path.join(output_dir, 'course_enrollments.csv')) + sinks['course_enrollments'].write_header(['course_id', 'id'] + course_varying_present) + if 'problem_responses' in reports: + sinks['problem_responses'] = _DictCsvSink(os.path.join(output_dir, 'problem_responses.csv')) + if 'ora2' in reports: + sinks['ora2_responses'] = _DictCsvSink(os.path.join(output_dir, 'ora2_responses.csv')) + if 'may_enroll' in reports: + sinks['may_enroll_info'] = _CsvSink(os.path.join(output_dir, 'may_enroll_info.csv')) + sinks['may_enroll_info'].write_header(['course_id'] + list(MAY_ENROLL_DEFAULT_FEATURES)) + return sinks + + def _write_course_csv(self, per_course_dir, course_id, report_name, header, rows): + """ + Write one course's untouched, tenant-filtered rows to + per_course/__.csv (full column fidelity, no + forced alignment across courses -- see module docstring). + """ + path = os.path.join(per_course_dir, u"{0}__{1}.csv".format(_safe_course_id(course_id), report_name)) + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, 'w', newline='', encoding='utf-8') as output_file: + writer = csv.writer(output_file, dialect='excel', quotechar='"', quoting=csv.QUOTE_ALL) + # header is NOT always a fixed constant this command controls -- + # CourseGradeReport's header includes course-author-supplied + # experiment-partition/assignment names -- so it gets the same + # formula-injection escaping as the data rows (_escape_csv_formula). + writer.writerow([_escape_csv_formula(value) for value in header]) + for row in rows: + writer.writerow([_escape_csv_formula(value) for value in row]) + return len(rows) + + def _write_manifest(self, output_dir, manifest): + """ + Write MANIFEST.json -- this command's own audit trail, replacing + the InstructorTask trail a real Celery submission would have left. + + Called from handle()'s finally block inside a try/except. If this + write fails while an original exception is already propagating + (KeyboardInterrupt/MemoryError mid-run), the failure is logged and + swallowed so it cannot replace that original exception -- the + operator's only signal for why the run stopped. On an otherwise + successful run there is nothing to mask, so the failure is raised + as a CommandError instead: an export that reports success with no + audit trail on disk is worse than a loud failure. + """ + path = os.path.join(output_dir, MANIFEST_FILENAME) + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, 'w', encoding='utf-8') as manifest_file: + json.dump(manifest, manifest_file, indent=2, default=text_type) + + def _course_has_report_error(self, course_status): + """ + True if `course_status` (one course's own entry in + manifest['courses'] -- a dict of {report_name: per-report status + dict}) has at least one report that lost something, for the + purposes of handle()'s own manifest['courses_with_errors'] rollup + (see module docstring). + + Two things count: + - A report whose own `status` is `'error'`. + - A nominally-`'success'` report that also recorded a secondary + sub-artifact failure -- today only `_run_grades`'s + `grades_errors_export_error` does this: that failure is + deliberately NOT allowed to flip the report's own `status` (the + primary grades data was written correctly, see _run_grades's own + docstring), but a real per-course file (grades_errors.csv) was + still lost, and the run-level rollup must not miss that. + + One thing deliberately does NOT count: `'skipped'` (today only + `_run_ora2`'s unverifiable-identity-column outcome) -- that is a + deliberate, already-documented abstention from writing an unsafe + unfiltered file, not a failure to write something that should have + succeeded. + + `--skip-course` entries (`{'_skipped': '--skip-course'}` -- a flat + dict of strings, not per-report status dicts) are never actually + processed and must not be misread as an error. + """ + if '_skipped' in course_status: + return False + for report_status in course_status.values(): + if not isinstance(report_status, dict): + continue + if report_status.get('status') == 'error': + return True + if any(key.endswith('_export_error') for key in report_status): + return True + return False + + def _course_needs_review(self, course_status): + """ + True if `course_status` has at least one report that's a plain + `'success'` (or `'skipped'`) but still worth an operator's attention + -- NOT counted by `_course_has_report_error` above, since nothing + actually failed, but real data is still missing or suspect for the + purposes of handle()'s own manifest['courses_needing_review'] + rollup (see module docstring). Deliberately kept SEPARATE from + `courses_with_errors` (and does not affect `manifest['status']`, + which stays 'complete' here) -- this is a weaker, "you may want to + look at this" signal, not "something failed". + + Two things count: + - Any report carrying its own `warning` key (`_empty_filter_warning` + -- every one of that course's rows for that report was filtered + out by the tenant-membership filter, which usually means the + identity column didn't actually match rather than "this course + genuinely has zero tenant rows"). + - A report whose `status` is `'skipped'` (today only `_run_ora2`'s + unverifiable-identity-column outcome) -- a deliberate abstention, + not a failure, but that course's ora2 data is still genuinely + absent from this export and an operator deciding whether that's + acceptable needs a top-level pointer to it, not just a per-course + JSON entry nobody was told to go look for. + + `--skip-course` entries are never counted (never actually processed). + """ + if '_skipped' in course_status: + return False + for report_status in course_status.values(): + if not isinstance(report_status, dict): + continue + if report_status.get('status') == 'skipped': + return True + if 'warning' in report_status: + return True + return False + + # ------------------------------------------------------------------ report invocation + + def _invoke_report(self, call_fn): + """ + Call one of the five report-generator functions via the direct-call + seam, returning ((header, rows), raw_result, error_result). + `raw_result` is the function's own return value (a progress dict, + or the 'failed' sentinel string for export_ora2_data) -- callers + branch on it before trusting `(header, rows)`. `(header, rows)` is + None if the call produced no captured upload (the ora2 internal- + failure path, or anything unexpected). + + `error_result` surfaces any SEPARATE captured upload whose + `csv_name` ends in `_err` -- as (error_header, error_rows), or None + if no such upload was captured. Only CourseGradeReport._upload + actually produces one of these today (the grade_report_err upload + listing students CourseGradeFactory failed to grade, + `tasks_helper/grades.py`); every other caller here will simply get + `error_result=None` back. Previously the `_err`-suffixed upload was + filtered out of `primary` and silently discarded entirely, along + with the students it named -- see _run_grades for how it's used. + """ + buffer = [] + with _celery_free_context(), _capture_csv_uploads(buffer): + raw_result = call_fn() + if raw_result == UPDATE_STATUS_FAILED: + return None, raw_result, None + + error_result = None + error_entries = [entry for entry in buffer if entry['csv_name'].endswith('_err')] + if error_entries and error_entries[0]['rows']: + error_rows_with_header = error_entries[0]['rows'] + error_result = (error_rows_with_header[0], error_rows_with_header[1:]) + + primary = [entry for entry in buffer if not entry['csv_name'].endswith('_err')] + if not primary: + return None, raw_result, error_result + rows = primary[0]['rows'] + if not rows: + return ([], []), raw_result, error_result + header, data_rows = rows[0], rows[1:] + return (header, data_rows), raw_result, error_result + + def _filter_rows_by_column(self, header, rows, column_name, allowed_values): + """ + Filter `rows` to those whose `column_name` value is in + `allowed_values` -- the tenant-membership filter every sink applies + (structural fix #1, see module docstring). Raises (caught by the + per-course try/except in `_process_course`) if the expected identity + column is missing -- refusing to silently ship an unfiltered export + is safer than guessing. + """ + if column_name not in header: + raise CommandError( + u"Expected identity column '{0}' not found in report header {1} -- refusing to " + u"write an unfiltered tenant export.".format(column_name, header) + ) + return self._filter_rows_by_index(rows, header.index(column_name), allowed_values) + + def _filter_rows_by_index(self, rows, idx, allowed_values): + """ + Filter `rows` to those whose value at `idx` is in `allowed_values`, + comparing as strings on both sides. Report values are NOT reliably + the same Python type as the id/username sets this command resolves + independently -- confirmed from source: + `enrolled_students_features`'s `extract_attr` + (`instructor_analytics/basic.py`) stringifies every profile feature + it returns, including `id`, via a `DjangoJSONEncoder().default(attr)` + call made directly (not through the normal encode path) -- the base + `json.JSONEncoder.default()` unconditionally raises `TypeError` for + a plain `int` (verified: `json.JSONEncoder().default(42)` raises), + and `DjangoJSONEncoder` doesn't special-case `int`, so the `except` + branch's `six.text_type(attr)` always fires, turning `student.id` + into `'42'`, not `42`. Comparing raw types here would silently match + nothing against `user_ids` (a set of real ints from `_get_user_ids`) + for every profiles/enrollments row, every run. Stringifying both + sides is safe everywhere else too: grades' `Student ID` is a real + int on both sides and still matches after stringifying; ORA2 anon + ids/usernames and problem_responses usernames are already strings. + """ + allowed = {text_type(value) for value in allowed_values} + return [row for row in rows if idx < len(row) and text_type(row[idx]) in allowed] + + def _empty_filter_warning(self, rows, filtered): + """ + If tenant filtering zeroed out an otherwise non-empty report, + surface that loudly (manifest + log) rather than reporting a quiet + `'rows': 0` success. An all-rows-filtered-out result is far more + likely to mean the identity column didn't actually match (wrong + type, wrong column) than "this course genuinely has zero tenant + rows" -- every course this command processes was resolved because + at least one tenant member is enrolled in it or it matched the + tenant's org filter (structural fix #2), so a total wipeout on a + non-empty report is a signal worth an operator's attention, not a + silent success. + """ + if rows and not filtered: + warning = u"all {0} row(s) were filtered out -- check the identity column".format(len(rows)) + logger.warning("export_tenant_reports_csv: %s", warning) + return warning + return None + + def _detect_ora2_identity_column(self, header, rows, course_id, override_column): + """ + Verify an ora2 identity column BY CONTENT against two identity + spaces this command controls, rather than guessing a column name + (see module docstring's ora2 gap). Returns (column_index, label) + where label is 'anonymous_user_id' or 'username', or (None, None) + if no column qualifies. `override_column`, if given, bypasses + detection entirely (an operator who has confirmed the real schema). + """ + if override_column: + if override_column not in header: + raise CommandError( + u"--ora2-identity-column '{0}' not found in ora2 report header {1}.".format( + override_column, header + ) + ) + return header.index(override_column), 'override' + + if not rows: + return None, None + + anon_superset = set( + AnonymousUserId.objects.using(read_replica_or_default()) + .filter(course_id=course_id).values_list('anonymous_user_id', flat=True) + ) + enrolled_user_ids = ( + CourseEnrollment.objects.using(read_replica_or_default()) + .filter(course_id=course_id).values_list('user_id', flat=True) + ) + username_superset = set( + get_user_model().objects.using(read_replica_or_default()) + .filter(id__in=enrolled_user_ids).values_list('username', flat=True) + ) + candidates = [('anonymous_user_id', anon_superset), ('username', username_superset)] + + for col_idx in range(len(header)): + values = [row[col_idx] for row in rows if col_idx < len(row) and row[col_idx] not in (None, '')] + if not values: + continue + for label, value_set in candidates: + if value_set and all(value in value_set for value in values): + return col_idx, label + return None, None + + # ------------------------------------------------------------------ per-course orchestration + + def _process_course(self, course_id, reports, user_ids, tenant_usernames, feature_list, operator_user, + ora2_identity_column, per_course_dir, sinks, seen_learner_ids): + """ + Run every requested report type against one course, isolated from + every other course by the try/except inside each `_run_*` method -- + a broken/draft/zero-enrollment course must not abort a run covering + many courses (e.g. CourseGradeReport._compile's `zip(*batched_rows)` + raises ValueError for a zero-enrollment course, confirmed from + source). + """ + status = {} + needed_keys = {GENERATOR_KEY_FOR_REPORT[r] for r in reports} + + if 'grades' in needed_keys: + status['grades'] = self._run_grades(course_id, user_ids, per_course_dir, sinks.get('grades_summary')) + + if 'student_features' in needed_keys: + want_profiles = 'profiles' in reports + want_enrollments = 'enrollments' in reports + profile_status = self._run_student_features( + course_id, user_ids, feature_list, per_course_dir, want_profiles, want_enrollments, + sinks.get('learner_profile'), sinks.get('course_enrollments'), seen_learner_ids, + ) + if want_profiles: + status['profiles'] = profile_status + if want_enrollments: + status['enrollments'] = profile_status + + if 'problem_responses' in needed_keys: + status['problem_responses'] = self._run_problem_responses( + course_id, tenant_usernames, operator_user, per_course_dir, sinks.get('problem_responses'), + ) + + if 'ora2' in needed_keys: + status['ora2'] = self._run_ora2( + course_id, user_ids, ora2_identity_column, per_course_dir, sinks.get('ora2_responses'), + ) + + if 'may_enroll' in needed_keys: + status['may_enroll'] = self._run_may_enroll(course_id, per_course_dir, sinks.get('may_enroll_info')) + + return status + + def _run_grades(self, course_id, user_ids, per_course_dir, sink): + """ + CourseGradeReport.generate -- identity column is 'Student ID' + (`user.id`, confirmed from `_success_headers`/`_rows_for_users`). + + CourseGradeReport also uploads a SEPARATE grade_report_err CSV + (`tasks_helper/grades.py:_upload`) listing every learner + CourseGradeFactory failed to grade -- previously silently dropped + by _invoke_report along with the failure count itself + (`context.task_progress.failed`, discarded by this method's own + `result, _ = ...`). Those error rows get the same tenant-membership + filter as every other sink here (see module docstring's structural + fix #1) and land in their own per-course file rather than vanishing. + """ + try: + result, raw_result, error_result = self._invoke_report( + lambda: grades_module.CourseGradeReport.generate(None, None, course_id, {}, 'grades') + ) + if result is None: + return {'status': 'error', 'error': 'no upload captured'} + header, rows = result + if not header: + return {'status': 'success', 'rows': 0} + filtered = self._filter_rows_by_column(header, rows, 'Student ID', user_ids) + warning = self._empty_filter_warning(rows, filtered) + + summary_rows = None + if sink is not None: + # Validate GRADES_SUMMARY_COLUMNS presence BEFORE writing anything -- + # a missing summary column must fail this course cleanly (caught + # below) rather than after the per-course file has already landed. + indices = [header.index(col) for col in GRADES_SUMMARY_COLUMNS] + summary_rows = [[text_type(course_id)] + [row[idx] for idx in indices] for row in filtered] + + row_count = self._write_course_csv(per_course_dir, course_id, 'grades', header, filtered) + if summary_rows is not None: + sink.write_rows(summary_rows) + + status = {'status': 'success', 'rows': row_count} + if warning: + status['warning'] = warning + + if error_result is not None: + error_header, error_rows = error_result + # Safe to record up front -- no IO, and the operator needs this count + # even if the error-row export below (filtering/writing + # grades_errors.csv) fails. These two counts can legitimately differ: + # 'failed' is CourseGradeReport's own course-wide count + # (context.task_progress.failed, every learner it couldn't grade, + # regardless of tenant); 'failed_rows_exported' (below) is how many of + # those rows actually belong to THIS tenant -- a course shared with + # another tenant can have failures that aren't this tenant's to see. + if isinstance(raw_result, dict): + status['failed'] = raw_result.get('failed', len(error_rows)) + else: + status['failed'] = len(error_rows) + try: + filtered_error_rows = self._filter_rows_by_column( + error_header, error_rows, 'Student ID', user_ids) + self._write_course_csv( + per_course_dir, course_id, 'grades_errors', error_header, filtered_error_rows) + status['failed_rows_exported'] = len(filtered_error_rows) + except Exception as exc: # pylint: disable=broad-except + # This runs AFTER the primary grades CSV/summary rows are already + # written to disk -- isolated in its own try/except so a failure here + # (writing grades_errors.csv, or the identity-column filter above) + # can't retroactively flip an otherwise-successful course to 'error' + # and misreport grades data that was actually written correctly. + logger.exception( + "export_tenant_reports_csv: grades error rows failed for %s", course_id + ) + status['grades_errors_export_error'] = text_type(exc) + + return status + except Exception as exc: # pylint: disable=broad-except + logger.exception("export_tenant_reports_csv: grades failed for %s", course_id) + return {'status': 'error', 'error': text_type(exc)} + + def _run_student_features(self, course_id, user_ids, feature_list, per_course_dir, want_profiles, + want_enrollments, profile_sink, enrollment_sink, seen_learner_ids): + """ + upload_students_csv -- task_input here is the bare feature list + itself, not a dict (confirmed from instructor_task/api.py's + submit_calculate_students_features_csv). Identity column is 'id' + (forced into feature_list by `_resolve_include_fields`). + """ + try: + result, _, _ = self._invoke_report( + lambda: enrollments_module.upload_students_csv(None, None, course_id, feature_list, 'features') + ) + if result is None: + return {'status': 'error', 'error': 'no upload captured'} + header, rows = result + if not header: + return {'status': 'success', 'rows': 0} + filtered = self._filter_rows_by_column(header, rows, 'id', user_ids) + warning = self._empty_filter_warning(rows, filtered) + + if want_profiles: + self._write_course_csv(per_course_dir, course_id, 'profiles', header, filtered) + if want_enrollments: + self._write_course_csv(per_course_dir, course_id, 'enrollments', header, filtered) + + id_idx = header.index('id') + if want_profiles and profile_sink is not None: + constant_columns = [c for c in header if c not in COURSE_VARYING_PROFILE_FEATURES] + indices = [header.index(c) for c in constant_columns] + new_rows = [] + for row in filtered: + learner_id = row[id_idx] + if learner_id in seen_learner_ids: + continue + seen_learner_ids.add(learner_id) + new_rows.append([row[idx] for idx in indices]) + profile_sink.write_rows(new_rows) + + if want_enrollments and enrollment_sink is not None: + course_varying_present = [c for c in COURSE_VARYING_PROFILE_FEATURES if c in header] + if course_varying_present: + indices = [header.index(c) for c in course_varying_present] + new_rows = [ + [text_type(course_id), row[id_idx]] + [row[idx] for idx in indices] + for row in filtered + ] + enrollment_sink.write_rows(new_rows) + + status = {'status': 'success', 'rows': len(filtered)} + if warning: + status['warning'] = warning + return status + except Exception as exc: # pylint: disable=broad-except + logger.exception("export_tenant_reports_csv: student_features failed for %s", course_id) + return {'status': 'error', 'error': text_type(exc)} + + def _run_problem_responses(self, course_id, tenant_usernames, operator_user, per_course_dir, sink): + """ + ProblemResponses.generate -- task_input is a dict requiring + 'problem_locations' (the course root usage key, as a string) and + 'user_id' (the operator's id). Identity column is 'username' + (confirmed from `_build_student_data`'s student_data_keys_list). + """ + try: + task_input = { + 'problem_locations': text_type(modulestore().make_course_usage_key(course_id)), + 'user_id': operator_user.id, + } + result, _, _ = self._invoke_report( + lambda: grades_module.ProblemResponses.generate(None, None, course_id, task_input, 'responses') + ) + if result is None: + return {'status': 'error', 'error': 'no upload captured'} + header, rows = result + if not header: + return {'status': 'success', 'rows': 0} + filtered = self._filter_rows_by_column(header, rows, 'username', tenant_usernames) + warning = self._empty_filter_warning(rows, filtered) + row_count = self._write_course_csv(per_course_dir, course_id, 'problem_responses', header, filtered) + if sink is not None: + sink.add_rows(course_id, header, filtered) + status = {'status': 'success', 'rows': row_count} + if warning: + status['warning'] = warning + return status + except Exception as exc: # pylint: disable=broad-except + logger.exception("export_tenant_reports_csv: problem_responses failed for %s", course_id) + return {'status': 'error', 'error': text_type(exc)} + + def _run_ora2(self, course_id, user_ids, ora2_identity_column, per_course_dir, sink): + """ + upload_ora2_data -- returns the plain string 'failed' + (UPDATE_STATUS_FAILED) on internal failure instead of raising or + returning an empty report. Identity column is detected by content, + not name (see module docstring's ora2 gap / `_detect_ora2_identity_column`). + """ + try: + result, raw_result, _ = self._invoke_report( + lambda: misc_module.upload_ora2_data(None, None, course_id, {}, 'ora2') + ) + if raw_result == UPDATE_STATUS_FAILED: + return {'status': 'error', 'error': 'upload_ora2_data returned the failure sentinel'} + if result is None: + return {'status': 'error', 'error': 'no upload captured'} + header, rows = result + if not header: + return {'status': 'success', 'rows': 0} + + col_idx, label = self._detect_ora2_identity_column(header, rows, course_id, ora2_identity_column) + if col_idx is None: + logger.warning( + "export_tenant_reports_csv: could not verify an identity column in the ora2 report " + "for %s -- skipping to avoid writing an unfiltered cross-tenant export (see module " + "docstring's ora2 gap; pass --ora2-identity-column to override).", + course_id, + ) + return {'status': 'skipped', 'reason': 'identity column unverified'} + + tenant_anon_ids = set( + AnonymousUserId.objects.using(read_replica_or_default()) + .filter(course_id=course_id, user_id__in=user_ids) + .values_list('anonymous_user_id', flat=True) + ) + tenant_usernames = set( + get_user_model().objects.using(read_replica_or_default()) + .filter(id__in=user_ids).values_list('username', flat=True) + ) + if label == 'anonymous_user_id': + tenant_values = tenant_anon_ids + elif label == 'username': + tenant_values = tenant_usernames + else: + # 'override' -- an operator-supplied column of unconfirmed shape (that's the + # whole point of the override: detection couldn't verify it by content). Match + # against the UNION of every tenant identity space this command knows, rather + # than guessing which one the operator's column actually holds -- assuming it's + # a raw user-id column (the previous, wrong behavior here) silently zeroed out + # every override run whose column was actually an anon-id or username column. + tenant_values = tenant_anon_ids | tenant_usernames | set(user_ids) + + filtered = self._filter_rows_by_index(rows, col_idx, tenant_values) + warning = self._empty_filter_warning(rows, filtered) + row_count = self._write_course_csv(per_course_dir, course_id, 'ora2', header, filtered) + if sink is not None: + sink.add_rows(course_id, header, filtered) + status = {'status': 'success', 'rows': row_count, 'identity_column': header[col_idx]} + if warning: + status['warning'] = warning + return status + except Exception as exc: # pylint: disable=broad-except + logger.exception("export_tenant_reports_csv: ora2 failed for %s", course_id) + return {'status': 'error', 'error': text_type(exc)} + + def _run_may_enroll(self, course_id, per_course_dir, sink): + """ + upload_may_enroll_csv -- NOT tenant-membership-filtered by design + (CourseEnrollmentAllowed rows have no user account, see module + docstring); only reached when explicitly requested via --reports. + """ + try: + result, _, _ = self._invoke_report( + lambda: enrollments_module.upload_may_enroll_csv( + None, None, course_id, {'features': list(MAY_ENROLL_DEFAULT_FEATURES)}, 'may_enroll' + ) + ) + if result is None: + return {'status': 'error', 'error': 'no upload captured'} + header, rows = result + if not header: + return {'status': 'success', 'rows': 0, 'caveat': 'not tenant-membership-filtered'} + row_count = self._write_course_csv(per_course_dir, course_id, 'may_enroll', header, rows) + if sink is not None: + summary_rows = [[text_type(course_id)] + row for row in rows] + sink.write_rows(summary_rows) + return {'status': 'success', 'rows': row_count, 'caveat': 'not tenant-membership-filtered'} + except Exception as exc: # pylint: disable=broad-except + logger.exception("export_tenant_reports_csv: may_enroll failed for %s", course_id) + return {'status': 'error', 'error': text_type(exc)} + + # ------------------------------------------------------------------ dry run / reporting + + def _dry_run(self, course_ids, reports): + """ + Print the resolved (course x report-type) matrix without generating anything. + """ + self.stdout.write(u"\nCourse x Report matrix ({0} courses):".format(len(course_ids))) + for course_id in sorted(course_ids, key=text_type): + self.stdout.write(u" {0}: {1}".format(course_id, ', '.join(reports))) + self.stdout.write(self.style.WARNING("\nDRY RUN -- no files written, no reports generated.")) + + def _print_header(self, slug, reports, dry_run): + """ + Print the command's run header. + """ + self.stdout.write("\n" + "=" * 72) + self.stdout.write(u"Export tenant CSV reports: {0}".format(slug) + (" [DRY RUN]" if dry_run else "")) + self.stdout.write(u"Reports: {0}".format(', '.join(reports))) + self.stdout.write("=" * 72) diff --git a/openedx/features/edly/management/commands/tests/test_export_tenant_reports_csv.py b/openedx/features/edly/management/commands/tests/test_export_tenant_reports_csv.py new file mode 100644 index 000000000000..7a950935951b --- /dev/null +++ b/openedx/features/edly/management/commands/tests/test_export_tenant_reports_csv.py @@ -0,0 +1,1887 @@ +""" +Tests for the export_tenant_reports_csv management command. + +Tenant/course/user resolution is exercised against the real ORM (sqlite in +test settings handles these fine, matching the pattern used throughout +openedx/features/edly/tests/ and test_export_learner_data.py). Report +invocation is exercised via the exact patch pattern test_tasks_helper.py +already uses to call these functions outside Celery +(`runner._get_current_task`, per-module `upload_csv_to_report_store`) -- +the fake report-generator functions below call that same real, unpatched +`_capture_csv_uploads`/`_celery_free_context` machinery, so what's under +test is this command's own capture/filter/merge/dedupe/manifest logic, not +a re-implementation of it. + +Known gap: none of the tests below exercise the real +`CourseGradeReport`/`ProblemResponses`/`upload_students_csv`/ +`upload_may_enroll_csv`/`upload_ora2_data` functions against a real course +fixture (ModuleStoreTestCase/TestReportMixin/InstructorTaskCourseTestCase, +the "gold standard" per this command's module docstring) -- that requires +a full devstack run, not exercised in the environment this file was +authored in. In particular the `problem_locations` course-root-usage-key +walk and the real `OraAggregateData.collect_ora2_data` column layout are +NOT verified end-to-end here; see the module docstring's own notes on both. + +`HandleTests` and `ReadReplicaRoutingTests` (added alongside the fixes +above) follow the same real-ORM/real-`call_command` pattern as the rest of +this file -- they were authored and reviewed against real source, but like +every other TestCase here, actually running them requires a real Django +test environment (sqlite test settings + migrations) that was not +available in the environment they were authored in either; see this +command's own module docstring for the read-replica note these tests cover. +""" +import csv +import json +import os +import stat +import tempfile +from collections import OrderedDict +from io import StringIO + +from django.conf import settings +from django.core.management import call_command +from django.core.management.base import CommandError +from django.test import TestCase +from mock import MagicMock, patch + +from common.djangoapps.student.models import AnonymousUserId +from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory +from openedx.features.edly.management.commands import export_tenant_reports_csv as mod +from openedx.features.edly.management.commands.export_tenant_reports_csv import ( + COURSE_VARYING_PROFILE_FEATURES, + DEFAULT_PROFILE_FEATURES, + DEFAULT_REPORTS, + GRADES_SUMMARY_COLUMNS, + REPORT_CHOICES, + Command, + _CsvSink, + _DictCsvSink, +) +from openedx.features.edly.tests.factories import EdlyMultiSiteAccessFactory, EdlySubOrganizationFactory +from student.tests.factories import CourseEnrollmentFactory, UserFactory + + +def _make_command(): + """ + Build a Command instance with stdout/style wired up for assertions, + same convention as test_export_learner_data.py's `_make_command`. + """ + command = Command() + command.stdout = StringIO() + command.stderr = StringIO() + command.style = MagicMock() + for attr in ('SUCCESS', 'ERROR', 'WARNING'): + setattr(command.style, attr, lambda value: value) + return command + + +class PatchTargetsRegressionTests(TestCase): + """ + Regression test asserting the direct-call seam this command relies on + still exists (see module docstring's "Execution mechanism" section): + a future upstream refactor of tasks_helper/ should fail loudly here, + not silently break this command. + """ + + def test_get_current_task_patch_seam_exists(self): + assert hasattr(mod.runner_module, '_get_current_task') + + def test_upload_csv_to_report_store_exists_on_all_three_modules(self): + assert hasattr(mod.grades_module, 'upload_csv_to_report_store') + assert hasattr(mod.enrollments_module, 'upload_csv_to_report_store') + assert hasattr(mod.misc_module, 'upload_csv_to_report_store') + + def test_five_report_generator_entry_points_exist(self): + assert hasattr(mod.grades_module, 'CourseGradeReport') + assert hasattr(mod.grades_module.CourseGradeReport, 'generate') + assert hasattr(mod.grades_module, 'ProblemResponses') + assert hasattr(mod.grades_module.ProblemResponses, 'generate') + assert hasattr(mod.enrollments_module, 'upload_students_csv') + assert hasattr(mod.enrollments_module, 'upload_may_enroll_csv') + assert hasattr(mod.misc_module, 'upload_ora2_data') + + def test_update_status_failed_sentinel_unchanged(self): + # upload_ora2_data returns this exact string on internal failure -- + # see module docstring / _run_ora2. + assert mod.UPDATE_STATUS_FAILED == 'failed' + + +class ParseReportsTests(TestCase): + """ + Tests for Command._parse_reports. + """ + + def test_default_reports_string_parses_to_the_documented_list(self): + command = _make_command() + assert command._parse_reports(','.join(DEFAULT_REPORTS)) == list(DEFAULT_REPORTS) + + def test_may_enroll_not_in_default_reports(self): + # Structural: may_enroll can never be tenant-membership-filtered + # (see module docstring) -- must stay opt-in only. + assert 'may_enroll' not in DEFAULT_REPORTS + assert 'may_enroll' in REPORT_CHOICES + + def test_unknown_report_type_raises_command_error(self): + command = _make_command() + with self.assertRaises(CommandError): + command._parse_reports('grades,not-a-real-report') + + def test_whitespace_and_empty_entries_are_tolerated(self): + command = _make_command() + assert command._parse_reports(' grades , profiles ,,') == ['grades', 'profiles'] + + +class ResolveIncludeFieldsTests(TestCase): + """ + Tests for Command._resolve_include_fields -- the 'meta' secrets guard + and the forced 'id' column (needed for membership filtering + the + learner_profile.csv dedup key). + """ + + def test_default_matches_documented_profile_feature_list(self): + command = _make_command() + assert command._resolve_include_fields(None, False) == list(DEFAULT_PROFILE_FEATURES) + + def test_meta_blocked_without_allow_meta_field(self): + command = _make_command() + with self.assertRaises(CommandError): + command._resolve_include_fields('id,meta', False) + + def test_meta_allowed_with_allow_meta_field(self): + command = _make_command() + fields = command._resolve_include_fields('id,meta', True) + assert 'meta' in fields + + def test_id_forced_even_when_caller_omits_it(self): + command = _make_command() + fields = command._resolve_include_fields('username,email', False) + assert 'id' in fields + + +class BuildFeatureListTests(TestCase): + """ + Tests for Command._build_feature_list -- the course-varying columns + are only requested when 'enrollments' is actually in --reports. + """ + + def test_adds_course_varying_columns_when_enrollments_requested(self): + command = _make_command() + features = command._build_feature_list(['id', 'username'], ['profiles', 'enrollments']) + for column in COURSE_VARYING_PROFILE_FEATURES: + assert column in features + + def test_omits_course_varying_columns_when_enrollments_not_requested(self): + command = _make_command() + features = command._build_feature_list(['id', 'username'], ['profiles']) + for column in COURSE_VARYING_PROFILE_FEATURES: + assert column not in features + + +class FilterRowsByColumnTests(TestCase): + """ + Tests for Command._filter_rows_by_column -- the shared tenant-membership + filter every report sink applies (structural fix #1, see module docstring). + """ + + def test_filters_rows_to_allowed_values(self): + command = _make_command() + header = ['Student ID', 'Email', 'Username'] + rows = [[1, 'a@x.com', 'alice'], [2, 'b@x.com', 'bob'], [3, 'c@x.com', 'carol']] + filtered = command._filter_rows_by_column(header, rows, 'Student ID', {1, 3}) + assert filtered == [[1, 'a@x.com', 'alice'], [3, 'c@x.com', 'carol']] + + def test_missing_identity_column_raises_rather_than_shipping_unfiltered(self): + command = _make_command() + with self.assertRaises(CommandError): + command._filter_rows_by_column(['x'], [['y']], 'Student ID', {1}) + + +class GetTenantCourseIdsTests(TestCase): + """ + Tests for Command._get_tenant_course_ids -- the amended course-set + union (structural fix #2, see module docstring). This is the single + scoping regression this command exists to not repeat. + """ + + def test_union_includes_org_filtered_and_member_enrolled_courses(self): + command = _make_command() + org_course = CourseOverviewFactory(org='tenant-org') + outside_course = CourseOverviewFactory(org='other-org') + member = UserFactory() + CourseEnrollmentFactory(user=member, course_id=outside_course.id, is_active=True) + + course_ids = command._get_tenant_course_ids(['tenant-org'], {member.id}) + + # The whole point of the union: a member's enrollment in a course + # OUTSIDE the tenant's resolved orgs must still surface here -- + # org-filtering alone would silently miss it. + assert org_course.id in course_ids + assert outside_course.id in course_ids + + def test_inactive_enrollment_not_included_via_member_path(self): + command = _make_command() + course = CourseOverviewFactory(org='other-org') + member = UserFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=False) + + course_ids = command._get_tenant_course_ids([], {member.id}) + + assert course.id not in course_ids + + def test_returns_real_course_key_objects_not_strings(self): + # The five report generators require actual CourseKey instances + # (confirmed from test_tasks_helper.py -- every call there passes + # self.course.id, not str(self.course.id)) -- unlike + # export_learner_data.py's _get_course_ids, which stringifies for + # JSON/display purposes only. + command = _make_command() + course = CourseOverviewFactory(org='tenant-org') + course_ids = command._get_tenant_course_ids(['tenant-org'], set()) + assert course.id in course_ids + assert not any(isinstance(cid, str) for cid in course_ids) + + +class GetSubOrgAndUserIdsTests(TestCase): + """ + Sanity checks that this command's copies of _get_sub_org/_get_user_ids + (reused verbatim from export_learner_data.py's pattern, see module + docstring) behave identically. + """ + + def test_get_sub_org_not_found_raises_command_error(self): + command = _make_command() + with self.assertRaises(CommandError): + command._get_sub_org('does-not-exist') + + def test_get_user_ids_uses_multisite_access_membership_only(self): + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + enrolled_only_user = UserFactory() + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=enrolled_only_user, course_id=course.id) + + command = _make_command() + user_ids = command._get_user_ids(sub_org) + + assert user_ids == {member.id} + + +class DetectOra2IdentityColumnTests(TestCase): + """ + Tests for Command._detect_ora2_identity_column -- content-based + detection against real AnonymousUserId/CourseEnrollment/User rows (see + module docstring's ora2 gap: this command does NOT guess a column name). + """ + + def test_detects_anonymous_user_id_column_by_content(self): + command = _make_command() + course = CourseOverviewFactory() + user = UserFactory() + AnonymousUserId.objects.create(user=user, course_id=course.id, anonymous_user_id='a' * 32) + + header = ['Submission ID', 'Anon Col', 'Response Text'] + rows = [['sub-1', 'a' * 32, 'The answer is 42']] + + col_idx, label = command._detect_ora2_identity_column(header, rows, course.id, None) + + assert (col_idx, label) == (1, 'anonymous_user_id') + + def test_detects_username_column_by_content_when_no_anon_id_match(self): + command = _make_command() + course = CourseOverviewFactory() + user = UserFactory(username='scorer_1') + CourseEnrollmentFactory(user=user, course_id=course.id) + + header = ['Submission ID', 'Scorer', 'Response Text'] + rows = [['sub-1', 'scorer_1', 'looks good']] + + col_idx, label = command._detect_ora2_identity_column(header, rows, course.id, None) + + assert (col_idx, label) == (1, 'username') + + def test_no_qualifying_column_returns_none_none(self): + command = _make_command() + course = CourseOverviewFactory() + + header = ['Submission ID', 'Response Text'] + rows = [['sub-1', 'free text nobody can be identified by']] + + col_idx, label = command._detect_ora2_identity_column(header, rows, course.id, None) + + assert (col_idx, label) == (None, None) + + def test_override_column_bypasses_detection(self): + command = _make_command() + course = CourseOverviewFactory() + header = ['Submission ID', 'Whatever Col', 'Response Text'] + rows = [['sub-1', 'anything', 'text']] + + col_idx, label = command._detect_ora2_identity_column(header, rows, course.id, 'Whatever Col') + + assert (col_idx, label) == (1, 'override') + + def test_override_column_missing_from_header_raises(self): + command = _make_command() + course = CourseOverviewFactory() + with self.assertRaises(CommandError): + command._detect_ora2_identity_column(['a', 'b'], [['1', '2']], course.id, 'not-a-column') + + +class CsvSinkTests(TestCase): + """ + Tests for _CsvSink -- the streaming tenant-wide sink for fixed-header + reports (grades_summary/learner_profile/course_enrollments/may_enroll_info). + """ + + def test_writes_header_once_and_streams_rows(self): + path = os.path.join(tempfile.mkdtemp(), 'sink.csv') + sink = _CsvSink(path) + sink.write_header(['course_id', 'Student ID']) + sink.write_header(['ignored', 'second', 'call']) # must be a no-op + sink.write_rows([['course-1', 1], ['course-1', 2]]) + sink.close() + + with open(path) as f: + rows = list(csv.reader(f)) + assert rows == [['course_id', 'Student ID'], ['course-1', '1'], ['course-1', '2']] + assert sink.row_count == 2 + + def test_escapes_csv_formula_injection_in_header_and_data(self): + """ + Item 6 regression: QUOTE_ALL alone does not stop a spreadsheet app + from evaluating a cell starting with =, +, -, or @ as a formula. + """ + path = os.path.join(tempfile.mkdtemp(), 'sink.csv') + sink = _CsvSink(path) + sink.write_header(['name', '=cmd(1,1)']) + sink.write_rows([['=cmd(1,1)'], ['+1+1'], ['-1+1'], ['@sum(1,1)'], ['alice']]) + sink.close() + + with open(path) as f: + rows = list(csv.reader(f)) + assert rows == [ + ['name', "'=cmd(1,1)"], + ["'=cmd(1,1)"], + ["'+1+1"], + ["'-1+1"], + ["'@sum(1,1)"], + ['alice'], + ] + + +class DictCsvSinkTests(TestCase): + """ + Tests for _DictCsvSink -- the buffered tenant-wide sink for reports + whose per-course header is not a predetermined constant + (problem_responses/ora2, see module docstring). + """ + + def test_unions_fieldnames_across_courses_with_differing_headers(self): + path = os.path.join(tempfile.mkdtemp(), 'sink.csv') + sink = _DictCsvSink(path) + sink.add_rows('course-1', ['username', 'state'], [['alice', 's1']]) + sink.add_rows('course-2', ['username', 'state', 'extra_col'], [['bob', 's2', 'xyz']]) + sink.close() + + with open(path) as f: + rows = list(csv.DictReader(f)) + assert set(rows[0].keys()) == {'course_id', 'username', 'state', 'extra_col'} + assert rows[0]['extra_col'] == '' # course-1 predates that column + assert rows[1]['course_id'] == 'course-2' + assert rows[1]['extra_col'] == 'xyz' + assert sink.row_count == 2 + + def test_escapes_csv_formula_injection_in_header_and_data(self): + """ + Item 6 regression, _DictCsvSink variant -- its fieldnames come from + each course's own report header (xblock/ora2 column names), not a + fixed constant, so the header needs the same escaping as the data. + """ + path = os.path.join(tempfile.mkdtemp(), 'sink.csv') + sink = _DictCsvSink(path) + sink.add_rows('course-1', ['=malicious_header', 'response'], [['x', '=cmd(1,1)']]) + sink.close() + + with open(path) as f: + reader = csv.reader(f) + header_row = next(reader) + data_row = next(reader) + assert header_row == ['course_id', "'=malicious_header", 'response'] + assert data_row[2] == "'=cmd(1,1)" + + +class EscapeCsvFormulaTests(TestCase): + """ + Tests for the module-level _escape_csv_formula helper (item 6). + """ + + def test_prefixes_values_starting_with_formula_trigger_characters(self): + for trigger in ('=', '+', '-', '@'): + value = trigger + 'cmd(1,1)' + assert mod._escape_csv_formula(value) == "'" + value + + def test_leaves_ordinary_strings_untouched(self): + assert mod._escape_csv_formula('alice') == 'alice' + assert mod._escape_csv_formula('') == '' + + def test_leaves_non_string_values_untouched(self): + assert mod._escape_csv_formula(42) == 42 + assert mod._escape_csv_formula(None) is None + assert mod._escape_csv_formula(True) is True + + +class WriteCourseCsvTests(TestCase): + """ + Tests for Command._write_course_csv -- the per-course raw-writer path + (full column fidelity, no forced alignment across courses, see module + docstring), including its formula-injection escaping (item 6). + """ + + def test_writes_header_and_rows_verbatim(self): + command = _make_command() + per_course_dir = tempfile.mkdtemp() + row_count = command._write_course_csv( + per_course_dir, 'course-1', 'grades', ['Student ID', 'Username'], [['1', 'alice'], ['2', 'bob']], + ) + assert row_count == 2 + with open(os.path.join(per_course_dir, 'course-1__grades.csv')) as f: + rows = list(csv.reader(f)) + assert rows == [['Student ID', 'Username'], ['1', 'alice'], ['2', 'bob']] + + def test_escapes_csv_formula_injection_in_header_and_data(self): + # CourseGradeReport's real header can include course-author-supplied + # experiment-partition/assignment names -- not always a fixed constant. + command = _make_command() + per_course_dir = tempfile.mkdtemp() + command._write_course_csv( + per_course_dir, 'course-1', 'grades', ['name', '=Experiment Group (evil)'], [['=cmd(1,1)', 'x']], + ) + with open(os.path.join(per_course_dir, 'course-1__grades.csv')) as f: + rows = list(csv.reader(f)) + assert rows == [['name', "'=Experiment Group (evil)"], ["'=cmd(1,1)", 'x']] + + +class InvokeReportAndRunGradesTests(TestCase): + """ + Tests exercising Command._invoke_report / _run_grades through the REAL + _celery_free_context/_capture_csv_uploads machinery -- the fake + `CourseGradeReport.generate` below calls the same + `upload_csv_to_report_store([header] + rows, ...)` shape the real + function calls (confirmed from tasks_helper/grades.py), so this proves + the capture/celery-free plumbing itself, not a re-implementation of it. + """ + + def test_invoke_report_captures_rows_and_filters_to_tenant(self): + command = _make_command() + + def fake_generate(_a, _b, course_id, _task_input, _action_name): + header = ['Student ID', 'Email', 'Username', 'Enrollment Status'] + rows = [[1, 'a@x.com', 'alice', 'enrolled'], [99, 'z@x.com', 'zeke', 'enrolled']] + mod.grades_module.upload_csv_to_report_store([header] + rows, 'grade_report', course_id, None) + return {'succeeded': 2} + + with patch.object(mod.grades_module.CourseGradeReport, 'generate', staticmethod(fake_generate)): + per_course_dir = tempfile.mkdtemp() + status = command._run_grades('course-1', {1}, per_course_dir, None) + + assert status == {'status': 'success', 'rows': 1} + with open(os.path.join(per_course_dir, 'course-1__grades.csv')) as f: + written = list(csv.reader(f)) + assert written == [ + ['Student ID', 'Email', 'Username', 'Enrollment Status'], + ['1', 'a@x.com', 'alice', 'enrolled'], + ] + + def test_run_grades_writes_tenant_wide_summary_with_fixed_columns(self): + command = _make_command() + + def fake_generate(_a, _b, course_id, _task_input, _action_name): + header = ['Student ID', 'Email', 'Username'] + GRADES_SUMMARY_COLUMNS[3:] + rows = [[1, 'a@x.com', 'alice'] + ['0.9', 'honor', 'N/A', 'N', 'N', '', 'enrolled']] + mod.grades_module.upload_csv_to_report_store([header] + rows, 'grade_report', course_id, None) + return {'succeeded': 1} + + with patch.object(mod.grades_module.CourseGradeReport, 'generate', staticmethod(fake_generate)): + per_course_dir = tempfile.mkdtemp() + sink_path = os.path.join(tempfile.mkdtemp(), 'grades_summary.csv') + sink = _CsvSink(sink_path) + sink.write_header(['course_id'] + GRADES_SUMMARY_COLUMNS) + status = command._run_grades('course-1', {1}, per_course_dir, sink) + sink.close() + + assert status['status'] == 'success' + with open(sink_path) as f: + rows = list(csv.DictReader(f)) + assert rows[0]['course_id'] == 'course-1' + assert rows[0]['Student ID'] == '1' + # Item 1 regression: grades_summary.csv previously had no grade in it at all + # (GRADES_SUMMARY_COLUMNS omitted 'Grade' even though CourseGradeReport's real + # header always emits it right after the identity columns). + assert 'Grade' in rows[0] + assert rows[0]['Grade'] == '0.9' + + def test_zero_enrollment_value_error_is_isolated_not_raised(self): + """ + CourseGradeReport._compile's `zip(*batched_rows)` raises ValueError + for a zero-enrollment course (confirmed from source: `zip()` with + no arguments unpacked into two names raises). One broken/empty + course must not abort a run covering many courses. + """ + command = _make_command() + + def raising_generate(*args, **kwargs): + raise ValueError("not enough values to unpack (expected 2, got 0)") + + with patch.object(mod.grades_module.CourseGradeReport, 'generate', staticmethod(raising_generate)): + per_course_dir = tempfile.mkdtemp() + status = command._run_grades('course-empty', {1}, per_course_dir, None) + + assert status['status'] == 'error' + assert 'not enough values' in status['error'] + + def test_missing_summary_column_fails_before_writing_the_per_course_file(self): + """ + Regression test: GRADES_SUMMARY_COLUMNS presence must be validated + BEFORE the per-course file is written, not after -- otherwise a + missing summary column marks the course 'error' in the manifest + while a per-course file sits on disk claiming otherwise. + """ + command = _make_command() + + def fake_generate_missing_column(_a, _b, course_id, _task_input, _action_name): + # Deliberately omits 'Certificate Type', one of GRADES_SUMMARY_COLUMNS -- + # 'Grade' IS present so this test isolates only the missing-'Certificate + # Type' case, not a (separate, already-covered) missing-'Grade' case. + header = [ + 'Student ID', 'Email', 'Username', 'Grade', 'Enrollment Track', 'Verification Status', + 'Certificate Eligible', 'Certificate Delivered', 'Enrollment Status', + ] + rows = [[1, 'a@x.com', 'alice', '0.9', 'honor', 'N/A', 'N', 'N', 'enrolled']] + mod.grades_module.upload_csv_to_report_store([header] + rows, 'grade_report', course_id, None) + return {'succeeded': 1} + + per_course_dir = tempfile.mkdtemp() + sink = _CsvSink(os.path.join(tempfile.mkdtemp(), 'grades_summary.csv')) + sink.write_header(['course_id'] + GRADES_SUMMARY_COLUMNS) + + with patch.object(mod.grades_module.CourseGradeReport, 'generate', staticmethod(fake_generate_missing_column)): + status = command._run_grades('course-1', {1}, per_course_dir, sink) + sink.close() + + assert status['status'] == 'error' + assert not os.path.exists(os.path.join(per_course_dir, 'course-1__grades.csv')) + + def test_grade_report_err_rows_are_captured_and_failure_count_recorded(self): + """ + Item 4 regression: CourseGradeReport._upload (tasks_helper/grades.py) + uploads a SEPARATE grade_report_err CSV listing every learner + CourseGradeFactory failed to grade. _invoke_report previously + filtered this out entirely (any csv_name ending in '_err'), and + _run_grades discarded the raw_result dict carrying + context.task_progress.failed with `result, _ = ...`. Neither the + rows nor the count survived. This fake is the first in this file to + emit both a grade_report and a grade_report_err upload for the same + call (no existing fake did this before item 4). + """ + command = _make_command() + + def fake_generate_with_errors(_a, _b, course_id, _task_input, _action_name): + header = ['Student ID', 'Email', 'Username'] + GRADES_SUMMARY_COLUMNS[3:] + rows = [[1, 'a@x.com', 'alice'] + ['0.9', 'honor', 'N/A', 'N', 'N', '', 'enrolled']] + mod.grades_module.upload_csv_to_report_store([header] + rows, 'grade_report', course_id, None) + error_header = ['Student ID', 'Username', 'Error'] + error_rows = [[1, 'alice', 'grading exception'], [99, 'outsider', 'grading exception']] + mod.grades_module.upload_csv_to_report_store( + [error_header] + error_rows, 'grade_report_err', course_id, None, + ) + return {'succeeded': 1, 'failed': 2, 'attempted': 3, 'total': 3} + + with patch.object(mod.grades_module.CourseGradeReport, 'generate', staticmethod(fake_generate_with_errors)): + per_course_dir = tempfile.mkdtemp() + # user_ids={1}: 'outsider' (99) is not a tenant member. + status = command._run_grades('course-1', {1}, per_course_dir, None) + + assert status['status'] == 'success' + # Course-wide count from CourseGradeReport's own task_progress, NOT just the + # tenant-filtered rows actually written below. + assert status['failed'] == 2 + assert status['failed_rows_exported'] == 1 + + with open(os.path.join(per_course_dir, 'course-1__grades_errors.csv')) as f: + error_rows_written = list(csv.reader(f)) + # Tenant-filtered: user 99 ('outsider') must not appear. + assert error_rows_written == [ + ['Student ID', 'Username', 'Error'], + ['1', 'alice', 'grading exception'], + ] + + def test_no_grade_report_err_upload_means_no_failed_keys_in_status(self): + """ + When CourseGradeReport._upload didn't capture a grade_report_err + upload at all (the common case: no failures), _run_grades must not + fabricate 'failed'/'failed_rows_exported' keys or write an errors file. + """ + command = _make_command() + + def fake_generate_no_errors(_a, _b, course_id, _task_input, _action_name): + header = ['Student ID', 'Email', 'Username'] + GRADES_SUMMARY_COLUMNS[3:] + rows = [[1, 'a@x.com', 'alice'] + ['0.9', 'honor', 'N/A', 'N', 'N', '', 'enrolled']] + mod.grades_module.upload_csv_to_report_store([header] + rows, 'grade_report', course_id, None) + return {'succeeded': 1, 'failed': 0} + + with patch.object(mod.grades_module.CourseGradeReport, 'generate', staticmethod(fake_generate_no_errors)): + per_course_dir = tempfile.mkdtemp() + status = command._run_grades('course-1', {1}, per_course_dir, None) + + assert status['status'] == 'success' + assert 'failed' not in status + assert 'failed_rows_exported' not in status + assert not os.path.exists(os.path.join(per_course_dir, 'course-1__grades_errors.csv')) + + def test_grades_error_row_failure_does_not_flip_a_successful_course_to_error(self): + """ + Grades-error-isolation regression: the error-row-handling block + (writing __grades_errors.csv, filtering by 'Student ID') + runs AFTER the primary grades CSV/summary rows are already written to + disk. If anything in that block raises, it must not retroactively + flip an otherwise-successful course to 'error' and misreport grades + data that was actually written correctly. Triggered here with an + error_header missing 'Student ID' -- _filter_rows_by_column raises + CommandError, which must be caught by the error-row block's OWN + try/except, not the outer one. + """ + command = _make_command() + + def fake_generate_broken_error_header(_a, _b, course_id, _task_input, _action_name): + header = ['Student ID', 'Email', 'Username'] + GRADES_SUMMARY_COLUMNS[3:] + rows = [[1, 'a@x.com', 'alice'] + ['0.9', 'honor', 'N/A', 'N', 'N', '', 'enrolled']] + mod.grades_module.upload_csv_to_report_store([header] + rows, 'grade_report', course_id, None) + # Deliberately missing 'Student ID' -- _filter_rows_by_column raises. + error_header = ['Username', 'Error'] + error_rows = [['alice', 'grading exception']] + mod.grades_module.upload_csv_to_report_store( + [error_header] + error_rows, 'grade_report_err', course_id, None, + ) + return {'succeeded': 1, 'failed': 1} + + with patch.object( + mod.grades_module.CourseGradeReport, 'generate', staticmethod(fake_generate_broken_error_header) + ): + per_course_dir = tempfile.mkdtemp() + status = command._run_grades('course-1', {1}, per_course_dir, None) + + # The course itself is still a success -- the primary grades file was fine. + assert status['status'] == 'success' + assert 'grades_errors_export_error' in status + # 'failed' is read from raw_result/error_rows BEFORE the try block that goes + # on to fail -- it's safe to record (no IO) and must survive this failure, + # unlike 'failed_rows_exported' which depends on the filtering that raised. + assert status['failed'] == 1 + assert 'failed_rows_exported' not in status + assert os.path.exists(os.path.join(per_course_dir, 'course-1__grades.csv')) + # The errors file itself was never written -- the failure happened before that write. + assert not os.path.exists(os.path.join(per_course_dir, 'course-1__grades_errors.csv')) + + +class RunStudentFeaturesTests(TestCase): + """ + Tests for Command._run_student_features -- the profiles/enrollments + split and the tenant-wide learner_profile.csv dedup (see module docstring). + """ + + def _fake_upload_students_csv(self, _a, _b, course_id, feature_list, _action_name): + header = list(feature_list) + idx = {c: i for i, c in enumerate(header)} + + def row_for(user_id, username, enrollment_mode): + row = [''] * len(header) + if 'id' in idx: + # Mirrors the REAL enrolled_students_features behavior, not a + # convenient shortcut: extract_attr (instructor_analytics/basic.py) + # calls DjangoJSONEncoder().default(attr) directly, which + # unconditionally raises TypeError for a plain int (verified: + # json.JSONEncoder().default(42) raises), so its except branch + # stringifies -- 'id' comes back as '1', not 1. A fake that used + # a raw int here would hide the exact bug this test guards against. + row[idx['id']] = str(user_id) + if 'username' in idx: + row[idx['username']] = username + if 'enrollment_mode' in idx: + row[idx['enrollment_mode']] = enrollment_mode + return row + + rows = [row_for(1, 'alice', 'honor'), row_for(2, 'bob', 'verified'), row_for(99, 'outsider', 'honor')] + mod.enrollments_module.upload_csv_to_report_store([header] + rows, 'student_profile_info', course_id, None) + return {'succeeded': 3} + + def test_dedups_learner_profile_across_courses_and_filters_non_members(self): + command = _make_command() + feature_list = command._build_feature_list(['id', 'username'], ['profiles', 'enrollments']) + output_dir = tempfile.mkdtemp() + per_course_dir = tempfile.mkdtemp() + sinks = command._open_sinks(output_dir, ['profiles', 'enrollments'], feature_list) + seen = set() + + with patch.object(mod.enrollments_module, 'upload_students_csv', self._fake_upload_students_csv): + status1 = command._run_student_features( + 'course-1', {1, 2}, feature_list, per_course_dir, True, True, + sinks['learner_profile'], sinks['course_enrollments'], seen, + ) + status2 = command._run_student_features( + 'course-2', {1, 2}, feature_list, per_course_dir, True, True, + sinks['learner_profile'], sinks['course_enrollments'], seen, + ) + for sink in sinks.values(): + sink.close() + + # 99 ('outsider') is filtered out of both courses. + assert status1 == {'status': 'success', 'rows': 2} + assert status2 == {'status': 'success', 'rows': 2} + + with open(os.path.join(output_dir, 'learner_profile.csv')) as f: + profile_rows = list(csv.DictReader(f)) + # Deduped on learner id across course-1 and course-2: 2 learners, not 4. + assert len(profile_rows) == 2 + assert 'enrollment_mode' not in profile_rows[0] + + with open(os.path.join(output_dir, 'course_enrollments.csv')) as f: + enrollment_rows = list(csv.DictReader(f)) + # One row per (course, learner): 2 courses x 2 members = 4. + assert len(enrollment_rows) == 4 + assert {'course_id', 'id', 'enrollment_mode'}.issubset(enrollment_rows[0].keys()) + + def test_all_rows_filtered_out_is_reported_as_a_warning_not_silent_success(self): + """ + Regression test: an operator must see this in the manifest, not + discover it later from an empty off-boarding bundle. This is the + exact shape the string/int 'id' mismatch bug produced before + _filter_rows_by_column was fixed to compare as strings. + """ + command = _make_command() + + def fake_all_outsiders(_a, _b, course_id, feature_list, _action_name): + header = list(feature_list) + idx = {c: i for i, c in enumerate(header)} + row = [''] * len(header) + row[idx['id']] = '999' + mod.enrollments_module.upload_csv_to_report_store([header] + [row], 'student_profile_info', course_id, None) + return {'succeeded': 1} + + feature_list = command._build_feature_list(['id'], ['profiles']) + sinks = command._open_sinks(tempfile.mkdtemp(), ['profiles'], feature_list) + with patch.object(mod.enrollments_module, 'upload_students_csv', fake_all_outsiders): + status = command._run_student_features( + 'course-1', {1, 2}, feature_list, tempfile.mkdtemp(), True, False, + sinks['learner_profile'], None, set(), + ) + for sink in sinks.values(): + sink.close() + + assert status['status'] == 'success' + assert status['rows'] == 0 + assert 'warning' in status + + +class RunProblemResponsesTests(TestCase): + """ + Tests for Command._run_problem_responses -- username-based tenant + filter and the task_input shape (problem_locations as a string, not a + list; user_id from the --as-user operator). + """ + + def test_filters_by_username_and_passes_course_root_usage_key(self): + command = _make_command() + captured_task_input = {} + + def fake_generate(_a, _b, course_id, task_input, _action_name): + captured_task_input.update(task_input) + header = ['username', 'title', 'location', 'block_key', 'state'] + rows = [ + ['alice', 'P1', 'loc', 'blk-1', 'state1'], + ['outsider', 'P1', 'loc', 'blk-1', 'state2'], + ] + mod.grades_module.upload_csv_to_report_store([header] + rows, 'student_state', course_id, None) + return {'succeeded': 2} + + operator = MagicMock(id=42) + with patch.object(mod.grades_module.ProblemResponses, 'generate', staticmethod(fake_generate)), \ + patch.object(mod, 'modulestore') as mock_modulestore: + mock_modulestore.return_value.make_course_usage_key.return_value = 'block-v1:course+root' + per_course_dir = tempfile.mkdtemp() + status = command._run_problem_responses('course-1', {'alice', 'bob'}, operator, per_course_dir, None) + + assert status == {'status': 'success', 'rows': 1} + assert captured_task_input['user_id'] == 42 + assert isinstance(captured_task_input['problem_locations'], str) + + +class RunOra2Tests(TestCase): + """ + Tests for Command._run_ora2 -- the 'failed' sentinel must be treated as + an error, and an unverifiable identity column must skip rather than + write an unfiltered cross-tenant export. + """ + + def test_failed_sentinel_is_treated_as_an_error(self): + command = _make_command() + with patch.object(mod.misc_module, 'upload_ora2_data', lambda *a, **k: 'failed'): + status = command._run_ora2('course-1', {1}, None, tempfile.mkdtemp(), None) + assert status['status'] == 'error' + + def test_unverifiable_identity_column_is_skipped_not_written_unfiltered(self): + command = _make_command() + course = CourseOverviewFactory() + + def fake_generate(_a, _b, course_id, _task_input, _action_name): + header = ['Submission ID', 'Response Text'] + rows = [['sub-1', 'free text']] + mod.misc_module.upload_csv_to_report_store([header] + rows, 'ORA_data', course_id, None) + return {'succeeded': 1} + + with patch.object(mod.misc_module, 'upload_ora2_data', fake_generate): + status = command._run_ora2(course.id, {1}, None, tempfile.mkdtemp(), None) + + assert status == {'status': 'skipped', 'reason': 'identity column unverified'} + + def test_detected_identity_column_filters_to_tenant(self): + command = _make_command() + course = CourseOverviewFactory() + member = UserFactory() + other_tenant_user = UserFactory() + # Detection needs the FULL course-scoped superset (both rows) to + # recognize the column at all; the tenant SET used for the actual + # filter only contains `member`'s anon id -- this is exactly the + # "course has more than one tenant's students" scenario the + # content-based detection exists to handle safely (see module + # docstring's ora2 gap). + AnonymousUserId.objects.create(user=member, course_id=course.id, anonymous_user_id='a' * 32) + AnonymousUserId.objects.create(user=other_tenant_user, course_id=course.id, anonymous_user_id='b' * 32) + + def fake_generate(_a, _b, course_id, _task_input, _action_name): + header = ['Submission ID', 'Anon Col', 'Response Text'] + rows = [ + ['sub-1', 'a' * 32, 'in tenant'], + ['sub-2', 'b' * 32, 'not in tenant'], + ] + mod.misc_module.upload_csv_to_report_store([header] + rows, 'ORA_data', course_id, None) + return {'succeeded': 2} + + with patch.object(mod.misc_module, 'upload_ora2_data', fake_generate): + status = command._run_ora2(course.id, {member.id}, None, tempfile.mkdtemp(), None) + + assert status['status'] == 'success' + assert status['rows'] == 1 + assert status['identity_column'] == 'Anon Col' + + def test_override_column_filters_against_the_union_of_identity_spaces(self): + """ + Regression test: --ora2-identity-column must not assume the + operator-confirmed column is a raw user-id column. It could just as + well be an anonymized-id or username column (that's the whole point + of the flag: detection couldn't verify it by content). Matching + against user_ids alone silently zeroed out every override run whose + column actually held anon ids or usernames. + """ + command = _make_command() + course = CourseOverviewFactory() + member = UserFactory() + AnonymousUserId.objects.create(user=member, course_id=course.id, anonymous_user_id='a' * 32) + + def fake_generate(_a, _b, course_id, _task_input, _action_name): + header = ['Submission ID', 'Confirmed Anon Col', 'Response Text'] + rows = [ + ['sub-1', 'a' * 32, 'in tenant'], + ['sub-2', 'b' * 32, 'not in tenant'], + ] + mod.misc_module.upload_csv_to_report_store([header] + rows, 'ORA_data', course_id, None) + return {'succeeded': 2} + + with patch.object(mod.misc_module, 'upload_ora2_data', fake_generate): + status = command._run_ora2(course.id, {member.id}, 'Confirmed Anon Col', tempfile.mkdtemp(), None) + + assert status['status'] == 'success' + assert status['rows'] == 1 + + +class RunMayEnrollTests(TestCase): + """ + Tests for Command._run_may_enroll -- deliberately NOT tenant-filtered + (see module docstring); the caveat must be present in the returned status. + """ + + def test_returns_unfiltered_rows_with_a_caveat_flag(self): + command = _make_command() + + def fake_may_enroll(_a, _b, course_id, task_input, _action_name): + header = list(task_input['features']) + rows = [['someone@example.com', True, '2026-01-01']] + mod.enrollments_module.upload_csv_to_report_store([header] + rows, 'may_enroll_info', course_id, None) + return {'succeeded': 1} + + with patch.object(mod.enrollments_module, 'upload_may_enroll_csv', fake_may_enroll): + status = command._run_may_enroll('course-1', tempfile.mkdtemp(), None) + + assert status['status'] == 'success' + assert status['caveat'] == 'not tenant-membership-filtered' + + +class ProcessCourseTests(TestCase): + """ + Tests for Command._process_course -- dispatches to every requested + report type and merges their statuses under the right --reports keys, + with 'profiles' and 'enrollments' sharing a single student_features call. + """ + + def test_dispatches_every_requested_report_and_shares_student_features_call(self): + command = _make_command() + call_count = {'student_features': 0} + + def fake_upload_students_csv(_a, _b, course_id, feature_list, _action_name): + call_count['student_features'] += 1 + header = list(feature_list) + mod.enrollments_module.upload_csv_to_report_store([header] + [], 'student_profile_info', course_id, None) + return {'succeeded': 0} + + with patch.object(mod.enrollments_module, 'upload_students_csv', fake_upload_students_csv): + feature_list = command._build_feature_list(['id'], ['profiles', 'enrollments']) + sinks = command._open_sinks(tempfile.mkdtemp(), ['profiles', 'enrollments'], feature_list) + status = command._process_course( + 'course-1', ['profiles', 'enrollments'], {1}, set(), feature_list, None, None, + tempfile.mkdtemp(), sinks, set(), + ) + for sink in sinks.values(): + sink.close() + + assert set(status.keys()) == {'profiles', 'enrollments'} + # One upload_students_csv call serves both report keys. + assert call_count['student_features'] == 1 + + +class WriteManifestTests(TestCase): + """ + Tests for Command._write_manifest. + """ + + def test_writes_valid_json_with_0600_permissions(self): + command = _make_command() + output_dir = tempfile.mkdtemp() + manifest = {'slug': 'acme', 'courses': {}, 'summary_files': {}} + + command._write_manifest(output_dir, manifest) + + path = os.path.join(output_dir, 'MANIFEST.json') + with open(path) as f: + loaded = json.load(f) + assert loaded == manifest + assert stat.S_IMODE(os.stat(path).st_mode) == 0o600 + + +class SafeCourseIdTests(TestCase): + """ + Tests for the module-level _safe_course_id helper. + """ + + def test_sanitizes_colons_slashes_and_plus_signs(self): + assert mod._safe_course_id('course-v1:org+course+run') == 'course-v1_org_course_run' + + +class MakePrivateDirTests(TestCase): + """ + Tests for Command._make_private_dir (item 2) -- --output-dir can point + at a path this command didn't create (e.g. MEDIA_ROOT itself); only a + directory this call actually creates should get chmod'd to 0700. + """ + + def test_freshly_created_directory_gets_0700(self): + command = _make_command() + base = tempfile.mkdtemp() + path = os.path.join(base, 'new_export_dir') + command._make_private_dir(path) + assert os.path.exists(path) + assert stat.S_IMODE(os.stat(path).st_mode) == 0o700 + + def test_preexisting_directory_permissions_are_left_untouched(self): + path = tempfile.mkdtemp() + os.chmod(path, 0o755) + command = _make_command() + command._make_private_dir(path) + assert stat.S_IMODE(os.stat(path).st_mode) == 0o755 + + +class MaxProblemResponsesOverrideTests(TestCase): + """ + Tests for Command._resolve_max_problem_responses and + _override_max_problem_responses_limit (item 5) -- lifting the + MAX_PROBLEM_RESPONSES_COUNT cap is opt-in via --max-problem-responses, + not automatic just because problem_responses was requested. + """ + + def test_resolve_returns_none_when_flag_not_passed(self): + command = _make_command() + assert command._resolve_max_problem_responses(None) is None + + def test_resolve_parses_unlimited_literal(self): + command = _make_command() + assert command._resolve_max_problem_responses('unlimited') == 'unlimited' + + def test_resolve_parses_integer_value(self): + command = _make_command() + assert command._resolve_max_problem_responses('10000') == 10000 + + def test_resolve_rejects_non_integer_non_unlimited_value(self): + command = _make_command() + with self.assertRaises(CommandError): + command._resolve_max_problem_responses('not-a-number') + + def test_cap_left_untouched_when_override_not_requested(self): + # Mirrors handle()'s own branch: max_problem_responses is None -> + # _null_context, FEATURES is never touched. + original = settings.FEATURES.get('MAX_PROBLEM_RESPONSES_COUNT') + with mod._null_context(): + assert settings.FEATURES.get('MAX_PROBLEM_RESPONSES_COUNT') == original + + def test_cap_overridden_to_none_and_restored_after(self): + settings.FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] = 5000 + with mod._override_max_problem_responses_limit(None): + assert settings.FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] is None + assert settings.FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] == 5000 + + def test_cap_overridden_to_explicit_integer_and_restored_after(self): + settings.FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] = 5000 + with mod._override_max_problem_responses_limit(20000): + assert settings.FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] == 20000 + assert settings.FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] == 5000 + + def test_cap_restored_even_if_the_run_raises(self): + settings.FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] = 5000 + with self.assertRaises(ValueError): + with mod._override_max_problem_responses_limit(None): + raise ValueError("simulated failure mid-run") + assert settings.FEATURES['MAX_PROBLEM_RESPONSES_COUNT'] == 5000 + + +class ReadReplicaRoutingTests(TestCase): + """ + Tests for item 11 -- this command's own direct queries (tenant/org/ + course/user resolution, ORA2 identity-column detection) route through + read_replica_or_default() (common.djangoapps.util.query, the same + pattern cache_programs.py uses). These assert the command actually + CALLS read_replica_or_default() at each query site -- deterministic, + and deliberately does NOT assert a specific DB-connection exception + class for a bad alias (that varies across Django versions, and this + environment has no working Django install to verify it against -- + see this file's own module docstring / item 13). + """ + + def _recording_wrapper(self, calls): + real = mod.read_replica_or_default + + def recording(*args, **kwargs): + calls.append(True) + return real(*args, **kwargs) + return recording + + def test_get_sub_org_uses_read_replica(self): + sub_org = EdlySubOrganizationFactory() + command = _make_command() + calls = [] + with patch.object(mod, 'read_replica_or_default', self._recording_wrapper(calls)): + command._get_sub_org(sub_org.slug) + assert calls + + def test_get_user_ids_uses_read_replica(self): + sub_org = EdlySubOrganizationFactory() + command = _make_command() + calls = [] + with patch.object(mod, 'read_replica_or_default', self._recording_wrapper(calls)): + command._get_user_ids(sub_org) + assert calls + + def test_get_tenant_course_ids_uses_read_replica(self): + command = _make_command() + calls = [] + with patch.object(mod, 'read_replica_or_default', self._recording_wrapper(calls)): + command._get_tenant_course_ids([], set()) + assert calls + + def test_resolve_operator_uses_read_replica(self): + user = UserFactory() + command = _make_command() + calls = [] + with patch.object(mod, 'read_replica_or_default', self._recording_wrapper(calls)): + command._resolve_operator(user.username) + assert calls + + def test_detect_ora2_identity_column_uses_read_replica(self): + course = CourseOverviewFactory() + command = _make_command() + calls = [] + with patch.object(mod, 'read_replica_or_default', self._recording_wrapper(calls)): + command._detect_ora2_identity_column(['a', 'b'], [['1', '2']], course.id, None) + assert calls + + def test_bogus_replica_alias_breaks_the_query(self): + """ + Secondary check, not the primary proof (see class docstring): + asserts broad Exception, not a named Django exception class. + """ + sub_org = EdlySubOrganizationFactory() + command = _make_command() + with patch.object(mod, 'read_replica_or_default', lambda: 'definitely-not-a-real-alias'): + with self.assertRaises(Exception): + command._get_sub_org(sub_org.slug) + + +class HandleTests(TestCase): + """ + Tests for Command.handle() itself (item 12) -- built from the real + argument parser, not hand-written option dicts. Covers the parser's + own defaults, --dry-run, the no-members early return, the --as-user/ + is_staff gating for problem_responses (items 7 + 12), the may_enroll + opt-in warning, and (with Command._process_course patched to raise + partway through a multi-course run) the interrupted-run try/finally + resilience added for item 3. + """ + + def _parse(self, *cli_args): + command = Command() + parser = command.create_parser('manage.py', 'export_tenant_reports_csv') + return vars(parser.parse_args(list(cli_args))) + + def test_parser_defaults(self): + # --output-dir is required (item 2's PII-exposure fix -- no safe default to fall back to), + # so it must be passed here for parse_args() to succeed at all; every other option below is + # still exercised for its own default. + output_dir = tempfile.mkdtemp() + options = self._parse('some-slug', '--output-dir', output_dir) + assert options['reports'] == ','.join(DEFAULT_REPORTS) + assert options['as_user'] is None + assert options['dry_run'] is False + assert options['output_dir'] == output_dir + assert options['include_fields'] is None + assert options['allow_meta_field'] is False + assert options['skip_course'] == [] + assert options['ora2_identity_column'] is None + assert options['max_problem_responses'] is None + + def test_missing_output_dir_raises(self): + """ + Item 2's PII-exposure fix: --output-dir is now required, with no MEDIA_ROOT/EDM_EXPORT_DIR + fallback left to silently catch the omission. Omitting it must fail loudly and immediately, + before handle() ever runs. Django's CommandParser routes argparse's own missing-required- + argument error through parser.error(), which raises CommandError (rather than exiting the + process via SystemExit) whenever the command isn't invoked from the real command line -- as + is the case here, through call_command(). + """ + sub_org = EdlySubOrganizationFactory() + with self.assertRaises(CommandError): + call_command('export_tenant_reports_csv', sub_org.slug) + + def test_dry_run_writes_nothing(self): + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=True) + + out = StringIO() + output_dir = os.path.join(tempfile.mkdtemp(), 'would-be-output') + # Default --reports (includes problem_responses) and no --as-user -- the command's own + # documented default invocation. handle() now checks dry_run BEFORE the --as-user/is_staff + # gate for problem_responses, so a dry run never hits that gate at all: it doesn't call any + # report generator or need an operator identity. Previously this exact invocation raised + # CommandError and the test had to work around it with an explicit --reports grades. + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--dry-run', + '--output-dir', output_dir, stdout=out, + ) + assert 'DRY RUN' in out.getvalue() + assert not os.path.exists(output_dir) + + def test_no_members_early_return(self): + sub_org = EdlySubOrganizationFactory() + out = StringIO() + call_command('export_tenant_reports_csv', sub_org.slug, '--output-dir', tempfile.mkdtemp(), stdout=out) + assert 'nothing to export' in out.getvalue() + + def test_problem_responses_without_as_user_raises(self): + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + with self.assertRaises(CommandError): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'problem_responses', + '--output-dir', tempfile.mkdtemp(), + ) + + def test_problem_responses_as_user_non_staff_raises(self): + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + operator = UserFactory(is_staff=False) + with self.assertRaises(CommandError): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'problem_responses', + '--as-user', operator.username, '--output-dir', tempfile.mkdtemp(), + ) + + def test_problem_responses_as_user_staff_passes_through(self): + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + operator = UserFactory(is_staff=True) + out = StringIO() + with patch.object( + Command, '_process_course', + return_value={'problem_responses': {'status': 'success', 'rows': 0}}, + ): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'problem_responses', + '--as-user', operator.username, '--output-dir', tempfile.mkdtemp(), stdout=out, + ) + assert 'Exported tenant CSV reports' in out.getvalue() + + def test_may_enroll_opt_in_warning(self): + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + out = StringIO() + with patch.object(Command, '_process_course', return_value={}): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'may_enroll', + '--output-dir', tempfile.mkdtemp(), stdout=out, + ) + assert 'cannot be tenant-membership-filtered' in out.getvalue() + + def test_interrupted_run_leaves_incomplete_manifest_and_partial_sink_files(self): + """ + Item 3 regression: simulate an unexpected exception partway through a + multi-course run (Command._process_course patched to succeed once + then raise) and assert the try/finally in handle() still closes + every sink -- including the _DictCsvSink-backed problem_responses/ + ora2 files, which previously were never written at all on an + interrupted run -- and still writes a manifest, marked incomplete. + + Note: this patches Command._process_course, which sits ABOVE each + _run_*'s own per-report `except Exception` -- it proves handle()'s + own try/finally survives an interruption, but NOT that a + KeyboardInterrupt/SystemExit raised INSIDE a report call itself isn't + swallowed by that inner `except Exception` (KeyboardInterrupt is a + BaseException, not an Exception, and RuntimeError is not evidence + either way). See + test_keyboard_interrupt_inside_a_report_call_is_not_swallowed_by_inner_except_exception + below for that property specifically. + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course_1 = CourseOverviewFactory() + course_2 = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course_1.id, is_active=True) + CourseEnrollmentFactory(user=member, course_id=course_2.id, is_active=True) + operator = UserFactory(is_staff=True) + + output_dir = tempfile.mkdtemp() + call_count = {'n': 0} + + def fake_process_course(self, course_id, *args, **kwargs): + call_count['n'] += 1 + if call_count['n'] == 1: + return {'grades': {'status': 'success', 'rows': 0}} + raise RuntimeError('simulated unexpected failure partway through the run') + + with patch.object(Command, '_process_course', fake_process_course): + with self.assertRaises(RuntimeError): + call_command( + 'export_tenant_reports_csv', sub_org.slug, + '--reports', 'grades,problem_responses,ora2', + '--as-user', operator.username, + '--output-dir', output_dir, + ) + + with open(os.path.join(output_dir, 'MANIFEST.json')) as f: + manifest = json.load(f) + assert manifest['status'] == 'incomplete' + assert len(manifest['courses_completed']) == 1 + + assert os.path.exists(os.path.join(output_dir, 'grades_summary.csv')) + assert os.path.exists(os.path.join(output_dir, 'problem_responses.csv')) + # 'ora2' IS in --reports above, so its _DictCsvSink must survive the + # interruption too, same as problem_responses.csv just above. + assert os.path.exists(os.path.join(output_dir, 'ora2_responses.csv')) + + def test_keyboard_interrupt_inside_a_report_call_is_not_swallowed_by_inner_except_exception(self): + """ + Finally-block-hardening regression: the test above patches + Command._process_course, which sits ABOVE _run_grades's own + per-report `except Exception` -- it proves handle()'s own try/finally + survives an interruption, but not that a KeyboardInterrupt raised + INSIDE a report call itself isn't swallowed by that inner `except + Exception`. KeyboardInterrupt/SystemExit are BaseExceptions, not + Exceptions, so `except Exception` in _run_grades (and every other + _run_* method) must not catch them. This patches the real + CourseGradeReport.generate call itself to raise KeyboardInterrupt, so + it must propagate all the way out of call_command(), while still + leaving a written, 'incomplete' manifest. (An external OOM-kill is + SIGKILL -- no Python code runs at all, so nothing in this command can + survive that; only an in-process KeyboardInterrupt or MemoryError are + actually within reach of this try/finally -- see module docstring.) + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=True) + + output_dir = tempfile.mkdtemp() + + def raising_generate(*args, **kwargs): + raise KeyboardInterrupt('simulated Ctrl-C during CourseGradeReport.generate') + + with patch.object(mod.grades_module.CourseGradeReport, 'generate', staticmethod(raising_generate)): + with self.assertRaises(KeyboardInterrupt): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'grades', + '--output-dir', output_dir, + ) + + with open(os.path.join(output_dir, 'MANIFEST.json')) as f: + manifest = json.load(f) + assert manifest['status'] == 'incomplete' + + def test_finally_block_survives_a_sink_close_failure_without_losing_the_manifest(self): + """ + Finally-block-hardening regression: the finally block that's supposed + to guarantee every sink gets flushed and the manifest gets written on + interruption previously did that work with a single unguarded loop -- + _DictCsvSink.close() opens a file and encodes/writes every buffered + row, so if THAT raised (disk full, encode error, MemoryError...), the + loop aborted before the remaining sinks closed or the manifest was + written at all -- exactly the failure this finally block exists to + prevent. This patches Command._open_sinks to return one broken sink + alongside a real one, and Command._process_course to raise partway + through (simulating the ORIGINAL interruption this finally block + exists for), asserting: the original exception still propagates (not + replaced/masked by the broken sink's own close() failure), the good + sink's file still lands on disk, and MANIFEST.json still gets + written -- recording the broken sink's failure instead of losing + everything. + + The broken sink is placed FIRST in the dict Command._open_sinks + returns (an OrderedDict -- insertion order is only guaranteed dict + behavior from Python 3.7 on, and this file still targets py35 per + tox.ini, so plain dict literal order isn't a safe thing to depend on + here). Order is load-bearing: this is the only arrangement that + actually proves a failing sink doesn't stop the sinks AFTER it from + closing -- good-first would let the good sink close before the + broken one ever raises, proving nothing. The good sink is a + _DictCsvSink (not _CsvSink): _CsvSink.__init__ creates its file via + os.open(..., O_CREAT) immediately, so os.path.exists() on one would + already be true before call_command() even runs, making that + assertion pass regardless of whether close() ran. _DictCsvSink only + creates its file inside close(), so os.path.exists() here is a real, + load-bearing proof that close() executed on this sink specifically. + + --reports is still just 'grades' -- Command._open_sinks is patched + out entirely, so the sink names/types below don't need to match the + requested reports. ('problem_responses' as a --reports value would + additionally require --as-user/is_staff, an unrelated gate this test + isn't exercising.) + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=True) + + output_dir = tempfile.mkdtemp() + good_sink_path = os.path.join(output_dir, 'problem_responses.csv') + good_sink = _DictCsvSink(good_sink_path) + good_sink.add_rows('course-1', ['username'], [['alice']]) + + class _BrokenSink(object): + # Nonzero and known before close() runs -- 0 would be indistinguishable + # from a placeholder default and wouldn't prove this count is the sink's + # real, pre-close value. + row_count = 3 + + def close(self): + raise MemoryError('simulated: this sink failed to flush/close') + + def raising_process_course(self, course_id, *args, **kwargs): + raise RuntimeError('simulated interruption (e.g. Ctrl-C) during _process_course') + + def fake_open_sinks(self, output_dir, reports, feature_list, sinks=None): + # Broken sink FIRST: proves a failing close() doesn't cost the + # sinks after it their close(), which is the actual regression. + # sinks= accepted (and ignored) only so this fake's signature still + # matches handle()'s call, which now passes its own pre-initialized + # dict through as a 4th positional arg (see _open_sinks docstring). + return OrderedDict([('broken', _BrokenSink()), ('problem_responses', good_sink)]) + + with patch.object(Command, '_process_course', raising_process_course), \ + patch.object(Command, '_open_sinks', fake_open_sinks): + with self.assertRaises(RuntimeError): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'grades', + '--output-dir', output_dir, + ) + + with open(os.path.join(output_dir, 'MANIFEST.json')) as f: + manifest = json.load(f) + assert manifest['status'] == 'incomplete' + # row_count is read before close() (inside the same guarded try as close() + # itself), so the broken sink's count (3) is still recorded even though its + # close() raised -- the error itself lands on the separate summary_file_errors + # key, not summary_files. + assert manifest['summary_files']['broken.csv'] == 3 + assert manifest['summary_file_errors']['broken.csv'] + assert manifest['summary_files']['problem_responses.csv'] == 1 + # Only exists if close() actually ran on the sink AFTER the broken one. + assert os.path.exists(good_sink_path) + + def test_write_manifest_failure_does_not_mask_the_original_interruption_exception(self): + """ + Finally-block-hardening regression: _write_manifest itself is now + guarded in its own try/except (previously unguarded, even after the + per-sink close() isolation above was added) -- if writing + MANIFEST.json fails (disk full, IO error), that failure must be + logged and swallowed, not allowed to replace/mask whatever original + exception (KeyboardInterrupt, MemoryError, or here a simulated + RuntimeError from _process_course) triggered the finally block in + the first place -- that original exception is the operator's only + signal about why the run actually stopped. + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=True) + + output_dir = tempfile.mkdtemp() + + def raising_process_course(self, course_id, *args, **kwargs): + raise RuntimeError('simulated interruption (e.g. Ctrl-C) during _process_course') + + def raising_write_manifest(self, output_dir, manifest): + raise IOError('simulated: disk full while writing MANIFEST.json') + + with patch.object(Command, '_process_course', raising_process_course), \ + patch.object(Command, '_write_manifest', raising_write_manifest): + with self.assertRaises(RuntimeError): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'grades', + '--output-dir', output_dir, + ) + + # The manifest-write failure is real (not a no-op) -- MANIFEST.json never landed. + assert not os.path.exists(os.path.join(output_dir, 'MANIFEST.json')) + + def test_write_manifest_failure_on_a_successful_run_is_not_reported_as_success(self): + """ + The finally block's _write_manifest guard must not turn a lost audit + trail into a clean exit. With no original exception in flight there + is nothing to mask, so a manifest-write failure on an otherwise + successful run must surface, not be swallowed behind the success banner. + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=True) + output_dir = tempfile.mkdtemp() + out = StringIO() + err = StringIO() + + def raising_write_manifest(self, output_dir, manifest): + raise IOError('simulated: disk full while writing MANIFEST.json') + + with patch.object(Command, '_process_course', return_value={'grades': {'status': 'success', 'rows': 0}}), \ + patch.object(Command, '_write_manifest', raising_write_manifest): + with self.assertRaises(CommandError): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'grades', + '--output-dir', output_dir, stdout=out, stderr=err, + ) + assert not os.path.exists(os.path.join(output_dir, 'MANIFEST.json')) + # The raise happens before the success banner is written -- this is the actual + # "not reported as success" the test name promises, not just the CommandError itself. + assert 'Exported tenant CSV reports' not in out.getvalue() + # Previously only the raised exception TYPE was checked here, not the + # operator-facing message -- half of what this fix is actually for: an + # operator watching stderr, not just a caller checking the exit code. + assert 'MANIFEST.json could not be written' in err.getvalue() + + def test_successful_run_with_a_failing_sink_close_raises_and_still_writes_manifest(self): + """ + Generalization regression: the "an operator must not get a clean exit + 0" principle the test above established for a lost MANIFEST.json + applies equally to a lost summary CSV. Previously a sink's close() + failing on an otherwise-successful run was recorded ONLY in + manifest['summary_file_errors'] -- a JSON key nobody reads on a run + that prints the success banner and exits 0 -- so an entire data CSV + (e.g. problem_responses.csv) could silently never land on disk. + + The manifest write must still happen BEFORE this raise (see the + module docstring and the comment above the finally block's + `if run_succeeded:` guard), so summary_file_errors itself survives on + disk even though the run as a whole now fails loudly. + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=True) + + output_dir = tempfile.mkdtemp() + out = StringIO() + err = StringIO() + + class _BrokenSink(object): + row_count = 4 # nonzero and known-before-close, same discipline as summary_files elsewhere. + + def close(self): + raise IOError('simulated: disk full while flushing problem_responses.csv') + + def fake_open_sinks(self, output_dir, reports, feature_list, sinks=None): + return OrderedDict([('problem_responses', _BrokenSink())]) + + with patch.object(Command, '_process_course', return_value={'grades': {'status': 'success', 'rows': 0}}), \ + patch.object(Command, '_open_sinks', fake_open_sinks): + with self.assertRaises(CommandError): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'grades', + '--output-dir', output_dir, stdout=out, stderr=err, + ) + + # MANIFEST.json IS still on disk -- written before the raise, unlike the + # write-manifest-failure case above where the write itself is what failed. + manifest_path = os.path.join(output_dir, 'MANIFEST.json') + assert os.path.exists(manifest_path) + with open(manifest_path) as f: + manifest = json.load(f) + assert manifest['summary_file_errors']['problem_responses.csv'] + # Round-6 addition: the count-before-close() behavior (see the finally block's + # own comment) is actually exercised here on the success path it exists for -- + # previously this test only checked summary_file_errors, never summary_files. + assert manifest['summary_files']['problem_responses.csv'] == 4 + # The run itself completed with no per-course report errors -- only the sink's + # own close() failed, a separate concern from the course loop's own success -- + # so status stays plain 'complete', not 'complete_with_errors'. + assert manifest['status'] == 'complete' + + assert 'Exported tenant CSV reports' not in out.getvalue() + assert 'problem_responses.csv' in err.getvalue() + + def test_courses_completed_excludes_skip_course_entries(self): + """ + Finally-block-hardening regression: manifest['courses_completed'] + previously listed every key in manifest['courses'], including courses + skipped via --skip-course (recorded as {'_skipped': '--skip-course'}) + -- those were never actually processed, so they must not count as + "completed". + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course_1 = CourseOverviewFactory() + course_2 = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course_1.id, is_active=True) + CourseEnrollmentFactory(user=member, course_id=course_2.id, is_active=True) + + output_dir = tempfile.mkdtemp() + with patch.object(Command, '_process_course', return_value={'grades': {'status': 'success', 'rows': 0}}): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'grades', + '--skip-course', str(course_2.id), + '--output-dir', output_dir, + ) + + with open(os.path.join(output_dir, 'MANIFEST.json')) as f: + manifest = json.load(f) + assert manifest['courses'][str(course_2.id)] == {'_skipped': '--skip-course'} + assert manifest['courses_completed'] == [str(course_1.id)] + # Round-6 addition: a --skip-course entry (a flat {'_skipped': ...} dict, not a + # per-report status dict) must not be misread as a course-level error by + # _course_has_report_error, and the one real course here was a plain success -- + # so this run's status must stay 'complete', not be downgraded. + assert manifest['courses_with_errors'] == [] + assert manifest['status'] == 'complete' + + def test_known_gaps_includes_enrollment_mismatch_even_without_ora2(self): + """ + known_gaps-ungating regression: the active/inactive enrollment- + membership mismatch between learner_profile.csv and + grades_summary.csv (see module docstring) affects any --reports + grades,profiles run, not just when ora2 is requested -- previously + manifest['known_gaps'] was only ever populated when 'ora2' was in the + requested reports, so an operator running a plain grades+profiles + export never saw this caveat surfaced in MANIFEST.json at all, only + in the module docstring. + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=True) + + output_dir = tempfile.mkdtemp() + with patch.object(Command, '_process_course', return_value={}): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'grades,profiles', + '--output-dir', output_dir, + ) + + with open(os.path.join(output_dir, 'MANIFEST.json')) as f: + manifest = json.load(f) + assert 'enrollment_membership_mismatch' in manifest['known_gaps'] + assert 'ora2_identity_column_detection' not in manifest['known_gaps'] + # 'ora2' was never in --reports, so _open_sinks never opened an ora2 sink -- + # this file must not exist (previously a stray assertion here claimed it did, + # copy-pasted from the interruption test above, which DOES request ora2). + assert not os.path.exists(os.path.join(output_dir, 'ora2_responses.csv')) + + def test_all_course_report_errors_downgrade_status_and_banner_but_do_not_raise(self): + """ + Round-6 fix: previously, a run in which EVERY course's EVERY report + returned {'status': 'error', ...} (no exception raised, no + interruption -- per-course isolation working exactly as designed) + still exited 0, still printed the plain unconditional success + banner, and still recorded manifest['status'] == 'complete', giving + an operator no top-level signal that every single course actually + failed. Per-course isolation itself is correct and must NOT change + here -- a broken course must not abort the whole run, so this must + NOT raise -- but the run's own summary must reflect what happened: + manifest['status'] downgrades to 'complete_with_errors', + manifest['courses_with_errors'] names the affected courses, and the + final stdout banner becomes a WARNING naming the error count instead + of the plain success line. + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course_1 = CourseOverviewFactory() + course_2 = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course_1.id, is_active=True) + CourseEnrollmentFactory(user=member, course_id=course_2.id, is_active=True) + + output_dir = tempfile.mkdtemp() + out = StringIO() + err = StringIO() + + def all_errors_process_course(self, course_id, *args, **kwargs): + return {'grades': {'status': 'error', 'error': 'simulated generator explosion'}} + + with patch.object(Command, '_process_course', all_errors_process_course): + # Must NOT raise -- per-course isolation means a run with only per-course + # report errors (no exception, no interruption) still completes. + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'grades', + '--output-dir', output_dir, stdout=out, stderr=err, + ) + + with open(os.path.join(output_dir, 'MANIFEST.json')) as f: + manifest = json.load(f) + assert manifest['status'] == 'complete_with_errors' + assert sorted(manifest['courses_with_errors']) == sorted([str(course_1.id), str(course_2.id)]) + # Neither course had a warning/skipped report -- only outright errors -- so the + # separate, weaker courses_needing_review rollup must stay empty here. + assert manifest['courses_needing_review'] == [] + # The plain, unconditional success banner text must not appear anywhere in stdout. + assert 'Exported tenant CSV reports' not in out.getvalue() + # Whatever warning IS printed must name the course-error count. + assert '2 course(s)' in out.getvalue() + assert 'MANIFEST.json' in out.getvalue() + + def test_both_sink_close_and_manifest_write_failing_still_names_the_sink_on_stderr(self): + """ + Round-6 fix: previously, when BOTH a sink's close() AND the + MANIFEST.json write failed on an otherwise-successful run, the + sink-failure-naming stderr message sat BELOW the manifest-failure + priority raise and was therefore unreachable on this path -- so + MANIFEST.json never landed (the write itself failed) AND stderr + never named which CSV was lost either, leaving the operator with NO + record of it anywhere. The sink-failure stderr message is now + written unconditionally (whenever summary_file_errors is non-empty) + BEFORE the raise-priority decision, so stderr becomes the operator's + only remaining channel for that information -- even though the + manifest-write failure still takes priority for which exception + actually propagates (only one can). + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=True) + + output_dir = tempfile.mkdtemp() + out = StringIO() + err = StringIO() + + class _BrokenSink(object): + row_count = 4 + + def close(self): + raise IOError('simulated: disk full while flushing problem_responses.csv') + + def fake_open_sinks(self, output_dir, reports, feature_list, sinks=None): + return OrderedDict([('problem_responses', _BrokenSink())]) + + def raising_write_manifest(self, output_dir, manifest): + raise IOError('simulated: disk full while writing MANIFEST.json') + + with patch.object(Command, '_process_course', return_value={'grades': {'status': 'success', 'rows': 0}}), \ + patch.object(Command, '_open_sinks', fake_open_sinks), \ + patch.object(Command, '_write_manifest', raising_write_manifest): + with self.assertRaises(CommandError): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'grades', + '--output-dir', output_dir, stdout=out, stderr=err, + ) + + # The manifest write itself failed -- it never landed on disk, so the sink + # failure was never recorded there either. + assert not os.path.exists(os.path.join(output_dir, 'MANIFEST.json')) + # Manifest-write failure takes priority for which exception propagates -- + # but the lost sink's CSV name is still named on stderr regardless, since + # stderr is now the operator's only remaining channel for it. + assert 'problem_responses.csv' in err.getvalue() + assert 'MANIFEST.json could not be written' in err.getvalue() + assert 'Exported tenant CSV reports' not in out.getvalue() + + def test_open_sinks_partial_failure_still_closes_and_records_the_already_opened_sinks(self): + """ + Round-6 fix: _open_sinks() raising partway through (e.g. a third + sink's file creation fails after two already succeeded) already + surfaced loudly -- that OSError propagates all the way out of + handle(), an already-loud failure. But the sinks it had already + created before that point were previously orphaned: local to + _open_sinks' own now-abandoned local dict, never visible to + handle()'s own `sinks` variable, so the finally block never closed + them (a descriptor leak) and MANIFEST.json['summary_files'] never + recorded them either. _open_sinks now mutates the SAME dict + handle() already holds, so those earlier sinks stay reachable. + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=True) + + output_dir = tempfile.mkdtemp() + real_csv_sink = mod._CsvSink + opened = [] + + class _FailOnThirdSink(real_csv_sink): + _count = [0] + + def __init__(self, path): + _FailOnThirdSink._count[0] += 1 + if _FailOnThirdSink._count[0] == 3: + raise OSError('simulated: third sink failed to open (ENOSPC)') + super(_FailOnThirdSink, self).__init__(path) + opened.append(self) + + with patch.object(mod, '_CsvSink', _FailOnThirdSink): + with self.assertRaises(OSError): + # grades -> grades_summary (1st _CsvSink), profiles -> learner_profile + # (2nd), enrollments -> course_enrollments (3rd, raises). _process_course + # is never reached: _open_sinks() is the first statement inside handle()'s + # try, so the course loop below it never starts. + call_command( + 'export_tenant_reports_csv', sub_org.slug, + '--reports', 'grades,profiles,enrollments', + '--output-dir', output_dir, + ) + + assert len(opened) == 2 + for sink in opened: + assert sink._file.closed + + with open(os.path.join(output_dir, 'MANIFEST.json')) as f: + manifest = json.load(f) + assert manifest['status'] == 'incomplete' + assert sorted(manifest['summary_files']) == ['grades_summary.csv', 'learner_profile.csv'] + + def test_skipped_ora2_course_lands_in_courses_needing_review_without_touching_status(self): + """ + Round-6 fix (2nd advisor-review finding): _run_ora2's {'status': + 'skipped', ...} outcome (an unverifiable identity column -- see + module docstring's ora2 gap) is a deliberate abstention, not a + failure, so it correctly does NOT count toward + manifest['courses_with_errors'] / manifest['status'] downgrading. + But that course's ora2 data really is missing from this export, and + an operator must not have to know to go dig through every per-course + entry in MANIFEST.json['courses'] to discover that -- it belongs in + the separate, weaker manifest['courses_needing_review'] rollup, and + the final banner must still be qualified, not the plain success line. + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=True) + + output_dir = tempfile.mkdtemp() + out = StringIO() + + def skipped_ora2_process_course(self, course_id, *args, **kwargs): + return {'ora2': {'status': 'skipped', 'reason': 'identity column unverified'}} + + with patch.object(Command, '_process_course', skipped_ora2_process_course): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'ora2', + '--output-dir', output_dir, stdout=out, + ) + + with open(os.path.join(output_dir, 'MANIFEST.json')) as f: + manifest = json.load(f) + assert manifest['courses_needing_review'] == [str(course.id)] + # Not a failure -- must not be conflated with the stronger courses_with_errors signal. + assert manifest['courses_with_errors'] == [] + assert manifest['status'] == 'complete' + assert 'Exported tenant CSV reports' not in out.getvalue() + assert 'warning or skipped report' in out.getvalue() + + def test_all_rows_filtered_warning_lands_in_courses_needing_review_without_touching_status(self): + """ + Round-6 fix (2nd advisor-review finding): _empty_filter_warning's + 'warning' key on an otherwise-'success' report (every row for that + report was filtered out by the tenant-membership filter -- usually a + sign the identity column didn't actually match) is the same shape of + "nothing failed, but something's still worth a second look" signal + as the ora2-skipped case above. It must land in + manifest['courses_needing_review'], and must NOT be counted as a + course-level error (manifest['status'] stays plain 'complete'). + """ + sub_org = EdlySubOrganizationFactory() + member = UserFactory() + EdlyMultiSiteAccessFactory(user=member, sub_org=sub_org) + course = CourseOverviewFactory() + CourseEnrollmentFactory(user=member, course_id=course.id, is_active=True) + + output_dir = tempfile.mkdtemp() + out = StringIO() + + def warned_process_course(self, course_id, *args, **kwargs): + return { + 'grades': { + 'status': 'success', 'rows': 0, + 'warning': 'all 3 row(s) were filtered out -- check the identity column', + }, + } + + with patch.object(Command, '_process_course', warned_process_course): + call_command( + 'export_tenant_reports_csv', sub_org.slug, '--reports', 'grades', + '--output-dir', output_dir, stdout=out, + ) + + with open(os.path.join(output_dir, 'MANIFEST.json')) as f: + manifest = json.load(f) + assert manifest['courses_needing_review'] == [str(course.id)] + assert manifest['courses_with_errors'] == [] + assert manifest['status'] == 'complete' + assert 'Exported tenant CSV reports' not in out.getvalue() + assert 'warning or skipped report' in out.getvalue()