From 6d9124712f8ba7e2f8146fa4252cec22580b703d Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 5 May 2026 01:20:13 -0400 Subject: [PATCH 1/2] docs: reformat model docstrings to RST conventions --- .../applets/collections/models.py | 128 ++--- .../applets/components/models.py | 192 ++++---- .../applets/containers/models.py | 164 ++++--- src/openedx_content/applets/media/models.py | 405 +++++++++------- .../applets/publishing/models/draft_log.py | 457 ++++++++++-------- .../publishing/models/learning_package.py | 33 +- .../applets/publishing/models/publish_log.py | 292 +++++------ .../publishing/models/publishable_entity.py | 306 ++++++------ .../applets/sections/models.py | 14 +- .../applets/subsections/models.py | 15 +- src/openedx_content/applets/units/models.py | 13 +- 11 files changed, 1129 insertions(+), 890 deletions(-) diff --git a/src/openedx_content/applets/collections/models.py b/src/openedx_content/applets/collections/models.py index ecea3005c..585543ec2 100644 --- a/src/openedx_content/applets/collections/models.py +++ b/src/openedx_content/applets/collections/models.py @@ -2,67 +2,73 @@ Core models for Collections TLDR Guidelines: + 1. DO NOT modify these models to store full version snapshots. 2. DO NOT use these models to try to reconstruct historical versions of Collections for fast querying. -If you're trying to do either of these things, you probably want a new model or -app. For more details, read on. +If you're trying to do either of these things, you probably want a new model +or app. For more details, read on. The goal of these models is to provide a lightweight method of organizing -PublishableEntities. The first use case for this is modeling the structure of a -v1 Content Library within a LearningPackage. This is what we'll use the -Collection model for. +:class:`PublishableEntity` objects. The first use case for this is modeling +the structure of a v1 Content Library within a :class:`LearningPackage`. +This is what we'll use the :class:`Collection` model for. An important thing to note here is that Collections are *NOT* publishable -entities themselves. They have no "Draft" or "Published" versions. Collections -are never "published", though the things inside of them are. +entities themselves. They have no "Draft" or "Published" versions. +Collections are never "published", though the things inside of them are. When a LibraryContentBlock makes use of a Content Library, it copies all of the items it will use into the Course itself. It will also store a version -on the LibraryContentBlock -- this is a MongoDB ObjectID in v1 and an integer in -v2 Libraries. Later on, the LibraryContentBlock will want to check back to see -if any updates have been made, using its version as a key. If a new version -exists, the course team has the option of re-copying data from the Library. +on the LibraryContentBlock -- this is a MongoDB ObjectID in v1 and an +integer in v2 Libraries. Later on, the LibraryContentBlock will want to +check back to see if any updates have been made, using its version as a +key. If a new version exists, the course team has the option of re-copying +data from the Library. ModuleStore-based v1 Libraries and OeXCore-based v2 libraries both version -the entire library in a series of snapshots. This makes it difficult to have -very large libraries, which is an explicit goal for Modular Learning. In -Open edX Core, we've moved to tracking the versions of individual Components to -address this issue. But that means we no longer have a single version indicator -for "has anything here changed"? - -We *could* have put that version in the ``publishing`` app's PublishLog, but -that would make it too broad. We want the ability to eventually collapse many v1 -Libraries into a single OeXCore backed v2 Library. If we tracked the -versioning in only a central location, then we'd have many false positives where -the version was bumped because something else in the Learning Package changed. -So instead, we're creating a new Collection model inside the LearningPackage to -track that concept. - -A critical takeaway is that we don't have to store snapshots of every version of -a Collection, because that data has been copied over by the LibraryContentBlock. -We only need to store the current state of the Collection, and increment the -version numbers when changes happen. This will allow the LibraryContentBlock to -check in and re-copy over the latest version if the course team desires. - -That's why these models only store the current state of a Collection. Unlike the -``components`` app, ``collections`` does not store fully materialized snapshots -of past versions. This is done intentionally in order to save space and reduce -the cost of writes. Collections may grow to be very large, and we don't want to -be writing N rows with every version, where N is the number of -PublishableEntities in a Collection. - -MVP of these models does not store changesets, but we can add this when there's a -use case for it. The number of rows in these changesets would grow in proportion -to the number of things that are actually changing (instead of copying over -everything on every version). This is could be used to make it easier to figure out -what changed between two given versions of a Collection. A LibraryContentBlock -in a course would have stored the version number of the last time it copied data -from the Collection, and we can eventually surface this data to the user. Note that -while it may be possible to reconstruct past versions of Collections based off of -this changeset data, it's going to be a very slow process to do so, and it is -strongly discouraged. +the entire library in a series of snapshots. This makes it difficult to +have very large libraries, which is an explicit goal for Modular Learning. +In Open edX Core, we've moved to tracking the versions of individual +Components to address this issue. But that means we no longer have a single +version indicator for "has anything here changed"? + +We *could* have put that version in the :mod:`publishing` app's +:class:`PublishLog`, but that would make it too broad. We want the ability +to eventually collapse many v1 Libraries into a single OeXCore backed v2 +Library. If we tracked the versioning in only a central location, then we'd +have many false positives where the version was bumped because something +else in the Learning Package changed. So instead, we're creating a new +:class:`Collection` model inside the :class:`LearningPackage` to track that +concept. + +A critical takeaway is that we don't have to store snapshots of every +version of a Collection, because that data has been copied over by the +LibraryContentBlock. We only need to store the current state of the +Collection, and increment the version numbers when changes happen. This +will allow the LibraryContentBlock to check in and re-copy over the latest +version if the course team desires. + +That's why these models only store the current state of a Collection. +Unlike the :mod:`components` app, :mod:`collections` does not store fully +materialized snapshots of past versions. This is done intentionally in +order to save space and reduce the cost of writes. Collections may grow to +be very large, and we don't want to be writing N rows with every version, +where N is the number of :class:`PublishableEntity` objects in a +Collection. + +MVP of these models does not store changesets, but we can add this when +there's a use case for it. The number of rows in these changesets would +grow in proportion to the number of things that are actually changing +(instead of copying over everything on every version). This is could be +used to make it easier to figure out what changed between two given +versions of a Collection. A LibraryContentBlock in a course would have +stored the version number of the last time it copied data from the +Collection, and we can eventually surface this data to the user. Note that +while it may be possible to reconstruct past versions of Collections based +off of this changeset data, it's going to be a very slow process to do so, +and it is strongly discouraged. """ from __future__ import annotations @@ -83,11 +89,12 @@ class CollectionManager(models.Manager): """ - Custom manager for Collection class. + Custom manager for :class:`Collection`. """ def get_by_code(self, learning_package_id: int, collection_code: str): """ - Get the Collection for the given Learning Package + collection code. + Get the :class:`Collection` for the given :class:`LearningPackage` + + collection code. """ return self.select_related('learning_package') \ .get(learning_package_id=learning_package_id, collection_code=collection_code) @@ -101,16 +108,21 @@ class Collection(models.Model): id = models.AutoField(primary_key=True) - # Each collection belongs to a learning package learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) + """ + Each collection belongs to a :class:`LearningPackage`. + """ - # Every collection is uniquely and permanently identified within its learning package - # by a 'code' that is set during creation. Both will appear in the - # collection's opaque key: - # e.g. "lib-collection:{org_code}:{library_code}:{collection_code}" - # is the opaque key for a library collection. - # TODO: Consider supporting unicode https://github.com/openedx/openedx-platform/issues/38413 + # TODO: Consider supporting unicode + # https://github.com/openedx/openedx-platform/issues/38413 collection_code = code_field(unicode=False) + """ + Every collection is uniquely and permanently identified within its + learning package by a 'code' that is set during creation. Both will + appear in the collection's opaque key: e.g. + ``lib-collection:{org_code}:{library_code}:{collection_code}`` is the + opaque key for a library collection. + """ title = case_insensitive_char_field( null=False, @@ -204,7 +216,7 @@ def __str__(self) -> str: class CollectionPublishableEntity(models.Model): """ - Collection -> PublishableEntity association. + :class:`Collection` → :class:`PublishableEntity` association. """ collection = models.ForeignKey( Collection, diff --git a/src/openedx_content/applets/components/models.py b/src/openedx_content/applets/components/models.py index 5dd0dee7c..a9b16dd19 100644 --- a/src/openedx_content/applets/components/models.py +++ b/src/openedx_content/applets/components/models.py @@ -1,19 +1,22 @@ """ -The model hierarchy is Component -> ComponentVersion -> Media. - -A Component is an entity like a Problem or Video. It has enough information to -identify the Component and determine what the handler should be (e.g. XBlock -Problem), but little beyond that. - -Components have one or more ComponentVersions, which represent saved versions of -that Component. Managing the publishing of these versions is handled through the -publishing app. Component maps 1:1 to PublishableEntity and ComponentVersion -maps 1:1 to PublishableEntityVersion. - -Multiple pieces of Media may be associated with a ComponentVersion, through -the ComponentVersionMedia model. ComponentVersionMedia allows to specify a -ComponentVersion-local identifier. We're using this like a file path by -convention, but it's possible we might want to have special identifiers later. +The model hierarchy is :class:`Component` → :class:`ComponentVersion` → +:class:`Media`. + +A :class:`Component` is an entity like a Problem or Video. It has enough +information to identify the Component and determine what the handler should be +(e.g. XBlock Problem), but little beyond that. + +Components have one or more :class:`ComponentVersion` objects, which represent +saved versions of that Component. Managing the publishing of these versions is +handled through the publishing app. :class:`Component` maps 1:1 to +:class:`PublishableEntity` and :class:`ComponentVersion` maps 1:1 to +:class:`PublishableEntityVersion`. + +Multiple pieces of :class:`Media` may be associated with a +:class:`ComponentVersion`, through the :class:`ComponentVersionMedia` model. +:class:`ComponentVersionMedia` allows to specify a ComponentVersion-local +identifier. We're using this like a file path by convention, but it's possible +we might want to have special identifiers later. """ from __future__ import annotations @@ -49,26 +52,33 @@ class ComponentType(models.Model): probably add a few others over time, such as a component type to represent packages of files for things like Files and Uploads or python_lib.zip files. - Make a ForeignKey against this table if you have to set policy based on the - type of Components–e.g. marking certain types of XBlocks as approved vs. - experimental for use in libraries. + Make a ForeignKey against this table if you have to set policy based on + the type of Components–e.g. marking certain types of XBlocks as approved + vs. experimental for use in libraries. """ - # We don't need the app default of 8-bytes for this primary key, but there - # is just a tiny chance that we'll use ComponentType in a novel, user- - # customizable way that will require more than 32K entries. So let's use a - # 4-byte primary key. id = models.AutoField(primary_key=True) + """ + We don't need the app default of 8-bytes for this primary key, but there + is just a tiny chance that we'll use :class:`ComponentType` in a novel, + user-customizable way that will require more than 32K entries. So let's + use a 4-byte primary key. + """ - # namespace and name work together to help figure out what Component needs - # to handle this data. A namespace is *required*. The namespace for XBlocks - # is "xblock.v1" (to match the setup.py entrypoint naming scheme). namespace = case_sensitive_char_field(max_length=100, blank=False) + """ + ``namespace`` and ``name`` work together to help figure out what + :class:`Component` needs to handle this data. A namespace is *required*. + The namespace for XBlocks is ``xblock.v1`` (to match the ``setup.py`` + entrypoint naming scheme). + """ - # name is a way to help sub-divide namespace if that's convenient. This - # field cannot be null, but it can be blank if it's not necessary. For an - # XBlock, this corresponds to tag, e.g. "video". It's also the block_type in - # the UsageKey. name = case_sensitive_char_field(max_length=100, blank=True) + """ + ``name`` is a way to help sub-divide ``namespace`` if that's convenient. + This field cannot be null, but it can be blank if it's not necessary. + For an XBlock, this corresponds to tag, e.g. "video". It's also the + block_type in the UsageKey. + """ class Meta: constraints = [ @@ -87,48 +97,51 @@ def __str__(self) -> str: class Component(PublishableEntityMixin): """ - This represents any Component that has ever existed in a LearningPackage. + This represents any Component that has ever existed in a + :class:`LearningPackage`. What is a Component ------------------- - A Component is an entity like a Problem or Video. It has enough information - to identify itself and determine what the handler should be (e.g. XBlock - Problem), but little beyond that. + A :class:`Component` is an entity like a Problem or Video. It has enough + information to identify itself and determine what the handler should be + (e.g. XBlock Problem), but little beyond that. - A Component will have many ComponentVersions over time, and most metadata is - associated with the ComponentVersion model and the Media that - ComponentVersions are associated with. + A :class:`Component` will have many :class:`ComponentVersion` objects + over time, and most metadata is associated with the + :class:`ComponentVersion` model and the :class:`Media` that + :class:`ComponentVersion` objects are associated with. - A Component belongs to exactly one LearningPackage. + A :class:`Component` belongs to exactly one :class:`LearningPackage`. - A Component is 1:1 with PublishableEntity and has matching primary key - values. More specifically, ``Component.pk`` maps to - ``Component.publishable_entity_id``, and any place where the Publishing API - module expects to get a ``PublishableEntity.id``, you can use a - ``Component.pk`` instead. + A :class:`Component` is 1:1 with :class:`PublishableEntity` and has + matching primary key values. More specifically, :attr:`Component.pk` + maps to :attr:`Component.publishable_entity_id`, and any place where + the Publishing API module expects to get a :attr:`PublishableEntity.id`, + you can use a :attr:`Component.pk` instead. Identifiers ----------- - Components have a ``publishable_entity`` OneToOneField to the ``publishing`` - app's PublishableEntity field, and it uses this as its primary key. Please - see PublishableEntity's docstring for how you should use its ``uuid`` and - ``key`` fields. + Components have a ``publishable_entity`` ``OneToOneField`` to the + :mod:`publishing` app's :class:`PublishableEntity` field, and it uses + this as its primary key. Please see :class:`PublishableEntity`'s + docstring for how you should use its ``uuid`` and ``key`` fields. State Consistency ----------------- - The ``key`` field on Component's ``publishable_entity`` is derived from the - ``component_type`` and ``component_code`` fields in this model. We don't - support changing the keys yet, but if we do, those values need to be kept - in sync. + The ``key`` field on Component's ``publishable_entity`` is derived from + the ``component_type`` and ``component_code`` fields in this model. We + don't support changing the keys yet, but if we do, those values need to + be kept in sync. How build on this model ----------------------- - Make a foreign key to the Component model when you need a stable reference - that will exist for as long as the LearningPackage itself exists. + Make a foreign key to the :class:`Component` model when you need a + stable reference that will exist for as long as the + :class:`LearningPackage` itself exists. """ ComponentID = NewType("ComponentID", PublishableEntity.ID) @@ -165,22 +178,31 @@ def pk(self): 'publishable_entity__published__publish_log_record__publish_log', ) - # This foreign key is technically redundant because we're already locked to - # a single LearningPackage through our publishable_entity relation. However, - # having this foreign key directly allows us to make indexes that efficiently - # query by other Component fields within a given LearningPackage, which is - # going to be a common use case (and we can't make a compound index using - # columns from different tables). learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) + """ + This foreign key is technically redundant because we're already locked + to a single :class:`LearningPackage` through our ``publishable_entity`` + relation. However, having this foreign key directly allows us to make + indexes that efficiently query by other :class:`Component` fields + within a given :class:`LearningPackage`, which is going to be a common + use case (and we can't make a compound index using columns from + different tables). + """ - # What kind of Component are we? This will usually represent a specific - # XBlock block_type, but we want it to be more flexible in the long term. component_type = models.ForeignKey(ComponentType, on_delete=models.PROTECT) + """ + What kind of :class:`Component` are we? This will usually represent a + specific XBlock block_type, but we want it to be more flexible in the + long term. + """ - # component_code is an identifier that is local to the learning_package and - # component_type. The publishable.entity_ref is derived from component_type - # and component_code. component_code = code_field(unicode=True) + """ + ``component_code`` is an identifier that is local to the + ``learning_package`` and ``component_type``. The + ``publishable.entity_ref`` is derived from ``component_type`` and + ``component_code``. + """ class Meta: constraints = [ @@ -224,26 +246,30 @@ def __str__(self) -> str: class ComponentVersion(PublishableEntityVersionMixin): """ - A particular version of a Component. + A particular version of a :class:`Component`. - This holds the media using a M:M relationship with Media via - ComponentVersionMedia. + This holds the media using a M:M relationship with :class:`Media` via + :class:`ComponentVersionMedia`. """ - # This is technically redundant, since we can get this through - # publishable_entity_version.publishable.component, but this is more - # convenient. component = models.ForeignKey( Component, on_delete=models.CASCADE, related_name="versions" ) + """ + This is technically redundant, since we can get this through + ``publishable_entity_version.publishable.component``, but this is more + convenient. + """ - # The media relation holds the actual interesting data associated with this - # ComponentVersion. media: models.ManyToManyField[Media, ComponentVersionMedia] = models.ManyToManyField( Media, through="ComponentVersionMedia", related_name="component_versions", ) + """ + The media relation holds the actual interesting data associated with + this :class:`ComponentVersion`. + """ class Meta: verbose_name = "Component Version" @@ -252,25 +278,29 @@ class Meta: class ComponentVersionMedia(models.Model): """ - Determines the Media for a given ComponentVersion. + Determines the :class:`Media` for a given :class:`ComponentVersion`. - An ComponentVersion may be associated with multiple pieces of binary data. - For instance, a Video ComponentVersion might be associated with multiple - transcripts in different languages. + An :class:`ComponentVersion` may be associated with multiple pieces of + binary data. For instance, a Video :class:`ComponentVersion` might be + associated with multiple transcripts in different languages. - When Media is associated with a ComponentVersion, it has a ``path`` - that is unique within the context of that ComponentVersion. This is - used as a local file-path-like identifier, e.g. "static/image.png". + When :class:`Media` is associated with a :class:`ComponentVersion`, it + has a ``path`` that is unique within the context of that + :class:`ComponentVersion`. This is used as a local file-path-like + identifier, e.g. ``static/image.png``. - Media is immutable and sharable across multiple ComponentVersions. + :class:`Media` is immutable and sharable across multiple + :class:`ComponentVersion` objects. """ component_version = models.ForeignKey(ComponentVersion, on_delete=models.CASCADE) media = models.ForeignKey(Media, on_delete=models.RESTRICT) - # path is a local file-path-like identifier for the media within a - # ComponentVersion. path = ref_field() + """ + ``path`` is a local file-path-like identifier for the media within a + :class:`ComponentVersion`. + """ class Meta: constraints = [ diff --git a/src/openedx_content/applets/containers/models.py b/src/openedx_content/applets/containers/models.py index c234b04b5..40018b399 100644 --- a/src/openedx_content/applets/containers/models.py +++ b/src/openedx_content/applets/containers/models.py @@ -32,12 +32,13 @@ class EntityList(models.Model): """ - EntityLists are a common structure to hold parent-child relations. + :class:`EntityList` is a common structure to hold parent-child relations. - EntityLists are not PublishableEntities in and of themselves. That's because - sometimes we'll want the same kind of data structure for things that we - dynamically generate for individual students (e.g. Variants). EntityLists are - anonymous in a sense–they're pointed to by ContainerVersions and + :class:`EntityList` objects are not :class:`PublishableEntity` objects in + and of themselves. That's because sometimes we'll want the same kind of + data structure for things that we dynamically generate for individual + students (e.g. Variants). :class:`EntityList` objects are anonymous in + a sense–they're pointed to by :class:`ContainerVersion` objects and other models, rather than being looked up by their own identifiers. """ @@ -46,54 +47,65 @@ def rows(self): """ Convenience method to iterate rows. - I'd normally make this the reverse lookup name for the EntityListRow -> - EntityList foreign key relation, but we already have references to - entitylistrow_set in various places, and I thought this would be better - than breaking compatibility. + I'd normally make this the reverse lookup name for the + :class:`EntityListRow` → :class:`EntityList` foreign key relation, + but we already have references to ``entitylistrow_set`` in various + places, and I thought this would be better than breaking + compatibility. """ return self.entitylistrow_set.order_by("order_num") class EntityListRow(models.Model): """ - Each EntityListRow points to a PublishableEntity, optionally at a specific - version. + Each :class:`EntityListRow` points to a :class:`PublishableEntity`, + optionally at a specific version. - There is a row in this table for each member of an EntityList. The order_num - field is used to determine the order of the members in the list. + There is a row in this table for each member of an :class:`EntityList`. + The ``order_num`` field is used to determine the order of the members + in the list. """ entity_list = models.ForeignKey(EntityList, on_delete=models.CASCADE) - # This ordering should be treated as immutable–if the ordering needs to - # change, we create a new EntityList and copy things over. order_num = models.PositiveIntegerField() + """ + This ordering should be treated as immutable–if the ordering needs to + change, we create a new :class:`EntityList` and copy things over. + """ - # Simple case would use these fields with our convention that null versions - # means "get the latest draft or published as appropriate". These entities - # could be Selectors, in which case we'd need to do more work to find the right - # variant. The publishing app itself doesn't know anything about Selectors - # however, and just treats it as another PublishableEntity. entity = models.ForeignKey(PublishableEntity, on_delete=models.RESTRICT) + """ + Simple case would use this field with our convention that null versions + means "get the latest draft or published as appropriate". These + entities could be Selectors, in which case we'd need to do more work + to find the right variant. The publishing app itself doesn't know + anything about Selectors however, and just treats it as another + :class:`PublishableEntity`. + """ - # The version references point to the specific PublishableEntityVersion that - # this EntityList has for this PublishableEntity for both the draft and - # published states. However, we don't want to have to create new EntityList - # every time that a member is updated, because that would waste a lot of - # space and make it difficult to figure out when the metadata of something - # like a Unit *actually* changed, vs. when its child members were being - # updated. Doing so could also potentially lead to race conditions when - # updating multiple layers of containers. - # - # So our approach to this is to use a value of None (null) to represent an - # unpinned reference to a PublishableEntity. It's shorthand for "just use - # the latest draft or published version of this, as appropriate". entity_version = models.ForeignKey( PublishableEntityVersion, on_delete=models.RESTRICT, null=True, related_name="+", # Do we need the reverse relation? ) + """ + The version references point to the specific + :class:`PublishableEntityVersion` that this :class:`EntityList` has for + this :class:`PublishableEntity` for both the draft and published + states. However, we don't want to have to create new + :class:`EntityList` every time that a member is updated, because that + would waste a lot of space and make it difficult to figure out when + the metadata of something like a Unit *actually* changed, vs. when its + child members were being updated. Doing so could also potentially + lead to race conditions when updating multiple layers of containers. + + So our approach to this is to use a value of None (null) to represent + an unpinned reference to a :class:`PublishableEntity`. It's shorthand + for "just use the latest draft or published version of this, as + appropriate". + """ def is_pinned(self): return self.entity_version_id is not None @@ -121,22 +133,25 @@ class ContainerImplementationMissingError(Exception): class ContainerType(models.Model): """ - Normalized representation of the type of Container. + Normalized representation of the type of :class:`Container`. - Typical container types are "unit", "subsection", and "section", but there - may be others in the future. + Typical container types are "unit", "subsection", and "section", but + there may be others in the future. """ id = models.AutoField(primary_key=True) - # type_code uniquely identifies the type of container, e.g. "unit", "subsection", etc. - # Plugins/apps that add their own ContainerTypes should prefix it, e.g. - # "myapp_custom_unit" instead of "custom_unit", to avoid collisions. type_code = case_sensitive_char_field( max_length=100, blank=False, unique=True, ) + """ + ``type_code`` uniquely identifies the type of container, e.g. ``unit``, + ``subsection``, etc. Plugins/apps that add their own + :class:`ContainerType` objects should prefix it, e.g. + ``myapp_custom_unit`` instead of ``custom_unit``, to avoid collisions. + """ class Meta: constraints = [ @@ -153,43 +168,54 @@ def __str__(self) -> str: # pylint: disable=invalid-str-returned class Container(PublishableEntityMixin): """ - A Container is a type of PublishableEntity that holds other - PublishableEntities. For example, a "Unit" Container might hold several - Components. + A :class:`Container` is a type of :class:`PublishableEntity` that holds + other :class:`PublishableEntity` objects. For example, a "Unit" + :class:`Container` might hold several :class:`Component` objects. For now, all containers have a static "entity list" that defines which - containers/components/enities they hold. As we complete the Containers API, - we will also add support for dynamic containers which may contain different - entities for different learners or at different times. + containers/components/enities they hold. As we complete the Containers + API, we will also add support for dynamic containers which may contain + different entities for different learners or at different times. """ ContainerID = NewType("ContainerID", PublishableEntity.ID) type ID = ContainerID type_code: str # Subclasses must override this, e.g. "unit" - # olx_code: the OLX for XML serialization. Subclasses _may_ override this. - # Only used in openedx-platform at the moment. We'll likely have to replace this with something more sophisticated. olx_tag_name: str = "" + """ + The OLX ```` for XML serialization. Subclasses _may_ override + this. Only used in openedx-platform at the moment. We'll likely have + to replace this with something more sophisticated. + """ _type_instance: ContainerType # Cache used by get_container_type() - # This foreign key is technically redundant because we're already locked to - # a single LearningPackage through our publishable_entity relation. However, - # having this foreign key directly allows us to make indexes that efficiently - # query by other Container fields within a given LearningPackage. learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) + """ + This foreign key is technically redundant because we're already locked + to a single :class:`LearningPackage` through our ``publishable_entity`` + relation. However, having this foreign key directly allows us to make + indexes that efficiently query by other :class:`Container` fields + within a given :class:`LearningPackage`. + """ - # The type of the container. Cannot be changed once the container is created. container_type = models.ForeignKey( ContainerType, null=False, on_delete=models.RESTRICT, editable=False, ) + """ + The type of the container. Cannot be changed once the container is + created. + """ - # container_code is an identifier that is local to the learning_package. - # Unlike component_code, it is unique across all container types within - # the same LearningPackage. container_code = code_field(unicode=True) + """ + ``container_code`` is an identifier that is local to the + ``learning_package``. Unlike ``component_code``, it is unique across + all container types within the same :class:`LearningPackage`. + """ @property def id(self) -> ID: @@ -281,23 +307,23 @@ def all_subclasses() -> list[type[Container]]: class ContainerVersion(PublishableEntityVersionMixin): """ - A version of a Container. + A version of a :class:`Container`. - By convention, we would only want to create new versions when the Container - itself changes, and not when the Container's child elements change. For - example: + By convention, we would only want to create new versions when the + :class:`Container` itself changes, and not when the :class:`Container`'s + child elements change. For example: - * Something was added to the Container. + * Something was added to the :class:`Container`. * We re-ordered the rows in the container. * Something was removed to the container. - * The Container's metadata changed, e.g. the title. - * We pin to different versions of the Container. - - The last looks a bit odd, but it's because *how we've defined the Unit* has - changed if we decide to explicitly pin a set of versions for the children, - and then later change our minds and move to a different set. It also just - makes things easier to reason about if we say that entity_list never - changes for a given ContainerVersion. + * The :class:`Container`'s metadata changed, e.g. the title. + * We pin to different versions of the :class:`Container`. + + The last looks a bit odd, but it's because *how we've defined the Unit* + has changed if we decide to explicitly pin a set of versions for the + children, and then later change our minds and move to a different set. + It also just makes things easier to reason about if we say that + ``entity_list`` never changes for a given :class:`ContainerVersion`. """ container = models.ForeignKey( @@ -306,13 +332,15 @@ class ContainerVersion(PublishableEntityVersionMixin): related_name="versions", ) - # The list of entities (frozen and/or unfrozen) in this container entity_list = models.ForeignKey( EntityList, on_delete=models.RESTRICT, null=False, related_name="container_versions", ) + """ + The list of entities (frozen and/or unfrozen) in this container. + """ def clean(self): """ diff --git a/src/openedx_content/applets/media/models.py b/src/openedx_content/applets/media/models.py index 30836358d..233421a1d 100644 --- a/src/openedx_content/applets/media/models.py +++ b/src/openedx_content/applets/media/models.py @@ -39,13 +39,15 @@ @cache def get_storage() -> Storage: """ - Return the Storage instance for our Media file persistence. + Return the :class:`Storage` instance for our :class:`Media` file + persistence. - This will first search for an OPENEDX_LEARNING config dictionary and return - a Storage subclass based on that configuration. + This will first search for an ``OPENEDX_LEARNING`` config dictionary and + return a :class:`Storage` subclass based on that configuration. - If there is no value for the OPENEDX_LEARNING setting, we return the default - MEDIA storage class. TODO: Should we make it just error instead? + If there is no value for the ``OPENEDX_LEARNING`` setting, we return the + default ``MEDIA`` storage class. TODO: Should we make it just error + instead? """ # TODO: Document & rename this setting (https://github.com/openedx/openedx-core/issues/481) config_dict = getattr(settings, 'OPENEDX_LEARNING', {}) @@ -65,57 +67,69 @@ def get_storage() -> Storage: class MediaType(models.Model): """ - Stores Media types for use by Media models. - - This is the same as MIME types (the IANA renamed MIME Types to Media Types). - We don't pre-populate this table, so APIs that add Media must ensure that - the desired Media Type exists. - - Media types are written as {type}/{sub_type}+{suffix}, where suffixes are - seldom used. Examples: - - * application/json - * text/css - * image/svg+xml - * application/vnd.openedx.xblock.v1.problem+xml - - We have this as a separate model (instead of a field on Media) because: - - 1. We can save a lot on storage and indexing for Media if we're just - storing foreign key references there, rather than the entire content - string to be indexed. This is especially relevant for our (long) custom - types like "application/vnd.openedx.xblock.v1.problem+xml". - 2. These values can occasionally change. For instance, "text/javascript" vs. - "application/javascript". Also, we will be using a fair number of "vnd." - style of custom content types, and we may want the flexibility of - changing that without having to worry about migrating millions of rows of - Media. - """ - # We're going to have many foreign key references from Media into this - # model, and we don't need to store those as 8-byte BigAutoField, as is the - # default for this app. It's likely that a SmallAutoField would work, but I - # can just barely imagine using more than 32K Media types if we have a bunch - # of custom "vnd." entries, or start tracking suffixes and parameters. Which - # is how we end up at the 4-byte AutoField. + Stores Media types for use by :class:`Media` models. + + This is the same as MIME types (the IANA renamed MIME Types to Media + Types). We don't pre-populate this table, so APIs that add + :class:`Media` must ensure that the desired Media Type exists. + + Media types are written as ``{type}/{sub_type}+{suffix}``, where + suffixes are seldom used. Examples: + + * ``application/json`` + * ``text/css`` + * ``image/svg+xml`` + * ``application/vnd.openedx.xblock.v1.problem+xml`` + + We have this as a separate model (instead of a field on :class:`Media`) + because: + + 1. We can save a lot on storage and indexing for :class:`Media` if + we're just storing foreign key references there, rather than the + entire content string to be indexed. This is especially relevant + for our (long) custom types like + ``application/vnd.openedx.xblock.v1.problem+xml``. + 2. These values can occasionally change. For instance, + ``text/javascript`` vs. ``application/javascript``. Also, we will + be using a fair number of ``vnd.`` style of custom content types, + and we may want the flexibility of changing that without having to + worry about migrating millions of rows of :class:`Media`. + + Media types are denoted as ``{type}/{sub_type}+{suffix}``. We currently + do not support parameters. + """ id = models.AutoField(primary_key=True) + """ + We're going to have many foreign key references from :class:`Media` + into this model, and we don't need to store those as 8-byte + ``BigAutoField``, as is the default for this app. It's likely that a + ``SmallAutoField`` would work, but I can just barely imagine using + more than 32K Media types if we have a bunch of custom ``vnd.`` + entries, or start tracking suffixes and parameters. Which is how we + end up at the 4-byte ``AutoField``. + """ - # Media types are denoted as {type}/{sub_type}+{suffix}. We currently do not - # support parameters. - - # Media type, e.g. "application", "text", "image". Per RFC 4288, this can be - # at most 127 chars long and is case insensitive. In practice, it's almost - # always written in lowercase. type = case_insensitive_char_field(max_length=127, blank=False, null=False) + """ + Media type, e.g. ``application``, ``text``, ``image``. Per RFC 4288, + this can be at most 127 chars long and is case insensitive. In + practice, it's almost always written in lowercase. + """ - # Media sub-type, e.g. "json", "css", "png". Per RFC 4288, this can be at - # most 127 chars long and is case insensitive. In practice, it's almost - # always written in lowercase. sub_type = case_insensitive_char_field(max_length=127, blank=False, null=False) + """ + Media sub-type, e.g. ``json``, ``css``, ``png``. Per RFC 4288, this + can be at most 127 chars long and is case insensitive. In practice, + it's almost always written in lowercase. + """ - # Suffix, like "xml" (e.g. "image/svg+xml"). Usually blank. I couldn't find - # an RFC description of the length limit, and 127 is probably excessive. But - # this table should be small enough where it doesn't really matter. suffix = case_insensitive_char_field(max_length=127, blank=True, null=False) + """ + Suffix, like ``xml`` (e.g. ``image/svg+xml``). Usually blank. I + couldn't find an RFC description of the length limit, and 127 is + probably excessive. But this table should be small enough where it + doesn't really matter. + """ class Meta: constraints = [ @@ -141,110 +155,134 @@ class Media(models.Model): """ This is the most primitive piece of content. - This model serves to lookup, de-duplicate, and store text and files. A piece - of Media is identified purely by its data, the media type, and the - LearningPackage it is associated with. It has no version or file name - metadata associated with it. It exists to be a dumb blob of data that higher - level models like ComponentVersions can assemble together. + This model serves to lookup, de-duplicate, and store text and files. A + piece of :class:`Media` is identified purely by its data, the media + type, and the :class:`LearningPackage` it is associated with. It has + no version or file name metadata associated with it. It exists to be a + dumb blob of data that higher level models like + :class:`ComponentVersion` objects can assemble together. - # In-model Text vs. File + In-model Text vs. File + ---------------------- - That being said, the Media model does have some complexity to accomodate - different access patterns that we have in our app. In particular, it can - store data in two ways: the ``text`` field and a file (``has_file=True``) - A Media object must use at least one of these methods, but can use both if - it's appropriate. + That being said, the :class:`Media` model does have some complexity to + accomodate different access patterns that we have in our app. In + particular, it can store data in two ways: the ``text`` field and a + file (``has_file=True``) A :class:`Media` object must use at least one + of these methods, but can use both if it's appropriate. Use the ``text`` field when: - * the content is a relatively small (< 50K, usually much less) piece of text + + * the content is a relatively small (< 50K, usually much less) piece + of text * you want to do be able to query across many rows at once * low, predictable latency is important Use file storage when: + * the content is large, or not text-based - * you want to be able to serve the file content directly to the browser + * you want to be able to serve the file content directly to the + browser - The high level tradeoff is that ``text`` will give you faster access, and - file storage will give you a much more affordable and scalable backend. The - backend used for files will also eventually allow direct browser download - access, whereas the ``text`` field will not. But again, you can use both at - the same time if needed. + The high level tradeoff is that ``text`` will give you faster access, + and file storage will give you a much more affordable and scalable + backend. The backend used for files will also eventually allow direct + browser download access, whereas the ``text`` field will not. But + again, you can use both at the same time if needed. - # Association with a LearningPackage + Association with a LearningPackage + ---------------------------------- - Media is associated with a specific LearningPackage. Doing so allows us to - more easily query for how much storge space a specific LearningPackage - (likely a library) is using, and to clean up unused data. + :class:`Media` is associated with a specific :class:`LearningPackage`. + Doing so allows us to more easily query for how much storge space a + specific :class:`LearningPackage` (likely a library) is using, and to + clean up unused data. - When we get to borrowing Media across LearningPackages, it's likely that - we will want to copy them. That way, even if the originating LearningPackage - is deleted, it won't break other LearningPackages that are making use if it. + When we get to borrowing :class:`Media` across :class:`LearningPackage` + objects, it's likely that we will want to copy them. That way, even if + the originating :class:`LearningPackage` is deleted, it won't break + other :class:`LearningPackage` objects that are making use if it. - # Media Types, and file duplication + Media Types, and file duplication + --------------------------------- - Media is almost 1:1 with the files that it pushes to a storage backend, - but not quite. The file locations are generated purely as a product of the - LearningPackage UUID and the Media's ``hash_digest``, but Media also - takes into account the ``media_type``. + :class:`Media` is almost 1:1 with the files that it pushes to a storage + backend, but not quite. The file locations are generated purely as a + product of the :class:`LearningPackage` UUID and the :class:`Media`'s + ``hash_digest``, but :class:`Media` also takes into account the + ``media_type``. - For example, say we had a Media with the following data: + For example, say we had a :class:`Media` with the following data:: ["hello", "world"] - That is legal syntax for both JSON and YAML. If you want to attach some - YAML-specific metadata in a new model, you could make it 1:1 with the - Media that matched the "application/yaml" media type. The YAML and JSON - versions of this data would be two separate Media rows that would share - the same ``hash_digest`` value. If they both stored a file, they would be - pointing to the same file location. If they only used the ``text`` field, - then that value would be duplicated across the two separate Media rows. - - The alternative would have been to associate media types at the level where - this data was being added to a ComponentVersion, but that would have added - more complexity. Right now, you could make an ImageMedia 1:1 model that - analyzed images and created metatdata entries for them (dimensions, GPS) - without having to understand how ComponentVerisons work. - - This is definitely an edge case, and it's likely the only time collisions - like this will happen in practice is with blank files. It also means that - using this table to measure disk usage may be slightly inaccurate when used - in a LearningPackage with collisions–though we expect to use numbers like - that mostly to get a broad sense of usage and look for major outliers, - rather than for byte-level accuracy (it wouldn't account for the non-trivial - indexing storage costs either). - - # Immutability - - From the outside, Media should appear immutable. Since the Media is - looked up by a hash of its data, a change in the data means that we should - look up the hash value of that new data and create a new Media if we don't - find a match. - - That being said, the Media model has different ways of storing that data, - and that is mutable. We could decide that a certain type of Media should - be optimized to store its text in the table. Or that a media type that we - had previously only stored as text now also needs to be stored on in the - file storage backend so that it can be made available to be downloaded. - These operations would be done as data migrations. - - # Extensibility - - Third-party apps are encouraged to create models that have a OneToOneField - relationship with Media. For instance, an ImageMedia model might join - 1:1 with all Media that has image/* media types, and provide additional - metadata for that data. - """ - # Max size of the file. + That is legal syntax for both JSON and YAML. If you want to attach + some YAML-specific metadata in a new model, you could make it 1:1 with + the :class:`Media` that matched the ``application/yaml`` media type. + The YAML and JSON versions of this data would be two separate + :class:`Media` rows that would share the same ``hash_digest`` value. + If they both stored a file, they would be pointing to the same file + location. If they only used the ``text`` field, then that value would + be duplicated across the two separate :class:`Media` rows. + + The alternative would have been to associate media types at the level + where this data was being added to a :class:`ComponentVersion`, but + that would have added more complexity. Right now, you could make an + ``ImageMedia`` 1:1 model that analyzed images and created metatdata + entries for them (dimensions, GPS) without having to understand how + :class:`ComponentVersion` objects work. + + This is definitely an edge case, and it's likely the only time + collisions like this will happen in practice is with blank files. It + also means that using this table to measure disk usage may be + slightly inaccurate when used in a :class:`LearningPackage` with + collisions–though we expect to use numbers like that mostly to get a + broad sense of usage and look for major outliers, rather than for + byte-level accuracy (it wouldn't account for the non-trivial indexing + storage costs either). + + Immutability + ------------ + + From the outside, :class:`Media` should appear immutable. Since the + :class:`Media` is looked up by a hash of its data, a change in the + data means that we should look up the hash value of that new data and + create a new :class:`Media` if we don't find a match. + + That being said, the :class:`Media` model has different ways of + storing that data, and that is mutable. We could decide that a + certain type of :class:`Media` should be optimized to store its text + in the table. Or that a media type that we had previously only stored + as text now also needs to be stored on in the file storage backend so + that it can be made available to be downloaded. These operations + would be done as data migrations. + + Extensibility + ------------- + + Third-party apps are encouraged to create models that have a + ``OneToOneField`` relationship with :class:`Media`. For instance, an + ``ImageMedia`` model might join 1:1 with all :class:`Media` that has + ``image/*`` media types, and provide additional metadata for that + data. + """ MAX_FILE_SIZE = 50_000_000 + """ + Max size of the file. + """ - # 50K is our limit for text data, like OLX. This means 50K *characters*, - # not bytes. Since UTF-8 encodes characters using as many as 4 bytes, this - # could be as much as 200K of data if we had nothing but emojis. MAX_TEXT_LENGTH = 50_000 + """ + 50K is our limit for text data, like OLX. This means 50K *characters*, + not bytes. Since UTF-8 encodes characters using as many as 4 bytes, + this could be as much as 200K of data if we had nothing but emojis. + """ - # Custom type for our primary key, to make it more type-safe when using in - # API calls. MediaID = NewType("MediaID", int) + """ + Custom type for our primary key, to make it more type-safe when using + in API calls. + """ type ID = MediaID class IDField(TypedBigAutoField[ID]): # Boilerplate for fully-typed ID field. @@ -256,36 +294,39 @@ class IDField(TypedBigAutoField[ID]): # Boilerplate for fully-typed ID field. learning_package = models.ForeignKey(LearningPackage, on_delete=models.CASCADE) - # What is the Media type (a.k.a. MIME type) of this data? media_type = models.ForeignKey(MediaType, on_delete=models.PROTECT) + """ + What is the :class:`MediaType` (a.k.a. MIME type) of this data? + """ - # This is the size of the file in bytes. This can be different than the - # character length of a text file, since UTF-8 encoding can use anywhere - # between 1-4 bytes to represent any given character. size = models.PositiveBigIntegerField( validators=[MaxValueValidator(MAX_FILE_SIZE)], ) + """ + This is the size of the file in bytes. This can be different than the + character length of a text file, since UTF-8 encoding can use anywhere + between 1-4 bytes to represent any given character. + """ - # This hash value may be calculated using create_hash_digest from the - # openedx.lib.fields module. When storing text, we hash the UTF-8 - # encoding of that text value, regardless of whether we also write it to a - # file or not. When storing just a file, we hash the bytes in the file. hash_digest = hash_field() + """ + This hash value may be calculated using ``create_hash_digest`` from the + ``openedx.lib.fields`` module. When storing text, we hash the UTF-8 + encoding of that text value, regardless of whether we also write it + to a file or not. When storing just a file, we hash the bytes in the + file. + """ - # Do we have file data stored for this Media in our file storage backend? - # We use has_file instead of a FileField because it's more space efficient. - # The location of a Media's file data is derivable from the Learning - # Package's UUID and the hash of the Media. There's no need to waste that - # space to encode it in every row. has_file = models.BooleanField() + """ + Do we have file data stored for this :class:`Media` in our file + storage backend? We use ``has_file`` instead of a ``FileField`` because + it's more space efficient. The location of a :class:`Media`'s file + data is derivable from the :class:`LearningPackage`'s UUID and the + hash of the :class:`Media`. There's no need to waste that space to + encode it in every row. + """ - # The ``text`` field contains the text representation of the Media, if - # it is available. A blank value means means that we are storing text for - # this Media, and that text happens to be an empty string. A null value - # here means that we are not storing any text here, and the Media exists - # only in file form. It is an error for ``text`` to be None and ``has_file`` - # to be False, since that would mean we haven't stored data anywhere at all. - # text = MultiCollationTextField( blank=True, null=True, @@ -298,19 +339,31 @@ class IDField(TypedBigAutoField[ID]): # Boilerplate for fully-typed ID field. "mysql": "utf8mb4_unicode_ci", } ) + """ + The ``text`` field contains the text representation of the + :class:`Media`, if it is available. A blank value means means that we + are storing text for this :class:`Media`, and that text happens to be + an empty string. A null value here means that we are not storing any + text here, and the :class:`Media` exists only in file form. It is an + error for ``text`` to be None and ``has_file`` to be False, since that + would mean we haven't stored data anywhere at all. + """ - # This should be manually set so that multiple Media rows being set in - # the same transaction are created with the same timestamp. The timestamp - # should be UTC. created = manual_date_time_field() + """ + This should be manually set so that multiple :class:`Media` rows being + set in the same transaction are created with the same timestamp. The + timestamp should be UTC. + """ @cached_property def mime_type(self) -> str: """ - The IANA media type (a.k.a. MIME type) of the Media, in string form. + The IANA media type (a.k.a. MIME type) of the :class:`Media`, in + string form. MIME types reference: - https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types + https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types """ return str(self.media_type) @@ -319,23 +372,24 @@ def path(self): """ Logical path at which this content is stored (or would be stored). - This path is relative to OPENEDX_LEARNING['MEDIA'] configured storage - root. This file may not exist because has_file=False, or because we - haven't written the file yet (this is the method we call when trying to - figure out where the file *should* go). + This path is relative to ``OPENEDX_LEARNING['MEDIA']`` configured + storage root. This file may not exist because ``has_file=False``, + or because we haven't written the file yet (this is the method we + call when trying to figure out where the file *should* go). - For historical reasons (and backwards compatibility), the prefix for - this path is "content/" and not "media/". + For historical reasons (and backwards compatibility), the prefix + for this path is ``content/`` and not ``media/``. """ return f"content/{self.learning_package.uuid}/{self.hash_digest}" def os_path(self): """ - The full OS path for the underlying file for this Media. + The full OS path for the underlying file for this :class:`Media`. - This will not be supported by all Storage class types. + This will not be supported by all :class:`Storage` class types. - This will return ``None`` if there is no backing file (has_file=False). + This will return ``None`` if there is no backing file + (``has_file=False``). """ try: if self.has_file: @@ -346,15 +400,16 @@ def os_path(self): def read_file(self) -> File: """ - Get a File object that has been open for reading. + Get a :class:`File` object that has been open for reading. - We intentionally don't expose an `open()` call where callers can open - this file in write mode. Writing a Media file should happen at most - once, and the logic is not obvious (see ``write_file``). + We intentionally don't expose an ``open()`` call where callers can + open this file in write mode. Writing a :class:`Media` file should + happen at most once, and the logic is not obvious (see + :meth:`write_file`). - At the end of the day, the caller can close the returned File and reopen - it in whatever mode they want, but we're trying to gently discourage - that kind of usage. + At the end of the day, the caller can close the returned + :class:`File` and reopen it in whatever mode they want, but we're + trying to gently discourage that kind of usage. """ return get_storage().open(self.path, 'rb') @@ -362,9 +417,9 @@ def write_file(self, file: File) -> None: """ Write file contents to the file storage backend. - This function does nothing if the file already exists. Note that Media - is supposed to be immutable, so this should normally only be called once - for a given Media row. + This function does nothing if the file already exists. Note that + :class:`Media` is supposed to be immutable, so this should + normally only be called once for a given :class:`Media` row. """ storage = get_storage() @@ -397,8 +452,8 @@ def clean(self): """ Make sure we're actually storing *something*. - If this Media has neither a file or text data associated with it, - it's in a broken/useless state and shouldn't be saved. + If this :class:`Media` has neither a file or text data associated + with it, it's in a broken/useless state and shouldn't be saved. """ if (not self.has_file) and (self.text is None): raise ValidationError( diff --git a/src/openedx_content/applets/publishing/models/draft_log.py b/src/openedx_content/applets/publishing/models/draft_log.py index 18ebf313b..f59119acc 100644 --- a/src/openedx_content/applets/publishing/models/draft_log.py +++ b/src/openedx_content/applets/publishing/models/draft_log.py @@ -14,61 +14,73 @@ class Draft(models.Model): """ - Find the active draft version of an entity (usually most recently created). + Find the active draft version of an entity (usually most recently + created). This model mostly only exists to allow us to join against a bunch of - PublishableEntity objects at once and get all their latest drafts. You might - use this together with Published in order to see which Drafts haven't been - published yet. - - A Draft entry should be created whenever a new PublishableEntityVersion is - created. This means there are three possible states: - - 1. No Draft entry for a PublishableEntity: This means a PublishableEntity - was created, but no PublishableEntityVersion was ever made for it, so - there was never a Draft version. - 2. A Draft entry exists and points to a PublishableEntityVersion: This is - the most common state. - 3. A Draft entry exists and points to a null version: This means a version - used to be the draft, but it's been functionally "deleted". The versions - still exist in our history, but we're done using it. - - It would have saved a little space to add this data to the Published model - (and possibly call the combined model something else). Split Modulestore did - this with its active_versions table. I keep it separate here to get a better - separation of lifecycle events: i.e. this table *only* changes when drafts - are updated, not when publishing happens. The Published model only changes - when something is published. - - .. no_pii + :class:`PublishableEntity` objects at once and get all their latest + drafts. You might use this together with :class:`Published` in order + to see which Drafts haven't been published yet. + + A :class:`Draft` entry should be created whenever a new + :class:`PublishableEntityVersion` is created. This means there are + three possible states: + + 1. No :class:`Draft` entry for a :class:`PublishableEntity`: This means + a :class:`PublishableEntity` was created, but no + :class:`PublishableEntityVersion` was ever made for it, so there + was never a Draft version. + 2. A :class:`Draft` entry exists and points to a + :class:`PublishableEntityVersion`: This is the most common state. + 3. A :class:`Draft` entry exists and points to a null version: This + means a version used to be the draft, but it's been functionally + "deleted". The versions still exist in our history, but we're done + using it. + + It would have saved a little space to add this data to the + :class:`Published` model (and possibly call the combined model + something else). Split Modulestore did this with its + ``active_versions`` table. I keep it separate here to get a better + separation of lifecycle events: i.e. this table *only* changes when + drafts are updated, not when publishing happens. The :class:`Published` + model only changes when something is published. + + .. no_pii: """ - # If we're removing a PublishableEntity entirely, also remove the Draft - # entry for it. This isn't a normal operation, but can happen if you're - # deleting an entire LearningPackage. entity = models.OneToOneField( PublishableEntity, on_delete=models.CASCADE, primary_key=True, ) + """ + If we're removing a :class:`PublishableEntity` entirely, also remove + the :class:`Draft` entry for it. This isn't a normal operation, but + can happen if you're deleting an entire :class:`LearningPackage`. + """ + version = models.OneToOneField( PublishableEntityVersion, on_delete=models.RESTRICT, null=True, blank=True, ) - # Note: this is actually a 1:1 relation in practice, but I'm keeping the - # definition more consistent with the Published model, which has an fkey - # to PublishLogRecord. Unlike PublishLogRecord, this fkey is a late - # addition to this data model, so we have to allow null values for the - # initial migration. But making this nullable also has another advantage, - # in that it allows us to set the draft_log_record to the most recent change - # while inside a bulk_draft_changes_for operation, and then delete that log - # record if it is undone in the same bulk operation. + draft_log_record = models.ForeignKey( "DraftChangeLogRecord", on_delete=models.SET_NULL, null=True, ) + """ + Note: this is actually a 1:1 relation in practice, but I'm keeping the + definition more consistent with the :class:`Published` model, which + has an fkey to :class:`PublishLogRecord`. Unlike + :class:`PublishLogRecord`, this fkey is a late addition to this data + model, so we have to allow null values for the initial migration. But + making this nullable also has another advantage, in that it allows us + to set the ``draft_log_record`` to the most recent change while inside + a ``bulk_draft_changes_for`` operation, and then delete that log + record if it is undone in the same bulk operation. + """ @property def log_record(self): @@ -81,14 +93,17 @@ class DraftQuerySet(models.QuerySet): def with_unpublished_changes(self): """ - Drafts with versions that are different from what is Published. - - This will not return Drafts that have unpublished changes in their - dependencies. Example: A Unit is published with a Component as one - of its child. Then someone modifies the draft of the Component. If - both the Unit and the Component Drafts were part of the queryset, - this method would return only the changed Component, and not the - Unit. (We can add this as an optional flag later if we want.) + :class:`Draft` objects with versions that are different from + what is Published. + + This will not return :class:`Draft` objects that have + unpublished changes in their dependencies. Example: A Unit is + published with a Component as one of its child. Then someone + modifies the draft of the Component. If both the Unit and the + Component :class:`Draft` objects were part of the queryset, + this method would return only the changed Component, and not + the Unit. (We can add this as an optional flag later if we + want.) """ return ( self.select_related("entity__published__version") @@ -114,18 +129,21 @@ def with_unpublished_changes(self): class DraftChangeLog(models.Model): """ - There is one row in this table for every time Drafts are created/modified. + There is one row in this table for every time :class:`Draft` objects are + created/modified. - There are some operations that affect many Drafts at once, such as - discarding changes (i.e. reset to the published versions) or doing an - import. These would be represented by one DraftChangeLog with many - DraftChangeLogRecords in it--one DraftChangeLogRecord for every - PublishableEntity that was modified. + There are some operations that affect many :class:`Draft` objects at + once, such as discarding changes (i.e. reset to the published versions) + or doing an import. These would be represented by one + :class:`DraftChangeLog` with many :class:`DraftChangeLogRecord` objects + in it—one :class:`DraftChangeLogRecord` for every + :class:`PublishableEntity` that was modified. Even if we're only directly changing the draft version of one - PublishableEntity, we will get multiple DraftChangeLogRecords if changing - that entity causes side-effects. See the docstrings for DraftChangeLogRecord - and DraftSideEffect for more details. + :class:`PublishableEntity`, we will get multiple + :class:`DraftChangeLogRecord` objects if changing that entity causes + side-effects. See the docstrings for :class:`DraftChangeLogRecord` and + :class:`DraftSideEffect` for more details. .. no_pii: """ @@ -146,97 +164,110 @@ class Meta: class DraftChangeLogRecord(models.Model): """ - A single change in the PublishableEntity that Draft points to. - - Within a single DraftChangeLog, there can be only one DraftChangeLogRecord - per PublishableEntity. If a PublishableEntity goes from v1 -> v2 and then v2 - -> v3 within the same DraftChangeLog, the expectation is that these will be - collapsed into one DraftChangeLogRecord that goes from v1 -> v3. A single - PublishableEntity may have many DraftChangeLogRecords that describe its full - draft edit history, but each DraftChangeLogRecord will be a part of a - different DraftChangeLog. - - New PublishableEntityVersions are created with a monotonically increasing - version_num for their PublishableEntity. However, knowing that is not enough - to accurately reconstruct how the Draft changes over time because the Draft - does not always point to the most recently created PublishableEntityVersion. - We also have the concept of side-effects, where we consider a - PublishableEntity to have changed in some way, even if no new version is - explicitly created. + A single change in the :class:`PublishableEntity` that :class:`Draft` + points to. + + Within a single :class:`DraftChangeLog`, there can be only one + :class:`DraftChangeLogRecord` per :class:`PublishableEntity`. If a + :class:`PublishableEntity` goes from v1 → v2 and then v2 → v3 within + the same :class:`DraftChangeLog`, the expectation is that these will + be collapsed into one :class:`DraftChangeLogRecord` that goes from + v1 → v3. A single :class:`PublishableEntity` may have many + :class:`DraftChangeLogRecord` objects that describe its full draft + edit history, but each :class:`DraftChangeLogRecord` will be a part of + a different :class:`DraftChangeLog`. + + New :class:`PublishableEntityVersion` objects are created with a + monotonically increasing ``version_num`` for their + :class:`PublishableEntity`. However, knowing that is not enough to + accurately reconstruct how the :class:`Draft` changes over time + because the :class:`Draft` does not always point to the most recently + created :class:`PublishableEntityVersion`. We also have the concept + of side-effects, where we consider a :class:`PublishableEntity` to + have changed in some way, even if no new version is explicitly + created. The following scenarios may occur: - Scenario 1: old_version is None, new_version.version_num = 1 - - This is the common case when we're creating the first version for editing. - - Scenario 2: old_version.version_num + 1 == new_version.version_num - - This is the common case when we've made an edit to something, which - creates the next version of an entity, which we then point the Draft at. - - Scenario 3: old_version.version_num >=1, new_version is None - - This is a soft-deletion. We never actually delete a row from the - PublishableEntity model, but set its current Draft version to be None - instead. - - Scenario 4: old_version.version_num > new_version.version_num - - This can happen if we "discard changes", meaning that we call - reset_drafts_to_published(). The versions we created before resetting - don't get deleted, but the Draft model's pointer to the current version - has been reset to match the Published model. - - Scenario 5: old_version.version_num + 1 < new_version.version_num - - Sometimes we'll have a gap between the two version numbers that is > 1. - This can happen if we make edits (new versions) after we called - reset_drafts_to_published. PublishableEntityVersions are created with a - monotonically incrementing version_num which will continue to go up with - the next edit, regardless of whether Draft is pointing to the most - recently created version or not. In terms of (old_version, new version) - changes, it could look like this: - - - (None, v1): Initial creation - - # Publish happens here, so v1 of this PublishableEntity is published. - - (v1, v2): Normal edit in draft - - (v2, v3): Normal edit in draft - - (v3, v1): Reset to published happened here. - - (v1, v4): Normal edit in draft - - This could also technically happen if we change the same entity more than - once in the the same bulk_draft_changes_for() context, thereby putting - them into the same DraftChangeLog, which forces us to squash the changes - together into one DraftChangeLogRecord. - - Scenario 6: old_version is None, new_version > 1 - - This edge case can happen if we soft-deleted a published entity, and then - called reset_drafts_to_published before we published that soft-deletion. - It would effectively undo our soft-delete because the published version - was not yet marked as deleted. - - Scenario 7: old_version == new_version - - This means that the data associated with the Draft version of an entity - has changed purely as a side-effect of some other entity changing. - - The main example we have of this are containers. Imagine that we have a - Unit that is at v1, and has unpinned references to various Components that - are its children. The Unit's version does not get incremented when the - Components are edited, because the Unit container is defined to always get - the most recent version of those Components. We would only make a new - version of the Unit if we changed the metadata of the Unit itself (e.g. - the title), or if we added, removed, or reordered the children. - - Yet updating a Component intuitively changes what we think of as the - content of the Unit. Users who are working on Units also expect that a - change to a Component will be reflected when looking at a Unit's - "last updated" info. The old_version == new_version convention lets us - represent that in a useful way because that Unit *is* a part of the change - set represented by a DraftChangeLog, even if its own versioned data hasn't - changed. + **Scenario 1: old_version is None, new_version.version_num = 1** + + This is the common case when we're creating the first version for + editing. + + **Scenario 2: old_version.version_num + 1 == new_version.version_num** + + This is the common case when we've made an edit to something, which + creates the next version of an entity, which we then point the + :class:`Draft` at. + + **Scenario 3: old_version.version_num >=1, new_version is None** + + This is a soft-deletion. We never actually delete a row from the + :class:`PublishableEntity` model, but set its current :class:`Draft` + version to be None instead. + + **Scenario 4: old_version.version_num > new_version.version_num** + + This can happen if we "discard changes", meaning that we call + ``reset_drafts_to_published()``. The versions we created before + resetting don't get deleted, but the :class:`Draft` model's pointer + to the current version has been reset to match the :class:`Published` + model. + + **Scenario 5: old_version.version_num + 1 < new_version.version_num** + + Sometimes we'll have a gap between the two version numbers that is + > 1. This can happen if we make edits (new versions) after we called + ``reset_drafts_to_published``. :class:`PublishableEntityVersion` + objects are created with a monotonically incrementing ``version_num`` + which will continue to go up with the next edit, regardless of + whether :class:`Draft` is pointing to the most recently created + version or not. In terms of (``old_version``, ``new_version``) + changes, it could look like this: + + - (None, v1): Initial creation + - # Publish happens here, so v1 of this :class:`PublishableEntity` is + published. + - (v1, v2): Normal edit in draft + - (v2, v3): Normal edit in draft + - (v3, v1): Reset to published happened here. + - (v1, v4): Normal edit in draft + + This could also technically happen if we change the same entity more + than once in the the same ``bulk_draft_changes_for()`` context, + thereby putting them into the same :class:`DraftChangeLog`, which + forces us to squash the changes together into one + :class:`DraftChangeLogRecord`. + + **Scenario 6: old_version is None, new_version > 1** + + This edge case can happen if we soft-deleted a published entity, and + then called ``reset_drafts_to_published`` before we published that + soft-deletion. It would effectively undo our soft-delete because the + published version was not yet marked as deleted. + + **Scenario 7: old_version == new_version** + + This means that the data associated with the :class:`Draft` version of + an entity has changed purely as a side-effect of some other entity + changing. + + The main example we have of this are containers. Imagine that we have + a Unit that is at v1, and has unpinned references to various + Components that are its children. The Unit's version does not get + incremented when the Components are edited, because the Unit + container is defined to always get the most recent version of those + Components. We would only make a new version of the Unit if we + changed the metadata of the Unit itself (e.g. the title), or if we + added, removed, or reordered the children. + + Yet updating a Component intuitively changes what we think of as the + content of the Unit. Users who are working on Units also expect that + a change to a Component will be reflected when looking at a Unit's + "last updated" info. The ``old_version == new_version`` convention + lets us represent that in a useful way because that Unit *is* a part + of the change set represented by a :class:`DraftChangeLog`, even if + its own versioned data hasn't changed. .. no_pii: """ @@ -257,21 +288,26 @@ class DraftChangeLogRecord(models.Model): PublishableEntityVersion, on_delete=models.RESTRICT, null=True, blank=True ) - # The dependencies_hash_digest is used when the version alone isn't enough - # to let us know the full draft state of an entity. This happens any time a - # Draft version has dependencies (see the PublishableEntityVersionDependency - # model), because changes in those dependencies will cause changes to the - # state of the Draft. The main example of this is containers, where changing - # an unpinned child affects the state of the parent container, even if that - # container's definition (and thus version) does not change. - # - # If a Draft has no dependencies, then its entire state is captured by its - # version, and the dependencies_hash_digest is blank. (Blank is slightly - # more convenient for database comparisons than NULL.) - # - # Note: There is an equivalent of this field in the Published model and the - # the values may drift away from each other. dependencies_hash_digest = hash_field(blank=True, default='', max_length=8) + """ + The ``dependencies_hash_digest`` is used when the version alone isn't + enough to let us know the full draft state of an entity. This happens + any time a :class:`Draft` version has dependencies (see the + :class:`PublishableEntityVersionDependency` model), because changes in + those dependencies will cause changes to the state of the + :class:`Draft`. The main example of this is containers, where + changing an unpinned child affects the state of the parent container, + even if that container's definition (and thus version) does not + change. + + If a :class:`Draft` has no dependencies, then its entire state is + captured by its version, and the ``dependencies_hash_digest`` is + blank. (Blank is slightly more convenient for database comparisons + than NULL.) + + Note: There is an equivalent of this field in the :class:`Published` + model and the the values may drift away from each other. + """ class Meta: constraints = [ @@ -307,63 +343,74 @@ def __str__(self): class DraftSideEffect(models.Model): """ - Model to track when a change in one Draft affects other Drafts. + Model to track when a change in one :class:`Draft` affects other + :class:`Draft` objects. - Our first use case for this is that changes involving child components are - thought to affect parent Units, even if the parent's version doesn't change. + Our first use case for this is that changes involving child components + are thought to affect parent Units, even if the parent's version + doesn't change. - Side-effects are recorded in a collapsed form that only captures one level. - So if Components C1 and C2 are both changed and they are part of Unit U1, - which is in turn a part of Subsection SS1, then the DraftSideEffect entries - are:: + Side-effects are recorded in a collapsed form that only captures one + level. So if Components C1 and C2 are both changed and they are part + of Unit U1, which is in turn a part of Subsection SS1, then the + :class:`DraftSideEffect` entries are:: (C1, U1) (C2, U1) (U1, SS1) - We do not keep entries for (C1, SS1) or (C2, SS1). This is to make the model - simpler, so we don't have to differentiate between direct side-effects and - transitive side-effects in the model. - - We will record side-effects on a parent container whenever a child changes, - even if the parent container is also changing in the same DraftChangeLog. - The child change is still affecting the parent container, whether the - container happens to be changing for other reasons as well. Whether a parent - -child relationship exists or not depends on the draft state of the - container at the *end* of a bulk_draft_changes_for context. To give concrete - examples: - - Setup: A Unit version U1.v1 has defined C1 to be a child. The current draft - version of C1 is C1.v1. - - Scenario 1: In the a bulk_draft_changes_for context, we edit C1 so that the - draft version of C1 is now C1.v2. Result: - - a DraftChangeLogRecord is created for C1.v1 -> C1.v2 - - a DraftChangeLogRecord is created for U1.v1 -> U1.v1 - - a DraftSideEffect is created with cause (C1.v1 -> C1.v2) and effect - (U1.v1 -> U1.v1). The Unit draft version has not been incremented because - the metadata a Unit defines for itself hasn't been altered, but the Unit - has *changed* in some way because of the side effect of its child being - edited. - - Scenario 2: In a bulk_draft_changes_for context, we edit C1 so that the - draft version of C1 is now C1.v2. In the same context, we edit U1's metadata - so that the draft version of U1 is now U1.v2. U1.v2 still lists C1 as a - child entity. Result: - - a DraftChangeLogRecord is created for C1.v1 -> C1.v2 - - a DraftChangeLogRecord is created for U1.v1 -> U1.v2 - - a DraftSideEffect is created with cause (C1.v1 -> C1.v2) and effect - (U1.v1 -> U1.v2) - - Scenario 3: In a bulk_draft_changes_for context, we edit C1 so that the - draft version of C1 is now C1.v2. In the same context, we edit U1's list of - children so that C1 is no longer a child of U1.v2. Result: - - a DraftChangeLogRecord is created for C1.v1 -> C1.v2 - - a DraftChangeLogRecord is created for U1.v1 -> U1.v2 - - no SideEffect is created, since changing C1 does not have an impact on the - current draft of U1 (U1.v2). A DraftChangeLog is considered a single - atomic operation, so there was never a point at which C1.v1 -> C1.v2 - affected the draft state of U1. + We do not keep entries for ``(C1, SS1)`` or ``(C2, SS1)``. This is to + make the model simpler, so we don't have to differentiate between + direct side-effects and transitive side-effects in the model. + + We will record side-effects on a parent container whenever a child + changes, even if the parent container is also changing in the same + :class:`DraftChangeLog`. The child change is still affecting the + parent container, whether the container happens to be changing for + other reasons as well. Whether a parent-child relationship exists or + not depends on the draft state of the container at the *end* of a + ``bulk_draft_changes_for`` context. To give concrete examples: + + Setup: A Unit version U1.v1 has defined C1 to be a child. The current + draft version of C1 is C1.v1. + + **Scenario 1: In the a bulk_draft_changes_for context, we edit C1 so + that the draft version of C1 is now C1.v2.** + + Result: + + - a :class:`DraftChangeLogRecord` is created for C1.v1 → C1.v2 + - a :class:`DraftChangeLogRecord` is created for U1.v1 → U1.v1 + - a :class:`DraftSideEffect` is created with cause (C1.v1 → C1.v2) and + effect (U1.v1 → U1.v1). The Unit draft version has not been + incremented because the metadata a Unit defines for itself hasn't + been altered, but the Unit has *changed* in some way because of the + side effect of its child being edited. + + **Scenario 2: In a bulk_draft_changes_for context, we edit C1 so that + the draft version of C1 is now C1.v2. In the same context, we edit + U1's metadata so that the draft version of U1 is now U1.v2. U1.v2 + still lists C1 as a child entity.** + + Result: + + - a :class:`DraftChangeLogRecord` is created for C1.v1 → C1.v2 + - a :class:`DraftChangeLogRecord` is created for U1.v1 → U1.v2 + - a :class:`DraftSideEffect` is created with cause (C1.v1 → C1.v2) and + effect (U1.v1 → U1.v2) + + **Scenario 3: In a bulk_draft_changes_for context, we edit C1 so that + the draft version of C1 is now C1.v2. In the same context, we edit + U1's list of children so that C1 is no longer a child of U1.v2.** + + Result: + + - a :class:`DraftChangeLogRecord` is created for C1.v1 → C1.v2 + - a :class:`DraftChangeLogRecord` is created for U1.v1 → U1.v2 + - no SideEffect is created, since changing C1 does not have an impact + on the current draft of U1 (U1.v2). A :class:`DraftChangeLog` is + considered a single atomic operation, so there was never a point at + which C1.v1 → C1.v2 affected the draft state of U1. .. no_pii: """ diff --git a/src/openedx_content/applets/publishing/models/learning_package.py b/src/openedx_content/applets/publishing/models/learning_package.py index 9baf9a5f2..4f18f7b25 100644 --- a/src/openedx_content/applets/publishing/models/learning_package.py +++ b/src/openedx_content/applets/publishing/models/learning_package.py @@ -21,20 +21,21 @@ class LearningPackage(models.Model): """ Top level container for a grouping of authored content. - Each PublishableEntity belongs to exactly one LearningPackage. + Each :class:`PublishableEntity` belongs to exactly one + :class:`LearningPackage`. """ LearningPackageID = NewType("LearningPackageID", int) type ID = LearningPackageID - # Explictly declare a 4-byte ID instead of using the app-default 8-byte ID. - # We do not expect to have more than 2 billion LearningPackages on a given - # site. Furthermore, many, many things have foreign keys to this model and - # uniqueness indexes on those foreign keys + their own fields, so the 4 - # bytes saved will add up over time. - - class IDField(TypedAutoField[ID]): # Note: this is ...AutoField not ...BigAutoField - pass + class IDField(TypedAutoField[ID]): # Note: ...AutoField not ...BigAutoField + """ + Explictly declare a 4-byte ID instead of using the app-default 8-byte + ID. We do not expect to have more than 2 billion LearningPackages on + a given site. Furthermore, many, many things have foreign keys to + this model and uniqueness indexes on those foreign keys + their own + fields, so the 4 bytes saved will add up over time. + """ id = IDField(primary_key=True) @@ -45,15 +46,13 @@ def pk(self) -> ID: uuid = immutable_uuid_field() - # package_ref is an opaque reference string for the LearningPackage. package_ref = ref_field() + """ + An opaque reference string for the :class:`LearningPackage`. + """ title = case_insensitive_char_field(max_length=500, blank=False) - # TODO: We should probably defer this field, since many things pull back - # LearningPackage as select_related. Usually those relations only care about - # the UUID and package_ref, so maybe it makes sense to separate the model at - # some point. description = MultiCollationTextField( blank=True, null=False, @@ -67,6 +66,12 @@ def pk(self) -> ID: "mysql": "utf8mb4_unicode_ci", } ) + """ + TODO: We should probably defer this field, since many things pull back + :class:`LearningPackage` as select_related. Usually those relations only + care about the UUID and ``package_ref``, so maybe it makes sense to + separate the model at some point. + """ created = manual_date_time_field() updated = manual_date_time_field() diff --git a/src/openedx_content/applets/publishing/models/publish_log.py b/src/openedx_content/applets/publishing/models/publish_log.py index c45426cb3..2f0f2a663 100644 --- a/src/openedx_content/applets/publishing/models/publish_log.py +++ b/src/openedx_content/applets/publishing/models/publish_log.py @@ -20,29 +20,33 @@ class PublishLog(models.Model): """ There is one row in this table for every time content is published. - Each PublishLog has 0 or more PublishLogRecords describing exactly which - PublishableEntites were published and what the version changes are. A - PublishLog is like a git commit in that sense, with individual - PublishLogRecords representing the files changed. - - Open question: Empty publishes are allowed at this time, and might be useful - for "fake" publishes that are necessary to invoke other post-publish - actions. It's not clear at this point how useful this will actually be. - - The absence of a ``version_num`` field in this model is intentional, because - having one would potentially cause write contention/locking issues when - there are many people working on different entities in a very large library. - We already see some contention issues occuring in ModuleStore for courses, - and we want to support Libraries that are far larger. - - If you need a LearningPackage-wide indicator for version and the only thing - you care about is "has *something* changed?", you can make a foreign key to - the most recent PublishLog, or use the most recent PublishLog's primary key. - This should be monotonically increasing, though there will be large gaps in - values, e.g. (5, 190, 1291, etc.). Be warned that this value will not port - across sites. If you need site-portability, the UUIDs for this model are a - safer bet, though there's a lot about import/export that we haven't fully - mapped out yet. + Each :class:`PublishLog` has 0 or more :class:`PublishLogRecord` + objects describing exactly which :class:`PublishableEntity` objects + were published and what the version changes are. A :class:`PublishLog` + is like a git commit in that sense, with individual + :class:`PublishLogRecord` objects representing the files changed. + + Open question: Empty publishes are allowed at this time, and might be + useful for "fake" publishes that are necessary to invoke other + post-publish actions. It's not clear at this point how useful this + will actually be. + + The absence of a ``version_num`` field in this model is intentional, + because having one would potentially cause write contention/locking + issues when there are many people working on different entities in a + very large library. We already see some contention issues occuring in + ModuleStore for courses, and we want to support Libraries that are + far larger. + + If you need a :class:`LearningPackage`-wide indicator for version and + the only thing you care about is "has *something* changed?", you can + make a foreign key to the most recent :class:`PublishLog`, or use the + most recent :class:`PublishLog`'s primary key. This should be + monotonically increasing, though there will be large gaps in values, + e.g. (5, 190, 1291, etc.). Be warned that this value will not port + across sites. If you need site-portability, the UUIDs for this model + are a safer bet, though there's a lot about import/export that we + haven't fully mapped out yet. """ uuid = immutable_uuid_field() @@ -65,16 +69,18 @@ class PublishLogRecord(models.Model): """ A record for each publishable entity version changed, for each publish. - To revert a publish, we would make a new publish that swaps ``old_version`` - and ``new_version`` field values. - - If the old_version and new_version of a PublishLogRecord match, it means - that the definition of the entity itself did not change (i.e. no new - PublishableEntityVersion was created), but something else was published that - had the side-effect of changing the published state of this entity. For - instance, if a Unit has unpinned references to its child Components (which - it almost always will), then publishing one of those Components will alter - the published state of the Unit, even if the UnitVersion does not change. + To revert a publish, we would make a new publish that swaps + ``old_version`` and ``new_version`` field values. + + If the ``old_version`` and ``new_version`` of a + :class:`PublishLogRecord` match, it means that the definition of the + entity itself did not change (i.e. no new + :class:`PublishableEntityVersion` was created), but something else was + published that had the side-effect of changing the published state of + this entity. For instance, if a Unit has unpinned references to its + child Components (which it almost always will), then publishing one + of those Components will alter the published state of the Unit, even + if the UnitVersion does not change. """ publish_log = models.ForeignKey( @@ -94,89 +100,101 @@ class PublishLogRecord(models.Model): PublishableEntityVersion, on_delete=models.RESTRICT, null=True, blank=True ) - # The dependencies_hash_digest is used when the version alone isn't enough - # to let us know the full draft state of an entity. This happens any time a - # Published version has dependencies (see the - # PublishableEntityVersionDependency model), because changes in those - # dependencies will cause changes to the state of the Draft. The main - # example of this is containers, where changing an unpinned child affects - # the state of the parent container, even if that container's definition - # (and thus version) does not change. - # - # If a Published version has no dependencies, then its entire state is - # captured by its version, and the dependencies_hash_digest is blank. (Blank - # is slightly more convenient for database comparisons than NULL.) - # - # Note: There is an equivalent of this field in the Draft model and the - # the values may drift away from each other. dependencies_hash_digest = hash_field(blank=True, default='', max_length=8) + """ + The ``dependencies_hash_digest`` is used when the version alone isn't + enough to let us know the full draft state of an entity. This happens + any time a :class:`Published` version has dependencies (see the + :class:`PublishableEntityVersionDependency` model), because changes in + those dependencies will cause changes to the state of the + :class:`Draft`. The main example of this is containers, where + changing an unpinned child affects the state of the parent container, + even if that container's definition (and thus version) does not + change. + + If a :class:`Published` version has no dependencies, then its entire + state is captured by its version, and the ``dependencies_hash_digest`` + is blank. (Blank is slightly more convenient for database comparisons + than NULL.) + + Note: There is an equivalent of this field in the :class:`Draft` model + and the the values may drift away from each other. + """ - # The "direct" field captures user intent during the publishing process. It - # is True if the user explicitly requested to publish the entity represented - # by this PublishLogRecord—i.e. they clicked "publish" on this entity or - # selected it for bulk publish. - # - # This field is False if this entity was indirectly published either as a - # child/dependency or side-effect of a directly published entity. - # - # If this field is None, that means that this PublishLogRecord was created - # before we started capturing user intent (pre-Verawood release), and we - # cannot reliably infer what the user clicked on. For example, say we had a - # Subsection > Unit > Component arrangement where the Component had an - # unpublished change. The user is allowed to press the "publish" button at - # the Subsection, Unit, or Component levels in the UI. Before we started - # recording this field, the resulting PublishLogs would have looked - # identical in all three cases: a version change PublishLogRecord for - # Component, and side-effect records for the Unit and Subsection. Therefore, - # we cannot accurately backfill this field. - # - # Here are some examples to illustrate how "direct" gets set and why: - # - # Example 1: The user clicks "publish" on a Component that's in a Unit. - # - # The Component has direct=True, but the side-effect PublishLogRecord for - # the Unit has direct=False. Likewise, any side-effect records at higher - # levels (subsection, section) also have direct=False. - # - # Example 2: The user clicks "publish" on a Unit, where both the Unit and - # Component have unpublished changes: - # - # In this case, the Unit has direct=True, and the Component has - # direct=False. The draft status of the Component is irrelevant. The user - # asked for the Unit to the published, so the Unit's PublishLogRecord is - # the only thing that gets direct=True. - # - # Example 3: The user clicks "publish" on a Unit that has no changes of its - # own (draft version == published version), but the Unit contains a Component - # that has changes. - # - # Again, only the PublishLogRecord for the Unit has direct=True. The - # Component's PublishLogRecord has direct=False. Even though the Unit's - # published version_num does not change (i.e. it is purely a side-effect - # publish), the user intent was to publish the Unit (and anything it - # contains), so the Unit gets direct=True. - # - # Example 4: The user selects multiple entities for bulk publishing. - # - # Those exact entities that the user selected get direct=True. It does not - # matter if some of those entities are children of other selected items or - # not. Other entries like dependencies or side-effects have direct=False. - # - # Example 5: The user selects "publish all". - # - # Selecting "publish all" in our system currently translates into "publish - # all the entities that have a draft version that is different from its - # published version". Those entities would get PublishLogRecords with - # direct=True, while all side-effects would get records with direct=False. - # So if a Unit's draft and published versions match, and one of its - # Components has unpublished changes, then "publish all" would cause the - # Component's record to have direct=True and the Unit's record to have - # direct=False. direct = models.BooleanField( null=True, blank=True, default=False, ) + """ + The ``direct`` field captures user intent during the publishing + process. It is True if the user explicitly requested to publish the + entity represented by this :class:`PublishLogRecord`—i.e. they + clicked "publish" on this entity or selected it for bulk publish. + + This field is False if this entity was indirectly published either as + a child/dependency or side-effect of a directly published entity. + + If this field is None, that means that this :class:`PublishLogRecord` + was created before we started capturing user intent (pre-Verawood + release), and we cannot reliably infer what the user clicked on. For + example, say we had a Subsection > Unit > Component arrangement where + the Component had an unpublished change. The user is allowed to press + the "publish" button at the Subsection, Unit, or Component levels in + the UI. Before we started recording this field, the resulting + :class:`PublishLog` objects would have looked identical in all three + cases: a version change :class:`PublishLogRecord` for Component, and + side-effect records for the Unit and Subsection. Therefore, we cannot + accurately backfill this field. + + Here are some examples to illustrate how ``direct`` gets set and why: + + **Example 1: The user clicks "publish" on a Component that's in a + Unit.** + + The Component has ``direct=True``, but the side-effect + :class:`PublishLogRecord` for the Unit has ``direct=False``. + Likewise, any side-effect records at higher levels (subsection, + section) also have ``direct=False``. + + **Example 2: The user clicks "publish" on a Unit, where both the Unit + and Component have unpublished changes.** + + In this case, the Unit has ``direct=True``, and the Component has + ``direct=False``. The draft status of the Component is irrelevant. + The user asked for the Unit to the published, so the Unit's + :class:`PublishLogRecord` is the only thing that gets ``direct=True``. + + **Example 3: The user clicks "publish" on a Unit that has no changes + of its own (draft version == published version), but the Unit + contains a Component that has changes.** + + Again, only the :class:`PublishLogRecord` for the Unit has + ``direct=True``. The Component's :class:`PublishLogRecord` has + ``direct=False``. Even though the Unit's published ``version_num`` + does not change (i.e. it is purely a side-effect publish), the user + intent was to publish the Unit (and anything it contains), so the + Unit gets ``direct=True``. + + **Example 4: The user selects multiple entities for bulk publishing.** + + Those exact entities that the user selected get ``direct=True``. It + does not matter if some of those entities are children of other + selected items or not. Other entries like dependencies or + side-effects have ``direct=False``. + + **Example 5: The user selects "publish all".** + + Selecting "publish all" in our system currently translates into + "publish all the entities that have a draft version that is different + from its published version". Those entities would get + :class:`PublishLogRecord` objects with ``direct=True``, while all + side-effects would get records with ``direct=False``. So if a Unit's + draft and published versions match, and one of its Components has + unpublished changes, then "publish all" would cause the Component's + record to have ``direct=True`` and the Unit's record to have + ``direct=False``. + """ class Meta: constraints = [ @@ -215,22 +233,24 @@ class Published(models.Model): Notes: - * There is only ever one published PublishableEntityVersion per - PublishableEntity at any given time. - * It may be possible for a PublishableEntity to exist only as a Draft (and thus - not show up in this table). - * If a row exists for a PublishableEntity, but the ``version`` field is - None, it means that the entity was published at some point, but is no - longer published now–i.e. it's functionally "deleted", even though all - the version history is preserved behind the scenes. - - TODO: Do we need to create a (redundant) title field in this model so that - we can more efficiently search across titles within a LearningPackage? - Probably not an immediate concern because the number of rows currently - shouldn't be > 10,000 in the more extreme cases. - - TODO: Do we need to make a "most_recently" published version when an entry - is unpublished/deleted? + * There is only ever one published :class:`PublishableEntityVersion` + per :class:`PublishableEntity` at any given time. + * It may be possible for a :class:`PublishableEntity` to exist only as + a :class:`Draft` (and thus not show up in this table). + * If a row exists for a :class:`PublishableEntity`, but the + ``version`` field is None, it means that the entity was published + at some point, but is no longer published now–i.e. it's functionally + "deleted", even though all the version history is preserved behind + the scenes. + + TODO: Do we need to create a (redundant) title field in this model so + that we can more efficiently search across titles within a + :class:`LearningPackage`? Probably not an immediate concern because + the number of rows currently shouldn't be > 10,000 in the more + extreme cases. + + TODO: Do we need to make a "most_recently" published version when an + entry is unpublished/deleted? """ entity = models.OneToOneField( @@ -259,23 +279,25 @@ class Meta: class PublishSideEffect(models.Model): """ - Model to track when a change in one Published entity affects others. + Model to track when a change in one :class:`Published` entity affects + others. - Our first use case for this is that changes involving child components are - thought to affect parent Units, even if the parent's version doesn't change. + Our first use case for this is that changes involving child components + are thought to affect parent Units, even if the parent's version + doesn't change. - Side-effects are recorded in a collapsed form that only captures one level. - So if Components C1 and C2 are both published and they are part of Unit U1, - which is in turn a part of Subsection SS1, then the PublishSideEffect - entries are:: + Side-effects are recorded in a collapsed form that only captures one + level. So if Components C1 and C2 are both published and they are + part of Unit U1, which is in turn a part of Subsection SS1, then the + :class:`PublishSideEffect` entries are:: (C1, U1) (C2, U1) (U1, SS1) - We do not keep entries for (C1, SS1) or (C2, SS1). This is to make the model - simpler, so we don't have to differentiate between direct side-effects and - transitive side-effects in the model. + We do not keep entries for ``(C1, SS1)`` or ``(C2, SS1)``. This is to + make the model simpler, so we don't have to differentiate between + direct side-effects and transitive side-effects in the model. .. no_pii: """ diff --git a/src/openedx_content/applets/publishing/models/publishable_entity.py b/src/openedx_content/applets/publishing/models/publishable_entity.py index 106d88859..5e16d3fe3 100644 --- a/src/openedx_content/applets/publishing/models/publishable_entity.py +++ b/src/openedx_content/applets/publishing/models/publishable_entity.py @@ -30,47 +30,51 @@ class PublishableEntity(models.Model): """ This represents any publishable thing that has ever existed in a - LearningPackage. It serves as a stable model that will not go away even if - these things are later unpublished or deleted. + :class:`LearningPackage`. It serves as a stable model that will not go + away even if these things are later unpublished or deleted. - A PublishableEntity belongs to exactly one LearningPackage. + A :class:`PublishableEntity` belongs to exactly one + :class:`LearningPackage`. Examples of Publishable Entities -------------------------------- - Components (e.g. VideoBlock, ProblemBlock), Units, and Sections/Subsections - would all be considered Publishable Entites. But anything that can be - imported, exported, published, and reverted in a course or library could be - modeled as a PublishableEntity, including things like Grading Policy or - possibly Taxonomies (?). + :class:`Component` objects (e.g. VideoBlock, ProblemBlock), Units, and + Sections/Subsections would all be considered Publishable Entites. But + anything that can be imported, exported, published, and reverted in a + course or library could be modeled as a :class:`PublishableEntity`, + including things like Grading Policy or possibly Taxonomies (?). How to use this model --------------------- - The publishing app understands that publishable entities exist, along with - their drafts and published versions. It has some basic metadata, such as - identifiers, who created it, and when it was created. It's meant to - encapsulate the draft and publishing related aspects of your content, but - the ``publishing`` app doesn't know anything about the actual content being - referenced. + The publishing app understands that publishable entities exist, along + with their drafts and published versions. It has some basic metadata, + such as identifiers, who created it, and when it was created. It's + meant to encapsulate the draft and publishing related aspects of your + content, but the :mod:`publishing` app doesn't know anything about + the actual content being referenced. - You have to provide actual meaning to PublishableEntity by creating your own - models that will represent your particular content and associating them to - PublishableEntity via a OneToOneField with primary_key=True. The easiest way - to do this is to have your model inherit from PublishableEntityMixin. + You have to provide actual meaning to :class:`PublishableEntity` by + creating your own models that will represent your particular content + and associating them to :class:`PublishableEntity` via a + ``OneToOneField`` with ``primary_key=True``. The easiest way to do + this is to have your model inherit from :class:`PublishableEntityMixin`. Identifiers ----------- The UUID is globally unique and should be treated as immutable. The key field *is* mutable, but changing it will affect all - PublishedEntityVersions. They are locally unique within the LearningPackage. + :class:`PublishableEntityVersion` objects. They are locally unique + within the :class:`LearningPackage`. If you are referencing this model from within the same process, use a - foreign key to the id. If you are referencing this PublishedEntity from an - external system/service, use the UUID. The key is the part that is most - likely to be human-readable, and may be exported/copied, but try not to rely - on it, since this value may change. + foreign key to the id. If you are referencing this + :class:`PublishableEntity` from an external system/service, use the + UUID. The key is the part that is most likely to be human-readable, + and may be exported/copied, but try not to rely on it, since this + value may change. Note: When we actually implement the ability to change identifiers, we should make a history table and a modified attribute on this model. @@ -78,39 +82,43 @@ class PublishableEntity(models.Model): Why are Identifiers in this Model? ---------------------------------- - A PublishableEntity never stands alone–it's always intended to be used with - a 1:1 model like Component or Unit. So why have all the identifiers in this - model instead of storing them in those other models? Two reasons: + A :class:`PublishableEntity` never stands alone–it's always intended + to be used with a 1:1 model like :class:`Component` or Unit. So why + have all the identifiers in this model instead of storing them in + those other models? Two reasons: - * Published things need to have the right identifiers so they can be used - throughout the system, and the UUID is serving the role of ISBN in physical - book publishing. - * We want to be able to enforce the idea that "entity_ref" is locally unique across - all PublishableEntities within a given LearningPackage. Component and Unit - can't do that without a shared model. + * Published things need to have the right identifiers so they can be + used throughout the system, and the UUID is serving the role of + ISBN in physical book publishing. + * We want to be able to enforce the idea that ``entity_ref`` is + locally unique across all :class:`PublishableEntity` objects within + a given :class:`LearningPackage`. :class:`Component` and Unit can't + do that without a shared model. - That being said, models that build on PublishableEntity are free to add - their own identifiers if it's useful to do so. + That being said, models that build on :class:`PublishableEntity` are + free to add their own identifiers if it's useful to do so. Why not Inherit from this Model? -------------------------------- - Django supports multi-table inheritance: - - https://docs.djangoproject.com/en/4.2/topics/db/models/#multi-table-inheritance + Django supports `multi-table inheritance + `_. We don't use that, primarily because we want to more clearly decouple - publishing concerns from the rest of the logic around Components, Units, - etc. If you made a Component and ComponentVersion models that subclassed - PublishableEntity and PublishableEntityVersion, and then accessed - ``component.versions``, you might expect ComponentVersions to come back and - be surprised when you get EntityVersions instead. + publishing concerns from the rest of the logic around + :class:`Component` objects, Units, etc. If you made a + :class:`Component` and :class:`ComponentVersion` models that + subclassed :class:`PublishableEntity` and + :class:`PublishableEntityVersion`, and then accessed + ``component.versions``, you might expect :class:`ComponentVersion` + objects to come back and be surprised when you get EntityVersions + instead. In general, we want freedom to add new Publishing models, fields, and - methods without having to worry about the downstream name collisions with - other apps (many of which might live in other repositories). The helper - mixins will provide a little syntactic sugar to make common access patterns - more convenient, like file access. + methods without having to worry about the downstream name collisions + with other apps (many of which might live in other repositories). + The helper mixins will provide a little syntactic sugar to make + common access patterns more convenient, like file access. """ PublishableEntityID = NewType("PublishableEntityID", int) @@ -128,11 +136,13 @@ class IDField(TypedBigAutoField[ID]): # Boilerplate for fully-typed ID field. related_name="publishable_entities", ) - # entity_ref is an opaque reference string assigned by the creator of this - # entity (e.g. derived from component_type + component_code for Components). - # Consumers must treat it as an atomic string — do not parse or reconstruct - # it. entity_ref = ref_field() + """ + ``entity_ref`` is an opaque reference string assigned by the creator of + this entity (e.g. derived from ``component_type + component_code`` for + :class:`Component` objects). Consumers must treat it as an atomic + string — do not parse or reconstruct it. + """ created = manual_date_time_field() created_by = models.ForeignKey( @@ -189,23 +199,25 @@ def __str__(self): class PublishableEntityVersion(models.Model): """ - A particular version of a PublishableEntity. + A particular version of a :class:`PublishableEntity`. - This model has its own ``uuid`` so that it can be referenced directly. The - ``uuid`` should be treated as immutable. + This model has its own ``uuid`` so that it can be referenced directly. + The ``uuid`` should be treated as immutable. - PublishableEntityVersions are created once and never updated. So for - instance, the ``title`` should never be modified. + :class:`PublishableEntityVersion` objects are created once and never + updated. So for instance, the ``title`` should never be modified. - Like PublishableEntity, the data in this model is only enough to cover the - parts that are most important for the actual process of managing drafts and - publishes. You will want to create your own models to represent the actual - content data that's associated with this PublishableEntityVersion, and - connect them using a OneToOneField with primary_key=True. The easiest way to - do this is to inherit from PublishableEntityVersionMixin. Be sure to treat - these versioned models in your app as immutable as well. + Like :class:`PublishableEntity`, the data in this model is only enough + to cover the parts that are most important for the actual process of + managing drafts and publishes. You will want to create your own + models to represent the actual content data that's associated with + this :class:`PublishableEntityVersion`, and connect them using a + ``OneToOneField`` with ``primary_key=True``. The easiest way to do + this is to inherit from :class:`PublishableEntityVersionMixin`. Be + sure to treat these versioned models in your app as immutable as + well. - .. no_pii + .. no_pii: """ uuid = immutable_uuid_field() @@ -213,34 +225,43 @@ class PublishableEntityVersion(models.Model): PublishableEntity, on_delete=models.CASCADE, related_name="versions" ) - # Most publishable things will have some sort of title, but blanks are - # allowed for those that don't require one. title = case_insensitive_char_field(max_length=500, blank=True, default="") + """ + Most publishable things will have some sort of title, but blanks are + allowed for those that don't require one. + """ - # The version_num starts at 1 and increments by 1 with each new version for - # a given PublishableEntity. Doing it this way makes it more convenient for - # users to refer to than a hash or UUID value. It also helps us catch race - # conditions on save, by setting a unique constraint on the entity and - # version_num. version_num = models.PositiveIntegerField( null=False, validators=[MinValueValidator(1)], ) + """ + The ``version_num`` starts at 1 and increments by 1 with each new + version for a given :class:`PublishableEntity`. Doing it this way + makes it more convenient for users to refer to than a hash or UUID + value. It also helps us catch race conditions on save, by setting a + unique constraint on the entity and ``version_num``. + """ - # All PublishableEntityVersions created as part of the same publish should - # have the exact same created datetime (not off by a handful of - # microseconds). created = manual_date_time_field() + """ + All :class:`PublishableEntityVersion` objects created as part of the + same publish should have the exact same created datetime (not off by + a handful of microseconds). + """ - # User who created the PublishableEntityVersion. This can be null if the - # user is later removed. Open edX in general doesn't let you remove users, - # but we should try to model it so that this is possible eventually. created_by = models.ForeignKey( settings.AUTH_USER_MODEL, on_delete=models.SET_NULL, null=True, blank=True, ) + """ + User who created the :class:`PublishableEntityVersion`. This can be + null if the user is later removed. Open edX in general doesn't let + you remove users, but we should try to model it so that this is + possible eventually. + """ dependencies: models.ManyToManyField[ PublishableEntity, PublishableEntityVersionDependency @@ -296,27 +317,29 @@ class Meta: class PublishableEntityVersionDependency(models.Model): """ - Track the PublishableEntities that a PublishableEntityVersion depends on. + Track the :class:`PublishableEntity` objects that a + :class:`PublishableEntityVersion` depends on. - For example, a partcular version of a Unit (U1.v1) might be defined to have - unpinned references to Components C1 and C2. That means that any changes in - C1 or C2 will affect U1.v1 via DraftSideEffects and PublishedSideEffects. We - say that C1 and C2 are dependencies of U1.v1. + For example, a partcular version of a Unit (U1.v1) might be defined to + have unpinned references to Components C1 and C2. That means that any + changes in C1 or C2 will affect U1.v1 via :class:`DraftSideEffect` and + :class:`PublishSideEffect`. We say that C1 and C2 are dependencies of + U1.v1. - An important restriction is that a PublishableEntityVersion's list of - dependencies are defined when the version is created. It is not modified - after that. No matter what happens to C1 or C2 (e.g. edit, deletion, - un-deletion, reset-draft-version-to-published), they will always be - dependencies of U1.v1. + An important restriction is that a :class:`PublishableEntityVersion`'s + list of dependencies are defined when the version is created. It is + not modified after that. No matter what happens to C1 or C2 (e.g. + edit, deletion, un-deletion, reset-draft-version-to-published), they + will always be dependencies of U1.v1. - If someone removes C2 from U1, then that requires creating a new version of - U1 (so U1.v2). + If someone removes C2 from U1, then that requires creating a new + version of U1 (so U1.v2). - This restriction is important because our ability to calculate and cache the - state of "this version of this publishable entity and all its dependencies - (children)" relies on this being true. + This restriction is important because our ability to calculate and + cache the state of "this version of this publishable entity and all + its dependencies (children)" relies on this being true. - .. no_pii + .. no_pii: """ referring_version = models.ForeignKey(PublishableEntityVersion, on_delete=models.CASCADE) referenced_entity = models.ForeignKey(PublishableEntity, on_delete=models.RESTRICT) @@ -332,13 +355,13 @@ class Meta: class PublishableEntityMixin(models.Model): """ - Convenience mixin to link your models against PublishableEntity. + Convenience mixin to link your models against :class:`PublishableEntity`. - Please see docstring for PublishableEntity for more details. + Please see docstring for :class:`PublishableEntity` for more details. - If you use this class, you *MUST* also use PublishableEntityVersionMixin and - the publishing app's api.register_publishable_models (see its docstring for - details). + If you use this class, you *MUST* also use + :class:`PublishableEntityVersionMixin` and the publishing app's + ``api.register_publishable_models`` (see its docstring for details). """ # select these related entities by default for all queries objects: ClassVar[WithRelationsManager[Self]] = WithRelationsManager( @@ -389,27 +412,29 @@ class VersioningHelper: """ Helper class to link content models to their versions. - The publishing app has PublishableEntity and PublishableEntityVersion. - This is a helper class so that if you mix PublishableEntityMixin into - a content model like Component, then you can do something like:: + The publishing app has :class:`PublishableEntity` and + :class:`PublishableEntityVersion`. This is a helper class so that + if you mix :class:`PublishableEntityMixin` into a content model + like :class:`Component`, then you can do something like:: component.versioning.draft # current draft ComponentVersion component.versioning.published # current published ComponentVersion - It links the relationships between content models and their versioned - counterparts *through* the connection between PublishableEntity and - PublishableEntityVersion. So ``component.versioning.draft`` ends up - querying: Component -> PublishableEntity -> Draft -> - PublishableEntityVersion -> ComponentVersion. But the people writing - Component don't need to understand how the publishing models work to do - these common queries. + It links the relationships between content models and their + versioned counterparts *through* the connection between + :class:`PublishableEntity` and :class:`PublishableEntityVersion`. + So ``component.versioning.draft`` ends up querying: + :class:`Component` → :class:`PublishableEntity` → :class:`Draft` → + :class:`PublishableEntityVersion` → :class:`ComponentVersion`. But + the people writing :class:`Component` don't need to understand + how the publishing models work to do these common queries. Caching Warning --------------- - Note that because we're just using the underlying model's relations, - calling this a second time will returned the cached relation, and - not cause a fetch of new data from the database. So for instance, if - you do:: + Note that because we're just using the underlying model's + relations, calling this a second time will returned the cached + relation, and not cause a fetch of new data from the database. So + for instance, if you do:: # Create a new Component + ComponentVersion component, component_version = create_component_and_version( @@ -475,10 +500,11 @@ def __init__(self, content_obj): def _content_obj_version(self, pub_ent_version: PublishableEntityVersion | None): """ - PublishableEntityVersion -> Content object version + :class:`PublishableEntityVersion` → Content object version. - Given a reference to a PublishableEntityVersion, return the version - of the content object that we've been mixed into. + Given a reference to a :class:`PublishableEntityVersion`, + return the version of the content object that we've been + mixed into. """ if pub_ent_version is None: return None @@ -492,13 +518,14 @@ def draft(self): """ Return the content version object that is the current draft. - So if you mix ``PublishableEntityMixin`` into ``Component``, then - ``component.versioning.draft`` will return you the - ``ComponentVersion`` that is the current draft (not the underlying - ``PublishableEntityVersion``). + So if you mix :class:`PublishableEntityMixin` into + :class:`Component`, then ``component.versioning.draft`` will + return you the :class:`ComponentVersion` that is the current + draft (not the underlying :class:`PublishableEntityVersion`). - If this is causing many queries, it might be the case that you need - to add ``select_related('publishable_entity__draft__version')`` to + If this is causing many queries, it might be the case that + you need to add + ``select_related('publishable_entity__draft__version')`` to the queryset. """ # Check if there's an entry in Drafts, i.e. has there ever been a @@ -533,13 +560,15 @@ def published(self): """ Return the content version object that is currently published. - So if you mix ``PublishableEntityMixin`` into ``Component``, then - ``component.versioning.published`` will return you the - ``ComponentVersion`` that is currently published (not the underlying - ``PublishableEntityVersion``). + So if you mix :class:`PublishableEntityMixin` into + :class:`Component`, then ``component.versioning.published`` + will return you the :class:`ComponentVersion` that is + currently published (not the underlying + :class:`PublishableEntityVersion`). - If this is causing many queries, it might be the case that you need - to add ``select_related('publishable_entity__published__version')`` + If this is causing many queries, it might be the case that + you need to add + ``select_related('publishable_entity__published__version')`` to the queryset. """ # Check if there's an entry in Published, i.e. has there ever been a @@ -584,7 +613,7 @@ def has_unpublished_changes(self): @property def last_publish_log(self): """ - Return the most recent PublishLog for this component. + Return the most recent :class:`PublishLog` for this component. Return None if the component is not published. """ @@ -596,10 +625,12 @@ def last_publish_log(self): @property def versions(self): """ - Return a QuerySet of content version models for this content model. + Return a QuerySet of content version models for this content + model. - Example: If you mix PublishableEntityMixin into a Component model, - This would return you a QuerySet of ComponentVersion models. + Example: If you mix :class:`PublishableEntityMixin` into a + :class:`Component` model, this would return you a QuerySet of + :class:`ComponentVersion` models. """ pub_ent = self.content_obj.publishable_entity return self.content_version_model_cls.objects.filter( @@ -619,13 +650,15 @@ def version_num(self, version_num): class PublishableEntityVersionMixin(models.Model): """ - Convenience mixin to link your models against PublishableEntityVersion. + Convenience mixin to link your models against + :class:`PublishableEntityVersion`. - Please see docstring for PublishableEntityVersion for more details. + Please see docstring for :class:`PublishableEntityVersion` for more + details. - If you use this class, you *MUST* also use PublishableEntityMixin and the - publishing app's api.register_publishable_models (see its docstring for - details). + If you use this class, you *MUST* also use + :class:`PublishableEntityMixin` and the publishing app's + ``api.register_publishable_models`` (see its docstring for details). """ # select these related entities by default for all queries @@ -666,7 +699,8 @@ class Meta: class PublishableContentModelRegistry: """ - This class tracks content models built on PublishableEntity(Version). + This class tracks content models built on :class:`PublishableEntity` + (and :class:`PublishableEntityVersion`). """ _unversioned_to_versioned: dict[type[PublishableEntityMixin], type[PublishableEntityVersionMixin]] = {} diff --git a/src/openedx_content/applets/sections/models.py b/src/openedx_content/applets/sections/models.py index 53e648652..3b4caaf1b 100644 --- a/src/openedx_content/applets/sections/models.py +++ b/src/openedx_content/applets/sections/models.py @@ -21,10 +21,11 @@ @Container.register_subclass class Section(Container): """ - A Section is type of Container that holds Subsections. + A Section is type of :class:`Container` that holds :class:`Subsection` + children. - Via Container and its PublishableEntityMixin, Sections are also publishable - entities and can be added to other containers. + Via :class:`Container` and its :class:`PublishableEntityMixin`, Sections + are also publishable entities and can be added to other containers. """ SectionID = NewType("SectionID", Container.ID) @@ -57,10 +58,11 @@ def validate_entity(cls, entity: PublishableEntity) -> None: class SectionVersion(ContainerVersion): """ - A SectionVersion is a specific version of a Section. + A SectionVersion is a specific version of a :class:`Section`. - Via ContainerVersion and its EntityList, it defines the list of Subsections - in this version of the Section. + Via :class:`ContainerVersion` and its :class:`EntityList`, it defines the + list of :class:`Subsection` children in this version of the + :class:`Section`. """ container_version = models.OneToOneField( diff --git a/src/openedx_content/applets/subsections/models.py b/src/openedx_content/applets/subsections/models.py index 7fd8c52ba..3b3dba567 100644 --- a/src/openedx_content/applets/subsections/models.py +++ b/src/openedx_content/applets/subsections/models.py @@ -21,10 +21,12 @@ @Container.register_subclass class Subsection(Container): """ - A Subsection is type of Container that holds Units. + A Subsection is type of :class:`Container` that holds :class:`Unit` + children. - Via Container and its PublishableEntityMixin, Subsections are also publishable - entities and can be added to other containers. + Via :class:`Container` and its :class:`PublishableEntityMixin`, + Subsections are also publishable entities and can be added to other + containers. """ SubsectionID = NewType("SubsectionID", Container.ID) @@ -57,10 +59,11 @@ def validate_entity(cls, entity: PublishableEntity) -> None: class SubsectionVersion(ContainerVersion): """ - A SubsectionVersion is a specific version of a Subsection. + A SubsectionVersion is a specific version of a :class:`Subsection`. - Via ContainerVersion and its EntityList, it defines the list of Units - in this version of the Subsection. + Via :class:`ContainerVersion` and its :class:`EntityList`, it defines the + list of :class:`Unit` children in this version of the + :class:`Subsection`. """ container_version = models.OneToOneField( diff --git a/src/openedx_content/applets/units/models.py b/src/openedx_content/applets/units/models.py index 109fb38b9..a54c69ea2 100644 --- a/src/openedx_content/applets/units/models.py +++ b/src/openedx_content/applets/units/models.py @@ -20,10 +20,11 @@ @Container.register_subclass class Unit(Container): """ - A Unit is type of Container that holds Components. + A Unit is type of :class:`Container` that holds :class:`Component` + children. - Via Container and its PublishableEntityMixin, Units are also publishable - entities and can be added to other containers. + Via :class:`Container` and its :class:`PublishableEntityMixin`, Units are + also publishable entities and can be added to other containers. """ UnitID = NewType("UnitID", Container.ID) @@ -54,10 +55,10 @@ def validate_entity(cls, entity: PublishableEntity) -> None: class UnitVersion(ContainerVersion): """ - A UnitVersion is a specific version of a Unit. + A UnitVersion is a specific version of a :class:`Unit`. - Via ContainerVersion and its EntityList, it defines the list of Components - in this version of the Unit. + Via :class:`ContainerVersion` and its :class:`EntityList`, it defines the + list of :class:`Component` children in this version of the :class:`Unit`. """ container_version = models.OneToOneField( From 0ca57bd4bc429b2b7ebdd5e80455b07c3a71559b Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 5 May 2026 01:37:10 -0400 Subject: [PATCH 2/2] docs: fix build --- src/openedx_content/applets/media/models.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/openedx_content/applets/media/models.py b/src/openedx_content/applets/media/models.py index 233421a1d..692cb7884 100644 --- a/src/openedx_content/applets/media/models.py +++ b/src/openedx_content/applets/media/models.py @@ -278,11 +278,10 @@ class Media(models.Model): this could be as much as 200K of data if we had nothing but emojis. """ + # Custom type for our primary key, to make it more type-safe when using + # in API calls. (Note: This can't be a docstring comment, because that + # breaks sphinxcontrib-django, which doesn't handle NewType properly). MediaID = NewType("MediaID", int) - """ - Custom type for our primary key, to make it more type-safe when using - in API calls. - """ type ID = MediaID class IDField(TypedBigAutoField[ID]): # Boilerplate for fully-typed ID field.