feat: Adds discussions settings for new discusions experience [BD-38] [TNL-8621] [BB-4854] #28749

xitij2000 · 2021-09-15T13:49:53Z

Description

This commit adds new discussions settings for the new discussions experience. These are stored in the course so they can be a part of course import/export flow.
These are also added to the discussions configuration API to allow MFEs to update the settings.

Supporting information

TNL-8621

Testing instructions

Use the updates course authoring MFE from here: fix: Update the API path and fix saving settings [BD-38] [TNL-8621] [BB-4854] frontend-app-authoring#197
Change the legacy provider settings for anonymous posting settings, and make sure they are persisted
The settings should be saved to the coruse structure as well which you can verify using the shell

Deadline

"None"

openedx-webhooks · 2021-09-15T13:50:02Z

Thanks for the pull request, @xitij2000! I've created BLENDED-963 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.

xitij2000 · 2021-10-06T15:51:41Z

jenkins run python

xitij2000 · 2021-10-06T16:34:18Z

cms/djangoapps/models/settings/course_metadata.py

This will hide these new settings from the advanced settings UI.

Might be worth removing it from here while testing to have an easy way to check for changes.

xitij2000 · 2021-10-06T16:34:41Z

cms/envs/common.py

Now that I've added a plugin entrypoint, this isn't needed any more.

xitij2000 · 2021-10-06T16:35:33Z

lms/djangoapps/discussion/django_comment_client/utils.py

Moving these over allows us to import this in both LMS and studio code.

openedx/core/djangoapps/discussions/apps.py

xitij2000 · 2021-10-06T16:38:01Z

openedx/core/djangoapps/discussions/models.py

We simply set the default here instead of in a dozen other places.

xitij2000 · 2021-10-06T16:39:01Z

openedx/core/djangoapps/discussions/serializers.py

This lets the standard machinery handle the regular fields and we can focus on the complex cases.

xitij2000 · 2021-10-06T16:40:31Z

openedx/core/djangoapps/discussions/serializers.py

This is a setting that specific to the inbuild forums, so it can be stored in the plugin configuraiton.

xitij2000 · 2021-10-06T16:41:15Z

openedx/core/djangoapps/discussions/serializers.py

This is direclty moved over.

xitij2000 · 2021-10-06T16:42:04Z

openedx/core/djangoapps/discussions/utils.py

All this code is just moved over without any changes other than adding type hints.

Agrendalath

@xitij2000, I've tested that the changes are persisted when I'm setting them through the advanced settings. From your first point, it looks that we should also test this with the frontend-app-course-authoring, right? When I'm visiting the discussions settings, I'm getting the following error:

TypeError
can't access property "map", basic is undefined
Call Stack
 FeaturesTable
  src/pages-and-resources/discussions/app-list/FeaturesTable.jsx:96:12
 renderWithHooks
  node_modules/react-dom/cjs/react-dom.development.js:14803:27
 mountIndeterminateComponent
  node_modules/react-dom/cjs/react-dom.development.js:17482:13
 beginWork
  node_modules/react-dom/cjs/react-dom.development.js:18596:16
 callCallback
  node_modules/react-dom/cjs/react-dom.development.js:188:14
 invokeGuardedCallbackDev
  node_modules/react-dom/cjs/react-dom.development.js:237:16
 invokeGuardedCallback
  node_modules/react-dom/cjs/react-dom.development.js:292:31
 beginWork$1
  node_modules/react-dom/cjs/react-dom.development.js:23203:28
 performUnitOfWork
  node_modules/react-dom/cjs/react-dom.development.js:22154:12
 workLoopSync
  node_modules/react-dom/cjs/react-dom.development.js:22130:22

openedx/core/djangoapps/discussions/utils.py

lms/djangoapps/discussion/django_comment_client/utils.py

openedx/core/djangoapps/discussions/apps.py

openedx/core/djangoapps/discussions/docs/decisions/0004-in-context-discussions-linking.rst

