Skip to content

feat: sync more discussion settings on course import#37696

Open
Agrendalath wants to merge 3 commits intoopenedx:masterfrom
open-craft:agrendalath/sync_discussion_config_on_course_import
Open

feat: sync more discussion settings on course import#37696
Agrendalath wants to merge 3 commits intoopenedx:masterfrom
open-craft:agrendalath/sync_discussion_config_on_course_import

Conversation

@Agrendalath
Copy link
Copy Markdown
Member

@Agrendalath Agrendalath commented Nov 26, 2025

Description

This adds synchronization for the following attributes:

  1. Discussion enabled. (not anymore - it was implemented in fix: discussion tab visibility on import #38084)
  2. In-context discussion enabled.
  3. Posting restrictions.
  4. Plugin configuration (e.g., grouping at the subsection level).

Useful information to include:

  • This impacts Course Authors.

Testing instructions

  1. Export a course.
  2. Check out this branch.
  3. Go to the course discussion config under [http://local.openedx.io:8000/admin/discussions/discussionsconfiguration/].
  4. Update the following values:
    • enable_in_context
    • posting_restrictions
    • plugin_configuration
  5. Re-import the course.
  6. Ensure that the original values have been restored.

Deadline

"None"

Other information

Private-ref: BB-10137

@Agrendalath Agrendalath self-assigned this Nov 26, 2025
@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Nov 26, 2025
@openedx-webhooks
Copy link
Copy Markdown

openedx-webhooks commented Nov 26, 2025

Thanks for the pull request, @Agrendalath!

This repository is currently maintained by @openedx/wg-maintenance-openedx-platform.

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.

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

@github-project-automation github-project-automation bot moved this to Needs Triage in Contributions Nov 26, 2025
@mphilbrick211 mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Dec 3, 2025
@mphilbrick211
Copy link
Copy Markdown

Hi @Agrendalath, is this still in progress?

@Agrendalath
Copy link
Copy Markdown
Member Author

@mphilbrick211, this should only be missing testing instructions, but I will double-check next week.

@Agrendalath Agrendalath force-pushed the agrendalath/sync_discussion_config_on_course_import branch from fc48247 to 10875a2 Compare March 31, 2026 14:28
This adds synchronization for the following attributes:
1. Discussion enabled.
2. In-context discussion enabled.
3. Posting restrictions.
4. Plugin configuration (e.g., grouping at the subsection level).
@Agrendalath Agrendalath force-pushed the agrendalath/sync_discussion_config_on_course_import branch from 10875a2 to 68d9032 Compare March 31, 2026 15:36
@Agrendalath Agrendalath requested a review from xitij2000 March 31, 2026 15:39
@Agrendalath Agrendalath marked this pull request as ready for review March 31, 2026 15:39
@Agrendalath Agrendalath moved this from Waiting on Author to Ready for Review in Contributions Mar 31, 2026
assert self.discussion_config.unit_level_visibility is False
assert self.discussion_config.enable_in_context is True
assert self.discussion_config.posting_restrictions == "enabled"
assert self.discussion_config.provider_type == Provider.LEGACY
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some of these issues exit because as new discussion setting were added, the import code was not updated. Could you add a test that will fail if new settings are added but not updated in the import code?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@xitij2000, good idea. Added in 7dd75c5.

@Anas12091101
Copy link
Copy Markdown
Contributor

@Agrendalath, I think we should also include a data migration to sync the discussion configuration for existing courses.

cc: @kdmccormick

Copy link
Copy Markdown
Contributor

@xitij2000 xitij2000 left a comment

Choose a reason for hiding this comment

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

👍 I've looked at the code and I think this is good to merge.

That said I haven't been able to test it yet, due to a broken devstack. I'll try to test ASAP.

@Agrendalath
Copy link
Copy Markdown
Member Author

@Anas12091101,

I think we should also include a data migration to sync the discussion configuration for existing courses.

Yes, it would be nice to have, but this PR modifies only the code related to course imports, so syncing configurations is a completely different area. Unfortunately, I don't have enough context on the legacy discussions or the plans for them, so I don't know whether this effort would be worth doing before, e.g., migrating courses out of the modulestore.

@xitij2000,

That said I haven't been able to test it yet, due to a broken devstack. I'll try to test ASAP.

Perhaps this will help, as it's one of the recent breaking changes when using the main branch.

@xitij2000
Copy link
Copy Markdown
Contributor

Perhaps this will help, as it's one of the recent breaking changes when using the main branch.

Nope, I faced and fixed that long back :-)
This was some variable not found in setting, just running launch again fixed it but I had spotty internet for a while that caused issues.

Copy link
Copy Markdown
Member

@kdmccormick kdmccormick left a comment

Choose a reason for hiding this comment

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

LGTM, but haven't tested it myself. Merge away once @xitij2000 confirms he's been able to manually test.

Copy link
Copy Markdown
Contributor

@xitij2000 xitij2000 left a comment

Choose a reason for hiding this comment

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

👍 Just managed to fix my devstack and test this.

  • I tested this: tested using instructions.
  • I read through the code
  • [-] I checked for accessibility issues (NA)
  • [-] Includes documentation (NA)
  • [-] I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository. (NA)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core contributor PR author is a Core Contributor (who may or may not have write access to this repo). open-source-contribution PR author is not from Axim or 2U

Projects

Status: Ready for Review

Development

Successfully merging this pull request may close these issues.

6 participants