feat: add export course tags menu #830

rpenido · 2024-02-09T21:11:09Z

Description

This PR adds an item in the Tools menu to allow a call of the function that exports the course tags to a CSV file.

Supporting Information

Closes:
- [Tagging] Download Course Tag Spreadsheet modular-learning#172
Depends on:
- feat: export tagged course as csv edx-platform#34091

Testing instructions

Use https://github.com/open-craft/taxonomy-sample-data/ to create some taxonomies.
Go to the course page in Studio and add some tags to the units in the course using the Manage Tags option:
Go to the course authoring MFE (i.e http://apps.local.edly.io:2001/course-authoring/course/course-v1:edX+DemoX+Demo_Course using tutor)
Click on the Export Tags menu from an existing taxonomy card
Check if a CSV file is downloaded with the Course structure and the tags applied to its blocks.

Private-ref:

FAL-3610

openedx-webhooks · 2024-02-09T21:11:14Z

Thanks for the pull request, @rpenido! 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:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

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.

codecov · 2024-02-09T21:20:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1555e9f) 89.13% compared to head (d0b1bec) 89.28%.
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #830      +/-   ##
==========================================
+ Coverage   89.13%   89.28%   +0.14%     
==========================================
  Files         548      551       +3     
  Lines        9647     9740      +93     
  Branches     2067     2101      +34     
==========================================
+ Hits         8599     8696      +97     
+ Misses       1001      996       -5     
- Partials       47       48       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pomegranited · 2024-02-13T00:31:42Z

@rpenido From this discussion on the issue, I thought we were going to add nested menus here? Please continue the conversation there to ensure that everyone is OK with the solution we're providing.

pomegranited · 2024-02-13T00:51:10Z

src/header/utils.js

+  {
+    href: `${studioBaseUrl}/api/content_tagging/v1/object_tags/${courseId}/export/`,
+    title: intl.formatMessage(messages['header.links.exportTags']),
+  },


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.

bradenmacdonald · 2024-02-13T21:52:49Z

@rpenido From this discussion on the issue, I thought we were going to add nested menus here? Please continue the conversation there to ensure that everyone is OK with the solution we're providing.

@pomegranited See https://chat.opencraft.com/opencraft/pl/955fm78zbfyddnzjo6akdyzjhh - I told him it's probably not worth the effort to implement submenus for now given the lack of framework support.

pomegranited

@rpenido Apologies, I didn't see that discussion about this menu.

This change is working great, but as mentioned, it needs to be gated by the feature flag and tests added. But the rest is great! I'll approve it when that change is made.

rpenido · 2024-02-15T14:03:24Z

@rpenido Apologies, I didn't see that discussion about this menu.

No need to apologize! I should have brought the discussion result here.

This change is working great, but as mentioned, it needs to be gated by the feature flag and tests added. But the rest is great! I'll approve it when that change is made.

Fixed 873b902

pomegranited

Thanks @rpenido ! This works great.
👍 with one request for your tests. I also have one question, but it doesn't have to block this moving to upstream review.

I tested this on my devstack while running feat: export tagged course as csv edx-platform#34091.
- I checked that with ENABLE_TAGGING_TAXONOMY_PAGES enabled, the "Export Course" and "Export Tags" menu items display, and work as expected.
- I checked that with ENABLE_TAGGING_TAXONOMY_PAGES disabled, only the "Export" (course) is displayed, and still works as expected.
I read through the code
I checked for accessibility issues by using my keyboard to navigate.
~~Includes documentation~~ N/A
User-facing strings are extracted for translation

pomegranited · 2024-02-16T02:42:08Z

src/header/utils.js

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


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

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.

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.

Done: 20727dc

pomegranited · 2024-02-16T02:42:24Z

src/header/utils.test.js

+        ENABLE_TAGGING_TAXONOMY_PAGES: 'true',
+      });
+      const actualItems = getToolsMenuItems(props);
+      expect(actualItems).toHaveLength(4);


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.

Sure! Done: effca98

pomegranited · 2024-02-16T02:43:01Z

src/header/utils.test.js

+        ENABLE_TAGGING_TAXONOMY_PAGES: 'false',
+      });
+      const actualItems = getToolsMenuItems(props);
+      expect(actualItems).toHaveLength(3);


xitij2000

Looks good! Just one issue with the export link.

xitij2000 · 2024-02-20T10:06:26Z

src/header/utils.js

-    href: `${studioBaseUrl}/export/${courseId}`,
-    title: intl.formatMessage(messages['header.links.export']),
-  }, {
+    href: `${studioBaseUrl}/import/${courseId}`,


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

Thank you @xitij2000 !
Fixed here: d0b1bec

openedx-webhooks · 2024-02-21T07:20:43Z

@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

feat: add export course tags menu

db898a8

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 9, 2024

rpenido mentioned this pull request Feb 9, 2024

feat: export tagged course as csv openedx/edx-platform#34091

Merged

rpenido requested a review from pomegranited February 9, 2024 22:09

pomegranited suggested changes Feb 13, 2024

View reviewed changes

rpenido added 2 commits February 15, 2024 10:11

fix: check taggng flag and add tests

873b902

style: fix eslint

c4c7daa

rpenido force-pushed the rpenido/fal-3610-add-export-course-tags-menu branch from d92636e to c4c7daa Compare February 15, 2024 13:24

pomegranited approved these changes Feb 16, 2024

View reviewed changes

rpenido added 2 commits February 16, 2024 10:34

test: check menu titles instead of count

effca98

fix: using only one string for Export Course

20727dc

xitij2000 approved these changes Feb 20, 2024

View reviewed changes

fix: wrong link

d0b1bec

xitij2000 merged commit 0fc0ce0 into openedx:master Feb 21, 2024
6 checks passed

xitij2000 deleted the rpenido/fal-3610-add-export-course-tags-menu branch February 21, 2024 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add export course tags menu #830

feat: add export course tags menu #830

rpenido commented Feb 9, 2024

openedx-webhooks commented Feb 9, 2024

codecov bot commented Feb 9, 2024 •

edited

Loading

pomegranited commented Feb 13, 2024

pomegranited Feb 13, 2024

bradenmacdonald commented Feb 13, 2024 •

edited

Loading

pomegranited left a comment

rpenido commented Feb 15, 2024

pomegranited left a comment •

edited

Loading

pomegranited Feb 16, 2024

rpenido Feb 16, 2024

bradenmacdonald Feb 16, 2024

rpenido Feb 16, 2024

pomegranited Feb 16, 2024

rpenido Feb 16, 2024

pomegranited Feb 16, 2024

xitij2000 left a comment

xitij2000 Feb 20, 2024

rpenido Feb 20, 2024

openedx-webhooks commented Feb 21, 2024

feat: add export course tags menu #830

feat: add export course tags menu #830

Conversation

rpenido commented Feb 9, 2024

Description

Supporting Information

Testing instructions

openedx-webhooks commented Feb 9, 2024

codecov bot commented Feb 9, 2024 • edited Loading

Codecov Report

pomegranited commented Feb 13, 2024

Choose a reason for hiding this comment

bradenmacdonald commented Feb 13, 2024 • edited Loading

pomegranited left a comment

Choose a reason for hiding this comment

rpenido commented Feb 15, 2024

pomegranited left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openedx-webhooks commented Feb 21, 2024

codecov bot commented Feb 9, 2024 •

edited

Loading

bradenmacdonald commented Feb 13, 2024 •

edited

Loading

pomegranited left a comment •

edited

Loading