Skip to content

Bug fix -- subcategories in forums topic list should now be sorted alphabetically - #720

Merged
caesar2164 merged 4 commits into
masterfrom
akshay/order-commentables
Aug 23, 2013
Merged

Bug fix -- subcategories in forums topic list should now be sorted alphabetically#720
caesar2164 merged 4 commits into
masterfrom
akshay/order-commentables

Conversation

@akjags

@akjags akjags commented Aug 17, 2013

Copy link
Copy Markdown
Contributor

@kevinchugh @caesar2164

We figured out that the reason subcategories weren't getting sorted was that they had a sort_key of None, whereas the entries had a sort_key of their title, so with this simple fix, the already existing sorting mechanism should take care of the subcategory sorting as well.

@ghost ghost assigned kevinchugh Aug 17, 2013
@sefk

sefk commented Aug 17, 2013

Copy link
Copy Markdown
Contributor

Nice fix @caesar2164 and @akjags -- much better to fix the code than add in a bunch of remedial junk.

@chrisndodge

Copy link
Copy Markdown
Contributor

Well, that was simple.

+1

@kevinchugh

Copy link
Copy Markdown
Contributor

The code looks good, my concern remains about the unintentional reordering of topics in production. sort_key is an ordinal, what this code does is says "if there is no value, replace this value with the title." But prior to this replacement, the order is the order that they were pulled from the DB, probably created_at. So, let's suppose the titles are C,A,B, and the course author placed them in this order, but never added a sort key. Deploying this to production will make the course topics appear as A,B,C. So while this fixes one bad thing, it also introduces a potential new bad thing, namely, the surprise reordering of topics in production.

Would you mind coming up with a rake task to go along with this deployment that checks to see if there are no sort_keys set on all of the topics, and then walk through them and assign 1,2,3,... in the order they appear? This way, when we deploy this, it won't surprise the course authors and students.

@caesar2164

Copy link
Copy Markdown
Contributor

Confirmed that this code works on edx-west staging server to fix sorting issue we have

@akjags akjags closed this Aug 22, 2013
@akjags akjags reopened this Aug 22, 2013
@akjags

akjags commented Aug 22, 2013

Copy link
Copy Markdown
Contributor Author

Oops that was an accident. But good news, Giulio @caesar2164 !!

@caesar2164

Copy link
Copy Markdown
Contributor

Ok, @kevinchugh & @chrisndodge, added course level advanced setting/feature flag which defaults to false to sort course categories & subcategories by alpha. I also wrote a pair of tests to show that if the flag is set to false it does not sort, and if it's set to true it does sort and also uses the titles as sort_keys.

I think this should be ready to go now, let me know if I need to do anything else before merging.

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.

Hm, what happens if course.discussion_sort_alpha is unset?

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.

it isn't because it defaults to false in course advanced settings

@caesar2164

Copy link
Copy Markdown
Contributor

@jzoldak - could you take a look at my unit tests?

@kevinchugh

Copy link
Copy Markdown
Contributor

thanks for the extra settings and unit test, please ensure courses are defaulted to old sort upon deployment :-) 👍

@jzoldak

jzoldak commented Aug 23, 2013

Copy link
Copy Markdown
Contributor

👍 on the tests!

caesar2164 added a commit that referenced this pull request Aug 23, 2013
Bug fix -- subcategories in forums topic list should now be sorted alphabetically
@caesar2164
caesar2164 merged commit a902201 into master Aug 23, 2013
@jzoldak
jzoldak deleted the akshay/order-commentables branch August 27, 2013 15:22
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
Use display names for module edit links in the cms
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Apr 19, 2016
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Apr 19, 2016
e-kolpakov referenced this pull request in open-craft/openedx-platform Jun 22, 2016
ziafazal/YONK-338: added pagination support and user grades
caesar2164 pushed a commit to caesar2164/edx-platform that referenced this pull request Dec 19, 2017
…-no-submission

Handle case where ora2 score override had no previous score
jfavellar90 added a commit to eduNEXT/edx-platform that referenced this pull request Apr 20, 2018
Bumping version of rocket chat xblock to 0.2.3
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
* fix: [AA-1044] add missing h2 for screenreaders

- Add placeholder h2 tag with message indicating reserve for future use
- internationalize placeholder text
DanielVZ96 referenced this pull request in open-craft/openedx-platform Jan 31, 2024
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.

7 participants