Skip to content

Conversation

@andrii-hantkovskyi
Copy link
Contributor

This PR implements the new subsections functionality in Learning Core, which allows for organizing units within hierarchical containers. Subsections serve as containers that can store and manage units within the content structure.

Changes in this PR

  • Created new subsections models and API as part of the subsections app
  • Implemented container behavior where publishing a subsection automatically publishes its child units
  • Added comprehensive functionality to manage the subsection lifecycle:
    • Creation of subsections and subsection versions
    • Ability to pin specific unit versions or use the latest versions
    • Retrieval of units contained within subsections (both draft and published)
  • Added high-level APIs for subsection management:
    • get_units_in_subsection to retrieve units in draft or published subsections
    • get_units_in_published_subsection_as_of to access historical states of subsections
    • Various utility functions for subsection creation and version management

This implementation extends our hierarchical content structure, allowing for more organized content arrangement with units being contained within subsections.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 11, 2025
@openedx-webhooks
Copy link

openedx-webhooks commented Apr 11, 2025

Thanks for the pull request, @andrii-hantkovskyi!

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.


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.

@andrii-hantkovskyi
Copy link
Contributor Author

Hi @kdmccormick and @ormsbee, could you please review this PR when you have a chance?

Comment on lines 1029 to 1328
entity_list = getattr(container.versioning.draft, "entity_list", None)
# ?FOR REVIEW: Is this correct?
if entity_list is None:
# This container has not been published yet, or has been deleted.
return False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

container.versioning.draft should always return an object with an entity_list. Are you finding that the attribute is missing sometimes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, test_soft_deleting_unit is failing when calling authoring_api.contains_unpublished_changes because of:

entity_list = container.versioning.draft.entity_list
E       AttributeError: 'NoneType' object has no attribute 'entity_list'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will happen if you create a publishable entity but haven't created any versions of that entity. It's probably worth just bootstrapping it so that creating a PublishableEntity always sets the Draft and Published versions to None, so we never have to deal with that state.

Copy link
Contributor Author

@andrii-hantkovskyi andrii-hantkovskyi Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dug down a bit and found it's not an issue with creating but soft-deleting. When we call the soft_delete_draft method, it sets the draft version of PublishableEntity to None, which is why we're getting that error.

For now, I left a comment inside if, which says that the container was soft-deleted. If this needs any other changes, please let me know

@mphilbrick211 mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Apr 14, 2025
@@ -0,0 +1,50 @@
"""
Models that implement units
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Models that implement units
Models that implement subsections

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines 90 to 91
publishable_entities_pks=publishable_entities_pks,
entity_version_pks=entity_version_pks,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API contract has changed as of a recent PR. Please update to the latest.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

Comment on lines 1029 to 1328
entity_list = getattr(container.versioning.draft, "entity_list", None)
# ?FOR REVIEW: Is this correct?
if entity_list is None:
# This container has not been published yet, or has been deleted.
return False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will happen if you create a publishable entity but haven't created any versions of that entity. It's probably worth just bootstrapping it so that creating a PublishableEntity always sets the Draft and Published versions to None, so we never have to deal with that state.

@andrii-hantkovskyi andrii-hantkovskyi force-pushed the axm-course-to-library-import branch 2 times, most recently from 7f5d5fd to 3010e9d Compare April 15, 2025 19:47
@ormsbee
Copy link
Contributor

ormsbee commented Apr 16, 2025

@andrii-hantkovskyi : as a general rule, please don't force push changes on PRs under active review unless you have to. It just makes it harder to filter out what's changed since the last commit.

@ormsbee
Copy link
Contributor

ormsbee commented Apr 16, 2025

@andrii-hantkovskyi: Please submit your signed contributor agreement, per the instructions below:

🔘 Submit a signed contributor agreement (CLA)
⚠️ We ask all contributors to the Open edX project to submit a signed contributor agreement or indicate their institutional affiliation. Please see the CONTRIBUTING file for more information.

If you've signed an agreement in the past, you may need to re-sign. See The New Home of the Open edX Codebase for details.

Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the CLA check by adding a comment below that you have signed it. If the CLA check continues to fail, you can tag the @openedx/cla-problems team in a comment for further assistance.

You can tag me on the ticket. We need this before we can merge any code written by you.

Thank you.

@ormsbee
Copy link
Contributor

ormsbee commented Apr 16, 2025

Also, please fix the various linter errors on this PR. Thank you.

@NiedielnitsevIvan NiedielnitsevIvan force-pushed the axm-course-to-library-import branch from 8f2fcdf to 498e5cd Compare April 18, 2025 16:53
Co-authored-by: Kyle McCormick <[email protected]>
@NiedielnitsevIvan NiedielnitsevIvan force-pushed the axm-course-to-library-import branch from 2ff78eb to 7b5cf6e Compare April 18, 2025 17:00
@kdmccormick kdmccormick merged commit c2f57df into openedx:main Apr 18, 2025
11 checks passed
@github-project-automation github-project-automation bot moved this from Waiting on Author to Done in Contributions Apr 18, 2025
ormsbee pushed a commit that referenced this pull request Apr 22, 2025
This is modeled on the existing apps for units and subsections.

- Created new sections models and API as part of the sections app
- Implemented container behavior where publishing a section automatically
  publishes its child subsections
- Added comprehensive functionality to manage the section lifecycle:
  - Creation of sections and section versions
  - Ability to pin specific subsection versions or use the latest
    versions
  - Retrieval of subsections contained within sections (both draft
    and published)
- Added high-level APIs for section management:
  - get_subsections_in_section to retrieve subsections in draft or
    published sections
  - get_subsections_in_published_section_as_of to access historical
    states of sections
  - Various utility functions for section creation and version
    management
- Bumped openedx_learning to 0.25.0 to account for changes from
  Subsections in Learning Core #301, which were merged without a
  version update.

---------

Co-authored-by: Andrii <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants