docs: ADRs for modeling containers capability

[mariajgrimaldi]

Description

ADRs with high-level decisions regarding the modeling of containers as a generalized capability of holding content, selectors as a strategy for selecting content that builds on the container idea, and units as a concrete implementation of containers that can contain static and dynamic content through selectors.

These decisions resulted from discussions in #38 and #219, which were consolidated into issue #240.

In order to validate the proposal, we created a POC implementation #254 of what has been discussed so far. These ADRs and the POC are still a work in progress, but early reviews are welcome to validate the proposal.

[openedx-webhooks]

Thanks for the pull request, @mariajgrimaldi!

This repository is currently maintained by @axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[mariajgrimaldi]

Should these decisions be part of this ADR as well?

• Unit members can exist as standalone items outside the unit.
• Members should indicate which unit they belong to.
• Members can be duplicated from units but keeping original references.

[mariajgrimaldi]

Should these decisions be part of this ADR as well?

• All members within the container should be published before the container can be reused.

[mariajgrimaldi]

Question:
What I'm still missing here is the behavior of publishing containers when they're being reused somewhere and modified in some way. I understand we discussed this in our last meeting, but it's still not clear to me what our approach should be from the modeling point of view.

[bradenmacdonald]

Thanks, I'm finding this to be a nice clear explanation in many places, but there are some things I'm confused by and I think we need to clarify them in the ADR.

[bradenmacdonald]

I'm not really clear on what this third line about shared definitions means. Maybe an example would help?

[mariajgrimaldi]

I think I explained it better in this comment: #251 (comment). I was referring to a Django application where the APIs and mixins that other container types would be built on.

[bradenmacdonald]

Suggested change

	- Each container holds different states of its members (user-defined, initial, and frozen final state) to support rollback operations.
	- Members can be added or removed from a container, and the container will maintain the state of the content for the previous version (frozen final state).
	- The initial state of a container is immutable.
	- Each container holds different states of its members (user-defined, initial, and frozen final state) to support rollback operations.
	- The initial state of a container is immutable.
	- Members can be added or removed from a container, and the container will maintain the state of the content for the previous version (frozen final state).

I was confused what "the initial state is immutable" meant until I realized that "initial state" is one of three "states" mentioned earlier, and doesn't mean "the first version of a container"

[mariajgrimaldi]

Now, it mentions the initial list of members instead of "state".

[bradenmacdonald]

I think an expanded description of these three states would be helpful. From this I'm not totally clear on what's the difference between initial and user-defined. And why "initial" is immutable but "frozen" is not (?) even though it sounds like it should be from the name.

[mariajgrimaldi]

For the expanded descriptions, I created a separate section for "Container States," grouping all the state management decisions instead: af7dce4. I think now each definition is clear according to what was proposed in the model sketches.

EDIT: it's now called container history and it'd probably keep changing.

[mariajgrimaldi]

why "initial" is immutable but "frozen" is not (?) even though it sounds like it should be from the name

I understand frozen lists are "this is how the container looked (a screenshot?) when a new version was created". So, if there's a unit U0 with components defined_list = P0latest, P1latest, P2latest all pointing to their latest versions at the time of the next container creation where frozen_list would be None and initial_list = P0, P1, P2 where P0, P1, and P2 are the pinned versions of P0latest and so on at moment of creation. When U1 is created, defined_list looks like defined_list = P0latest, P1latest, P2latest so frozen_list would be frozen_list = P0', P1'', P2' (the pinned versions of P0latest and so on). New versions for P0,...,1 are created when U1 defined_list would point to those, so we'd need a way of knowing what versions were in U1 in case we want to go back to it.

Frozen lists are useful when there are unpinned versions of members, since when they're all pinned, then defined_list = initial_list = frozen_list.

These in-line comments Dave left in #240 might explain this better: https://github.com/openedx/openedx-learning/pull/254/files#diff-6f2c589dc4ba5960e91d39f6488eb5e2e2e63ddaff63a75909091c760b877802R112-R154

[bradenmacdonald]

Thanks for expanding the descriptions, but I'm still confused :/

The code seems to call these "lists" not "states" - it would be good to pick a consistent term.

The user-defined state of a container is the state that the author has defined for the version of the container

I don't think we should call this "user-defined" when it's actually "author-defined." To me, "user-defined" sounds more like "the version of this container that a specific user will see (after we've accounted for groups, A/B testing, randomized content, exam permissions, etc.). But "author-defined" is clear.

• The initial state of a container is the state of the container when it was first created.

