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: https://github.com/edx/edx-platform/pull/29285/files#diff-518c435070f29d67bd5b37cd2946ec41cd30f0601a8284259093541b5a001b63R1237-R1250 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: https://github.com/edx/frontend-app-discussions/pull/36. 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:
https://github.com/edx/edx-platform/pull/29300
https://github.com/edx/frontend-app-course-authoring/pull/217
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 https://github.com/edx/cs_comments_service/pull/352 :
Why are we fetching data every time a new page is loaded? Why can we not cache this data, once fetched?
Are standard stats for API request are sufficient or do we need to add new relic traces?
Will traces be added to the same PR?
Will performance testing be done by OC or Infinity?
What courses do we need to target?
Resolution:
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.
Need to add new relic traces to differentiate between time spent on fetching data from DB, processing and rendering.
Yes
Infinity. OC does not have access to staging or production servers.
Infinity to decide.
Update: Ref to PR https://github.com/edx/frontend-app-discussions/pull/36. 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 https://github.com/edx/cs_comments_service/pull/352 , 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:
Should we create a new model for programs?
Should we create this model in LTI consumer XBlock of somewhere else?
Resolution:
Program model shall be created from scratch
This model will be created in LTI consumer Xblock.
Concern: In reference to PR https://github.com/edx/edx-platform/pull/29331 , 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: https://github.com/edx/edx-platform/pull/29530/files
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:
cs_comment_service only appreciates topics and their IDs. These topics have category and subcategory as their property.
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 “/”).
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
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.
The backend code does use "/" for separation. You can see the code [here}(https://github.com/edx/edx-platform/blob/dee42c6498b9cff64816e229c234495c518c1362/lms/djangoapps/discussion/django_comment_client/utils.py#L298 ) however this is only used for the legacy UI. Even the mobile UI doesn't seem to use this code.
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:
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”.
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:
Course hierarchy | Discussions hierarchy |
---|---|
Matrices
Introduction
Multiplication
Addition
Determinant
Cramer’s rule
Echelon form
Derivatives
Introduction
Continuous functions
Rate of change
Order of derivatives
First order
Second order | General
Matrices
Introduction
Multiplication
Addition
Determinant
Cramer’s rule
Echelon form
Derivatives
Introduction
Continuous functions
Rate of change
Order of derivatives
First order
Second order |
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.
Learner’s view | Admin view |
---|---|
General
Matrices
Introduction
Addition
Determinant
Cramer’s rule
Echelon form
Derivatives
Introduction
Continuous functions
Rate of change
Order of derivatives
First order
Second order |
If the course team deletes a subsection named Determinant
, the corresponding discussion topics will appear as follows:
Learner’s view | Admin view |
---|---|
If the course team deletes a section named Matrices
, the corresponding discussion topics will appear as follows:
Learner’s view | Admin view |
---|---|
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:
Move unit(s) to another (existing or new) subsection
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:
What do we want viewers to see when the use the new MFE?
How will we achieve what we want to see?
What side-effects could there be (so we can communicate them to partners)?
Resolution:
Consider the following discussion Xblock configuration. We would want “/” to simply be treated as a character in category name string.
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:
Discussion URL needs to be modified. This needs to be done before enabling new MFE on stage to avoid confusion over broken links afterwards.
Banner needs to be updated to provide option to toggle between legacy UI and the new MFE.
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.
We should implement a confirmation dialog box for deleting a post (to avoid accidental deletion)
What message do we want to show in that dialog box?
What should the UI look like for dialog box?
Resolution:
Agreed
Pending …
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?
Do we want to have 2 separate waffle flags; one for new MFE (v1.6) and one for in-context discussions (v1.7)?
Yes
Do we agree on the following matrix?
Waffle flags | v1.7 - Disabled | v1.7 - Enabled |
---|---|---|
v1.6 - Disabled | No change | Remove possibility for this to happen. How? |
v1.6 - Enabled |
|
|
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.
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:
Request from existing courses using v1.9 and v1.10
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: https://www.figma.com/file/LVFnO9oRHYiamoicZuXDAC/Discussion-page?node-id=4066%3A368050
Discuss: v1.6, v1.7, v1.9, v1.10, v1.11, v1.12 Rollout sequence
Step | Feature | Audience | Additional details |
---|---|---|---|
1 | New MFE (v1.6) | Educators |
|
2 | Learners area (v1.9, v1.10) as part of new MFE | Educators |
|
3 | Content edit/close reasons (v1.11, v1.12) as part of new MFE | Educators |
|
4 | New MFE with learners area and reasons (v1.6, v1.9, 1.10, v1.11, v1.12) | All |
|
5 | In-context discussions (v1.7) for new courses | All |
|
6 | In-context discussions (v1.7) for existing courses | Educators |
|
AA to answer following questions:
How many courses in past 6 months have been created?
How many of those can benefit from v1.7?
How many have 0 discussion xblocks? Real easy conversion.
How many have one discussion referenced in several xblocks.
???
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 PRhttps://github.com/openedx/cs_comments_service/pull/356 deployed on production on 18th Jan. Two potential solutions are:
Merge the corresponding platform PR
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 https://github.com/openedx/cs_comments_service/pull/365
Question: What is the fate of PR for user stats: https://github.com/openedx/cs_comments_service/pull/361 ?
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: https://github.com/edx/edx-platform/pull/29287.
Question: Can Infinity take up the work for integrating the feedback tool for discussion?
Resolution: Yes. Helpful resources are:
PR for feedback button: https://github.com/openedx/edx-platform/pull/29785
Potential plugin: https://feeder.sh/
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
Milestone | Tasks | Tentative finish |
---|---|---|
1.6 | Improvements and bug fixes | In-parallel as they come |
1.7 | Sidebar PR needs rebasing and some changes. | 3rd week of feb |
1.7 | Add new discussions provider to course authoring mfe. | 3rd week of feb |
1.9 & 1.10 | Platform PR of user stats needs rebasing, merge and deployment on prod. | 1st week of feb |
1.9 & 1.10 | Testing of stats PR. | 2nd week of feb |
1.9 & 1.10 | UI work is in progress. | 3rd week of feb |
1.11 & 1.12 | UI is ready. Need reason codes to be integrated. | 1st week of feb |
1.11 & 1.12 | Putting features behind the flag. | 3rd week of feb |
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:
Current provider is NOT legacy provider
Flag for new MFE is enabled
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:
At present: Only legacy topic structure is available. DONE
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.
We don’t want all course teams to see 2 edX discussion providers (legacy and new topic structure).
We want to have legacy topic structure as the default topic structure Done
We want to have some mechanism to enable the new topic structure on NEW courses offered by interested partners. need grooming
Future: The new topic structure will be default for NEW courses. Legacy topic structure will be supported for running courses. need grooming
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:
We keep the 2 edX providers on frontend named:
edX
(legacy topic structure)edX-new
(new topic structure)
Only the global staff will be able to see
edX-new
provider.In our partner portal post, we’ll ask partners to contact their respective edX support teams to get the new topic structure configured.
Once a partner support team receives the request, they’ll ask infinity to configure the new topic structure.
Infinity team will configure the
edX-new
provider for the course(s).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.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 https://openedx.atlassian.net/browse/TNL-8348 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 https://openedx.atlassian.net/browse/TNL-9778
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: https://2u-internal.atlassian.net/browse/INF-220
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: https://2u-internal.atlassian.net/browse/INF-210
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:
Use legacy API (which may need modification to make it compatible with REST framework)
Modify the newly created cs_comment_service API to fetch threads
14 June 2022
Discuss: https://2u-internal.atlassian.net/browse/INF-220
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