Skip to content

fix: updated topic sorting for discussions MFE - #31124

Merged
AhtishamShahid merged 3 commits into
masterfrom
ahtisham/fix-topics-sorting
Oct 13, 2022
Merged

fix: updated topic sorting for discussions MFE#31124
AhtishamShahid merged 3 commits into
masterfrom
ahtisham/fix-topics-sorting

Conversation

@AhtishamShahid

Copy link
Copy Markdown
Contributor

Description

Topics that have alphanumeric categories were being sorted in weird ways i.e

data = ['week 2', 'week 1', 'week 10', 'week 9']
data = sorted (data)
## returns ['week 1', 'week 10', 'week 2', 'week 9']

Created new key mapping that would sort it as expected

Like this :

## returns ['week 1', 'week 2', 'week 9', 'week 10']

@AhtishamShahid
AhtishamShahid force-pushed the ahtisham/fix-topics-sorting branch from 6fb6236 to c919006 Compare October 7, 2022 07:02

return sorted(category_list, key=alphanum_key)

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

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.

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.

@saadyousafarbi saadyousafarbi left a comment

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.

We might need more test cases for this, we can do that after rolling out and basing the test cases on feedback received!

@AhtishamShahid
AhtishamShahid merged commit c2696c0 into master Oct 13, 2022
@AhtishamShahid
AhtishamShahid deleted the ahtisham/fix-topics-sorting branch October 13, 2022 10:09
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

arbrandes pushed a commit that referenced this pull request Dec 8, 2022
* fix: updated topic sorting for discussions MFE

* feat: updated topic sorting test case
awais786 pushed a commit that referenced this pull request Jan 13, 2023
* fix: updated topic sorting for discussions MFE

* feat: updated topic sorting test case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants