Skip to content
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

[FC-0036] feat: Sort taxonomies #949

Merged
merged 3 commits into from
Apr 18, 2024
Merged

[FC-0036] feat: Sort taxonomies #949

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2904ca
feat: Sort taxonomies
ChrisChV Apr 11, 2024
56bc069
test: Tests for sorted taxonomies
ChrisChV Apr 11, 2024
9ded66e
refactor: Sort Taxonomies
ChrisChV Apr 17, 2024
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
36 changes: 35 additions & 1 deletion src/content-tags-drawer/ContentTagsDrawer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,40 @@ const ContentTagsDrawer = ({ id, onClose }) => {
}, []);

const taxonomies = useMemo(() => {
const sortTaxonomies = (taxonomiesList) => {
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];
};
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


if (taxonomyListData && contentTaxonomyTagsData) {
// Initialize list of content tags in taxonomies to populate
const taxonomiesList = taxonomyListData.results.map((taxonomy) => ({
Expand All @@ -126,7 +160,7 @@ const ContentTagsDrawer = ({ id, onClose }) => {
}
});

return taxonomiesList;
return sortTaxonomies(taxonomiesList);
}
return [];
}, [taxonomyListData, contentTaxonomyTagsData]);
Expand Down
95 changes: 89 additions & 6 deletions src/content-tags-drawer/ContentTagsDrawer.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,78 @@ describe('<ContentTagsDrawer />', () => {
},
],
},
{
name: 'Taxonomy 2',
taxonomyId: 124,
canTagObject: true,
tags: [
{
value: 'Tag 1',
lineage: ['Tag 1'],
canDeleteObjecttag: true,
},
],
},
{
name: 'Taxonomy 3',
taxonomyId: 125,
canTagObject: true,
tags: [
{
value: 'Tag 1.1.1',
lineage: ['Tag 1', 'Tag 1.1', 'Tag 1.1.1'],
canDeleteObjecttag: true,
},
],
},
{
name: '(B) Taxonomy 4',
taxonomyId: 126,
canTagObject: true,
tags: [],
},
{
name: '(A) Taxonomy 5',
taxonomyId: 127,
canTagObject: true,
tags: [],
},
],
},
});
getTaxonomyListData.mockResolvedValue({
results: [{
id: 123,
name: 'Taxonomy 1',
description: 'This is a description 1',
canTagObject: true,
}],
results: [
{
id: 123,
name: 'Taxonomy 1',
description: 'This is a description 1',
canTagObject: true,
},
{
id: 124,
name: 'Taxonomy 2',
description: 'This is a description 2',
canTagObject: true,
},
{
id: 125,
name: 'Taxonomy 3',
description: 'This is a description 3',
canTagObject: true,
},
{
id: 126,
name: '(B) Taxonomy 4',
description: 'This is a description 4',
canTagObject: true,
},
{
id: 127,
name: '(A) Taxonomy 5',
description: 'This is a description 5',
canTagObject: true,
},
],
});

useTaxonomyTagsData.mockReturnValue({
Expand Down Expand Up @@ -413,4 +475,25 @@ describe('<ContentTagsDrawer />', () => {

postMessageSpy.mockRestore();
});

it('should taxonomies must be ordered', async () => {
setupMockDataForStagedTagsTesting();
render(<RootWrapper />);
expect(await screen.findByText('Taxonomy 1')).toBeInTheDocument();

// First, taxonomies with content sorted by count implicit
// Later, empty taxonomies sorted by name
const expectedOrder = [
'Taxonomy 3', // 3 tags
'Taxonomy 1', // 2 tags
'Taxonomy 2', // 1 tag
'(A) Taxonomy 5',
'(B) Taxonomy 4',
];

const taxonomies = screen.getAllByText(/.*Taxonomy.*/);
for (let i = 0; i !== taxonomies.length; i++) {
expect(taxonomies[i].textContent).toBe(expectedOrder[i]);
}
});
});
Loading