feat: Adds a new discussion topic configuration mechanism [BD-38] [TNL-8623] [BB-4968] #29082

xitij2000 · 2021-10-21T15:10:34Z

Description

The new discussion configuration system links discussion topics directly to the course structure. This change adds a new task that sends a discussion update signal if there are any changes to the course. This signal includes all the context needed to update the configuration of the course.

The handler for this new event will create a new entry for each unit that needs a topic in the database. In the future this will be used to see the topics in the course.

Dependencies

This PR build on a few other PRs:

The relevant commit is currently: 11de31553ca4e0f1dc816ae88235e41160e1d5f3

Supporting information

Testing instructions

For any existing course, change any setting and publish.
You should see new DiscussionTopicLink objects, one for each general topic and one for each unit.
If there are any graded discussions they will not be included.
Try changing the settings and make sure that the behaviour in the ADR is matched.
Try importing and exporting the course. The settings should be retained.
Try exporting a course and importing it as a new course. The discussion config model should be created to match the original settings.

Deadline

"None"

openedx-webhooks · 2021-10-21T15:12:02Z

Thanks for the pull request, @xitij2000! I've created BLENDED-987 to keep track of it in Jira. More details are on the BD-38 project page.

When this pull request is ready, tag your edX technical lead.

Cup0fCoffee · 2021-10-22T22:20:29Z

cms/djangoapps/contentstore/signals/handlers.py

Not sure if it's a comment or code commented out.

Right now I'm leaving this comment out since we don't have the new discussion provider set up or usable. The idea is to only send this signal if the provider needs it.

In this case I think we can just move this logic to inside the task.

xitij2000 · 2021-10-23T08:54:06Z

openedx/core/djangoapps/discussions/handlers.py

If a course_wide topic is deleted we need to handle moving all content from that topic to the general topic which can't be deleted.

For this we can introduce a new signal: topic_deleted here: https://github.com/edx/edx-platform/blob/master/openedx/core/djangoapps/django_comment_common/signals.py

The signal handler for this can then execute a query to move over all content.

Note for future: Might be useful to also have events for topic_created and topic_edited though I'm not sure if we should just have this code send signal and move the logic to signal handlers, since it seems inefficient, and I don't have a real use case other than delete yet.

xitij2000 · 2021-10-23T08:55:00Z

openedx/core/djangoapps/discussions/models.py

This is meant to be for the new discussions experience. It will share most code with the legacy experience.

xitij2000 · 2021-10-23T08:56:31Z

openedx/core/djangoapps/discussions/models.py

We don't really need a group right now since we don't have different topic ids across cohorts. This makes it easier to merge them since you can just stop filtering for group_id if cohorting is disabled.

However, a future in-context external provider might need this.

If preferable, we can remove this for now.

xitij2000 · 2021-10-23T09:00:39Z

openedx/core/djangoapps/discussions/handlers.py

The APIs and other mechanism already make sure that changes applied are in sync across the course structure and the models.

Cup0fCoffee · 2021-10-27T01:07:21Z

👍

I tested this: followed the testing steps
I read through the code
Includes documentation

asadazam93 · 2021-11-08T14:03:01Z

cms/djangoapps/contentstore/signals/handlers.py

Does this need to be triggered every time? Or should it be triggered only if the course has in-context discussions enabled?

I don't want to add that logic to a generic place. So that logic is in the task itself. Additionally, there may be other things that this code can do which apply to providers that don't support in-context discussions as well.

asadazam93 · 2021-11-08T14:04:12Z

openedx/core/djangoapps/discussions/data.py

What is this external_id used for?

The external_id is what will store the commentable id that is used by cs_comments_service. In the future, if we are dealing with another provider that supports in-context discussions, this can be used for them as well.

openedx/core/djangoapps/discussions/handlers.py

asadazam93 · 2021-11-12T06:12:56Z

@xitij2000 Can you please resolve the conflicts on this PR?

The new discussion configuration system links discussion topics directly to the course structure. This change adds a new task that sends a discussion update signal if there are any changes to the course. This signal includes all the context needed to update the configuration of the course. The handler for this new event will create a new entry for each unit that needs a topic in the database. In the future this will be used to see the topics in the course.

xitij2000 · 2021-11-12T06:21:04Z

@xitij2000 Can you please resolve the conflicts on this PR?

Done.

edx-status-bot · 2021-11-12T06:59:57Z

Your PR has finished running tests. There were no failures.

asadazam93 · 2021-11-12T11:13:27Z

@xitij2000 Is this ready to be merged?

xitij2000 · 2021-11-12T11:15:36Z

@xitij2000 Is this ready to be merged?

Yes.

openedx-webhooks · 2021-11-12T14:08:50Z

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