openedx/core/djangoapps/discussions/tests/test_views.py

openedx/core/djangoapps/discussions/serializers.py

Agrendalath

👍

I tested this: verified that the course settings are applied correctly through the MFE
I read through the code
I checked for accessibility issues: n/a
Includes documentation
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

openedx/core/djangoapps/discussions/apps.py

openedx/core/djangoapps/discussions/serializers.py

awaisdar001 · 2021-10-18T18:34:15Z

openedx/core/djangoapps/discussions/serializers.py

I know it doesn't load the course every time, but let's make it simpler and store that value in a variable course = self._get_course() to make it more readable.

after refactor this would becomecourse. discussions_settings[key] = value

We can do that but in that case every call to this API will load the course, even if it doesn't need to.

Can you explain how it would be any different in loading the course with respect to storing it in var and using it and directly calling the get_course? Here is what I mean.

Approch 1: self._get_course().discussions_settings[updated_provider_type] = value self._get_course().discussions_settings[key] = value

Approch 2: course = self._get_course() course.discussions_settings[updated_provider_type] = value course.discussions_settings[key] = value ... modulestore().update_item(course, self.context['user_id'])

I found approach 2 more readable as we are updating the "same" course we have modified.

Ah sure! I thought you meant loading it once up top and using that. Will update.

Actually, on reading through the code again, I think the approach you've mentioned will cause a performance hit when the course doesn't need to be loaded. I'll explain why.

There are three places that self._get_course() is used:

Line 336: Here, we only want the course to be loaded if there is a setting that has changed, which is what we're checking in the if condition above. That is why we call get course only if that condition applies.

Line 342: Here once again we check if the configuration has changed, and if it has, we load the course and update the settings.

Line 352: Here, we only want to load the course if the settings have changed. At this point the course has definitely been loaded,

The approach you've outlined above won't work because each access to the course object is happening in a different conditional, so before applying each change we'd need to check if course is None and then load it, which is exactly why that was moved to a method in the first place.

openedx/core/djangoapps/discussions/serializers.py

This commit adds new discussions settings for the new discussions experience. These are stored in the course so they can be a part of course import/export flow. These are also added to the discussions configuraiton API to allow MFEs to update the settings. The discussions API is currently available via LMS, however that means it cannot save changes to the modulestore. This also adds the API to the studio config so it can now also be accessed from studio and be used to save course settings.

edx-status-bot · 2021-10-21T12:09:33Z

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

awaisdar001

👍

xitij2000 · 2021-10-26T18:13:27Z

@awaisdar001 This is good to merge on my side.

awaisdar001 · 2021-10-26T19:52:12Z

I'll merge this in the morning.

openedx-webhooks · 2021-10-27T10:41:31Z

@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-10-27T11:40:36Z

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

…#28749)" This reverts commit bb0c031.

edx-pipeline-bot · 2021-10-27T16:17:59Z

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

edx-pipeline-bot · 2021-10-27T16:17:59Z

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

openedx-webhooks added blended PR is managed through 2U's blended developmnt program waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Sep 15, 2021

xitij2000 force-pushed the kshiitj/discussions-structure-in-course branch 2 times, most recently from 66e4d95 to 92abd91 Compare October 6, 2021 09:38

xitij2000 marked this pull request as ready for review October 6, 2021 09:39

openedx-webhooks added needs triage and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 6, 2021

xitij2000 force-pushed the kshiitj/discussions-structure-in-course branch 2 times, most recently from 5b40d6f to a4d3a19 Compare October 6, 2021 12:52

xitij2000 commented Oct 6, 2021

View reviewed changes

openedx/core/djangoapps/discussions/apps.py Outdated Show resolved Hide resolved

xitij2000 commented Oct 6, 2021

View reviewed changes

Agrendalath reviewed Oct 9, 2021

View reviewed changes

xitij2000 force-pushed the kshiitj/discussions-structure-in-course branch 4 times, most recently from 4a92c14 to 2699f07 Compare October 11, 2021 08:45

