[BD-38][TNL-7615] [BB-4846] feat: Add discussion_enabled for Unit #28864

farhaanbukhsh · 2021-09-28T12:54:33Z

This PR adds a discussion_enabled flag to each unit, so that instead of adding an xblocks for discussion,
the user can enable it using a checkbox or a switch. This is just an API-level implementation.

JIRA tickets: BB-4846, TNL-7615

Discussions: Link to any public dicussions about this PR or the design/architecture. Otherwise omit this.

Dependencies: None

Screenshots: Always include screenshots if there is any change to the UI.

Sandbox URL: TBD - sandbox is being provisioned.

Merge deadline: "None" if there's no rush, "ASAP" if it's critical, or provide a specific date if there is one.

Testing instructions:

Running the tests is the best way to see it working,

Author notes and concerns:

A lot of inspiration is taken from https://github.com/edx/edx-platform/pull/24380

Reviewers

@tinumide , @xitij2000

openedx-webhooks · 2021-09-28T12:54:37Z

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

natabene · 2021-09-28T20:11:14Z

@farhaanbukhsh Thank you for your contribution.

xitij2000 · 2021-09-29T10:44:58Z

common/lib/xmodule/xmodule/vertical_block.py

I think this should be moved to SequenceFields

That makes sense this only had the logic and not the fields. I should have noted this earlier 😅

@xitij2000 there is just one issue when I move this field to SequenceFields the MongoDB calls increase since now any Xblock that uses SequenceMixin will also have dscussion_enabled do you think creating another mixin to just add the discussion enabled field will be a better solution? Since then it will make way lesser call than now.

Depending on this I can make the changes to make the tests pass. :)

I'm not sure, to be honest. Let's keep it here for now. I'd like to get feedback from someone who understands this better.

tinuademargaret

👍

I tested that this PR adds a discussion_enabled flag to each unit
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

tinuademargaret · 2021-10-01T09:25:18Z

common/lib/xmodule/xmodule/seq_module.py

Should the discussion here start with D? I think Title case is the intent here, right?

@tinumide I thinks this is okay :)

xitij2000 · 2021-09-29T10:45:45Z

common/lib/xmodule/xmodule/vertical_block.py

It won't show up anywhere, but I think this should be changed to:

Suggested change

"Add discussion for the Unit."

"Enable in-context discussions for the Unit."

xitij2000 · 2021-09-29T10:46:38Z

common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py

Wow, this is a huge increase. Do you know why?

@xitij2000 IIUC when we introduce a new field the "find" call takes that into consideration and to generate the xml/tree it makes these extra calls.

jristau1984 · 2021-10-06T18:33:37Z

@farhaanbukhsh - Can you please add the "[BD-38]" label to this PR to ensure our workflow automation picks this request up and handles it appropriately? Thanks!

farhaanbukhsh · 2021-10-07T01:00:11Z

@farhaanbukhsh - Can you please add the "[BD-38]" label to this PR to ensure our workflow automation picks this request up and handles it appropriately? Thanks!

Added, thanks @jristau1984 😄

xitij2000 · 2021-10-12T07:10:59Z

common/lib/xmodule/xmodule/seq_module.py

Suggested change

scope=Scope.content,

scope=Scope.settings,

I think you should move this to a new mixin class for VerticalBlock called VerticalFields so that it's only added to the vertical block.

saadyousafarbi · 2021-10-14T06:54:05Z

common/lib/xmodule/xmodule/modulestore/tests/test_mongo_call_count.py

Why have the calls increased so drastically from 38 to 131?

@farhaanbukhsh ^

hey @saadyousafarbi and @awaisdar001, I don't know the concrete reason yet, I will try to dig down on the calls and see what is going on.

But on a shallow level, this looks like when a new field is introduced the new find calls are made and since this field is introduced in SequenceFields so all places that are using Sequence Field and Sequence Mixing are going to do additional find calls. This seems to be bumping the number of calls, I will dig deeper and get to the bottom of this and probably I will have better proof.

thanks that would be helpful.

