Test tidying sandbox#845
Closed
Kelketek wants to merge 477 commits into
Closed
Conversation
…s.py Two imports were genuinely unused: - `Mock, patch` from unittest.mock: the file uses `mock.Mock()` and `mock.patch()` via the `mock` module imported on the line above, never the bare names. - `CommentClientMaintenanceError`: imported but never referenced in the file or re-exported to other modules. Both were kept with noqa suppressions that are no longer needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The UP032 noqa suppression added by the --add-noqa pass was placed on the continuation line of a backslash-split string literal, where ruff cannot see it (the violation is reported on the line where the string starts). Rather than restructuring the suppression, convert the two-line backslash-continued .format() call to an f-string, which fits within the 120-char line limit and removes the need for any suppression. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ruff's --add-noqa injected # noqa: UP032 after the backslash line continuation on two multi-line .format() strings in get_expiration_banner_text(), corrupting the syntax (a comment cannot follow a line continuation character). Rather than converting to f-strings, use ruff's block-level suppression (# ruff: disable[UP032] / # ruff: enable[UP032]) which avoids the syntax corruption and results in a smaller diff than an f-string rewrite. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The ruff --add-noqa pass added # noqa: <RULE> comments to all lines with existing violations. Pylint's C0301 (line-too-long) counts the full line including trailing comments, so these additions pushed 292 lines over 120 chars. Add per-line # pylint: disable=line-too-long to each of those lines. On lines that already had a # pylint: disable=<something> clause, extend the existing clause (e.g. # pylint: disable=raise-missing-from,line-too-long) rather than adding a second directive. Also removes # lint-amnesty, prefixes encountered during the edit pass -- these were temporary suppressions that are no longer needed. This suppression is temporary while both pylint and ruff are running. Once we drop pylint (PR 4+), all these per-line # pylint: disable=line-too-long comments can be removed as part of that cleanup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix: apply feedback related to test and types refactor: abstract process in _get_course_keys_from_scopes
Add noqa comments to suppress PT009 linting warnings for unittest-style assertions (assertEqual, assertTrue, etc.) which are appropriate for tests inheriting from unittest.TestCase/ModuleStoreTestCase.
feat: remove change_enterprise_user_username management command
feat: support organization-level scopes for course list [authz]
Push this forward so things don't automatically in a few years for future courses.
…RT_DATE Tests were asserting against the literal `2030-01-01` value instead of importing the constant, causing failures after the default was updated to 2040. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Commit 3453e7d introduced a fix to synchronize DiscussionsConfiguration.enabled (DB) with tab.is_hidden (modulestore) during course imports. This commit improves upon that fix in two ways: * It updates the discussion CourseAppStatus as well, which is necessary in order for the app to be fully enabled. * It adds a data migration to perform the above synchronization retroactively for all existing courses, using the CourseOverTab table.
* feat: use openedx-core branch with strongly-typed keys * chore: update to use strongly-typed IDs from openedx_content * feat: fully typed primary keys for StagedContent model * chore: misc typing improvements + type-check `helpers.py` in CMS * chore: explain mypy error and suppress it for now * chore: use .id instead of .pk
* fix: update legacy attribute key
…hedule & details page changes
fix: add weight from max_weight if its missing from meta
…ublished-signals fix: fix multiple COURSE_PUBLISHED signals being fired when saving
…ubclasses Three test classes in the certificates app were calling CourseFactory() in setUp() despite extending SharedModuleStoreTestCase. Unlike ModuleStoreTestCase, SharedModuleStoreTestCase shares a single modulestore across all tests in the class and only closes MongoDB connections at tearDownClass. Calling CourseFactory() in setUp() created a new MongoDB course (and opened connections) for every test method without releasing them, causing connection accumulation across the full test run. Affected classes: - CertificateFiltersTest (test_filters.py) - CertificateInvalidationTest (test_models.py) - CertificateAllowlistTest (test_models.py) In each case the course is only read by test methods (test data such as users, enrollments and certificates is written via Django ORM and rolled back between tests), so sharing a single course across the class is correct. See: https://github.com/openedx/openedx-platform/blob/master/xmodule/modulestore/tests/django_utils.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…vents.testing openedx_events/tests/utils.py was moved to openedx_events/testing.py in openedx/openedx-events#559 so the test utilities are included in the installed package (setup.py excludes the tests/ subpackage from the wheel). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
When OpenEdxEventsTestMixin was listed after a TestCase subclass (e.g. Foo(SharedModuleStoreTestCase, OpenEdxEventsTestMixin)), it landed after unittest.case.TestCase in the MRO. Since unittest.case.TestCase.setUpClass and tearDownClass do not call super(), the mixin's lifecycle methods never ran. The workaround was to manually call cls.start_events_isolation() in each class's setUpClass, but there was no corresponding tearDownClass to restore event state, causing events disabled by one test class to leak into subsequent classes in the same process. Fix by placing OpenEdxEventsTestMixin first in the base class list so it appears before unittest.case.TestCase in the MRO. This lets setUpClass and tearDownClass run automatically through the cooperative super() chain, removing the need for manual start_events_isolation() calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This mixin is already included via one of the other mixins on this test class so including it again was messing with the MRO for the test classes.
Commit generated by workflow `openedx/openedx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` Co-authored-by: bcitro <67378070+bcitro@users.noreply.github.com>
…ata (#38427) - Adds redaction + delete flow for PendingSecondaryEmailChange. - Updates AccountRecovery.retire_recovery_email to redact before delete. - Integrated cleanup into retirement/deactivation flows and management commands. - Adds redact before delete testing utilities.
Remove user retirement Day 0 social-auth unlinking from create retirement so cancel retirement can fully undo.
Fixes for PII annotations Commit generated by workflow `openedx/openedx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
…-wiki-66758e9 feat: Upgrade Python dependency openedx-django-wiki
… retirement (#38671) When ENABLE_REDACT_HISTORICAL_PII_RETIREMENT is enabled, GeneratedCertificate's django-simple-history table (certificates_historicalgeneratedcertificate) will also have the user's name redacted as part of user retirement.
fix: remove annotated models from safelist and annotate openedx models on PII
The PUT /api/contentstore/v1/videos/{course_id}/download endpoint fetched
every client-supplied files[].url server-side with
requests.get(url, allow_redirects=True) and returned the bytes inside the
ZIP response. Because the URLs were never validated, an authenticated user
with studio read access could point them at internal services or cloud
metadata endpoints and exfiltrate the responses (GHSA-fpf9-9rpr-jvrx).
By design these URLs are always a subset of the course's own VAL
encoded_videos[].url values (the same data the video listing hands the
frontend). Restrict fetches to that allowlist: build the set of legitimate
URLs for the course and reject any request containing a URL outside it
before any HTTP request is made. This eliminates the SSRF rather than
merely narrowing it.
Adds VideoDownloadViewTest (the endpoint previously had no test coverage)
covering the allowed-URL success path, rejection of disallowed URLs without
any outbound request, mixed allowed/disallowed batches, and the non-staff
permission gate.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fix: int channels safe dict lookup Commit generated by workflow `openedx/openedx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master`
Commit generated by workflow `openedx/openedx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` Co-authored-by: bcitro <67378070+bcitro@users.noreply.github.com>
django-countries 9.0.0 changed nullable CountryField semantics: a NULL database value now returns None instead of Country(code=None). Update all UserProfile.country call sites that previously relied on the old behavior: - Replace `country.code is None` checks with `country is None` (embargo/api.py, credit/api/provider.py). - Guard `.code`/`.name` attribute accesses against None (credit/tests/factories.py, user_api/accounts/serializers.py, courseware/views/views.py). - Emit "" instead of str(None) == "None" for Segment traits when country is null (student/models/user.py, user_authn/views/register.py) to preserve existing serialized output and matching test assertions. Release notes: https://github.com/SmileyChris/django-countries/blob/main/CHANGES.rst#900-10-june-2026 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 'render the volume control' spec asserted only that *some* element with class `.volume` existed in the global DOM. It never referenced the `volumeControl` instance set up in `beforeEach`, so it would pass as long as any `.volume`-classed element was rendered anywhere — and it ran synchronously, so it didn't tolerate the rendering completing on a later tick. Wait for VideoVolumeControl's own element to attach to its `.secondary-controls` parent using the `jasmine.waitUntil` pattern already established in neighboring specs (video_poster_spec.js, video_progress_slider_spec.js), then assert the element is in the DOM. Add an explicit `.fail()` branch so the next timeout produces an actionable error instead of a generic Jasmine timeout.
Before this change, the Student Profile Information CSV that instructors download from the instructor dashboard showed the literal text "None" in the city column for users who had not set a city. The country column for the same users showed up as an empty cell instead. The cause was a quirk in how the report extracted values: any field whose Python value was None ended up stringified to "None", but country was a special object that stringified to "" instead. A 2016 code comment flagged this as "somewhat inconsistent" but it was never fixed. The django-countries 9.0.0 upgrade removes the country special case: an unset country is now plain None, just like city. That made the country column also show "None", which broke the test that documented the old quirk. Rather than accept "None" cells in two columns, this commit fixes the underlying behaviour so any unset field shows as an empty cell. BREAKING CHANGE: In the Student Profile Information CSV report, cells for unset fields (city, country, language, mailing address, etc.) that used to read the literal text "None" are now empty cells. Country cells that used to be empty stay empty. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
openedx-authz 1.18.0 added validation that rejects the bare global scope wildcard '*'. content_libraries calls authz_api.is_user_allowed with that wildcard from user_can_create_library, so the upgrade broke LibraryRestoreViewTestCase::test_restore_library_unauthorized (and likely other code paths exercised at runtime). Roll openedx-authz back to 1.16.0 until the team owning the authz migration lands the call-site updates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Settings
Tutor requirements