# inital_list is an EntityList where all the versions are pinned, to show
# what the exact versions of the children were at the time that the
# Container was created.

Is initial list really the list when the container was first created? (which I assume would usually be an empty list). I think this should say "the initial list is a copy of the author-defined list that has all versions pinned as they were at the time the ContainerEntityVersion was created." Because if this is really something immutable from when the container was created, not something for when each version is created, then it should be on the container model, not redundantly specified for every ContainerEntityVersion.

The frozen final state of a container is the state of the container at the time when a new version is created.

I think this needs an example: "While this ContainerEntityVersion is the current draft, this will be None which means that unpinned entities in the list should be showing their latest version. However, once this is published or an even newer draft is created, this frozen list should be saved to reflect the history of the list at that exact point in time when the version was finalized."

---

I also don't really understand the need for an initial_list at all. Isn't it the same as "the frozen_list of the previous version"? And whether that's the case or not, when/why do we need to use it (initial_list)? I don't see any explanation in #240 .

[mariajgrimaldi]

Oh, now I see that in the ADR itself I called it container states in some places and in others lists. I updated this also considering the author-defined suggestion. Thanks for the suggestions!

---

Is initial list really the list when the container was first created? (which I assume would usually be an empty list).

No. It's when the next container version is created, not the container entity itself.

I think this should say "the initial list is a copy of the author-defined list that has all versions pinned as they were at the time the ContainerEntityVersion was created."

Yes, that's a better description of what's happening here. I wanted to keep this as high-level as possible, so I avoided mentioning container entities or other model-specific definitions. Do you think it is necessary to reference each model for clarity?

Because if this is really something immutable from when the container was created, not something for when each version is created, then it should be on the container model, not redundantly specified for every ContainerEntityVersion.

Exactly, these lists are actually related to each container version, not the container entity. I'll make that clear.

---

I think this needs an example: "While this ContainerEntityVersion is the current draft, this will be None which means that unpinned entities in the list should be showing their latest version. However, once this is published or an even newer draft is created, this frozen list should be saved to reflect the history of the list at that exact point in time when the version was finalized."

In "...unpinned entities in the list should be showing their lates version" do you refer to the author-defined list, right?

---

I also don't really understand the need for an initial_list at all. Isn't it the same as "the frozen_list of the previous version"? And whether that's the case or not, when/why do we need to use it (initial_list)? I don't see any explanation in #240 .

I think the idea of an initial state came from this comment: #38 (comment) -- still, it doesn't say the end purpose. But as far as I understand, it's included for convenience:

# inital_list is an EntityList where all the versions are pinned, to show
# what the exact versions of the children were at the time that the
# Container was created. We could technically derive this, but it would be
# awkward to query.
#
# If the Container was defined so that all references were pinned, then this
# can point to the exact same EntityList as defined_list.

[bradenmacdonald]

Yeah, I saw that, but I still don't understand the use case. I also can't tell if it's any different than "the frozen_list of the previous version", or if those are the same thing.

@ormsbee maybe you can clarify?

[bradenmacdonald]

Is there a difference between specifying "latest draft" and specifying "latest published" or is there just a way to say "latest" (version=None), and whether that is draft or published depends on _______ ?

EDIT: OK, from looking at the code it appears that each container (via EntityListRow) can specify both a draft_version and a published_version at the same time, separately. And each can be either pinned or unpinned. I guess I'm very unclear on how this works when the ContainerEntityVersion itself is versioned and has a draft + published version.

If I have a draft ContainerEntityVersion, what do its EntityListRows' published_versions mean? And when I publish it, so it's now a published ContainerEntityVersion, what do its EntityListRows' draft_versions mean?

[ormsbee]

If I have a draft ContainerEntityVersion, what do its EntityListRows' published_versions mean? And when I publish it, so it's now a published ContainerEntityVersion, what do its EntityListRows' draft_versions mean?

I originally had it with just version, but I vaguely remember thinking that I had to add draft_version separately from published_version to address some edge case... which I have totally forgotten the specifics of. And maybe was wrong-headed to begin with. 😛 Let's take it back out for now and just keep version–if said weird edge case was real, I'm sure we'll run into it in the not-too-distant future, and deal with it then.

[mariajgrimaldi]

@ormsbee: during our latest discussion, you mentioned that having both, mainly the draft version, covered the CCX use cases where we don't necessarily control the latest draft version, so we'd want to pin it for the author's context. Do you think we should cover that case later on?

