Skip to content

Commit

Permalink
Merge branch 'master' into rpenido/fal-3611-download-library-tag-spre…
Browse files Browse the repository at this point in the history 
…adsheet
  • Loading branch information
@rpenido
rpenido committed Feb 29, 2024
2 parents 6189a9f + 547f7fe commit 07e8fa9
Show file tree
Hide file tree
Showing 19 changed files with 545 additions and 36 deletions.
2 changes: 1 addition & 1 deletion .github/CODEOWNERS
View file
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ openedx/core/djangoapps/credit @openedx/2U-
openedx/core/djangoapps/heartbeat/
openedx/core/djangoapps/oauth_dispatch
openedx/core/djangoapps/user_api/ @openedx/2U-aperture
openedx/core/djangoapps/user_authn/
openedx/core/djangoapps/user_authn/ @openedx/2U-vanguards
openedx/features/course_experience/
xmodule/

Expand Down
13 changes: 9 additions & 4 deletions lms/djangoapps/discussion/django_comment_client/base/tests.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -399,6 +399,7 @@ def count_queries(func): # pylint: disable=no-self-argument
Decorates test methods to count mongo and SQL calls for a
particular modulestore.
"""

def inner(self, default_store, block_count, mongo_calls, sql_queries, *args, **kwargs):
with modulestore().default_store(default_store):
self.set_up_course(block_count=block_count)
Expand Down Expand Up @@ -794,7 +795,8 @@ def flag_thread(self, mock_request, is_closed):
('get', f'{CS_PREFIX}/threads/518d4237b023791dca00000d'),
{
'data': None,
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False},
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False,
'merge_question_type_responses': False},
'headers': ANY,
'timeout': 5
}
Expand All @@ -812,7 +814,8 @@ def flag_thread(self, mock_request, is_closed):
('get', f'{CS_PREFIX}/threads/518d4237b023791dca00000d'),
{
'data': None,
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False},
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False,
'merge_question_type_responses': False},
'headers': ANY,
'timeout': 5
}
Expand Down Expand Up @@ -871,7 +874,8 @@ def un_flag_thread(self, mock_request, is_closed):
('get', f'{CS_PREFIX}/threads/518d4237b023791dca00000d'),
{
'data': None,
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False},
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False,
'merge_question_type_responses': False},
'headers': ANY,
'timeout': 5
}
Expand All @@ -889,7 +893,8 @@ def un_flag_thread(self, mock_request, is_closed):
('get', f'{CS_PREFIX}/threads/518d4237b023791dca00000d'),
{
'data': None,
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False},
'params': {'mark_as_read': True, 'request_id': ANY, 'with_responses': False, 'reverse_order': False,
'merge_question_type_responses': False},
'headers': ANY,
'timeout': 5
}
Expand Down
16 changes: 9 additions & 7 deletions lms/djangoapps/discussion/rest_api/api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1168,7 +1168,8 @@ def get_learner_active_thread_list(request, course_key, query_params):
})


def get_comment_list(request, thread_id, endorsed, page, page_size, flagged=False, requested_fields=None):
def get_comment_list(request, thread_id, endorsed, page, page_size, flagged=False, requested_fields=None,
merge_question_type_responses=False):
"""
Return the list of comments in the given thread.
Expand Down Expand Up @@ -1211,13 +1212,13 @@ def get_comment_list(request, thread_id, endorsed, page, page_size, flagged=Fals
"response_skip": response_skip,
"response_limit": page_size,
"reverse_order": reverse_order,
"merge_question_type_responses": merge_question_type_responses
}
)

# Responses to discussion threads cannot be separated by endorsed, but
# responses to question threads must be separated by endorsed due to the
# existing comments service interface
if cc_thread["thread_type"] == "question":
if cc_thread["thread_type"] == "question" and not merge_question_type_responses:
if endorsed is None: # lint-amnesty, pylint: disable=no-else-raise
raise ValidationError({"endorsed": ["This field is required for question threads."]})
elif endorsed:
Expand All @@ -1229,10 +1230,11 @@ def get_comment_list(request, thread_id, endorsed, page, page_size, flagged=Fals
responses = cc_thread["non_endorsed_responses"]
resp_total = cc_thread["non_endorsed_resp_total"]
else:
if endorsed is not None:
raise ValidationError(
{"endorsed": ["This field may not be specified for discussion threads."]}
)
if not merge_question_type_responses:
if endorsed is not None:
raise ValidationError(
{"endorsed": ["This field may not be specified for discussion threads."]}
)
responses = cc_thread["children"]
resp_total = cc_thread["resp_total"]

Expand Down
1 change: 1 addition & 0 deletions lms/djangoapps/discussion/rest_api/forms.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ class CommentListGetForm(_PaginationForm):
flagged = BooleanField(required=False)
endorsed = ExtendedNullBooleanField(required=False)
requested_fields = MultiValueField(required=False)
merge_question_type_responses = BooleanField(required=False)


class UserCommentListGetForm(_PaginationForm):
Expand Down
56 changes: 42 additions & 14 deletions lms/djangoapps/discussion/rest_api/tests/test_api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1260,13 +1260,15 @@ def make_minimal_cs_thread(self, overrides=None):
overrides.setdefault("course_id", str(self.course.id))
return make_minimal_cs_thread(overrides)

def get_comment_list(self, thread, endorsed=None, page=1, page_size=1):
def get_comment_list(self, thread, endorsed=None, page=1, page_size=1,
merge_question_type_responses=False):
"""
Register the appropriate comments service response, then call
get_comment_list and return the result.
"""
self.register_get_thread_response(thread)
return get_comment_list(self.request, thread["id"], endorsed, page, page_size)
return get_comment_list(self.request, thread["id"], endorsed, page, page_size,
merge_question_type_responses=merge_question_type_responses)

def test_nonexistent_thread(self):
thread_id = "nonexistent_thread"
Expand Down Expand Up @@ -1398,10 +1400,14 @@ def test_basic_query_params(self):
"resp_limit": ["14"],
"with_responses": ["True"],
"reverse_order": ["False"],
"merge_question_type_responses": ["False"],
}
)

