Skip to content

fix: updated topic sorting for discussions MFE #31124

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

Merged
AhtishamShahid merged 3 commits into from
Oct 13, 2022
Merged

fix: updated topic sorting for discussions MFE #31124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c919006
fix: updated topic sorting for disucssions MFE
AhtishamShahid Oct 7, 2022
c17a9e3
feat: updated topic sorting test case
AhtishamShahid Oct 13, 2022
00d50b3
feat: updated topic sorting test case
AhtishamShahid Oct 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 38 additions & 21 deletions lms/djangoapps/discussion/rest_api/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
from __future__ import annotations

import itertools
import re
from collections import defaultdict

from enum import Enum
from typing import Dict, Iterable, List, Literal, Optional, Set, Tuple
from urllib.parse import urlencode, urlunparse
Expand All @@ -22,12 +22,8 @@
from opaque_keys.edx.locator import CourseKey
from rest_framework import status
from rest_framework.exceptions import PermissionDenied
from rest_framework.response import Response
from rest_framework.request import Request

from xmodule.course_module import CourseBlock
from xmodule.modulestore.django import modulestore
from xmodule.tabs import CourseTabList
from rest_framework.response import Response

from lms.djangoapps.course_blocks.api import get_course_blocks
from lms.djangoapps.courseware.courses import get_course_with_access
Expand All @@ -41,48 +37,53 @@
from openedx.core.djangoapps.django_comment_common.comment_client.comment import Comment
from openedx.core.djangoapps.django_comment_common.comment_client.course import (
get_course_commentable_counts,
get_course_user_stats,
get_course_user_stats
)
from openedx.core.djangoapps.django_comment_common.comment_client.thread import Thread
from openedx.core.djangoapps.django_comment_common.comment_client.utils import CommentClientRequestError, \
CommentClient500Error
from openedx.core.djangoapps.django_comment_common.comment_client.utils import (
CommentClient500Error,
CommentClientRequestError
)
from openedx.core.djangoapps.django_comment_common.models import (
FORUM_ROLE_ADMINISTRATOR,
FORUM_ROLE_COMMUNITY_TA,
FORUM_ROLE_GROUP_MODERATOR,
FORUM_ROLE_MODERATOR,
CourseDiscussionSettings,
Role,
Role
)
from openedx.core.djangoapps.django_comment_common.signals import (
comment_created,
comment_deleted,
comment_edited,
comment_flagged,
comment_voted,
thread_created,
thread_deleted,
thread_edited,
thread_voted,
thread_flagged,
comment_flagged,
thread_voted
)
from openedx.core.djangoapps.user_api.accounts.api import get_account_settings
from openedx.core.lib.exceptions import CourseNotFoundError, DiscussionNotFoundError, PageNotFoundError
from ..config.waffle import ENABLE_LEARNERS_STATS
from xmodule.course_module import CourseBlock
from xmodule.modulestore.django import modulestore
from xmodule.tabs import CourseTabList

from ..config.waffle import ENABLE_LEARNERS_STATS
from ..django_comment_client.base.views import (
track_comment_created_event,
track_comment_deleted_event,
track_thread_created_event,
track_thread_deleted_event,
track_thread_viewed_event,
track_voted_event,
track_voted_event
)
from ..django_comment_client.utils import (
get_group_id_for_user,
get_user_role_names,
has_discussion_privileges,
is_commentable_divided,
is_commentable_divided
)
from ..toggles import ENABLE_DISCUSSION_MODERATION_REASON_CODES
from .exceptions import CommentNotFoundError, DiscussionBlackOutException, DiscussionDisabledError, ThreadNotFoundError
Expand All @@ -92,7 +93,7 @@
can_delete,
get_editable_fields,
get_initializable_comment_fields,
get_initializable_thread_fields,
get_initializable_thread_fields
)
from .serializers import (
CommentSerializer,
Expand All @@ -101,14 +102,14 @@
ThreadSerializer,
TopicOrdering,
UserStatsSerializer,
get_context,
get_context
)

from .utils import (
add_stats_for_users_with_no_discussion_content,
discussion_open_for_user,
get_usernames_for_course,
get_usernames_from_search_string,
add_stats_for_users_with_no_discussion_content,
set_attribute, get_usernames_for_course,
set_attribute
)

User = get_user_model()
Expand Down Expand Up @@ -383,7 +384,23 @@ def get_courseware_topics(
for xblock in discussion_xblocks:
xblocks_by_category[xblock.discussion_category].append(xblock)

for category in xblocks_by_category.keys():
def sort_categories(category_list):
"""
Sorts the given iterable containing alphanumeric correctly.
Required arguments:
category_list -- list of categories.
"""
def convert(text):
if text.isdigit():
return int(text)
return text

def alphanum_key(key):
return [convert(c) for c in re.split('([0-9]+)', key)]

return sorted(category_list, key=alphanum_key)

for category in sort_categories(xblocks_by_category.keys()):

@asadazam93 asadazam93 Oct 7, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will sort the topics in alphabetical order which is not required. The expectation is that the topics should have the same order as they have in the course which will not always be alphabetical.

@ayub02 ayub02 Oct 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alpha numeric seems to be a better approach. Let put this on prod and see feedback.

children = []
for xblock in xblocks_by_category[category]:
if not topic_ids or xblock.discussion_id in topic_ids:
Expand Down
24 changes: 12 additions & 12 deletions lms/djangoapps/discussion/rest_api/tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,35 +408,35 @@ def test_many(self):
"B": {"id": "non-courseware-2"},
}
self.store.update_item(self.course, self.user.id)
self.make_discussion_xblock("courseware-1", "A", "1")
self.make_discussion_xblock("courseware-2", "A", "2")
self.make_discussion_xblock("courseware-3", "B", "1")
self.make_discussion_xblock("courseware-4", "B", "2")
self.make_discussion_xblock("courseware-5", "C", "1")
self.make_discussion_xblock("courseware-1", "Week 1", "1")
self.make_discussion_xblock("courseware-2", "Week 1", "2")
self.make_discussion_xblock("courseware-3", "Week 10", "1")
self.make_discussion_xblock("courseware-4", "Week 10", "2")
self.make_discussion_xblock("courseware-5", "Week 9", "1")
actual = self.get_course_topics()
expected = {
"courseware_topics": [
self.make_expected_tree(
None,
"A",
"Week 1",
[
self.make_expected_tree("courseware-1", "1"),
self.make_expected_tree("courseware-2", "2"),
]
),
self.make_expected_tree(
None,
"B",
"Week 9",
[self.make_expected_tree("courseware-5", "1")]
),
self.make_expected_tree(
None,
"Week 10",
[
self.make_expected_tree("courseware-3", "1"),
self.make_expected_tree("courseware-4", "2"),
]
),
self.make_expected_tree(
None,
"C",
[self.make_expected_tree("courseware-5", "1")]
),
],
"non_courseware_topics": [
self.make_expected_tree("non-courseware-1", "A"),
Expand Down