[FC-0036] feat: Add content tags tree state + editing

[yusuf-musleh]

Description

This PR adds the editing functionality for content tags. You are able to select/deselect tags from the dropdowns in each taxonomy and the tags tree structure is updated, requests are sent to the backend, everything gets automatically updated and the state persists.

When a tag is select, all its ancestors automatically become 'implicit' and are unselected and cannot 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.

Supporting information

Related Tickets:

• [Tagging] Edit a content item's tags from the tag drawer modular-learning#122

Testing instructions

1. Run you local dev stack.
2. Next, navigate to http://localhost:18010/admin/waffle/flag/ and create a new flag: new_studio_mfe.use_tagging_taxonomy_list_page make sure it is active for everyone
3. If you don't already have sample taxonomy/tags data, follow the instructions in this repo to generate sample data: https://github.com/open-craft/taxonomy-sample-data
4. Navigate to the sample courses and open the tags drawer for the units
5. Check that all the taxonomies are shown in the drawer and the applied tags are show
6. Expand some taxonomies and check that the applied tags shows, implicit and explicit
7. Click on "Add Tags" button and confirm that the applied tags are reflected in the dropdown selector as well
8. Confirm that the implicit tags are not clickable
9. Confirm that when you check/uncheck a tag it is reflected in the tags tree
10. Confirm that if you check a tag, all its ancestors become implicit
11. Confirm when you uncheck a tag and it is the only child, all its ancestors get unchecked
12. Confirm when you uncheck a tag and its has siblings, the ancestors do not get removed
13. Confirm that "Load More" button functions correctly and loads more tags
14. Confirm that the tags number gets updated correctly whenever you add/remove tags
15. Confirm that you can remove tags by clicking on the "x" for explicit tags (same behaviors described above should be observed)

---

Private-ref: FAL-3530

[openedx-webhooks]

Thanks for the pull request, @yusuf-musleh! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[bradenmacdonald]

Any handler functions should be wrapped in the React.useCallback hook

[yusuf-musleh]

So this function is being used inside the handler, does it also need to be wrapped in the React.useCallback if the handler itself is wrapped?

[bradenmacdonald]

Oh, no then it's not necessary... but in that case I would personally just move this function outside of the component altogether to become a top-level function.

BTW the rule generally is if you're using a function as a prop value, it should be wrapped in a hook. The point of a hook like useCallback is that if it's called multiple times, it still returns the exact same function (same instance), and it only re-declares the function if the array of dependencies changes. Otherwise, every time your parent component is rendered, a new function instance is initialized and when you pass it as a prop, react considers it a changed prop (it's an identical function but a different instance, so it's not the same by reference), and then will have to re-render the child component. And as you saw, sometimes the variables captured within your function get out of sync with the components.

[yusuf-musleh]

Got it, the addTags function uses dispatchers from the useCheckboxSetValues so I left it there, but I moved the other functions outside the component. 7c02136

[bradenmacdonald]

Can we delete this line @returns {import("./types.mjs").UseMutationResult} and the custom UseMutationResult type? TypeScript can infer the correct type from the react-query code, and the full type is much more useful/accurate.

With the current code:

Without that line you actually get the full, accurate type:

No need for useContentTaxonomyTagsDataResponse

Also, there is a similar issue above. The return type of useContentTaxonomyTagsData is wrong, and should be set to @returns {import("@tanstack/react-query").UseQueryResult<import("./types.mjs").ContentTaxonomyTagsData>}.

If you do that, you'll see there's no need for the useContentTaxonomyTagsDataResponse function at all.

Instead of

  const useContentTaxonomyTagsData = () => {
    const contentTaxonomyTagsData = useContentTaxonomyTagsDataResponse(contentId);
    const isContentTaxonomyTagsLoaded = useIsContentTaxonomyTagsDataLoaded(contentId);
    return { contentTaxonomyTagsData, isContentTaxonomyTagsLoaded };
  };
  const { contentTaxonomyTagsData, isContentTaxonomyTagsLoaded } = useContentTaxonomyTagsData();

With the accurate types in place, you can just write:

const {data, isSuccess} = useContentTaxonomyTagsData(...);

Or if you want to rename those variables, write it as:

const {data: contentTaxonomyTagsData, isSuccess: isContentTaxonomyTagsLoaded} = useContentTaxonomyTagsData(...);

which is exactly the same thing as the code you have now, but with way less boilerplate.

[yusuf-musleh]

Interesting! Thanks for the detailed breakdown. That's definitely much simpler and cleaner, I updated all of them to follow this structure: bdc8d30

[bradenmacdonald]

Nice, much simpler :)

[bradenmacdonald]

