[WIP] feat: implement sketch models and api for interactions based on 0017/18 #254

mariajgrimaldi · 2024-11-05T13:23:22Z

Description

This implementation is based on the high-level decisions made in #251

Unit tests are now the way I'm testing this out.

openedx-webhooks · 2024-11-05T13:23:26Z

Thanks for the pull request, @mariajgrimaldi!

What's next?

Please work through the following steps to get your changes 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.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @axim-engineering. Tag them in a comment and let them know that your changes are ready for review.

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:

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 · 2024-11-08T10:01:01Z

openedx_learning/apps/authoring/containers/api.py

+        new_user_defined_list = create_entity_list_with_rows(
+            entity_pks=publishable_entities_pk,
+            draft_version_pks=[None] * len(publishable_entities_pk),
+            published_version_pks=[None] * len(publishable_entities_pk),
+        )
+        container_version = ContainerEntityVersion.objects.create(
+            publishable_entity_version=publishable_entity_version,
+            container_id=container_pk,
+            defined_list=new_user_defined_list,
+            initial_list=last_version.initial_list,
+            frozen_list=last_version.defined_list,
+        )


This is a naive approach to the implementation for the sake of simplicity. Now, I'll modify it to comply with the requirements I've compiled in the cover letter, so the code will probably be different when you see this.

Still, I have some questions about the requirements:

