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.

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 import taxonomy feature [FC-0036] #675

Merged
merged 48 commits into from
Jan 8, 2024
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
0c60c37
feat: add import taxonomy feature
rpenido Nov 9, 2023
c273af1
test: add api.test.js
rpenido Nov 9, 2023
047530d
test: add import button click test
rpenido Nov 9, 2023
303e7ba
test: add action.test.js
rpenido Nov 9, 2023
3955e91
test: add more tests to action.tests.js
rpenido Nov 9, 2023
8176973
test: add more tests to action.tests.js 2
rpenido Nov 9, 2023
4f31714
Merge branch 'openedx:master' into rpenido/fal-3532-import-taxonomy
rpenido Nov 10, 2023
df441fb
fix: import
rpenido Nov 10, 2023
a0e92f0
test: simplify import test
rpenido Nov 10, 2023
dc8ed9e
fix: remove undefined var
rpenido Nov 10, 2023
120154e
refactor: rename actions.js -> utils.js
rpenido Nov 10, 2023
318bbb7
revert: change in the jest.config
rpenido Nov 10, 2023
a8c2fd5
Merge branch 'master' into rpenido/fal-3532-import-taxonomy
pomegranited Nov 16, 2023
70ac2a7
Merge branch 'openedx:master' into rpenido/fal-3532-import-taxonomy
rpenido Nov 20, 2023
adacf23
chore: trigger CD/CI
rpenido Nov 20, 2023
5983953
Merge branch 'master' into rpenido/fal-3532-import-taxonomy
rpenido Nov 20, 2023
7756c7f
feat: import tags to existing taxonomy
rpenido Nov 24, 2023
4202c27
Merge branch 'master' into rpenido/fal-3532-import-taxonomy
rpenido Nov 24, 2023
6bcd924
refactor: merges TaxonomyCardMenu and TaxonomyDetailMenu (#13)
rpenido Nov 30, 2023
f318dfc
refactor: improve menu organization
rpenido Dec 4, 2023
204b2b9
fix: types
rpenido Dec 9, 2023
5e0f599
Merge branch 'master' into rpenido/fal-3532-import-taxonomy
rpenido Dec 12, 2023
f888ecf
refactor: merging conflicts
rpenido Dec 12, 2023
3c8da66
refactor: change export syntax
rpenido Dec 12, 2023
40479c1
fix: eslint
rpenido Dec 12, 2023
46e5d1b
fix: export TaxonomyDetail
rpenido Dec 12, 2023
db8ba58
fix: eslint
rpenido Dec 12, 2023
8fe8c07
Merge branch 'openedx:master' into rpenido/fal-3532-import-taxonomy
rpenido Dec 14, 2023
1440215
fix: TaxonomyContext types
rpenido Dec 15, 2023
e5b8e58
Merge branch 'master' into rpenido/fal-3532-import-taxonomy
rpenido Dec 19, 2023
fafcb9b
chore: rebasing
rpenido Dec 19, 2023
e80194f
fix: types
rpenido Dec 19, 2023
1e0dde3
fix: remove typo
rpenido Dec 19, 2023
7cae57b
test: cleaning test removing useMutation mock
rpenido Dec 19, 2023
1002fc6
style: cleaning code and fix lint
rpenido Dec 19, 2023
fdc74c7
Merge branch 'master' into rpenido/fal-3532-import-taxonomy
rpenido Dec 21, 2023
e9893dc
fix: change expect getBy toThrow pattern to queryBy not.toBeInDocument
rpenido Dec 28, 2023
a8b4197
test: add jest-each
rpenido Dec 28, 2023
a3c40fd
fix: remove getTaxonomyMenuItems function
rpenido Dec 28, 2023
1a31aa9
fix: remove tests from temp code
rpenido Dec 28, 2023
c6d39e5
Merge branch 'openedx:master' into rpenido/fal-3532-import-taxonomy
rpenido Dec 28, 2023
583ab51
test: improve coverage
rpenido Dec 28, 2023
151ba9e
test: refactor import button test
rpenido Dec 29, 2023
df9f2a0
fix: eslint
rpenido Dec 29, 2023
67c42cb
Merge branch 'master' into rpenido/fal-3532-import-taxonomy
rpenido Jan 4, 2024
793810f
fix: errors fixing merge conflicts
rpenido Jan 4, 2024
70954b3
Merge branch 'master' into rpenido/fal-3532-import-taxonomy
pomegranited Jan 7, 2024
68e820d
fixup!: remove jest-each in favour of inbuilt functionality
xitij2000 Jan 8, 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
30 changes: 11 additions & 19 deletions src/taxonomy/TaxonomyListPage.jsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useContext } from 'react';
import React from 'react';
import {
Button,
CardView,
Expand All @@ -13,14 +13,15 @@ import {
Add,
} from '@edx/paragon/icons';
import { useIntl } from '@edx/frontend-platform/i18n';

import { Helmet } from 'react-helmet';
import SubHeader from '../generic/sub-header/SubHeader';
import getPageHeadTitle from '../generic/utils';
import { getTaxonomyTemplateApiUrl } from './data/api';
import { useTaxonomyListDataResponse, useIsTaxonomyListDataLoaded } from './data/apiHooks';
import { importTaxonomy } from './import-tags';
import messages from './messages';
import TaxonomyCard from './taxonomy-card';
import { getTaxonomyTemplateApiUrl } from './data/api';
import { useTaxonomyListDataResponse, useIsTaxonomyListDataLoaded, useDeleteTaxonomy } from './data/apiHooks';
import { TaxonomyContext } from './common/context';

const TaxonomyListHeaderButtons = () => {
const intl = useIntl();
Expand Down Expand Up @@ -57,7 +58,11 @@ const TaxonomyListHeaderButtons = () => {
</Dropdown.Menu>
</Dropdown>
</OverlayTrigger>
<Button iconBefore={Add} disabled>
<Button
iconBefore={Add}
onClick={() => importTaxonomy(intl)}
data-testid="taxonomy-import-button"
>
{intl.formatMessage(messages.importButtonLabel)}
</Button>
</>
Expand All @@ -66,19 +71,6 @@ const TaxonomyListHeaderButtons = () => {

const TaxonomyListPage = () => {
const intl = useIntl();
const deleteTaxonomy = useDeleteTaxonomy();
const { setToastMessage } = useContext(TaxonomyContext);

const onDeleteTaxonomy = React.useCallback((id, name) => {
deleteTaxonomy({ pk: id }, {
onSuccess: async () => {
setToastMessage(intl.formatMessage(messages.taxonomyDeleteToast, { name }));
},
onError: async () => {
// TODO: display the error to the user
},
});
}, [setToastMessage]);

Comment on lines -145 to -154
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code went to taxonomy-menu feature

const useTaxonomyListData = () => {
const taxonomyListData = useTaxonomyListDataResponse();
Expand Down Expand Up @@ -139,7 +131,7 @@ const TaxonomyListPage = () => {
>
<CardView
className="bg-light-400 p-5"
CardComponent={(row) => TaxonomyCard({ ...row, onDeleteTaxonomy })}
CardComponent={(row) => TaxonomyCard(row)}
/>
</DataTable>
)}
Expand Down
68 changes: 36 additions & 32 deletions src/taxonomy/TaxonomyListPage.test.jsx
Original file line number Diff line number Diff line change
@@ -1,50 +1,53 @@
import React, { useMemo } from 'react';
import React from 'react';
import { IntlProvider, injectIntl } from '@edx/frontend-platform/i18n';
import { initializeMockApp } from '@edx/frontend-platform';
import { AppProvider } from '@edx/frontend-platform/react';
import { act, render, fireEvent } from '@testing-library/react';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import { act, fireEvent, render } from '@testing-library/react';

import initializeStore from '../store';
import { getTaxonomyTemplateApiUrl } from './data/api';
import TaxonomyListPage from './TaxonomyListPage';
import { useTaxonomyListDataResponse, useIsTaxonomyListDataLoaded } from './data/apiHooks';
import { importTaxonomy } from './import-tags';
import { TaxonomyContext } from './common/context';

let store;
const mockSetToastMessage = jest.fn();
const mockDeleteTaxonomy = jest.fn();

const taxonomies = [{
id: 1,
name: 'Taxonomy',
description: 'This is a description',
}];

jest.mock('./data/apiHooks', () => ({
...jest.requireActual('./data/apiHooks'),
useTaxonomyListDataResponse: jest.fn(),
useIsTaxonomyListDataLoaded: jest.fn(),
useDeleteTaxonomy: () => mockDeleteTaxonomy,
}));
jest.mock('./taxonomy-card/TaxonomyCardMenu', () => jest.fn(({ onClickMenuItem }) => (
// eslint-disable-next-line jsx-a11y/control-has-associated-label
<button type="button" data-testid="test-delete-button" onClick={() => onClickMenuItem('delete')} />
)));

const RootWrapper = () => {
const context = useMemo(() => ({
toastMessage: null,
setToastMessage: mockSetToastMessage,
}), []);
jest.mock('./import-tags', () => ({
importTaxonomy: jest.fn(),
}));

const context = {
toastMessage: null,
setToastMessage: jest.fn(),
};

const queryClient = new QueryClient();

return (
<AppProvider store={store}>
<IntlProvider locale="en" messages={{}}>
const RootWrapper = () => (
<AppProvider store={store}>
<IntlProvider locale="en" messages={{}}>
<QueryClientProvider client={queryClient}>
<TaxonomyContext.Provider value={context}>
<TaxonomyListPage intl={injectIntl} />
</TaxonomyContext.Provider>
</IntlProvider>
</AppProvider>
);
};
</QueryClientProvider>
</IntlProvider>
</AppProvider>
);

describe('<TaxonomyListPage />', async () => {
beforeEach(async () => {
Expand Down Expand Up @@ -102,20 +105,21 @@ describe('<TaxonomyListPage />', async () => {
expect(templateButton.href).toBe(getTaxonomyTemplateApiUrl(fileFormat.toLowerCase()));
});

it('should show the success toast after delete', async () => {
it('calls the import taxonomy action when the import button is clicked', async () => {
useIsTaxonomyListDataLoaded.mockReturnValue(true);
useTaxonomyListDataResponse.mockReturnValue({
results: taxonomies,
results: [{
id: 1,
name: 'Taxonomy',
description: 'This is a description',
}],
});
mockDeleteTaxonomy.mockImplementationOnce(async (params, callbacks) => {
callbacks.onSuccess();
await act(async () => {
const { getByTestId } = render(<RootWrapper />);
const importButton = getByTestId('taxonomy-import-button');
expect(importButton).toBeInTheDocument();
importButton.click();
expect(importTaxonomy).toHaveBeenCalled();
});
const { getByTestId, getByLabelText } = render(<RootWrapper />);
rpenido marked this conversation as resolved.
Show resolved Hide resolved
fireEvent.click(getByTestId('test-delete-button'));
fireEvent.change(getByLabelText('Type DELETE to confirm'), { target: { value: 'DELETE' } });
fireEvent.click(getByTestId('delete-button'));

expect(mockDeleteTaxonomy).toBeCalledTimes(1);
expect(mockSetToastMessage).toBeCalledWith(`"${taxonomies[0].name}" deleted`);
});
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 test can be skipped since it isn't doing anything. It's testing the presence of the import button, and then checking that clicking on the button fires the click handler. I think it should either be a deeper test of that UI or just be skipped.

Copy link
Contributor Author

@rpenido rpenido Dec 28, 2023

Choose a reason for hiding this comment

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

This is only for test coverage. Is it ok to skip it? More about this in the comment at api.test.js

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 think that is a recurring issue. I've made a suggestion for the test, other than that I guess we should leave it for now.

});
5 changes: 3 additions & 2 deletions src/taxonomy/common/context.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
// @ts-check
/* eslint-disable import/prefer-default-export */
import React from 'react';

export const TaxonomyContext = React.createContext({
toastMessage: null,
setToastMessage: null,
toastMessage: /** @type{null|string} */ (null),
setToastMessage: /** @type{null|function} */ (null),
});
2 changes: 2 additions & 0 deletions src/taxonomy/data/types.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@
* @typedef {Object} TaxonomyData
* @property {number} id
* @property {string} name
* @property {string} description
* @property {boolean} enabled
* @property {boolean} allowMultiple
* @property {boolean} allowFreeText
* @property {boolean} systemDefined
* @property {boolean} visibleToAuthors
* @property {number} tagsCount
* @property {string[]} orgs
*/

Expand Down
99 changes: 51 additions & 48 deletions src/taxonomy/delete-dialog/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, { useState } from 'react';
import {
ActionRow,
Button,
Container,
Form,
ModalDialog,
Icon,
Expand Down Expand Up @@ -36,55 +37,57 @@ const DeleteDialog = ({
}, [onClose, onDelete]);

return (
<ModalDialog
title={intl.formatMessage(messages.deleteDialogTitle, { taxonomyName })}
isOpen={isOpen}
onClose={onClose}
size="md"
hasCloseButton={false}
variant="warning"
className="taxonomy-delete-dialog"
>
<ModalDialog.Header>
<ModalDialog.Title>
<Icon src={Warning} className="d-inline-block text-warning warning-icon" />
{intl.formatMessage(messages.deleteDialogTitle, { taxonomyName })}
</ModalDialog.Title>
</ModalDialog.Header>
<ModalDialog.Body>
<div className="mb-4">
{/* Delete `(?)` after implement get tags count of a taxonomy */}
{intl.formatMessage(messages.deleteDialogBody, {
tagsCount: tagsCount !== undefined ? tagsCount : '(?)',
})}
</div>
<Form.Group>
<Form.Label>
{intl.formatMessage(messages.deleteDialogConfirmLabel, {
deleteLabel: <b>{deleteLabel}</b>,
<Container onClick={(e) => e.stopPropagation() /* This prevents calling onClick handler from the parent */}>
<ModalDialog
title={intl.formatMessage(messages.deleteDialogTitle, { taxonomyName })}
isOpen={isOpen}
onClose={onClose}
size="md"
hasCloseButton={false}
variant="warning"
className="taxonomy-delete-dialog"
>
<ModalDialog.Header>
<ModalDialog.Title>
<Icon src={Warning} className="d-inline-block text-warning warning-icon" />
{intl.formatMessage(messages.deleteDialogTitle, { taxonomyName })}
</ModalDialog.Title>
</ModalDialog.Header>
<ModalDialog.Body>
<div className="mb-4">
{/* Delete `(?)` after implement get tags count of a taxonomy */}
{intl.formatMessage(messages.deleteDialogBody, {
tagsCount: tagsCount !== undefined ? tagsCount : '(?)',
})}
</Form.Label>
<Form.Control
onChange={handleInputChange}
/>
</Form.Group>
</ModalDialog.Body>
<ModalDialog.Footer>
<ActionRow>
<ModalDialog.CloseButton variant="tertiary">
{intl.formatMessage(messages.deleteDialogCancelLabel)}
</ModalDialog.CloseButton>
<Button
variant="primary"
disabled={deleteButtonDisabled}
onClick={onClickDelete}
data-testid="delete-button"
>
{intl.formatMessage(messages.deleteDialogDeleteLabel)}
</Button>
</ActionRow>
</ModalDialog.Footer>
</ModalDialog>
</div>
<Form.Group>
<Form.Label>
{intl.formatMessage(messages.deleteDialogConfirmLabel, {
deleteLabel: <b>{deleteLabel}</b>,
})}
</Form.Label>
<Form.Control
onChange={handleInputChange}
/>
</Form.Group>
</ModalDialog.Body>
<ModalDialog.Footer>
<ActionRow>
<ModalDialog.CloseButton variant="tertiary">
{intl.formatMessage(messages.deleteDialogCancelLabel)}
</ModalDialog.CloseButton>
<Button
variant="primary"
disabled={deleteButtonDisabled}
onClick={onClickDelete}
data-testid="delete-button"
>
{intl.formatMessage(messages.deleteDialogDeleteLabel)}
</Button>
</ActionRow>
</ModalDialog.Footer>
</ModalDialog>
</Container>
);
};

Expand Down
Loading