In useContentData (above, but I can't comment directly on the right line), the query key is wrong. It needs to include the contentId, so a good query key would be ['contentData', contentId]. Otherwise the data about XBlock B will be read from the cache of XBlock A and users will see the wrong data.

Same for useContentTaxonomyTagsData - the query key needs to include the content ID.

[yusuf-musleh]

I see, makes sense, I updated it. ec2c842

[bradenmacdonald]

Here's how to get the right typing for mutationFn:

1. Remove the incomplete @returns type as mentioned above.
2. Add a type declaration to mutationFn of @type {import("@tanstack/react-query").MutateFunction<any, any, {contentId: string, taxonomyId: number, tags: string[]}>} as seen in the screenshot below.
3. Now all the types are accurate - TypeScript can infer the rest automatically. You can see in the screenshot that when you go to call mutate() it knows what arguments you need.

Note: I think taxonomyId should be a number, but you can do it as a string if you want.

[yusuf-musleh]

Updated as well (also changed ittaxonomyId to a number), bdc8d30

[codecov]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (56ad86e) 88.85% compared to head (ae22fb1) 88.85%.

Files	Patch %	Lines
...ntent-tags-drawer/ContentTagsCollapsibleHelper.jsx	95.29%	4 Missing ⚠️
...ontent-tags-drawer/ContentTagsDropDownSelector.jsx	85.00%	3 Missing ⚠️
src/content-tags-drawer/data/apiHooks.jsx	77.77%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #704   +/-   ##
=======================================
  Coverage   88.85%   88.85%           
=======================================
  Files         468      469    +1     
  Lines        7168     7252   +84     
  Branches     1539     1558   +19     
=======================================
+ Hits         6369     6444   +75     
- Misses        772      781    +9     
  Partials       27       27           

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

[ChrisChV]

👍

• I tested this: I followed the testing instructions.
• I read through the code and considered the security, stability and performance implications of the changes.
• I tested that the UI can be used with a keyboard only (tab order, keyboard controls).
• Includes tests for bugfixes and/or features added.
• Includes documentation

[bradenmacdonald]

Some minor React hook usage tips...

[bradenmacdonald]

Nit: I think this lint error was here for good reason. The current implementation is modifying the input objects, which I don't think it should.

const orig = {
    c: {label: "C", children: {}},
    a: {label: "A", children: {}},
    b: {label: "B", children: {
        c: {label: "BC", children: {}},
        a: {label: "BA", children: {}},
        b: {label: "BB", children: {}},
    }},
};

const changed = mergeTrees(orig, orig);
console.log(orig);

If you run this (e.g. in your browser console), you'll see that orig gets unexpectedly modified by this, and its b.children have become sorted in-place.

If you run

const orig = {
    c: {label: "C", children: {}},
    a: {label: "A", children: {}},
    b: Object.freeze({label: "B", children: {
        c: {label: "BC", children: {}},
        a: {label: "BA", children: {}},
        b: {label: "BB", children: {}},
    }}),
};

const changed = mergeTrees(orig, orig);

you'll get an error when it tries to modify the object b in-place instead of creating a copy of it and modifying that.

It's better to use immutable data structures when possible so although this may not be causing any bugs now, I'd recommend tweaking it to not modify the input objects in any way so that this function behaves more consistently.

[yusuf-musleh]

@bradenmacdonald Thanks for catching this very intricate bug! Though the main issue wasn't the reassigning of the param, as the linter mentions, since the destination is always copied over (each level of the recursion), however the issue was that the source tree/value was not, hence it was getting modified.

I tried copying over the source value (tree) in the same style (using { ...sourceValue }) however that made it really slow. I realize we have lodash installed, and it has a much more efficient clone method, so I used that instead, and it works great, the input objects no longer get modified. 9fd8c6e

[bradenmacdonald]

Nit/docs: Just from reading this comment, I'm still unclear on what this state is doing, without reading more of the code. What does it mean "to determine whether to update the backend or not" ?

[yusuf-musleh]

Got it. It essentially determines whether we should make a call to the update endpoint. I wrote a more descriptive comment: a5087dd

[bradenmacdonald]

This fetchUrl pattern is a bit weird to me. I would suggest putting in a comment that for the future, we'd probably want to refactor to having a count of how many times the user has clicked "load more" and then use useQueries to load all the pages based on that. No need to refactor now though if this is working.

[yusuf-musleh]

Yeah fetchUrl feels a bit overloaded and is doing too much at the moment. I agree it warrants a refactor, I added a TODO comment to address it in the future.

[bradenmacdonald]

Needs editable and removeTagHandler as dependencies too, since you use those prop variables inside this callback. I'm surprise a react hook linter didn't catch this...

[yusuf-musleh]

It seems like the exhaustive-deps option is turned off for the linter, not really sure why. That's why it didn't catch it, when I turned it on the linter pointed it out.

https://github.com/openedx/frontend-app-course-authoring/blob/a2dceac62fc13623434ff5324392608d470bc13b/.eslintrc.js#L12

I included the missing dependancies.

[bradenmacdonald]

@yusuf-musleh Can you please add [FC-0036] to the PR title?

[yusuf-musleh]

@bradenmacdonald Sure thing

[bradenmacdonald]

@yusuf-musleh If this is hard to fix, it can be a future PR, but the "add tags" widget is still moving around awkwardly when I click on tags:

recording.mov

[bradenmacdonald]

Other than the above issue, I'm happy with the code now. Thanks! We'll continue to refine and improve this in future PRs.

[yusuf-musleh]

@yusuf-musleh If this is hard to fix, it can be a future PR, but the "add tags" widget is still moving around awkwardly when I click on tags:

recording.mov

@bradenmacdonald hmm, I looked into it a bit. The problem is that the dropdown is "attached" to the "add tags" button, so it moves with it when the new tags are added causing this awkward movement. Keeping it in places however, I feel, would be more awkward as the button moves further away and the dropdown would be "floating" by itself. I think the proper fix would be, as you and @ali-hugo mentioned before, to have the tags added after the dropdown is closed, that should properly fix the issue. I can work on as part of the UI/UX refinements PR.

[bradenmacdonald]

@yusuf-musleh Sounds good.

[xitij2000]

I think not keeping a tree structure, and embracing recursive components might simplify this code quite a bit.

[xitij2000]

This is not specific to this PR but to most the PRs using useQuery. I think a term like mutation is pretty unclear. I understand that is the preference of this framework, however, it's not clear from this function what it's doing.

I think what might be nice, is to change this name to useContentTaxonomyTagsUpdater, and have that directly require the function to call for the update. i.e.

`

  const updateTags = useContentTaxonomyTagsUpdate(contentId, id);
  ...
  updateTags({ tags });

That way someone who's not familiar with the framework can still understand what is going on.

[yusuf-musleh]

Make sense, I've renamed it for clarity, however I kept return object as is, since the mutation object contains methods/properties (such as isError and reset()) that I make use of through out the code.

[xitij2000]

There is a lot of code in this component before the UI part of this component starts. In situations like this, I think it can be clearer to extract a hook out of it so that the logic and UI are less intertwined.

For example, you could extract the following react hook out of this component:

const useTagCollapsableHelper = (contentId, taxonomyAndTagsData) => {
  const {
    id, contentTags,
  } = taxonomyAndTagsData;
  // State to determine whether the tags are being updating so we can make a call
  // to the update endpoint to the reflect those changes
  const [updatingTags, setUpdatingTags] = React.useState(false);
  const mutation = useContentTaxonomyTagsMutation(contentId, id);

  // Keeps track of the content objects tags count (both implicit and explicit)
  const [contentTagsCount, setContentTagsCount] = React.useState(0);

  // 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, clear }] = useCheckboxSetValues();

  // 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 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);
      const tags = checkedTags.map(t => decodeURIComponent(t.split(',').slice(-1)));
      mutation.mutate({ tags });
    }
  }, [contentId, id, checkedTags]);

  // This converts the contentTags prop to the tree structure mentioned above
  const appliedContentTags = React.useMemo(() => {
    let contentTagsCounter = 0;

    // 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;

      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);
          contentTagsCounter += 1;
        }

        currentLevel = currentLevel[key].children;
      });
    });

    setContentTagsCount(contentTagsCounter);
    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
  // 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;
    });
  };

  const tagChangeHandler = React.useCallback((tagSelectableBoxValue, checked) => {
    const tagLineage = tagSelectableBoxValue.split(',').map(t => decodeURIComponent(t));
    const selectedTag = tagLineage.slice(-1)[0];

    const addedTree = { ...addedContentTags };
    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(tagSelectableBoxValue);

      // 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);
    setUpdatingTags(true);
  });
  return { tagChangeHandler, tagsTree, contentTagsCount, checkedTags };
};

This could be put in a separate file in the component package. It'll help keep the function a bit cleaner.

This is just my preference of course if you feel otherwise do tell.

[yusuf-musleh]

Agreed, I think splitting it out makes it cleaner, i've done that here: 8d1f7e9

[xitij2000]

Here you can extract out all the pagination related code into a separate hook called: usePaginatedTaxonomyTagsData. This can be a reusable hook that can handle all the pagination:

const usePaginatedTaxonomyTagsData = (subTagsUrl) => {

  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)
  //
  // TODO: In the future we may need to refactor this to keep track
  // of the count for how many times the user clicked on "load more" then
  // use useQueries to load all the pages based on that.
  const [fetchUrl, setFetchUrl] = useState(subTagsUrl);

  const { data: taxonomyTagsData, isSuccess: isTaxonomyTagsLoaded } = useTaxonomyTagsData(taxonomyId, fetchUrl);

  useEffect(() => {
    if (isTaxonomyTagsLoaded && taxonomyTagsData) {
      setTags([...tags, ...taxonomyTagsData.results]);
      setNextPage(taxonomyTagsData.next);
    }
  }, [isTaxonomyTagsLoaded, taxonomyTagsData]);

  const loadMoreTags = useCallback(() => {
    setFetchUrl(nextPage);
  }, [nextPage]);

  return { tags, loadMoreTags, nextPage};
}

You can then just use it as const { tags, nextPage, loadMoreTags} = usePaginatedTaxonomyTagsData(subTagsUrl);

[yusuf-musleh]

Great suggestion! Since I am refactoring the way I do pagination in the search tags PR I think it makes sense to include this improvement as part of that PR as well.

[xitij2000]

I'm not sure why decodeURIComponent and encodeURIComponent are being used here. Why not use the direct value?

[yusuf-musleh]

This is because commas (,) are used as delimiters between tags in the serialized lineage, and tags can contain contain commas. So we encode/decode to avoid any issue there. More discussions: #704 (comment)

[xitij2000]

Why is this needed?

[yusuf-musleh]

This is to closer match the Figma design mockups. As the default styling for the selectable box component in Paragon contains borders/greyed out text that differ from the styles in Figma

[xitij2000]

One thing to note is that often these Figma designs are based on the 2U/edX theme instead of the vanilla theme. So you should try installing that branding package and then seeing what doesn't match.

Additionally, I think we can also start by commenting on the design about changing the design to match figma rather than overriding a component like this.

Note that this will apply to all instances of this component in course-authoring. Which means that other components of course-authoring will stop matching their figma designs.

If the idea is to move paragon in this direction, then that change needs to be made in paragon.

I think it's worth having this discussion and saving this change for later, till the discussion as been had.

[yusuf-musleh]

Makes sense, I installed the 2U edx theme and style was still off from the Figma designs (note the spacing/alignment in the screenshots are fixed in open-craft#16):

Default Theme without overrides:

2U edx theme without overrides:

I agree, these style changes should be part of the theme or Paragon rather than overriding them here, any idea's who we can discuss this with? Maybe @ali-hugo might have some insights?

For now would it be acceptable to make the overrides more specific by adding the class so it doesn't override all these styles? cae319e

[bradenmacdonald]

@yusuf-musleh @xitij2000 CC @ali-hugo Also please remember the mockups for this project are not pixel-perfect UX designs. So you don't need to try to match them exactly; if there's an off-the-shelf Paragon component or CSS class that provides the functionality we need, just use it.

But something like you're showing above, where the outline is behind another component, definitely should be fixed.

For questions about how to improve Paragon, I find it best to ask on Slack #wg-frontend

Let's get this PR merged though; we can fine-tune the the appearance in follow-up PRs. We are in MVP mode here and this one holding up other work.

[yusuf-musleh]

Sounds good, I've asked about it in the #wg-frontend channel.

[yusuf-musleh]

@xitij2000 Thanks for the review! I've addressed the issues brought up. However regarding the tree structure refactor, I think I'll wait on @bradenmacdonald to weigh in on it in the ticket to see how we should proceed.

[yusuf-musleh]

@xitij2000 Friendly reminder for final review.

[xitij2000]

Looks good and seems to be working. My only concern is the overriding of paragon styles.

[xitij2000]

One thing to note is that often these Figma designs are based on the 2U/edX theme instead of the vanilla theme. So you should try installing that branding package and then seeing what doesn't match.

Additionally, I think we can also start by commenting on the design about changing the design to match figma rather than overriding a component like this.

Note that this will apply to all instances of this component in course-authoring. Which means that other components of course-authoring will stop matching their figma designs.

If the idea is to move paragon in this direction, then that change needs to be made in paragon.

I think it's worth having this discussion and saving this change for later, till the discussion as been had.

[bradenmacdonald]

@yusuf-musleh Could you please squash this to a single commit so @xitij2000 can merge? He prefers that you squash it yourself.

[yusuf-musleh]

@yusuf-musleh Could you please squash this to a single commit so @xitij2000 can merge? He prefers that you squash it yourself.

@bradenmacdonald Done, commits squashed to one. cc: @xitij2000

[openedx-webhooks]

@yusuf-musleh 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.