Standup Meeting Notes with OpenCraft

16 Nov, 2021

Question: Ref to PR https://github.com/edx/edx-platform/pull/29262 , why is the tab reordering needed?

Resolution: Marco has requested this ordering. It is a product decision. Please refer to https://openedx.atlassian.net/browse/TNL-9174

Concern: https://github.com/edx/frontend-app-discussions/pull/32 The test data is quite big and may result in longer test runs.

Resolution: The test data is big because it is using the exact same data as for the demo course. This makes the test more reliable since it is an actual data set and not a dummy one. Also, if there are any changes to the course in future, we can fetch and update the test data with a single API call. If, however, we find that this particular data set is resulting in prohibitively longer test run times, we can circle back and reduce the amount of data. For now, lets proceed as is.

18 Nov, 2021

Question: I believe the discussion tab should include the mfe link if the provider we have set is “edX” provider? What if non edX provider is set in the DiscussionConfiguration model?

Resolution: Discussion tab is only used for edx providers. When using LTI, the URL and tabs will be entirely different.

Update: . Since we intend to enable discussions MFE for on-going courses, we need to figure out what goes in the breadcrumb bar. Because the current discussions schema does not conform to course hierarchy but our new MFE does. @Kshitij Sobti is going to explore how the existing topics can be mapped to the new MFE requirements. If found to be too much work, we may go for skipping breadcrumb bar in the legacy experience.

Update: For following PRs, only tests need to be added:

Recommendation: Having separate flags for rendering new MFE and in-context discussions may come in handy if we decide to role out one feature but not the other. Need team’s feedback on this.

Blocker: For v1.7, we need mockups for UX on mobile.

23 Nov, 2021

Questions: Ref to PR :

1. Why are we fetching data every time a new page is loaded? Why can we not cache this data, once fetched?

2. Are standard stats for API request are sufficient or do we need to add new relic traces?

3. Will traces be added to the same PR?

4. Will performance testing be done by OC or Infinity?

5. What courses do we need to target?

Resolution:

1. As of now, it is not clear if the current mongo query is the way to go. It will become clear after testing performance of the query. If the performance is fine, then we can improve pagination logic and select a caching system.

2. Need to add new relic traces to differentiate between time spent on fetching data from DB, processing and rendering.

3. Yes

4. Infinity. OC does not have access to staging or production servers.

5. Infinity to decide.

Update: Ref to PR . Breadcrumbs will be added to the new MFE. For discussions created using legacy experience, exploration still being done on how/whether or not to show the breadcrumb bar.

Question: Which providers require PII?

Resolution: OC has tested integrations with Discourse, YD and Piazza. Discourse requires PII info sharing or the implementation breaks. Piazza and YD do not require PII and assign an ID to the user. Users can chose their name and username.

25 Nov, 2021

Question: Ref to PR , is it ready for review? It is Draft and some tests are failing.

30 Nov, 2021

No meeting

2 Dec, 2021

Question: In reference to enabling LTI sharing for programs:

1. Should we create a new model for programs?

2. Should we create this model in LTI consumer XBlock of somewhere else?

Resolution:

1. Program model shall be created from scratch

2. This model will be created in LTI consumer Xblock.

Concern: In reference to PR , currently both LTI and legacy settings are being returned. However, there is no way to differentiate between the two.

Resolution: The API needs to be split so that it returns legacy and LTI settings separately upon request. @Kshitij Sobti will write an ADR before implementation.

9 Dec 2021

Question: Ref:

Whats' the motivation of adding discussion topics v2? Where this API will be used? on the Course authoring MFE legacy discussion provider?

Response:

CS comment service does not have any concept of hierarchy. It only appreciates topic IDs to which posts, responses and comments are attached.

At present: A discussion xBlock requires specifying Category and Subcategory. An API parses through all the discussion topics in the CS comment service, and infers the hierarchy based on values of Category and Subcategory. For example if two topics have the same Category value, they will be assumed to be children of that Category.

