Edly: Integrate Forum V2 Application into edx-platform #35671

Faraz32123 · 2024-10-18T10:45:34Z

This PR introduces the integration of the new Forum V2 application into the edx-platform, allowing a course-level choice between the legacy forum (V1) and the new Forum V2.

Key Changes 🚀🚀:

Waffle Flag for Forum V2:
A new course waffle flag, discussions.enable_forum_v2, has been introduced. When enabled, it will activate the Forum V2 application for the selected course, allowing Forum V1 and V2 to coexist in the platform, used on a per-course basis.
Data Storage Options:
By default, Forum V2 stores data in MongoDB. However, if MySQL is preferred (as is the default in Tutor), an additional course waffle flag, forum_v2.enable_mysql_backend, can be used to switch the storage backend to MySQL.
Course-Level Data Migration:
A new management command, forum_migrate_course_from_mongodb_to_mysql, enables data migration on a per-course basis from MongoDB to MySQL. This provides a smooth transition for courses already using Forum V2 with MongoDB who wish to switch to MySQL.

asadazam93 · 2024-10-23T09:53:47Z

@Faraz32123 Are the unit tests complete now? Did you test the new forum v2 with the discussions MFE?

Faraz32123 · 2024-10-23T13:37:30Z

@Faraz32123 Are the unit tests complete now? Did you test the new forum v2 with the discussions MFE?

As for unit tests, current edx-platform tests that uses v1 are modified accordingly. Tests that uses v2 are under work and will come in a separate PR that will cover v2 native API calls in edx-platform.
Yup, we have tested the forum v2 with the discussion MFE.

xitij2000 · 2024-10-24T12:58:55Z

lms/djangoapps/discussion/django_comment_client/base/tests.py

@@ -82,6 +82,7 @@ def _set_mock_request_data(self, mock_request, data):


 @patch('openedx.core.djangoapps.django_comment_common.comment_client.utils.requests.request', autospec=True)
+@patch('lms.djangoapps.discussion.toggles.ENABLE_FORUM_V2.is_enabled', autospec=True)


Why not just use @override_waffle_flag(ENABLE_FORUM_V2, True)

Yup, that can be also used but the idea was to keep the extra patching/mocking method same.

With that method you wouldn't need to pass around the mock, and eventually when forums v2 is the only one you can just drop the relevant code easily.

we have created test files(those will come in next PR) that cover forum v2. So in case when we are only left with forum v2, we'll eventually remove this file completely.

Faraz32123 · 2024-10-25T11:16:41Z

can @regisb you or someone with access run the workflows again. Thanks.
Dependency conflicts with forum should be resolved now after we run CI again.

ormsbee · 2024-11-01T05:16:06Z

@Faraz32123, @regisb: What do you need at this point in order to get this PR merged?

regisb · 2024-11-01T09:16:49Z

@ormsbee All that's needed is a 👍 review. It's a fairly big PR, so if you want we can walk you through the changes in a live call. Who do you think would be most suited to review this? We can find someone internally with expertise on cs_comments_service.

ormsbee · 2024-11-11T16:31:42Z

@hurtstotouchfire: Wanted to make sure you folks were aware of this PR. This introduces the switching code between old and new forums. It should not affect the default site behavior, in that it will route to the cs_comments_service by default. But it will allow you folks to start experimenting with the new backend.

I know you've said that 2U doesn't need to be in the loop for PRs as long as the default behaviors are unchanged for non-tutor users. But I wanted to make sure you and your team had a chance to review if you wanted, and that you knew this was coming down the pipe in case you notice any regressions.

I'm doing a review as well, and we'll target a merge for later this week.

ormsbee

I still need to look through the test code properly, and probably take a second look at models.py, but I wanted to put these comments down sooner rather than later. Thanks folks.

ormsbee · 2024-11-11T05:17:06Z

lms/djangoapps/discussion/toggles.py

+    f"{WAFFLE_FLAG_NAMESPACE}.enable_discussions_mfe", __name__
+)
+
+FORUM_V2_WAFFLE_FLAG_NAMESPACE = "forum_v2"


