docs: ADR for in context discussions [BD-38] [TNL-8512] [BB-4481]

[xitij2000]

This ADR describes the new mechanism for associating in-context discussions with units in which they will appear.

The new discussions approach no longer uses a discussion XBlock, but has a different approach for automatically linking discussions topics to Units. This ADR investigates how we can do that.

[openedx-webhooks]

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

[arjunsinghy96]

@xitij2000 This LGTM. I commented some minor nits, please look into it. I am conditionally approving this PR.

👍

• I tested this: (describe what you tested)
• I read through the document
• I checked for accessibility issues
• Includes documentation
• I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

[ormsbee]

Do we need to store anything else in order to make it so that the course app doesn't need to check with the modulestore in the process of rendering the discussion? So for example, the title, user partition group settings, teams, etc.?

[xitij2000]

I assumed this would already be cached in the DB in learning sequences, but I guess that doesn't capture unit-level data.
I will add additional field for split discussions and the unit title.

Teams is currently not in scope here, it already seems to have its way of linking to a discussion id.

[marcotuts]

Yes, Teams created topics via the teams topic configuration, and these are unlisted in the main discussion app topic selection they're only used to create discussions for each team discussion.

[ormsbee]

What does "custom visibility" mean?

[xitij2000]

I think more details are available in this mockup: https://www.figma.com/file/SeNpRff5HxzdpCgoD5pmlQ/Apps-and-resources-landing-page?node-id=3786%3A18911

Custom visibility means that you can toggle discussions at the individual unit level instead of enabling/disabling it for all units at once.

[ormsbee]

This might be a good place to consider what it would take to decouple this from the modulestore, for other apps that might want to implement discussions. Modulestore access is going to be one of the obstacles to testing and building course apps in general.

For instance, if the pattern for implementing an in-context discussion app is "catch the publish event, call modulestore's get_course() and then traverse the tree looking for the data you need", that ties it tightly to edx-platform and the idiosyncrasies of modulestore. Testing will involve building little courses with ItemFactory, further tying it to edx-platform.

But suppose we decide to create a new DiscussableContentUpdated signal, one that might even live in the openedx-events repo. And say that event contains everything we would normally extract from the modulestore to seed our discussion app data model for this course version–course config, lists of usage keys, titles, etc.

In that scenario, there's something that lives in the Studio process that understands the modulestore and knows how to generate a DiscussableContentUpdated event and generates one whenever a course is published, but then that event becomes the shared dependency that any discussions integration app has to implement. That will make testing and development of third party alternatives much simpler, because the hard part that touches modulestore will already be done for them. They (and our own discussions app) can be insulated from where that content data comes from, and all the unnecessary details that they shouldn't have to worry about.

It's almost the same amount of code to write. We'd mainly be shifting it so that instead of the app understanding modulestore, we have something in Studio extract data out of the modulestore and push it into a simplified data model for the app. We do something like this with Learning Sequences where there is Studio code that listens for a course publish and does the necessary extraction so that the learning_sequences app avoids the modulestore dependency.

[ormsbee]

FYI @mariajgrimaldi and @felipemontoya, as this might be a good use case for openedx-events, but I don't know when that might be stable enough for general use.

[xitij2000]

I think that makes a lot of sense and makes the process a lot cleaner. In fact, since the only thing needed here are the usage keys for units, with some minimal context, we can probably use use the existing blocks or learning sequences APIs to get the relevant data, and the signal can be lighter.

[ormsbee]

Learning Sequences doesn't currently have Unit data (though I do want that later). Since you're hooking into the publish cycle and block transformers are not guaranteed to have run yet, I'd suggest just going straight to the modulestore for generating this event.

[ormsbee]

... and sadly, I think you'll need to do the normal crawling-over-the-course iteration, because using query params on the modulestore fetch to grab you just vertical instances will also return you nested vertical blocks that are inside of content library blocks, which probably isn't what you want. :-(

[xitij2000]

OK. I will update the ADR to take this into account.

[felipemontoya]

Sorry to be late to this discussion. Still I better leave my comments here.

About the decoupling via an openedx-event, yes, that is exactly what the framework supports. According to the comment by @xitij2000

In fact, since the only thing needed here are the usage keys for units, with some minimal context

Then we would also want an attr class that can hold the required data using python basic datastructures and usage keys (we already support opaque_keys in the attr classes).

In terms of availability, we are doing the final push to get the first 3 events in the the https://github.com/edx/edx-platform/pull/28266 in the next few days and we expect to get 8 events supported before maple is cut. By then also filters should be available although those are not needed for this I think.

[xitij2000]

@felipemontoya I think that sounds perfect for this use case.
Good to know it's close to landing in the platform!

[ormsbee]

Wait, does that mean changing a course setting could potentially trigger changing/publishing every Unit in the course? Won't that autopublish the contents of any Unit that has draft content in it as a side-effect?

[xitij2000]

Hmm.. that is true, that would be a pretty bad side-effect.

One alternative I see is to store the discussion enabled status in the database only. In this case it won't be imported/exported, but unless custom visibility is enabled, the import/export will anyway re-create these links. For custom visibility this data can be stored in some other course field, i.e. a list of keys with discussions enabled, which will need special handling on import-export.

[ormsbee]

FWIW, I think it's fine to leave the field at the Unit level, as long as we don't do a mass reset when the config toggle changes at a high level. I don't think we should worry about keeping values in-sync, since people are going to have a chance to do weird things with OLX import anyway. The important thing is that the resulting configuration is unambiguous. So we can say, "Yeah, go ahead and flip this toggle at the Unit level, but we'll just ignore it unless custom_visibility is true." We can also reflect that in the Unit UX without having to do too much extra work (i.e. disable it in the UI when the course settings indicate it won't be used).

[xitij2000]

👍🏽
Sure, I was hoping to just use that flag in the Unit as a single source of truth when determining if a unit has discussions enabled or not, but it seems doing that will just move complexity from one place to another.

[xitij2000]

@ormsbee I've updated the ADR based on your suggestions. Could you have another look?

[awaisdar001]

@ormsbee Can you take another look when you have time?

thanks :)

[ormsbee]

Sorry for the long delay. Thank you for the updates. I have a minor typo fix and a couple of non-blocking model questions, but the model stuff can be hashed out elsewhere.

[ormsbee]

I'm not sure what group_id means here, but I'm fine with it going forward as is and hashing out any model issues over time.

[xitij2000]

It is the id used to separate divided discussions. It corresponds to the same field in cs_comments_service.

[ormsbee]

Why is this a str instead of a foreign key to a model representing the discussion provider?

[xitij2000]

Discussion providers don't currently have a model representation. They are currently all in a dict here:

https://github.com/edx/edx-platform/blob/84441bfd3a28862ab01834accb0c41a49c57a924/openedx/core/djangoapps/discussions/models.py#L90-L221

[edx-status-bot]

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

[awaisdar001]

@asadazam93 Discuss this PR and I think we can merge this now.

[edx-pipeline-bot]

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

[edx-pipeline-bot]

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