Merge branch 'master' into a-asad/video-player-logging-fix

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

openedx/core/djangoapps/notifications/email/utils.py

@@ -9,7 +9,7 @@
 from django.contrib.auth import get_user_model
 from django.shortcuts import get_object_or_404
 from pytz import utc
-from waffle import get_waffle_flag_model   # pylint: disable=invalid-django-waffle-import
+from waffle import get_waffle_flag_model  # pylint: disable=invalid-django-waffle-import
 
 from common.djangoapps.student.models import CourseEnrollment
 from lms.djangoapps.branding.api import get_logo_url_for_email
@@ -29,7 +29,6 @@
 
 from .notification_icons import NotificationTypeIcons
 
-
 User = get_user_model()
 
 
@@ -370,14 +369,6 @@ def is_name_match(name, param_name):
         """
         return True if param_name is None else name == param_name
 
-    def is_editable(app_name, notification_type, channel):
-        """
-        Returns if notification type channel is editable
-        """
-        if notification_type == 'core':
-            return channel not in COURSE_NOTIFICATION_APPS[app_name]['non_editable']
-        return channel not in COURSE_NOTIFICATION_TYPES[notification_type]['non_editable']
-
     def get_default_cadence_value(app_name, notification_type):
         """
         Returns default email cadence value
@@ -417,9 +408,18 @@ def get_updated_preference(pref):
                 for channel in ['web', 'email', 'push']:
                     if not is_name_match(channel, channel_value):
                         continue
-                    if is_editable(app_name, noti_type, channel):
+                    if is_notification_type_channel_editable(app_name, noti_type, channel):
                         type_prefs[channel] = pref_value
                         if channel == 'email' and pref_value and type_prefs.get('email_cadence') == EmailCadence.NEVER:
                             type_prefs['email_cadence'] = get_default_cadence_value(app_name, noti_type)
         preference.save()
     notification_preference_unsubscribe_event(user)
+
+
+def is_notification_type_channel_editable(app_name, notification_type, channel):
+    """
+    Returns if notification type channel is editable
+    """
+    if notification_type == 'core':
+        return channel not in COURSE_NOTIFICATION_APPS[app_name]['non_editable']
+    return channel not in COURSE_NOTIFICATION_TYPES[notification_type]['non_editable']