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

feat: add export course tags menu #830

Merged
merged 6 commits into from
Feb 21, 2024
+47 −7
Merged

feat: add export course tags menu #830

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
db898a8
feat: add export course tags menu
rpenido Feb 9, 2024
873b902
fix: check taggng flag and add tests
rpenido Feb 15, 2024
c4c7daa
style: fix eslint
rpenido Feb 15, 2024
effca98
test: check menu titles instead of count
rpenido Feb 16, 2024
20727dc
fix: using only one string for Export Course
rpenido Feb 16, 2024
d0b1bec
fix: wrong link
rpenido Feb 20, 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
10 changes: 10 additions & 0 deletions src/header/messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,16 @@ const messages = defineMessages({
defaultMessage: 'Export',
description: 'Link to Studio Export page',
},
'header.links.exportCourse': {
id: 'header.links.exportCourse',
defaultMessage: 'Export Course',
description: 'Link to Studio Export page',
},
'header.links.exportTags': {
id: 'header.links.exportTags',
defaultMessage: 'Export Tags',
description: 'Download course content tags as CSV',
},
'header.links.checklists': {
id: 'header.links.checklists',
defaultMessage: 'Checklists',
Expand Down
17 changes: 14 additions & 3 deletions src/header/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,20 @@ export const getToolsMenuItems = ({ studioBaseUrl, courseId, intl }) => ([
title: intl.formatMessage(messages['header.links.import']),
},
{
href: `${studioBaseUrl}/export/${courseId}`,
title: intl.formatMessage(messages['header.links.export']),
}, {
href: `${studioBaseUrl}/import/${courseId}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

This path seems to point to import, however the title seems to be for export.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @xitij2000 !
Fixed here: d0b1bec

title: intl.formatMessage(
getConfig().ENABLE_TAGGING_TAXONOMY_PAGES === 'true'
? messages['header.links.exportCourse']
: messages['header.links.export'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. I'm not sure we need to do this? But I'm not sure who to ask..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this to avoid changing the menu for everyone without ENABLE_TAGGING_TAXONOMY_PAGES. But I'm also not sure if we need this. Let's see if we have some comments in the Upstream/CC review.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine to change the base menu text ("Export" -> "Export Course"), and I think that's better than having this more complex logic. The "Export Tags" should be gated by the flag though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: 20727dc

),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Inclusion of this menu item needs to be gated by the same flags that we gate the other taxonomy menu items, which also means it needs tests added like these.

...(getConfig().ENABLE_TAGGING_TAXONOMY_PAGES === 'true'
? [{
href: `${studioBaseUrl}/api/content_tagging/v1/object_tags/${courseId}/export/`,
title: intl.formatMessage(messages['header.links.exportTags']),
}] : []
),
{
href: `${studioBaseUrl}/checklists/${courseId}`,
title: intl.formatMessage(messages['header.links.checklists']),
},
Expand Down
20 changes: 19 additions & 1 deletion src/header/utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getConfig, setConfig } from '@edx/frontend-platform';
import { getContentMenuItems } from './utils';
import { getContentMenuItems, getToolsMenuItems } from './utils';

const props = {
studioBaseUrl: 'UrLSTuiO',
Expand Down Expand Up @@ -28,4 +28,22 @@ describe('header utils', () => {
expect(actualItems).toHaveLength(4);
});
});
describe('getToolsMenuItems', () => {
it('should include export tags option', () => {
setConfig({
...getConfig(),
ENABLE_TAGGING_TAXONOMY_PAGES: 'true',
});
const actualItems = getToolsMenuItems(props);
expect(actualItems).toHaveLength(4);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please test that the items shown are the items we expect (instead of just checking the length)? It's ok to just check their titles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! Done: effca98

});
it('should not include export tags option', () => {
setConfig({
...getConfig(),
ENABLE_TAGGING_TAXONOMY_PAGES: 'false',
});
const actualItems = getToolsMenuItems(props);
expect(actualItems).toHaveLength(3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

});
});
});
Loading