From 0d020cd69eaccb28a9bde59cd32f340507160e3d Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Tue, 21 Nov 2023 19:19:27 +0300 Subject: [PATCH 01/24] feat: Add content tags tree state + editing This commit adds the editing functionality for content tags (minus commuincation with the backend). You are able to select/deselect tags from the dropdowns in each taxonomy and the tags tree structure is updated and the state persists. When a tag is select, all its ancestors automatically become 'implicit' and are unselected and connot be selected. If an explicit tag is unselected and the parent (or ancestor) no longer has explicit children, all ancestor tags will also be removed. --- .../ContentTagsCollapsible.jsx | 212 +++++++++++++++++- .../ContentTagsDropDownSelector.jsx | 26 ++- .../ContentTagsDropDownSelector.scss | 8 + src/content-tags-drawer/ContentTagsTree.jsx | 87 +++---- 4 files changed, 278 insertions(+), 55 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx index 5e27f8291c..3c35cf0f6d 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx @@ -6,6 +6,7 @@ import { Button, ModalPopup, useToggle, + useCheckboxSetValues, } from '@edx/paragon'; import PropTypes from 'prop-types'; import classNames from 'classnames'; @@ -21,6 +22,73 @@ import ContentTagsTree from './ContentTagsTree'; * Collapsible component that holds a Taxonomy along with Tags that belong to it. * This includes both applied tags and tags that are available to select * from a dropdown list. + * + * This component also handles all the logic with selecting/deselecting tags and keeps track of the + * tags tree in the state. That is used to render the Tag bubbgles as well as the populating the + * state of the tags in the dropdown selectors. + * + * The `contentTags` that is passed are consolidated and converted to a tree structure. For example: + * + * FROM: + * + * [ + * { + * "value": "DNA Sequencing", + * "lineage": [ + * "Science and Research", + * "Genetics Subcategory", + * "DNA Sequencing" + * ] + * }, + * { + * "value": "Virology", + * "lineage": [ + * "Science and Research", + * "Molecular, Cellular, and Microbiology", + * "Virology" + * ] + * } + * ] + * + * TO: + * + * { + * "Science and Research": { + * explicit: false, + * children: { + * "Genetics Subcategory": { + * explicit: false, + * children: { + * "DNA Sequencing": { + * explicit: true, + * children: {} + * } + * } + * }, + * "Molecular, Cellular, and Microbiology": { + * explicit: false, + * children: { + * "Virology": { + * explicit: true, + * children: {} + * } + * } + * } + * } + * } + * }; + * + * + * It also keeps track of newly added tags as they are selected in the dropdown selectors. + * They are store in the same format above, and then merged to one tree that is used as the + * source of truth for both the tag bubble and the dropdowns. They keys are order alphabetically. + * + * In the dropdowns, the value of each SelectableBox is stored along with it's lineage and is URI encoded. + * Ths is so we are able to traverse and manipulate different parts of the tree leading to it. + * Here is an example of what the value of the "Virology" tag would be: + * + * "Science%20and%20Research,Molecular%2C%20Cellular%2C%20and%20Microbiology,Virology" + * * @param {Object} taxonomyAndTagsData - Object containing Taxonomy meta data along with applied tags * @param {number} taxonomyAndTagsData.id - id of Taxonomy * @param {string} taxonomyAndTagsData.name - name of Taxonomy @@ -45,11 +113,150 @@ const ContentTagsCollapsible = ({ taxonomyAndTagsData }) => { const [isOpen, open, close] = useToggle(false); const [target, setTarget] = React.useState(null); + // Keeps track of the tree structure for the applied content tags passed + // in as a prop. + const [appliedContentTags, setAppliedContentTags] = React.useState({}); + + // Keeps track of the tree structure for tags that are add by selecting/unselecting + // tags in the dropdowns. + const [addedContentTags, setAddedContentTags] = React.useState({}); + + // To handle checking/unchecking tags in the SelectableBox + const [checkedTags, { add, remove }] = useCheckboxSetValues(); + + const mergeTrees = (tree1, tree2) => { + const mergedTree = { ...tree1 }; + + const sortKeysAlphabetically = (obj) => { + const sortedObj = {}; + Object.keys(obj) + .sort() + .forEach((key) => { + sortedObj[key] = obj[key]; + if (obj[key] && typeof obj[key] === 'object') { + sortedObj[key].children = sortKeysAlphabetically(obj[key].children); + } + }); + return sortedObj; + }; + + const mergeRecursively = (destination, source) => { + Object.entries(source).forEach(([key, sourceValue]) => { + const destinationValue = destination[key]; + + if (destinationValue && sourceValue && typeof destinationValue === 'object' && typeof sourceValue === 'object') { + mergeRecursively(destinationValue, sourceValue); + } else { + // eslint-disable-next-line no-param-reassign + destination[key] = sourceValue; + } + }); + }; + + mergeRecursively(mergedTree, tree2); + return sortKeysAlphabetically(mergedTree); + }; + + // This converts the contentTags prop to the tree structure mentioned above + React.useEffect(() => { + const resultTree = {}; + + contentTags.forEach(item => { + let currentLevel = resultTree; + + item.lineage.forEach((key, index) => { + if (!currentLevel[key]) { + const isExplicit = index === item.lineage.length - 1; + currentLevel[key] = { + explicit: isExplicit, + children: {}, + }; + + // Populating the SelectableBox with "selected" (explicit) tags + const value = item.lineage.map(l => encodeURIComponent(l)).join(','); + // eslint-disable-next-line no-unused-expressions + isExplicit ? add(value) : remove(value); + } + + currentLevel = currentLevel[key].children; + }); + }); + + setAppliedContentTags(resultTree); + }, [contentTags]); + + // This is out source of truth that represents the current state of tags in + // this Taxonomy as a tree. Whenever either the `appliedContentTags` (i.e. tags passed in + // the prop from the backed) change, or when the `addedContentTags` (i.e. tags added by + // selecting/unselecting them in the dropdown) change, the tree is recomputed. + const tagsTree = React.useMemo(() => ( + mergeTrees(appliedContentTags, addedContentTags) + ), [appliedContentTags, addedContentTags]); + + // Add tag to the tree, and while traversing remove any selected ancestor tags + // as they should become implicit + const addTags = (tree, tagLineage, selectedTag) => { + const value = []; + let traversal = tree; + tagLineage.forEach(tag => { + const isExplicit = selectedTag === tag; + + if (!traversal[tag]) { + traversal[tag] = { explicit: isExplicit, children: {} }; + } else { + traversal[tag].explicit = isExplicit; + } + + // Clear out the ancestor tags leading to newly selected tag + // as they automatically become implicit + value.push(encodeURIComponent(tag)); + // eslint-disable-next-line no-unused-expressions + isExplicit ? add(value.join(',')) : remove(value.join(',')); + + traversal = traversal[tag].children; + }); + }; + + // Remove the tag along with it's ancestors if it was the only explicit child tag + const removeTags = (tree, tagsToRemove) => { + if (!tree || !tagsToRemove.length) { + return; + } + const key = tagsToRemove[0]; + if (tree[key]) { + removeTags(tree[key].children, tagsToRemove.slice(1)); + + if (Object.keys(tree[key].children).length === 0 && (tree[key].explicit === false || tagsToRemove.length === 1)) { + // eslint-disable-next-line no-param-reassign + delete tree[key]; + } + } + }; + + const handleSelectableBoxChange = (e) => { + // eslint-disable-next-line no-unused-expressions + e.target.checked ? add(e.target.value) : remove(e.target.value); + const tagLineage = e.target.value.split(',').map(t => decodeURIComponent(t)); + const selectedTag = tagLineage.slice(-1)[0]; + + const addedTree = { ...addedContentTags }; + if (e.target.checked) { + addTags(addedTree, tagLineage, selectedTag); + } else { + // We remove them from both incase we are unselecting from an + // existing applied Tag or a newly added one + removeTags(addedTree, tagLineage); + removeTags(appliedContentTags, tagLineage); + } + + setAddedContentTags(addedTree); + }; + return (
- +
@@ -76,11 +283,14 @@ const ContentTagsCollapsible = ({ taxonomyAndTagsData }) => { columns={1} ariaLabel={intl.formatMessage(messages.taxonomyTagsAriaLabel)} className="taxonomy-tags-selectable-box-set" + onChange={handleSelectableBoxChange} + value={checkedTags} >
diff --git a/src/content-tags-drawer/ContentTagsDropDownSelector.jsx b/src/content-tags-drawer/ContentTagsDropDownSelector.jsx index e71ac8a80f..14f77e0f46 100644 --- a/src/content-tags-drawer/ContentTagsDropDownSelector.jsx +++ b/src/content-tags-drawer/ContentTagsDropDownSelector.jsx @@ -13,7 +13,7 @@ import './ContentTagsDropDownSelector.scss'; import { useTaxonomyTagsDataResponse, useIsTaxonomyTagsDataLoaded } from './data/apiHooks'; const ContentTagsDropDownSelector = ({ - taxonomyId, level, subTagsUrl, + taxonomyId, level, subTagsUrl, lineage, tagsTree, }) => { const intl = useIntl(); // This object represents the states of the dropdowns on this level @@ -32,6 +32,17 @@ const ContentTagsDropDownSelector = ({ const taxonomyTagsData = useTaxonomyTagsDataResponse(taxonomyId, subTagsUrl); const isTaxonomyTagsLoaded = useIsTaxonomyTagsDataLoaded(taxonomyId, subTagsUrl); + const isImplicit = (tag) => { + // Traverse the tags tree using the lineage + let traversal = tagsTree; + lineage.forEach(t => { + // We need to decode the tag to traverse the tree since the lineage value is encoded + traversal = traversal[decodeURIComponent(t)]?.children || {}; + }); + + return (traversal[tag.value] && !traversal[tag.value].explicit) || false; + }; + return ( isTaxonomyTagsLoaded && taxonomyTagsData ? taxonomyTagsData.results.map((taxonomyTag, i) => ( @@ -43,6 +54,9 @@ const ContentTagsDropDownSelector = ({ className="taxonomy-tags-selectable-box" aria-label={`${taxonomyTag.value} checkbox`} data-selectable-box="taxonomy-tags" + value={[...lineage, encodeURIComponent(taxonomyTag.value)].join(',')} + isIndeterminate={isImplicit(taxonomyTag)} + disabled={isImplicit(taxonomyTag)} > {taxonomyTag.value} @@ -65,6 +79,8 @@ const ContentTagsDropDownSelector = ({ taxonomyId={taxonomyId} subTagsUrl={taxonomyTag.subTagsUrl} level={level + 1} + lineage={[...lineage, encodeURIComponent(taxonomyTag.value)]} + tagsTree={tagsTree} /> )} @@ -84,12 +100,20 @@ const ContentTagsDropDownSelector = ({ ContentTagsDropDownSelector.defaultProps = { subTagsUrl: undefined, + lineage: [], }; ContentTagsDropDownSelector.propTypes = { taxonomyId: PropTypes.number.isRequired, level: PropTypes.number.isRequired, subTagsUrl: PropTypes.string, + lineage: PropTypes.arrayOf(PropTypes.string), + tagsTree: PropTypes.objectOf( + PropTypes.shape({ + explicit: PropTypes.bool.isRequired, + children: PropTypes.objectOf().isRequired, + }).isRequired, + ).isRequired, }; export default ContentTagsDropDownSelector; diff --git a/src/content-tags-drawer/ContentTagsDropDownSelector.scss b/src/content-tags-drawer/ContentTagsDropDownSelector.scss index 33c29517e8..03a3b03b0d 100644 --- a/src/content-tags-drawer/ContentTagsDropDownSelector.scss +++ b/src/content-tags-drawer/ContentTagsDropDownSelector.scss @@ -6,3 +6,11 @@ box-shadow: none; padding: 0; } + +.pgn__selectable_box:disabled, .pgn__selectable_box[disabled] { + opacity: 1 !important; +} + +.pgn__selectable_box-active { + outline: none !important; +} \ No newline at end of file diff --git a/src/content-tags-drawer/ContentTagsTree.jsx b/src/content-tags-drawer/ContentTagsTree.jsx index e75ead4766..360cd9f555 100644 --- a/src/content-tags-drawer/ContentTagsTree.jsx +++ b/src/content-tags-drawer/ContentTagsTree.jsx @@ -1,63 +1,42 @@ -import React, { useMemo } from 'react'; +import React from 'react'; import PropTypes from 'prop-types'; import TagBubble from './TagBubble'; /** - * Component that renders Tags under a Taxonomy in the nested tree format - * It constructs a tree structure consolidating the tag data. Example: + * Component that renders Tags under a Taxonomy in the nested tree format. * - * FROM: - * - * [ - * { - * "value": "DNA Sequencing", - * "lineage": [ - * "Science and Research", - * "Genetics Subcategory", - * "DNA Sequencing" - * ] - * }, - * { - * "value": "Virology", - * "lineage": [ - * "Science and Research", - * "Molecular, Cellular, and Microbiology", - * "Virology" - * ] - * } - * ] - * - * TO: + * Example: * * { * "Science and Research": { - * "Genetics Subcategory": { - * "DNA Sequencing": {} - * }, - * "Molecular, Cellular, and Microbiology": { - * "Virology": {} + * explicit: false, + * children: { + * "Genetics Subcategory": { + * explicit: false, + * children: { + * "DNA Sequencing": { + * explicit: true, + * children: {} + * } + * } + * }, + * "Molecular, Cellular, and Microbiology": { + * explicit: false, + * children: { + * "Virology": { + * explicit: true, + * children: {} + * } + * } + * } * } * } - * } + * }; * - * @param {Object[]} appliedContentTags - Array of taxonomy tags that are applied to the content - * @param {string} appliedContentTags.value - Value of applied Tag - * @param {string} appliedContentTags.lineage - Array of Tag's ancestors sorted (ancestor -> tag) + * @param {Object} tagsTree - Array of taxonomy tags that are applied to the content */ -const ContentTagsTree = ({ appliedContentTags }) => { - const tagsTree = useMemo(() => { - const tree = {}; - appliedContentTags.forEach(tag => { - tag.lineage.reduce((currentLevel, ancestor) => { - // eslint-disable-next-line no-param-reassign - currentLevel[ancestor] = currentLevel[ancestor] || {}; - return currentLevel[ancestor]; - }, tree); - }); - return tree; - }, [appliedContentTags]); - +const ContentTagsTree = ({ tagsTree }) => { const renderTagsTree = (tag, level) => Object.keys(tag).map((key) => { if (tag[key] !== undefined) { return ( @@ -65,10 +44,10 @@ const ContentTagsTree = ({ appliedContentTags }) => { - { renderTagsTree(tag[key], level + 1) } + { renderTagsTree(tag[key].children, level + 1) }
); } @@ -79,10 +58,12 @@ const ContentTagsTree = ({ appliedContentTags }) => { }; ContentTagsTree.propTypes = { - appliedContentTags: PropTypes.arrayOf(PropTypes.shape({ - value: PropTypes.string, - lineage: PropTypes.arrayOf(PropTypes.string), - })).isRequired, + tagsTree: PropTypes.objectOf( + PropTypes.shape({ + explicit: PropTypes.bool.isRequired, + children: PropTypes.objectOf().isRequired, + }).isRequired, + ).isRequired, }; export default ContentTagsTree; From f40f1ea55000d4ee47e2b0f2b02641163bdd575a Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Wed, 22 Nov 2023 18:24:59 +0300 Subject: [PATCH 02/24] feat: Add API calls to backend when updating tags This makes use of useMutation to communicate changes to the applied tags to the backend so they are updated in the DB. When a selectable box is checked/unchecked a request is made. And when an explicit tag is clicked on, a request is also made. --- .../ContentTagsCollapsible.jsx | 31 ++++++++++++++++--- src/content-tags-drawer/ContentTagsDrawer.jsx | 2 +- .../ContentTagsDropDownSelector.jsx | 2 +- src/content-tags-drawer/ContentTagsTree.jsx | 14 ++++++--- src/content-tags-drawer/TagBubble.jsx | 18 ++++++++--- src/content-tags-drawer/data/api.js | 14 +++++++++ src/content-tags-drawer/data/apiHooks.jsx | 19 ++++++++++-- src/content-tags-drawer/data/types.mjs | 5 +++ 8 files changed, 88 insertions(+), 17 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx index 3c35cf0f6d..789cf18d73 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx @@ -18,6 +18,8 @@ import ContentTagsDropDownSelector from './ContentTagsDropDownSelector'; import ContentTagsTree from './ContentTagsTree'; +import { useContentTaxonomyTagsMutation } from './data/apiHooks'; + /** * Collapsible component that holds a Taxonomy along with Tags that belong to it. * This includes both applied tags and tags that are available to select @@ -88,7 +90,7 @@ import ContentTagsTree from './ContentTagsTree'; * Here is an example of what the value of the "Virology" tag would be: * * "Science%20and%20Research,Molecular%2C%20Cellular%2C%20and%20Microbiology,Virology" - * + * @param {string} contentId - Id of the content object * @param {Object} taxonomyAndTagsData - Object containing Taxonomy meta data along with applied tags * @param {number} taxonomyAndTagsData.id - id of Taxonomy * @param {string} taxonomyAndTagsData.name - name of Taxonomy @@ -104,12 +106,16 @@ import ContentTagsTree from './ContentTagsTree'; * @param {string} taxonomyAndTagsData.contentTags.value - Value of applied Tag * @param {string} taxonomyAndTagsData.contentTags.lineage - Array of Tag's ancestors sorted (ancestor -> tag) */ -const ContentTagsCollapsible = ({ taxonomyAndTagsData }) => { +const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { const intl = useIntl(); const { id, name, contentTags, } = taxonomyAndTagsData; + // State to determine whether to update the backend or not + const [updatingTags, setUpdatingTags] = React.useState(false); + const mutation = useContentTaxonomyTagsMutation(); + const [isOpen, open, close] = useToggle(false); const [target, setTarget] = React.useState(null); @@ -124,6 +130,18 @@ const ContentTagsCollapsible = ({ taxonomyAndTagsData }) => { // To handle checking/unchecking tags in the SelectableBox const [checkedTags, { add, remove }] = useCheckboxSetValues(); + // Handles make requests to the backend whenever the checked tags change + React.useEffect(() => { + // We have this check because this hook is fired when the component first load + // and reloads (on refocus). We only want to make requests to the backend when + // the user is updating the tags. + if (updatingTags) { + setUpdatingTags(false); + const tags = checkedTags.map(t => decodeURIComponent(t.split(',').slice(-1))); + mutation.mutate({ contentId, taxonomyId: id, tags }); + } + }, [contentId, id, checkedTags]); + const mergeTrees = (tree1, tree2) => { const mergedTree = { ...tree1 }; @@ -235,14 +253,17 @@ const ContentTagsCollapsible = ({ taxonomyAndTagsData }) => { const handleSelectableBoxChange = (e) => { // eslint-disable-next-line no-unused-expressions - e.target.checked ? add(e.target.value) : remove(e.target.value); const tagLineage = e.target.value.split(',').map(t => decodeURIComponent(t)); const selectedTag = tagLineage.slice(-1)[0]; const addedTree = { ...addedContentTags }; if (e.target.checked) { + // We "add" the tag to the SelectableBox.Set inside the addTags method addTags(addedTree, tagLineage, selectedTag); } else { + // Remove tag from the SelectableBox.Set + remove(e.target.value); + // We remove them from both incase we are unselecting from an // existing applied Tag or a newly added one removeTags(addedTree, tagLineage); @@ -250,13 +271,14 @@ const ContentTagsCollapsible = ({ taxonomyAndTagsData }) => { } setAddedContentTags(addedTree); + setUpdatingTags(true); }; return (
- +
@@ -314,6 +336,7 @@ const ContentTagsCollapsible = ({ taxonomyAndTagsData }) => { }; ContentTagsCollapsible.propTypes = { + contentId: PropTypes.string.isRequired, taxonomyAndTagsData: PropTypes.shape({ id: PropTypes.number, name: PropTypes.string, diff --git a/src/content-tags-drawer/ContentTagsDrawer.jsx b/src/content-tags-drawer/ContentTagsDrawer.jsx index eeed39be1d..49335c4e77 100644 --- a/src/content-tags-drawer/ContentTagsDrawer.jsx +++ b/src/content-tags-drawer/ContentTagsDrawer.jsx @@ -113,7 +113,7 @@ const ContentTagsDrawer = () => { { isTaxonomyListLoaded && isContentTaxonomyTagsLoaded ? taxonomies.map((data) => (
- +
)) diff --git a/src/content-tags-drawer/ContentTagsDropDownSelector.jsx b/src/content-tags-drawer/ContentTagsDropDownSelector.jsx index 14f77e0f46..a6c1485364 100644 --- a/src/content-tags-drawer/ContentTagsDropDownSelector.jsx +++ b/src/content-tags-drawer/ContentTagsDropDownSelector.jsx @@ -111,7 +111,7 @@ ContentTagsDropDownSelector.propTypes = { tagsTree: PropTypes.objectOf( PropTypes.shape({ explicit: PropTypes.bool.isRequired, - children: PropTypes.objectOf().isRequired, + children: PropTypes.shape({}).isRequired, }).isRequired, ).isRequired, }; diff --git a/src/content-tags-drawer/ContentTagsTree.jsx b/src/content-tags-drawer/ContentTagsTree.jsx index 360cd9f555..6a71ee1f08 100644 --- a/src/content-tags-drawer/ContentTagsTree.jsx +++ b/src/content-tags-drawer/ContentTagsTree.jsx @@ -36,8 +36,9 @@ import TagBubble from './TagBubble'; * * @param {Object} tagsTree - Array of taxonomy tags that are applied to the content */ -const ContentTagsTree = ({ tagsTree }) => { - const renderTagsTree = (tag, level) => Object.keys(tag).map((key) => { +const ContentTagsTree = ({ tagsTree, removeTagHandler }) => { + const renderTagsTree = (tag, level, lineage) => Object.keys(tag).map((key) => { + const updatedLineage = [...lineage, encodeURIComponent(key)]; if (tag[key] !== undefined) { return (
@@ -46,24 +47,27 @@ const ContentTagsTree = ({ tagsTree }) => { value={key} implicit={!tag[key].explicit} level={level} + lineage={updatedLineage} + removeTagHandler={removeTagHandler} /> - { renderTagsTree(tag[key].children, level + 1) } + { renderTagsTree(tag[key].children, level + 1, updatedLineage) }
); } return null; }); - return renderTagsTree(tagsTree, 0); + return renderTagsTree(tagsTree, 0, []); }; ContentTagsTree.propTypes = { tagsTree: PropTypes.objectOf( PropTypes.shape({ explicit: PropTypes.bool.isRequired, - children: PropTypes.objectOf().isRequired, + children: PropTypes.shape({}).isRequired, }).isRequired, ).isRequired, + removeTagHandler: PropTypes.func.isRequired, }; export default ContentTagsTree; diff --git a/src/content-tags-drawer/TagBubble.jsx b/src/content-tags-drawer/TagBubble.jsx index 8c7137ffa0..b28c1a912c 100644 --- a/src/content-tags-drawer/TagBubble.jsx +++ b/src/content-tags-drawer/TagBubble.jsx @@ -8,10 +8,18 @@ import PropTypes from 'prop-types'; import TagOutlineIcon from './TagOutlineIcon'; const TagBubble = ({ - value, subTagsCount, implicit, level, + value, implicit, level, lineage, removeTagHandler, }) => { const className = `tag-bubble mb-2 ${implicit ? 'implicit' : ''}`; const tagIcon = () => (implicit ? : ); + + const handleClick = (e) => { + if (e.target.value) { + e.target.checked = false; + removeTagHandler(e); + } + }; + return (
); }; TagBubble.defaultProps = { - subTagsCount: 0, implicit: true, level: 0, }; TagBubble.propTypes = { value: PropTypes.string.isRequired, - subTagsCount: PropTypes.number, implicit: PropTypes.bool, level: PropTypes.number, + lineage: PropTypes.arrayOf(PropTypes.string).isRequired, + removeTagHandler: PropTypes.func.isRequired, }; export default TagBubble; diff --git a/src/content-tags-drawer/data/api.js b/src/content-tags-drawer/data/api.js index e63b3d0842..d18a17f875 100644 --- a/src/content-tags-drawer/data/api.js +++ b/src/content-tags-drawer/data/api.js @@ -40,3 +40,17 @@ export async function getContentData(contentId) { const { data } = await getAuthenticatedHttpClient().get(getContentDataApiUrl(contentId)); return camelCaseObject(data); } + +/** + * Update content object's applied tags + * @param {string} contentId The id of the content object (unit/component) + * @param {string} taxonomyId The id of the taxonomy the tags belong to + * @param {string[]} tags The list of tags (values) to set on content object + * @returns {Promise} + */ +export async function updateContentTaxonomyTags(contentId, taxonomyId, tags) { + let url = getContentTaxonomyTagsApiUrl(contentId); + url = `${url}?taxonomy=${taxonomyId}`; + const { data } = await getAuthenticatedHttpClient().put(url, { tags }); + return camelCaseObject(data); +} diff --git a/src/content-tags-drawer/data/apiHooks.jsx b/src/content-tags-drawer/data/apiHooks.jsx index 099c88129d..3f6d5fa071 100644 --- a/src/content-tags-drawer/data/apiHooks.jsx +++ b/src/content-tags-drawer/data/apiHooks.jsx @@ -1,6 +1,11 @@ // @ts-check -import { useQuery } from '@tanstack/react-query'; -import { getTaxonomyTagsData, getContentTaxonomyTagsData, getContentData } from './api'; +import { useQuery, useMutation } from '@tanstack/react-query'; +import { + getTaxonomyTagsData, + getContentTaxonomyTagsData, + getContentData, + updateContentTaxonomyTags, +} from './api'; /** * Builds the query to get the taxonomy tags @@ -109,3 +114,13 @@ export const useContentDataResponse = (contentId) => { export const useIsContentDataLoaded = (contentId) => ( useContentData(contentId).status === 'success' ); + +/** + * Builds the mutation to update the tags applied to the content object + * @returns {import("./types.mjs").UseMutationResult} + */ +export const useContentTaxonomyTagsMutation = () => ( + useMutation({ + mutationFn: ({ contentId, taxonomyId, tags }) => updateContentTaxonomyTags(contentId, taxonomyId, tags), + }) +); diff --git a/src/content-tags-drawer/data/types.mjs b/src/content-tags-drawer/data/types.mjs index 00b3fefd4c..78fcf9598d 100644 --- a/src/content-tags-drawer/data/types.mjs +++ b/src/content-tags-drawer/data/types.mjs @@ -105,3 +105,8 @@ * @property {Object} data * @property {string} status */ + +/** + * @typedef {Object} UseMutationResult + * @property {(variables: {Object}, { onSuccess, onSettled, onError }) => void} mutate + */ From 5929062015162276ef316efe4cd69300d28f48cc Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Thu, 23 Nov 2023 15:29:44 +0300 Subject: [PATCH 03/24] fix: Use Chip instead of Button for TagBubble Also refactor tag change handler and wrap it with useCallback --- .../ContentTagsCollapsible.jsx | 17 ++++++++------ src/content-tags-drawer/TagBubble.jsx | 23 ++++++++----------- src/content-tags-drawer/TagBubble.scss | 17 +++++++++----- src/content-tags-drawer/TagOutlineIcon.jsx | 1 - 4 files changed, 31 insertions(+), 27 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx index 789cf18d73..913e0b12a9 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx @@ -176,7 +176,7 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { }; // This converts the contentTags prop to the tree structure mentioned above - React.useEffect(() => { + React.useMemo(() => { const resultTree = {}; contentTags.forEach(item => { @@ -251,18 +251,17 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { } }; - const handleSelectableBoxChange = (e) => { - // eslint-disable-next-line no-unused-expressions - const tagLineage = e.target.value.split(',').map(t => decodeURIComponent(t)); + const tagChangeHandler = (tagSelectableBoxValue, checked) => { + const tagLineage = tagSelectableBoxValue.split(',').map(t => decodeURIComponent(t)); const selectedTag = tagLineage.slice(-1)[0]; const addedTree = { ...addedContentTags }; - if (e.target.checked) { + if (checked) { // We "add" the tag to the SelectableBox.Set inside the addTags method addTags(addedTree, tagLineage, selectedTag); } else { // Remove tag from the SelectableBox.Set - remove(e.target.value); + remove(tagSelectableBoxValue); // We remove them from both incase we are unselecting from an // existing applied Tag or a newly added one @@ -274,11 +273,15 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { setUpdatingTags(true); }; + const handleSelectableBoxChange = React.useCallback((e) => { + tagChangeHandler(e.target.value, e.target.checked); + }); + return (
- +
diff --git a/src/content-tags-drawer/TagBubble.jsx b/src/content-tags-drawer/TagBubble.jsx index b28c1a912c..84a0cfbe55 100644 --- a/src/content-tags-drawer/TagBubble.jsx +++ b/src/content-tags-drawer/TagBubble.jsx @@ -1,6 +1,6 @@ import React from 'react'; import { - Button, + Chip, } from '@edx/paragon'; import { Tag, Close } from '@edx/paragon/icons'; import PropTypes from 'prop-types'; @@ -11,27 +11,24 @@ const TagBubble = ({ value, implicit, level, lineage, removeTagHandler, }) => { const className = `tag-bubble mb-2 ${implicit ? 'implicit' : ''}`; - const tagIcon = () => (implicit ? : ); - const handleClick = (e) => { - if (e.target.value) { - e.target.checked = false; - removeTagHandler(e); + const handleClick = React.useCallback(() => { + if (!implicit) { + removeTagHandler(lineage.join(','), false); } - }; + }, [implicit, lineage]); return (
- +
); }; diff --git a/src/content-tags-drawer/TagBubble.scss b/src/content-tags-drawer/TagBubble.scss index 281d0fe209..e139d71226 100644 --- a/src/content-tags-drawer/TagBubble.scss +++ b/src/content-tags-drawer/TagBubble.scss @@ -1,13 +1,18 @@ -.tag-bubble.btn-outline-dark { +.tag-bubble.pgn__chip { border-color: $light-300; + border-style: solid; + border-width: 2px; + background-color: transparent; &:hover { - color: $white; + .pgn__chip__icon-before * { + color: $white; + fill: $white; + } + .pgn__chip__label { + color: $white; + } background-color: $dark; border-color: $dark; } } - -.implicit > .implicit-tag-icon { - color: $dark; -} diff --git a/src/content-tags-drawer/TagOutlineIcon.jsx b/src/content-tags-drawer/TagOutlineIcon.jsx index f817b1f077..7f9d439254 100644 --- a/src/content-tags-drawer/TagOutlineIcon.jsx +++ b/src/content-tags-drawer/TagOutlineIcon.jsx @@ -10,7 +10,6 @@ const TagOutlineIcon = (props) => ( aria-hidden="true" {...props} > - From aac5c56a915410e2d76d27c9334e68211c6bb2eb Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Thu, 23 Nov 2023 15:48:05 +0300 Subject: [PATCH 04/24] chore: fix stylelint issues --- src/content-tags-drawer/ContentTagsDropDownSelector.scss | 5 +++-- src/content-tags-drawer/TagBubble.scss | 2 ++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsDropDownSelector.scss b/src/content-tags-drawer/ContentTagsDropDownSelector.scss index 03a3b03b0d..c6c659ccd6 100644 --- a/src/content-tags-drawer/ContentTagsDropDownSelector.scss +++ b/src/content-tags-drawer/ContentTagsDropDownSelector.scss @@ -7,10 +7,11 @@ padding: 0; } -.pgn__selectable_box:disabled, .pgn__selectable_box[disabled] { +.pgn__selectable_box:disabled, +.pgn__selectable_box[disabled] { opacity: 1 !important; } .pgn__selectable_box-active { outline: none !important; -} \ No newline at end of file +} diff --git a/src/content-tags-drawer/TagBubble.scss b/src/content-tags-drawer/TagBubble.scss index e139d71226..3878265fd3 100644 --- a/src/content-tags-drawer/TagBubble.scss +++ b/src/content-tags-drawer/TagBubble.scss @@ -9,9 +9,11 @@ color: $white; fill: $white; } + .pgn__chip__label { color: $white; } + background-color: $dark; border-color: $dark; } From debe73b467c3b269ce8160e0f012a68341a6cf97 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Thu, 23 Nov 2023 17:46:31 +0300 Subject: [PATCH 05/24] feat: "Load more" button to handle paginated tags --- .../ContentTagsDropDownSelector.jsx | 53 +++++++++++++++---- src/content-tags-drawer/messages.js | 4 ++ 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsDropDownSelector.jsx b/src/content-tags-drawer/ContentTagsDropDownSelector.jsx index a6c1485364..284377c044 100644 --- a/src/content-tags-drawer/ContentTagsDropDownSelector.jsx +++ b/src/content-tags-drawer/ContentTagsDropDownSelector.jsx @@ -1,10 +1,11 @@ -import React, { useState } from 'react'; +import React, { useState, useEffect, useCallback } from 'react'; import { SelectableBox, Icon, Spinner, + Button, } from '@edx/paragon'; -import { useIntl } from '@edx/frontend-platform/i18n'; +import { useIntl, FormattedMessage } from '@edx/frontend-platform/i18n'; import { ArrowDropDown, ArrowDropUp } from '@edx/paragon/icons'; import PropTypes from 'prop-types'; import messages from './messages'; @@ -21,6 +22,15 @@ const ContentTagsDropDownSelector = ({ // the value true (open) false (closed) const [dropdownStates, setDropdownStates] = useState({}); + const [tags, setTags] = useState([]); + const [nextPage, setNextPage] = useState(null); + + // `fetchUrl` is initially `subTagsUrl` to fetch the initial data, + // however if it is null that means it is the root, and the apiHooks + // would automatically handle it. Later this url is set to the next + // page of results (if any) + const [fetchUrl, setFetchUrl] = useState(subTagsUrl); + const isOpen = (i) => dropdownStates[i]; const clickAndEnterHandler = (i) => { @@ -29,8 +39,8 @@ const ContentTagsDropDownSelector = ({ setDropdownStates({ ...dropdownStates, [i]: !dropdownStates[i] }); }; - const taxonomyTagsData = useTaxonomyTagsDataResponse(taxonomyId, subTagsUrl); - const isTaxonomyTagsLoaded = useIsTaxonomyTagsDataLoaded(taxonomyId, subTagsUrl); + const taxonomyTagsData = useTaxonomyTagsDataResponse(taxonomyId, fetchUrl); + const isTaxonomyTagsLoaded = useIsTaxonomyTagsDataLoaded(taxonomyId, fetchUrl); const isImplicit = (tag) => { // Traverse the tags tree using the lineage @@ -43,9 +53,20 @@ const ContentTagsDropDownSelector = ({ return (traversal[tag.value] && !traversal[tag.value].explicit) || false; }; + useEffect(() => { + if (isTaxonomyTagsLoaded && taxonomyTagsData) { + setTags([...tags, ...taxonomyTagsData.results]); + setNextPage(taxonomyTagsData.next); + } + }, [isTaxonomyTagsLoaded, taxonomyTagsData]); + + const loadMoreTags = useCallback(() => { + setFetchUrl(nextPage); + }, [nextPage]); + return ( - isTaxonomyTagsLoaded && taxonomyTagsData - ? taxonomyTagsData.results.map((taxonomyTag, i) => ( + <> + {tags.map((taxonomyTag, i) => (
- )) - : ( + ))} + + { nextPage && isTaxonomyTagsLoaded + ? ( + + ) + : null} + + { !isTaxonomyTagsLoaded ? (
- ) + ) : null} + ); }; diff --git a/src/content-tags-drawer/messages.js b/src/content-tags-drawer/messages.js index 6203bb2e83..2fe9deb994 100644 --- a/src/content-tags-drawer/messages.js +++ b/src/content-tags-drawer/messages.js @@ -17,6 +17,10 @@ const messages = defineMessages({ id: 'course-authoring.content-tags-drawer.tags-dropdown-selector.spinner.loading', defaultMessage: 'Loading tags', }, + loadMoreTagsButtonText: { + id: 'course-authoring.content-tags-drawer.tags-dropdown-selector.load-more-tags.button', + defaultMessage: 'Load more', + }, taxonomyTagsAriaLabel: { id: 'course-authoring.content-tags-drawer.content-tags-collapsible.selectable-box.selection.aria.label', defaultMessage: 'taxonomy tags selection', From c5165f22d7c7025658dae6ba2ab609d98d3ef670 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Thu, 23 Nov 2023 18:14:31 +0300 Subject: [PATCH 06/24] fix: Remove arrow from tags dropdown modal --- src/content-tags-drawer/ContentTagsCollapsible.jsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx index 913e0b12a9..c422aab423 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx @@ -294,7 +294,6 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => {
Date: Fri, 24 Nov 2023 14:52:40 +0300 Subject: [PATCH 07/24] refactor: Move util functions outside component --- .../ContentTagsCollapsible.jsx | 119 ++++++++++-------- 1 file changed, 67 insertions(+), 52 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx index c422aab423..abc0108706 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx @@ -20,6 +20,70 @@ import ContentTagsTree from './ContentTagsTree'; import { useContentTaxonomyTagsMutation } from './data/apiHooks'; +/** + * Util function that consolidates two tag trees into one, sorting the keys in + * alphabetical order. + * + * @param {object} tree1 - first tag tree + * @param {object} tree2 - second tag tree + * @returns {object} merged tree containing both tree1 and tree2 + */ +const mergeTrees = (tree1, tree2) => { + const mergedTree = { ...tree1 }; + + const sortKeysAlphabetically = (obj) => { + const sortedObj = {}; + Object.keys(obj) + .sort() + .forEach((key) => { + sortedObj[key] = obj[key]; + if (obj[key] && typeof obj[key] === 'object') { + sortedObj[key].children = sortKeysAlphabetically(obj[key].children); + } + }); + return sortedObj; + }; + + const mergeRecursively = (destination, source) => { + Object.entries(source).forEach(([key, sourceValue]) => { + const destinationValue = destination[key]; + + if (destinationValue && sourceValue && typeof destinationValue === 'object' && typeof sourceValue === 'object') { + mergeRecursively(destinationValue, sourceValue); + } else { + // eslint-disable-next-line no-param-reassign + destination[key] = sourceValue; + } + }); + }; + + mergeRecursively(mergedTree, tree2); + return sortKeysAlphabetically(mergedTree); +}; + +/** + * Util function that removes the tag along with it's ancestors if it was + * the only explicit child tag. + * + * @param {object} tree - tag tree to remove the tag from + * @param {string[]} tagsToRemove - full lineage of tag to remove. + * eg: ['grand parent', 'parent', 'tag'] + */ +const removeTags = (tree, tagsToRemove) => { + if (!tree || !tagsToRemove.length) { + return; + } + const key = tagsToRemove[0]; + if (tree[key]) { + removeTags(tree[key].children, tagsToRemove.slice(1)); + + if (Object.keys(tree[key].children).length === 0 && (tree[key].explicit === false || tagsToRemove.length === 1)) { + // eslint-disable-next-line no-param-reassign + delete tree[key]; + } + } +}; + /** * Collapsible component that holds a Taxonomy along with Tags that belong to it. * This includes both applied tags and tags that are available to select @@ -142,39 +206,6 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { } }, [contentId, id, checkedTags]); - const mergeTrees = (tree1, tree2) => { - const mergedTree = { ...tree1 }; - - const sortKeysAlphabetically = (obj) => { - const sortedObj = {}; - Object.keys(obj) - .sort() - .forEach((key) => { - sortedObj[key] = obj[key]; - if (obj[key] && typeof obj[key] === 'object') { - sortedObj[key].children = sortKeysAlphabetically(obj[key].children); - } - }); - return sortedObj; - }; - - const mergeRecursively = (destination, source) => { - Object.entries(source).forEach(([key, sourceValue]) => { - const destinationValue = destination[key]; - - if (destinationValue && sourceValue && typeof destinationValue === 'object' && typeof sourceValue === 'object') { - mergeRecursively(destinationValue, sourceValue); - } else { - // eslint-disable-next-line no-param-reassign - destination[key] = sourceValue; - } - }); - }; - - mergeRecursively(mergedTree, tree2); - return sortKeysAlphabetically(mergedTree); - }; - // This converts the contentTags prop to the tree structure mentioned above React.useMemo(() => { const resultTree = {}; @@ -203,7 +234,7 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { setAppliedContentTags(resultTree); }, [contentTags]); - // This is out source of truth that represents the current state of tags in + // This is the source of truth that represents the current state of tags in // this Taxonomy as a tree. Whenever either the `appliedContentTags` (i.e. tags passed in // the prop from the backed) change, or when the `addedContentTags` (i.e. tags added by // selecting/unselecting them in the dropdown) change, the tree is recomputed. @@ -235,23 +266,7 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { }); }; - // Remove the tag along with it's ancestors if it was the only explicit child tag - const removeTags = (tree, tagsToRemove) => { - if (!tree || !tagsToRemove.length) { - return; - } - const key = tagsToRemove[0]; - if (tree[key]) { - removeTags(tree[key].children, tagsToRemove.slice(1)); - - if (Object.keys(tree[key].children).length === 0 && (tree[key].explicit === false || tagsToRemove.length === 1)) { - // eslint-disable-next-line no-param-reassign - delete tree[key]; - } - } - }; - - const tagChangeHandler = (tagSelectableBoxValue, checked) => { + const tagChangeHandler = React.useCallback((tagSelectableBoxValue, checked) => { const tagLineage = tagSelectableBoxValue.split(',').map(t => decodeURIComponent(t)); const selectedTag = tagLineage.slice(-1)[0]; @@ -271,7 +286,7 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { setAddedContentTags(addedTree); setUpdatingTags(true); - }; + }); const handleSelectableBoxChange = React.useCallback((e) => { tagChangeHandler(e.target.value, e.target.checked); From ec2c842656454f958b5af394fd13754cb3e4bbc8 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 24 Nov 2023 14:59:59 +0300 Subject: [PATCH 08/24] fix: Add missing `contentId` to queryKey --- src/content-tags-drawer/data/apiHooks.jsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/content-tags-drawer/data/apiHooks.jsx b/src/content-tags-drawer/data/apiHooks.jsx index 3f6d5fa071..d646f71340 100644 --- a/src/content-tags-drawer/data/apiHooks.jsx +++ b/src/content-tags-drawer/data/apiHooks.jsx @@ -54,7 +54,7 @@ export const useIsTaxonomyTagsDataLoaded = (taxonomyId, fullPathProvided) => ( */ const useContentTaxonomyTagsData = (contentId) => ( useQuery({ - queryKey: ['contentTaxonomyTags'], + queryKey: ['contentTaxonomyTags', contentId], queryFn: () => getContentTaxonomyTagsData(contentId), }) ); @@ -88,7 +88,7 @@ export const useIsContentTaxonomyTagsDataLoaded = (contentId) => ( */ const useContentData = (contentId) => ( useQuery({ - queryKey: ['contentData'], + queryKey: ['contentData', contentId], queryFn: () => getContentData(contentId), }) ); From bdc8d3026150400e6d7b081e71e61180b83d5ff3 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 24 Nov 2023 16:15:13 +0300 Subject: [PATCH 09/24] refactor: Simplify react-query hooks + fix types --- src/content-tags-drawer/ContentTagsDrawer.jsx | 25 +--- .../ContentTagsDropDownSelector.jsx | 5 +- src/content-tags-drawer/data/api.js | 2 +- src/content-tags-drawer/data/apiHooks.jsx | 90 ++---------- .../data/apiHooks.test.jsx | 138 +++++++----------- src/content-tags-drawer/data/types.mjs | 5 - 6 files changed, 78 insertions(+), 187 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsDrawer.jsx b/src/content-tags-drawer/ContentTagsDrawer.jsx index 49335c4e77..40ae170617 100644 --- a/src/content-tags-drawer/ContentTagsDrawer.jsx +++ b/src/content-tags-drawer/ContentTagsDrawer.jsx @@ -10,10 +10,8 @@ import messages from './messages'; import ContentTagsCollapsible from './ContentTagsCollapsible'; import { extractOrgFromContentId } from './utils'; import { - useContentTaxonomyTagsDataResponse, - useIsContentTaxonomyTagsDataLoaded, - useContentDataResponse, - useIsContentDataLoaded, + useContentTaxonomyTagsData, + useContentData, } from './data/apiHooks'; import { useTaxonomyListDataResponse, useIsTaxonomyListDataLoaded } from '../taxonomy/data/apiHooks'; import Loading from '../generic/Loading'; @@ -24,26 +22,17 @@ const ContentTagsDrawer = () => { const org = extractOrgFromContentId(contentId); - const useContentData = () => { - const contentData = useContentDataResponse(contentId); - const isContentDataLoaded = useIsContentDataLoaded(contentId); - return { contentData, isContentDataLoaded }; - }; - - const useContentTaxonomyTagsData = () => { - const contentTaxonomyTagsData = useContentTaxonomyTagsDataResponse(contentId); - const isContentTaxonomyTagsLoaded = useIsContentTaxonomyTagsDataLoaded(contentId); - return { contentTaxonomyTagsData, isContentTaxonomyTagsLoaded }; - }; - const useTaxonomyListData = () => { const taxonomyListData = useTaxonomyListDataResponse(org); const isTaxonomyListLoaded = useIsTaxonomyListDataLoaded(org); return { taxonomyListData, isTaxonomyListLoaded }; }; - const { contentData, isContentDataLoaded } = useContentData(); - const { contentTaxonomyTagsData, isContentTaxonomyTagsLoaded } = useContentTaxonomyTagsData(); + const { data: contentData, isSuccess: isContentDataLoaded } = useContentData(contentId); + const { + data: contentTaxonomyTagsData, + isSuccess: isContentTaxonomyTagsLoaded, + } = useContentTaxonomyTagsData(contentId); const { taxonomyListData, isTaxonomyListLoaded } = useTaxonomyListData(); const closeContentTagsDrawer = () => { diff --git a/src/content-tags-drawer/ContentTagsDropDownSelector.jsx b/src/content-tags-drawer/ContentTagsDropDownSelector.jsx index 284377c044..c26edc1aef 100644 --- a/src/content-tags-drawer/ContentTagsDropDownSelector.jsx +++ b/src/content-tags-drawer/ContentTagsDropDownSelector.jsx @@ -11,7 +11,7 @@ import PropTypes from 'prop-types'; import messages from './messages'; import './ContentTagsDropDownSelector.scss'; -import { useTaxonomyTagsDataResponse, useIsTaxonomyTagsDataLoaded } from './data/apiHooks'; +import { useTaxonomyTagsData } from './data/apiHooks'; const ContentTagsDropDownSelector = ({ taxonomyId, level, subTagsUrl, lineage, tagsTree, @@ -39,8 +39,7 @@ const ContentTagsDropDownSelector = ({ setDropdownStates({ ...dropdownStates, [i]: !dropdownStates[i] }); }; - const taxonomyTagsData = useTaxonomyTagsDataResponse(taxonomyId, fetchUrl); - const isTaxonomyTagsLoaded = useIsTaxonomyTagsDataLoaded(taxonomyId, fetchUrl); + const { data: taxonomyTagsData, isSuccess: isTaxonomyTagsLoaded } = useTaxonomyTagsData(taxonomyId, fetchUrl); const isImplicit = (tag) => { // Traverse the tags tree using the lineage diff --git a/src/content-tags-drawer/data/api.js b/src/content-tags-drawer/data/api.js index d18a17f875..96db8acd8f 100644 --- a/src/content-tags-drawer/data/api.js +++ b/src/content-tags-drawer/data/api.js @@ -44,7 +44,7 @@ export async function getContentData(contentId) { /** * Update content object's applied tags * @param {string} contentId The id of the content object (unit/component) - * @param {string} taxonomyId The id of the taxonomy the tags belong to + * @param {number} taxonomyId The id of the taxonomy the tags belong to * @param {string[]} tags The list of tags (values) to set on content object * @returns {Promise} */ diff --git a/src/content-tags-drawer/data/apiHooks.jsx b/src/content-tags-drawer/data/apiHooks.jsx index d646f71340..679f17f2a4 100644 --- a/src/content-tags-drawer/data/apiHooks.jsx +++ b/src/content-tags-drawer/data/apiHooks.jsx @@ -9,117 +9,51 @@ import { /** * Builds the query to get the taxonomy tags - * @param {string} taxonomyId The id of the taxonomy to fetch tags for + * @param {number} taxonomyId The id of the taxonomy to fetch tags for * @param {string} fullPathProvided Optional param that contains the full URL to fetch data * If provided, we use it instead of generating the URL. This is usually for fetching subTags - * @returns {import("./types.mjs").UseQueryResult} + * @returns {import("@tanstack/react-query").UseQueryResult} */ -const useTaxonomyTagsData = (taxonomyId, fullPathProvided) => ( +export const useTaxonomyTagsData = (taxonomyId, fullPathProvided) => ( useQuery({ queryKey: [`taxonomyTags${ fullPathProvided || taxonomyId }`], queryFn: () => getTaxonomyTagsData(taxonomyId, fullPathProvided), }) ); -/** - * Gets the taxonomy tags data - * @param {string} taxonomyId The id of the taxonomy to fetch tags for - * @param {string} fullPathProvided Optional param that contains the full URL to fetch data - * If provided, we use it instead of generating the URL. This is usually for fetching subTags - * @returns {import("./types.mjs").TaxonomyTagsData | undefined} - */ -export const useTaxonomyTagsDataResponse = (taxonomyId, fullPathProvided) => { - const response = useTaxonomyTagsData(taxonomyId, fullPathProvided); - if (response.status === 'success') { - return response.data; - } - return undefined; -}; - -/** - * Returns the status of the taxonomy tags query - * @param {string} taxonomyId The id of the taxonomy to fetch tags for - * @param {string} fullPathProvided Optional param that contains the full URL to fetch data - * If provided, we use it instead of generating the URL. This is usually for fetching subTags - * @returns {boolean} - */ -export const useIsTaxonomyTagsDataLoaded = (taxonomyId, fullPathProvided) => ( - useTaxonomyTagsData(taxonomyId, fullPathProvided).status === 'success' -); - /** * Builds the query to get the taxonomy tags applied to the content object * @param {string} contentId The id of the content object to fetch the applied tags for - * @returns {import("./types.mjs").UseQueryResult} + * @returns {import("@tanstack/react-query").UseQueryResult} */ -const useContentTaxonomyTagsData = (contentId) => ( +export const useContentTaxonomyTagsData = (contentId) => ( useQuery({ queryKey: ['contentTaxonomyTags', contentId], queryFn: () => getContentTaxonomyTagsData(contentId), }) ); -/** - * Gets the taxonomy tags applied to the content object - * @param {string} contentId The id of the content object to fetch the applied tags for - * @returns {import("./types.mjs").ContentTaxonomyTagsData | undefined} - */ -export const useContentTaxonomyTagsDataResponse = (contentId) => { - const response = useContentTaxonomyTagsData(contentId); - if (response.status === 'success') { - return response.data; - } - return undefined; -}; - -/** - * Gets the status of the content taxonomy tags query - * @param {string} contentId The id of the content object to fetch the applied tags for - * @returns {boolean} - */ -export const useIsContentTaxonomyTagsDataLoaded = (contentId) => ( - useContentTaxonomyTagsData(contentId).status === 'success' -); - /** * Builds the query to get meta data about the content object * @param {string} contentId The id of the content object (unit/component) - * @returns {import("./types.mjs").UseQueryResult} + * @returns {import("@tanstack/react-query").UseQueryResult} */ -const useContentData = (contentId) => ( +export const useContentData = (contentId) => ( useQuery({ queryKey: ['contentData', contentId], queryFn: () => getContentData(contentId), }) ); -/** - * Gets the information about the content object - * @param {string} contentId The id of the content object (unit/component) - * @returns {import("./types.mjs").ContentData | undefined} - */ -export const useContentDataResponse = (contentId) => { - const response = useContentData(contentId); - if (response.status === 'success') { - return response.data; - } - return undefined; -}; - -/** - * Gets the status of the content data query - * @param {string} contentId The id of the content object (unit/component) - * @returns {boolean} - */ -export const useIsContentDataLoaded = (contentId) => ( - useContentData(contentId).status === 'success' -); - /** * Builds the mutation to update the tags applied to the content object - * @returns {import("./types.mjs").UseMutationResult} */ export const useContentTaxonomyTagsMutation = () => ( + /** + * @type { + * import("@tanstack/react-query").MutateFunction + * } + */ useMutation({ mutationFn: ({ contentId, taxonomyId, tags }) => updateContentTaxonomyTags(contentId, taxonomyId, tags), }) diff --git a/src/content-tags-drawer/data/apiHooks.test.jsx b/src/content-tags-drawer/data/apiHooks.test.jsx index f969782adc..79f701e73f 100644 --- a/src/content-tags-drawer/data/apiHooks.test.jsx +++ b/src/content-tags-drawer/data/apiHooks.test.jsx @@ -1,121 +1,95 @@ -import { useQuery } from '@tanstack/react-query'; +import { useQuery, useMutation } from '@tanstack/react-query'; +import { act } from '@testing-library/react'; import { - useTaxonomyTagsDataResponse, - useIsTaxonomyTagsDataLoaded, - useContentTaxonomyTagsDataResponse, - useIsContentTaxonomyTagsDataLoaded, - useContentDataResponse, - useIsContentDataLoaded, + useTaxonomyTagsData, + useContentTaxonomyTagsData, + useContentData, + useContentTaxonomyTagsMutation, } from './apiHooks'; +import { updateContentTaxonomyTags } from './api'; + jest.mock('@tanstack/react-query', () => ({ useQuery: jest.fn(), + useMutation: jest.fn(), })); -describe('useTaxonomyTagsDataResponse', () => { - it('should return data when status is success', () => { - useQuery.mockReturnValueOnce({ status: 'success', data: { data: 'data' } }); - const taxonomyId = '123'; - const result = useTaxonomyTagsDataResponse(taxonomyId); - - expect(result).toEqual({ data: 'data' }); - }); - - it('should return undefined when status is not success', () => { - useQuery.mockReturnValueOnce({ status: 'error' }); - const taxonomyId = '123'; - const result = useTaxonomyTagsDataResponse(taxonomyId); - - expect(result).toBeUndefined(); - }); -}); +jest.mock('./api', () => ({ + updateContentTaxonomyTags: jest.fn(), +})); -describe('useIsTaxonomyTagsDataLoaded', () => { - it('should return true when status is success', () => { - useQuery.mockReturnValueOnce({ status: 'success' }); - const taxonomyId = '123'; - const result = useIsTaxonomyTagsDataLoaded(taxonomyId); +describe('useTaxonomyTagsData', () => { + it('should return success response', () => { + useQuery.mockReturnValueOnce({ isSuccess: true, data: 'data' }); + const taxonomyId = 123; + const result = useTaxonomyTagsData(taxonomyId); - expect(result).toBe(true); + expect(result).toEqual({ isSuccess: true, data: 'data' }); }); - it('should return false when status is not success', () => { - useQuery.mockReturnValueOnce({ status: 'error' }); - const taxonomyId = '123'; - const result = useIsTaxonomyTagsDataLoaded(taxonomyId); + it('should return failure response', () => { + useQuery.mockReturnValueOnce({ isSuccess: false }); + const taxonomyId = 123; + const result = useTaxonomyTagsData(taxonomyId); - expect(result).toBe(false); + expect(result).toEqual({ isSuccess: false }); }); }); -describe('useContentTaxonomyTagsDataResponse', () => { - it('should return data when status is success', () => { - useQuery.mockReturnValueOnce({ status: 'success', data: { data: 'data' } }); +describe('useContentTaxonomyTagsData', () => { + it('should return success response', () => { + useQuery.mockReturnValueOnce({ isSuccess: true, data: 'data' }); const contentId = '123'; - const result = useContentTaxonomyTagsDataResponse(contentId); + const result = useContentTaxonomyTagsData(contentId); - expect(result).toEqual({ data: 'data' }); + expect(result).toEqual({ isSuccess: true, data: 'data' }); }); - it('should return undefined when status is not success', () => { - useQuery.mockReturnValueOnce({ status: 'error' }); + it('should return failure response', () => { + useQuery.mockReturnValueOnce({ isSuccess: false }); const contentId = '123'; - const result = useContentTaxonomyTagsDataResponse(contentId); + const result = useContentTaxonomyTagsData(contentId); - expect(result).toBeUndefined(); + expect(result).toEqual({ isSuccess: false }); }); }); -describe('useIsContentTaxonomyTagsDataLoaded', () => { - it('should return true when status is success', () => { - useQuery.mockReturnValueOnce({ status: 'success' }); +describe('useContentData', () => { + it('should return success response', () => { + useQuery.mockReturnValueOnce({ isSuccess: true, data: 'data' }); const contentId = '123'; - const result = useIsContentTaxonomyTagsDataLoaded(contentId); + const result = useContentData(contentId); - expect(result).toBe(true); + expect(result).toEqual({ isSuccess: true, data: 'data' }); }); - it('should return false when status is not success', () => { - useQuery.mockReturnValueOnce({ status: 'error' }); + it('should return failure response', () => { + useQuery.mockReturnValueOnce({ isSuccess: false }); const contentId = '123'; - const result = useIsContentTaxonomyTagsDataLoaded(contentId); + const result = useContentData(contentId); - expect(result).toBe(false); + expect(result).toEqual({ isSuccess: false }); }); }); -describe('useContentDataResponse', () => { - it('should return data when status is success', () => { - useQuery.mockReturnValueOnce({ status: 'success', data: { data: 'data' } }); - const contentId = '123'; - const result = useContentDataResponse(contentId); +describe('useContentTaxonomyTagsMutation', () => { + it('should call the update content taxonomy tags function', async () => { + useMutation.mockReturnValueOnce({ mutate: jest.fn() }); - expect(result).toEqual({ data: 'data' }); - }); + const mutation = useContentTaxonomyTagsMutation(); + mutation.mutate(); - it('should return undefined when status is not success', () => { - useQuery.mockReturnValueOnce({ status: 'error' }); - const contentId = '123'; - const result = useContentDataResponse(contentId); + expect(useMutation).toBeCalled(); - expect(result).toBeUndefined(); - }); -}); - -describe('useIsContentDataLoaded', () => { - it('should return true when status is success', () => { - useQuery.mockReturnValueOnce({ status: 'success' }); - const contentId = '123'; - const result = useIsContentDataLoaded(contentId); - - expect(result).toBe(true); - }); - - it('should return false when status is not success', () => { - useQuery.mockReturnValueOnce({ status: 'error' }); - const contentId = '123'; - const result = useIsContentDataLoaded(contentId); + const [config] = useMutation.mock.calls[0]; + const { mutationFn } = config; - expect(result).toBe(false); + await act(async () => { + const contentId = 'testerContent'; + const taxonomyId = 123; + const tags = ['tag1', 'tag2']; + await mutationFn({ contentId, taxonomyId, tags }); + expect(updateContentTaxonomyTags).toBeCalledWith(contentId, taxonomyId, tags); + }); }); }); diff --git a/src/content-tags-drawer/data/types.mjs b/src/content-tags-drawer/data/types.mjs index 78fcf9598d..00b3fefd4c 100644 --- a/src/content-tags-drawer/data/types.mjs +++ b/src/content-tags-drawer/data/types.mjs @@ -105,8 +105,3 @@ * @property {Object} data * @property {string} status */ - -/** - * @typedef {Object} UseMutationResult - * @property {(variables: {Object}, { onSuccess, onSettled, onError }) => void} mutate - */ From ae34667591d9cd289f43c491e6b96baa20bfee4b Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Fri, 24 Nov 2023 19:04:07 +0300 Subject: [PATCH 10/24] feat: Invalidate query refetch content tags count --- .../ContentTagsCollapsible.jsx | 4 ++-- src/content-tags-drawer/data/apiHooks.jsx | 17 +++++++++++------ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx index abc0108706..2faf59b830 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx @@ -178,7 +178,7 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { // State to determine whether to update the backend or not const [updatingTags, setUpdatingTags] = React.useState(false); - const mutation = useContentTaxonomyTagsMutation(); + const mutation = useContentTaxonomyTagsMutation(contentId, id); const [isOpen, open, close] = useToggle(false); const [target, setTarget] = React.useState(null); @@ -202,7 +202,7 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { if (updatingTags) { setUpdatingTags(false); const tags = checkedTags.map(t => decodeURIComponent(t.split(',').slice(-1))); - mutation.mutate({ contentId, taxonomyId: id, tags }); + mutation.mutate({ tags }); } }, [contentId, id, checkedTags]); diff --git a/src/content-tags-drawer/data/apiHooks.jsx b/src/content-tags-drawer/data/apiHooks.jsx index 679f17f2a4..dfb02d81c8 100644 --- a/src/content-tags-drawer/data/apiHooks.jsx +++ b/src/content-tags-drawer/data/apiHooks.jsx @@ -1,5 +1,5 @@ // @ts-check -import { useQuery, useMutation } from '@tanstack/react-query'; +import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { getTaxonomyTagsData, getContentTaxonomyTagsData, @@ -48,13 +48,18 @@ export const useContentData = (contentId) => ( /** * Builds the mutation to update the tags applied to the content object */ -export const useContentTaxonomyTagsMutation = () => ( +export const useContentTaxonomyTagsMutation = (contentId, taxonomyId) => { + const queryClient = useQueryClient(); + /** * @type { * import("@tanstack/react-query").MutateFunction * } */ - useMutation({ - mutationFn: ({ contentId, taxonomyId, tags }) => updateContentTaxonomyTags(contentId, taxonomyId, tags), - }) -); + return useMutation({ + mutationFn: ({ tags }) => updateContentTaxonomyTags(contentId, taxonomyId, tags), + onSettled: () => { + queryClient.invalidateQueries({ queryKey: ['contentTaxonomyTags', contentId] }); + }, + }); +}; From 9592d1135d716584b66ac4b9b4378f6c023ddd8e Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Sun, 26 Nov 2023 15:07:40 +0300 Subject: [PATCH 11/24] feat: Add `editable` flag to ContentTagsDrawer Currently set to always be editable. Need to implement proper permission checks to determine whether user has access to readonly/editable drawer. --- .../ContentTagsCollapsible.jsx | 23 +++++++++++-------- src/content-tags-drawer/ContentTagsDrawer.jsx | 3 ++- src/content-tags-drawer/ContentTagsTree.jsx | 6 ++++- src/content-tags-drawer/TagBubble.jsx | 7 +++--- 4 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx index 2faf59b830..695d3f01b6 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx @@ -169,8 +169,9 @@ const removeTags = (tree, tagsToRemove) => { * @param {Object[]} taxonomyAndTagsData.contentTags - Array of taxonomy tags that are applied to the content * @param {string} taxonomyAndTagsData.contentTags.value - Value of applied Tag * @param {string} taxonomyAndTagsData.contentTags.lineage - Array of Tag's ancestors sorted (ancestor -> tag) + * @param {boolean} editable - Whether the tags can be edited */ -const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => { +const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) => { const intl = useIntl(); const { id, name, contentTags, @@ -296,17 +297,20 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData }) => {
- +
- + + {editable && ( + + )}
{ { isTaxonomyListLoaded && isContentTaxonomyTagsLoaded ? taxonomies.map((data) => (
- + {/* TODO: Properly set whether tags should be editable or not based on permissions */} +
)) diff --git a/src/content-tags-drawer/ContentTagsTree.jsx b/src/content-tags-drawer/ContentTagsTree.jsx index 6a71ee1f08..08ae7e145f 100644 --- a/src/content-tags-drawer/ContentTagsTree.jsx +++ b/src/content-tags-drawer/ContentTagsTree.jsx @@ -35,8 +35,10 @@ import TagBubble from './TagBubble'; * }; * * @param {Object} tagsTree - Array of taxonomy tags that are applied to the content + * @param {Func} removeTagHandler - Function that is called when removing tags from tree + * @param {boolean} editable - Whether the tags appear with an 'x' allowing the user to remove them */ -const ContentTagsTree = ({ tagsTree, removeTagHandler }) => { +const ContentTagsTree = ({ tagsTree, removeTagHandler, editable }) => { const renderTagsTree = (tag, level, lineage) => Object.keys(tag).map((key) => { const updatedLineage = [...lineage, encodeURIComponent(key)]; if (tag[key] !== undefined) { @@ -49,6 +51,7 @@ const ContentTagsTree = ({ tagsTree, removeTagHandler }) => { level={level} lineage={updatedLineage} removeTagHandler={removeTagHandler} + editable={editable} /> { renderTagsTree(tag[key].children, level + 1, updatedLineage) }
@@ -68,6 +71,7 @@ ContentTagsTree.propTypes = { }).isRequired, ).isRequired, removeTagHandler: PropTypes.func.isRequired, + editable: PropTypes.bool.isRequired, }; export default ContentTagsTree; diff --git a/src/content-tags-drawer/TagBubble.jsx b/src/content-tags-drawer/TagBubble.jsx index 84a0cfbe55..c4cefee60e 100644 --- a/src/content-tags-drawer/TagBubble.jsx +++ b/src/content-tags-drawer/TagBubble.jsx @@ -8,12 +8,12 @@ import PropTypes from 'prop-types'; import TagOutlineIcon from './TagOutlineIcon'; const TagBubble = ({ - value, implicit, level, lineage, removeTagHandler, + value, implicit, level, lineage, removeTagHandler, editable, }) => { const className = `tag-bubble mb-2 ${implicit ? 'implicit' : ''}`; const handleClick = React.useCallback(() => { - if (!implicit) { + if (!implicit && editable) { removeTagHandler(lineage.join(','), false); } }, [implicit, lineage]); @@ -24,7 +24,7 @@ const TagBubble = ({ className={className} variant="light" iconBefore={!implicit ? Tag : TagOutlineIcon} - iconAfter={!implicit ? Close : null} + iconAfter={!implicit && editable ? Close : null} onIconAfterClick={handleClick} > {value} @@ -44,6 +44,7 @@ TagBubble.propTypes = { level: PropTypes.number, lineage: PropTypes.arrayOf(PropTypes.string).isRequired, removeTagHandler: PropTypes.func.isRequired, + editable: PropTypes.bool.isRequired, }; export default TagBubble; From 313728b5687a044e46a968807c3bfc320eb5f49e Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Sun, 26 Nov 2023 16:07:47 +0300 Subject: [PATCH 12/24] fix: Clear checkboxes when mutation error This is because we invalidate the query when the mutation fails (or succeeds) so we get the latest applied tags from the backend. So we clear the checkboxes and applied tags to reflect the latest state from the backend. --- .../ContentTagsCollapsible.jsx | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx index 695d3f01b6..6e3fe11336 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx @@ -193,7 +193,7 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) => const [addedContentTags, setAddedContentTags] = React.useState({}); // To handle checking/unchecking tags in the SelectableBox - const [checkedTags, { add, remove }] = useCheckboxSetValues(); + const [checkedTags, { add, remove, clear }] = useCheckboxSetValues(); // Handles make requests to the backend whenever the checked tags change React.useEffect(() => { @@ -209,8 +209,18 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) => // This converts the contentTags prop to the tree structure mentioned above React.useMemo(() => { - const resultTree = {}; + // Clear all the tags that have not been commited and the checked boxes when + // fresh contentTags passed in so the latest state from the backend is rendered + setAddedContentTags({}); + clear(); + + // When an error occurs while updating, the contentTags query is invalidated, + // hence they will be recalculated, and the mutation should be reset. + if (mutation.isError) { + mutation.reset(); + } + const resultTree = {}; contentTags.forEach(item => { let currentLevel = resultTree; @@ -233,7 +243,7 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) => }); setAppliedContentTags(resultTree); - }, [contentTags]); + }, [contentTags, setAddedContentTags, mutation.isError]); // This is the source of truth that represents the current state of tags in // this Taxonomy as a tree. Whenever either the `appliedContentTags` (i.e. tags passed in From 92713389956ed90bb4febf822da514217a6f35dd Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Mon, 27 Nov 2023 15:47:26 +0300 Subject: [PATCH 13/24] fix: Fix typing issues with mutations --- src/content-tags-drawer/data/api.js | 2 +- src/content-tags-drawer/data/apiHooks.jsx | 16 +++++++++++----- src/content-tags-drawer/data/apiHooks.test.jsx | 2 +- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/content-tags-drawer/data/api.js b/src/content-tags-drawer/data/api.js index 96db8acd8f..20c4a57295 100644 --- a/src/content-tags-drawer/data/api.js +++ b/src/content-tags-drawer/data/api.js @@ -9,7 +9,7 @@ export const getContentDataApiUrl = (contentId) => new URL(`/xblock/outline/${co /** * Get all tags that belong to taxonomy. - * @param {string} taxonomyId The id of the taxonomy to fetch tags for + * @param {number} taxonomyId The id of the taxonomy to fetch tags for * @param {string} fullPathProvided Optional param that contains the full URL to fetch data * If provided, we use it instead of generating the URL. This is usually for fetching subTags * @returns {Promise} diff --git a/src/content-tags-drawer/data/apiHooks.jsx b/src/content-tags-drawer/data/apiHooks.jsx index dfb02d81c8..24d0bf2211 100644 --- a/src/content-tags-drawer/data/apiHooks.jsx +++ b/src/content-tags-drawer/data/apiHooks.jsx @@ -47,16 +47,22 @@ export const useContentData = (contentId) => ( /** * Builds the mutation to update the tags applied to the content object + * @param {string} contentId The id of the content object to update tags for + * @param {number} taxonomyId The id of the taxonomy the tags belong to */ export const useContentTaxonomyTagsMutation = (contentId, taxonomyId) => { const queryClient = useQueryClient(); - /** - * @type { - * import("@tanstack/react-query").MutateFunction - * } - */ return useMutation({ + /** + * @type {import("@tanstack/react-query").MutateFunction< + * any, + * any, + * { + * tags: string[] + * } + * >} + */ mutationFn: ({ tags }) => updateContentTaxonomyTags(contentId, taxonomyId, tags), onSettled: () => { queryClient.invalidateQueries({ queryKey: ['contentTaxonomyTags', contentId] }); diff --git a/src/content-tags-drawer/data/apiHooks.test.jsx b/src/content-tags-drawer/data/apiHooks.test.jsx index 79f701e73f..61f353f31f 100644 --- a/src/content-tags-drawer/data/apiHooks.test.jsx +++ b/src/content-tags-drawer/data/apiHooks.test.jsx @@ -77,7 +77,7 @@ describe('useContentTaxonomyTagsMutation', () => { useMutation.mockReturnValueOnce({ mutate: jest.fn() }); const mutation = useContentTaxonomyTagsMutation(); - mutation.mutate(); + mutation.mutate({ tags: ['a', 'b', 'c'] }); expect(useMutation).toBeCalled(); From b9e9657ff80873132d2af7773c4b3c32e2641423 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Mon, 27 Nov 2023 18:15:52 +0300 Subject: [PATCH 14/24] test: Update tests for latest code --- .../ContentTagsCollapsible.test.jsx | 57 +++++++---- .../ContentTagsDrawer.test.jsx | 95 ++++++++++--------- .../ContentTagsDropDownSelector.test.jsx | 79 +++++++++------ .../ContentTagsTree.test.jsx | 59 +++++++----- src/content-tags-drawer/TagBubble.test.jsx | 50 ++++++---- .../data/apiHooks.test.jsx | 11 ++- 6 files changed, 210 insertions(+), 141 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.test.jsx b/src/content-tags-drawer/ContentTagsCollapsible.test.jsx index eca8c94bfc..537ece7127 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.test.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.test.jsx @@ -6,32 +6,38 @@ import PropTypes from 'prop-types'; import ContentTagsCollapsible from './ContentTagsCollapsible'; jest.mock('./data/apiHooks', () => ({ - useTaxonomyTagsDataResponse: jest.fn(), - useIsTaxonomyTagsDataLoaded: jest.fn(), + useContentTaxonomyTagsMutation: jest.fn(() => ({ + isError: false, + })), })); const data = { - id: 123, - name: 'Taxonomy 1', - contentTags: [ - { - value: 'Tag 1', - lineage: ['Tag 1'], - }, - { - value: 'Tag 2', - lineage: ['Tag 2'], - }, - ], + contentId: 'block-v1:SampleTaxonomyOrg1+STC1+2023_1+type@vertical+block@7f47fe2dbcaf47c5a071671c741fe1ab', + taxonomyAndTagsData: { + id: 123, + name: 'Taxonomy 1', + contentTags: [ + { + value: 'Tag 1', + lineage: ['Tag 1'], + }, + { + value: 'Tag 2', + lineage: ['Tag 2'], + }, + ], + }, + editable: true, }; -const ContentTagsCollapsibleComponent = ({ taxonomyAndTagsData }) => ( +const ContentTagsCollapsibleComponent = ({ contentId, taxonomyAndTagsData, editable }) => ( - + ); ContentTagsCollapsibleComponent.propTypes = { + contentId: PropTypes.string.isRequired, taxonomyAndTagsData: PropTypes.shape({ id: PropTypes.number, name: PropTypes.string, @@ -40,12 +46,19 @@ ContentTagsCollapsibleComponent.propTypes = { lineage: PropTypes.arrayOf(PropTypes.string), })), }).isRequired, + editable: PropTypes.bool.isRequired, }; describe('', () => { it('should render taxonomy tags data along content tags number badge', async () => { await act(async () => { - const { container, getByText } = render(); + const { container, getByText } = render( + , + ); expect(getByText('Taxonomy 1')).toBeInTheDocument(); expect(container.getElementsByClassName('badge').length).toBe(1); expect(getByText('2')).toBeInTheDocument(); @@ -53,9 +66,15 @@ describe('', () => { }); it('should render taxonomy tags data without tags number badge', async () => { - data.contentTags = []; + data.taxonomyAndTagsData.contentTags = []; await act(async () => { - const { container, getByText } = render(); + const { container, getByText } = render( + , + ); expect(getByText('Taxonomy 1')).toBeInTheDocument(); expect(container.getElementsByClassName('invisible').length).toBe(1); }); diff --git a/src/content-tags-drawer/ContentTagsDrawer.test.jsx b/src/content-tags-drawer/ContentTagsDrawer.test.jsx index ad479b1569..137e6f23f3 100644 --- a/src/content-tags-drawer/ContentTagsDrawer.test.jsx +++ b/src/content-tags-drawer/ContentTagsDrawer.test.jsx @@ -4,10 +4,8 @@ import { act, render, fireEvent } from '@testing-library/react'; import ContentTagsDrawer from './ContentTagsDrawer'; import { - useContentTaxonomyTagsDataResponse, - useIsContentTaxonomyTagsDataLoaded, - useContentDataResponse, - useIsContentDataLoaded, + useContentTaxonomyTagsData, + useContentData, } from './data/apiHooks'; import { useTaxonomyListDataResponse, useIsTaxonomyListDataLoaded } from '../taxonomy/data/apiHooks'; @@ -19,12 +17,17 @@ jest.mock('react-router-dom', () => ({ })); jest.mock('./data/apiHooks', () => ({ - useContentTaxonomyTagsDataResponse: jest.fn(), - useIsContentTaxonomyTagsDataLoaded: jest.fn(), - useContentDataResponse: jest.fn(), - useIsContentDataLoaded: jest.fn(), - useTaxonomyTagsDataResponse: jest.fn(), - useIsTaxonomyTagsDataLoaded: jest.fn(), + useContentTaxonomyTagsData: jest.fn(() => ({ + isSuccess: false, + data: {}, + })), + useContentData: jest.fn(() => ({ + isSuccess: false, + data: {}, + })), + useContentTaxonomyTagsMutation: jest.fn(() => ({ + isError: false, + })), })); jest.mock('../taxonomy/data/apiHooks', () => ({ @@ -45,7 +48,6 @@ describe('', () => { }); it('shows spinner before the content data query is complete', async () => { - useIsContentDataLoaded.mockReturnValue(false); await act(async () => { const { getAllByRole } = render(); const spinner = getAllByRole('status')[0]; @@ -55,7 +57,6 @@ describe('', () => { it('shows spinner before the taxonomy tags query is complete', async () => { useIsTaxonomyListDataLoaded.mockReturnValue(false); - useIsContentTaxonomyTagsDataLoaded.mockReturnValue(false); await act(async () => { const { getAllByRole } = render(); const spinner = getAllByRole('status')[1]; @@ -64,9 +65,11 @@ describe('', () => { }); it('shows the content display name after the query is complete', async () => { - useIsContentDataLoaded.mockReturnValue(true); - useContentDataResponse.mockReturnValue({ - displayName: 'Unit 1', + useContentData.mockReturnValue({ + isSuccess: true, + data: { + displayName: 'Unit 1', + }, }); await act(async () => { const { getByText } = render(); @@ -76,36 +79,38 @@ describe('', () => { it('shows the taxonomies data including tag numbers after the query is complete', async () => { useIsTaxonomyListDataLoaded.mockReturnValue(true); - useIsContentTaxonomyTagsDataLoaded.mockReturnValue(true); - useContentTaxonomyTagsDataResponse.mockReturnValue({ - taxonomies: [ - { - name: 'Taxonomy 1', - taxonomyId: 123, - editable: true, - tags: [ - { - value: 'Tag 1', - lineage: ['Tag 1'], - }, - { - value: 'Tag 2', - lineage: ['Tag 2'], - }, - ], - }, - { - name: 'Taxonomy 2', - taxonomyId: 124, - editable: true, - tags: [ - { - value: 'Tag 3', - lineage: ['Tag 3'], - }, - ], - }, - ], + useContentTaxonomyTagsData.mockReturnValue({ + isSuccess: true, + data: { + taxonomies: [ + { + name: 'Taxonomy 1', + taxonomyId: 123, + editable: true, + tags: [ + { + value: 'Tag 1', + lineage: ['Tag 1'], + }, + { + value: 'Tag 2', + lineage: ['Tag 2'], + }, + ], + }, + { + name: 'Taxonomy 2', + taxonomyId: 124, + editable: true, + tags: [ + { + value: 'Tag 3', + lineage: ['Tag 3'], + }, + ], + }, + ], + }, }); useTaxonomyListDataResponse.mockReturnValue({ results: [{ diff --git a/src/content-tags-drawer/ContentTagsDropDownSelector.test.jsx b/src/content-tags-drawer/ContentTagsDropDownSelector.test.jsx index 80ca659632..5823feaac4 100644 --- a/src/content-tags-drawer/ContentTagsDropDownSelector.test.jsx +++ b/src/content-tags-drawer/ContentTagsDropDownSelector.test.jsx @@ -1,51 +1,64 @@ import React from 'react'; import { IntlProvider } from '@edx/frontend-platform/i18n'; -import { act, render } from '@testing-library/react'; +import { act, render, waitFor } from '@testing-library/react'; import PropTypes from 'prop-types'; import ContentTagsDropDownSelector from './ContentTagsDropDownSelector'; -import { useTaxonomyTagsDataResponse, useIsTaxonomyTagsDataLoaded } from './data/apiHooks'; +import { useTaxonomyTagsData } from './data/apiHooks'; jest.mock('./data/apiHooks', () => ({ - useTaxonomyTagsDataResponse: jest.fn(), - useIsTaxonomyTagsDataLoaded: jest.fn(), + useTaxonomyTagsData: jest.fn(() => ({ + isSuccess: false, + data: {}, + })), })); const data = { taxonomyId: 123, level: 0, + tagsTree: {}, }; -const TaxonomyTagsDropDownSelectorComponent = ({ - taxonomyId, level, subTagsUrl, +const ContentTagsDropDownSelectorComponent = ({ + taxonomyId, level, subTagsUrl, lineage, tagsTree, }) => ( ); -TaxonomyTagsDropDownSelectorComponent.defaultProps = { +ContentTagsDropDownSelectorComponent.defaultProps = { subTagsUrl: undefined, + lineage: [], }; -TaxonomyTagsDropDownSelectorComponent.propTypes = { +ContentTagsDropDownSelectorComponent.propTypes = { taxonomyId: PropTypes.number.isRequired, level: PropTypes.number.isRequired, subTagsUrl: PropTypes.string, + lineage: PropTypes.arrayOf(PropTypes.string), + tagsTree: PropTypes.objectOf( + PropTypes.shape({ + explicit: PropTypes.bool.isRequired, + children: PropTypes.shape({}).isRequired, + }).isRequired, + ).isRequired, }; describe('', () => { it('should render taxonomy tags drop down selector loading with spinner', async () => { - useIsTaxonomyTagsDataLoaded.mockReturnValue(false); await act(async () => { const { getByRole } = render( - , ); const spinner = getByRole('status'); @@ -54,43 +67,53 @@ describe('', () => { }); it('should render taxonomy tags drop down selector with no sub tags', async () => { - useIsTaxonomyTagsDataLoaded.mockReturnValue(true); - useTaxonomyTagsDataResponse.mockReturnValue({ - results: [{ - value: 'Tag 1', - subTagsUrl: null, - }], + useTaxonomyTagsData.mockReturnValue({ + isSuccess: true, + data: { + results: [{ + value: 'Tag 1', + subTagsUrl: null, + }], + }, }); await act(async () => { const { container, getByText } = render( - , ); - expect(getByText('Tag 1')).toBeInTheDocument(); - expect(container.getElementsByClassName('taxonomy-tags-arrow-drop-down').length).toBe(0); + await waitFor(() => { + expect(getByText('Tag 1')).toBeInTheDocument(); + expect(container.getElementsByClassName('taxonomy-tags-arrow-drop-down').length).toBe(0); + }); }); }); it('should render taxonomy tags drop down selector with sub tags', async () => { - useIsTaxonomyTagsDataLoaded.mockReturnValue(true); - useTaxonomyTagsDataResponse.mockReturnValue({ - results: [{ - value: 'Tag 2', - subTagsUrl: 'https://example.com', - }], + useTaxonomyTagsData.mockReturnValue({ + isSuccess: true, + data: { + results: [{ + value: 'Tag 2', + subTagsUrl: 'https://example.com', + }], + }, }); await act(async () => { const { container, getByText } = render( - , ); - expect(getByText('Tag 2')).toBeInTheDocument(); - expect(container.getElementsByClassName('taxonomy-tags-arrow-drop-down').length).toBe(1); + await waitFor(() => { + expect(getByText('Tag 2')).toBeInTheDocument(); + expect(container.getElementsByClassName('taxonomy-tags-arrow-drop-down').length).toBe(1); + }); }); }); }); diff --git a/src/content-tags-drawer/ContentTagsTree.test.jsx b/src/content-tags-drawer/ContentTagsTree.test.jsx index dd28cc9a98..ac41f3c1f1 100644 --- a/src/content-tags-drawer/ContentTagsTree.test.jsx +++ b/src/content-tags-drawer/ContentTagsTree.test.jsx @@ -5,42 +5,53 @@ import PropTypes from 'prop-types'; import ContentTagsTree from './ContentTagsTree'; -const data = [ - { - value: 'DNA Sequencing', - lineage: [ - 'Science and Research', - 'Genetics Subcategory', - 'DNA Sequencing', - ], +const data = { + 'Science and Research': { + explicit: false, + children: { + 'Genetics Subcategory': { + explicit: false, + children: { + 'DNA Sequencing': { + explicit: true, + children: {}, + }, + }, + }, + 'Molecular, Cellular, and Microbiology': { + explicit: false, + children: { + Virology: { + explicit: true, + children: {}, + }, + }, + }, + }, }, - { - value: 'Virology', - lineage: [ - 'Science and Research', - 'Molecular, Cellular, and Microbiology', - 'Virology', - ], - }, -]; +}; -const ContentTagsTreeComponent = ({ appliedContentTags }) => ( +const ContentTagsTreeComponent = ({ tagsTree, removeTagHandler, editable }) => ( - + ); ContentTagsTreeComponent.propTypes = { - appliedContentTags: PropTypes.arrayOf(PropTypes.shape({ - value: PropTypes.string, - lineage: PropTypes.arrayOf(PropTypes.string), - })).isRequired, + tagsTree: PropTypes.objectOf( + PropTypes.shape({ + explicit: PropTypes.bool.isRequired, + children: PropTypes.shape({}).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(); + const { getByText } = render( {}} editable />); 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.test.jsx b/src/content-tags-drawer/TagBubble.test.jsx index 90ba32f288..1aee0df2e4 100644 --- a/src/content-tags-drawer/TagBubble.test.jsx +++ b/src/content-tags-drawer/TagBubble.test.jsx @@ -7,48 +7,55 @@ import TagBubble from './TagBubble'; const data = { value: 'Tag 1', + lineage: [], + removeTagHandler: () => {}, }; -const TagBubbleComponent = ({ value, subTagsCount, implicit }) => ( +const TagBubbleComponent = ({ + value, implicit, level, lineage, removeTagHandler, editable, +}) => ( - + ); TagBubbleComponent.defaultProps = { - subTagsCount: 0, implicit: true, + level: 0, }; TagBubbleComponent.propTypes = { value: PropTypes.string.isRequired, - subTagsCount: PropTypes.number, implicit: PropTypes.bool, + level: PropTypes.number, + lineage: PropTypes.arrayOf(PropTypes.string).isRequired, + removeTagHandler: PropTypes.func.isRequired, + editable: PropTypes.bool.isRequired, }; describe('', () => { - it('should render only value of the implicit tag with no sub tags', () => { - const { container, getByText } = render(); - expect(getByText(data.value)).toBeInTheDocument(); - expect(container.getElementsByClassName('implicit').length).toBe(1); - }); - - it('should render value of the implicit tag with sub tags', () => { - const tagBubbleData = { - subTagsCount: 5, - ...data, - }; + it('should render implicit tag', () => { const { container, getByText } = render( , ); - expect(getByText(`${tagBubbleData.value} (${tagBubbleData.subTagsCount})`)).toBeInTheDocument(); + expect(getByText(data.value)).toBeInTheDocument(); expect(container.getElementsByClassName('implicit').length).toBe(1); + expect(container.getElementsByClassName('pgn__chip__icon-after').length).toBe(0); }); - it('should render value of the explicit tag with no sub tags', () => { + it('should render explicit tag', () => { const tagBubbleData = { implicit: false, ...data, @@ -56,11 +63,14 @@ describe('', () => { const { container, getByText } = render( , ); expect(getByText(`${tagBubbleData.value}`)).toBeInTheDocument(); expect(container.getElementsByClassName('implicit').length).toBe(0); - expect(container.getElementsByClassName('btn-icon-after').length).toBe(1); + expect(container.getElementsByClassName('pgn__chip__icon-after').length).toBe(1); }); }); diff --git a/src/content-tags-drawer/data/apiHooks.test.jsx b/src/content-tags-drawer/data/apiHooks.test.jsx index 61f353f31f..038087914c 100644 --- a/src/content-tags-drawer/data/apiHooks.test.jsx +++ b/src/content-tags-drawer/data/apiHooks.test.jsx @@ -12,6 +12,7 @@ import { updateContentTaxonomyTags } from './api'; jest.mock('@tanstack/react-query', () => ({ useQuery: jest.fn(), useMutation: jest.fn(), + useQueryClient: jest.fn(), })); jest.mock('./api', () => ({ @@ -76,8 +77,10 @@ describe('useContentTaxonomyTagsMutation', () => { it('should call the update content taxonomy tags function', async () => { useMutation.mockReturnValueOnce({ mutate: jest.fn() }); - const mutation = useContentTaxonomyTagsMutation(); - mutation.mutate({ tags: ['a', 'b', 'c'] }); + const contentId = 'testerContent'; + const taxonomyId = 123; + const mutation = useContentTaxonomyTagsMutation(contentId, taxonomyId); + mutation.mutate({ tags: ['tag1', 'tag2'] }); expect(useMutation).toBeCalled(); @@ -85,10 +88,8 @@ describe('useContentTaxonomyTagsMutation', () => { const { mutationFn } = config; await act(async () => { - const contentId = 'testerContent'; - const taxonomyId = 123; const tags = ['tag1', 'tag2']; - await mutationFn({ contentId, taxonomyId, tags }); + await mutationFn({ tags }); expect(updateContentTaxonomyTags).toBeCalledWith(contentId, taxonomyId, tags); }); }); From 63e965c99624c2f785a6d017c96a7cbb6f8e1213 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Tue, 28 Nov 2023 18:39:35 +0300 Subject: [PATCH 15/24] test: Add tests for new content tags editing --- .../ContentTagsCollapsible.test.jsx | 140 +++++++++++++++++- src/content-tags-drawer/TagBubble.test.jsx | 26 +++- src/content-tags-drawer/__mocks__/index.js | 1 + .../updateContentTaxonomyTagsMock.js | 25 ++++ src/content-tags-drawer/data/api.js | 2 +- src/content-tags-drawer/data/api.test.js | 23 ++- 6 files changed, 207 insertions(+), 10 deletions(-) create mode 100644 src/content-tags-drawer/__mocks__/updateContentTaxonomyTagsMock.js diff --git a/src/content-tags-drawer/ContentTagsCollapsible.test.jsx b/src/content-tags-drawer/ContentTagsCollapsible.test.jsx index 537ece7127..a57359c651 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.test.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.test.jsx @@ -1,13 +1,25 @@ import React from 'react'; import { IntlProvider } from '@edx/frontend-platform/i18n'; -import { act, render } from '@testing-library/react'; +import { + act, + render, + fireEvent, + waitFor, +} from '@testing-library/react'; import PropTypes from 'prop-types'; import ContentTagsCollapsible from './ContentTagsCollapsible'; +import messages from './messages'; +import { useTaxonomyTagsData } from './data/apiHooks'; jest.mock('./data/apiHooks', () => ({ useContentTaxonomyTagsMutation: jest.fn(() => ({ isError: false, + mutate: jest.fn(), + })), + useTaxonomyTagsData: jest.fn(() => ({ + isSuccess: false, + data: {}, })), })); @@ -65,16 +77,136 @@ describe('', () => { }); }); - it('should render taxonomy tags data without tags number badge', async () => { - data.taxonomyAndTagsData.contentTags = []; + it('should render new tags as they are checked in the dropdown', async () => { + useTaxonomyTagsData.mockReturnValue({ + isSuccess: true, + data: { + results: [{ + value: 'Tag 1', + subTagsUrl: null, + }, { + value: 'Tag 2', + subTagsUrl: null, + }, { + value: 'Tag 3', + subTagsUrl: null, + }], + }, + }); + await act(async () => { - const { container, getByText } = render( + const { container, getByText, getAllByText } = render( , ); + + // Expand the Taxonomy to view applied tags and "Add tags" button + const expandToggle = container.getElementsByClassName('collapsible-trigger')[0]; + await act(async () => { + fireEvent.click(expandToggle); + }); + + // Click on "Add tags" button to open dropdown to select new tags + const addTagsButton = getByText(messages.addTagsButtonText.defaultMessage); + await act(async () => { + fireEvent.click(addTagsButton); + }); + + // Wait for the dropdown selector for tags to open, + // Tag 3 should only appear there + await waitFor(() => { + expect(getByText('Tag 3')).toBeInTheDocument(); + expect(getAllByText('Tag 3').length === 1); + }); + + const tag3 = getByText('Tag 3'); + await act(async () => { + fireEvent.click(tag3); + }); + + // After clicking on Tag 3, it should also appear in amongst + // the tag bubbles in the tree + await waitFor(() => { + expect(getAllByText('Tag 3').length === 2); + }); + }); + }); + + it('should remove tag when they are unchecked in the dropdown', async () => { + useTaxonomyTagsData.mockReturnValue({ + isSuccess: true, + data: { + results: [{ + value: 'Tag 1', + subTagsUrl: null, + }, { + value: 'Tag 2', + subTagsUrl: null, + }, { + value: 'Tag 3', + subTagsUrl: null, + }], + }, + }); + + await act(async () => { + const { container, getByText, getAllByText } = render( + , + ); + + // Expand the Taxonomy to view applied tags and "Add tags" button + const expandToggle = container.getElementsByClassName('collapsible-trigger')[0]; + await act(async () => { + fireEvent.click(expandToggle); + }); + + // Check that Tag 2 appears in tag bubbles + await waitFor(() => { + expect(getByText('Tag 2')).toBeInTheDocument(); + }); + + // Click on "Add tags" button to open dropdown to select new tags + const addTagsButton = getByText(messages.addTagsButtonText.defaultMessage); + await act(async () => { + fireEvent.click(addTagsButton); + }); + + // Wait for the dropdown selector for tags to open, + // Tag 3 should only appear there, (i.e. the dropdown is open, since Tag 3 is not applied) + await waitFor(() => { + expect(getByText('Tag 3')).toBeInTheDocument(); + }); + + // Get the Tag 2 checkbox and click on it + const tag2 = getAllByText('Tag 2')[1]; + await act(async () => { + fireEvent.click(tag2); + }); + + // After clicking on Tag 2, it should be removed from + // the tag bubbles in so only the one in the dropdown appears + expect(getAllByText('Tag 2').length === 1); + }); + }); + + it('should render taxonomy tags data without tags number badge', async () => { + const updatedData = { ...data }; + updatedData.taxonomyAndTagsData.contentTags = []; + await act(async () => { + const { container, getByText } = render( + , + ); expect(getByText('Taxonomy 1')).toBeInTheDocument(); expect(container.getElementsByClassName('invisible').length).toBe(1); }); diff --git a/src/content-tags-drawer/TagBubble.test.jsx b/src/content-tags-drawer/TagBubble.test.jsx index 1aee0df2e4..48fe71ecfd 100644 --- a/src/content-tags-drawer/TagBubble.test.jsx +++ b/src/content-tags-drawer/TagBubble.test.jsx @@ -1,6 +1,6 @@ import React from 'react'; import { IntlProvider } from '@edx/frontend-platform/i18n'; -import { render } from '@testing-library/react'; +import { act, render, fireEvent } from '@testing-library/react'; import PropTypes from 'prop-types'; import TagBubble from './TagBubble'; @@ -8,7 +8,7 @@ import TagBubble from './TagBubble'; const data = { value: 'Tag 1', lineage: [], - removeTagHandler: () => {}, + removeTagHandler: jest.fn(), }; const TagBubbleComponent = ({ @@ -73,4 +73,26 @@ describe('', () => { expect(container.getElementsByClassName('implicit').length).toBe(0); expect(container.getElementsByClassName('pgn__chip__icon-after').length).toBe(1); }); + + it('should call removeTagHandler when "x" clicked on explicit tag', async () => { + const tagBubbleData = { + implicit: false, + ...data, + }; + const { container } = render( + , + ); + + const xButton = container.getElementsByClassName('pgn__chip__icon-after')[0]; + await act(async () => { + fireEvent.click(xButton); + }); + expect(data.removeTagHandler).toHaveBeenCalled(); + }); }); diff --git a/src/content-tags-drawer/__mocks__/index.js b/src/content-tags-drawer/__mocks__/index.js index b09fc5d3ab..5ec3027386 100644 --- a/src/content-tags-drawer/__mocks__/index.js +++ b/src/content-tags-drawer/__mocks__/index.js @@ -1,3 +1,4 @@ export { default as taxonomyTagsMock } from './taxonomyTagsMock'; export { default as contentTaxonomyTagsMock } from './contentTaxonomyTagsMock'; export { default as contentDataMock } from './contentDataMock'; +export { default as updateContentTaxonomyTagsMock } from './updateContentTaxonomyTagsMock'; diff --git a/src/content-tags-drawer/__mocks__/updateContentTaxonomyTagsMock.js b/src/content-tags-drawer/__mocks__/updateContentTaxonomyTagsMock.js new file mode 100644 index 0000000000..cc319f1197 --- /dev/null +++ b/src/content-tags-drawer/__mocks__/updateContentTaxonomyTagsMock.js @@ -0,0 +1,25 @@ +module.exports = { + 'block-v1:SampleTaxonomyOrg1+STC1+2023_1+type@vertical+block@aaf8b8eb86b54281aeeab12499d2cb0b': { + taxonomies: [ + { + name: 'FlatTaxonomy', + taxonomyId: 3, + editable: true, + tags: [ + { + value: 'flat taxonomy tag 100', + lineage: [ + 'flat taxonomy tag 100', + ], + }, + { + value: 'flat taxonomy tag 3856', + lineage: [ + 'flat taxonomy tag 3856', + ], + }, + ], + }, + ], + }, +}; diff --git a/src/content-tags-drawer/data/api.js b/src/content-tags-drawer/data/api.js index 20c4a57295..562bfaa1ec 100644 --- a/src/content-tags-drawer/data/api.js +++ b/src/content-tags-drawer/data/api.js @@ -52,5 +52,5 @@ export async function updateContentTaxonomyTags(contentId, taxonomyId, tags) { let url = getContentTaxonomyTagsApiUrl(contentId); url = `${url}?taxonomy=${taxonomyId}`; const { data } = await getAuthenticatedHttpClient().put(url, { tags }); - return camelCaseObject(data); + 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 ffe19ab960..0d474ee0c2 100644 --- a/src/content-tags-drawer/data/api.test.js +++ b/src/content-tags-drawer/data/api.test.js @@ -2,7 +2,12 @@ import MockAdapter from 'axios-mock-adapter'; import { initializeMockApp } from '@edx/frontend-platform'; import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; -import { taxonomyTagsMock, contentTaxonomyTagsMock, contentDataMock } from '../__mocks__'; +import { + taxonomyTagsMock, + contentTaxonomyTagsMock, + contentDataMock, + updateContentTaxonomyTagsMock, +} from '../__mocks__'; import { getTaxonomyTagsApiUrl, @@ -11,6 +16,7 @@ import { getTaxonomyTagsData, getContentTaxonomyTagsData, getContentData, + updateContentTaxonomyTags, } from './api'; let axiosMock; @@ -33,7 +39,7 @@ describe('content tags drawer api calls', () => { }); it('should get taxonomy tags data', async () => { - const taxonomyId = '123'; + const taxonomyId = 123; axiosMock.onGet().reply(200, taxonomyTagsMock); const result = await getTaxonomyTagsData(taxonomyId); @@ -42,7 +48,7 @@ describe('content tags drawer api calls', () => { }); it('should get taxonomy tags data with fullPathProvided', async () => { - const taxonomyId = '123'; + const taxonomyId = 123; const fullPathProvided = 'http://example.com/'; axiosMock.onGet().reply(200, taxonomyTagsMock); const result = await getTaxonomyTagsData(taxonomyId, fullPathProvided); @@ -68,4 +74,15 @@ describe('content tags drawer api calls', () => { expect(axiosMock.history.get[0].url).toEqual(getContentDataApiUrl(contentId)); expect(result).toEqual(contentDataMock); }); + + it('should update content taxonomy tags', async () => { + 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); + const result = await updateContentTaxonomyTags(contentId, taxonomyId, tags); + + expect(axiosMock.history.put[0].url).toEqual(`${getContentTaxonomyTagsApiUrl(contentId)}?taxonomy=${taxonomyId}`); + expect(result).toEqual(updateContentTaxonomyTagsMock[contentId]); + }); }); From 9fd8c6e40fe7ffc4c7895354cd02c32c5d84cf07 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Wed, 29 Nov 2023 14:16:06 +0300 Subject: [PATCH 16/24] fix: Copy tree to avoid modifying originals --- src/content-tags-drawer/ContentTagsCollapsible.jsx | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx index 6e3fe11336..2e93c1082a 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx @@ -1,4 +1,5 @@ import React from 'react'; +import _ from 'lodash'; import { Badge, Collapsible, @@ -29,7 +30,7 @@ import { useContentTaxonomyTagsMutation } from './data/apiHooks'; * @returns {object} merged tree containing both tree1 and tree2 */ const mergeTrees = (tree1, tree2) => { - const mergedTree = { ...tree1 }; + const mergedTree = _.cloneDeep(tree1); const sortKeysAlphabetically = (obj) => { const sortedObj = {}; @@ -52,7 +53,7 @@ const mergeTrees = (tree1, tree2) => { mergeRecursively(destinationValue, sourceValue); } else { // eslint-disable-next-line no-param-reassign - destination[key] = sourceValue; + destination[key] = _.cloneDeep(sourceValue); } }); }; From a5087dd0b8e36d18573d7bfc4b10829d8818e682 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Wed, 29 Nov 2023 14:41:31 +0300 Subject: [PATCH 17/24] refactor: Nits from PR review --- .../ContentTagsCollapsible.jsx | 25 ++++++++----------- .../ContentTagsDropDownSelector.jsx | 4 +++ src/content-tags-drawer/TagBubble.jsx | 2 +- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/content-tags-drawer/ContentTagsCollapsible.jsx b/src/content-tags-drawer/ContentTagsCollapsible.jsx index 2e93c1082a..f26533d9ec 100644 --- a/src/content-tags-drawer/ContentTagsCollapsible.jsx +++ b/src/content-tags-drawer/ContentTagsCollapsible.jsx @@ -178,16 +178,13 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) => id, name, contentTags, } = taxonomyAndTagsData; - // State to determine whether to update the backend or not + // State to determine whether the tags are being updating so we can make a call + // to the update endpoint to the reflect those changes const [updatingTags, setUpdatingTags] = React.useState(false); const mutation = useContentTaxonomyTagsMutation(contentId, id); const [isOpen, open, close] = useToggle(false); - const [target, setTarget] = React.useState(null); - - // Keeps track of the tree structure for the applied content tags passed - // in as a prop. - const [appliedContentTags, setAppliedContentTags] = React.useState({}); + const [addTagsButtonRef, setAddTagsButtonRef] = React.useState(null); // Keeps track of the tree structure for tags that are add by selecting/unselecting // tags in the dropdowns. @@ -196,10 +193,10 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) => // To handle checking/unchecking tags in the SelectableBox const [checkedTags, { add, remove, clear }] = useCheckboxSetValues(); - // Handles make requests to the backend whenever the checked tags change + // Handles making requests to the update endpoint whenever the checked tags change React.useEffect(() => { - // We have this check because this hook is fired when the component first load - // and reloads (on refocus). We only want to make requests to the backend when + // We have this check because this hook is fired when the component first loads + // and reloads (on refocus). We only want to make a request to the update endpoint when // the user is updating the tags. if (updatingTags) { setUpdatingTags(false); @@ -209,7 +206,7 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) => }, [contentId, id, checkedTags]); // This converts the contentTags prop to the tree structure mentioned above - React.useMemo(() => { + const appliedContentTags = React.useMemo(() => { // Clear all the tags that have not been commited and the checked boxes when // fresh contentTags passed in so the latest state from the backend is rendered setAddedContentTags({}); @@ -243,8 +240,8 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) => }); }); - setAppliedContentTags(resultTree); - }, [contentTags, setAddedContentTags, mutation.isError]); + return resultTree; + }, [contentTags, mutation.isError]); // This is the source of truth that represents the current state of tags in // this Taxonomy as a tree. Whenever either the `appliedContentTags` (i.e. tags passed in @@ -315,7 +312,7 @@ const ContentTagsCollapsible = ({ contentId, taxonomyAndTagsData, editable }) => {editable && (