feat: add download template button to taxonomy list [FC-0036] #674

rpenido · 2023-11-08T20:44:32Z

Description

This PR adds a new button in the Taxonomy List to allow users to download a sample taxonomy template in the format used to import taxonomies.

Supporting Information

Part of: [Tagging] Download Taxonomy Template modular-learning#114

Testing instructions

Go to the taxonomy list page (http://localhost:2001/taxonomy-list)
Click on the Download template and then on the CSV template option
Check if the template.csv is correctly saved.
Click on the Download template and then on the JSON template option
Check if the template.json is correctly saved.

Private-ref: FAL-3532

openedx-webhooks · 2023-11-08T20:44:36Z

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 · 2023-11-08T20:51:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (94725df) 88.96% compared to head (7f6319e) 88.96%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #674   +/-   ##
=======================================
  Coverage   88.96%   88.96%           
=======================================
  Files         472      472           
  Lines        7376     7378    +2     
  Branches     1573     1573           
=======================================
+ Hits         6562     6564    +2     
  Misses        787      787           
  Partials       27       27

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

pomegranited · 2023-11-10T00:45:44Z

@rpenido I think you're missing some style changes from the original PR? I'm seeing this when I run this branch:

pomegranited

👍 Looks great @rpenido !

Could you also please update the PR description and un-draft this to get it ready for upstream/CC review?

I tested this on my devstack
I read through the code
I checked for accessibility issues by using only my keyboard to navigate
Includes documentation
User-facing strings are extracted for translation

rpenido · 2023-11-10T18:46:52Z

@rpenido I think you're missing some style changes from the original PR? I'm seeing this when I run this branch:

Fixed!

pomegranited · 2023-11-13T00:55:42Z

@bradenmacdonald This PR is ready for upstream review -- who should we ask?

bradenmacdonald · 2023-11-13T20:18:40Z

This PR is ready for upstream review -- who should we ask?

@jristau1984 has offered to coordinate reviews for us for now. @jristau1984 could you please line up a review for this small PR? Or if you folks don't have much capacity for the near future, let me know and I can find a reviewer from Axim or an OpenCraft Core Contributor (as we did on e.g. #645).

openedx-webhooks · 2023-11-14T13:56:59Z

I've created issue TNL-11202 in the private 2U Jira.

pomegranited · 2023-11-16T01:51:58Z

@rpenido could you add "[FC-0036]" to the PR title here? Thanks :)

rpenido · 2023-12-12T23:36:47Z

Hi, @jristau1984! 😃

The same here: do we have an estimated due date for this review?

Thank you! 😃

jristau1984 · 2023-12-13T13:13:00Z

Due to heavy focus on internal goal delivery before the end of the year, I cannot promise any reviews before January. Tagging @cablaa77 and @jimjohnson8 in case they are able to prioritize this earlier.

xitij2000

The code looks good. Just some minor suggestions.

xitij2000 · 2023-12-15T02:57:17Z

src/taxonomy/TaxonomyListPage.test.jsx

+    await act(async () => {
+      fireEvent.click(importMenu);
+    });


These don't need to be wrapped in act since the library itself wraps them "act" internally.

Done 7f6319e!

xitij2000 · 2023-12-15T03:26:45Z

src/taxonomy/TaxonomyListPage.test.jsx

+  it('downloads the taxonomy template json', async () => {
+    useIsTaxonomyListDataLoaded.mockReturnValue(true);
+    useTaxonomyListDataResponse.mockReturnValue({
+      results: [{
+        id: 1,
+        name: 'Taxonomy',
+        description: 'This is a description',
+      }],
+    });
+    const { getByTestId } = render(<RootWrapper />);
+    const importMenu = getByTestId('taxonomy-download-template');
+    expect(importMenu).toBeInTheDocument();
+    await act(async () => {
+      fireEvent.click(importMenu);
+    });
+
+    const importButton = getByTestId('taxonomy-download-template-json');
+    expect(importButton).toBeInTheDocument();
+    await act(async () => {
+      fireEvent.click(importButton);
+    });
+    expect(getTaxonomyTemplateFile).toHaveBeenCalled();
+  });