We also discussed that having both would help us know which published/draft versions were in the frozen list when creating the next version, but I think that the use case could be covered by having a single reference to the current version draft or published. Would you agree?

[ormsbee]

@ormsbee: during our latest discussion, you mentioned that having both, mainly the draft version, covered the CCX use cases where we don't necessarily control the latest draft version, so we'd want to pin it for the author's context. Do you think we should cover that case later on?

I think I was mistaken there, because if the author wanted to pin to a specific version, they could do so, and then when they decided to undo that pin to get something later, they could do so with a new version of the container–no need to keep the publish versioned separate.

We also discussed that having both would help us know which published/draft versions were in the frozen list when creating the next version, but I think that the use case could be covered by having a single reference to the current version draft or published. Would you agree?

Yeah, I agree.

I think where my head might have been going is what happens in the following scenario:

1. UnitVersion UV1 has an unpinned reference to Component C1.
2. C1 has ComponentVersions CV1 and CV2. CV1 is the current published version, while CV2 is the draft version.
3. C1 is soft-deleted, forcing UV1 to pin down its references to the last version. But does it pin to CV1 or CV2?

---

So I'm still kicking it around in my head, but I'm trying to see how things line up with Unit/Container modeling if we don't force new container versions to be created when things get deleted, and instead filter out the deleted stuff at read time. I think it could get rid of a lot of the bookkeeping needs for the model. I'm trying to sketch this out this evening to see if it fits together in a reasonable way...

[bradenmacdonald]

Can you "pin" to a specific draft version or published version? I would assume you can only pin to a specific version number, which isn't really considered "draft" or "published" - it's just a specific version (that may have been the latest published version and/or latest draft version at some point, but may or may not be now).

e.g.

1 2 3 4 5 6 7 8
    ^   ^     ^
    |   |     v8 is draft
    |   v5 is published
    Pinned to v3

In this scenario, v3 is just a specific version, neither published nor draft.

Edit: OK, I see now you can pin "draft" and "published" separately for each EntityRow, but I'm confused on the implications of that.

[bradenmacdonald]

What is a "unit application"?

[mariajgrimaldi]

I think this explains it better: 46999f8

I'm specifically referring to:

• Generalized containers (containers app–lowest level of these)
• Selectors for dynamically selecting 0-N PublishableEntities, i.e. how we're going to do things like SplitTest and Randomized (selectors app, builds on containers).
• Units (units app, builds on containers and selectors)–basically empty shells at the moment.

From #240. Let me know if there's a better way of saying this.

[bradenmacdonald]

• Generalized containers (containers app–lowest level of these)
• Selectors for dynamically selecting 0-N PublishableEntities, i.e. how we're going to do things like SplitTest and Randomized (selectors app, builds on containers).
• Units (units app, builds on containers and selectors)–basically empty shells at the moment.

Can you include this whole example? That really makes it more clear.

[mariajgrimaldi]

Yes, sure!

[bradenmacdonald]

How will "dynamically generated content" work?

[mariajgrimaldi]

According to #240, dynamically generated content will be built on top of what the issue called "selectors". I thought we could write a different ADR for it. I'm going to create an empty ADR with some basic info at the moment, so it's clear we will address it in this PR.

Although the issue calls it "dynamically selected" i.e SplitTest or Randomized content, so I believe it'd be better to change this from "static and dynamically generated" to "static and dynamic content" removing "generated".

[mariajgrimaldi]

I removed this from the generalized container capability into the unit ADR, since it makes more sense to be there.

[bradenmacdonald]

Ah, I see now:

    # These entities
    # could be Selectors, in which case we'd need to do more work to find the right
    # variant.

The part that I'm struggling with is that we seem to be using a lot of complexity and work (frozen_list, initial_list) to support both pinned and unpinned versions at this base layer, and I was kind of assuming that by paying the price of that complexity, we would also be able to handle "selectors" and dynamic content. But it seems like that's going to be yet another layer on top of this.

The way that "selectors" will have to deal with pinned/unpinned references and children seems very similar and I am kind of hoping that we'll be able to find some commonality and simplify this. It may be too early to know that though.

Note that we could implement containers and units first, and add selectors later, but I would like to validate selectors because it's the riskiest part of this design.

I agree with this statement. It's hard to know how good this high level design is until we see how it works with selectors.

[ormsbee]

The way that "selectors" will have to deal with pinned/unpinned references and children seems very similar and I am kind of hoping that we'll be able to find some commonality and simplify this. It may be too early to know that though.

