Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use object permissions in Tagging frontend [FC-0036] #787

Merged
merged 9 commits into from
Jan 29, 2024
Merged

Use object permissions in Tagging frontend [FC-0036] #787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
18a2aa3
feat: use the permissions returned by the taxonomy REST API
pomegranited Jan 7, 2024
cb2d814
fix: REST API is now returning flattened user permissions
pomegranited Jan 11, 2024
f6a3487
Merge branch 'master' into jill/tagging-rest-perms
pomegranited Jan 11, 2024
93b0ad0
feat: Content Tags Drawer uses permissions returned by the content ta…
pomegranited Jan 11, 2024
3b97b18
fix: renames the `can<Action>` fields to `can<Action><Modelname>`
pomegranited Jan 12, 2024
af8b611
Merge branch 'master' into jill/tagging-rest-perms
pomegranited Jan 12, 2024
5a1de30
fix: use await findByTestId instead of wrapping menu clicks in act()
pomegranited Jan 18, 2024
1348604
revert: removes include_perms from the tagging REST API calls
pomegranited Jan 23, 2024
0eb4661
refactor: use component PropTypes in tagging tests
pomegranited Jan 23, 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
11 changes: 5 additions & 6 deletions src/content-tags-drawer/ContentTagsCollapsible.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -141,12 +140,12 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) =>
<div className="d-flex">
<Collapsible title={name} styling="card-lg" className="taxonomy-tags-collapsible">
<div key={id}>
<ContentTagsTree tagsTree={tagsTree} removeTagHandler={tagChangeHandler} editable={editable} />
<ContentTagsTree tagsTree={tagsTree} removeTagHandler={tagChangeHandler} />
</div>

<div className="d-flex taxonomy-tags-selector-menu">

{editable && (
{canTagObject && (
<Button
ref={setAddTagsButtonRef}
variant="outline-primary"
Expand Down Expand Up @@ -216,8 +215,8 @@ ContentTagsCollapsible.propTypes = {
value: PropTypes.string,
lineage: PropTypes.arrayOf(PropTypes.string),
})),
canTagObject: PropTypes.bool.isRequired,
}).isRequired,
editable: PropTypes.bool.isRequired,
};
Comment on lines 215 to 220
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these type definition are repeated in multiple places, it might be worth defining them once and them reusing them.

i.e. you could do put into a data/proptypes.js the following:

import PropTypes from 'prop-types';

const tag = PropTypes.shape({
  value: PropTypes.string,
  lineage: PropTypes.arrayOf(PropTypes.string),
});

const taxonomy = PropTypes.shape({
  id: PropTypes.number,
  name: PropTypes.string,
  contentTags: PropTypes.arrayOf(tag),
  canTagObject: PropTypes.bool.isRequired,
});

export default {
  tag,
  taxonomy,
};

Then they can be used anywhere as:

import TaxonomyProps from './data/proptypes';

ContentTagsCollapsible.propTypes = {
  contentId: PropTypes.string.isRequired,
  taxonomyAndTagsData: TaxonomyProps.taxonomy.isRequired,
};

This will avoid needing to update all places when the structure changes, and also will also make it easier to discover all the places that rely on this data structure.

Copy link
Contributor Author

@pomegranited pomegranited Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your review @xitij2000 ! And yep, we have to wait for openedx/edx-platform#34004 to merge before merging here.

Some of these type definition are repeated in multiple places, it might be worth defining them once and them reusing them.

Unfortunately, it looks like the components in src/taxonomy and src/content-tags-drawer don't share enough propTypes to warrant creating a prototypes.js file for them to use.

However, I do think it's silly that our test modules don't just re-use the propTypes of the components they're testing, so I've fixed that: 0eb4661
Let me know what you think?

Copy link
Contributor

@xitij2000 xitij2000 Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, I think that makes sense. I don't see any benefit in duplicating this, since it's already a development/testing tool more than a production tool.


export default ContentTagsCollapsible;
32 changes: 15 additions & 17 deletions src/content-tags-drawer/ContentTagsCollapsible.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -22,6 +21,7 @@ jest.mock('./data/apiHooks', () => ({
tagPages: [{
isLoading: true,
isError: false,
canAddTag: false,
data: [],
}],
})),
Expand All @@ -32,42 +32,34 @@ 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,
},
],
},
editable: true,
};