This and the above test seem to be exactly the same with very minor differences, so we can use parametrisation here as well.

Suggested change

it('downloads the taxonomy template json', async () => {

useIsTaxonomyListDataLoaded.mockReturnValue(true);

useTaxonomyListDataResponse.mockReturnValue({

results: [{

id: 1,

name: 'Taxonomy',

description: 'This is a description',

}],

});

const { getByTestId } = render(<RootWrapper />);

const importMenu = getByTestId('taxonomy-download-template');

expect(importMenu).toBeInTheDocument();

await act(async () => {

fireEvent.click(importMenu);

});

const importButton = getByTestId('taxonomy-download-template-json');

expect(importButton).toBeInTheDocument();

await act(async () => {

fireEvent.click(importButton);

});

expect(getTaxonomyTemplateFile).toHaveBeenCalled();

});

it.each(['CSV', 'JSON'])('downloads the taxonomy template %s', async (fileFormat) => {

useIsTaxonomyListDataLoaded.mockReturnValue(true);

useTaxonomyListDataResponse.mockReturnValue({

results: [{

id: 1,

name: 'Taxonomy',

description: 'This is a description',

}],

});

const { findByRole } = render(<RootWrapper />);

const importMenu = await findByRole('button', {name: /download template/i});

fireEvent.click(importMenu);

const importButton = await findByRole('button', {name: `${fileFormat} template` });

fireEvent.click(importButton);

expect(getTaxonomyTemplateFile).toHaveBeenCalled();

});

Note that I've simplified the test somewhat.

Using findByRole etc is preferred to finding by test id.

No need for act around fireEvent

No need to assert the presence of a component in the document, if the component is missing for some reason then the findByRole will still cause the test to fail.

Thank you for the explanation @xitij2000!
Done 7f6319e!
But I didn't manage to find a way to check that the download really started.

src/taxonomy/data/api.js

xitij2000 · 2023-12-15T03:31:36Z

src/taxonomy/data/api.js

+
+/**
+ * Downloads the template file for import taxonomies
+ * @param {('json'|'csv')} format
+ * @returns {void}
+ */
+export function getTaxonomyTemplateFile(format) {
+  window.location.href = getTaxonomyTemplateApiUrl(format);
+}


Instead of doing this, I think you can just add this URL as a link direct using the download property.

i.e.

<a href={getTaxonomyTemplateApiUrl(format)} download=taxonomy-template.${format}>

Done! I skipped the download attribute: the download file name is set in the backend API.

xitij2000

Looks good! Just one small nit.

xitij2000 · 2023-12-18T12:52:12Z

src/taxonomy/TaxonomyListPage.jsx

+            <Dropdown.Item
+              href={getTaxonomyTemplateApiUrl('csv')}
+              data-testid="taxonomy-download-template-csv"
+            >
+              {intl.formatMessage(messages.downloadTemplateButtonCSVLabel)}
+            </Dropdown.Item>
+            <Dropdown.Item
+              href={getTaxonomyTemplateApiUrl('json')}
+              data-testid="taxonomy-download-template-json"
+            >
+              {intl.formatMessage(messages.downloadTemplateButtonJSONLabel)}
+            </Dropdown.Item>


Adding download=template.json/.csv or just download will also enforce that the browser will downlod the file instead of opening it in a tab.

Thank you for the review @xitij2000 !

We are doing it in the backend here: https://github.com/openedx/openedx-learning/blob/3e2d6a497efddfae7a1c24b361ddfd36704dbb80/openedx_tagging/core/tagging/rest_api/v1/views_import.py#L47 using Content-Disposition. Should we also do it here?

openedx-webhooks · 2023-12-19T04:01:51Z

@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.

This commit adds a new button in the Taxonomy List to allow users to download a sample taxonomy template in the format used to import taxonomies.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 8, 2023

rpenido force-pushed the rpenido/fal-3532-download-template branch from 7b6d7ad to 80d737b Compare November 9, 2023 12:22

rpenido mentioned this pull request Nov 9, 2023

[temp] feat: add download template button to taxonomy list open-craft/frontend-app-authoring#7

