Merge branch 'master' into jci/issue#35245

docs/concepts/frontend/static_assets.rst → docs/decisions/0000-static-asset-plan.rst

@@ -1,6 +1,22 @@
-#######################################
-edx-platform Static Asset Pipeline Plan
-#######################################
+0. edx-platform Static Asset Pipeline Plan
+##########################################
+
+Status
+******
+
+Accepted ~2017
+Partially adopted 2017-2024
+
+This was an old plan for modernizing Open edX's frontend assets. We've
+retroactively turned it into an ADR because it has some valuable insights.
+Although most of these improvements weren't applied as written, these ideas
+(particularly, separating Python concerns from frontend tooling concerns) were
+applied to both legacy edx-platform assets as well as the Micro-Frontend
+framework that was developed 2017-2019.
+
+Context, Decision, Consequences
+*******************************
+
 
 Static asset handling in edx-platform has evolved in a messy way over the years.
 This has led to a lot of complexity and inconsistencies. This is a proposal for
@@ -9,20 +25,9 @@ this is not a detailed guide for how to write React or Bootstrap code. This is
 instead going to talk about conventions for how we arrange, extract, and compile
 static assets.
 
-Big Open Questions (TODO)
-*************************
-
-This document is a work in progress, as the design for some of this is still in
-flux, particularly around extensibility.
-
-* Pluggable third party apps and Webpack packaging.
-* Keep the Django i18n mechanism?
-* Stance on HTTP/2 and bundling granularity.
-* Optimizing theme assets.
-* Tests
 
 Requirements
-************
+============
 
 Any proposed solution must support:
 
@@ -35,7 +40,7 @@ Any proposed solution must support:
 * Other kinds of pluggability???
 
 Assumptions
-***********
+===========
 
 Some assumptions/opinions that this proposal is based on:
 
@@ -54,8 +59,8 @@ Some assumptions/opinions that this proposal is based on:
 * It should be possible to pre-build static assets and deploy them onto S3 or
   similar.
 
-Where We Are Today
-******************
+Where We Are Today (2017)
+=========================
 
 We have a static asset pipeline that is mostly driven by Django's built-in
 staticfiles finders and the collectstatic process. We use the popular
@@ -95,9 +100,9 @@ places (typically ``/edx/var/edxapp/staticfiles`` for LMS and
 ``/edx/var/edxapp/staticfiles/studio`` for Studio) and can be collected
 separately. However in practice they're always run together because we deploy
 them from the same commits and to the same servers.
- 
+
 Django vs. Webpack Conventions
-******************************
+==============================
 
 The Django convention for having an app with bundled assets is to namespace them
 locally with the app name so that they get their own directories when they are
@@ -112,7 +117,7 @@ the root of edx-platform, which would specify all bundles in the project.
 TODO: The big, "pluggable Webpack components" question.
 
 Proposed Repo Structure
-***********************
+=======================
 
 All assets that are in common spaces like ``common/static``, ``lms/static``,
 and ``cms/static`` would be moved to be under the Django apps that they are a