def test_discussion_content(self):
def get_source_and_expected_comments(self):
"""
Returns the source comments and expected comments for testing purposes.
"""
source_comments = [
{
"type": "comment",
Expand All @@ -1414,7 +1420,7 @@ def test_discussion_content(self):
"created_at": "2015-05-11T00:00:00Z",
"updated_at": "2015-05-11T11:11:11Z",
"body": "Test body",
"endorsed": False,
"endorsed": True,
"abuse_flaggers": [],
"votes": {"up_count": 4},
"child_count": 0,
Expand Down Expand Up @@ -1449,7 +1455,7 @@ def test_discussion_content(self):
"updated_at": "2015-05-11T11:11:11Z",
"raw_body": "Test body",
"rendered_body": "<p>Test body</p>",
"endorsed": False,
"endorsed": True,
"endorsed_by": None,
"endorsed_by_label": None,
"endorsed_at": None,
Expand Down Expand Up @@ -1508,12 +1514,26 @@ def test_discussion_content(self):
},
},
]
return source_comments, expected_comments

def test_discussion_content(self):
source_comments, expected_comments = self.get_source_and_expected_comments()
actual_comments = self.get_comment_list(
self.make_minimal_cs_thread({"children": source_comments})
).data["results"]
assert actual_comments == expected_comments

def test_question_content(self):
def test_question_content_with_merge_question_type_responses(self):
source_comments, expected_comments = self.get_source_and_expected_comments()
actual_comments = self.get_comment_list(
self.make_minimal_cs_thread({
"thread_type": "question",
"children": source_comments,
"resp_total": len(source_comments)
}), merge_question_type_responses=True).data["results"]
assert actual_comments == expected_comments

def test_question_content_(self):
thread = self.make_minimal_cs_thread({
"thread_type": "question",
"endorsed_responses": [make_minimal_cs_comment({"id": "endorsed_comment", "username": self.user.username})],
Expand Down Expand Up @@ -1547,11 +1567,13 @@ def test_endorsed_by_anonymity(self):
assert actual_comments[0]['endorsed_by'] is None

@ddt.data(
("discussion", None, "children", "resp_total"),
("question", False, "non_endorsed_responses", "non_endorsed_resp_total"),
("discussion", None, "children", "resp_total", False),
("question", False, "non_endorsed_responses", "non_endorsed_resp_total", False),
("question", None, "children", "resp_total", True),
)
@ddt.unpack
def test_cs_pagination(self, thread_type, endorsed_arg, response_field, response_total_field):
def test_cs_pagination(self, thread_type, endorsed_arg, response_field,
response_total_field, merge_question_type_responses):
"""
Test cases in which pagination is done by the comments service.
Expand All @@ -1571,22 +1593,26 @@ def test_cs_pagination(self, thread_type, endorsed_arg, response_field, response
})

# Only page
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=1, page_size=5).data
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=1, page_size=5,
merge_question_type_responses=merge_question_type_responses).data
assert actual['pagination']['next'] is None
assert actual['pagination']['previous'] is None

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

# Middle page of many
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=2, page_size=2).data
actual = self.get_comment_list(thread, endorsed=endorsed_arg, page=2, page_size=2,
merge_question_type_responses=merge_question_type_responses).data
assert actual['pagination']['next'] == 'http://testserver/test_path?page=3'
assert actual['pagination']['previous'] == 'http://testserver/test_path?page=1'

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

Expand All @@ -1597,7 +1623,8 @@ def test_cs_pagination(self, thread_type, endorsed_arg, response_field, response
response_total_field: 5
})
with pytest.raises(PageNotFoundError):
self.get_comment_list(thread, endorsed=endorsed_arg, page=2, page_size=5)
self.get_comment_list(thread, endorsed=endorsed_arg, page=2, page_size=5,
merge_question_type_responses=merge_question_type_responses)

def test_question_endorsed_pagination(self):
thread = self.make_minimal_cs_thread({
Expand Down Expand Up @@ -4078,6 +4105,7 @@ class CourseTopicsV2Test(ModuleStoreTestCase):
"""
Tests for discussions topic API v2 code.
"""

def setUp(self) -> None:
super().setUp()
self.course = CourseFactory.create(
Expand Down
3 changes: 2 additions & 1 deletion lms/djangoapps/discussion/rest_api/tests/test_forms.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,8 @@ def test_basic(self):
'page': 2,
'page_size': 13,
'flagged': False,
'requested_fields': set()
'requested_fields': set(),
'merge_question_type_responses': False
}

def test_missing_thread_id(self):
Expand Down
Loading

0 comments on commit 07e8fa9

Please sign in to comment.