Future: For in-context discussions (v1.7) we are aiming to align discussion topics hierarchy with course hierarchy. To achieve this, we are getting rid of the concept of Category and Subcategory. Instead each discussion topic will be associated with either a “Unit” or a “Course”. The discussion topics associated with each Unit will be rendered in the side-bar (in-context) when that Unit is accessed. The discussion topics associated with all Units and the course itself will be available in the Discussions tab. Topics associated with course will always be at the top level of the hierarchy. Topics associated with Units will be present in form of a 3 level hierarchy: Section-> Subsection → Topic (associated with Unit). Mapping between discussion topics and Units/Courses shall exist in the database. API in this PR will fetch this mapping and construct topic hierarchy by combining this mapping information with course hierarchy information (fetched from another already-existing API).

FYI: In reference to enabling switching between legacy discussion UI and the new MFE, OC will achieve this by using a query parameter in the request URL. A button on the banner will enable educators to perform the switch.

14 Dec 2021

Argument: Do we really need to depreciate “/” feature given that i’ll eventually be deprecated automatically once v1.7 is rolled out?

Counter-argument: Courses that have already created nested categories using “/” will be require some effort when they will be migrated to v1.7. Deprecating “/” now will reduce the total number of courses that use this feature thus reducing the amount of effort (on part of developers or educators).

Question: Inherent assumption in the counter-argument above is that “/” will cause additional effort during migration. My understanding is that:

1. cs_comment_service only appreciates topics and their IDs. These topics have category and subcategory as their property.

2. The “/” is nowhere used in the backend code. Backend and cs_comment_service treat Category name as a plain string (that may or may not contain “/”).

3. Only the frontend uses “/” for rendering purposes.

If thats the case, the we don’t need to worry “/” during migrations, and consequently no need to deprecate it now. Is my understanding correct?

Response:

I think in terms of deprecation, from the last conversation I think we discovered that this was never an officially supported feature. Additionally, in a lot of the courses that that seemed to have it the "/" was being used unintentionally.So for 1.7 there is no mechanism to specify a category and subcategory, the topics follow the course structure, which will be section -> subsection -> unit. When migrating to the new provider there is no way to retain the original topic structure, or names, unless the structure already follows the course structure, so there is no real way to support the '/' in the new system anyway.So we don't need to do anything special for migrating in those cases since that data is not retained and has no way of being retained.If a course keeps the legacy provider, but uses the new MFE then the topic structure they see will not be what they expect, for these cases we either need to deprecate the feature or implement it.Re: questions

1. cs_comments_service accepts a commentable_id when creating a thread/post. This id corresponds to the topic id. So the taxonomy of these topics isn't something it concerns itself with.

2. The backend code does use "/" for separation.  You can see the code [here}( ) however this is only used for the legacy UI. Even the mobile UI doesn't seem to use this code.

3. No. The frontend doesn't know about slashes. The backend creates a category structure taking slashes into account, and the frontend should be able to handle multiple nested levels of categories, however it seems at some point this particular piece of code broke and now the frontend doesn't support that anymore.

23 Dec 2021

Question: In reference to in-context discussions, what happens to discussion Xblock if course team deletes a section/subsection/unit?

Preferred outcome:

In such an event the preferred outcome would be to:

1. Discussion topics and messages associated with a deleted unit, will no longer be visible to learners. Learners will however see the posts that they have created or are following. These posts will be in “Closed state”.

2. Discussion topics and messages associated with a deleted unit, will be visible to staff and admin roles.

Consider a course named Math101 where course and discussions hierarchies are as follows:

If course team deletes a unit named Multiplication, the corresponding discussion topic will disappear from learner's view. It will however be visible to course admins under a general topic category named Archived. Its new topic name will reflect the entire 3 level hierarchy with an appropriate delimiter. Archived will appear after general topic and course topics and will only be visible if there are archived discussion topics in there.

If the course team deletes a subsection named Determinant, the corresponding discussion topics will appear as follows:

If the course team deletes a section named Matrices, the corresponding discussion topics will appear as follows:

Resolution:

This would have been easy to achieve we only aimed to preserve unit information. Unit information is already attached to topics. Kshitij will do some discovery to find out how much work it is to associate section and subsection info with the topics as well.

Question: In reference to in-context discussions, what happens to discussion Xblock if course team changes visibility of a section/subsection/unit?

Preferred outcome:

Visibility of discussion topics will mimic the visibility of the course units that they are associated with. Learners will however be able to see and interact with the posts that they have created or are following.

Resolution:

“The topic structure is build for the entire unfiltered course, and then can be filtered based on the current users”. Seems like this can be easily implemented.

Question: In reference to in-context discussions, what happens to discussion Xblock if course team changes hierarchy of the course? Specifically, if the course team:

1. Move unit(s) to another (existing or new) subsection

2. Move subsection to another (existing or new) section

Preferred outcome:

In both cases, we would prefer that discussion topics hierarchy mimics the hierarchy of the course.

Resolution:

Pending… (can we achieve this? If not, why? Challenges? Est. time?)

This is already achieved with the current implementation. No action required.

Question:

Do we have caching mechanism in place for performance reasons? If so, how would that affect the change in course hierarchy/ visibility/ deletion of course units?

Resolution:

“The topics are cached to the database instead of being directly loaded from the course structure. So the topic listing is one single database query.

If the course structure changes a signal mechanism triggers an async job to update the topics in the database, so there shouldn’t be much delay.

If we want to add further caching we can, and have the cache be cleared when any operation changes the topics.”

Question:

What is the difference between Open edX and Legacy discussion providers as both use cs_comments_service?

Resolution:

“The legacy provider is the name of the provider that already existed in the platform for discussions, before there was even the concept of providers.

The Open edX provider is a new provider that uses the same backend services as legacy but allows us to make breaking changes to the way discussions work (especially topics) without affecting existing courses on the legacy provider.”

Question: Re “/”, i agree with your comment that learners will not see what they expect if they were to view the new MFE (v1.6) with legacy backend. So now the questions are:

1. What do we want viewers to see when the use the new MFE?

2. How will we achieve what we want to see?

3. What side-effects could there be (so we can communicate them to partners)?

Resolution:

1. Consider the following discussion Xblock configuration. We would want “/” to simply be treated as a character in category name string.

Open

Open

2. “The screenshot shows below is comparatively straightforward to achieve, since it will just involve removing existing code and tests for handling nesting. It might still be timeconsuming since we’re dealing with existing django/mako/underscore template code and there may be bits and pieces strewn about the codebase for this.

The MFE already works the way that is presented in ‘What we want to show’ ”.

3. “It will not result in any data loss, but will just change how the topic structure is presented.”

28 Dec 2021

Question v1.6: We would prefer to review the UI and features of new discussions MFE on stage. What work needs to be done in to achieve this?

Resolution:

1. Discussion URL needs to be modified. This needs to be done before enabling new MFE on stage to avoid confusion over broken links afterwards.

2. Banner needs to be updated to provide option to toggle between legacy UI and the new MFE.

3. Existing PRs for bug fixes need to be merged.

Question v1.6: As per current implementation, once we enabled new MFE (v1.6) for a course, new UI will be visible in Discussions tab. However, the discussion xblocks in the content will use the legacy UI. Are we ok with that? If no, what do we need to do?

Resolution: Marco: “If the work for having new UI within the content is a lot, then we can skip it. It not, then we can rollout the educator preview and afterwards, make the necessary improvements to have the new UI within content”. FYI:@Kshitij Sobti

Question v1.6: Do we need to depreciate “/” before enabling the new MFE on stage?

Resolution: No. The new MFE uses same API as mobile app, which is already agnostic to “/”.

Question v1.6: Post deletion workflow is not specified in mockups.

1. We should implement a confirmation dialog box for deleting a post (to avoid accidental deletion)

2. What message do we want to show in that dialog box?

3. What should the UI look like for dialog box?

Resolution:

1. Agreed

2. Pending …

3. We can design it like the “Close post” dialog box in Figma mockups.

Question v1.6 v1.7: In reference to the waffle flags and toggle buttons for the new MFE (v1.6) and in-context discussions/new topic structure (v1.7), how do the two options co-exist?

1. Do we want to have 2 separate waffle flags; one for new MFE (v1.6) and one for in-context discussions (v1.7)?

   1. Yes

2. Do we agree on the following matrix?

Resolution: Pending …

Question v1.7: We need to figure out the mapping scheme between courses using legacy backend that want to switch to v1.7.

1. Can we defer this and create a separate milestone?

Resolution: Pending …

Comment v1.9 v.10: It’ll be really helpful to connect someone in edx who is proficient in ruby, for reviews on PRs for to cs_comment_service repo.

Info v1.9 v.10: Management command for creating stats for past user actions has been deferred until the use case calls for it. For now, potential use cases include:

1. Request from existing courses using v1.9 and v1.10

2. If a bug introduces inaccuracy in the current stats recording