edx-pipeline-bot · 2021-11-12T15:12:48Z

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

edx-pipeline-bot · 2021-11-12T16:30:10Z

EdX Release Notice: This PR has been deployed to the production environment.

awaisdar001 · 2021-11-13T07:18:42Z

openedx/core/djangoapps/discussions/models.py

+    'openedx': {
+        'features': [
+            Features.BASIC_CONFIGURATION.value,
+            Features.PRIMARY_DISCUSSION_APP_EXPERIENCE.value,
+            Features.QUESTION_DISCUSSION_SUPPORT.value,
+            Features.COMMUNITY_TA_SUPPORT.value,
+            Features.REPORT_FLAG_CONTENT_TO_MODERATORS.value,
+            Features.AUTOMATIC_LEARNER_ENROLLMENT.value,
+            Features.ANONYMOUS_POSTING.value,


Why this provider was added? this actually breaks the discussions app on stage/prod.
https://course-authoring.stage.edx.org/course/course-v1:UBCx+Water201x_2+2T2015/pages-and-resources/discussion/settings

@awaisdar001 I think I mentioned this on one of the calls. It's the provider for the new experience.

I have a PR ready to add support for it in the MFE here: openedx/frontend-app-authoring#217

I think I should have gotten that merged first, or included this change with a separate PR.

@xitij2000 the mentioned PR is not ready yet, can you suggest how we can address this "bad user experience" in the short term? Maybe only revert the provide change in a separate PR?

@awaisdar001 I will make a PR that removes this provider right now.

https://github.com/edx/edx-platform/pull/29314

thanks @xitij2000

openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels Oct 21, 2021

xitij2000 force-pushed the kshitij/tnl-8623/discussons-signals branch 3 times, most recently from 0d0fd11 to 11de315 Compare October 22, 2021 15:40

Cup0fCoffee reviewed Oct 22, 2021

View reviewed changes

xitij2000 commented Oct 23, 2021

View reviewed changes

xitij2000 force-pushed the kshitij/tnl-8623/discussons-signals branch 3 times, most recently from 6d33b98 to d6d3dd4 Compare October 26, 2021 18:26

xitij2000 force-pushed the kshitij/tnl-8623/discussons-signals branch 5 times, most recently from 3afc4df to b508fb3 Compare November 1, 2021 09:18

asadazam93 reviewed Nov 8, 2021

View reviewed changes

xitij2000 force-pushed the kshitij/tnl-8623/discussons-signals branch 2 times, most recently from 1c1a2e2 to 98b18d6 Compare November 9, 2021 04:04

xitij2000 mentioned this pull request Nov 11, 2021

feat: Add support for the new Open edX discussion provider [BD-38] [TNL-8622] openedx/frontend-app-authoring#217

Merged

xitij2000 added 3 commits November 12, 2021 11:50

fix: add support for marking a provider as supporting LTI

abc3f68

fix: review feedback

7d2bab5

xitij2000 force-pushed the kshitij/tnl-8623/discussons-signals branch from 98b18d6 to 7d2bab5 Compare November 12, 2021 06:20

asadazam93 approved these changes Nov 12, 2021

View reviewed changes

awaisdar001 approved these changes Nov 12, 2021

View reviewed changes

asadazam93 merged commit 285e2c4 into openedx:master Nov 12, 2021

openedx-webhooks added merged and removed needs triage labels Nov 12, 2021

Agrendalath deleted the kshitij/tnl-8623/discussons-signals branch November 12, 2021 14:16

awaisdar001 reviewed Nov 13, 2021

View reviewed changes

This was referenced Feb 15, 2022

feat: Discussion event extraction, in openedx-event library openedx/openedx-events#45

Merged

refactor: modified the imports to use openedx-events library #29930

Merged

feat: Adds a new discussion topic configuration mechanism [BD-38] [TNL-8623] [BB-4968] #29082

feat: Adds a new discussion topic configuration mechanism [BD-38] [TNL-8623] [BB-4968] #29082

Uh oh!

Conversation

xitij2000 commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Dependencies

Supporting information

Testing instructions

Deadline

Uh oh!

openedx-webhooks commented Oct 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cup0fCoffee commented Oct 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

asadazam93 commented Nov 12, 2021

Uh oh!

xitij2000 commented Nov 12, 2021

Uh oh!

edx-status-bot commented Nov 12, 2021

Uh oh!

asadazam93 commented Nov 12, 2021

Uh oh!

xitij2000 commented Nov 12, 2021

Uh oh!

openedx-webhooks commented Nov 12, 2021

Uh oh!

edx-pipeline-bot commented Nov 12, 2021

Uh oh!

edx-pipeline-bot commented Nov 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

xitij2000 commented Oct 21, 2021 •

edited

Loading