I think that when I first sketched this part of the data model, my thinking was that we'd want to always pin all the entities referenced from the Variant, because we might be dynamically generating a different Variant per user–something that would make it really expensive to amend things if someone deleted an entity that was referenced in an unpinned way. I also suspect that I sketched the Selector/Variant piece before I started making the distinction between defined/initial/frozen lists, since a lot of that was added to deal with deleting members.

But I agree with your point that there's more overlap here than this model is representing now. Author-defined Variants (like A/B tests) are much more like their own kind of ContainerEntityVersions. Dynamically generated Variants (like randomized subset), could be pointers to those containers + a pinned EntityList.

I feel like all the fundamental pieces are there, if we just nudge things around a bit. I'll chew on it.

[mariajgrimaldi]

But I agree with your point that there's more overlap here than this model is representing now. Author-defined Variants (like A/B tests) are much more like their own kind of ContainerEntityVersions. Dynamically generated Variants (like randomized subset), could be pointers to those containers + a pinned EntityList.

Am I right in thinking that variants would be a container type with special rules (author-defined or per-user) for their author-defined list shown to students? I wonder how this would work with other container types like units.

[mariajgrimaldi]

@bradenmacdonald: what do you think about this section about the new single entity list?

[bradenmacdonald]

I think it's more accurate to the prototype to just say "The entity list represents the content of a container version as defined by the author, which may be a list of specific component versions or just a list of components without version information."

[mariajgrimaldi]

Got it! Thank you

[kdmccormick]

@cmltaWt0 These are the ADRs I was talking about

[kdmccormick]

My only feedback is to drop "soft-" from "soft-deletion"; I find it confusing. Consider that optional feedback.

Otherwise, LGTM--solid architecturally and very well written. As someone coming back to this project after a few weeks working on other things, these did a good job helping me get back up to speed.

[kdmccormick]

I removed Braden's review request since he's on vacation for the rest of the month.

@ormsbee , did you want to do another pass , or is Maria good to merge?

[ormsbee]

@mariajgrimaldi, @kdmccormick: I think it's good to go, but I'd like to do one more pass this afternoon. Thank you for your patience.

[mariajgrimaldi]

@ormsbee: thank you! I'll be very attentive if any changes are needed.

[ormsbee]

Containers were folded into the publishing app because of the current coupling that exists between drafts and publishing between children and parents. The alternative would have been to do everything via signals, but we decided that it wasn't worth the added complexity and potential for confusion.

[mariajgrimaldi]

Updated! Thank you

6152dba

[ormsbee]

This still affects what we see for "last published at..." for this container though, doesn't it? When a child is published?

[mariajgrimaldi]

Should I change this to "no new version will be created" but publishing metadata might change?

[mariajgrimaldi]

@kdmccormick: what do you think about this?

[kdmccormick]

I think I'm confused. @ormsbee , why does publishing a child affect its parent's "last published at"? Is that just a UI-level decision we made, or is there something in the data model / python API which is bubbling the publish action up to the parent's metadata?

[ormsbee]

It's a UI-level decision, but I thought it was worth calling out. At the model level, this statement is correct in that we won't add a PublishLogEntry for the parent when a child is published. But we do have to signal to the user that "something in this container was published at this time" when a child is published--that's the "last published at" date that users expect, per product guidance.

I was actually wondering if it would be feasible to make the parent re-publish, just with the same before and after versions to indicate a child change. But regardless of whether that works or not, I think it's worth noting the product intent here.

[kdmccormick]

Thanks Dave. I am curious how we will avoid confusing authors into thinking that their full container's changes are published when only a child's changes have been published. But, we don't need to resolve that for this ADR.

@mariajgrimaldi , may I suggest:

• Containers are not affected by the publishing process of its children. This means that publishing a component won't trigger new publishing processes for a container. However, authors will find it relevant that a container's child has changed; therefore, in the UI, publishing a child will most likely affect the date at which its ancestor containers are shown to have been "last published at".

[mariajgrimaldi]

Done! Thank you both for the help :)

728c1e7

[ormsbee]

Minor request to indicate that we put container logic in the publishing app, rather than giving it its own Django app, but otherwise looks good to merge to me.

[kdmccormick]

Latest edits LGTM content-wise.

@mariajgrimaldi , I think some of the headings are mixed up--the title heading and the subsection headings are the same size. Can you take look at those, and also update them to match the RST style guide? https://docs.openedx.org/en/latest/documentors/references/quick_reference_rst.html#headings

[mariajgrimaldi]

@ormsbee @bradenmacdonald @kdmccormick: I appreciate all the help! Thank you!!