30 Dec, 2021

Question: It appears that there is an API that fetches stats for a user in the legacy discussions experience. How is this API working since it does not seem to have performance issues and why did we needed a new one (apart from fetching reported content)?

Resolution: Pending …

11 Jan, 2022

Comment: OC: In reference to this item, at present, enabling the waffle flag for v1.6, the new view will be rendered in Discussions tab and the in-content discussion xblocks. Since this is what we want, no further work is required.

Comment: OC: Regarding migration of existing course discussions, we should create some automatic check as to whether the migration can be automated or not. For example, courses that do not contain more than 1 discussion block in a Unit and courses that do not reference the same discussion block in multiple Units etc.

Comment: Aamir: We need to figure out the qualification criteria for automated migration of courses.

18 Jan, 2022

Discuss: v1.6 How about deletion dialog boxes as seen below. OC can take a look at the paragon details.

Update:

Discuss: v1.6, v1.7, v1.9, v1.10, v1.11, v1.12 Rollout sequence

AA to answer following questions:

1. How many courses in past 6 months have been created?

2. How many of those can benefit from v1.7?

3. How many have 0 discussion xblocks? Real easy conversion.

4. How many have one discussion referenced in several xblocks.

5. ???

Question: v1.6 Will the new MFE be automatically enabled for Teams?

Resolution: No information about Teams at the moment. Will need to explore in future.

Question: v1.6 Can we enable the new MFE on stage now?

Resolution:

• Good to enable on stage.

• Default URL has been changed. This may require updating paths elsewhere.

• Flags that need to be enabled: No flags need to be enabled to get it working. We can visit MFE URL for any existing course.

Question: v1.9 v.10: What UI when stats don’t exist for a learner in a course?

Resolution: Pending …

25 Jan, 2022

Discuss: v1.9 v.10: Users are facing problems on production when editing a post, comment or response (as seen below and in CR-4472 and CR-4474). We think this maybe due to PR deployed on production on 18th Jan. Two potential solutions are:

1. Merge the corresponding platform PR

2. Rever this PR

Resolution: Reverting the PR makes more sense. OC will take a look at the PR to find the root cause. This is the revert PR

Question: What is the fate of PR for user stats: ?

Resolution: OC needs to rebase the PR after reverting the PR in the item above. This PR can be merged after rebase along with the corresponding platform PR: .

Question: Can Infinity take up the work for integrating the feedback tool for discussion?

Resolution: Yes. Helpful resources are:

1. Slack post by Marco

2. PR for feedback button:

3. Potential plugin:

1 Feb, 2022

Comment: Tab bar should be modelled as a separate component so that it can integrated with react apps.

Discussion: Timelines and milestones

Question: Why do we need a new provider for v1.7? Why not simply use a toggle?

Resolution: Initially, we’ll only show the option of new discussion provider to courses that fulfil the following criteria:

1. Current provider is NOT legacy provider

2. Flag for new MFE is enabled

3. Course has not been started yet

It is easier to implement these conditions with a new provider as compared to toggle.

22 Feb, 2022

Discuss: We need a separate toggle to hide v1.7 while rolling out v1.6.

Resolution: Agreed. New toggle will be named Enable_new_topic_structure. New discussion provider for v1.7 will only show up if this toggle is enabled in addition to these conditions.

Discuss: How do we know which bugs/ new tasks reported for new MFE in tracking sheet, are being picked up by OC or Infinity?

Resolution: @Kshitij Sobti will assign tickets, intended to be picked up by OC, to himself and mark them as prioritised. @Asad Azam @Aamir Ayub will assign tickets, intended to be picked up by Infinity, to themselves and mark them as prioritised.

28 Feb, 2022

Discussion: v1.7 Stages of new topic structure rollout is as follows:

1. At present: Only legacy topic structure is available. DONE

2. Transition period: This is a period where we would prefer limited number of NEW courses to try out the new topic structure and provide feedback.

   1. We don’t want all course teams to see 2 edX discussion providers (legacy and new topic structure).

   2. We want to have legacy topic structure as the default topic structure Done

   3. We want to have some mechanism to enable the new topic structure on NEW courses offered by interested partners. need grooming

3. Future: The new topic structure will be default for NEW courses. Legacy topic structure will be supported for running courses. need grooming

4. Migration: A running course wants to transition to the new topic structure. This is only possible under certain conditions. need grooming

