Skip to content

feat: count XBlocks with optional_completion#279

Merged
Agrendalath merged 5 commits intomasterfrom
agrendalath/bb-9331-optional-completion
Mar 17, 2026
Merged

feat: count XBlocks with optional_completion#279
Agrendalath merged 5 commits intomasterfrom
agrendalath/bb-9331-optional-completion

Conversation

@Agrendalath
Copy link
Copy Markdown
Member

@Agrendalath Agrendalath commented Jan 27, 2026

Description: This adds support for optional completion implemented in open-craft/openedx-platform#823.

Dependencies:

  1. feat(redwood): implement optional completion openedx-platform#823

Merge deadline: None

Installation instructions: For now, this requires a Redwood instance, as the optional completion is implemented only in this release. Therefore, it may be easier to test this one on a sandbox instance.

Testing instructions:

  1. Add ENABLE_COURSE_ACTIVITY_SIGNALS = True to your instance configs.
  2. Change some sections, subsections, or units to be optional and check the results under http://local.openedx.io:8000/completion-aggregator/v1/course/course-v1:edX+DemoX+Demo_Course/?include_optional=true.

Reviewers:

  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed
  • PR author is listed in AUTHORS

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

Private-ref: BB-9331

@Agrendalath Agrendalath changed the base branch from agrendalath/bb-10290-ulmo to master January 27, 2026 19:35
@Agrendalath Agrendalath mentioned this pull request Jan 27, 2026
11 tasks
@Agrendalath Agrendalath self-assigned this Jan 27, 2026
@Agrendalath Agrendalath force-pushed the agrendalath/bb-9331-optional-completion branch from eef91da to 53473df Compare January 28, 2026 14:50
@Agrendalath Agrendalath force-pushed the agrendalath/bb-9331-optional-completion branch from 80c26d7 to e1a6a84 Compare February 4, 2026 18:38
@Agrendalath Agrendalath changed the title feat: ignore XBlocks with optional_completion feat: count XBlocks with optional_completion Feb 4, 2026
@Agrendalath Agrendalath requested a review from Copilot February 4, 2026 18:39
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds support for tracking optional completion of XBlocks in the completion-aggregator service. The feature enables separate tracking and reporting of optional vs. required blocks in Open edX courses, with optional blocks identified by the optional_completion XBlock field.

Changes:

  • Added three new fields to the Aggregator model: optional_earned, optional_possible, and optional_percent with corresponding database migration
  • Extended the aggregation logic to separately track completion of optional blocks
  • Added include_optional API parameter to optionally expose optional completion data in API responses
  • Updated serializers to conditionally include optional completion data
  • Added comprehensive test coverage for the new functionality

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
completion_aggregator/models.py Added three new fields for tracking optional completions and updated SQL query to include them
completion_aggregator/migrations/0007_aggregator_optional_completion.py Migration to add optional completion fields to Aggregator model
completion_aggregator/core.py Updated aggregation logic to separately track optional blocks and calculate their completion stats
completion_aggregator/compat.py Added is_block_optional() function to check if a block is optional
completion_aggregator/transformers.py Updated to request optional_completion XBlock field
completion_aggregator/serializers.py Added optional completion serializer and updated existing serializers to conditionally include optional data
completion_aggregator/api/common.py Added get_include_optional() method to parse the include_optional parameter
completion_aggregator/api/v0/views.py Updated views to pass include_optional parameter and documented the new parameter
completion_aggregator/api/v1/views.py Updated views to pass include_optional parameter and documented the new parameter
test_utils/compat.py Added test implementation of is_block_optional()
tests/test_models.py Added tests for optional completion field defaults, validation, and get_values behavior
tests/test_core.py Added tests for optional block aggregation with full and partial completion
tests/test_serializers.py Added tests for optional completion serialization in various scenarios
tests/test_views.py Added tests for include_optional parameter in list and detail views
completion_aggregator/init.py Version bumped to 4.4.0rc
CHANGELOG.rst Added changelog entry for version 4.4.0
codecov.yml Reduced patch coverage target from 100% to 75%
.coveragerc Updated coverage omit patterns

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread completion_aggregator/migrations/0007_aggregator_optional_completion.py Outdated
Comment thread codecov.yml Outdated
Comment thread completion_aggregator/core.py Outdated
Comment thread .coveragerc
@Agrendalath Agrendalath force-pushed the agrendalath/bb-9331-optional-completion branch 2 times, most recently from e265b71 to d0fed71 Compare February 9, 2026 18:22
@tecoholic
Copy link
Copy Markdown
Member

tecoholic commented Mar 11, 2026

@Agrendalath The API flag works as expected and the results contain the "optional" set of values. However, the number are a bit confusing for me. I think, I don't have a good understanding of what the functionality is. So, can you kindly give me a set of test steps that can differentiate the existing and the PR behaviors.

My source of confusion is this response, which has both earned and possible values set to 0.0.

HTTP 200 OK
Allow: GET, POST, HEAD, OPTIONS
Content-Type: application/json
Vary: Accept