@@ -122,7 +127,7 @@ part of and follow the Django naming convention (e.g.
 any client-side templates will be put in ``static/{appname}/templates``.
 
 Proposed Compiled Structure
-***************************
+===========================
 
 This is meant to be a sample of the different types of things we'd have, not a
 full list:
@@ -150,14 +155,14 @@ full list:
   /theming/themes/open-edx
                  /red-theme
                  /edx.org
-  
+
   # XBlocks still collect their assets into a common space (/xmodule goes away)
   # We consider this to be the XBlock Runtime's app, and it collects static
   # assets from installed XBlocks.
   /xblock
 
 Django vs. Webpack Roles
-************************
+========================
 
 Rule of thumb: Django/Python still serves static assets, Webpack processes and
 optimizes them.

docs/decisions/0021-fixing-quality-and-js-checks.rst

@@ -0,0 +1,143 @@
+Fixing the Quality and JS checks
+################################
+
+Status
+******
+
+Accepted
+
+Implemented by https://github.com/openedx/edx-platform/pull/35159
+
+Context
+*******
+
+edx-platform PRs need to pass a series of CI checks before merging, including
+but not limited to: a CLA check, various unit tests, and various code quality
+tests. Of these checks, two checks were implemented using the "Paver" Python
+package, a scripting library `which we have been trying to move off of`_. These
+two checks and their steps were:
+
+* **Check: Quality others**
+
+  * **pii_check**: Ensure that Django models have PII annotations as
+    described in `OEP-30`_, with a minimum threshold of **94.5%** of models
+    annotated.
+  * **stylelint**: Statically check sass stylesheets for common errors.
+  * **pep8**: Run pycodestyle against Python code.
+  * **eslint**: Statically check javascript code for common errors.
+  * **xsslint**: Check python & javascript for xss vulnerabilities.
+  * **check_keywords**: Compare Django model field names against a denylist of
+    reserved keywords.
+
+* **Check: JS**
+
+  * **test-js**: Run javascript unit tests.
+  * **coverage-js**: Check that javascript test coverage has not dropped.
+
+As we worked to reimplement these checks without Paver, we unfortunately
+noticed that four of those steps had bugs in their implementations, and thus
+had not been enforcing what they promised to:
+
+* **pii_check**: Instead of just checking the result of the underlying
+  code_annotations command, this check wrote an annotations report to a file,
+  and then used regex to parse the report and determine whether the check
+  should pass. However, the check failed to validate that the generation of the
+  report itself was successful. So, when malformed annotations were introduced
+  to the edx-proctoring repository, which edx-platform installs, the check
+  began silently passing.
+
+* **stylelint**: At some point, the `stylelint` binary stopped being available
+  on the runner's `$PATH`. Rather than causing the Quality Others check to
+  fail, the Paver code quietly ignored the shell error, and considered the
+  empty stylelint report file to indicate that there were not linting
+  violations.
+
+* **test-js**: There are eight suites within test-js. Six of them work fine.
+  But three of them--specifically the suites that test code built by Webpack--
+  have not been running for some unknown amount of time. The Webpack test build
+  has been failing without signalling that the test suite should fail,
+  both preventing the tests from runnning and preventing anyone from noticing
+  that the tests weren't running.
+
+* **coverage-js**: This check tried to use `diff-cover` in order to compare the
+  coverage report on the current branch with the coverage report on the master
+  branch. However, the coverage report does not exist on the master branch, and
+  it's not clear when it ever did. The coverage-js step failed to validate that
+  `diff-cover` ran successfully, and instead of raising an error, it allowed
+  the JS check to pass.
+
+Decision & Consequences
+***********************
+
+pii_check
+=========
+
+We `fixed the malformed annotations`_ in edx-proctoring, allowing the pii_check
+to once again check model coverage. We have ensured that any future failure of
+the code_annotations command (due to, for example, future malformed
+annotations) will cause the pii_check step and the overall Quality Others check
+to fail. We have stopped trying to parse the result of the annotations report
+in CI, as this was and is completely unneccessary.
+
+In order to keep "Quality others" passing on the edx-platform master branch, we
+lowered the PII annotation coverage threshold to reflect the percentage of
+then-annotated models: **71.6%**. After a timeboxed effort to add missing
+annotations and expand the annotation allowlist as appropriate, we have managed
+to raise the threshold to **85.3%**. It is not clear whether we will put in
+further effort to raise the annotation threshold back to 95%.
+
+This was all already `announced on the forums`_.
+
+stylelint
+=========
+
+We have removed the **stylelint** step entirely from the "Quality Others"
+check. Sass code in the edx-platform repository will no longer be subject to
+any static analysis.
+
+test-js
+=======
+
+We have stopped running these Webpack-based suites in CI:
+
+* ``npm run test-lms-webpack``
+* ``npm run test-cms-webpack``
+* ``npm run test-xmodule-webpack``
+
+We have created a new edx-platform backlog issue for
+`fixing and re-enabling these suites`_.
+It is not clear whether we will prioritize that issue, or instead prioritize
+deprecation and removal of the code that those suites were supposed to be
+testing.
+
+coverage-js
+===========
+
+We will remove the **coverage-js** step entirely from the "JS" check.
+JavaScript code in the edx-platform repository will no longer be subject to any
+unit test coverage checking.
+
+Rejected Alternatives
+*********************
+
+* While it would be ideal to raise the pii_check threshold to 94.5% or even
+  100%, we do not have the resources to promise this.
+
+* It would also be nice to institute a "racheting" mechanism for the PII
+  annotation coverage threshold. That is, every commit to master could save the
+  coverage percentage to a persisted artifact, allowing subsequent PRs to
+  ensure that the pii_check never returns lower than the current threshold. We
+  will put this in the Aximprovements backlog, but we cannot commit to
+  implementing it right now.
+
+* We will not fix or apply amnestly in order to re-enable stlylint or
+  coverage-js. That could take significant effort, which we believe would be
+  better spent completing the migration off of this legacy Sass and JS and onto
+  our modern React frontends.
+
+
+.. _fixing and re-enabling these suites: https://github.com/openedx/edx-platform/issues/35956
+.. _which we have been trying to move off of: https://github.com/openedx/edx-platform/issues/34467
+.. _announced on the forums: https://discuss.openedx.org/t/checking-pii-annotations-with-a-lower-coverage-threshold/14254
+.. _OEP-30: https://docs.openedx.org/projects/openedx-proposals/en/latest/architectural-decisions/oep-0030-arch-pii-markup-and-auditing.html
+.. _fix the malformed annotations: https://github.com/openedx/edx-proctoring/issues/1241

lms/djangoapps/course_goals/management/commands/goal_reminder_email.py

@@ -1,6 +1,7 @@
 """
 Command to trigger sending reminder emails for learners to achieve their Course Goals
 """
+import time
 from datetime import date, datetime, timedelta
 from eventtracking import tracker
 import logging
@@ -119,9 +120,9 @@ def send_ace_message(goal, session_id):
 
     with emulate_http_request(site, user):
         try:
-            start_time = datetime.now()
+            start_time = time.perf_counter()
             ace.send(msg)
-            end_time = datetime.now()
+            end_time = time.perf_counter()
             log.info(f"Goal Reminder for {user.id} for course {goal.course_key} sent in {end_time - start_time} "
                      f"using {'SES' if is_ses_enabled else 'others'}")
         except Exception as exc:  # pylint: disable=broad-except

openedx/core/djangoapps/notifications/tasks.py

@@ -187,7 +187,7 @@ def send_notifications(user_ids, course_key: str, app_name, notification_type, c
                     group_user_notifications(new_notification, existing_notifications[user_id])
                 else:
                     notifications.append(new_notification)
-                    generated_notification_audience.append(user_id)
+                generated_notification_audience.append(user_id)
 
         # send notification to users but use bulk_create
         notification_objects = Notification.objects.bulk_create(notifications)

package.json

@@ -14,24 +14,25 @@
     "watch-webpack": "npm run webpack-dev -- --watch",
     "watch-sass": "scripts/watch_sass.sh",
     "lint": "python scripts/eslint.py",
-    "test": "npm run test-cms && npm run test-lms && npm run test-xmodule && npm run test-common && npm run test-jest",
-    "test-kind-vanilla": "npm run test-cms-vanilla && npm run test-xmodule-vanilla && npm run test-common-vanilla",
-    "test-kind-require": "npm run test-cms-require && npm run test-common-require",
-    "test-kind-webpack": "npm run test-cms-webpack && npm run test-lms-webpack && npm run test-xmodule-webpack",
-    "test-cms": "npm run test-cms-vanilla && npm run test-cms-require",
-    "test-cms-vanilla": "npm run test-suite -- cms/static/karma_cms.conf.js",
-    "test-cms-require": "npm run test-suite -- cms/static/karma_cms_squire.conf.js",
-    "test-cms-webpack": "npm run test-suite -- cms/static/karma_cms_webpack.conf.js",
-    "test-lms": "echo 'WARNING: Webpack JS tests are disabled. No LMS JS tests will be run. See https://github.com/openedx/edx-platform/issues/35956 for details.'",
-    "test-lms-webpack": "npm run test-suite -- lms/static/karma_lms.conf.js",
-    "test-xmodule": "npm run test-xmodule-vanilla",
-    "test-xmodule-vanilla": "npm run test-suite -- xmodule/js/karma_xmodule.conf.js",
-    "test-xmodule-webpack": "npm run test-suite -- xmodule/js/karma_xmodule_webpack.conf.js",
+    "test": "npm run test-jest && npm run test-karma",
+    "test-jest": "jest",
+    "test-karma": "npm run test-karma-vanilla && npm run test-karma-require && echo 'WARNING: Skipped broken webpack tests. For details, see: https://github.com/openedx/edx-platform/issues/35956'",
+    "test-karma-vanilla": "npm run test-cms-vanilla && npm run test-xmodule-vanilla && npm run test-common-vanilla",
+    "test-karma-require": "npm run test-cms-require && npm run test-common-require",
+    "test-karma-webpack": "npm run test-cms-webpack && npm run test-lms-webpack && npm run test-xmodule-webpack",
+    "test-karma-conf": "${NODE_WRAPPER:-xvfb-run --auto-servernum} node --max_old_space_size=4096 node_modules/.bin/karma start --single-run=true --capture-timeout=60000 --browsers=FirefoxNoUpdates",
+    "test-cms": "npm run test-cms-vanilla && npm run test-cms-require && npm run test-cms-webpack",
+    "test-cms-vanilla": "npm run test-karma-conf -- cms/static/karma_cms.conf.js",
+    "test-cms-require": "npm run test-karma-conf -- cms/static/karma_cms_squire.conf.js",
+    "test-cms-webpack": "npm run test-karma-conf -- cms/static/karma_cms_webpack.conf.js",
+    "test-lms": "npm run test-jest && npm run test-lms-webpack",
+    "test-lms-webpack": "npm run test-karma-conf -- lms/static/karma_lms.conf.js",
+    "test-xmodule": "npm run test-xmodule-vanilla && npm run test-xmodule-webpack",
+    "test-xmodule-vanilla": "npm run test-karma-conf -- xmodule/js/karma_xmodule.conf.js",
+    "test-xmodule-webpack": "npm run test-karma-conf -- xmodule/js/karma_xmodule_webpack.conf.js",
     "test-common": "npm run test-common-vanilla && npm run test-common-require",
-    "test-common-vanilla": "npm run test-suite -- common/static/karma_common.conf.js",
-    "test-common-require": "npm run test-suite -- common/static/karma_common_requirejs.conf.js",
-    "test-suite": "${NODE_WRAPPER:-xvfb-run --auto-servernum} node --max_old_space_size=4096 node_modules/.bin/karma start --single-run=true --capture-timeout=60000 --browsers=FirefoxNoUpdates",
-    "test-jest": "jest"
+    "test-common-vanilla": "npm run test-karma-conf -- common/static/karma_common.conf.js",
+    "test-common-require": "npm run test-karma-conf -- common/static/karma_common_requirejs.conf.js"
   },
   "dependencies": {
     "@babel/core": "7.26.0",

requirements/constraints.txt

@@ -78,7 +78,7 @@ django-storages<1.14.4
 # The team that owns this package will manually bump this package rather than having it pulled in automatically.
 # This is to allow them to better control its deployment and to do it in a process that works better
 # for them.
-edx-enterprise==5.5.1
+edx-enterprise==5.5.2
 
 # Date: 2024-05-09
 # This has to be constrained as well because newer versions of edx-i18n-tools need the

requirements/edx/base.txt

@@ -472,7 +472,7 @@ edx-drf-extensions==10.5.0
     #   edx-when
     #   edxval
     #   openedx-learning
-edx-enterprise==5.5.1
+edx-enterprise==5.5.2
     # via
     #   -c requirements/edx/../constraints.txt
     #   -r requirements/edx/kernel.in

requirements/edx/development.txt

@@ -748,7 +748,7 @@ edx-drf-extensions==10.5.0
     #   edx-when
     #   edxval
     #   openedx-learning
-edx-enterprise==5.5.1
+edx-enterprise==5.5.2
     # via
     #   -c requirements/edx/../constraints.txt
     #   -r requirements/edx/doc.txt

requirements/edx/doc.txt

@@ -555,7 +555,7 @@ edx-drf-extensions==10.5.0
     #   edx-when
     #   edxval
     #   openedx-learning
-edx-enterprise==5.5.1
+edx-enterprise==5.5.2
     # via
     #   -c requirements/edx/../constraints.txt
     #   -r requirements/edx/base.txt

requirements/edx/testing.txt

@@ -576,7 +576,7 @@ edx-drf-extensions==10.5.0
     #   edx-when
     #   edxval
     #   openedx-learning
-edx-enterprise==5.5.1
+edx-enterprise==5.5.2
     # via
     #   -c requirements/edx/../constraints.txt
     #   -r requirements/edx/base.txt