Closed

feat: add download taxonomy template button

5381f15

rpenido force-pushed the rpenido/fal-3532-download-template branch from 80d737b to 5381f15 Compare November 9, 2023 12:44

rpenido and others added 3 commits November 9, 2023 17:23

test: add api.test.js

c9a28ca

test: add new tests to TaxonomyListPage.test.jsx

a9414cb

Merge branch 'master' into rpenido/fal-3532-download-template

aaf957e

rpenido force-pushed the rpenido/fal-3532-download-template branch from 193cf98 to a9414cb Compare November 9, 2023 23:23

fix: Dropdown.Menu

4515f9f

pomegranited approved these changes Nov 10, 2023

View reviewed changes

test: fix download template click

48c93b1

rpenido marked this pull request as ready for review November 10, 2023 18:46

jristau1984 added the jira:2u We want an issue in the 2U Jira instance label Nov 14, 2023

Merge branch 'master' into rpenido/fal-3532-download-template

86d3333

rpenido changed the title ~~feat: add download template button to taxonomy list~~ feat: add download template button to taxonomy list [FC-0036] Nov 16, 2023

rpenido and others added 3 commits November 20, 2023 13:56

Merge branch 'openedx:master' into rpenido/fal-3532-download-template

b6c6675

Merge branch 'master' into rpenido/fal-3532-download-template

64c64cb

style: lint

4db7091

Merge branch 'master' into rpenido/fal-3532-download-template

36d62dd

style: fix eslint

b36d455

xitij2000 approved these changes Dec 15, 2023

View reviewed changes

fix: changes from review

7f6319e

xitij2000 approved these changes Dec 18, 2023

View reviewed changes

xitij2000 merged commit a37d13f into openedx:master Dec 19, 2023
6 checks passed

xitij2000 deleted the rpenido/fal-3532-download-template branch December 19, 2023 04:01

kenclary mentioned this pull request Jan 9, 2024

fix: configure webpack to fix paid powerpaste plugin #783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add download template button to taxonomy list [FC-0036] #674

feat: add download template button to taxonomy list [FC-0036] #674

rpenido commented Nov 8, 2023 •

edited

Loading

openedx-webhooks commented Nov 8, 2023 •

edited

Loading

codecov bot commented Nov 8, 2023 •

edited

Loading

pomegranited commented Nov 10, 2023

pomegranited left a comment

rpenido commented Nov 10, 2023

pomegranited commented Nov 13, 2023

bradenmacdonald commented Nov 13, 2023

openedx-webhooks commented Nov 14, 2023

pomegranited commented Nov 16, 2023

rpenido commented Dec 12, 2023

jristau1984 commented Dec 13, 2023

xitij2000 left a comment

xitij2000 Dec 15, 2023

rpenido Dec 15, 2023

xitij2000 Dec 15, 2023

rpenido Dec 15, 2023

xitij2000 Dec 15, 2023

rpenido Dec 15, 2023

xitij2000 left a comment

xitij2000 Dec 18, 2023

rpenido Dec 18, 2023

openedx-webhooks commented Dec 19, 2023

feat: add download template button to taxonomy list [FC-0036] #674

feat: add download template button to taxonomy list [FC-0036] #674

Conversation

rpenido commented Nov 8, 2023 • edited Loading

Description

Supporting Information

Testing instructions

openedx-webhooks commented Nov 8, 2023 • edited Loading

codecov bot commented Nov 8, 2023 • edited Loading

Codecov Report

pomegranited commented Nov 10, 2023

pomegranited left a comment

Choose a reason for hiding this comment

rpenido commented Nov 10, 2023

pomegranited commented Nov 13, 2023

bradenmacdonald commented Nov 13, 2023

openedx-webhooks commented Nov 14, 2023

pomegranited commented Nov 16, 2023

rpenido commented Dec 12, 2023

jristau1984 commented Dec 13, 2023

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

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 Dec 19, 2023

rpenido commented Nov 8, 2023 •

edited

Loading

openedx-webhooks commented Nov 8, 2023 •

edited

Loading

codecov bot commented Nov 8, 2023 •

edited

Loading