Possible solution for 2c:

1. We keep the 2 edX providers on frontend named:

   1. edX (legacy topic structure)

   2. edX-new (new topic structure)

2. Only the global staff will be able to see edX-new provider.

3. In our partner portal post, we’ll ask partners to contact their respective edX support teams to get the new topic structure configured.

4. Once a partner support team receives the request, they’ll ask infinity to configure the new topic structure.

5. Infinity team will configure the edX-new provider for the course(s).

6. Once edX-new provider has been configured on a course, it will be visible to the course team (Can we easily implement this? @Asad Azam ). Course team can then make necessary changes in configurations.

7. Once edX-new provider has been configured, can they easily go back to the legacy topic structure (with help from edX staff)? @Kshitij Sobti

1 Mar, 2022

Question: v1.6 discussions MFE UI looks zoomed out than the original designs available in Figma

Discussion /solution: the default sizes in paragon are larger than the Figma we may have to apply class “small“ to match designs with Figma.

9 Mar, 2022

Discussion: v1.7 Orphaned posts needs update.

Scenario 1: If a archived topic does not contain any posts, should it be deleted automatically or manually? Need feedback for Marco.

Scenario 2: If a course team wants to clean-up archived topic: If a course team deletes the archived topic, what happens to posts in there? Three possible solutions:

Preference is to decouple the clean-up work and maybe groom and implement in future.

Discussion: v1.7 How course-reruns will opt-in to new topic structure?

Resolution: ADR coming up. Will be helpful to other people as well.

28 Mar, 2022

Discuss: v1.6

Conclusion:

Question: Related to users endpoint in the cs_comment_service, specifically to the users endpoint where we are getting this error ["Username is already taken"]  in response to this endpoint http://localhost:4567/api/v1/users/<user_id> (endpoint URL from local setup). This is happening when accessing the discussion tab for a specific learner in legacy experience!
Wanted any context around the specific error we are getting that could be useful, and is this expected, when is it actually raised?

Answer:

14 April 2022

Question: related to filtering posts by topics and also search I have a query related to a significant performance issue that was disclosed whiles testing/fixing some of the related issues,
the front-end makes unnecessary API calls, instead of directly querying for a particular topic under consideration, I would ideally want to know the reason behind the selection of the implemented solution and why not these filters were implemented in APIs to reduce complications and performance issues on the frontend.

13 May 2022

Question: related to feedback posted by kshitij on PR for adding mathjax support in TinyMCE,

• how do we know that the wiris plugin is sending data to third parties?

• kshitij replied that from the network tab while typing the expression the calls can be seen which are made to wiris site for image generation and analytics information (does not seem like an issue to me as wiris is well-managed tool and is in use by hundreds of users and we can connect with their team on privacy issues)

• licensing information is mentioned on their site and a license can also be purchased by connecting with their team

• do we have other solutions available? he has not worked on this thing before so he is kind of unfamiliar with any more solutions.

18 May 2022

Discuss:

Resolution:

Topics: All topics are already loaded and can be searched on the frontend, similar to legacy experience.

Users: Usernames are present in platform and cs_comment_service both. However, its better to fetch users from cs_comment_service because user stats are returned too. @Kshitij Sobti will explore how much of an effort it will be to search partial usernames.

Discuss:

Resolution:

abuse_flagged_count property of a post keeps count of any responses or comments that have been reported. Infinity to explore this further.

Discuss: Learner area new UI

Resolution: Kshitij pointed out that this new UI may not require modifications in cs_comment_service except for fetching threads that a particular user has interacted with. For threads, there are two options that @Asad Azam will explore further, in the order below:

1. Use legacy API (which may need modification to make it compatible with REST framework)

2. Modify the newly created cs_comment_service API to fetch threads

14 June 2022

Discuss:

Resolution:

Changes are to be made on the cs_comment_service to add the capacity to retrieve learners stats for a subset of users in a course. Right now, the api fetches stats for all users within a course. Some key points to keep in mind:

• username/email information of users are present in the platform,

• we will need to filter users based on the partial username on the platform side, and then request for stats on the cs_comment_service endpoint.

• Followup: we might want to look into who can access this endpoint? We might not want to make it accessible to all users as we dont want to make discussion stats available to all users

  • We might have to setup permissions on this endpoint to restrict all users from getting this information