The defined_list is the EntityList that the author defines. This should never change, even if the things it references get soft-deleted (because we'll need to maintain it for reverts).

Questions

~~How do we ensure this doesn't change even if references get soft deleted?~~ Because soft-deletion is removing references in the draft log for a version, so it's not deleted nor removed.

Is the discard operation (made changes that created a new draft version) different from revert?

initial_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.

Questions

Let's say a version for a container is created referencing the latest versions for all members. Then, we manually pin all members when creating this list. Pinning a version would mean pointing the EntityListRow draft/published version to the latest versions at the time for this particular case, am I right? YES!

frozen_list is mutable if and only if there are unpinned references in defined_list. In that case, frozen_list should start as None, and be updated to pin references when another version of this Container becomes the Draft version. But if this version ever becomes the Draft again (e.g. the user hits "discard changes" or some kind of revert happens), then we need to clear this back to None.

Questions

~~Even if there's an unpinned reference, should frozen_list start as None?~~ YES!

~~When a new version is created and frozen_list is None, then we should pin all references from the defined_list when creating the new frozen_list?~~ YES!

~~Should we explicitly check when a version becomes draft again, so we can clear the frozen list? How could we detect a version becomes a draft again?~~ Solved in a sync discussion!

mariajgrimaldi · 2024-11-08T10:14:53Z

openedx_learning/apps/authoring/containers/api.py

+    return ContainerEntity.objects.get(pk=pk)
+
+
+def get_defined_list_for_container_version(


Question:
I wonder whether we should provide this kind of API method or whether users of the containers API should be unaware of EntityListRows' existence.

mariajgrimaldi · 2024-11-08T10:25:28Z

openedx_learning/apps/authoring/units/api.py

+        # TODO: how can we enforce that publishable entities must be components?
+        # This currently allows for any publishable entity to be added to a unit.


Question
Should we enforce the fact that publishable entities added to a unit are components here? Or should this be flexible enough to provide methods to do so but not explicitly enforce it?

mariajgrimaldi · 2024-11-08T10:28:49Z

openedx_learning/apps/authoring/units/api.py

+        unit_version_pk: The unit version ID.
+    """
+    unit_version = UnitVersion.objects.get(pk=unit_version_pk)
+    return container_api.get_defined_list_for_container_version(unit_version.container_entity_version)


Note to self:

I have the same question here about whether to return entity lists or publishable entities. I'm guessing the latter, but I'd like to confirm.

openedx_learning/apps/authoring/containers/api.py

mariajgrimaldi · 2024-11-08T12:30:03Z

Hey @ormsbee! I’ve been making progress on this and left some comments in the code where I could use some guidance. Thanks a ton for the help!

I'm pushing changes as I go, so I'm sorry if some comments seem out of date, questions are still relevant, though. Code itself might not be ready for review, so I'd hold off on a review for now.

mariajgrimaldi · 2024-11-08T12:32:42Z

openedx_learning/apps/authoring/containers/api.py

+    publishable_entities_pk: list[int],
+    draft_version_pks: list[int | None],
+    published_version_pks: list[int | None],


Question:
Should the draft/published versions be derived somehow from the publishable entities?

This is still under discussion in the ADR, so we'll probably use a single reference to the versions.

openedx_learning/apps/authoring/containers/api.py

mariajgrimaldi · 2024-11-08T12:50:36Z

openedx_learning/apps/authoring/containers/api.py

+        new_user_defined_list = create_entity_list_with_rows(
+            entity_pks=publishable_entities_pk,
+            draft_version_pks=draft_version_pks,
+            published_version_pks=published_version_pks,
+        )
+        new_frozen_list = copy_rows_to_new_entity_list(
+            rows=get_defined_list_rows_for_container_version(last_version)
+        )
+
+        container_version = ContainerEntityVersion.objects.create(
+            publishable_entity_version=publishable_entity_version,
+            container_id=container_pk,
+            defined_list=new_user_defined_list,
+            initial_list=last_version.initial_list,
+            frozen_list=new_frozen_list,
+        )


Note to self:

Here, I'm creating a new defined_list (with new rows) for each new version and also copying things over from the previously defined_list into a new frozen_list (also, with new rows). This is done to comply with:

defined_list should never change for a given ContainerEntityVersion to simplify reasoning.

The defined_list is the EntityList that the author defines. This should never change, even if the things it references get soft-deleted (because we'll need to maintain it for reverts).

But we also have this requirement:

Ordering should be treated as immutable–if the ordering needs to change, we create a new EntityList and copy things over.

So no new entity list rows for defined_lists should be created if the ordering doesn't change or versions are not pinned, but new members are added. Which I'm currently not considering.

I remember discussing this with Dave a couple of weeks ago too, so implementing it seems like the right approach here.

I'm testing my changes out before pushing, I'm hoping I can do it today.

mariajgrimaldi · 2024-11-12T15:58:10Z

openedx_learning/apps/authoring/containers/api.py

+        new_frozen_list = copy_rows_to_new_entity_list(
+            rows=get_defined_list_rows_for_container_version(last_version)
+        )


Note to self:

This is pinning references for the next version of the container instead of pinning versions for the previous version. Which is what should happen according to this comment: https://github.com/openedx/openedx-learning/pull/254/files#diff-6f2c589dc4ba5960e91d39f6488eb5e2e2e63ddaff63a75909091c760b877802R142-R147

So, when have two scenarios when creating a new version:

The author defined list has unpinned references

The frozen_list of the current container is updated to pin specific versions of the members

The next version frozen state points to None

The author defined list has pinned references

The current version frozen list points to the current defined list

The next version frozen list points to the new defined list

And any other combination between the two in case defined_list can pin/unpin references

openedx_learning/apps/authoring/containers/api.py

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 5, 2024

mariajgrimaldi changed the title ~~feat: implement sketch models and api for interactions based on 0017/18~~ [WIP] feat: implement sketch models and api for interactions based on 0017/18 Nov 5, 2024

mariajgrimaldi mentioned this pull request Nov 5, 2024

ADR for Units and Containers #240

Open

mariajgrimaldi commented Nov 8, 2024

View reviewed changes

openedx_learning/apps/authoring/containers/api.py Outdated Show resolved Hide resolved

mariajgrimaldi added 5 commits November 8, 2024 13:21

feat: implement sketch models and api for interactions based on 0017/18

8239985

refactor: register unit models in app setup

b9fed67

refactor: change model_mixin name to models_mixin

149c480

refactor: use container API to get unit members

3780e49

refactor: pin versions in frozen list each time a new version is created

1f10aec

mariajgrimaldi force-pushed the MJG/containers-units-dev branch from 60fdb44 to 1f10aec Compare November 8, 2024 12:22

mariajgrimaldi commented Nov 8, 2024

View reviewed changes

mariajgrimaldi mentioned this pull request Nov 8, 2024

docs: ADRs for modeling containers capability #251

Open

mariajgrimaldi commented Nov 12, 2024

View reviewed changes

mariajgrimaldi commented Nov 14, 2024

View reviewed changes

openedx_learning/apps/authoring/containers/api.py Outdated Show resolved Hide resolved

mariajgrimaldi added 4 commits November 14, 2024 18:17

tmp -- needs testing

d9cdb21

tmp

dc57c10

tmp

1ad12f5

refactor: address PR reviews and implement unittests

5d0083b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] feat: implement sketch models and api for interactions based on 0017/18 #254

[WIP] feat: implement sketch models and api for interactions based on 0017/18 #254

mariajgrimaldi commented Nov 5, 2024 •

edited

Loading

openedx-webhooks commented Nov 5, 2024

mariajgrimaldi Nov 8, 2024 •

edited

Loading

mariajgrimaldi Nov 8, 2024 •

edited

Loading

mariajgrimaldi Nov 8, 2024

mariajgrimaldi Nov 8, 2024 •

edited

Loading

mariajgrimaldi commented Nov 8, 2024 •

edited

Loading

mariajgrimaldi Nov 8, 2024 •

edited

Loading

mariajgrimaldi Nov 8, 2024 •

edited

Loading

mariajgrimaldi Nov 20, 2024

mariajgrimaldi Nov 12, 2024 •

edited

Loading

		return ContainerEntity.objects.get(pk=pk)


		def get_defined_list_for_container_version(

		# TODO: how can we enforce that publishable entities must be components?
		# This currently allows for any publishable entity to be added to a unit.

[WIP] feat: implement sketch models and api for interactions based on 0017/18 #254

Are you sure you want to change the base?

[WIP] feat: implement sketch models and api for interactions based on 0017/18 #254

Conversation

mariajgrimaldi commented Nov 5, 2024 • edited Loading

Description

openedx-webhooks commented Nov 5, 2024

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Update the status of your PR

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

mariajgrimaldi Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

mariajgrimaldi Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

mariajgrimaldi Nov 8, 2024

Choose a reason for hiding this comment

mariajgrimaldi Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

mariajgrimaldi commented Nov 8, 2024 • edited Loading

mariajgrimaldi Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

mariajgrimaldi Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

mariajgrimaldi Nov 20, 2024

Choose a reason for hiding this comment

mariajgrimaldi Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

mariajgrimaldi commented Nov 5, 2024 •

edited

Loading

mariajgrimaldi Nov 8, 2024 •

edited

Loading

mariajgrimaldi Nov 8, 2024 •

edited

Loading

mariajgrimaldi Nov 8, 2024 •

edited

Loading

mariajgrimaldi commented Nov 8, 2024 •

edited

Loading

mariajgrimaldi Nov 8, 2024 •

edited

Loading

mariajgrimaldi Nov 8, 2024 •

edited

Loading

mariajgrimaldi Nov 12, 2024 •

edited

Loading