const ContentTagsCollapsibleComponent = ({ contentId, taxonomyAndTagsData, editable }) => (
const ContentTagsCollapsibleComponent = ({ contentId, taxonomyAndTagsData }) => (
<IntlProvider locale="en" messages={{}}>
<ContentTagsCollapsible contentId={contentId} taxonomyAndTagsData={taxonomyAndTagsData} editable={editable} />
<ContentTagsCollapsible contentId={contentId} taxonomyAndTagsData={taxonomyAndTagsData} />
</IntlProvider>
);

ContentTagsCollapsibleComponent.propTypes = {
contentId: PropTypes.string.isRequired,
taxonomyAndTagsData: PropTypes.shape({
id: PropTypes.number,
name: PropTypes.string,
contentTags: PropTypes.arrayOf(PropTypes.shape({
value: PropTypes.string,
lineage: PropTypes.arrayOf(PropTypes.string),
})),
}).isRequired,
editable: PropTypes.bool.isRequired,
};
ContentTagsCollapsibleComponent.propTypes = ContentTagsCollapsible.propTypes;

describe('<ContentTagsCollapsible />', () => {
beforeAll(() => {
Expand All @@ -85,14 +77,14 @@ describe('<ContentTagsCollapsible />', () => {
<ContentTagsCollapsibleComponent
contentId={componentData.contentId}
taxonomyAndTagsData={componentData.taxonomyAndTagsData}
editable={componentData.editable}
/>,
);
}

function setupTaxonomyMock() {
useTaxonomyTagsData.mockReturnValue({
hasMorePages: false,
canAddTag: false,
tagPages: [{
isLoading: false,
isError: false,
Expand All @@ -104,6 +96,8 @@ describe('<ContentTagsCollapsible />', () => {
parentValue: null,
id: 12345,
subTagsUrl: null,
canChangeTag: false,
canDeleteTag: false,
}, {
value: 'Tag 2',
externalId: null,
Expand All @@ -112,6 +106,8 @@ describe('<ContentTagsCollapsible />', () => {
parentValue: null,
id: 12346,
subTagsUrl: null,
canChangeTag: false,
canDeleteTag: false,
}, {
value: 'Tag 3',
externalId: null,
Expand All @@ -120,6 +116,8 @@ describe('<ContentTagsCollapsible />', () => {
parentValue: null,
id: 12347,
subTagsUrl: null,
canChangeTag: false,
canDeleteTag: false,
}],
}],
});
Expand Down
13 changes: 10 additions & 3 deletions src/content-tags-drawer/ContentTagsCollapsibleHelper.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ const removeTags = (tree, tagsToRemove) => {
*/
const useContentTagsCollapsibleHelper = (contentId, taxonomyAndTagsData) => {
const {
id, contentTags,
id, contentTags, canTagObject,
} = taxonomyAndTagsData;
// State to determine whether the tags are being updating so we can make a call
// to the update endpoint to the reflect those changes
Expand All @@ -101,7 +101,7 @@ const useContentTagsCollapsibleHelper = (contentId, taxonomyAndTagsData) => {
const tags = checkedTags.map(t => decodeURIComponent(t.split(',').slice(-1)));
updateTags.mutate({ tags });
}
}, [contentId, id, checkedTags]);
}, [contentId, id, canTagObject, checkedTags]);

// This converts the contentTags prop to the tree structure mentioned above
const appliedContentTags = React.useMemo(() => {
Expand All @@ -128,6 +128,8 @@ const useContentTagsCollapsibleHelper = (contentId, taxonomyAndTagsData) => {
currentLevel[key] = {
explicit: isExplicit,
children: {},
canChangeObjecttag: item.canChangeObjecttag,
canDeleteObjecttag: item.canDeleteObjecttag,
};

// Populating the SelectableBox with "selected" (explicit) tags
Expand Down Expand Up @@ -162,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;
}
Expand Down
3 changes: 1 addition & 2 deletions src/content-tags-drawer/ContentTagsDrawer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,7 @@ const ContentTagsDrawer = () => {
{ isTaxonomyListLoaded && isContentTaxonomyTagsLoaded
? taxonomies.map((data) => (
<div key={`taxonomy-tags-collapsible-${data.id}`}>
{/* TODO: Properly set whether tags should be editable or not based on permissions */}
<ContentTagsCollapsible contentId={contentId} taxonomyAndTagsData={data} editable />
<ContentTagsCollapsible contentId={contentId} taxonomyAndTagsData={data} />
<hr />
</div>
))
Expand Down
9 changes: 7 additions & 2 deletions src/content-tags-drawer/ContentTagsDrawer.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,26 +86,29 @@ describe('<ContentTagsDrawer />', () => {
{
name: 'Taxonomy 1',
taxonomyId: 123,
editable: true,
canTagObject: true,
tags: [
{
value: 'Tag 1',
lineage: ['Tag 1'],
canDeleteObjecttag: true,
},
{
value: 'Tag 2',
lineage: ['Tag 2'],
canDeleteObjecttag: true,
},
],
},
{
name: 'Taxonomy 2',
taxonomyId: 124,
editable: true,
canTagObject: true,
tags: [
{
value: 'Tag 3',
lineage: ['Tag 3'],
canDeleteObjecttag: true,
},
],
},
Expand All @@ -117,10 +120,12 @@ describe('<ContentTagsDrawer />', () => {
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 () => {
Expand Down
14 changes: 1 addition & 13 deletions src/content-tags-drawer/ContentTagsDropDownSelector.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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('<ContentTagsDropDownSelector />', () => {
afterEach(() => {
Expand Down
7 changes: 3 additions & 4 deletions src/content-tags-drawer/ContentTagsTree.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,8 @@ import TagBubble from './TagBubble';
* tagSelectableBoxValue: string,
* checked: boolean
* ) => void} props.removeTagHandler - Function that is called when removing tags from the tree.
* @param {boolean} props.editable - Whether the tags appear with an 'x' allowing the user to remove them.
*/
const ContentTagsTree = ({ tagsTree, removeTagHandler, editable }) => {
const ContentTagsTree = ({ tagsTree, removeTagHandler }) => {
const renderTagsTree = (tag, level, lineage) => Object.keys(tag).map((key) => {
const updatedLineage = [...lineage, encodeURIComponent(key)];
if (tag[key] !== undefined) {
Expand All @@ -56,7 +55,7 @@ const ContentTagsTree = ({ tagsTree, removeTagHandler, editable }) => {
level={level}
lineage={updatedLineage}
removeTagHandler={removeTagHandler}
editable={editable}
canRemove={tag[key].canDeleteObjecttag}
/>
{ renderTagsTree(tag[key].children, level + 1, updatedLineage) }
</div>
Expand All @@ -73,10 +72,10 @@ ContentTagsTree.propTypes = {
PropTypes.shape({
explicit: PropTypes.bool.isRequired,
children: PropTypes.shape({}).isRequired,
canDeleteObjecttag: PropTypes.bool.isRequired,
}).isRequired,
).isRequired,
removeTagHandler: PropTypes.func.isRequired,
editable: PropTypes.bool.isRequired,
};

export default ContentTagsTree;
23 changes: 9 additions & 14 deletions src/content-tags-drawer/ContentTagsTree.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,57 +1,52 @@
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';

const data = {
'Science and Research': {
explicit: false,
canDeleteObjecttag: false,
children: {
'Genetics Subcategory': {
explicit: false,
children: {
'DNA Sequencing': {
explicit: true,
children: {},
canDeleteObjecttag: true,
},
},
canDeleteObjecttag: false,
},
'Molecular, Cellular, and Microbiology': {
explicit: false,
children: {
Virology: {
explicit: true,
children: {},
canDeleteObjecttag: true,
},
},
canDeleteObjecttag: false,
},
},
},
};

const ContentTagsTreeComponent = ({ tagsTree, removeTagHandler, editable }) => (
const ContentTagsTreeComponent = ({ tagsTree, removeTagHandler }) => (
<IntlProvider locale="en" messages={{}}>
<ContentTagsTree tagsTree={tagsTree} removeTagHandler={removeTagHandler} editable={editable} />
<ContentTagsTree tagsTree={tagsTree} removeTagHandler={removeTagHandler} />
</IntlProvider>
);

ContentTagsTreeComponent.propTypes = {
tagsTree: PropTypes.objectOf(
PropTypes.shape({
explicit: PropTypes.bool.isRequired,
children: PropTypes.shape({}).isRequired,
}).isRequired,
).isRequired,
removeTagHandler: PropTypes.func.isRequired,
editable: PropTypes.bool.isRequired,
};
ContentTagsTreeComponent.propTypes = ContentTagsTree.propTypes;

describe('<ContentTagsTree />', () => {
it('should render taxonomy tags data along content tags number badge', async () => {
await act(async () => {
const { getByText } = render(<ContentTagsTreeComponent tagsTree={data} removeTagHandler={() => {}} editable />);
const { getByText } = render(<ContentTagsTreeComponent tagsTree={data} removeTagHandler={() => {}} />);
expect(getByText('Science and Research')).toBeInTheDocument();
expect(getByText('Genetics Subcategory')).toBeInTheDocument();
expect(getByText('Molecular, Cellular, and Microbiology')).toBeInTheDocument();
Expand Down
Loading