The waffle flag namespace is meant to represent the app (or group of apps), not the specific feature. You don't need to define a new one here–please just re-use the imported WAFFLE_FLAG_NAMESPACE.

ormsbee · 2024-11-11T05:32:59Z

lms/djangoapps/discussion/toggles.py

+# .. toggle_use_cases: temporary, open_edx
+# .. toggle_creation_date: 2024-9-26
+# .. toggle_target_removal_date: 2025-12-05
+ENABLE_FORUM_V2 = CourseWaffleFlag(


What is the reason that this is defined here, instead of in https://github.com/openedx/edx-platform/blob/master/openedx/core/djangoapps/discussions/config/waffle.py like most of the others? If there is no strong reason, please move it there so that we aren't importing stuff from the lms package into openedx.core

Yeah, we can move it to openedx.core. There's no reason as such.

ormsbee · 2024-11-11T18:14:12Z

openedx/core/djangoapps/django_comment_common/comment_client/utils.py

+def get_course_key(course_id):
+    if course_id and isinstance(course_id, str):
+        course_id = CourseKey.from_string(course_id)
+    return course_id


It looks like this function is written to be a little permissive in its inputs, e.g. you can send it a CourseKey and get it back unmodified, None will get returned. But a blank string would raise an InvalidKeyError.

I think that's okay, but please add a docstring (and optionally a type annotation) explaining what this function is expecting to get and why. Without that, it's hard to understand what the edge case behavior is intended to be.

yup, we can add a docstring/type annotation to explain well.

openedx/core/djangoapps/django_comment_common/comment_client/models.py

ormsbee · 2024-11-13T06:10:56Z

openedx/core/djangoapps/django_comment_common/comment_client/thread.py

+                if not params.get("course_id"):
+                    params = query_params['course_id']
+                response = forum_api.get_user_threads(**params)


I'm confused about how this works. Is query_params['course_id'] a dict?

yup, query_params is a dict.
You can view it here and on line 155.

That link doesn't work for me.

I get that query_params is a dict. But here it looks like we're saying, "If the course_id in params is missing/blank/null, then replace all of params with query_params['course_id']".

I'm not clear what lines 82 and 83 are doing here. If line 83 executes and query_params['course_id'] is a string, then the call to get_user_threads(**params) will fail. If line 83 was meant to be params['course_id'] = query_params['course_id'], then it seems redundant given how params is initially defined.

This was the link: https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/discussion/views.py.

Yup, you are right, the code is redundant here as params is already getting course_id above in code from query_params and params is a dict(good catch).

ormsbee · 2024-11-13T06:12:50Z

openedx/core/djangoapps/django_comment_common/comment_client/thread.py

+        course_key = utils.get_course_key(course_id)
+        if is_forum_v2_enabled(course_key):
+            if user_id := request_params.get('user_id'):
+                request_params['user_id'] = str(user_id)


[question (not a request for change)]: user_id seems to get passed around various things as strings, while group_id gets passed around as ints. What's the reasoning for that?

yeah, that's right. Basically V1(cs_comment_service) was saving user_id as strings and group_id as int in mongodb. So, we wanted to replicate the same behaviour as that of V1 in forum_V2 for mongo and then same in mysql.

ormsbee · 2024-11-13T06:16:52Z

openedx/core/djangoapps/django_comment_common/comment_client/user.py

+            try:
+                response = forum_api.get_user(self.attributes["id"], retrieve_params)
+            except ForumV2RequestError as e:
+                self.save({"course_key": course_key})


[question] Should error logging happen here?

It doesn't need an error log here I think.
The use case for this catch here is when a new user opens the discussion forum(UI) first time. It tries to get the user from mongo/mysql but as it doesn't exists, it just creates it there and then get it again in the catch(except) block to continue.
Hope it answers the question.

Course waffle flag name was updated in the latest edx-platform PR: openedx/edx-platform#35671

ormsbee

Finally got through the test code. Thank you for your patience. The only thing there that I'd ask is that we use the override_waffle_flag helper where it would be appropriate, instead of the manual patching of 'openedx.core.djangoapps.discussions.config.waffle.ENABLE_FORUM_V2.is_enabled'. If that would cause problems that I'm not seeing, please let me know.

I want to talk with someone at 2U before we merge this, but I'm hopeful that we can bring this in on Monday or Tuesday.

I know it's been a long road. Thank you.

Faraz32123 · 2024-11-18T06:22:40Z

Finally got through the test code. Thank you for your patience. The only thing there that I'd ask is that we use the override_waffle_flag helper where it would be appropriate, instead of the manual patching of 'openedx.core.djangoapps.discussions.config.waffle.ENABLE_FORUM_V2.is_enabled'. If that would cause problems that I'm not seeing, please let me know.

@ormsbee thanks for the review.

Basically while using override_waffle_flag, we were facing error: AssertionError: expected a course key or other learning context key which occurs in edx-platform/openedx/core/djangoapps/waffle_utils/__init__.py at line 121. This override_waffle_flag wasn't mocking the is_enabled() method of the CourseWaffleFlag. So we had just manually added patches for it.
override_waffle_flag would be helpful in case if we were just checking, ENABLE_FORUM_V2 is enabled or not. But as now we are checking ENABLE_FORUM_V2 for each course while calling APIs from V1 or V2, We had to patch is_enabled method.

ormsbee · 2024-11-18T15:06:12Z

Ah, I see, thank you.

ormsbee · 2024-11-26T18:16:38Z

Apologies for the delay in merging this. 2U had requested time to get their stage environment ready for testing, and they're also rolling out a database config change to prod this coming Monday (this is the long Thanksgiving weekend in the U.S.). The plan will be to merge this on Monday (December 2nd).

ormsbee · 2024-12-02T15:01:39Z

2U is all clear now, so this can be merged as soon as conflicts are resolved. Thank you for your patience.

ormsbee · 2024-12-03T13:09:27Z

@Faraz32123, @regisb: Just want to flag that there's a test failure.

This commit introduces the new Forum V2 application, allowing users to choose between the legacy Forum V1 and the new Forum V2 at the course level. Key Changes: - Added waffle flag `discussions.enable_forum_v2` to enable Forum V2 for selected courses, allowing coexistence with Forum V1. - Default data storage for Forum V2 is set to MongoDB, with an option to switch to MySQL using the waffle flag `forum_v2.enable_mysql_backend`. - Introduced management command `forum_migrate_course_from_mongodb_to_mysql` for per-course data migration from MongoDB to MySQL. Note: This PR does not include all unit tests for the Forum V2 native API due to ongoing migration efforts. Further updates will follow to ensure full test coverage before final release. Co-authored-by: [Muhammad Faraz Maqsood] <faraz.maqsood@arbisoft.com> Co-authored-by: [Ali Salman] <ali.salman@arbisoft.com>

regisb · 2024-12-03T16:31:52Z

🥳

edx-pipeline-bot · 2024-12-03T17:25:16Z

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

edx-pipeline-bot · 2024-12-06T15:34:02Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-12-06T15:34:02Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-12-06T15:46:25Z

2U Release Notice: This PR has been rolled back from the edX production environment.

edx-pipeline-bot · 2024-12-06T15:46:25Z

2U Release Notice: This PR has been rolled back from the edX production environment.

edx-pipeline-bot · 2024-12-06T17:59:33Z

2U Release Notice: This PR has been deployed to the edX production environment.

edx-pipeline-bot · 2024-12-06T17:59:33Z

2U Release Notice: This PR has been deployed to the edX production environment.

Faraz32123 requested a review from a team as a code owner October 18, 2024 10:45

Faraz32123 requested a review from regisb October 18, 2024 10:46

Faraz32123 force-pushed the edly/forumv2 branch from 24a00ff to 59a0227 Compare October 18, 2024 11:31

Faraz32123 requested review from feanil and a team as code owners October 18, 2024 11:31

Faraz32123 force-pushed the edly/forumv2 branch from 59a0227 to dba51e4 Compare October 18, 2024 11:39

regisb mentioned this pull request Oct 22, 2024

[WIP] feat: call python methods from forum v2 #35490

Closed

Faraz32123 mentioned this pull request Oct 22, 2024

Migrate from HTTP to native Python API in edx-platform openedx/forum#49

Closed

xitij2000 reviewed Oct 24, 2024

View reviewed changes

regisb force-pushed the edly/forumv2 branch 5 times, most recently from 400dc29 to 4a75633 Compare November 12, 2024 20:22

ormsbee reviewed Nov 13, 2024

View reviewed changes

Faraz32123 force-pushed the edly/forumv2 branch 4 times, most recently from 9337c7c to 5e2da74 Compare November 13, 2024 12:19

regisb added a commit to openedx/forum that referenced this pull request Nov 13, 2024

docs: fix forum v2 waffle name

de778c2

Course waffle flag name was updated in the latest edx-platform PR: openedx/edx-platform#35671

regisb mentioned this pull request Nov 13, 2024

docs: fix forum v2 waffle name openedx/forum#130

Merged

Faraz32123 force-pushed the edly/forumv2 branch from 5e2da74 to 46ac5da Compare November 14, 2024 06:19

ormsbee requested changes Nov 16, 2024

View reviewed changes

ormsbee approved these changes Nov 18, 2024

View reviewed changes

awais786 approved these changes Nov 19, 2024

View reviewed changes

Faraz32123 force-pushed the edly/forumv2 branch 3 times, most recently from 9f0f7a5 to 9f9b02a Compare November 19, 2024 16:01

regisb mentioned this pull request Nov 27, 2024

feat: Integrate Forum V2 into edx-platform #35933

Merged

Faraz32123 force-pushed the edly/forumv2 branch from 9f9b02a to 18a36a3 Compare December 3, 2024 08:04

regisb force-pushed the edly/forumv2 branch from 18a36a3 to 2440b53 Compare December 3, 2024 11:47

regisb force-pushed the edly/forumv2 branch 3 times, most recently from 2ec7925 to 1f83f24 Compare December 3, 2024 14:14

regisb force-pushed the edly/forumv2 branch from 1f83f24 to 6a28970 Compare December 3, 2024 15:13

ormsbee merged commit 70b60ff into openedx:master Dec 3, 2024
51 checks passed

DawoudSheraz mentioned this pull request Dec 9, 2024

chore: remove platform changes patch overhangio/tutor-forum#56

Merged

regisb mentioned this pull request Dec 10, 2024

feat: reapply forum v2 changes #36002

Merged

		@@ -82,6 +82,7 @@ def _set_mock_request_data(self, mock_request, data):


		@patch('openedx.core.djangoapps.django_comment_common.comment_client.utils.requests.request', autospec=True)
		@patch('lms.djangoapps.discussion.toggles.ENABLE_FORUM_V2.is_enabled', autospec=True)

Edly: Integrate Forum V2 Application into edx-platform #35671

Edly: Integrate Forum V2 Application into edx-platform #35671

Conversation

Faraz32123 commented Oct 18, 2024 • edited by ormsbee Loading

asadazam93 commented Oct 23, 2024

Faraz32123 commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Faraz32123 commented Oct 25, 2024 • edited Loading

ormsbee commented Nov 1, 2024

regisb commented Nov 1, 2024

ormsbee commented Nov 11, 2024

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee left a comment

Choose a reason for hiding this comment

Faraz32123 commented Nov 18, 2024

ormsbee commented Nov 18, 2024

ormsbee commented Nov 26, 2024

ormsbee commented Dec 2, 2024

ormsbee commented Dec 3, 2024

regisb commented Dec 3, 2024

edx-pipeline-bot commented Dec 3, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

edx-pipeline-bot commented Dec 6, 2024

Faraz32123 commented Oct 18, 2024 •

edited by ormsbee

Loading

Faraz32123 commented Oct 25, 2024 •

edited

Loading

ormsbee Nov 13, 2024 •

edited

Loading