Skip to content

Commit c913a55

Browse files
authored
feat: updated api to get all question type reponses (#34215)
* feat: updated api to get all question type reponses * test: fixed and added new test cases
1 parent c13c7d3 commit c913a55

File tree

8 files changed

+119
-30
lines changed

8 files changed

+119
-30
lines changed

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

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,7 @@ def count_queries(func): # pylint: disable=no-self-argument
399399
Decorates test methods to count mongo and SQL calls for a
400400
particular modulestore.
401401
"""
402+
402403
def inner(self, default_store, block_count, mongo_calls, sql_queries, *args, **kwargs):
403404
with modulestore().default_store(default_store):
404405
self.set_up_course(block_count=block_count)
@@ -794,7 +795,8 @@ def flag_thread(self, mock_request, is_closed):
794795
('get', f'{CS_PREFIX}/threads/518d4237b023791dca00000d'),
795796
{
796797
'data': None,
797-
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False},
798+
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False,
799+
'merge_question_type_responses': False},
798800
'headers': ANY,
799801
'timeout': 5
800802
}
@@ -812,7 +814,8 @@ def flag_thread(self, mock_request, is_closed):
812814
('get', f'{CS_PREFIX}/threads/518d4237b023791dca00000d'),
813815
{
814816
'data': None,
815-
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False},
817+
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False,
818+
'merge_question_type_responses': False},
816819
'headers': ANY,
817820
'timeout': 5
818821
}
@@ -871,7 +874,8 @@ def un_flag_thread(self, mock_request, is_closed):
871874
('get', f'{CS_PREFIX}/threads/518d4237b023791dca00000d'),
872875
{
873876
'data': None,
874-
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False},
877+
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False,
878+
'merge_question_type_responses': False},
875879
'headers': ANY,
876880
'timeout': 5
877881
}
@@ -889,7 +893,8 @@ def un_flag_thread(self, mock_request, is_closed):
889893
('get', f'{CS_PREFIX}/threads/518d4237b023791dca00000d'),
890894
{
891895
'data': None,
892-
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False},
896+
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False,
897+
'merge_question_type_responses': False},
893898
'headers': ANY,
894899
'timeout': 5
895900
}

lms/djangoapps/discussion/rest_api/api.py

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1168,7 +1168,8 @@ def get_learner_active_thread_list(request, course_key, query_params):
11681168
})
11691169

11701170

1171-
def get_comment_list(request, thread_id, endorsed, page, page_size, flagged=False, requested_fields=None):
1171+
def get_comment_list(request, thread_id, endorsed, page, page_size, flagged=False, requested_fields=None,
1172+
merge_question_type_responses=False):
11721173
"""
11731174
Return the list of comments in the given thread.
11741175
@@ -1211,13 +1212,13 @@ def get_comment_list(request, thread_id, endorsed, page, page_size, flagged=Fals
12111212
"response_skip": response_skip,
12121213
"response_limit": page_size,
12131214
"reverse_order": reverse_order,
1215+
"merge_question_type_responses": merge_question_type_responses
12141216
}
12151217
)
1216-
12171218
# Responses to discussion threads cannot be separated by endorsed, but
12181219
# responses to question threads must be separated by endorsed due to the
12191220
# existing comments service interface
1220-
if cc_thread["thread_type"] == "question":
1221+
if cc_thread["thread_type"] == "question" and not merge_question_type_responses:
12211222
if endorsed is None: # lint-amnesty, pylint: disable=no-else-raise
12221223
raise ValidationError({"endorsed": ["This field is required for question threads."]})
12231224
elif endorsed:
@@ -1229,10 +1230,11 @@ def get_comment_list(request, thread_id, endorsed, page, page_size, flagged=Fals
12291230
responses = cc_thread["non_endorsed_responses"]
12301231
resp_total = cc_thread["non_endorsed_resp_total"]
12311232
else:
1232-
if endorsed is not None:
1233-
raise ValidationError(
1234-
{"endorsed": ["This field may not be specified for discussion threads."]}
1235-
)
1233+
if not merge_question_type_responses:
1234+
if endorsed is not None:
1235+
raise ValidationError(
1236+
{"endorsed": ["This field may not be specified for discussion threads."]}
1237+
)
12361238
responses = cc_thread["children"]
12371239
resp_total = cc_thread["resp_total"]
12381240

lms/djangoapps/discussion/rest_api/forms.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ class CommentListGetForm(_PaginationForm):
130130
flagged = BooleanField(required=False)
131131
endorsed = ExtendedNullBooleanField(required=False)
132132
requested_fields = MultiValueField(required=False)
133+
merge_question_type_responses = BooleanField(required=False)
133134

134135

135136
class UserCommentListGetForm(_PaginationForm):

lms/djangoapps/discussion/rest_api/tests/test_api.py

Lines changed: 42 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1260,13 +1260,15 @@ def make_minimal_cs_thread(self, overrides=None):
12601260
overrides.setdefault("course_id", str(self.course.id))
12611261
return make_minimal_cs_thread(overrides)
12621262

1263-
def get_comment_list(self, thread, endorsed=None, page=1, page_size=1):
1263+
def get_comment_list(self, thread, endorsed=None, page=1, page_size=1,
1264+
merge_question_type_responses=False):
12641265
"""
12651266
Register the appropriate comments service response, then call
12661267
get_comment_list and return the result.
12671268
"""
12681269
self.register_get_thread_response(thread)
1269-
return get_comment_list(self.request, thread["id"], endorsed, page, page_size)
1270+
return get_comment_list(self.request, thread["id"], endorsed, page, page_size,
1271+
merge_question_type_responses=merge_question_type_responses)
12701272

12711273
def test_nonexistent_thread(self):
12721274
thread_id = "nonexistent_thread"
@@ -1398,10 +1400,14 @@ def test_basic_query_params(self):
13981400
"resp_limit": ["14"],
13991401
"with_responses": ["True"],
14001402
"reverse_order": ["False"],
1403+
"merge_question_type_responses": ["False"],
14011404
}
14021405
)
14031406

