Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
docs: ADRs for modeling containers capability #251
base: main
Are you sure you want to change the base?
docs: ADRs for modeling containers capability #251
Changes from 13 commits
2906792
b1b1a88
413b66c
0deab25
22eb1ae
db61fda
b5f05d8
9d2c62d
8648f16
80cf370
b98c02c
ccada60
778ce2b
e738778
3a28fa5
bbce789
af7dce4
506d9cd
466b450
46999f8
0c426d2
46dfa9e
d37a414
64e4db9
46e5a09
822d9a0
8646225
1f1c962
f260756
86141e5
aff8b9b
76e89d5
6ecde96
f7cc446
dc5a0a2
d0f1fc8
83f8d04
8fa418e
6fd6864
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really clear on what this third line about shared definitions means. Maybe an example would help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will "dynamically generated content" work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this from the generalized container capability into the unit ADR, since it makes more sense to be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see now:
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 wherefrozen_list
would beNone
andinitial_list = P0, P1, P2
where P0, P1, and P2 are the pinned versions ofP0latest
and so on at moment of creation. When U1 is created,defined_list
looks likedefined_list = P0latest, P1latest, P2latest
so frozen_list would befrozen_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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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.
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 .There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
No. It's when the next container version is created, not the container entity itself.
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?
Exactly, these lists are actually related to each container version, not the container entity. I'll make that clear.
In "...unpinned entities in the list should be showing their lates version" do you refer to the author-defined list, right?
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, it mentions the initial list of members instead of "state".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 adraft_version
and apublished_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 theContainerEntityVersion
itself is versioned and has a draft + published version.If I have a draft
ContainerEntityVersion
, what do itsEntityListRow
s'published_version
s mean? And when I publish it, so it's now a publishedContainerEntityVersion
, what do itsEntityListRow
s'draft_version
s mean?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally had it with just
version
, but I vaguely remember thinking that I had to adddraft_version
separately frompublished_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 keepversion
–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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Yeah, I agree.
I think where my head might have been going is what happens in the following scenario:
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Edit: OK, I see now you can pin "draft" and "published" separately for each EntityRow, but I'm confused on the implications of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these decisions be part of this ADR as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this requires its own ADR, but it helps illustrate the decisions using a more familiar concept like units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a "unit application"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this explains it better: 46999f8
I'm specifically referring to:
From #240. Let me know if there's a better way of saying this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you include this whole example? That really makes it more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these decisions be part of this ADR as well?