asadazam93 requested review from AhtishamShahid, asadazam93 and saadyousafarbi October 11, 2021 12:48

Agrendalath approved these changes Oct 13, 2021

View reviewed changes

awaisdar001 assigned asadazam93 Oct 15, 2021

awaisdar001 reviewed Oct 18, 2021

View reviewed changes

openedx/core/djangoapps/discussions/apps.py Outdated Show resolved Hide resolved

awaisdar001 reviewed Oct 18, 2021

View reviewed changes

openedx/core/djangoapps/discussions/serializers.py Outdated Show resolved Hide resolved

awaisdar001 reviewed Oct 18, 2021

View reviewed changes

openedx/core/djangoapps/discussions/serializers.py Outdated Show resolved Hide resolved

xitij2000 force-pushed the kshiitj/discussions-structure-in-course branch from 2699f07 to c104fbd Compare October 19, 2021 15:15

xitij2000 mentioned this pull request Oct 21, 2021

fix: Update the API path and fix saving settings [BD-38] [TNL-8621] [BB-4854] openedx/frontend-app-authoring#197

Merged

xitij2000 force-pushed the kshiitj/discussions-structure-in-course branch from 9d717a5 to ffc158c Compare October 21, 2021 11:33

xitij2000 force-pushed the kshiitj/discussions-structure-in-course branch from ffc158c to 98a9b8e Compare October 21, 2021 11:35

xitij2000 mentioned this pull request Oct 21, 2021

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

Merged

awaisdar001 approved these changes Oct 25, 2021

View reviewed changes

asadazam93 approved these changes Oct 27, 2021

View reviewed changes

asadazam93 merged commit bb0c031 into openedx:master Oct 27, 2021

openedx-webhooks added merged and removed needs triage labels Oct 27, 2021

xitij2000 deleted the kshiitj/discussions-structure-in-course branch October 27, 2021 11:29

ormsbee pushed a commit that referenced this pull request Oct 27, 2021

Revert "feat: Adds discussions settings for new discusions experience (…

525439d

…#28749)" This reverts commit bb0c031.

ormsbee mentioned this pull request Oct 27, 2021

Revert "feat: Adds discussions settings for new discusions experience [BD-38] [TNL-8621] [BB-4854]" #29129

Merged

ormsbee pushed a commit that referenced this pull request Oct 27, 2021

Revert "feat: Adds discussions settings for new discusions experience (…

5d01323

…#28749)" This reverts commit bb0c031.

xitij2000 restored the kshiitj/discussions-structure-in-course branch October 27, 2021 15:19

awaisdar001 mentioned this pull request Oct 27, 2021

feat: Adds discussions settings for new discusions experience [BD-38] [TNL-8621] [BB-4854] #29131

Merged

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 discussions settings for new discusions experience [BD-38] [TNL-8621] [BB-4854] #28749

feat: Adds discussions settings for new discusions experience [BD-38] [TNL-8621] [BB-4854] #28749

Uh oh!

Conversation

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

Uh oh!

Description

Supporting information

Testing instructions

Deadline

Uh oh!

openedx-webhooks commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xitij2000 commented Oct 6, 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!

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!

Agrendalath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Agrendalath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

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!

Uh oh!

edx-status-bot commented Oct 21, 2021

Uh oh!

awaisdar001 left a comment

Choose a reason for hiding this comment

Uh oh!

xitij2000 commented Oct 26, 2021

Uh oh!

awaisdar001 commented Oct 26, 2021

Uh oh!

openedx-webhooks commented Oct 27, 2021

Uh oh!

edx-pipeline-bot commented Oct 27, 2021

Uh oh!

edx-pipeline-bot commented Oct 27, 2021

Uh oh!

edx-pipeline-bot commented Oct 27, 2021

xitij2000 commented Sep 15, 2021 •

edited

Loading

openedx-webhooks commented Sep 15, 2021 •

edited

Loading

awaisdar001 Oct 18, 2021 •

edited

Loading