1404-
def test_discussion_content(self):
1407+
def get_source_and_expected_comments(self):
1408+
"""
1409+
Returns the source comments and expected comments for testing purposes.
1410+
"""
14051411
source_comments = [
14061412
{
14071413
"type": "comment",
@@ -1414,7 +1420,7 @@ def test_discussion_content(self):
14141420
"created_at": "2015-05-11T00:00:00Z",
14151421
"updated_at": "2015-05-11T11:11:11Z",
14161422
"body": "Test body",
1417-
"endorsed": False,
1423+
"endorsed": True,
14181424
"abuse_flaggers": [],
14191425
"votes": {"up_count": 4},
14201426
"child_count": 0,
@@ -1449,7 +1455,7 @@ def test_discussion_content(self):
14491455
"updated_at": "2015-05-11T11:11:11Z",
14501456
"raw_body": "Test body",
14511457
"rendered_body": "<p>Test body</p>",
1452-
"endorsed": False,
1458+
"endorsed": True,
14531459
"endorsed_by": None,
14541460
"endorsed_by_label": None,
14551461
"endorsed_at": None,
@@ -1508,12 +1514,26 @@ def test_discussion_content(self):
15081514
},
15091515
},
15101516
]
1517+
return source_comments, expected_comments
1518+
1519+
def test_discussion_content(self):
1520+
source_comments, expected_comments = self.get_source_and_expected_comments()
15111521
actual_comments = self.get_comment_list(
15121522
self.make_minimal_cs_thread({"children": source_comments})
15131523
).data["results"]
15141524
assert actual_comments == expected_comments
15151525

