From bdb1ae53151c43de258b5997f011d171c73523d3 Mon Sep 17 00:00:00 2001 From: marlonkeating <322346+marlonkeating@users.noreply.github.com> Date: Wed, 18 Dec 2024 16:19:29 +0000 Subject: [PATCH 1/8] feat: Upgrade Python dependency edx-enterprise This change adds support for the page_size query parameter on the enterprise-customer-members endpoint. Commit generated by workflow `openedx/edx-platform/.github/workflows/upgrade-one-python-dependency.yml@refs/heads/master` --- requirements/constraints.txt | 2 +- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/doc.txt | 2 +- requirements/edx/testing.txt | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index 24d822f0874..e65e19a574f 100644 --- a/requirements/constraints.txt +++ b/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 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 6f942d00b09..bbe8c32ce54 100644 --- a/requirements/edx/base.txt +++ b/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 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 97b74c737b6..e32139843ed 100644 --- a/requirements/edx/development.txt +++ b/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 diff --git a/requirements/edx/doc.txt b/requirements/edx/doc.txt index 4261f47c2be..76005577e95 100644 --- a/requirements/edx/doc.txt +++ b/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 diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index dbc2ea7613f..5880b578d9a 100644 --- a/requirements/edx/testing.txt +++ b/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 From 4c0d94e367033351690e6c776ff28a567930cce8 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Thu, 12 Dec 2024 14:11:13 -0500 Subject: [PATCH 2/8] docs: Update testing guide --- docs/concepts/testing/testing.rst | 324 +++++++++--------------------- 1 file changed, 94 insertions(+), 230 deletions(-) diff --git a/docs/concepts/testing/testing.rst b/docs/concepts/testing/testing.rst index 9d448afd5bd..d8a79b9d661 100644 --- a/docs/concepts/testing/testing.rst +++ b/docs/concepts/testing/testing.rst @@ -1,4 +1,3 @@ -####### Testing ####### @@ -7,7 +6,7 @@ Testing :depth: 3 Overview -======== +******** We maintain two kinds of tests: unit tests and integration tests. @@ -26,10 +25,10 @@ tests. Most of our tests are unit tests or integration tests. Test Types ----------- +========== Unit Tests -~~~~~~~~~~ +---------- - Each test case should be concise: setup, execute, check, and teardown. If you find yourself writing tests with many steps, @@ -38,18 +37,18 @@ Unit Tests - As a rule of thumb, your unit tests should cover every code branch. -- Mock or patch external dependencies. We use the voidspace `Mock Library`_. +- Mock or patch external dependencies using `unittest.mock`_ functions. - We unit test Python code (using `unittest`_) and Javascript (using `Jasmine`_) -.. _Mock Library: http://www.voidspace.org.uk/python/mock/ +.. _unittest.mock: https://docs.python.org/3/library/unittest.mock.html .. _unittest: http://docs.python.org/2/library/unittest.html .. _Jasmine: http://jasmine.github.io/ Integration Tests -~~~~~~~~~~~~~~~~~ +----------------- - Test several units at the same time. Note that you can still mock or patch dependencies that are not under test! For example, you might test that @@ -67,7 +66,7 @@ Integration Tests .. _Django test client: https://docs.djangoproject.com/en/dev/topics/testing/overview/ Test Locations --------------- +============== - Python unit and integration tests: Located in subpackages called ``tests``. For example, the tests for the ``capa`` package are @@ -80,14 +79,29 @@ Test Locations the test for ``src/views/module.js`` should be written in ``spec/views/module_spec.js``. -Running Tests -============= +Factories +========= -**Unless otherwise mentioned, all the following commands should be run from inside the lms docker container.** +Many tests delegate set-up to a "factory" class. For example, there are +factories for creating courses, problems, and users. This encapsulates +set-up logic from tests. +Factories are often implemented using `FactoryBoy`_. + +In general, factories should be located close to the code they use. For +example, the factory for creating problem XML definitions is located in +``xmodule/capa/tests/response_xml_factory.py`` because the +``capa`` package handles problem XML. + +.. _FactoryBoy: https://readthedocs.org/projects/factoryboy/ Running Python Unit tests -------------------------- +************************* + +The following commands need to be run within a Python environment in +which requirements/edx/testing.txt has been installed. If you are using a +Docker-based Open edX distribution, then you probably will want to run these +commands within the LMS and/or CMS Docker containers. We use `pytest`_ to run Python tests. Pytest is a testing framework for python and should be your goto for local Python unit testing. @@ -97,16 +111,16 @@ Pytest (and all of the plugins we use with it) has a lot of options. Use `pytest Running Python Test Subsets -~~~~~~~~~~~~~~~~~~~~~~~~~~~ +=========================== When developing tests, it is often helpful to be able to really just run one single test without the overhead of PIP installs, UX builds, etc. Various ways to run tests using pytest:: - pytest path/test_m­odule.py # Run all tests in a module. - pytest path/test_m­odule.p­y:­:te­st_func # Run a specific test within a module. - pytest path/test_m­odule.p­y:­:Te­stC­las­s # Run all tests in a class - pytest path/test_m­odule.p­y:­:Te­stC­las­s::­tes­t_m­ethod # Run a specific method of a class. + pytest path/test_module.py # Run all tests in a module. + pytest path/test_module.py::test_func # Run a specific test within a module. + pytest path/test_module.py::TestClass # Run all tests in a class + pytest path/test_module.py::TestClass::test_method # Run a specific method of a class. pytest path/testing/ # Run all tests in a directory. For example, this command runs a single python unit test file:: @@ -114,7 +128,7 @@ For example, this command runs a single python unit test file:: pytest xmodule/tests/test_stringify.py Note - -edx-platorm has multiple services (lms, cms) in it. The environment for each service is different enough that we run some tests in both environments in Github Actions. +edx-platorm has multiple services (lms, cms) in it. The environment for each service is different enough that we run some tests in both environments in Github Actions. To test in each of these environments (especially for tests in "common" and "xmodule" directories), you will need to test in each seperately. To specify that the tests are run with the relevant service as root, Add --rootdir flag at end of your pytest call and specify the env to test in:: @@ -139,7 +153,7 @@ Various tools like ddt create tests with very complex names, rather than figurin pytest xmodule/tests/test_stringify.py --collectonly Testing with migrations -*********************** +======================= For the sake of speed, by default the python unit test database tables are created directly from apps' models. If you want to run the tests @@ -149,7 +163,7 @@ against a database created by applying the migrations instead, use the pytest test --create-db --migrations Debugging a test -~~~~~~~~~~~~~~~~ +================ There are various ways to debug tests in Python and more specifically with pytest: @@ -173,7 +187,7 @@ There are various ways to debug tests in Python and more specifically with pytes How to output coverage locally -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +============================== These are examples of how to run a single test and get coverage:: @@ -220,234 +234,84 @@ run one of these commands:: .. _YouTube stub server: https://github.com/openedx/edx-platform/blob/master/common/djangoapps/terrain/stubs/tests/test_youtube_stub.py -Debugging Unittest Flakiness -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -As we move over to running our unittests with Jenkins Pipelines and pytest-xdist, -there are new ways for tests to flake, which can sometimes be difficult to debug. -If you run into flakiness, check (and feel free to contribute to) this -`confluence document `__ for help. - -Running Javascript Unit Tests ------------------------------ - -Before running Javascript unit tests, you will need to be running Firefox or Chrome in a place visible to edx-platform. If running this in devstack, you can run ``make dev.up.firefox`` or ``make dev.up.chrome``. Firefox is the default browser for the tests, so if you decide to use Chrome, you will need to prefix the test command with ``SELENIUM_BROWSER=chrome SELENIUM_HOST=edx.devstack.chrome`` (if using devstack). - -We use Jasmine to run JavaScript unit tests. To run all the JavaScript -tests:: - - paver test_js - -To run a specific set of JavaScript tests and print the results to the -console, run these commands:: - - paver test_js_run -s lms - paver test_js_run -s cms - paver test_js_run -s cms-squire - paver test_js_run -s xmodule - paver test_js_run -s xmodule-webpack - paver test_js_run -s common - paver test_js_run -s common-requirejs - -To run JavaScript tests in a browser, run these commands:: - - paver test_js_dev -s lms - paver test_js_dev -s cms - paver test_js_dev -s cms-squire - paver test_js_dev -s xmodule - paver test_js_dev -s xmodule-webpack - paver test_js_dev -s common - paver test_js_dev -s common-requirejs - -Debugging Specific Javascript Tests -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -The best way to debug individual tests is to run the test suite in the browser and -use your browser's Javascript debugger. The debug page will allow you to select -an individual test and only view the results of that test. - - -Debugging Tests in a Browser -~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -To debug these tests on devstack in a local browser: - -* first run the appropriate test_js_dev command from above -* open http://localhost:19876/debug.html in your host system's browser of choice -* this will run all the tests and show you the results including details of any failures -* you can click on an individually failing test and/or suite to re-run it by itself -* you can now use the browser's developer tools to debug as you would any other JavaScript code - -Note: the port is also output to the console that you ran the tests from if you find that easier. - -These paver commands call through to Karma. For more -info, see `karma-runner.github.io `__. - -Testing internationalization with dummy translations ----------------------------------------------------- - -Any text you add to the platform should be internationalized. To generate translations for your new strings, run the following command:: - - paver i18n_dummy - -This command generates dummy translations for each dummy language in the -platform and puts the dummy strings in the appropriate language files. -You can then preview the dummy languages on your local machine and also in your sandbox, if and when you create one. - -The dummy language files that are generated during this process can be -found in the following locations:: - - conf/locale/{LANG_CODE} - -There are a few JavaScript files that are generated from this process. You can find those in the following locations:: - - lms/static/js/i18n/{LANG_CODE} - cms/static/js/i18n/{LANG_CODE} - -Do not commit the ``.po``, ``.mo``, ``.js`` files that are generated -in the above locations during the dummy translation process! +Handling flaky unit tests +========================= -Test Coverage and Quality -------------------------- +See this `confluence document `_. -Viewing Test Coverage -~~~~~~~~~~~~~~~~~~~~~ -We currently collect test coverage information for Python -unit/integration tests. +Running JavaScript Unit Tests +***************************** -To view test coverage: +Before running Javascript unit tests, you will need to be running Firefox or Chrome in a place visible to edx-platform. +If you are using Tutor Dev to run edx-platform, then you can do so by installing and enabling the +``test-legacy-js`` plugin from `openedx-tutor-plugins`_, and then rebuilding +the ``openedx-dev`` image:: -1. Run the test suite with this command:: + tutor plugins install https://github.com/openedx/openedx-tutor-plugins/tree/main/plugins/tutor-contrib-test-legacy-js + tutor plugins enable test-legacy-js + tutor images build openedx-dev - paver test +.. _openedx-tutor-plugins: https://github.com/openedx/openedx-tutor-plugins/ -2. Generate reports with this command:: +We use Jasmine (via Karma) to run most JavaScript unit tests. We use Jest to +run a small handful of additional JS unit tests. You can use the ``npm run +test*`` commands to run them:: - paver coverage + npm run test-karma # Run all Jasmine+Karma tests. + npm run test-jest # Run all Jest tests. + npm run test # Run both of the above. -3. Reports are located in the ``reports`` folder. The command generates - HTML and XML (Cobertura format) reports. +The Karma tests are further broken down into three types depending on how the +JavaScript it is testing is built:: -Python Code Style Quality -~~~~~~~~~~~~~~~~~~~~~~~~~ + npm run test-karma-vanilla # Our very oldest JS, which doesn't even use RequireJS + npm run test-karma-require # Old JS that uses RequireJS + npm run test-karma-webpack # Slightly "newer" JS which is built with Webpack -To view Python code style quality (including PEP 8 and pylint violations) run this command:: +Unfortunately, at the time of writing, the build for the ``test-karma-webpack`` +tests is broken. The tests are excluded from ``npm run test-karma`` as to not +fail CI. We `may fix this one day`_. - paver run_quality +.. _may fix this one day: https://github.com/openedx/edx-platform/issues/35956 -More specific options are below. +To run all Karma+Jasmine tests for a particular top-level edx-platform folder, +you can run:: -- These commands run a particular quality report:: + npm run test-cms + npm run test-lms + npm run test-xmodule + npm run test-common - paver run_pep8 - paver run_pylint - -- This command runs a report, and sets it to fail if it exceeds a given number - of violations:: - - paver run_pep8 --limit=800 - -- The ``run_quality`` uses the underlying diff-quality tool (which is packaged - with `diff-cover`_). With that, the command can be set to fail if a certain - diff threshold is not met. For example, to cause the process to fail if - quality expectations are less than 100% when compared to master (or in other - words, if style quality is worse than what is already on master):: - - paver run_quality --percentage=100 - -- Note that 'fixme' violations are not counted with run\_quality. To - see all 'TODO' lines, use this command:: - - paver find_fixme --system=lms - - ``system`` is an optional argument here. It defaults to - ``cms,lms,common``. - -.. _diff-cover: https://github.com/Bachmann1234/diff-cover - - -JavaScript Code Style Quality -~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -To view JavaScript code style quality run this command:: - - paver run_eslint - -- This command also comes with a ``--limit`` switch, this is an example of that switch:: - - paver run_eslint --limit=50000 - - -Code Complexity Tools -===================== - -Tool(s) available for evaluating complexity of edx-platform code: - - -- `plato `__ for JavaScript code - complexity. Several options are available on the command line; see - documentation. Below, the following command will produce an HTML report in a - subdirectory called "jscomplexity":: - - plato -q -x common/static/js/vendor/ -t common -e .eslintrc.json -r -d jscomplexity common/static/js/ - -Other Testing Tips -================== - -Connecting to Browser ---------------------- - -If you want to see the browser being automated for JavaScript, -you can connect to the container running it via VNC. - -+------------------------+----------------------+ -| Browser | VNC connection | -+========================+======================+ -| Firefox (Default) | vnc://0.0.0.0:25900 | -+------------------------+----------------------+ -| Chrome (via Selenium) | vnc://0.0.0.0:15900 | -+------------------------+----------------------+ - -On macOS, enter the VNC connection string in Safari to connect via VNC. The VNC -passwords for both browsers are randomly generated and logged at container -startup, and can be found by running ``make vnc-passwords``. - -Most tests are run in Firefox by default. To use Chrome for tests that normally -use Firefox instead, prefix the test command with -``SELENIUM_BROWSER=chrome SELENIUM_HOST=edx.devstack.chrome`` - -Factories ---------- - -Many tests delegate set-up to a "factory" class. For example, there are -factories for creating courses, problems, and users. This encapsulates -set-up logic from tests. - -Factories are often implemented using `FactoryBoy`_. - -In general, factories should be located close to the code they use. For -example, the factory for creating problem XML definitions is located in -``xmodule/capa/tests/response_xml_factory.py`` because the -``capa`` package handles problem XML. - -.. _FactoryBoy: https://readthedocs.org/projects/factoryboy/ +Finally, if you want to pass any options to the underlying ``node`` invocation +for Karma+Jasmine tests, you can run one of these specific commands, and put +your arguments after the ``--`` separator:: -Running Tests on Paver Scripts ------------------------------- + npm run test-cms-vanilla -- --your --args --here + npm run test-cms-require -- --your --args --here + npm run test-cms-webpack -- --your --args --here + npm run test-lms-webpack -- --your --args --here + npm run test-xmodule-vanilla -- --your --args --here + npm run test-xmodule-webpack -- --your --args --here + npm run test-common-vanilla -- --your --args --here + npm run test-common-require -- --your --args --here -To run tests on the scripts that power the various Paver commands, use the following command:: - pytest pavelib +Code Quality +************ -Testing using queue servers ---------------------------- +We use several tools to analyze code quality. The full set of them is:: -When testing problems that use a queue server on AWS (e.g. -sandbox-xqueue.edx.org), you'll need to run your server on your public IP, like so:: + mypy $PATHS... + pycodestyle $PATHS... + pylint $PATHS... + lint-imports + scripts/verify-dunder-init.sh + make xsslint + make pii_check + make check_keywords + npm run lint - ./manage.py lms runserver 0.0.0.0:8000 +Where ``$PATHS...`` is a list of folders and files to analyze, or nothing if +you would like to analyze the entire codebase (which can take a while). -When you connect to the LMS, you need to use the public ip. Use -``ifconfig`` to figure out the number, and connect e.g. to -``http://18.3.4.5:8000/`` From 41d28f3ce332d824891a6127367fe1d0cc21181c Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Tue, 17 Dec 2024 15:30:07 -0500 Subject: [PATCH 3/8] docs: Turn old static asset plan into a retroactive ADR --- .../0000-static-asset-plan.rst} | 53 ++++++++++--------- 1 file changed, 29 insertions(+), 24 deletions(-) rename docs/{concepts/frontend/static_assets.rst => decisions/0000-static-asset-plan.rst} (90%) diff --git a/docs/concepts/frontend/static_assets.rst b/docs/decisions/0000-static-asset-plan.rst similarity index 90% rename from docs/concepts/frontend/static_assets.rst rename to docs/decisions/0000-static-asset-plan.rst index 89e7a64f445..00fcc0726ad 100644 --- a/docs/concepts/frontend/static_assets.rst +++ b/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. From 29d4c69e5716168cfc23c2230f77de083eee6199 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Fri, 13 Dec 2024 15:46:39 -0500 Subject: [PATCH 4/8] build!: Rejigger "npm run test" to be more intuitive This is technically a breaking change, but these commands were added a week ago and were not yet documented, so I'm not worried about breaking anyone's workflow with this commit. --- package.json | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/package.json b/package.json index 2f09f8a7df9..8ed93a32263 100644 --- a/package.json +++ b/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", From 54250632d8b83feb5613f8e8920cc229390d2309 Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Wed, 18 Dec 2024 14:09:43 -0500 Subject: [PATCH 5/8] docs: ADR for Fixing Quality and JS Checks (#35741) This is a record of the various existing CI issues we faced and the trade-offs we made to move forward while replacing the Paver CI commands as part of: https://github.com/openedx/edx-platform/issues/34467 --- .../0021-fixing-quality-and-js-checks.rst | 143 ++++++++++++++++++ 1 file changed, 143 insertions(+) create mode 100644 docs/decisions/0021-fixing-quality-and-js-checks.rst diff --git a/docs/decisions/0021-fixing-quality-and-js-checks.rst b/docs/decisions/0021-fixing-quality-and-js-checks.rst new file mode 100644 index 00000000000..7d80039a8cb --- /dev/null +++ b/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 From 930989e5e6dbd2c5253ec3d05652e55c43379725 Mon Sep 17 00:00:00 2001 From: Muhammad Adeel Tajamul <77053848+muhammadadeeltajamul@users.noreply.github.com> Date: Thu, 19 Dec 2024 16:57:27 +0500 Subject: [PATCH 6/8] fix: fixed notification generated event for grouped notifications (#36048) --- openedx/core/djangoapps/notifications/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/notifications/tasks.py b/openedx/core/djangoapps/notifications/tasks.py index 75ad3f1ecd0..ddd690cf996 100644 --- a/openedx/core/djangoapps/notifications/tasks.py +++ b/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) From 85a5890dd1f14a585b024da03954dca6f7a95aa9 Mon Sep 17 00:00:00 2001 From: Ahtisham Shahid Date: Thu, 19 Dec 2024 19:04:16 +0500 Subject: [PATCH 7/8] feat: added api to update all notification preferences for user (#35795) --- .../djangoapps/notifications/email/utils.py | 22 +- .../djangoapps/notifications/serializers.py | 116 ++++++- .../notifications/tests/test_utils.py | 288 ++++++++++++++++ .../notifications/tests/test_views.py | 315 +++++++++++++++++- openedx/core/djangoapps/notifications/urls.py | 14 +- .../core/djangoapps/notifications/utils.py | 115 ++++++- .../core/djangoapps/notifications/views.py | 155 ++++++++- 7 files changed, 1001 insertions(+), 24 deletions(-) create mode 100644 openedx/core/djangoapps/notifications/tests/test_utils.py diff --git a/openedx/core/djangoapps/notifications/email/utils.py b/openedx/core/djangoapps/notifications/email/utils.py index 9fd761785e5..34c24530878 100644 --- a/openedx/core/djangoapps/notifications/email/utils.py +++ b/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'] diff --git a/openedx/core/djangoapps/notifications/serializers.py b/openedx/core/djangoapps/notifications/serializers.py index 79c8c4af9d1..80b1577b635 100644 --- a/openedx/core/djangoapps/notifications/serializers.py +++ b/openedx/core/djangoapps/notifications/serializers.py @@ -1,6 +1,7 @@ """ Serializers for the notifications API. """ + from django.core.exceptions import ValidationError from rest_framework import serializers @@ -9,9 +10,12 @@ from openedx.core.djangoapps.notifications.models import ( CourseNotificationPreference, Notification, - get_notification_channels, get_additional_notification_channel_settings + get_additional_notification_channel_settings, + get_notification_channels ) + from .base_notification import COURSE_NOTIFICATION_APPS, COURSE_NOTIFICATION_TYPES, EmailCadence +from .email.utils import is_notification_type_channel_editable from .utils import remove_preferences_with_no_access @@ -202,3 +206,113 @@ class Meta: 'last_seen', 'created', ) + + +def validate_email_cadence(email_cadence: str) -> str: + """ + Validate email cadence value. + """ + if EmailCadence.get_email_cadence_value(email_cadence) is None: + raise ValidationError(f'{email_cadence} is not a valid email cadence.') + return email_cadence + + +def validate_notification_app(notification_app: str) -> str: + """ + Validate notification app value. + """ + if not COURSE_NOTIFICATION_APPS.get(notification_app): + raise ValidationError(f'{notification_app} is not a valid notification app.') + return notification_app + + +def validate_notification_app_enabled(notification_app: str) -> str: + """ + Validate notification app is enabled. + """ + + if COURSE_NOTIFICATION_APPS.get(notification_app) and COURSE_NOTIFICATION_APPS.get(notification_app)['enabled']: + return notification_app + raise ValidationError(f'{notification_app} is not a valid notification app.') + + +def validate_notification_type(notification_type: str) -> str: + """ + Validate notification type value. + """ + if not COURSE_NOTIFICATION_TYPES.get(notification_type): + raise ValidationError(f'{notification_type} is not a valid notification type.') + return notification_type + + +def validate_notification_channel(notification_channel: str) -> str: + """ + Validate notification channel value. + """ + valid_channels = set(get_notification_channels()) | set(get_additional_notification_channel_settings()) + if notification_channel not in valid_channels: + raise ValidationError(f'{notification_channel} is not a valid notification channel setting.') + return notification_channel + + +class UserNotificationPreferenceUpdateAllSerializer(serializers.Serializer): + """ + Serializer for user notification preferences update with custom field validators. + """ + notification_app = serializers.CharField( + required=True, + validators=[validate_notification_app, validate_notification_app_enabled] + ) + value = serializers.BooleanField(required=False) + notification_type = serializers.CharField( + required=True, + ) + notification_channel = serializers.CharField( + required=False, + validators=[validate_notification_channel] + ) + email_cadence = serializers.CharField( + required=False, + validators=[validate_email_cadence] + ) + + def validate(self, attrs): + """ + Cross-field validation for notification preference update. + """ + notification_app = attrs.get('notification_app') + notification_type = attrs.get('notification_type') + notification_channel = attrs.get('notification_channel') + email_cadence = attrs.get('email_cadence') + + # Validate email_cadence requirements + if email_cadence and not notification_type: + raise ValidationError({ + 'notification_type': 'notification_type is required for email_cadence.' + }) + + # Validate notification_channel requirements + if not email_cadence and notification_type and not notification_channel: + raise ValidationError({ + 'notification_channel': 'notification_channel is required for notification_type.' + }) + + # Validate notification type + if all([not COURSE_NOTIFICATION_TYPES.get(notification_type), notification_type != "core"]): + raise ValidationError(f'{notification_type} is not a valid notification type.') + + # Validate notification type and channel is editable + if notification_channel and notification_type: + if not is_notification_type_channel_editable( + notification_app, + notification_type, + notification_channel + ): + raise ValidationError({ + 'notification_channel': ( + f'{notification_channel} is not editable for notification type ' + f'{notification_type}.' + ) + }) + + return attrs diff --git a/openedx/core/djangoapps/notifications/tests/test_utils.py b/openedx/core/djangoapps/notifications/tests/test_utils.py new file mode 100644 index 00000000000..e300c63c1d7 --- /dev/null +++ b/openedx/core/djangoapps/notifications/tests/test_utils.py @@ -0,0 +1,288 @@ +""" +Test cases for the notification utility functions. +""" +import unittest + +from openedx.core.djangoapps.notifications.utils import aggregate_notification_configs + + +class TestAggregateNotificationConfigs(unittest.TestCase): + """ + Test cases for the aggregate_notification_configs function. + """ + + def test_empty_configs_list_returns_default(self): + """ + If the configs list is empty, the default config should be returned. + """ + default_config = [{ + "grading": { + "enabled": False, + "non_editable": {}, + "notification_types": { + "core": { + "web": False, + "push": False, + "email": False, + "email_cadence": "Daily" + } + } + } + }] + + result = aggregate_notification_configs(default_config) + assert result == default_config[0] + + def test_enable_notification_type(self): + """ + If a config enables a notification type, it should be enabled in the result. + """ + + config_list = [ + { + "grading": { + "enabled": False, + "non_editable": {}, + "notification_types": { + "core": { + "web": False, + "push": False, + "email": False, + "email_cadence": "Weekly" + } + } + } + }, + { + "grading": { + "enabled": True, + "notification_types": { + "core": { + "web": True, + "push": True, + "email": True, + "email_cadence": "Weekly" + } + } + } + }] + + result = aggregate_notification_configs(config_list) + assert result["grading"]["enabled"] is True + assert result["grading"]["notification_types"]["core"]["web"] is True + assert result["grading"]["notification_types"]["core"]["push"] is True + assert result["grading"]["notification_types"]["core"]["email"] is True + # Use default email_cadence + assert result["grading"]["notification_types"]["core"]["email_cadence"] == "Weekly" + + def test_merge_core_notification_types(self): + """ + Core notification types should be merged across configs. + """ + + config_list = [ + { + "discussion": { + "enabled": True, + "core_notification_types": ["new_comment"], + "notification_types": {} + } + }, + { + "discussion": { + "core_notification_types": ["new_response", "new_comment"] + } + + }] + + result = aggregate_notification_configs(config_list) + assert set(result["discussion"]["core_notification_types"]) == { + "new_comment", "new_response" + } + + def test_multiple_configs_aggregate(self): + """ + Multiple configs should be aggregated together. + """ + + config_list = [ + { + "updates": { + "enabled": False, + "notification_types": { + "course_updates": { + "web": False, + "push": False, + "email": False, + "email_cadence": "Weekly" + } + } + } + }, + { + "updates": { + "enabled": True, + "notification_types": { + "course_updates": { + "web": True, + "email_cadence": "Weekly" + } + } + } + }, + { + "updates": { + "notification_types": { + "course_updates": { + "push": True, + "email_cadence": "Weekly" + } + } + } + } + ] + + result = aggregate_notification_configs(config_list) + assert result["updates"]["enabled"] is True + assert result["updates"]["notification_types"]["course_updates"]["web"] is True + assert result["updates"]["notification_types"]["course_updates"]["push"] is True + assert result["updates"]["notification_types"]["course_updates"]["email"] is False + # Use default email_cadence + assert result["updates"]["notification_types"]["course_updates"]["email_cadence"] == "Weekly" + + def test_ignore_unknown_notification_types(self): + """ + Unknown notification types should be ignored. + """ + config_list = [ + { + "grading": { + "enabled": False, + "notification_types": { + "core": { + "web": False, + "push": False, + "email": False, + "email_cadence": "Daily" + } + } + } + }, + { + "grading": { + "notification_types": { + "unknown_type": { + "web": True, + "push": True, + "email": True + } + } + } + }] + + result = aggregate_notification_configs(config_list) + assert "unknown_type" not in result["grading"]["notification_types"] + assert result["grading"]["notification_types"]["core"]["web"] is False + + def test_ignore_unknown_categories(self): + """ + Unknown categories should be ignored. + """ + + config_list = [ + { + "grading": { + "enabled": False, + "notification_types": {} + } + }, + { + "unknown_category": { + "enabled": True, + "notification_types": {} + } + }] + + result = aggregate_notification_configs(config_list) + assert "unknown_category" not in result + assert result["grading"]["enabled"] is False + + def test_preserves_default_structure(self): + """ + The resulting config should have the same structure as the default config. + """ + + config_list = [ + { + "discussion": { + "enabled": False, + "non_editable": {"core": ["web"]}, + "notification_types": { + "core": { + "web": False, + "push": False, + "email": False, + "email_cadence": "Weekly" + } + }, + "core_notification_types": [] + } + }, + { + "discussion": { + "enabled": True, + "extra_field": "should_not_appear" + } + } + ] + + result = aggregate_notification_configs(config_list) + assert set(result["discussion"].keys()) == { + "enabled", "non_editable", "notification_types", "core_notification_types" + } + assert "extra_field" not in result["discussion"] + + def test_if_email_cadence_has_diff_set_mix_as_value(self): + """ + If email_cadence is different in the configs, set it to "Mixed". + """ + config_list = [ + { + "grading": { + "enabled": False, + "notification_types": { + "core": { + "web": False, + "push": False, + "email": False, + "email_cadence": "Daily" + } + } + } + }, + { + "grading": { + "enabled": True, + "notification_types": { + "core": { + "web": True, + "push": True, + "email": True, + "email_cadence": "Weekly" + } + } + } + }, + { + "grading": { + "notification_types": { + "core": { + "email_cadence": "Monthly" + } + } + } + } + ] + + result = aggregate_notification_configs(config_list) + assert result["grading"]["notification_types"]["core"]["email_cadence"] == "Mixed" diff --git a/openedx/core/djangoapps/notifications/tests/test_views.py b/openedx/core/djangoapps/notifications/tests/test_views.py index 70e6fbc5739..3e73c95be5b 100644 --- a/openedx/core/djangoapps/notifications/tests/test_views.py +++ b/openedx/core/djangoapps/notifications/tests/test_views.py @@ -2,11 +2,14 @@ Tests for the views in the notifications app. """ import json +from copy import deepcopy from datetime import datetime, timedelta from unittest import mock +from unittest.mock import patch import ddt from django.conf import settings +from django.contrib.auth import get_user_model from django.test.utils import override_settings from django.urls import reverse from edx_toggles.toggles.testutils import override_waffle_flag @@ -27,19 +30,21 @@ FORUM_ROLE_MODERATOR ) from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS +from openedx.core.djangoapps.notifications.email.utils import encrypt_object, encrypt_string from openedx.core.djangoapps.notifications.models import ( CourseNotificationPreference, Notification, get_course_notification_preference_config_version ) from openedx.core.djangoapps.notifications.serializers import NotificationCourseEnrollmentSerializer -from openedx.core.djangoapps.notifications.email.utils import encrypt_object, encrypt_string from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from ..base_notification import COURSE_NOTIFICATION_APPS, COURSE_NOTIFICATION_TYPES, NotificationAppManager from ..utils import get_notification_types_with_visibility_settings +User = get_user_model() + @ddt.ddt class CourseEnrollmentListViewTest(ModuleStoreTestCase): @@ -903,6 +908,7 @@ class UpdatePreferenceFromEncryptedDataView(ModuleStoreTestCase): """ Tests if preference is updated when encrypted url is hit """ + def setUp(self): """ Setup test case @@ -968,3 +974,310 @@ def remove_notifications_with_visibility_settings(expected_response): notification_type ) return expected_response + + +class UpdateAllNotificationPreferencesViewTests(APITestCase): + """ + Tests for the UpdateAllNotificationPreferencesView. + """ + + def setUp(self): + # Create test user + self.user = User.objects.create_user( + username='testuser', + password='testpass123' + ) + self.client = APIClient() + self.client.force_authenticate(user=self.user) + self.url = reverse('update-all-notification-preferences') + + # Complex notification config structure + self.base_config = { + "grading": { + "enabled": True, + "non_editable": {}, + "notification_types": { + "core": { + "web": True, + "push": True, + "email": True, + "email_cadence": "Daily" + }, + "ora_staff_notification": { + "web": False, + "push": False, + "email": False, + "email_cadence": "Daily" + } + }, + "core_notification_types": [] + }, + "updates": { + "enabled": True, + "non_editable": {}, + "notification_types": { + "core": { + "web": True, + "push": True, + "email": True, + "email_cadence": "Daily" + }, + "course_updates": { + "web": True, + "push": True, + "email": False, + "email_cadence": "Daily" + } + }, + "core_notification_types": [] + }, + "discussion": { + "enabled": True, + "non_editable": { + "core": ["web"] + }, + "notification_types": { + "core": { + "web": True, + "push": True, + "email": True, + "email_cadence": "Daily" + }, + "content_reported": { + "web": True, + "push": True, + "email": True, + "email_cadence": "Daily" + }, + "new_question_post": { + "web": True, + "push": False, + "email": False, + "email_cadence": "Daily" + }, + "new_discussion_post": { + "web": True, + "push": False, + "email": False, + "email_cadence": "Daily" + } + }, + "core_notification_types": [ + "new_comment_on_response", + "new_comment", + "new_response", + "response_on_followed_post", + "comment_on_followed_post", + "response_endorsed_on_thread", + "response_endorsed" + ] + } + } + + # Create test notification preferences + self.preferences = [] + for i in range(3): + pref = CourseNotificationPreference.objects.create( + user=self.user, + course_id=f'course-v1:TestX+Test{i}+2024', + notification_preference_config=deepcopy(self.base_config), + is_active=True + ) + self.preferences.append(pref) + + # Create an inactive preference + self.inactive_pref = CourseNotificationPreference.objects.create( + user=self.user, + course_id='course-v1:TestX+Inactive+2024', + notification_preference_config=deepcopy(self.base_config), + is_active=False + ) + + def test_update_discussion_notification(self): + """ + Test updating discussion notification settings + """ + data = { + 'notification_app': 'discussion', + 'notification_type': 'content_reported', + 'notification_channel': 'push', + 'value': False + } + + response = self.client.post(self.url, data, format='json') + + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data['status'], 'success') + self.assertEqual(response.data['data']['total_updated'], 3) + + # Verify database updates + for pref in CourseNotificationPreference.objects.filter(is_active=True): + self.assertFalse( + pref.notification_preference_config['discussion']['notification_types']['content_reported']['push'] + ) + + def test_update_non_editable_field(self): + """ + Test attempting to update a non-editable field + """ + data = { + 'notification_app': 'discussion', + 'notification_type': 'core', + 'notification_channel': 'web', + 'value': False + } + + response = self.client.post(self.url, data, format='json') + + # Should fail because 'web' is non-editable for 'core' in discussion + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.data['status'], 'error') + + # Verify database remains unchanged + for pref in CourseNotificationPreference.objects.filter(is_active=True): + self.assertTrue( + pref.notification_preference_config['discussion']['notification_types']['core']['web'] + ) + + def test_update_email_cadence(self): + """ + Test updating email cadence setting + """ + data = { + 'notification_app': 'discussion', + 'notification_type': 'content_reported', + 'email_cadence': 'Weekly' + } + + response = self.client.post(self.url, data, format='json') + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data['status'], 'success') + + # Verify database updates + for pref in CourseNotificationPreference.objects.filter(is_active=True): + notification_type = pref.notification_preference_config['discussion']['notification_types'][ + 'content_reported'] + self.assertEqual( + notification_type['email_cadence'], + 'Weekly' + ) + + @patch.dict('openedx.core.djangoapps.notifications.serializers.COURSE_NOTIFICATION_APPS', { + **COURSE_NOTIFICATION_APPS, + 'grading': { + 'enabled': False, + 'core_info': 'Notifications for submission grading.', + 'core_web': True, + 'core_email': True, + 'core_push': True, + 'core_email_cadence': 'Daily', + 'non_editable': [] + } + }) + def test_update_disabled_app(self): + """ + Test updating notification for a disabled app + """ + # Disable the grading app in all preferences + for pref in self.preferences: + config = pref.notification_preference_config + config['grading']['enabled'] = False + pref.notification_preference_config = config + pref.save() + + data = { + 'notification_app': 'grading', + 'notification_type': 'core', + 'notification_channel': 'email', + 'value': False + } + response = self.client.post(self.url, data, format='json') + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(response.data['status'], 'error') + + def test_invalid_serializer_data(self): + """ + Test handling of invalid input data + """ + test_cases = [ + { + 'notification_app': 'invalid_app', + 'notification_type': 'core', + 'notification_channel': 'push', + 'value': False + }, + { + 'notification_app': 'discussion', + 'notification_type': 'invalid_type', + 'notification_channel': 'push', + 'value': False + }, + { + 'notification_app': 'discussion', + 'notification_type': 'core', + 'notification_channel': 'invalid_channel', + 'value': False + }, + { + 'notification_app': 'discussion', + 'notification_type': 'core', + 'notification_channel': 'email_cadence', + 'value': 'Invalid_Cadence' + } + ] + + for test_case in test_cases: + response = self.client.post(self.url, test_case, format='json') + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + +class GetAggregateNotificationPreferencesTest(APITestCase): + """ + Tests for the GetAggregateNotificationPreferences API view. + """ + + def setUp(self): + # Set up a user and API client + self.user = User.objects.create_user(username='testuser', password='testpass') + self.client = APIClient() + self.client.force_authenticate(user=self.user) + self.url = reverse('notification-preferences-aggregated') # Adjust with the actual name + + def test_no_active_notification_preferences(self): + """ + Test case: No active notification preferences found for the user + """ + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.data['status'], 'error') + self.assertEqual(response.data['message'], 'No active notification preferences found') + + @patch('openedx.core.djangoapps.notifications.views.aggregate_notification_configs') + def test_with_active_notification_preferences(self, mock_aggregate): + """ + Test case: Active notification preferences found for the user + """ + # Mock aggregate_notification_configs for a controlled output + mock_aggregate.return_value = {'mocked': 'data'} + + # Create active notification preferences for the user + CourseNotificationPreference.objects.create( + user=self.user, + is_active=True, + notification_preference_config={'example': 'config'} + ) + + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data['status'], 'success') + self.assertEqual(response.data['message'], 'Notification preferences retrieved') + self.assertEqual(response.data['data'], {'mocked': 'data'}) + + def test_unauthenticated_user(self): + """ + Test case: Request without authentication + """ + # Test case: Request without authentication + self.client.logout() # Remove authentication + response = self.client.get(self.url) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) diff --git a/openedx/core/djangoapps/notifications/urls.py b/openedx/core/djangoapps/notifications/urls.py index 7f611bc2c4c..9892fa72de0 100644 --- a/openedx/core/djangoapps/notifications/urls.py +++ b/openedx/core/djangoapps/notifications/urls.py @@ -11,13 +11,13 @@ NotificationCountView, NotificationListAPIView, NotificationReadAPIView, + UpdateAllNotificationPreferencesView, UserNotificationPreferenceView, - preference_update_from_encrypted_username_view, + preference_update_from_encrypted_username_view, AggregatedNotificationPreferences ) router = routers.DefaultRouter() - urlpatterns = [ path('enrollments/', CourseEnrollmentListView.as_view(), name='enrollment-list'), re_path( @@ -25,6 +25,11 @@ UserNotificationPreferenceView.as_view(), name='notification-preferences' ), + path( + 'configurations/', + AggregatedNotificationPreferences.as_view(), + name='notification-preferences-aggregated' + ), path('', NotificationListAPIView.as_view(), name='notifications-list'), path('count/', NotificationCountView.as_view(), name='notifications-count'), path( @@ -35,6 +40,11 @@ path('read/', NotificationReadAPIView.as_view(), name='notifications-read'), path('preferences/update///', preference_update_from_encrypted_username_view, name='preference_update_from_encrypted_username_view'), + path( + 'preferences/update-all/', + UpdateAllNotificationPreferencesView.as_view(), + name='update-all-notification-preferences' + ), ] urlpatterns += router.urls diff --git a/openedx/core/djangoapps/notifications/utils.py b/openedx/core/djangoapps/notifications/utils.py index fa948dcf425..c05837b5f15 100644 --- a/openedx/core/djangoapps/notifications/utils.py +++ b/openedx/core/djangoapps/notifications/utils.py @@ -1,11 +1,12 @@ """ Utils function for notifications app """ -from typing import Dict, List +import copy +from typing import Dict, List, Set from common.djangoapps.student.models import CourseAccessRole, CourseEnrollment from openedx.core.djangoapps.django_comment_common.models import Role -from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NOTIFICATIONS, ENABLE_NEW_NOTIFICATION_VIEW +from openedx.core.djangoapps.notifications.config.waffle import ENABLE_NEW_NOTIFICATION_VIEW, ENABLE_NOTIFICATIONS from openedx.core.lib.cache_utils import request_cached @@ -158,3 +159,113 @@ def clean_arguments(kwargs): if kwargs.get('created', {}): clean_kwargs.update(kwargs.get('created')) return clean_kwargs + + +def update_notification_types( + app_config: Dict, + user_app_config: Dict, +) -> None: + """ + Update notification types for a specific category configuration. + """ + if "notification_types" not in user_app_config: + return + + for type_key, type_config in user_app_config["notification_types"].items(): + if type_key not in app_config["notification_types"]: + continue + + update_notification_fields( + app_config["notification_types"][type_key], + type_config, + ) + + +def update_notification_fields( + target_config: Dict, + source_config: Dict, +) -> None: + """ + Update individual notification fields (web, push, email) and email_cadence. + """ + for field in ["web", "push", "email"]: + if field in source_config: + target_config[field] |= source_config[field] + if "email_cadence" in source_config: + if isinstance(target_config["email_cadence"], str) or not target_config["email_cadence"]: + target_config["email_cadence"] = set() + + target_config["email_cadence"].add(source_config["email_cadence"]) + + +def update_core_notification_types(app_config: Dict, user_config: Dict) -> None: + """ + Update core notification types by merging existing and new types. + """ + if "core_notification_types" not in user_config: + return + + existing_types: Set = set(app_config.get("core_notification_types", [])) + existing_types.update(user_config["core_notification_types"]) + app_config["core_notification_types"] = list(existing_types) + + +def process_app_config( + app_config: Dict, + user_config: Dict, + app: str, + default_config: Dict, +) -> None: + """ + Process a single category configuration against another config. + """ + if app not in user_config: + return + + user_app_config = user_config[app] + + # Update enabled status + app_config["enabled"] |= user_app_config.get("enabled", False) + + # Update core notification types + update_core_notification_types(app_config, user_app_config) + + # Update notification types + update_notification_types(app_config, user_app_config) + + +def aggregate_notification_configs(existing_user_configs: List[Dict]) -> Dict: + """ + Update default notification config with values from other configs. + Rules: + 1. Start with default config as base + 2. If any value is True in other configs, make it True + 3. Set email_cadence to "Mixed" if different cadences found, else use default + + Args: + existing_user_configs: List of notification config dictionaries to apply + + Returns: + Updated config following the same structure + """ + if not existing_user_configs: + return {} + + result_config = copy.deepcopy(existing_user_configs[0]) + apps = result_config.keys() + + for app in apps: + app_config = result_config[app] + + for user_config in existing_user_configs: + process_app_config(app_config, user_config, app, existing_user_configs[0]) + + # if email_cadence is mixed, set it to "Mixed" + for app in result_config: + for type_key, type_config in result_config[app]["notification_types"].items(): + if len(type_config["email_cadence"]) > 1: + result_config[app]["notification_types"][type_key]["email_cadence"] = "Mixed" + else: + result_config[app]["notification_types"][type_key]["email_cadence"] = ( + result_config[app]["notification_types"][type_key]["email_cadence"].pop()) + return result_config diff --git a/openedx/core/djangoapps/notifications/views.py b/openedx/core/djangoapps/notifications/views.py index e87274088f8..8e41b11554c 100644 --- a/openedx/core/djangoapps/notifications/views.py +++ b/openedx/core/djangoapps/notifications/views.py @@ -1,9 +1,11 @@ """ Views for the notifications API. """ +import copy from datetime import datetime, timedelta from django.conf import settings +from django.db import transaction from django.db.models import Count from django.shortcuts import get_object_or_404 from django.utils.translation import gettext as _ @@ -17,10 +19,7 @@ from common.djangoapps.student.models import CourseEnrollment from openedx.core.djangoapps.notifications.email.utils import update_user_preferences_from_patch -from openedx.core.djangoapps.notifications.models import ( - CourseNotificationPreference, - get_course_notification_preference_config_version -) +from openedx.core.djangoapps.notifications.models import get_course_notification_preference_config_version from openedx.core.djangoapps.notifications.permissions import allow_any_authenticated_user from .base_notification import COURSE_NOTIFICATION_APPS @@ -32,14 +31,15 @@ notification_tray_opened_event, notifications_app_all_read_event ) -from .models import Notification +from .models import CourseNotificationPreference, Notification from .serializers import ( NotificationCourseEnrollmentSerializer, NotificationSerializer, UserCourseNotificationPreferenceSerializer, - UserNotificationPreferenceUpdateSerializer, + UserNotificationPreferenceUpdateAllSerializer, + UserNotificationPreferenceUpdateSerializer ) -from .utils import get_show_notifications_tray, get_is_new_notification_view_enabled +from .utils import get_is_new_notification_view_enabled, get_show_notifications_tray, aggregate_notification_configs @allow_any_authenticated_user() @@ -444,3 +444,144 @@ def preference_update_from_encrypted_username_view(request, username, patch): """ update_user_preferences_from_patch(username, patch) return Response({"result": "success"}, status=status.HTTP_200_OK) + + +@allow_any_authenticated_user() +class UpdateAllNotificationPreferencesView(APIView): + """ + API view for updating all notification preferences for the current user. + """ + + def post(self, request): + """ + Update all notification preferences for the current user. + """ + # check if request have required params + serializer = UserNotificationPreferenceUpdateAllSerializer(data=request.data) + if not serializer.is_valid(): + return Response({ + 'status': 'error', + 'message': serializer.errors + }, status=status.HTTP_400_BAD_REQUEST) + # check if required config is not editable + try: + with transaction.atomic(): + # Get all active notification preferences for the current user + notification_preferences = ( + CourseNotificationPreference.objects + .select_for_update() + .filter( + user=request.user, + is_active=True + ) + ) + + if not notification_preferences.exists(): + return Response({ + 'status': 'error', + 'message': 'No active notification preferences found' + }, status=status.HTTP_404_NOT_FOUND) + + data = serializer.validated_data + app = data['notification_app'] + email_cadence = data.get('email_cadence', None) + channel = data.get('notification_channel', 'email_cadence' if email_cadence else None) + notification_type = data['notification_type'] + value = data.get('value', email_cadence if email_cadence else None) + + updated_courses = [] + errors = [] + + # Update each preference + for preference in notification_preferences: + try: + # Create a deep copy of the current config + updated_config = copy.deepcopy(preference.notification_preference_config) + + # Check if the path exists and update the value + if ( + updated_config.get(app, {}) + .get('notification_types', {}) + .get(notification_type, {}) + .get(channel) + ) is not None: + + # Update the specific setting in the config + updated_config[app]['notification_types'][notification_type][channel] = value + + # Update the notification preference + preference.notification_preference_config = updated_config + preference.save() + + updated_courses.append({ + 'course_id': str(preference.course_id), + 'current_setting': updated_config[app]['notification_types'][notification_type] + }) + else: + errors.append({ + 'course_id': str(preference.course_id), + 'error': f'Invalid path: {app}.notification_types.{notification_type}.{channel}' + }) + + except Exception as e: + errors.append({ + 'course_id': str(preference.course_id), + 'error': str(e) + }) + + response_data = { + 'status': 'success' if updated_courses else 'partial_success' if errors else 'error', + 'message': 'Notification preferences update completed', + 'data': { + 'updated_value': value, + 'notification_type': notification_type, + 'channel': channel, + 'app': app, + 'successfully_updated_courses': updated_courses, + 'total_updated': len(updated_courses), + 'total_courses': notification_preferences.count() + } + } + + if errors: + response_data['errors'] = errors + + return Response( + response_data, + status=status.HTTP_200_OK if updated_courses else status.HTTP_400_BAD_REQUEST + ) + + except Exception as e: + return Response({ + 'status': 'error', + 'message': str(e) + }, status=status.HTTP_500_INTERNAL_SERVER_ERROR) + + +@allow_any_authenticated_user() +class AggregatedNotificationPreferences(APIView): + """ + API view for getting the aggregate notification preferences for the current user. + """ + + def get(self, request): + """ + API view for getting the aggregate notification preferences for the current user. + """ + notification_preferences = CourseNotificationPreference.objects.filter(user=request.user, is_active=True) + + if not notification_preferences.exists(): + return Response({ + 'status': 'error', + 'message': 'No active notification preferences found' + }, status=status.HTTP_404_NOT_FOUND) + notification_configs = notification_preferences.values_list('notification_preference_config', flat=True) + notification_configs = aggregate_notification_configs( + notification_configs + ) + + return Response({ + 'status': 'success', + 'message': 'Notification preferences retrieved', + 'data': notification_configs + }, status=status.HTTP_200_OK) From b391fb54002d29dd53d2b48ef6c758ff96efa998 Mon Sep 17 00:00:00 2001 From: "Kyle D. McCormick" Date: Thu, 19 Dec 2024 14:16:57 -0500 Subject: [PATCH 8/8] build: unpin django-stubs now that we're on django >=4.2 --- requirements/constraints.txt | 7 ------- 1 file changed, 7 deletions(-) diff --git a/requirements/constraints.txt b/requirements/constraints.txt index e65e19a574f..78a5f83a787 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -60,13 +60,6 @@ django-webpack-loader==0.7.0 # Adding pin to avoid any major upgrade djangorestframework<3.15.0 -# Date: 2023-07-19 -# The version of django-stubs we can use depends on which Django release we're using -# 1.16.0 works with Django 3.2 through 4.1 -# Issue for unpinning: https://github.com/openedx/edx-platform/issues/35275 -django-stubs==1.16.0 -djangorestframework-stubs==3.14.0 # Pinned to match django-stubs. Remove this when we can remove the above pin. - # Date: 2024-07-23 # django-storages==1.14.4 breaks course imports # Two lines were added in 1.14.4 that make file_exists_in_storage function always return False,