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.

🛠 Note that this PR does not include all unit tests for the forum v2 native API. This is because migrating unit tests is taking much longer than expected. We will take out this PR from draft as soon as it is considered production-ready, despite the fact that some code in edx-platform might not be 100% covered.

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

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>

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.

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

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

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

docs: fix forum v2 waffle name

8c91859

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

ormsbee requested changes Nov 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Faraz32123 commented Oct 18, 2024 •

edited

Loading

asadazam93 commented Oct 23, 2024

Faraz32123 commented Oct 23, 2024

xitij2000 Oct 24, 2024

Faraz32123 Oct 25, 2024

xitij2000 Oct 25, 2024

Faraz32123 Oct 25, 2024

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

ormsbee Nov 11, 2024

ormsbee Nov 11, 2024

Faraz32123 Nov 13, 2024

ormsbee Nov 11, 2024

Faraz32123 Nov 13, 2024

ormsbee Nov 13, 2024

Faraz32123 Nov 13, 2024

ormsbee Nov 13, 2024 •

edited

Loading

Faraz32123 Nov 14, 2024

ormsbee Nov 13, 2024

Faraz32123 Nov 13, 2024

ormsbee Nov 13, 2024

Faraz32123 Nov 13, 2024

ormsbee left a comment

Faraz32123 commented Nov 18, 2024

		@@ -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

Are you sure you want to change the base?

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

Conversation

Faraz32123 commented Oct 18, 2024 • edited 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

Faraz32123 commented Oct 18, 2024 •

edited

Loading

Faraz32123 commented Oct 25, 2024 •

edited

Loading

ormsbee Nov 13, 2024 •

edited

Loading