You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
refactor: combine authoring apps to openedx_content
This commit makes the following major refactorings:
- All apps previously under openedx_learning/apps/authoring have now
effectively been merged together to one openedx_content app.
- The models and logic for those apps continues to be encapsulated in
what we're calling "applets", inside of openedx_content/applets/...
- In order to facilitate smooth database migrations, the AppConfigs and
database migrations for the old apps continue to be preserved in
openedx_content/backcompat/...
- A new openedx_learning.api.django.openedx_learning_apps_to_install()
has been created to make it easier for openedx-platform to get all the
necessary apps to install.
The rationale for this change is detailed in
docs/decisions/0020-authoring-as-one-app.rst, but the short version is
that we hope this arrangement will allow us to keep many of the benefits
of having small apps (easy to reason about), while also making it easier
to refactor internally. Refactoring is currently hindered by the
difficulty in moving models across apps.
The name change away from "authoring" also reflects that we intend to
use these APIs and models on the "learning" side of things as well, e.g.
when reading content for rendering to a student. In the longer term, we
will probably make openedx_content a top-level package in this repo, but
that would be a later step.
Most of this commit is just shuffling things around and renaming
references, but the truly complicated bits are around the sequencing of
database migrations, particularly the ones removing model state from the
old apps, and transferring that state into the new openedx_content app
without changing the actual database state. We also have some hacky
logic in openedx_content/apps.py in order to properly patch migration
dependencies so that we don't break existing openedx-platform
migrations.
This also bumps the version to 0.31.0.
Copy file name to clipboardExpand all lines: docs/decisions/0016-python-public-api-conventions.rst
+5-5Lines changed: 5 additions & 5 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -17,10 +17,10 @@ Decision
17
17
In addition to OEP-49, we will adopt the following practices:
18
18
19
19
Learning Core Django apps will be grouped into packages.
20
-
Apps in ``openedx_learning`` will be grouped into broadly related packages under ``openedx_learning.apps``. The first of these groups will be "authoring" (``openedx_learning.apps.authoring``). Future packages may include "learner", "personalization", "activity", "grading", etc.
20
+
Apps in ``openedx_learning`` will be grouped into broadly related packages under ``openedx_learning.apps``. The first of these groups will be "authoring" (``openedx_learning.apps.openedx_content``). Future packages may include "learner", "personalization", "activity", "grading", etc.
21
21
22
22
Learning Core Django apps will continue to have their own ``api`` modules.
23
-
So for example, ``openedx_learning.apps.authoring.components.api`` will continue to exist.
23
+
So for example, ``openedx_learning.apps.openedx_content.applets.components.api`` will continue to exist.
24
24
25
25
Learning Core will have a top level package for its public API.
26
26
All public APIs intended for use by consumers of Learning Core will be represented as modules in the ``openedx_learning.api`` package that corresponds to the app groupings (e.g. ``openedx_learning.api.authoring``).
@@ -35,14 +35,14 @@ App ``api`` modules will define their public functions using ``__all__``.
35
35
This relies on the individual apps to properly set ``__all__`` to the list of functions that they are willing to publicly support.
36
36
37
37
App ``api`` modules within a package of apps still import from each other.
38
-
So for example, ``openedx_learning.apps.authoring.components.api`` will continue to import APIs that it needs from ``..publishing.api``, instead of using the public API at ``openedx_learning.api.authoring``. These imports should not use wildcards.
38
+
So for example, ``openedx_learning.apps.openedx_content.applets.components.api`` will continue to import APIs that it needs from ``..publishing.api``, instead of using the public API at ``openedx_learning.api.authoring``. These imports should not use wildcards.
39
39
40
40
Functions and constants that are not listed as part of a module's ``__all__`` may still be imported by other app APIs in the same package grouping. This should allow a package more flexibility to create provisional APIs that we may not want to support publicly.
41
41
42
42
If a function or attribute is intended to be completely private to an app's ``api`` module (i.e. not used even by other apps in its package), it should be prefixed with an underscore.
43
43
44
44
App ``api`` modules should not import directly from apps outside their package.
45
-
For example, ``openedx_learning.apps.personalization.api`` should import authoring API functions from ``openedx_learning.api.authoring``, **not** directly from something like ``openedx_learning.apps.authoring.components.api``. This will help to limit the impact of refactoring app package internal changes, as well as exposing shortcomings in the existing public APIs.
45
+
For example, ``openedx_learning.apps.personalization.api`` should import authoring API functions from ``openedx_learning.api.authoring``, **not** directly from something like ``openedx_learning.apps.openedx_content.applets.components.api``. This will help to limit the impact of refactoring app package internal changes, as well as exposing shortcomings in the existing public APIs.
46
46
47
47
Public API modules may implement their own functions.
48
48
In addition to aggregating app ``api`` modules via wildcard imports, public API modules like ``openedx_learning.api.authoring`` may implement their own functionality. This will be useful for convenience functions that invoke multiple app APIs, and for backwards compatibility shims. When possible, the bulk of the logic for these should continue to live in app-defined APIs, with the public API module acting more as a glue layer.
@@ -54,7 +54,7 @@ Rejected Alternatives
54
54
55
55
Public APIs in each app package
56
56
57
-
We could have added these aggregations as ``api`` modules in each app group package, e.g. ``openedx_learning.apps.authoring.api``. We are not doing this because:
57
+
We could have added these aggregations as ``api`` modules in each app group package, e.g. ``openedx_learning.apps.openedx_content.api``. We are not doing this because:
58
58
59
59
* It's more convenient for browsing and documentation generation to have the public API modules in the same package.
60
60
* It's more idiomatic for Python libraries to expose their APIs in appropriately named modules (like ``authoring``), rather than all imported modules being named ``api``.
Up to this point, Learning Core has used many small apps with a narrow focus (e.g. ``components``, ``collections``, etc.) in order to make each individual app simpler to reason about. This has been useful overall, but it has made refactoring more cumbersome. For instance:
8
+
9
+
#. Moving models between apps is tricky, requiring the use of Django's ``SeparateDatabaseAndState`` functionality to fake a deletion in one app and a creation in another without actually altering the database. It also requires doctoring the migration files for models in other repos that might have foreign key relations to the model being moved, so that they're pointing to the new ``app_label``. This will be an issue when we try to extract container-related models and logic out of publishing and into a new ``containers`` app.
10
+
#. Renaming an app is also cumbersome, because the process requires creating a new app and transitioning the models over. This came up when trying to rename the ``contents`` app to ``media``.
11
+
12
+
There have also been minor inconveniences, like having a long list of ``INSTALLED_APPS`` to maintain in edx-platform over time, or not having these tables easily grouped together in the Django admin interface.
13
+
14
+
Decisions
15
+
---------
16
+
17
+
1. Single openedx_content App
18
+
~~~~~~~~~~~~~~~~~~~~~~~
19
+
20
+
All existing authoring apps will be merged into one Django app (``openedx_learning.app.openedx_content``). Some consequences of this decision:
21
+
22
+
- The tables will be renamed to have the ``openedx_content`` label prefix.
23
+
- All management commands will be moved to the ``openedx_content`` app.
24
+
25
+
2. Logical Separation via Applets
26
+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
27
+
28
+
We will continue to keep internal API boundaries between individual applets, and use the ``api.py`` modules. This is both to insulate applets from implementation changes in other applets, as well as to provide a set of APIs that third-party plugins can utilize. As before, we will use Import Linter to enforce dependency ordering.
29
+
30
+
3. Restructuring Specifics
31
+
~~~~~~~~~~~~~~~~~~~~~~~~~~
32
+
33
+
In one pull request, we are going to:
34
+
35
+
#. Rename the ``openedx_learning.apps.authoring`` package to be ``openedx_learning.apps.openedx_content``.
36
+
#. Create bare shells of the existing ``authoring`` apps (``backup_restore``, ``collections``, ``components``, ``contents``, ``publishing``, ``sections``, ``subsections``, ``units``), and move them to the ``openedx_learning.apps.openedx_content.backcompat`` package. These shells will have an ``apps.py`` file and the ``migrations`` package for each existing app. This will allow for a smooth schema migration to transition the models from these individual apps to ``openedx_content``.
37
+
#. Move the actual models files and API logic for our existing authoring apps to the ``openedx_learning.apps.openedx_content.applets`` package.
38
+
#. Convert the top level ``openedx_learning.apps.openedx_content`` package to be a Django app. The top level ``admin.py``, ``api.py``, and ``models.py`` modules will do wildcard imports from the corresponding modules across all applet packages.
39
+
40
+
In terms of model migrations, all existing apps will have a final migration that uses ``SeparateDatabaseAndState`` to remove all model state, but make no actual database changes. The initial ``openedx_content`` app migration will then also use ``SeparateDatabaseAndState`` to create the model state without doing any actual database operations. The next ``openedx_content`` app migration will rename all existing database tables to use the ``openedx_content`` prefix, for uniformity.
41
+
42
+
The ordering of these migrations is important, and existing edx-platform migrations should remain unchanged. This is important to make sure that we do not introduce ordering inconsistencies for existing installations that are upgrading.
43
+
44
+
Therefore, the migrations will happen in the following order:
45
+
46
+
#. All ``backcompat.*`` apps migrations except for the final ones that delete model state. This takes us up to where migrations would already be before we make any changes.
47
+
#. The ``openedx_content`` app's ``0001_intial`` migration that adds model state without changing the database. At this point, model state exists for the same models in all the old ``backcompat.*`` apps as well as the new ``openedx_content`` app.
48
+
#. edx-platform apps that had foreign keys to old ``backcompat.*`` apps models will need to be switched to point to the new ``openedx_content`` app models. This will likewise be done without a database change, because they're still pointing to the same tables and columns.
49
+
#. Now that edx-platform references have been updated, we can delete the model state from the old ``backcompat.*`` apps and rename the underlying tables (in either order).
50
+
51
+
The tricky part is to make sure that the old ``backcompat.*`` apps models still exist when the edx-platform migrations to move over the references runs. This is problematic because the edx-platform migrations can only specify that they run *after the new openedx_content models are created*. They cannot specify that they run *before the old backcompat models are dropped*.
52
+
53
+
So in order to enforce this ordering, we do the following:
54
+
55
+
* The ``openedx_content`` migration ``0001_initial`` requires that all ``backcompat.*`` migrations except the last ones removing model state are run.
56
+
* The ``openedx_content`` migration ``0002_rename_tables_to_openedx_content`` migration requires that the edx-platform migrations changing refrences over run. This is important anyway, because we want to make sure those reference changes happen before we change any table names.
57
+
* The final ``backcompat.*`` migrations that remove model field state will list ``openedx_content`` app's ``0002_rename_tables_to_openedx_content`` as a dependency.
58
+
59
+
A further complication is that ``openedx_learning`` will often run its migrations without edx-platform present (e.g. for CI or standalone dev purposes), so we can't force ``0002_rename_tables_to_openedx_content`` in the ``openedx_content`` app to have references to edx-platform migrations. To get around this, we dynamically inject those migration dependencies only if we detect those edx-platform apps exist in the currently loaded Django project. This injection happens in the ``apps.py`` initialization for the ``openedx_content`` app.
60
+
61
+
The final complication is that we want these migration dependencies to be the same regardless of whether you're running edx-platform migrations with the LMS or CMS (Studio) settings, or we run the risk of getting into an inconsistent state and dropping the old models before all the edx-platform apps can run their migrations to move their references. To do this, we have to make sure that the edx-platform apps that reference Learning Core models are present in the ``INSTALLED_APPS`` for both configurations.
62
+
63
+
4. The Bigger Picture
64
+
~~~~~~~~~~~~~~~~~~~~~
65
+
66
+
This practice means that the ``openedx_content`` Django app corresponds to a Subdomain in Domain Driven Design terminology, with each applet being a Bounded Context. We call these "Applets" instead of "Bounded Contexts" because we don't want it to get confused for Django's notion of Contexts and Context Processors (or Python's notion of Context Managers).
0 commit comments