[FC-0036] feat: Sort taxonomies #949
Conversation
|
Thanks for the pull request, @ChrisChV! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
openedx-webhooks
added
the
open-source-contribution
There was a problem hiding this comment.
👍 @ChrisChV Looks good to me!
- I tested this: I confirmed that the implicit tags are now also including when sorting
- I read through the code
-
I checked for accessibility issues -
Includes documentation
| const taxonomiesWithData = taxonomiesList.filter( | ||
| (t) => t.contentTags.length !== 0, | ||
| ); | ||
|
|
||
| // Count implicit tags per taxonomy | ||
| const tagsCountBytaxonomy = {}; | ||
| taxonomiesWithData.forEach((tax) => { | ||
| const countedTags = []; | ||
| let tagsCounter = 0; | ||
| tax.contentTags.forEach((tag) => { | ||
| tag.lineage.forEach((value) => { | ||
| if (!countedTags.includes(value)) { | ||
| countedTags.push(value); | ||
| tagsCounter += 1; | ||
| } | ||
| }); | ||
| }); | ||
| tagsCountBytaxonomy[tax.id] = tagsCounter; | ||
| }); | ||
|
|
||
| // Sort taxonomies with data by implicit count | ||
| const sortedTaxonomiesWithData = taxonomiesWithData.sort( | ||
| (a, b) => tagsCountBytaxonomy[b.id] - tagsCountBytaxonomy[a.id], | ||
| ); | ||
|
|
||
| // Sort empty taxonomies by name | ||
| const emptyTaxonomies = taxonomiesList.filter( | ||
| (t) => t.contentTags.length === 0, | ||
| ).sort((a, b) => a.name.localeCompare(b.name)); | ||
|
|
||
| return [...sortedTaxonomiesWithData, ...emptyTaxonomies]; | ||
| }; |
There was a problem hiding this comment.
I think this code can be simplified quite a bit. If I understand correctly, you want to sort first by the tag count and then by the name? The complication being that all implicit tags need to be counted as well.
I see that this value is being displayed in the UI, so can't that value be cached and reused? Why is it being recalculated for the sorting here?
I think javascript sorting is stable, so if the API response is already sorted by name you can just sort by tag count here, and it should sort the whole list by tags while retaining the order of the other elements.
So the whole tag count code can be replaced with roughly:
taxonomiesList.map(item => (new Set(item.contentTags.flatMap(item=>item.lineage))).size)
This will create a flat list of all lineage items, convert that to a set, so that duplicates are removed an then get the size. You then have a list of tag counts and can sort by this list.
There was a problem hiding this comment.
Thanks for the feedback! I updated the code here 9ded66e
I see that this value is being displayed in the UI, so can't that value be cached and reused? Why is it being recalculated for the sorting here?
Yes, you are right, that calculation is done individually here: https://github.com/openedx/frontend-app-course-authoring/blob/08140226c3d3601378f36215f366826557b9023e/src/content-tags-drawer/ContentTagsCollapsibleHelper.jsx#L140-L176
For reasons of complexity and budget I have left a comment about this, to do it in the future. The good thing is that with or without the counting the function will be executed so it does not affect performance.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #949 +/- ##
==========================================
+ Coverage 92.00% 92.10% +0.09%
==========================================
Files 612 634 +22
Lines 10746 11091 +345
Branches 2305 2387 +82
==========================================
+ Hits 9887 10215 +328
- Misses 830 847 +17
Partials 29 29 ☔ View full report in Codecov by Sentry. |
|
@ChrisChV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions and their rationales should be documented in the repo (docstring / ADR), per
Useful information to include:
"Developer", and "Operator".
changes.
Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Other information
Include anything else that will help reviewers and consumers understand the change.