@saadyousafarbi, @awaisdar001: Thank you for flagging this. That is a serious performance regression, and is definitely something we would want to avoid merging if possible.

@farhaanbukhsh: I suspect this is because your new XBlock field is content scoped–meaning that it gets put in a separate definition doc for each sequence, which is constantly being reloaded every time that field is accessed in outline traversal operations. If that's the case, switching it to settings scope as @xitij2000 suggested should take care of it (that will place all data for the field across all instances of the course in one structure document).

FYI @edx/perf-interest

@ormsbee I think the content scoping is the issue. I've tested locally by setting the scope of settings and the counts remain the same.

@ormsbee @xitij2000 That's a beautiful catch :)

The scope change did make the calls come back to normal for me on my devstack. I am still waiting for the tests to run on the CI.

cms/djangoapps/contentstore/views/tests/test_discussion_enabled.py

farhaanbukhsh · 2021-10-25T13:47:19Z

jenkins run all

farhaanbukhsh · 2021-10-25T14:54:04Z

@awaisdar001 This is ready for review :)

common/lib/xmodule/xmodule/seq_module.py

xitij2000

👍

I tested this: tested by checking for the field via python shell
I read through the code
[na] I checked for accessibility issues
[na] Includes documentation

cms/djangoapps/contentstore/views/tests/test_discussion_enabled.py

cms/djangoapps/contentstore/views/item.py

awaisdar001

Pass - 2 done, I just have one question regarding the code raising "JSONDecodeError" when a user is not authorized to get/set vertical discussion status. otherwise the PR looks good to know.

saadyousafarbi

can you address the questions by @awaisdar001 on the PR?
Other than that, this looks good!

awaisdar001

thanks, looks good to me.

👍 once the tests pass.

awaisdar001 · 2021-11-02T19:48:10Z

fyi @ormsbee if you would like another look at the PR.

awaisdar001 · 2021-11-03T10:41:13Z

@farhaanbukhsh can you rebase and squash the commits?

For each unit discussion_enabled flag is added so that each unit can be made discussable when needed. Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>

edx-status-bot · 2021-11-03T12:43:06Z

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

farhaanbukhsh · 2021-11-03T13:04:43Z

@farhaanbukhsh can you rebase and squash the commits?

@awaisdar001 done :)

xitij2000 · 2021-11-08T11:15:43Z

@awaisdar001 Is this good to merge?

awaisdar001 · 2021-11-08T11:16:49Z

Yes - Merged!!

openedx-webhooks · 2021-11-08T11:17:14Z

@farhaanbukhsh 🎉 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-08T12:21:06Z

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

edx-pipeline-bot · 2021-11-08T14:36:06Z

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

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Sep 28, 2021

farhaanbukhsh force-pushed the farhaan/bb-4846-add-discussion-enabled-flag branch from 50dffa8 to 46f79bd Compare September 28, 2021 17:17

openedx-webhooks added awaiting prioritization and removed needs triage labels Sep 28, 2021

xitij2000 reviewed Sep 29, 2021

View reviewed changes

farhaanbukhsh force-pushed the farhaan/bb-4846-add-discussion-enabled-flag branch 3 times, most recently from 8627544 to df16ce5 Compare September 30, 2021 06:18

tinuademargaret approved these changes Oct 1, 2021

View reviewed changes

xitij2000 reviewed Oct 1, 2021

View reviewed changes

farhaanbukhsh changed the title ~~[TNL-7615] [BB-4846] feat: Add discussion_enabled for Unit~~ [BD-38][TNL-7615] [BB-4846] feat: Add discussion_enabled for Unit Oct 7, 2021

openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage and removed open-source-contribution PR author is not from Axim or 2U awaiting prioritization labels Oct 7, 2021

xitij2000 reviewed Oct 12, 2021

View reviewed changes

saadyousafarbi requested review from asadazam93 and saadyousafarbi October 13, 2021 06:13

saadyousafarbi reviewed Oct 14, 2021

View reviewed changes

awaisdar001 assigned asadazam93 and saadyousafarbi Oct 15, 2021

awaisdar001 reviewed Oct 20, 2021

View reviewed changes

cms/djangoapps/contentstore/views/tests/test_discussion_enabled.py Outdated Show resolved Hide resolved

awaisdar001 reviewed Oct 20, 2021

View reviewed changes

cms/djangoapps/contentstore/views/tests/test_discussion_enabled.py Outdated Show resolved Hide resolved

awaisdar001 reviewed Oct 20, 2021

View reviewed changes

cms/djangoapps/contentstore/views/tests/test_discussion_enabled.py Outdated Show resolved Hide resolved

awaisdar001 reviewed Oct 20, 2021

View reviewed changes

cms/djangoapps/contentstore/views/tests/test_discussion_enabled.py Outdated Show resolved Hide resolved

xitij2000 mentioned this pull request Oct 21, 2021

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

Merged

farhaanbukhsh force-pushed the farhaan/bb-4846-add-discussion-enabled-flag branch 2 times, most recently from 911a805 to 0a0867d Compare October 25, 2021 14:10

xitij2000 suggested changes Oct 26, 2021

View reviewed changes

common/lib/xmodule/xmodule/seq_module.py Outdated Show resolved Hide resolved

xitij2000 reviewed Oct 26, 2021

View reviewed changes

common/lib/xmodule/xmodule/seq_module.py Outdated Show resolved Hide resolved

xitij2000 approved these changes Oct 26, 2021

View reviewed changes

awaisdar001 reviewed Oct 29, 2021

View reviewed changes

cms/djangoapps/contentstore/views/tests/test_discussion_enabled.py Outdated Show resolved Hide resolved

awaisdar001 reviewed Oct 29, 2021

View reviewed changes

cms/djangoapps/contentstore/views/item.py Outdated Show resolved Hide resolved

awaisdar001 reviewed Oct 29, 2021

View reviewed changes

saadyousafarbi approved these changes Nov 1, 2021

View reviewed changes

awaisdar001 approved these changes Nov 2, 2021

View reviewed changes

farhaanbukhsh force-pushed the farhaan/bb-4846-add-discussion-enabled-flag branch from 0b0cd72 to 8ea281a Compare November 3, 2021 04:09

feat: Add discussion_enabled for Unit

f1ce013

For each unit discussion_enabled flag is added so that each unit can be made discussable when needed. Signed-off-by: Farhaan Bukhsh <farhaan@opencraft.com>

farhaanbukhsh force-pushed the farhaan/bb-4846-add-discussion-enabled-flag branch from 6bfb236 to f1ce013 Compare November 3, 2021 12:09

awaisdar001 merged commit cfe75e9 into openedx:master Nov 8, 2021

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

farhaanbukhsh deleted the farhaan/bb-4846-add-discussion-enabled-flag branch November 8, 2021 12:55

JuanDavidBuitrago mentioned this pull request Feb 15, 2022

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

Merged

	"Add discussion for the Unit."
	"Enable in-context discussions for the Unit."

[BD-38][TNL-7615] [BB-4846] feat: Add discussion_enabled for Unit #28864

[BD-38][TNL-7615] [BB-4846] feat: Add discussion_enabled for Unit #28864

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

natabene commented Sep 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

farhaanbukhsh Sep 30, 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!

tinuademargaret left a comment

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!

jristau1984 commented Oct 6, 2021

Uh oh!

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

ormsbee Oct 20, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

farhaanbukhsh commented Oct 25, 2021

Uh oh!

farhaanbukhsh commented Oct 25, 2021

Uh oh!

Uh oh!

Uh oh!

xitij2000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

awaisdar001 left a comment

Choose a reason for hiding this comment

Uh oh!

saadyousafarbi left a comment

farhaanbukhsh commented Sep 28, 2021 •

edited

Loading

openedx-webhooks commented Sep 28, 2021 •

edited

Loading

farhaanbukhsh Sep 30, 2021 •

edited

Loading

ormsbee Oct 20, 2021 •

edited

Loading