{
    "results": [
        {
            "course_key": "course-v1:OpenedX+DemoX+DemoCourse",
            "completion": {
                "earned": 28.0,
                "possible": 311.0,
                "percent": 0.09003215434083602
            },
            "username": "learner_1",
            "optional_completion": {
                "earned": 0.0,
                "possible": 0.0,
                "percent": 1.0
            }
        }
    ],
    "pagination": {
        "next": null,
        "previous": null,
        "count": 2,
        "num_pages": 1
    }
}

Meanwhile, this is what I see on the Learning MFE Progress page:

image

And also on the sidebar

image

Shouldn't these 2 values be correlated? I submitted these units multiple times while testing. Do I need to use fresh units?

Sidenote: some of the units aren't getting marked as completed ("Single-select Multiple Choice Problems" in the screenshot for example), no matter what. I am not sure about what's going on there.

@Agrendalath
Copy link
Copy Markdown
Member Author

@tecoholic,

My source of confusion is this response, which has both earned and possible values set to 0.0.

If you mark some XBlocks (whether they are sections, subsections, units, or individual XBlocks), and have ENABLE_COURSE_ACTIVITY_SIGNALS enabled, the structure should be automatically recalculated after publishing any changes in Studio, so optional_completion -> possible should equal the number of all optional individual XBlocks. For example, let's say you have the following structure:

  1. Required Section
    1. Required Subsection
      1. Required Unit
        1. Required Problem
        2. Optional HTML
    2. Optional Subsection
      1. Unit (inherited - always optional)
        1. Problem (inherited - always optional)
        2. HTML (inherited - always optional)

The value for completion -> possible will be 1, because there is only 1 required XBlock ("Required Problem"). The value for optional_completion -> possible will be 3 ("Optional HTML", "Problem (inherited - always optional)", "HTML (inherited - always optional)").

If the optional_completion -> possible value is 0, and you have some optional content in this course, please run tutor dev exec lms ./manage.py lms reaggregate_course course-v1:OpenedX+DemoX+DemoCourse && tutor dev exec lms ./manage.py lms run_aggregator_service and see if it changes.

Meanwhile, this is what I see on the Learning MFE Progress page:

This is a bit confusing because the Progress page in the Learning MFE counts the number of completed Units rather than individual XBlocks. I don't know why it was designed this way, but I have a WIP plugin that will replace these values for learners, so we do not need to worry about this discrepancy.

Sidenote: some of the units aren't getting marked as completed ("Single-select Multiple Choice Problems" in the screenshot for example), no matter what. I am not sure about what's going on there.

Could you please try the following steps?

  1. Add COMPLETION_AGGREGATOR_ASYNC_AGGREGATION: true to your instance settings
  2. tutor config save
  3. Submit the problem again.
  4. Run tutor dev exec lms ./manage.py lms reaggregate_course course-v1:OpenedX+DemoX+DemoCourse && tutor dev exec lms ./manage.py lms run_aggregator_service.

In Prod, we use async mode because the sync one can cause DB deadlocks. It wasn't happening in the devstack, but for some reason, it happens sometimes in tutor. I did not look into this, but the described approach could prevent the behavior you described. Unfortunately, it means that (in most cases) you need to run tutor dev exec lms ./manage.py lms run_aggregator_service to see new completions reflected in the Completion Aggregator.

Copy link
Copy Markdown
Member

@tecoholic tecoholic left a comment

Choose a reason for hiding this comment

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

@Agrendalath 👍 Thanks for the additional details. I has the env setup with the other config, so running the management commands resolved the issue with the zero values.

  • I tested this: Verified that the API behaves as expected.
  • I read through the code
  • I checked for accessibility issues
  • Includes documentation

@Agrendalath Agrendalath force-pushed the agrendalath/bb-9331-optional-completion branch from d0fed71 to 753a8c4 Compare March 17, 2026 15:39
@Agrendalath Agrendalath enabled auto-merge (rebase) March 17, 2026 15:43
@Agrendalath Agrendalath disabled auto-merge March 17, 2026 16:52
@Agrendalath Agrendalath enabled auto-merge March 17, 2026 16:52
@Agrendalath Agrendalath force-pushed the agrendalath/bb-9331-optional-completion branch 3 times, most recently from 3bb8f4c to 30b3047 Compare March 17, 2026 17:32
Required after openedx/opaque-keys#426

Service containers do not support passing CMD arguments.
Related discussion: https://github.com/orgs/community/discussions/160506
@Agrendalath Agrendalath force-pushed the agrendalath/bb-9331-optional-completion branch from 30b3047 to 696d728 Compare March 17, 2026 17:36
@Agrendalath Agrendalath force-pushed the agrendalath/bb-9331-optional-completion branch from 4389efd to dd52b82 Compare March 17, 2026 17:57
@Agrendalath Agrendalath merged commit a012a19 into master Mar 17, 2026
11 checks passed
@Agrendalath Agrendalath deleted the agrendalath/bb-9331-optional-completion branch March 17, 2026 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants