feat!: remove cs_comments_service support for forums flag API #36706
Conversation
Ali-Salman29
force-pushed
the
feat/remove_forum_v1_flag_api
branch
from
6e51065 to
913c863
Compare
There was a problem hiding this comment.
I have a handful of questions, but my high-level concern is with the extent of the mocking we're doing on calls to the internal forum API. I want to make sure I'm not misunderstanding what's going on. I think that it absolutely makes sense to mock any and all network calls to the cs_comments_service in tests. But if we have tests against the in-process forum repo's API's, it seems like we should be just letting it call that code without mocking so that we can be confident that the forum APIs haven't accidentally been changed in backwards-incompatible ways over the coming years. We can check that it worked as expected by reading the data back from the forum API.
Is there something I'm missing?
| "user_id": "1", | ||
| "course_id": str(self.course.id), | ||
| } | ||
| self.check_mock_called_with("update_thread_flag", -1, **params) |
There was a problem hiding this comment.
Why mock the interface to the forum API here? I get that we want to test that edx-platform code is calling the forum API with the correct arguments, but shouldn't we also be testing to make sure that the forum API hasn't changed in some breaking way? I'm concerned that if someone changes the signature of update_thread_flag in some backwards-incompatible way, these tests will still continue to work because it will be mocked out.
Why not actually let it do the write all the way through to the forum API and its data model, and then query the forum API directly to make sure the state is what you expect?
There was a problem hiding this comment.
In the cs_comment_service, we were mocking requests. In our case, we are mocking functions within the Forum repository. We used this approach so that we could utilize all our previous tests. Currently, changing the signature of the functions won't cause any of the tests to fail. To address this, we have two approaches:
- Remove mocks and create a factory class for initializing dummy threads and comments.
- Add different tests for validating parameters in edx-platform and forum.
We are evaluating both approaches and will share estimates with you soon.
There was a problem hiding this comment.
Remove mocks and create a factory class for initializing dummy threads and comments.
We plan to eliminate mocks and introduce a factory class to initialize dummy threads and comments for more realistic and maintainable tests. The task breakdown is as follows:
- Set up abstract methods and factory classes for mocking MySQL: 3 days
- Refactor 7 test files: 3 days per file × 7 files = 21 days
- Final testing and validation: 2 days
However, since both Taimoor and I will work in parallel, the effective duration will be halved.
Add different tests for validating parameters in edx-platform and forum.
We will add new tests to validate various parameters in edx-platform and the forum, and it'll approximately takes 2 working days.
Note:
Moreover, we will begin this work after completing Phase II, which we started yesterday. Since this task is independent, it will not interfere with any existing functionality.
There was a problem hiding this comment.
Understood. Please don't start on that work before I clear it budget-wise. Thank you.
There was a problem hiding this comment.
After discussing this with Axim folks, we can add it in a follow-on SOW for a later phase. Thank you.
This will force the use of the new v2 forum's APIs for flaging/unflaging.
Ali-Salman29
force-pushed
the
feat/remove_forum_v1_flag_api
branch
from
12c2e33 to
f75899c
Compare
|
@ormsbee I've squashed commits. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
Description
This pull request removes legacy code related to the "flag" api functionality within the discussion forum v1 app. The rationale behind this change is to clean up deprecated or unused code paths, thereby simplifying the codebase and reducing potential maintenance overhead.
Implications for users:
• There should be no impact on end-users, as the removed code pertains to deprecated functionality that is no longer in active use.
• Developers working on the discussion forum module will benefit from a cleaner codebase, making future enhancements and maintenance more straightforward.