Skip to content
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

feat: making lms.djangoapps.ccx AppConfig ready. #34220

Merged
merged 2 commits into from
Mar 20, 2024

Conversation

alexjmpb
Copy link
Contributor

Description

The CCX app uses a signal receiver (lms.djangoapps.ccx.course_published_handler) to know whenever the outline of a course is updated, the receiver listens to the signal "course_published". Right now, the lms.djangoapps.ccx app does not count with an AppConfig module to import the receivers, and the course_published signal can't be aware of them. This causes an unexpected behavior where the outline of the CCX is not updated. This PR aims to add an apps.py module containing an AppConfig class that imports the signal receivers of the app.

Useful information to include:

  • As an useful note, the lms.djangoapps.ccx is not installed in Studio, it should also be added to the INSTALLED_APPS for the signal to be aware of the receiver. Installing lms.djangoapps.ccx also requires to install lms.djangoapps.program_enrollments.

Testing instructions

  1. Verify that currently, the CCX outline is not updated whenever the parent course is changed (you can follow the steps 8-11 below to create a CCX).
  2. Install a Tutor 17 (https://docs.tutor.edly.io/install.html).
  3. Build a custom image using this branch (https://docs.tutor.edly.io/configuration.html#running-a-fork-of-edx-platform).
  4. Create a tutor plugin (https://docs.tutor.edly.io/tutorials/plugin.html) that has the following structure.
from tutor import hooks

hooks.Filters.ENV_PATCHES.add_items([
    (
        "openedx-common-settings",
        "FEATURES['CUSTOM_COURSES_EDX'] = True",
    ),
    (
        "openedx-lms-common-settings",
        """
INSTALLED_APPS += ['lms.djangoapps.ccx', 'openedx.core.djangoapps.ccxcon.apps.CCXConnectorConfig', 'user_tasks', 'cms.djangoapps.contentstore.apps.ContentstoreConfig']
COURSE_IMPORT_EXPORT_STORAGE = 'django.core.files.storage.FileSystemStorage'
COURSEGRAPH_DUMP_COURSE_ON_PUBLISH = False
MODULESTORE_FIELD_OVERRIDE_PROVIDERS += ('lms.djangoapps.ccx.overrides.CustomCoursesForEdxOverrideProvider',)
"""
    ),
    (
        "openedx-cms-common-settings",
        """
INSTALLED_APPS += ['lms.djangoapps.ccx', 'lms.djangoapps.program_enrollments']
BULK_EMAIL_DEFAULT_RETRY_DELAY = 30
BULK_EMAIL_MAX_RETRIES = 5
"""
    ),
])

It will suffice to add the settings stated in there to enable all features.

  1. Start tutor with the custom image and settings (https://docs.tutor.edly.io/quickstart.html).
  2. In studio, create a course and go to advanced settings, once you are there, toggle the "Enable CCX" setting.
  3. Go to the course and in the instructor dashboard go to the membership tab and add a CCX coach.
  4. Login with your CCX coach, go to the course that you created, go to the CCX coach tab and create a CCX.
  5. Now all changes that you make in the parent course outline (sections, subsections) should be reflected in the CCX course. (All the steps to create a CCX can be viewed in the docs https://edx.readthedocs.io/projects/open-edx-building-and-running-a-course/en/latest/set_up_course/studio_add_course_information/custom_courses.html)

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 12, 2024
@openedx-webhooks
Copy link

Thanks for the pull request, @alexjmpb! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@pdpinch
Copy link
Contributor

pdpinch commented Feb 12, 2024

Is there an issue associated with this PR somewhere? I'd like to know more about the circumstances that lead to its discovery.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 12, 2024
@alexjmpb
Copy link
Contributor Author

Is there an issue associated with this PR somewhere? I'd like to know more about the circumstances that lead to its discovery.

@pdpinch We use the CCX feature in the openedx instances of our organization, and noticed that the outline of the CCXs were not being updated. This issue began after a migration from Juniper to Olive, and we started to debug why this was occurring. After some testings, we found that the task that updates the outline for each CCX (lms.djangoapps.ccx.send_ccx_course_published) was not being called, this was happening due to the issue with the course_published signal commented in the PR description. I then tested this issues in master and found that it's still present.

Copy link
Contributor

@Anas12091101 Anas12091101 left a comment

Choose a reason for hiding this comment

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

LGTM

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Feb 21, 2024
@pdpinch
Copy link
Contributor

pdpinch commented Mar 4, 2024

@alexjmpb I think this is ready to merge after you fix the quality issue.

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 20, 2024
@pdpinch pdpinch merged commit e7f02fe into openedx:master Mar 20, 2024
65 checks passed
@openedx-webhooks
Copy link

@alexjmpb 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@pdpinch
Copy link
Contributor

pdpinch commented Mar 20, 2024

Thanks @alexjmpb. I hope you don't mind, but I squashed your commits when I merged them. I should have asked you to do this, but I didn't want to add another day or two to this.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

pdpinch added a commit to mitodl/edx-platform that referenced this pull request Mar 20, 2024
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

@alexjmpb
Copy link
Contributor Author

Thanks @alexjmpb. I hope you don't mind, but I squashed your commits when I merged them. I should have asked you to do this, but I didn't want to add another day or two to this.

No worries at all. I was thinking of doing the same. Thanks for merging.

@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 28, 2024
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.

7 participants