1516-
def test_question_content(self):
1526+
def test_question_content_with_merge_question_type_responses(self):
1527+
source_comments, expected_comments = self.get_source_and_expected_comments()
1528+
actual_comments = self.get_comment_list(
1529+
self.make_minimal_cs_thread({
1530+
"thread_type": "question",
1531+
"children": source_comments,
1532+
"resp_total": len(source_comments)
1533+
}), merge_question_type_responses=True).data["results"]
1534+
assert actual_comments == expected_comments
1535+
1536+
def test_question_content_(self):
15171537
thread = self.make_minimal_cs_thread({
15181538
"thread_type": "question",
15191539
"endorsed_responses": [make_minimal_cs_comment({"id": "endorsed_comment", "username": self.user.username})],
@@ -1547,11 +1567,13 @@ def test_endorsed_by_anonymity(self):
15471567
assert actual_comments[0]['endorsed_by'] is None
15481568

15491569
@ddt.data(
1550-
("discussion", None, "children", "resp_total"),
1551-
("question", False, "non_endorsed_responses", "non_endorsed_resp_total"),
1570+
("discussion", None, "children", "resp_total", False),
1571+
("question", False, "non_endorsed_responses", "non_endorsed_resp_total", False),
1572+
("question", None, "children", "resp_total", True),
15521573
)
15531574
@ddt.unpack
1554-
def test_cs_pagination(self, thread_type, endorsed_arg, response_field, response_total_field):
1575+
def test_cs_pagination(self, thread_type, endorsed_arg, response_field,
1576+
response_total_field, merge_question_type_responses):
15551577
"""
15561578
Test cases in which pagination is done by the comments service.
15571579
@@ -1571,22 +1593,26 @@ def test_cs_pagination(self, thread_type, endorsed_arg, response_field, response
15711593
})
15721594

15731595
# Only page
1574-
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=1, page_size=5).data
1596+
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=1, page_size=5,
1597+
merge_question_type_responses=merge_question_type_responses).data
15751598
assert actual['pagination']['next'] is None
15761599
assert actual['pagination']['previous'] is None
15771600

15781601
# First page of many
1579-
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=1, page_size=2).data
1602+
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=1, page_size=2,
1603+
merge_question_type_responses=merge_question_type_responses).data
15801604
assert actual['pagination']['next'] == 'http://testserver/test_path?page=2'
15811605
assert actual['pagination']['previous'] is None
15821606

15831607
# Middle page of many
1584-
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=2, page_size=2).data
1608+
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=2, page_size=2,
1609+
merge_question_type_responses=merge_question_type_responses).data
15851610
assert actual['pagination']['next'] == 'http://testserver/test_path?page=3'
15861611
assert actual['pagination']['previous'] == 'http://testserver/test_path?page=1'
15871612

15881613
# Last page of many
1589-
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=3, page_size=2).data
1614+
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=3, page_size=2,
1615+
merge_question_type_responses=merge_question_type_responses).data
15901616
assert actual['pagination']['next'] is None
15911617
assert actual['pagination']['previous'] == 'http://testserver/test_path?page=2'
15921618

@@ -1597,7 +1623,8 @@ def test_cs_pagination(self, thread_type, endorsed_arg, response_field, response
15971623
response_total_field: 5
15981624
})
15991625
with pytest.raises(PageNotFoundError):
1600-
self.get_comment_list(thread, endorsed=endorsed_arg, page=2, page_size=5)
1626+
self.get_comment_list(thread, endorsed=endorsed_arg, page=2, page_size=5,
1627+
merge_question_type_responses=merge_question_type_responses)
16011628

16021629
def test_question_endorsed_pagination(self):
16031630
thread = self.make_minimal_cs_thread({
@@ -4078,6 +4105,7 @@ class CourseTopicsV2Test(ModuleStoreTestCase):
40784105
"""
40794106
Tests for discussions topic API v2 code.
40804107
"""
4108+
40814109
def setUp(self) -> None:
40824110
super().setUp()
40834111
self.course = CourseFactory.create(

lms/djangoapps/discussion/rest_api/tests/test_forms.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,8 @@ def test_basic(self):
207207
'page': 2,
208208
'page_size': 13,
209209
'flagged': False,
210-
'requested_fields': set()
210+
'requested_fields': set(),
211+
'merge_question_type_responses': False
211212
}
212213

213214
def test_missing_thread_id(self):

0 commit comments

Comments
 (0)