From 18a2aa3f2f0f1dfb4a63f22f568999a4f9bd2ba2 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Mon, 8 Jan 2024 01:09:45 +1030 Subject: [PATCH 1/7] feat: use the permissions returned by the taxonomy REST API to determine which actions/menu items to offer the user --- src/taxonomy/TaxonomyListPage.jsx | 10 +++- src/taxonomy/TaxonomyListPage.test.jsx | 33 ++++++++---- src/taxonomy/__mocks__/taxonomyListMock.js | 17 ++++++ src/taxonomy/data/api.js | 1 - src/taxonomy/data/types.mjs | 8 +++ .../taxonomy-card/TaxonomyCard.test.jsx | 37 ++++++++++++- src/taxonomy/taxonomy-card/index.jsx | 4 ++ .../TaxonomyDetailPage.test.jsx | 34 ++++++++++-- src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx | 19 +++---- .../taxonomy-menu/TaxonomyMenu.test.jsx | 53 +++++++++---------- 10 files changed, 162 insertions(+), 54 deletions(-) diff --git a/src/taxonomy/TaxonomyListPage.jsx b/src/taxonomy/TaxonomyListPage.jsx index 76b9593102..d0c6d9a408 100644 --- a/src/taxonomy/TaxonomyListPage.jsx +++ b/src/taxonomy/TaxonomyListPage.jsx @@ -31,7 +31,7 @@ import TaxonomyCard from './taxonomy-card'; const ALL_TAXONOMIES = 'All taxonomies'; const UNASSIGNED = 'Unassigned'; -const TaxonomyListHeaderButtons = () => { +const TaxonomyListHeaderButtons = ({ canAdd }) => { const intl = useIntl(); return ( <> @@ -70,6 +70,7 @@ const TaxonomyListHeaderButtons = () => { iconBefore={Add} onClick={() => importTaxonomy(intl)} data-testid="taxonomy-import-button" + disabled={!canAdd} > {intl.formatMessage(messages.importButtonLabel)} @@ -156,6 +157,7 @@ const TaxonomyListPage = () => { return { taxonomyListData, isLoaded }; }; const { taxonomyListData, isLoaded } = useTaxonomyListData(); + const canAdd = isLoaded ? taxonomyListData.canAdd : false; const getOrgSelect = () => ( // Initialize organization select component @@ -177,7 +179,7 @@ const TaxonomyListPage = () => { } + headerActions={} hideBorder /> @@ -232,6 +234,10 @@ const TaxonomyListPage = () => { ); }; +TaxonomyListHeaderButtons.propTypes = { + canAdd: PropTypes.bool.isRequired, +}; + OrganizationFilterSelector.propTypes = { isOrganizationListLoaded: PropTypes.bool.isRequired, organizationListData: PropTypes.arrayOf(PropTypes.string).isRequired, diff --git a/src/taxonomy/TaxonomyListPage.test.jsx b/src/taxonomy/TaxonomyListPage.test.jsx index 34f55f68f3..ee013376f1 100644 --- a/src/taxonomy/TaxonomyListPage.test.jsx +++ b/src/taxonomy/TaxonomyListPage.test.jsx @@ -21,6 +21,12 @@ const taxonomies = [{ id: 1, name: 'Taxonomy', description: 'This is a description', + showSystemBadge: false, + tagsCount: 0, + userPermissions: { + canChange: true, + canDelete: true, + }, }]; const organizationsListUrl = 'http://localhost:18010/organizations'; const organizations = ['Org 1', 'Org 2']; @@ -99,11 +105,7 @@ describe('', () => { 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', - }], + results: taxonomies, }); const { findByRole } = render(); const templateMenu = await findByRole('button', { name: 'Download template' }); @@ -114,19 +116,28 @@ describe('', () => { expect(templateButton.href).toBe(getTaxonomyTemplateApiUrl(fileFormat.toLowerCase())); }); + it('disables the import taxonomy button if not permitted', async () => { + useIsTaxonomyListDataLoaded.mockReturnValue(true); + useTaxonomyListDataResponse.mockReturnValue({ + results: [], + canAdd: false, + }); + + const { getByRole } = render(); + const importButton = getByRole('button', { name: 'Import' }); + expect(importButton).toBeDisabled(); + }); + it('calls the import taxonomy action when the import button is clicked', async () => { useIsTaxonomyListDataLoaded.mockReturnValue(true); useTaxonomyListDataResponse.mockReturnValue({ - results: [{ - id: 1, - name: 'Taxonomy', - description: 'This is a description', - }], + results: [], + canAdd: true, }); const { getByRole } = render(); const importButton = getByRole('button', { name: 'Import' }); - expect(importButton).toBeInTheDocument(); + expect(importButton).not.toBeDisabled(); fireEvent.click(importButton); expect(importTaxonomy).toHaveBeenCalled(); }); diff --git a/src/taxonomy/__mocks__/taxonomyListMock.js b/src/taxonomy/__mocks__/taxonomyListMock.js index 5eab0af023..c31228b922 100644 --- a/src/taxonomy/__mocks__/taxonomyListMock.js +++ b/src/taxonomy/__mocks__/taxonomyListMock.js @@ -5,6 +5,7 @@ module.exports = { numPages: 1, currentPage: 1, start: 0, + canAdd: true, results: [ { id: -2, @@ -15,6 +16,10 @@ module.exports = { allowFreeText: false, systemDefined: true, visibleToAuthors: false, + userPermissions: { + canChange: false, + canDelete: false, + }, }, { id: -1, @@ -25,6 +30,10 @@ module.exports = { allowFreeText: false, systemDefined: true, visibleToAuthors: true, + userPermissions: { + canChange: false, + canDelete: false, + }, }, { id: 1, @@ -35,6 +44,10 @@ module.exports = { allowFreeText: false, systemDefined: false, visibleToAuthors: true, + userPermissions: { + canChange: true, + canDelete: true, + }, }, { id: 2, @@ -45,6 +58,10 @@ module.exports = { allowFreeText: false, systemDefined: false, visibleToAuthors: true, + userPermissions: { + canChange: true, + canDelete: true, + }, }, ], }; diff --git a/src/taxonomy/data/api.js b/src/taxonomy/data/api.js index be3c276e16..46f5c44494 100644 --- a/src/taxonomy/data/api.js +++ b/src/taxonomy/data/api.js @@ -7,7 +7,6 @@ const getApiBaseUrl = () => getConfig().STUDIO_BASE_URL; export const getTaxonomyListApiUrl = (org) => { const url = new URL('api/content_tagging/v1/taxonomies/', getApiBaseUrl()); url.searchParams.append('enabled', 'true'); - url.searchParams.append('page_size', '500'); // For the tagging MVP, we don't paginate the taxonomy list if (org !== undefined) { if (org === 'Unassigned') { url.searchParams.append('unassigned', 'true'); diff --git a/src/taxonomy/data/types.mjs b/src/taxonomy/data/types.mjs index b892592a5e..72c165fdc3 100644 --- a/src/taxonomy/data/types.mjs +++ b/src/taxonomy/data/types.mjs @@ -1,5 +1,11 @@ // @ts-check +/** + * @typedef {Object} UserPermissions + * @property {boolean} canChange + * @property {boolean} canDelete + **/ + /** * @typedef {Object} TaxonomyData * @property {number} id @@ -12,6 +18,7 @@ * @property {boolean} visibleToAuthors * @property {number} tagsCount * @property {string[]} orgs + * @property {UserPermissions} userPermissions */ /** @@ -23,5 +30,6 @@ * @property {number} currentPage * @property {number} start * @property {function} refetch + * @property {boolean} canAdd * @property {TaxonomyData[]} results */ diff --git a/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx b/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx index 6cbbf4c630..3439b9b36a 100644 --- a/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx +++ b/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx @@ -3,7 +3,7 @@ import { IntlProvider } from '@edx/frontend-platform/i18n'; import { initializeMockApp } from '@edx/frontend-platform'; import { AppProvider } from '@edx/frontend-platform/react'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; -import { render } from '@testing-library/react'; +import { fireEvent, render } from '@testing-library/react'; import PropTypes from 'prop-types'; import initializeStore from '../../store'; @@ -16,6 +16,10 @@ const data = { id: taxonomyId, name: 'Taxonomy 1', description: 'This is a description', + userPermissions: { + canChange: true, + canDelete: true, + }, }; const queryClient = new QueryClient(); @@ -40,6 +44,10 @@ TaxonomyCardComponent.propTypes = { systemDefined: PropTypes.bool, orgsCount: PropTypes.number, onDeleteTaxonomy: PropTypes.func, + userPermissions: PropTypes.shape({ + canChange: PropTypes.bool, + canDelete: PropTypes.bool, + }), }).isRequired, }; @@ -62,6 +70,33 @@ describe('', async () => { expect(getByText(data.description)).toBeInTheDocument(); }); + it('should show the ⋮ menu', () => { + const { getByTestId, queryByTestId } = render(); + + // Menu closed/doesn't exist yet + expect(queryByTestId('taxonomy-menu')).not.toBeInTheDocument(); + + // Click on the menu button to open + fireEvent.click(getByTestId('taxonomy-menu-button')); + + // Menu opened + expect(getByTestId('taxonomy-menu')).toBeVisible(); + expect(getByTestId('taxonomy-menu-import')).toBeVisible(); + expect(getByTestId('taxonomy-menu-export')).toBeVisible(); + expect(getByTestId('taxonomy-menu-delete')).toBeVisible(); + + // Click on button again to close the menu + fireEvent.click(getByTestId('taxonomy-menu-button')); + + // Menu closed + // Jest bug: toBeVisible() isn't checking opacity correctly + // expect(getByTestId('taxonomy-menu')).not.toBeVisible(); + expect(getByTestId('taxonomy-menu').style.opacity).toEqual('0'); + + // Menu button still visible + expect(getByTestId('taxonomy-menu-button')).toBeVisible(); + }); + it('not show the system-defined badge with normal taxonomies', () => { const { queryByText } = render(); expect(queryByText('System-level')).not.toBeInTheDocument(); diff --git a/src/taxonomy/taxonomy-card/index.jsx b/src/taxonomy/taxonomy-card/index.jsx index 1449730ae5..d9c5e58ea5 100644 --- a/src/taxonomy/taxonomy-card/index.jsx +++ b/src/taxonomy/taxonomy-card/index.jsx @@ -149,6 +149,10 @@ TaxonomyCard.propTypes = { systemDefined: PropTypes.bool, orgsCount: PropTypes.number, tagsCount: PropTypes.number, + userPermissions: PropTypes.shape({ + canChange: PropTypes.bool.isRequired, + canDelete: PropTypes.bool.isRequired, + }).isRequired, }).isRequired, }; diff --git a/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx b/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx index 1f245356b6..4432a91f78 100644 --- a/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx +++ b/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx @@ -2,7 +2,7 @@ import React, { useMemo } from 'react'; import { initializeMockApp } from '@edx/frontend-platform'; import { IntlProvider } from '@edx/frontend-platform/i18n'; import { AppProvider } from '@edx/frontend-platform/react'; -import { render } from '@testing-library/react'; +import { fireEvent, render } from '@testing-library/react'; import { useTaxonomyDetailData } from './data/api'; import initializeStore from '../../store'; @@ -89,10 +89,36 @@ describe('', async () => { name: 'Test taxonomy', description: 'This is a description', systemDefined: true, + userPermissions: { + canChange: true, + canDelete: true, + }, }, }); - const { getByRole } = render(); - expect(getByRole('heading')).toHaveTextContent('Test taxonomy'); + const { getByTestId, queryByTestId } = render(); + + // Menu closed/doesn't exist yet + expect(queryByTestId('taxonomy-menu')).not.toBeInTheDocument(); + + // Click on the menu button to open + fireEvent.click(getByTestId('taxonomy-menu-button')); + + // Menu opened + expect(getByTestId('taxonomy-menu')).toBeVisible(); + expect(getByTestId('taxonomy-menu-import')).toBeVisible(); + expect(getByTestId('taxonomy-menu-export')).toBeVisible(); + expect(getByTestId('taxonomy-menu-delete')).toBeVisible(); + + // Click on button again to close the menu + fireEvent.click(getByTestId('taxonomy-menu-button')); + + // Menu closed + // Jest bug: toBeVisible() isn't checking opacity correctly + // expect(getByTestId('taxonomy-menu')).not.toBeVisible(); + expect(getByTestId('taxonomy-menu').style.opacity).toEqual('0'); + + // Menu button still visible + expect(getByTestId('taxonomy-menu-button')).toBeVisible(); }); it('should show system defined badge', async () => { @@ -105,6 +131,7 @@ describe('', async () => { name: 'Test taxonomy', description: 'This is a description', systemDefined: true, + userPermissions: {}, }, }); const { getByText } = render(); @@ -121,6 +148,7 @@ describe('', async () => { name: 'Test taxonomy', description: 'This is a description', systemDefined: false, + userPermissions: {}, }, }); const { queryByText } = render(); diff --git a/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx b/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx index 0db273cf46..2f66e2a2e0 100644 --- a/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx +++ b/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx @@ -9,7 +9,7 @@ import { IconButton, } from '@edx/paragon'; import { MoreVert } from '@edx/paragon/icons'; -import { omitBy } from 'lodash'; +import { pickBy } from 'lodash'; import PropTypes from 'prop-types'; import { useNavigate } from 'react-router-dom'; @@ -50,7 +50,7 @@ const TaxonomyMenu = ({ * @typedef {Object} MenuItem * @property {string} title - The title of the menu item * @property {() => void} action - The action to perform when the menu item is clicked - * @property {boolean} [hide] - Whether or not to hide the menu item + * @property {boolean} [show] - Whether or not to show the menu item * * @constant * @type {Record} @@ -59,23 +59,22 @@ const TaxonomyMenu = ({ import: { title: intl.formatMessage(messages.importMenu), action: () => importTaxonomyTags(taxonomy.id, intl), - // Hide import menu item if taxonomy is system defined or allows free text - hide: taxonomy.systemDefined || taxonomy.allowFreeText, + show: taxonomy.userPermissions.canChange, }, export: { title: intl.formatMessage(messages.exportMenu), action: exportModalOpen, + show: true, // if we can view the taxonomy, we can export it }, delete: { title: intl.formatMessage(messages.deleteMenu), action: deleteDialogOpen, - // Hide delete menu item if taxonomy is system defined - hide: taxonomy.systemDefined, + show: taxonomy.userPermissions.canDelete, }, }; // Remove hidden menu items - menuItems = omitBy(menuItems, (value) => value.hide); + menuItems = pickBy(menuItems, (value) => value.show); const renderModals = () => ( <> @@ -136,9 +135,11 @@ TaxonomyMenu.propTypes = { taxonomy: PropTypes.shape({ id: PropTypes.number.isRequired, name: PropTypes.string.isRequired, - systemDefined: PropTypes.bool.isRequired, - allowFreeText: PropTypes.bool.isRequired, tagsCount: PropTypes.number.isRequired, + userPermissions: PropTypes.shape({ + canChange: PropTypes.bool.isRequired, + canDelete: PropTypes.bool.isRequired, + }).isRequired, }).isRequired, iconMenu: PropTypes.bool, }; diff --git a/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx b/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx index d8a44033ea..f30201ed40 100644 --- a/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx +++ b/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx @@ -31,9 +31,9 @@ const queryClient = new QueryClient(); const mockSetToastMessage = jest.fn(); const TaxonomyMenuComponent = ({ - systemDefined, - allowFreeText, iconMenu, + canChange, + canDelete, }) => { const context = useMemo(() => ({ toastMessage: null, @@ -49,9 +49,10 @@ const TaxonomyMenuComponent = ({ taxonomy={{ id: taxonomyId, name: taxonomyName, - systemDefined, - allowFreeText, tagsCount: 0, + userPermissions: { + canChange, canDelete, + }, }} iconMenu={iconMenu} /> @@ -64,13 +65,13 @@ const TaxonomyMenuComponent = ({ TaxonomyMenuComponent.propTypes = { iconMenu: PropTypes.bool.isRequired, - systemDefined: PropTypes.bool, - allowFreeText: PropTypes.bool, + canChange: PropTypes.bool, + canDelete: PropTypes.bool, }; TaxonomyMenuComponent.defaultProps = { - systemDefined: false, - allowFreeText: false, + canChange: true, + canDelete: true, }; describe.each([true, false])('', async (iconMenu) => { @@ -114,36 +115,34 @@ describe.each([true, false])('', async (iconMenu) => expect(getByTestId('taxonomy-menu-button')).toBeVisible(); }); - test('doesnt show systemDefined taxonomies disabled menus', () => { - const { getByTestId, queryByTestId } = render(); - - // Menu closed/doesn't exist yet - expect(queryByTestId('taxonomy-menu')).not.toBeInTheDocument(); + test('Shows menu actions that user is permitted', () => { + const { getByTestId, queryByTestId } = render(); // Click on the menu button to open fireEvent.click(getByTestId('taxonomy-menu-button')); - // Menu opened - expect(getByTestId('taxonomy-menu')).toBeVisible(); - - // Check that the import menu is not show - expect(queryByTestId('taxonomy-menu-import')).not.toBeInTheDocument(); + // Ensure that the menu items are present + expect(queryByTestId('taxonomy-menu-export')).toBeInTheDocument(); + expect(queryByTestId('taxonomy-menu-import')).toBeInTheDocument(); + expect(queryByTestId('taxonomy-menu-delete')).toBeInTheDocument(); }); - test('doesnt show freeText taxonomies disabled menus', () => { - const { getByTestId, queryByTestId } = render(); - - // Menu closed/doesn't exist yet - expect(queryByTestId('taxonomy-menu')).not.toBeInTheDocument(); + test('Hides menu actions that user is not permitted', () => { + const { getByTestId, queryByTestId } = render( + , + ); // Click on the menu button to open fireEvent.click(getByTestId('taxonomy-menu-button')); - // Menu opened - expect(getByTestId('taxonomy-menu')).toBeVisible(); - - // Check that the import menu is not show + // Ensure expected menu items are found + expect(queryByTestId('taxonomy-menu-export')).toBeInTheDocument(); expect(queryByTestId('taxonomy-menu-import')).not.toBeInTheDocument(); + expect(queryByTestId('taxonomy-menu-delete')).not.toBeInTheDocument(); }); test('should open export modal on export menu click', () => { From cb2d8146808dd17f23c87a45eaf8f256b1f6a82f Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 11 Jan 2024 15:58:17 +1030 Subject: [PATCH 2/7] fix: REST API is now returning flattened user permissions --- src/taxonomy/TaxonomyListPage.test.jsx | 6 ++--- src/taxonomy/__mocks__/taxonomyListMock.js | 24 +++++++------------ src/taxonomy/data/types.mjs | 9 ++----- .../taxonomy-card/TaxonomyCard.test.jsx | 12 ++++------ src/taxonomy/taxonomy-card/index.jsx | 6 ++--- .../TaxonomyDetailPage.test.jsx | 7 ++---- src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx | 10 ++++---- .../taxonomy-menu/TaxonomyMenu.test.jsx | 5 ++-- 8 files changed, 26 insertions(+), 53 deletions(-) diff --git a/src/taxonomy/TaxonomyListPage.test.jsx b/src/taxonomy/TaxonomyListPage.test.jsx index ee013376f1..978f1731ac 100644 --- a/src/taxonomy/TaxonomyListPage.test.jsx +++ b/src/taxonomy/TaxonomyListPage.test.jsx @@ -23,10 +23,8 @@ const taxonomies = [{ description: 'This is a description', showSystemBadge: false, tagsCount: 0, - userPermissions: { - canChange: true, - canDelete: true, - }, + canChange: true, + canDelete: true, }]; const organizationsListUrl = 'http://localhost:18010/organizations'; const organizations = ['Org 1', 'Org 2']; diff --git a/src/taxonomy/__mocks__/taxonomyListMock.js b/src/taxonomy/__mocks__/taxonomyListMock.js index c31228b922..5e96b064eb 100644 --- a/src/taxonomy/__mocks__/taxonomyListMock.js +++ b/src/taxonomy/__mocks__/taxonomyListMock.js @@ -16,10 +16,8 @@ module.exports = { allowFreeText: false, systemDefined: true, visibleToAuthors: false, - userPermissions: { - canChange: false, - canDelete: false, - }, + canChange: false, + canDelete: false, }, { id: -1, @@ -30,10 +28,8 @@ module.exports = { allowFreeText: false, systemDefined: true, visibleToAuthors: true, - userPermissions: { - canChange: false, - canDelete: false, - }, + canChange: false, + canDelete: false, }, { id: 1, @@ -44,10 +40,8 @@ module.exports = { allowFreeText: false, systemDefined: false, visibleToAuthors: true, - userPermissions: { - canChange: true, - canDelete: true, - }, + canChange: true, + canDelete: true, }, { id: 2, @@ -58,10 +52,8 @@ module.exports = { allowFreeText: false, systemDefined: false, visibleToAuthors: true, - userPermissions: { - canChange: true, - canDelete: true, - }, + canChange: true, + canDelete: true, }, ], }; diff --git a/src/taxonomy/data/types.mjs b/src/taxonomy/data/types.mjs index 72c165fdc3..82f1fb427a 100644 --- a/src/taxonomy/data/types.mjs +++ b/src/taxonomy/data/types.mjs @@ -1,11 +1,5 @@ // @ts-check -/** - * @typedef {Object} UserPermissions - * @property {boolean} canChange - * @property {boolean} canDelete - **/ - /** * @typedef {Object} TaxonomyData * @property {number} id @@ -18,7 +12,8 @@ * @property {boolean} visibleToAuthors * @property {number} tagsCount * @property {string[]} orgs - * @property {UserPermissions} userPermissions + * @property {boolean} canChange + * @property {boolean} canDelete */ /** diff --git a/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx b/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx index 3439b9b36a..eb315717e5 100644 --- a/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx +++ b/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx @@ -16,10 +16,8 @@ const data = { id: taxonomyId, name: 'Taxonomy 1', description: 'This is a description', - userPermissions: { - canChange: true, - canDelete: true, - }, + canChange: true, + canDelete: true, }; const queryClient = new QueryClient(); @@ -44,10 +42,8 @@ TaxonomyCardComponent.propTypes = { systemDefined: PropTypes.bool, orgsCount: PropTypes.number, onDeleteTaxonomy: PropTypes.func, - userPermissions: PropTypes.shape({ - canChange: PropTypes.bool, - canDelete: PropTypes.bool, - }), + canChange: PropTypes.bool, + canDelete: PropTypes.bool, }).isRequired, }; diff --git a/src/taxonomy/taxonomy-card/index.jsx b/src/taxonomy/taxonomy-card/index.jsx index d9c5e58ea5..cc64d05c86 100644 --- a/src/taxonomy/taxonomy-card/index.jsx +++ b/src/taxonomy/taxonomy-card/index.jsx @@ -149,10 +149,8 @@ TaxonomyCard.propTypes = { systemDefined: PropTypes.bool, orgsCount: PropTypes.number, tagsCount: PropTypes.number, - userPermissions: PropTypes.shape({ - canChange: PropTypes.bool.isRequired, - canDelete: PropTypes.bool.isRequired, - }).isRequired, + canChange: PropTypes.bool, + canDelete: PropTypes.bool, }).isRequired, }; diff --git a/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx b/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx index 4432a91f78..fc57a9f601 100644 --- a/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx +++ b/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx @@ -89,10 +89,8 @@ describe('', async () => { name: 'Test taxonomy', description: 'This is a description', systemDefined: true, - userPermissions: { - canChange: true, - canDelete: true, - }, + canChange: true, + canDelete: true, }, }); const { getByTestId, queryByTestId } = render(); @@ -148,7 +146,6 @@ describe('', async () => { name: 'Test taxonomy', description: 'This is a description', systemDefined: false, - userPermissions: {}, }, }); const { queryByText } = render(); diff --git a/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx b/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx index 2f66e2a2e0..12fbe0dc39 100644 --- a/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx +++ b/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx @@ -59,7 +59,7 @@ const TaxonomyMenu = ({ import: { title: intl.formatMessage(messages.importMenu), action: () => importTaxonomyTags(taxonomy.id, intl), - show: taxonomy.userPermissions.canChange, + show: taxonomy.canChange, }, export: { title: intl.formatMessage(messages.exportMenu), @@ -69,7 +69,7 @@ const TaxonomyMenu = ({ delete: { title: intl.formatMessage(messages.deleteMenu), action: deleteDialogOpen, - show: taxonomy.userPermissions.canDelete, + show: taxonomy.canDelete, }, }; @@ -136,10 +136,8 @@ TaxonomyMenu.propTypes = { id: PropTypes.number.isRequired, name: PropTypes.string.isRequired, tagsCount: PropTypes.number.isRequired, - userPermissions: PropTypes.shape({ - canChange: PropTypes.bool.isRequired, - canDelete: PropTypes.bool.isRequired, - }).isRequired, + canChange: PropTypes.bool, + canDelete: PropTypes.bool, }).isRequired, iconMenu: PropTypes.bool, }; diff --git a/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx b/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx index f30201ed40..20e172ae62 100644 --- a/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx +++ b/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx @@ -50,9 +50,8 @@ const TaxonomyMenuComponent = ({ id: taxonomyId, name: taxonomyName, tagsCount: 0, - userPermissions: { - canChange, canDelete, - }, + canChange, + canDelete, }} iconMenu={iconMenu} /> From 93b0ad0838516d2c9c8b850bd8a76dff8821a784 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 12 Jan 2024 02:08:25 +1030 Subject: [PATCH 3/7] feat: Content Tags Drawer uses permissions returned by the content tagging REST API to decide what actions to offer the user. These object-specific permissions replace the global "editable" field, which has been removed from the REST API. --- src/content-tags-drawer/ContentTagsCollapsible.jsx | 11 +++++------ .../ContentTagsCollapsible.test.jsx | 9 ++++----- .../ContentTagsCollapsibleHelper.jsx | 5 +++-- src/content-tags-drawer/ContentTagsDrawer.jsx | 3 +-- src/content-tags-drawer/ContentTagsDrawer.test.jsx | 4 ++-- src/content-tags-drawer/ContentTagsTree.jsx | 7 +++---- src/content-tags-drawer/ContentTagsTree.test.jsx | 12 ++++++++---- src/content-tags-drawer/TagBubble.jsx | 10 +++++----- src/content-tags-drawer/TagBubble.test.jsx | 12 ++++++------ .../__mocks__/contentTaxonomyTagsMock.js | 4 ++-- .../__mocks__/updateContentTaxonomyTagsMock.js | 2 +- src/content-tags-drawer/data/api.js | 6 +++--- src/content-tags-drawer/data/api.test.js | 4 ++-- src/content-tags-drawer/data/types.mjs | 2 +- src/taxonomy/data/api.js | 4 ++-- src/taxonomy/data/types.mjs | 1 + 16 files changed, 49 insertions(+), 47 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx index 574e1617fb..470b38f9ce 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx @@ -99,11 +99,10 @@ import useContentTagsCollapsibleHelper from './ContentTagsCollapsibleHelper'; * @param {Object} props - The component props. * @param {string} props.contentId - Id of the content object * @param {TaxonomyData & {contentTags: ContentTagData[]}} props.taxonomyAndTagsData - Taxonomy metadata & applied tags - * @param {boolean} props.editable - Whether the tags can be edited */ -const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) => { +const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { const intl = useIntl(); - const { id, name } = taxonomyAndTagsData; + const { id, name, canTagObject } = taxonomyAndTagsData; const { tagChangeHandler, tagsTree, contentTagsCount, checkedTags, @@ -141,12 +140,12 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) =>
- +
- {editable && ( + {canTagObject && (
@@ -73,10 +72,10 @@ ContentTagsTree.propTypes = { PropTypes.shape({ explicit: PropTypes.bool.isRequired, children: PropTypes.shape({}).isRequired, + canDelete: PropTypes.bool.isRequired, }).isRequired, ).isRequired, removeTagHandler: PropTypes.func.isRequired, - editable: PropTypes.bool.isRequired, }; export default ContentTagsTree; diff --git a/src/content-tags-drawer/ContentTagsTree.test.jsx b/src/content-tags-drawer/ContentTagsTree.test.jsx index ac41f3c1f1..b42d08f482 100644 --- a/src/content-tags-drawer/ContentTagsTree.test.jsx +++ b/src/content-tags-drawer/ContentTagsTree.test.jsx @@ -15,8 +15,10 @@ const data = { 'DNA Sequencing': { explicit: true, children: {}, + canDelete: true, }, }, + canDelete: false, }, 'Molecular, Cellular, and Microbiology': { explicit: false, @@ -24,16 +26,18 @@ const data = { Virology: { explicit: true, children: {}, + canDelete: true, }, }, + canDelete: false, }, }, }, }; -const ContentTagsTreeComponent = ({ tagsTree, removeTagHandler, editable }) => ( +const ContentTagsTreeComponent = ({ tagsTree, removeTagHandler }) => ( - + ); @@ -42,16 +46,16 @@ ContentTagsTreeComponent.propTypes = { PropTypes.shape({ explicit: PropTypes.bool.isRequired, children: PropTypes.shape({}).isRequired, + canDelete: PropTypes.bool.isRequired, }).isRequired, ).isRequired, removeTagHandler: PropTypes.func.isRequired, - editable: PropTypes.bool.isRequired, }; describe('', () => { it('should render taxonomy tags data along content tags number badge', async () => { await act(async () => { - const { getByText } = render( {}} editable />); + const { getByText } = render( {}} />); expect(getByText('Science and Research')).toBeInTheDocument(); expect(getByText('Genetics Subcategory')).toBeInTheDocument(); expect(getByText('Molecular, Cellular, and Microbiology')).toBeInTheDocument(); diff --git a/src/content-tags-drawer/TagBubble.jsx b/src/content-tags-drawer/TagBubble.jsx index 2287e48ab3..2eb1583444 100644 --- a/src/content-tags-drawer/TagBubble.jsx +++ b/src/content-tags-drawer/TagBubble.jsx @@ -8,15 +8,15 @@ import PropTypes from 'prop-types'; import TagOutlineIcon from './TagOutlineIcon'; const TagBubble = ({ - value, implicit, level, lineage, removeTagHandler, editable, + value, implicit, level, lineage, removeTagHandler, canDelete, }) => { const className = `tag-bubble mb-2 border-light-300 ${implicit ? 'implicit' : ''}`; const handleClick = React.useCallback(() => { - if (!implicit && editable) { + if (!implicit && canDelete) { removeTagHandler(lineage.join(','), false); } - }, [implicit, lineage, editable, removeTagHandler]); + }, [implicit, lineage, canDelete, removeTagHandler]); return (
@@ -24,7 +24,7 @@ const TagBubble = ({ className={className} variant="light" iconBefore={!implicit ? Tag : TagOutlineIcon} - iconAfter={!implicit && editable ? Close : null} + iconAfter={!implicit && canDelete ? Close : null} onIconAfterClick={handleClick} > {value} @@ -44,7 +44,7 @@ TagBubble.propTypes = { level: PropTypes.number, lineage: PropTypes.arrayOf(PropTypes.string).isRequired, removeTagHandler: PropTypes.func.isRequired, - editable: PropTypes.bool.isRequired, + canDelete: PropTypes.bool.isRequired, }; export default TagBubble; diff --git a/src/content-tags-drawer/TagBubble.test.jsx b/src/content-tags-drawer/TagBubble.test.jsx index e03fe18726..e15853e842 100644 --- a/src/content-tags-drawer/TagBubble.test.jsx +++ b/src/content-tags-drawer/TagBubble.test.jsx @@ -12,7 +12,7 @@ const data = { }; const TagBubbleComponent = ({ - value, implicit, level, lineage, removeTagHandler, editable, + value, implicit, level, lineage, removeTagHandler, canDelete, }) => ( ); @@ -37,7 +37,7 @@ TagBubbleComponent.propTypes = { level: PropTypes.number, lineage: PropTypes.arrayOf(PropTypes.string).isRequired, removeTagHandler: PropTypes.func.isRequired, - editable: PropTypes.bool.isRequired, + canDelete: PropTypes.bool.isRequired, }; describe('', () => { @@ -45,7 +45,7 @@ describe('', () => { const { container, getByText } = render( , @@ -63,7 +63,7 @@ describe('', () => { const { container, getByText } = render( ', () => { const { container } = render( getConfig().STUDIO_BASE_URL; * @returns {string} the URL */ export const getTaxonomyTagsApiUrl = (taxonomyId, options = {}) => { - const url = new URL(`api/content_tagging/v1/taxonomies/${taxonomyId}/tags/`, getApiBaseUrl()); + const url = new URL(`api/content_tagging/v1/taxonomies/${taxonomyId}/tags/?include_perms`, getApiBaseUrl()); if (options.parentTag) { url.searchParams.append('parent_tag', options.parentTag); } @@ -28,7 +28,7 @@ export const getTaxonomyTagsApiUrl = (taxonomyId, options = {}) => { return url.href; }; -export const getContentTaxonomyTagsApiUrl = (contentId) => new URL(`api/content_tagging/v1/object_tags/${contentId}/`, getApiBaseUrl()).href; +export const getContentTaxonomyTagsApiUrl = (contentId) => new URL(`api/content_tagging/v1/object_tags/${contentId}/?include_perms`, getApiBaseUrl()).href; export const getXBlockContentDataApiURL = (contentId) => new URL(`/xblock/outline/${contentId}`, getApiBaseUrl()).href; export const getLibraryContentDataApiUrl = (contentId) => new URL(`/api/libraries/v2/blocks/${contentId}/`, getApiBaseUrl()).href; @@ -76,7 +76,7 @@ export async function getContentData(contentId) { */ export async function updateContentTaxonomyTags(contentId, taxonomyId, tags) { let url = getContentTaxonomyTagsApiUrl(contentId); - url = `${url}?taxonomy=${taxonomyId}`; + url = `${url}&taxonomy=${taxonomyId}`; const { data } = await getAuthenticatedHttpClient().put(url, { tags }); return camelCaseObject(data[contentId]); } diff --git a/src/content-tags-drawer/data/api.test.js b/src/content-tags-drawer/data/api.test.js index d183f59ea2..229e6daee1 100644 --- a/src/content-tags-drawer/data/api.test.js +++ b/src/content-tags-drawer/data/api.test.js @@ -110,10 +110,10 @@ describe('content tags drawer api calls', () => { const contentId = 'block-v1:SampleTaxonomyOrg1+STC1+2023_1+type@vertical+block@aaf8b8eb86b54281aeeab12499d2cb0b'; const taxonomyId = 3; const tags = ['flat taxonomy tag 100', 'flat taxonomy tag 3856']; - axiosMock.onPut(`${getContentTaxonomyTagsApiUrl(contentId)}?taxonomy=${taxonomyId}`).reply(200, updateContentTaxonomyTagsMock); + axiosMock.onPut(`${getContentTaxonomyTagsApiUrl(contentId)}&taxonomy=${taxonomyId}`).reply(200, updateContentTaxonomyTagsMock); const result = await updateContentTaxonomyTags(contentId, taxonomyId, tags); - expect(axiosMock.history.put[0].url).toEqual(`${getContentTaxonomyTagsApiUrl(contentId)}?taxonomy=${taxonomyId}`); + expect(axiosMock.history.put[0].url).toEqual(`${getContentTaxonomyTagsApiUrl(contentId)}&taxonomy=${taxonomyId}`); expect(result).toEqual(updateContentTaxonomyTagsMock[contentId]); }); }); diff --git a/src/content-tags-drawer/data/types.mjs b/src/content-tags-drawer/data/types.mjs index 2145f41b95..9e249f7ade 100644 --- a/src/content-tags-drawer/data/types.mjs +++ b/src/content-tags-drawer/data/types.mjs @@ -10,7 +10,7 @@ * @typedef {Object} ContentTaxonomyTagData A list of the tags from one taxonomy that are applied to a content object. * @property {string} name * @property {number} taxonomyId - * @property {boolean} editable + * @property {boolean} canTagObject * @property {Tag[]} tags */ diff --git a/src/taxonomy/data/api.js b/src/taxonomy/data/api.js index c4a3a7d653..aa282dcb09 100644 --- a/src/taxonomy/data/api.js +++ b/src/taxonomy/data/api.js @@ -5,7 +5,7 @@ import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; const getApiBaseUrl = () => getConfig().STUDIO_BASE_URL; export const getTaxonomyListApiUrl = (org) => { - const url = new URL('api/content_tagging/v1/taxonomies/', getApiBaseUrl()); + const url = new URL('api/content_tagging/v1/taxonomies/?include_perms', getApiBaseUrl()); url.searchParams.append('enabled', 'true'); if (org !== undefined) { if (org === 'Unassigned') { @@ -32,7 +32,7 @@ export const getTaxonomyTemplateApiUrl = (format) => new URL( * @param {number} pk * @returns {string} */ -export const getTaxonomyApiUrl = (pk) => new URL(`api/content_tagging/v1/taxonomies/${pk}/`, getApiBaseUrl()).href; +export const getTaxonomyApiUrl = (pk) => new URL(`api/content_tagging/v1/taxonomies/${pk}/?include_perms`, getApiBaseUrl()).href; /** * Get list of taxonomies. diff --git a/src/taxonomy/data/types.mjs b/src/taxonomy/data/types.mjs index 6268b882e0..2e606712a1 100644 --- a/src/taxonomy/data/types.mjs +++ b/src/taxonomy/data/types.mjs @@ -15,6 +15,7 @@ * @property {boolean} allOrgs * @property {boolean} canChange * @property {boolean} canDelete + * @property {boolean} canTagObject */ /** From 3b97b181b30d44967ef5b0d3b5683c7d1862a8e8 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Fri, 12 Jan 2024 12:50:08 +1030 Subject: [PATCH 4/7] fix: renames the `can` fields to `can` to alleviate confusion about what permissions they reflect. Also fixes some properties and wraps menu clicks in act() to remove errors from the tests. --- .../ContentTagsCollapsible.test.jsx | 16 +- .../ContentTagsCollapsibleHelper.jsx | 10 +- .../ContentTagsDrawer.test.jsx | 5 + src/content-tags-drawer/ContentTagsTree.jsx | 4 +- .../ContentTagsTree.test.jsx | 11 +- src/content-tags-drawer/TagBubble.jsx | 11 +- src/content-tags-drawer/TagBubble.test.jsx | 32 +++- src/content-tags-drawer/data/types.mjs | 2 + src/taxonomy/TaxonomyListPage.jsx | 10 +- src/taxonomy/TaxonomyListPage.test.jsx | 16 +- src/taxonomy/__mocks__/taxonomyListMock.js | 18 +-- src/taxonomy/data/types.mjs | 6 +- .../taxonomy-card/TaxonomyCard.test.jsx | 16 +- src/taxonomy/taxonomy-card/index.jsx | 4 +- .../TaxonomyDetailPage.test.jsx | 9 +- src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx | 11 +- .../taxonomy-menu/TaxonomyMenu.test.jsx | 138 +++++++++++++----- 17 files changed, 221 insertions(+), 98 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.test.jsx b/src/content-tags-drawer/ContentTagsCollapsible.test.jsx index bd660ab200..fe1d7e1290 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.test.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.test.jsx @@ -22,6 +22,7 @@ jest.mock('./data/apiHooks', () => ({ tagPages: [{ isLoading: true, isError: false, + canAddTag: false, data: [], }], })), @@ -32,21 +33,24 @@ const data = { taxonomyAndTagsData: { id: 123, name: 'Taxonomy 1', + canTagObject: true, contentTags: [ { value: 'Tag 1', lineage: ['Tag 1'], + canDeleteObjecttag: true, }, { value: 'Tag 1.1', lineage: ['Tag 1', 'Tag 1.1'], + canDeleteObjecttag: true, }, { value: 'Tag 2', lineage: ['Tag 2'], + canDeleteObjecttag: true, }, ], - canTagObject: true, }, }; @@ -61,11 +65,12 @@ ContentTagsCollapsibleComponent.propTypes = { taxonomyAndTagsData: PropTypes.shape({ id: PropTypes.number, name: PropTypes.string, + canTagObject: PropTypes.bool.isRequired, contentTags: PropTypes.arrayOf(PropTypes.shape({ value: PropTypes.string, lineage: PropTypes.arrayOf(PropTypes.string), + canDeleteObjecttag: PropTypes.bool.isRequired, })), - canTagObjects: PropTypes.bool.isRequired, }).isRequired, }; @@ -92,6 +97,7 @@ describe('', () => { function setupTaxonomyMock() { useTaxonomyTagsData.mockReturnValue({ hasMorePages: false, + canAddTag: false, tagPages: [{ isLoading: false, isError: false, @@ -103,6 +109,8 @@ describe('', () => { parentValue: null, id: 12345, subTagsUrl: null, + canChangeTag: false, + canDeleteTag: false, }, { value: 'Tag 2', externalId: null, @@ -111,6 +119,8 @@ describe('', () => { parentValue: null, id: 12346, subTagsUrl: null, + canChangeTag: false, + canDeleteTag: false, }, { value: 'Tag 3', externalId: null, @@ -119,6 +129,8 @@ describe('', () => { parentValue: null, id: 12347, subTagsUrl: null, + canChangeTag: false, + canDeleteTag: false, }], }], }); diff --git a/src/content-tags-drawer/ContentTagsCollapsibleHelper.jsx b/src/content-tags-drawer/ContentTagsCollapsibleHelper.jsx index fb6db9b176..677fe806f0 100644 --- a/src/content-tags-drawer/ContentTagsCollapsibleHelper.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsibleHelper.jsx @@ -128,7 +128,8 @@ const useContentTagsCollapsibleHelper = (contentId, taxonomyAndTagsData) => { currentLevel[key] = { explicit: isExplicit, children: {}, - canDelete: item.canDelete, + canChangeObjecttag: item.canChangeObjecttag, + canDeleteObjecttag: item.canDeleteObjecttag, }; // Populating the SelectableBox with "selected" (explicit) tags @@ -163,7 +164,12 @@ const useContentTagsCollapsibleHelper = (contentId, taxonomyAndTagsData) => { const isExplicit = selectedTag === tag; if (!traversal[tag]) { - traversal[tag] = { explicit: isExplicit, children: {} }; + traversal[tag] = { + explicit: isExplicit, + children: {}, + canChangeObjecttag: false, + canDeleteObjecttag: false, + }; } else { traversal[tag].explicit = isExplicit; } diff --git a/src/content-tags-drawer/ContentTagsDrawer.test.jsx b/src/content-tags-drawer/ContentTagsDrawer.test.jsx index 1d3df425b7..b8fe58c3b8 100644 --- a/src/content-tags-drawer/ContentTagsDrawer.test.jsx +++ b/src/content-tags-drawer/ContentTagsDrawer.test.jsx @@ -91,10 +91,12 @@ describe('', () => { { value: 'Tag 1', lineage: ['Tag 1'], + canDeleteObjecttag: true, }, { value: 'Tag 2', lineage: ['Tag 2'], + canDeleteObjecttag: true, }, ], }, @@ -106,6 +108,7 @@ describe('', () => { { value: 'Tag 3', lineage: ['Tag 3'], + canDeleteObjecttag: true, }, ], }, @@ -117,10 +120,12 @@ describe('', () => { id: 123, name: 'Taxonomy 1', description: 'This is a description 1', + canTagObject: false, }, { id: 124, name: 'Taxonomy 2', description: 'This is a description 2', + canTagObject: false, }], }); await act(async () => { diff --git a/src/content-tags-drawer/ContentTagsTree.jsx b/src/content-tags-drawer/ContentTagsTree.jsx index c011f2c59d..40695a2f96 100644 --- a/src/content-tags-drawer/ContentTagsTree.jsx +++ b/src/content-tags-drawer/ContentTagsTree.jsx @@ -55,7 +55,7 @@ const ContentTagsTree = ({ tagsTree, removeTagHandler }) => { level={level} lineage={updatedLineage} removeTagHandler={removeTagHandler} - canDelete={tag[key].canDelete} + canRemove={tag[key].canDeleteObjecttag} /> { renderTagsTree(tag[key].children, level + 1, updatedLineage) }
@@ -72,7 +72,7 @@ ContentTagsTree.propTypes = { PropTypes.shape({ explicit: PropTypes.bool.isRequired, children: PropTypes.shape({}).isRequired, - canDelete: PropTypes.bool.isRequired, + canDeleteObjecttag: PropTypes.bool.isRequired, }).isRequired, ).isRequired, removeTagHandler: PropTypes.func.isRequired, diff --git a/src/content-tags-drawer/ContentTagsTree.test.jsx b/src/content-tags-drawer/ContentTagsTree.test.jsx index b42d08f482..f45f06604c 100644 --- a/src/content-tags-drawer/ContentTagsTree.test.jsx +++ b/src/content-tags-drawer/ContentTagsTree.test.jsx @@ -8,6 +8,7 @@ import ContentTagsTree from './ContentTagsTree'; const data = { 'Science and Research': { explicit: false, + canDeleteObjecttag: false, children: { 'Genetics Subcategory': { explicit: false, @@ -15,10 +16,10 @@ const data = { 'DNA Sequencing': { explicit: true, children: {}, - canDelete: true, + canDeleteObjecttag: true, }, }, - canDelete: false, + canDeleteObjecttag: false, }, 'Molecular, Cellular, and Microbiology': { explicit: false, @@ -26,10 +27,10 @@ const data = { Virology: { explicit: true, children: {}, - canDelete: true, + canDeleteObjecttag: true, }, }, - canDelete: false, + canDeleteObjecttag: false, }, }, }, @@ -46,7 +47,7 @@ ContentTagsTreeComponent.propTypes = { PropTypes.shape({ explicit: PropTypes.bool.isRequired, children: PropTypes.shape({}).isRequired, - canDelete: PropTypes.bool.isRequired, + canDeleteObjecttag: PropTypes.bool.isRequired, }).isRequired, ).isRequired, removeTagHandler: PropTypes.func.isRequired, diff --git a/src/content-tags-drawer/TagBubble.jsx b/src/content-tags-drawer/TagBubble.jsx index 2eb1583444..aa5c5d60ab 100644 --- a/src/content-tags-drawer/TagBubble.jsx +++ b/src/content-tags-drawer/TagBubble.jsx @@ -8,15 +8,15 @@ import PropTypes from 'prop-types'; import TagOutlineIcon from './TagOutlineIcon'; const TagBubble = ({ - value, implicit, level, lineage, removeTagHandler, canDelete, + value, implicit, level, lineage, removeTagHandler, canRemove, }) => { const className = `tag-bubble mb-2 border-light-300 ${implicit ? 'implicit' : ''}`; const handleClick = React.useCallback(() => { - if (!implicit && canDelete) { + if (!implicit && canRemove) { removeTagHandler(lineage.join(','), false); } - }, [implicit, lineage, canDelete, removeTagHandler]); + }, [implicit, lineage, canRemove, removeTagHandler]); return (
@@ -24,7 +24,7 @@ const TagBubble = ({ className={className} variant="light" iconBefore={!implicit ? Tag : TagOutlineIcon} - iconAfter={!implicit && canDelete ? Close : null} + iconAfter={!implicit && canRemove ? Close : null} onIconAfterClick={handleClick} > {value} @@ -36,6 +36,7 @@ const TagBubble = ({ TagBubble.defaultProps = { implicit: true, level: 0, + canRemove: false, }; TagBubble.propTypes = { @@ -44,7 +45,7 @@ TagBubble.propTypes = { level: PropTypes.number, lineage: PropTypes.arrayOf(PropTypes.string).isRequired, removeTagHandler: PropTypes.func.isRequired, - canDelete: PropTypes.bool.isRequired, + canRemove: PropTypes.bool, }; export default TagBubble; diff --git a/src/content-tags-drawer/TagBubble.test.jsx b/src/content-tags-drawer/TagBubble.test.jsx index e15853e842..0cf7961613 100644 --- a/src/content-tags-drawer/TagBubble.test.jsx +++ b/src/content-tags-drawer/TagBubble.test.jsx @@ -12,7 +12,7 @@ const data = { }; const TagBubbleComponent = ({ - value, implicit, level, lineage, removeTagHandler, canDelete, + value, implicit, level, lineage, removeTagHandler, canRemove, }) => ( ); @@ -29,6 +29,7 @@ const TagBubbleComponent = ({ TagBubbleComponent.defaultProps = { implicit: true, level: 0, + canRemove: false, }; TagBubbleComponent.propTypes = { @@ -37,7 +38,7 @@ TagBubbleComponent.propTypes = { level: PropTypes.number, lineage: PropTypes.arrayOf(PropTypes.string).isRequired, removeTagHandler: PropTypes.func.isRequired, - canDelete: PropTypes.bool.isRequired, + canRemove: PropTypes.bool, }; describe('', () => { @@ -45,7 +46,6 @@ describe('', () => { const { container, getByText } = render( , @@ -58,12 +58,13 @@ describe('', () => { it('should render explicit tag', () => { const tagBubbleData = { implicit: false, + canRemove: true, ...data, }; const { container, getByText } = render( ', () => { it('should call removeTagHandler when "x" clicked on explicit tag', async () => { const tagBubbleData = { implicit: false, + canRemove: true, ...data, }; const { container } = render( ', () => { fireEvent.click(xButton); expect(data.removeTagHandler).toHaveBeenCalled(); }); + + it('should not show "x" when canRemove is not allowed', async () => { + const tagBubbleData = { + implicit: false, + canRemove: false, + ...data, + }; + const { container } = render( + , + ); + + expect(container.getElementsByClassName('pgn__chip__icon-after')[0]).toBeUndefined(); + }); }); diff --git a/src/content-tags-drawer/data/types.mjs b/src/content-tags-drawer/data/types.mjs index 9e249f7ade..3fad73ccf1 100644 --- a/src/content-tags-drawer/data/types.mjs +++ b/src/content-tags-drawer/data/types.mjs @@ -4,6 +4,8 @@ * @typedef {Object} Tag A tag that has been applied to some content. * @property {string} value The value of the tag, also its ID. e.g. "Biology" * @property {string[]} lineage The values of the tag and its parent(s) in the hierarchy + * @property {boolean} canChangeObjecttag + * @property {boolean} canDeleteObjecttag */ /** diff --git a/src/taxonomy/TaxonomyListPage.jsx b/src/taxonomy/TaxonomyListPage.jsx index 01d81a5294..0ebdaeff99 100644 --- a/src/taxonomy/TaxonomyListPage.jsx +++ b/src/taxonomy/TaxonomyListPage.jsx @@ -32,7 +32,7 @@ import TaxonomyCard from './taxonomy-card'; const ALL_TAXONOMIES = 'All taxonomies'; const UNASSIGNED = 'Unassigned'; -const TaxonomyListHeaderButtons = ({ canAdd }) => { +const TaxonomyListHeaderButtons = ({ canAddTaxonomy }) => { const intl = useIntl(); return ( <> @@ -71,7 +71,7 @@ const TaxonomyListHeaderButtons = ({ canAdd }) => { iconBefore={Add} onClick={() => importTaxonomy(intl)} data-testid="taxonomy-import-button" - disabled={!canAdd} + disabled={!canAddTaxonomy} > {intl.formatMessage(messages.importButtonLabel)} @@ -158,7 +158,7 @@ const TaxonomyListPage = () => { return { taxonomyListData, isLoaded }; }; const { taxonomyListData, isLoaded } = useTaxonomyListData(); - const canAdd = isLoaded ? taxonomyListData.canAdd : false; + const canAddTaxonomy = isLoaded ? taxonomyListData.canAddTaxonomy : false; const getOrgSelect = () => ( // Initialize organization select component @@ -180,7 +180,7 @@ const TaxonomyListPage = () => { } + headerActions={} hideBorder /> @@ -236,7 +236,7 @@ const TaxonomyListPage = () => { }; TaxonomyListHeaderButtons.propTypes = { - canAdd: PropTypes.bool.isRequired, + canAddTaxonomy: PropTypes.bool.isRequired, }; OrganizationFilterSelector.propTypes = { diff --git a/src/taxonomy/TaxonomyListPage.test.jsx b/src/taxonomy/TaxonomyListPage.test.jsx index b9aa42c8c8..4819fca125 100644 --- a/src/taxonomy/TaxonomyListPage.test.jsx +++ b/src/taxonomy/TaxonomyListPage.test.jsx @@ -22,9 +22,9 @@ const taxonomies = [{ name: 'Taxonomy', description: 'This is a description', showSystemBadge: false, + canChangeTaxonomy: true, + canDeleteTaxonomy: true, tagsCount: 0, - canChange: true, - canDelete: true, }]; const organizationsListUrl = 'http://localhost:18010/organizations'; const organizations = ['Org 1', 'Org 2']; @@ -95,6 +95,7 @@ describe('', () => { useIsTaxonomyListDataLoaded.mockReturnValue(true); useTaxonomyListDataResponse.mockReturnValue({ results: taxonomies, + canAddTaxonomy: false, }); await act(async () => { const { getByTestId } = render(); @@ -106,6 +107,7 @@ describe('', () => { useIsTaxonomyListDataLoaded.mockReturnValue(true); useTaxonomyListDataResponse.mockReturnValue({ results: taxonomies, + canAddTaxonomy: false, }); const { findByRole } = render(); const templateMenu = await findByRole('button', { name: 'Download template' }); @@ -120,7 +122,7 @@ describe('', () => { useIsTaxonomyListDataLoaded.mockReturnValue(true); useTaxonomyListDataResponse.mockReturnValue({ results: [], - canAdd: false, + canAddTaxonomy: false, }); const { getByRole } = render(); @@ -132,7 +134,7 @@ describe('', () => { useIsTaxonomyListDataLoaded.mockReturnValue(true); useTaxonomyListDataResponse.mockReturnValue({ results: [], - canAdd: true, + canAddTaxonomy: true, }); const { getByRole } = render(); @@ -149,7 +151,12 @@ describe('', () => { id: 1, name: 'Taxonomy', description: 'This is a description', + showSystemBadge: false, + canChangeTaxonomy: false, + canDeleteTaxonomy: false, + tagsCount: 0, }], + canAddTaxonomy: false, }); const { @@ -181,6 +188,7 @@ describe('', () => { useIsTaxonomyListDataLoaded.mockReturnValue(true); useTaxonomyListDataResponse.mockReturnValue({ results: taxonomies, + canAddTaxonomy: false, }); const { getByRole } = render(); diff --git a/src/taxonomy/__mocks__/taxonomyListMock.js b/src/taxonomy/__mocks__/taxonomyListMock.js index 5e96b064eb..0e7b24e4f7 100644 --- a/src/taxonomy/__mocks__/taxonomyListMock.js +++ b/src/taxonomy/__mocks__/taxonomyListMock.js @@ -5,7 +5,7 @@ module.exports = { numPages: 1, currentPage: 1, start: 0, - canAdd: true, + canAddTaxonomy: true, results: [ { id: -2, @@ -16,8 +16,8 @@ module.exports = { allowFreeText: false, systemDefined: true, visibleToAuthors: false, - canChange: false, - canDelete: false, + canChangeTaxonomy: false, + canDeleteTaxonomy: false, }, { id: -1, @@ -28,8 +28,8 @@ module.exports = { allowFreeText: false, systemDefined: true, visibleToAuthors: true, - canChange: false, - canDelete: false, + canChangeTaxonomy: false, + canDeleteTaxonomy: false, }, { id: 1, @@ -40,8 +40,8 @@ module.exports = { allowFreeText: false, systemDefined: false, visibleToAuthors: true, - canChange: true, - canDelete: true, + canChangeTaxonomy: true, + canDeleteTaxonomy: true, }, { id: 2, @@ -52,8 +52,8 @@ module.exports = { allowFreeText: false, systemDefined: false, visibleToAuthors: true, - canChange: true, - canDelete: true, + canChangeTaxonomy: true, + canDeleteTaxonomy: true, }, ], }; diff --git a/src/taxonomy/data/types.mjs b/src/taxonomy/data/types.mjs index 2e606712a1..6609a342a2 100644 --- a/src/taxonomy/data/types.mjs +++ b/src/taxonomy/data/types.mjs @@ -13,8 +13,8 @@ * @property {number} tagsCount * @property {string[]} orgs * @property {boolean} allOrgs - * @property {boolean} canChange - * @property {boolean} canDelete + * @property {boolean} canChangeTaxonomy + * @property {boolean} canDeleteTaxonomy * @property {boolean} canTagObject */ @@ -27,6 +27,6 @@ * @property {number} currentPage * @property {number} start * @property {function} refetch - * @property {boolean} canAdd + * @property {boolean} canAddTaxonomy * @property {TaxonomyData[]} results */ diff --git a/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx b/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx index eb315717e5..06147b8158 100644 --- a/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx +++ b/src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx @@ -16,8 +16,10 @@ const data = { id: taxonomyId, name: 'Taxonomy 1', description: 'This is a description', - canChange: true, - canDelete: true, + systemDefined: false, + canChangeTaxonomy: true, + canDeleteTaxonomy: true, + tagsCount: 0, }; const queryClient = new QueryClient(); @@ -42,8 +44,8 @@ TaxonomyCardComponent.propTypes = { systemDefined: PropTypes.bool, orgsCount: PropTypes.number, onDeleteTaxonomy: PropTypes.func, - canChange: PropTypes.bool, - canDelete: PropTypes.bool, + canChangeTaxonomy: PropTypes.bool, + canDeleteTaxonomy: PropTypes.bool, }).isRequired, }; @@ -99,10 +101,8 @@ describe('', async () => { }); it('shows the system-defined badge with system taxonomies', () => { - const cardData = { - systemDefined: true, - ...data, - }; + const cardData = { ...data }; + cardData.systemDefined = true; const { getByText } = render(); expect(getByText('System-level')).toBeInTheDocument(); diff --git a/src/taxonomy/taxonomy-card/index.jsx b/src/taxonomy/taxonomy-card/index.jsx index cc64d05c86..87d6da9f17 100644 --- a/src/taxonomy/taxonomy-card/index.jsx +++ b/src/taxonomy/taxonomy-card/index.jsx @@ -149,8 +149,8 @@ TaxonomyCard.propTypes = { systemDefined: PropTypes.bool, orgsCount: PropTypes.number, tagsCount: PropTypes.number, - canChange: PropTypes.bool, - canDelete: PropTypes.bool, + canChangeTaxonomy: PropTypes.bool, + canDeleteTaxonomy: PropTypes.bool, }).isRequired, }; diff --git a/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx b/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx index 30bc5324c4..aad601a0f1 100644 --- a/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx +++ b/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx @@ -86,8 +86,9 @@ describe('', () => { name: 'Test taxonomy', description: 'This is a description', system_defined: false, - canChange: true, - canDelete: true, + can_change_taxonomy: true, + can_delete_taxonomy: true, + tagsCount: 0, }); const { getByTestId, queryByTestId, findByRole } = render(); @@ -124,6 +125,8 @@ describe('', () => { name: 'Test taxonomy', description: 'This is a description', system_defined: true, + can_change_taxonomy: false, + can_delete_taxonomy: false, }); const { findByRole, getByText } = render(); @@ -138,6 +141,8 @@ describe('', () => { name: 'Test taxonomy', description: 'This is a description', system_defined: false, + can_change_taxonomy: false, + can_delete_taxonomy: false, }); const { findByRole, queryByText } = render(); diff --git a/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx b/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx index 6033b618de..b0803ceb61 100644 --- a/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx +++ b/src/taxonomy/taxonomy-menu/TaxonomyMenu.jsx @@ -61,7 +61,7 @@ const TaxonomyMenu = ({ import: { title: intl.formatMessage(messages.importMenu), action: () => importTaxonomyTags(taxonomy.id, intl), - show: taxonomy.canChange, + show: taxonomy.canChangeTaxonomy && !taxonomy.systemDefined, }, export: { title: intl.formatMessage(messages.exportMenu), @@ -71,12 +71,12 @@ const TaxonomyMenu = ({ delete: { title: intl.formatMessage(messages.deleteMenu), action: deleteDialogOpen, - show: taxonomy.canDelete, + show: taxonomy.canDeleteTaxonomy && !taxonomy.systemDefined, }, manageOrgs: { title: intl.formatMessage(messages.manageOrgsMenu), action: manageOrgsModalOpen, - show: taxonomy.canChange, + show: taxonomy.canChangeTaxonomy, }, }; @@ -150,8 +150,9 @@ TaxonomyMenu.propTypes = { id: PropTypes.number.isRequired, name: PropTypes.string.isRequired, tagsCount: PropTypes.number.isRequired, - canChange: PropTypes.bool, - canDelete: PropTypes.bool, + systemDefined: PropTypes.bool.isRequired, + canChangeTaxonomy: PropTypes.bool.isRequired, + canDeleteTaxonomy: PropTypes.bool.isRequired, }).isRequired, iconMenu: PropTypes.bool, }; diff --git a/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx b/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx index 24237b5761..3aa930535a 100644 --- a/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx +++ b/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx @@ -3,7 +3,9 @@ import { IntlProvider } from '@edx/frontend-platform/i18n'; import { initializeMockApp } from '@edx/frontend-platform'; import { AppProvider } from '@edx/frontend-platform/react'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; -import { render, fireEvent, waitFor } from '@testing-library/react'; +import { + act, render, fireEvent, waitFor, +} from '@testing-library/react'; import PropTypes from 'prop-types'; import { TaxonomyContext } from '../common/context'; @@ -33,8 +35,9 @@ const mockSetToastMessage = jest.fn(); const TaxonomyMenuComponent = ({ iconMenu, - canChange, - canDelete, + systemDefined, + canChangeTaxonomy, + canDeleteTaxonomy, }) => { const context = useMemo(() => ({ toastMessage: null, @@ -51,8 +54,9 @@ const TaxonomyMenuComponent = ({ id: taxonomyId, name: taxonomyName, tagsCount: 0, - canChange, - canDelete, + systemDefined, + canChangeTaxonomy, + canDeleteTaxonomy, }} iconMenu={iconMenu} /> @@ -65,13 +69,15 @@ const TaxonomyMenuComponent = ({ TaxonomyMenuComponent.propTypes = { iconMenu: PropTypes.bool.isRequired, - canChange: PropTypes.bool, - canDelete: PropTypes.bool, + canChangeTaxonomy: PropTypes.bool, + systemDefined: PropTypes.bool, + canDeleteTaxonomy: PropTypes.bool, }; TaxonomyMenuComponent.defaultProps = { - canChange: true, - canDelete: true, + systemDefined: false, + canChangeTaxonomy: true, + canDeleteTaxonomy: true, }; describe.each([true, false])('', async (iconMenu) => { @@ -91,20 +97,24 @@ describe.each([true, false])('', async (iconMenu) => jest.clearAllMocks(); }); - test('should open and close menu on button click', () => { + test('should open and close menu on button click', async () => { const { getByTestId, queryByTestId } = render(); // Menu closed/doesn't exist yet expect(queryByTestId('taxonomy-menu')).not.toBeInTheDocument(); // Click on the menu button to open - fireEvent.click(getByTestId('taxonomy-menu-button')); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-button')); + }); // Menu opened expect(getByTestId('taxonomy-menu')).toBeVisible(); // Click on button again to close the menu - fireEvent.click(getByTestId('taxonomy-menu-button')); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-button')); + }); // Menu closed // Jest bug: toBeVisible() isn't checking opacity correctly @@ -115,17 +125,17 @@ describe.each([true, false])('', async (iconMenu) => expect(getByTestId('taxonomy-menu-button')).toBeVisible(); }); - test('Shows menu actions that user is permitted', () => { + test('Shows menu actions that user is permitted', async () => { const { getByTestId, queryByTestId } = render( , ); // Click on the menu button to open - fireEvent.click(getByTestId('taxonomy-menu-button')); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-button')); + }); // Menu opened expect(getByTestId('taxonomy-menu')).toBeVisible(); @@ -137,17 +147,20 @@ describe.each([true, false])('', async (iconMenu) => expect(queryByTestId('taxonomy-menu-delete')).toBeInTheDocument(); }); - test('Hides menu actions that user is not permitted', () => { + test('Hides menu actions that user is not permitted', async () => { const { getByTestId, queryByTestId } = render( , ); // Click on the menu button to open - fireEvent.click(getByTestId('taxonomy-menu-button')); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-button')); + }); // Ensure expected menu items are found expect(queryByTestId('taxonomy-menu-export')).toBeInTheDocument(); @@ -156,15 +169,40 @@ describe.each([true, false])('', async (iconMenu) => expect(queryByTestId('taxonomy-menu-delete')).not.toBeInTheDocument(); }); - test('should open export modal on export menu click', () => { + test('Hides import/delete actions for system-defined taxonomies', async () => { + const systemDefined = true; + const { getByTestId, queryByTestId } = render( + , + ); + + // Click on the menu button to open + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-button')); + }); + + // Ensure expected menu items are found + expect(queryByTestId('taxonomy-menu-export')).toBeInTheDocument(); + expect(queryByTestId('taxonomy-menu-import')).not.toBeInTheDocument(); + expect(queryByTestId('taxonomy-menu-manageOrgs')).toBeInTheDocument(); + expect(queryByTestId('taxonomy-menu-delete')).not.toBeInTheDocument(); + }); + + test('should open export modal on export menu click', async () => { const { getByTestId, getByText, queryByText } = render(); // Modal closed expect(queryByText('Select format to export')).not.toBeInTheDocument(); // Click on export menu - fireEvent.click(getByTestId('taxonomy-menu-button')); - fireEvent.click(getByTestId('taxonomy-menu-export')); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-button')); + }); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-export')); + }); // Modal opened expect(getByText('Select format to export')).toBeInTheDocument(); @@ -176,41 +214,57 @@ describe.each([true, false])('', async (iconMenu) => expect(queryByText('Select format to export')).not.toBeInTheDocument(); }); - test('should call import tags when menu click', () => { + test('should call import tags when menu click', async () => { const { getByTestId } = render(); // Click on import menu - fireEvent.click(getByTestId('taxonomy-menu-button')); - fireEvent.click(getByTestId('taxonomy-menu-import')); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-button')); + }); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-import')); + }); expect(importTaxonomyTags).toHaveBeenCalled(); }); - test('should export a taxonomy', () => { + test('should export a taxonomy', async () => { const { getByTestId, getByText, queryByText } = render(); // Click on export menu - fireEvent.click(getByTestId('taxonomy-menu-button')); - fireEvent.click(getByTestId('taxonomy-menu-export')); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-button')); + }); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-export')); + }); // Select JSON format and click on export - fireEvent.click(getByText('JSON file')); - fireEvent.click(getByTestId('export-button-1')); + await act(async () => { + fireEvent.click(getByText('JSON file')); + }); + await act(async () => { + fireEvent.click(getByTestId('export-button-1')); + }); // Modal closed expect(queryByText('Select format to export')).not.toBeInTheDocument(); expect(getTaxonomyExportFile).toHaveBeenCalledWith(taxonomyId, 'json'); }); - test('should open delete dialog on delete menu click', () => { + test('should open delete dialog on delete menu click', async () => { const { getByTestId, getByText, queryByText } = render(); // Modal closed expect(queryByText(`Delete "${taxonomyName}"`)).not.toBeInTheDocument(); // Click on delete menu - fireEvent.click(getByTestId('taxonomy-menu-button')); - fireEvent.click(getByTestId('taxonomy-menu-delete')); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-button')); + }); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-delete')); + }); // Modal opened expect(getByText(`Delete "${taxonomyName}"`)).toBeInTheDocument(); @@ -226,8 +280,12 @@ describe.each([true, false])('', async (iconMenu) => const { getByTestId, getByLabelText, queryByText } = render(); // Click on delete menu - fireEvent.click(getByTestId('taxonomy-menu-button')); - fireEvent.click(getByTestId('taxonomy-menu-delete')); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-button')); + }); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-delete')); + }); const deleteButton = getByTestId('delete-button'); @@ -274,8 +332,12 @@ describe.each([true, false])('', async (iconMenu) => expect(queryByText('Assign to organizations')).not.toBeInTheDocument(); // Click on delete menu - fireEvent.click(getByTestId('taxonomy-menu-button')); - fireEvent.click(getByTestId('taxonomy-menu-manageOrgs')); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-button')); + }); + await act(async () => { + fireEvent.click(getByTestId('taxonomy-menu-manageOrgs')); + }); // Modal opened expect(await findByText('Assign to organizations')).toBeInTheDocument(); From 5a1de307d518803fbef3d8e2d2aa8a1fe186d097 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Thu, 18 Jan 2024 13:44:16 +1030 Subject: [PATCH 5/7] fix: use await findByTestId instead of wrapping menu clicks in act() --- .../taxonomy-menu/TaxonomyMenu.test.jsx | 110 +++++++----------- 1 file changed, 40 insertions(+), 70 deletions(-) diff --git a/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx b/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx index c47100d393..fac7a0aeab 100644 --- a/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx +++ b/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx @@ -3,9 +3,7 @@ import { IntlProvider } from '@edx/frontend-platform/i18n'; import { initializeMockApp } from '@edx/frontend-platform'; import { AppProvider } from '@edx/frontend-platform/react'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; -import { - act, render, fireEvent, waitFor, -} from '@testing-library/react'; +import { render, fireEvent, waitFor } from '@testing-library/react'; import PropTypes from 'prop-types'; import { TaxonomyContext } from '../common/context'; @@ -93,23 +91,21 @@ describe.each([true, false])('', async (iconMenu) => }); test('should open and close menu on button click', async () => { - const { getByTestId, queryByTestId } = render(); + const { findByTestId, getByTestId, queryByTestId } = render( + , + ); // Menu closed/doesn't exist yet expect(queryByTestId('taxonomy-menu')).not.toBeInTheDocument(); // Click on the menu button to open - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-button')); - }); + fireEvent.click(await findByTestId('taxonomy-menu-button')); // Menu opened expect(getByTestId('taxonomy-menu')).toBeVisible(); // Click on button again to close the menu - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-button')); - }); + fireEvent.click(await findByTestId('taxonomy-menu-button')); // Menu closed // Jest bug: toBeVisible() isn't checking opacity correctly @@ -121,16 +117,14 @@ describe.each([true, false])('', async (iconMenu) => }); test('Shows menu actions that user is permitted', async () => { - const { getByTestId, queryByTestId } = render( + const { findByTestId, getByTestId, queryByTestId } = render( , ); // Click on the menu button to open - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-button')); - }); + fireEvent.click(await findByTestId('taxonomy-menu-button')); // Menu opened expect(getByTestId('taxonomy-menu')).toBeVisible(); @@ -143,7 +137,7 @@ describe.each([true, false])('', async (iconMenu) => }); test('Hides menu actions that user is not permitted', async () => { - const { getByTestId, queryByTestId } = render( + const { findByTestId, queryByTestId } = render( ', async (iconMenu) => ); // Click on the menu button to open - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-button')); - }); + fireEvent.click(await findByTestId('taxonomy-menu-button')); // Ensure expected menu items are found expect(queryByTestId('taxonomy-menu-export')).toBeInTheDocument(); @@ -166,7 +158,7 @@ describe.each([true, false])('', async (iconMenu) => test('Hides import/delete actions for system-defined taxonomies', async () => { const systemDefined = true; - const { getByTestId, queryByTestId } = render( + const { queryByTestId } = render( ', async (iconMenu) => ); // Click on the menu button to open - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-button')); - }); + fireEvent.click(queryByTestId('taxonomy-menu-button')); // Ensure expected menu items are found expect(queryByTestId('taxonomy-menu-export')).toBeInTheDocument(); @@ -186,18 +176,16 @@ describe.each([true, false])('', async (iconMenu) => }); test('should open export modal on export menu click', async () => { - const { getByTestId, getByText, queryByText } = render(); + const { findByTestId, getByText, queryByText } = render( + , + ); // Modal closed expect(queryByText('Select format to export')).not.toBeInTheDocument(); // Click on export menu - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-button')); - }); - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-export')); - }); + fireEvent.click(await findByTestId('taxonomy-menu-button')); + fireEvent.click(await findByTestId('taxonomy-menu-export')); // Modal opened expect(getByText('Select format to export')).toBeInTheDocument(); @@ -210,37 +198,27 @@ describe.each([true, false])('', async (iconMenu) => }); test('should call import tags when menu click', async () => { - const { getByTestId, getByText } = render(); + const { findByTestId, getByText } = render(); // Click on import menu - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-button')); - }); - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-import')); - }); + fireEvent.click(await findByTestId('taxonomy-menu-button')); + fireEvent.click(await findByTestId('taxonomy-menu-import')); expect(getByText('Update "Taxonomy 1"')).toBeInTheDocument(); }); test('should export a taxonomy', async () => { - const { getByTestId, getByText, queryByText } = render(); + const { + findByTestId, getByTestId, getByText, queryByText, + } = render(); // Click on export menu - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-button')); - }); - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-export')); - }); + fireEvent.click(await findByTestId('taxonomy-menu-button')); + fireEvent.click(await findByTestId('taxonomy-menu-export')); // Select JSON format and click on export - await act(async () => { - fireEvent.click(getByText('JSON file')); - }); - await act(async () => { - fireEvent.click(getByTestId('export-button-1')); - }); + fireEvent.click(getByText('JSON file')); + fireEvent.click(getByTestId('export-button-1')); // Modal closed expect(queryByText('Select format to export')).not.toBeInTheDocument(); @@ -248,18 +226,16 @@ describe.each([true, false])('', async (iconMenu) => }); test('should open delete dialog on delete menu click', async () => { - const { getByTestId, getByText, queryByText } = render(); + const { findByTestId, getByText, queryByText } = render( + , + ); // Modal closed expect(queryByText(`Delete "${taxonomyName}"`)).not.toBeInTheDocument(); // Click on delete menu - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-button')); - }); - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-delete')); - }); + fireEvent.click(await findByTestId('taxonomy-menu-button')); + fireEvent.click(await findByTestId('taxonomy-menu-delete')); // Modal opened expect(getByText(`Delete "${taxonomyName}"`)).toBeInTheDocument(); @@ -272,15 +248,13 @@ describe.each([true, false])('', async (iconMenu) => }); test('should delete a taxonomy', async () => { - const { getByTestId, getByLabelText, queryByText } = render(); + const { + getByTestId, getByLabelText, findByTestId, queryByText, + } = render(); // Click on delete menu - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-button')); - }); - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-delete')); - }); + fireEvent.click(await findByTestId('taxonomy-menu-button')); + fireEvent.click(await findByTestId('taxonomy-menu-delete')); const deleteButton = getByTestId('delete-button'); @@ -312,7 +286,7 @@ describe.each([true, false])('', async (iconMenu) => it('should open manage orgs dialog menu click', async () => { const { - findByText, getByTestId, getByText, queryByText, + findByTestId, findByText, getByText, queryByText, } = render(); // We need to provide a taxonomy or the modal will not open @@ -327,12 +301,8 @@ describe.each([true, false])('', async (iconMenu) => expect(queryByText('Assign to organizations')).not.toBeInTheDocument(); // Click on delete menu - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-button')); - }); - await act(async () => { - fireEvent.click(getByTestId('taxonomy-menu-manageOrgs')); - }); + fireEvent.click(await findByTestId('taxonomy-menu-button')); + fireEvent.click(await findByTestId('taxonomy-menu-manageOrgs')); // Modal opened expect(await findByText('Assign to organizations')).toBeInTheDocument(); From 1348604105622ebc7f46b856ea996c5e411a730a Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 23 Jan 2024 12:34:59 +1030 Subject: [PATCH 6/7] revert: removes include_perms from the tagging REST API calls Permissions are included in the response; no need to request them. --- src/content-tags-drawer/data/api.js | 4 ++-- src/taxonomy/data/api.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/content-tags-drawer/data/api.js b/src/content-tags-drawer/data/api.js index 5bcd86b886..86cbb192c2 100644 --- a/src/content-tags-drawer/data/api.js +++ b/src/content-tags-drawer/data/api.js @@ -11,7 +11,7 @@ const getApiBaseUrl = () => getConfig().STUDIO_BASE_URL; * @returns {string} the URL */ export const getTaxonomyTagsApiUrl = (taxonomyId, options = {}) => { - const url = new URL(`api/content_tagging/v1/taxonomies/${taxonomyId}/tags/?include_perms`, getApiBaseUrl()); + const url = new URL(`api/content_tagging/v1/taxonomies/${taxonomyId}/tags/`, getApiBaseUrl()); if (options.parentTag) { url.searchParams.append('parent_tag', options.parentTag); } @@ -28,7 +28,7 @@ export const getTaxonomyTagsApiUrl = (taxonomyId, options = {}) => { return url.href; }; -export const getContentTaxonomyTagsApiUrl = (contentId) => new URL(`api/content_tagging/v1/object_tags/${contentId}/?include_perms`, getApiBaseUrl()).href; +export const getContentTaxonomyTagsApiUrl = (contentId) => new URL(`api/content_tagging/v1/object_tags/${contentId}/`, getApiBaseUrl()).href; export const getXBlockContentDataApiURL = (contentId) => new URL(`/xblock/outline/${contentId}`, getApiBaseUrl()).href; export const getLibraryContentDataApiUrl = (contentId) => new URL(`/api/libraries/v2/blocks/${contentId}/`, getApiBaseUrl()).href; diff --git a/src/taxonomy/data/api.js b/src/taxonomy/data/api.js index aa282dcb09..c4a3a7d653 100644 --- a/src/taxonomy/data/api.js +++ b/src/taxonomy/data/api.js @@ -5,7 +5,7 @@ import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; const getApiBaseUrl = () => getConfig().STUDIO_BASE_URL; export const getTaxonomyListApiUrl = (org) => { - const url = new URL('api/content_tagging/v1/taxonomies/?include_perms', getApiBaseUrl()); + const url = new URL('api/content_tagging/v1/taxonomies/', getApiBaseUrl()); url.searchParams.append('enabled', 'true'); if (org !== undefined) { if (org === 'Unassigned') { @@ -32,7 +32,7 @@ export const getTaxonomyTemplateApiUrl = (format) => new URL( * @param {number} pk * @returns {string} */ -export const getTaxonomyApiUrl = (pk) => new URL(`api/content_tagging/v1/taxonomies/${pk}/?include_perms`, getApiBaseUrl()).href; +export const getTaxonomyApiUrl = (pk) => new URL(`api/content_tagging/v1/taxonomies/${pk}/`, getApiBaseUrl()).href; /** * Get list of taxonomies. From 0eb466160bd4dc1776210ddf048d467a237fa229 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Tue, 23 Jan 2024 19:13:29 +1030 Subject: [PATCH 7/7] refactor: use component PropTypes in tagging tests to avoid duplication. --- .../ContentTagsCollapsible.test.jsx | 15 +-------------- .../ContentTagsDropDownSelector.test.jsx | 14 +------------- src/content-tags-drawer/ContentTagsTree.test.jsx | 12 +----------- src/content-tags-drawer/TagBubble.test.jsx | 11 ++--------- src/taxonomy/taxonomy-card/TaxonomyCard.test.jsx | 14 +------------- .../taxonomy-detail/TaxonomyDetailPage.test.jsx | 11 ----------- .../taxonomy-detail/TaxonomyDetailSideCard.jsx | 8 ++++---- .../TaxonomyDetailSideCard.test.jsx | 8 +------- src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx | 11 ++++------- 9 files changed, 15 insertions(+), 89 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.test.jsx b/src/content-tags-drawer/ContentTagsCollapsible.test.jsx index fe1d7e1290..09a28f55d3 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.test.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.test.jsx @@ -6,7 +6,6 @@ import { fireEvent, } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import PropTypes from 'prop-types'; import ContentTagsCollapsible from './ContentTagsCollapsible'; import messages from './messages'; @@ -60,19 +59,7 @@ const ContentTagsCollapsibleComponent = ({ contentId, taxonomyAndTagsData }) => ); -ContentTagsCollapsibleComponent.propTypes = { - contentId: PropTypes.string.isRequired, - taxonomyAndTagsData: PropTypes.shape({ - id: PropTypes.number, - name: PropTypes.string, - canTagObject: PropTypes.bool.isRequired, - contentTags: PropTypes.arrayOf(PropTypes.shape({ - value: PropTypes.string, - lineage: PropTypes.arrayOf(PropTypes.string), - canDeleteObjecttag: PropTypes.bool.isRequired, - })), - }).isRequired, -}; +ContentTagsCollapsibleComponent.propTypes = ContentTagsCollapsible.propTypes; describe('', () => { beforeAll(() => { diff --git a/src/content-tags-drawer/ContentTagsDropDownSelector.test.jsx b/src/content-tags-drawer/ContentTagsDropDownSelector.test.jsx index ac24a39e9a..8f25e40a25 100644 --- a/src/content-tags-drawer/ContentTagsDropDownSelector.test.jsx +++ b/src/content-tags-drawer/ContentTagsDropDownSelector.test.jsx @@ -6,7 +6,6 @@ import { waitFor, fireEvent, } from '@testing-library/react'; -import PropTypes from 'prop-types'; import ContentTagsDropDownSelector from './ContentTagsDropDownSelector'; import { useTaxonomyTagsData } from './data/apiHooks'; @@ -47,18 +46,7 @@ ContentTagsDropDownSelectorComponent.defaultProps = { searchTerm: '', }; -ContentTagsDropDownSelectorComponent.propTypes = { - taxonomyId: PropTypes.number.isRequired, - level: PropTypes.number.isRequired, - lineage: PropTypes.arrayOf(PropTypes.string), - tagsTree: PropTypes.objectOf( - PropTypes.shape({ - explicit: PropTypes.bool.isRequired, - children: PropTypes.shape({}).isRequired, - }).isRequired, - ).isRequired, - searchTerm: PropTypes.string, -}; +ContentTagsDropDownSelectorComponent.propTypes = ContentTagsDropDownSelector.propTypes; describe('', () => { afterEach(() => { diff --git a/src/content-tags-drawer/ContentTagsTree.test.jsx b/src/content-tags-drawer/ContentTagsTree.test.jsx index f45f06604c..23941393ab 100644 --- a/src/content-tags-drawer/ContentTagsTree.test.jsx +++ b/src/content-tags-drawer/ContentTagsTree.test.jsx @@ -1,7 +1,6 @@ import React from 'react'; import { IntlProvider } from '@edx/frontend-platform/i18n'; import { act, render } from '@testing-library/react'; -import PropTypes from 'prop-types'; import ContentTagsTree from './ContentTagsTree'; @@ -42,16 +41,7 @@ const ContentTagsTreeComponent = ({ tagsTree, removeTagHandler }) => ( ); -ContentTagsTreeComponent.propTypes = { - tagsTree: PropTypes.objectOf( - PropTypes.shape({ - explicit: PropTypes.bool.isRequired, - children: PropTypes.shape({}).isRequired, - canDeleteObjecttag: PropTypes.bool.isRequired, - }).isRequired, - ).isRequired, - removeTagHandler: PropTypes.func.isRequired, -}; +ContentTagsTreeComponent.propTypes = ContentTagsTree.propTypes; describe('', () => { it('should render taxonomy tags data along content tags number badge', async () => { diff --git a/src/content-tags-drawer/TagBubble.test.jsx b/src/content-tags-drawer/TagBubble.test.jsx index 0cf7961613..df73586426 100644 --- a/src/content-tags-drawer/TagBubble.test.jsx +++ b/src/content-tags-drawer/TagBubble.test.jsx @@ -1,7 +1,6 @@ import React from 'react'; import { IntlProvider } from '@edx/frontend-platform/i18n'; import { render, fireEvent } from '@testing-library/react'; -import PropTypes from 'prop-types'; import TagBubble from './TagBubble'; @@ -32,14 +31,7 @@ TagBubbleComponent.defaultProps = { canRemove: false, }; -TagBubbleComponent.propTypes = { - value: PropTypes.string.isRequired, - implicit: PropTypes.bool, - level: PropTypes.number, - lineage: PropTypes.arrayOf(PropTypes.string).isRequired, - removeTagHandler: PropTypes.func.isRequired, - canRemove: PropTypes.bool, -}; +TagBubbleComponent.propTypes = TagBubble.propTypes; describe('', () => { it('should render implicit tag', () => { @@ -105,6 +97,7 @@ describe('', () => { const { container } = render( ( ); -TaxonomyCardComponent.propTypes = { - original: PropTypes.shape({ - id: PropTypes.number, - name: PropTypes.string, - description: PropTypes.string, - systemDefined: PropTypes.bool, - orgsCount: PropTypes.number, - onDeleteTaxonomy: PropTypes.func, - canChangeTaxonomy: PropTypes.bool, - canDeleteTaxonomy: PropTypes.bool, - }).isRequired, -}; +TaxonomyCardComponent.propTypes = TaxonomyCard.propTypes; describe('', async () => { beforeEach(async () => { diff --git a/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx b/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx index aad601a0f1..bbcb68059a 100644 --- a/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx +++ b/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.test.jsx @@ -106,17 +106,6 @@ describe('', () => { expect(getByTestId('taxonomy-menu-import')).toBeVisible(); expect(getByTestId('taxonomy-menu-export')).toBeVisible(); expect(getByTestId('taxonomy-menu-delete')).toBeVisible(); - - // Click on button again to close the menu - fireEvent.click(getByTestId('taxonomy-menu-button')); - - // Menu closed - // Jest bug: toBeVisible() isn't checking opacity correctly - // expect(getByTestId('taxonomy-menu')).not.toBeVisible(); - expect(getByTestId('taxonomy-menu').style.opacity).toEqual('0'); - - // Menu button still visible - expect(getByTestId('taxonomy-menu-button')).toBeVisible(); }); it('should show system defined badge', async () => { diff --git a/src/taxonomy/taxonomy-detail/TaxonomyDetailSideCard.jsx b/src/taxonomy/taxonomy-detail/TaxonomyDetailSideCard.jsx index 57fe8614fd..e4eec9cb68 100644 --- a/src/taxonomy/taxonomy-detail/TaxonomyDetailSideCard.jsx +++ b/src/taxonomy/taxonomy-detail/TaxonomyDetailSideCard.jsx @@ -2,7 +2,7 @@ import { useIntl } from '@edx/frontend-platform/i18n'; import { Card, } from '@edx/paragon'; -import Proptypes from 'prop-types'; +import PropTypes from 'prop-types'; import messages from './messages'; @@ -23,9 +23,9 @@ const TaxonomyDetailSideCard = ({ taxonomy }) => { }; TaxonomyDetailSideCard.propTypes = { - taxonomy: Proptypes.shape({ - name: Proptypes.string.isRequired, - description: Proptypes.string.isRequired, + taxonomy: PropTypes.shape({ + name: PropTypes.string.isRequired, + description: PropTypes.string.isRequired, }).isRequired, }; diff --git a/src/taxonomy/taxonomy-detail/TaxonomyDetailSideCard.test.jsx b/src/taxonomy/taxonomy-detail/TaxonomyDetailSideCard.test.jsx index fb053eca72..7b2dd1fd3a 100644 --- a/src/taxonomy/taxonomy-detail/TaxonomyDetailSideCard.test.jsx +++ b/src/taxonomy/taxonomy-detail/TaxonomyDetailSideCard.test.jsx @@ -3,7 +3,6 @@ import { IntlProvider } from '@edx/frontend-platform/i18n'; import { initializeMockApp } from '@edx/frontend-platform'; import { AppProvider } from '@edx/frontend-platform/react'; import { render } from '@testing-library/react'; -import PropTypes from 'prop-types'; import initializeStore from '../../store'; @@ -25,12 +24,7 @@ const TaxonomyCardComponent = ({ taxonomy }) => ( ); -TaxonomyCardComponent.propTypes = { - taxonomy: PropTypes.shape({ - name: PropTypes.string, - description: PropTypes.string, - }).isRequired, -}; +TaxonomyCardComponent.propTypes = TaxonomyDetailSideCard.propTypes; describe('', async () => { beforeEach(async () => { diff --git a/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx b/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx index fac7a0aeab..666c2543fb 100644 --- a/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx +++ b/src/taxonomy/taxonomy-menu/TaxonomyMenu.test.jsx @@ -4,7 +4,6 @@ import { initializeMockApp } from '@edx/frontend-platform'; import { AppProvider } from '@edx/frontend-platform/react'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; import { render, fireEvent, waitFor } from '@testing-library/react'; -import PropTypes from 'prop-types'; import { TaxonomyContext } from '../common/context'; import initializeStore from '../../store'; @@ -60,14 +59,12 @@ const TaxonomyMenuComponent = ({ ); }; -TaxonomyMenuComponent.propTypes = { - iconMenu: PropTypes.bool.isRequired, - canChangeTaxonomy: PropTypes.bool, - systemDefined: PropTypes.bool, - canDeleteTaxonomy: PropTypes.bool, -}; +TaxonomyMenuComponent.propTypes = TaxonomyMenu.propTypes; TaxonomyMenuComponent.defaultProps = { + id: taxonomyId, + name: taxonomyName, + tagsCount: 0, systemDefined: false, canChangeTaxonomy: true, canDeleteTaxonomy: true,