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 9 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.
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.
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?