Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 42 commits into from
Closed

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

wants to merge 42 commits into from

Conversation

Faraz32123
Copy link
Contributor

@Faraz32123 Faraz32123 commented Sep 16, 2024 •
edited
Loading

This PR is still a work in progress and not yet ready for final review.

  • Added new CourseWaffle flag named ENABLE_FORUM_V2. If it's not set for the course, V1(cs_comment_service) will be called.
  • Now we are using our forum v2 python native APIs based on CourseWaffle, ENABLE_FORUM_V2.
  • Modified old/current unit tests of edx-platform that uses forum APIs as there's now implementation of WaffleFla. So, had to modify the unit tests.
  • Added forum to the edx-platform requirements.

@Faraz32123 Faraz32123 requested a review from a team as a code owner September 16, 2024 11:31
feat: call python methods from forum v2
9126017 
- directly call python native APIs from forum v2 for pin, unpin thread,
commentables count_stats and get user's data by user_id
- add forum to the edx-platform requirements
@Faraz32123 Faraz32123 force-pushed the feat/migrate_APIs_from_http_to_python_call branch from 813c770 to 9126017 Compare September 16, 2024 11:45
This was referenced Sep 16, 2024
Closed
Merged
feat: migrate some APIs to native python
5ff3c66 
- directly call python native APIs from forum v2 for get parent comment,
create parent comment and create child comment.
- rename retrieve_commentables_stats method to get_commentables_stats and
retrieve_user to get_user.
@Faraz32123 Faraz32123 marked this pull request as draft September 18, 2024 08:53
@Faraz32123 Faraz32123 changed the title feat: call python methods from forum v2 [WIP] feat: call python methods from forum v2 Sep 18, 2024
feat: pass params to python native APIs
896f9a1 
- refactored code and now pass proper parameters to python native APIs
instead of a single dict
@regisb regisb mentioned this pull request Sep 19, 2024
Closed
@ormsbee
Copy link
Contributor

ormsbee commented Sep 20, 2024

As you folks are building this out, please use a CourseWaffleFlag to switch between running the old code that hits the service from edx-platform and the new code that uses the forum app. When a migration this big rolls out on a site that runs off of master (like 2U or one of MIT's sites), it will usually go:

  1. Turn it on for a few select courses to try it out, see if there are any major regressions.
  2. Turn it on by default for everyone.
  3. Turn it back off for a few select courses where weird edge case bugs arise.

We can turn this flag on by default for Sumac.

Ali-Salman29 and others added 4 commits September 24, 2024 10:58
@Ali-Salman29
fix: user tests
bf87423
feat: fix tests for get_user API
b382053 
- get_user API tests are now passing in test_views.py and test_serializers.py
- add get_user api patch in all tests
- fix httppretty request count in some tests
- fix test_patch_read_non_owner_user test
feat: use new coursewaffle flag to run old code
284e7da 
- add `ENABLE_FORUM_V2` course waffle flag to switch between old code i.e.
cs_comment_service and new code i.e. forum v2.
- mock course waffle flag is_enabled method i.e. ENABLE_FORUM_V2.is_enabled(),
so that old unit tests can be run and passed.
- refactor code(that parts of code whose native APIs are implemented till now)
where we call the native APIs
@Faraz32123
Merge branch 'master' into feat/migrate_APIs_from_http_to_python_call
cb58346
@Faraz32123 Faraz32123 force-pushed the feat/migrate_APIs_from_http_to_python_call branch from eb04713 to 60703b4 Compare September 27, 2024 09:56
fix: resolve inconsistant python dependencies
740b127 
- run `make compile-requirements forum`. Error: It appears that the
Python dependencies in this PR are inconsistent: A re-run of
`make compile-requirements` produced changes.
- fix quality checks
@Faraz32123 Faraz32123 force-pushed the feat/migrate_APIs_from_http_to_python_call branch from 60703b4 to 740b127 Compare September 27, 2024 10:39
fix: fix new failing tests
8cdc9fc 
- fix new tests related to discussion that failed after fixing previous tests
these are failing due to no.of argument difference
https://github.com/openedx/edx-platform/actions/runs/11069160532/job/30756121710?pr=35490
@Faraz32123 Faraz32123 force-pushed the feat/migrate_APIs_from_http_to_python_call branch 3 times, most recently from 738fa84 to 9440a91 Compare October 2, 2024 07:26
@Faraz32123 Faraz32123 force-pushed the feat/migrate_APIs_from_http_to_python_call branch from 9440a91 to 1447495 Compare October 2, 2024 08:34
@regisb regisb mentioned this pull request Oct 2, 2024
Open
7 tasks
@Faraz32123 Faraz32123 force-pushed the feat/migrate_APIs_from_http_to_python_call branch 2 times, most recently from 596a6fd to 78e6536 Compare October 2, 2024 10:58
@Faraz32123 Faraz32123 force-pushed the feat/migrate_APIs_from_http_to_python_call branch from fb0c2c9 to 5ef8196 Compare October 10, 2024 07:30
Muhammad Faraz Maqsood and others added 2 commits October 10, 2024 14:44
@Ali-Salman29 Ali-Salman29 force-pushed the feat/migrate_APIs_from_http_to_python_call branch from 90c31e7 to c2111ef Compare October 11, 2024 09:34
@Faraz32123 Faraz32123 force-pushed the feat/migrate_APIs_from_http_to_python_call branch 5 times, most recently from 43bf218 to af8e057 Compare October 11, 2024 14:13
@Faraz32123 Faraz32123 force-pushed the feat/migrate_APIs_from_http_to_python_call branch from 0ff43a0 to 1df14f2 Compare October 13, 2024 01:29
@Faraz32123 Faraz32123 force-pushed the feat/migrate_APIs_from_http_to_python_call branch from 1df14f2 to 4bcf51b Compare October 13, 2024 01:51
@Faraz32123 Faraz32123 force-pushed the feat/migrate_APIs_from_http_to_python_call branch from 07a50f0 to 40ff7f1 Compare October 14, 2024 11:07
@regisb
Copy link
Contributor

regisb commented Oct 16, 2024

The goal of this PR is to integrate the new forum application into edx-platform. In practice, this means the following changes:

  • Introduce a course waffle flag "forum_v2.enable_forum_v2".
  • Courses for which this waffle flag is enabled will make use of the new forum app. In practice, it's possible to have both the legacy forum v1 and the new v2 app coexist with one another, on a course per course basis.
  • The forum v2 app will default to storing data in mongodb. But to use mysql as a data storage (the default in tutor), you should use the "forum_v2.enable_mysql_backend" course waffle flag.
  • To migrate data on a course-per-course basis from mongodb to mysql, you should use the "forum_migrate_course_from_mongodb_to_mysql" management command.

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.

@regisb regisb closed this Oct 18, 2024
@regisb regisb deleted the feat/migrate_APIs_from_http_to_python_call branch October 18, 2024 08:10
@regisb
Copy link
Contributor

regisb commented Oct 18, 2024

My branch rename as caused this PR to close... Sorry about that. Can you please open another PR @Faraz32123? Likewise for edly-io#585

@Faraz32123
Copy link
Contributor Author

My branch rename as caused this PR to close... Sorry about that. Can you please open another PR @Faraz32123? Likewise for edly-io#585

Yeah, i noticed that. I'll sure create a new PR.

@regisb
Copy link
Contributor

regisb commented Oct 22, 2024

New PR: #35671

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taimoor-ahmed-1 taimoor-ahmed-1 taimoor-ahmed-1 left review comments

@regisb regisb regisb left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Faraz32123 @ormsbee @regisb @taimoor-ahmed-1 @Ali-Salman29