feat: tag sections, subsections, and the whole course (FC-0053) #879

rpenido · 2024-03-08T19:33:50Z

Description

Adds the Manage tags menu item and TagCount button to Sections, Subsections and the whole Course.

More Infomation

Closes: [Tagging] Tag sections, subsections, and the whole course modular-learning#194

Testing Instructions

Enable contentstore.new_studio_mfe.use_new_course_outline_page waffle
Run Taxonomy Sample Data script to create units with tags.
Go to a course to display the course outline.
Click in the Manage Tags in the course header and add some tags
Verify the tag count component is updated in real time
Use the Manage tags menu item in the three dots to add tags to Sections, Subsections and Units
Verify the tag count component is updated in real time

Private ref: FAL-3678

openedx-webhooks · 2024-03-08T19:33:54Z

Thanks for the pull request, @rpenido! 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.

rpenido · 2024-03-11T21:20:27Z

src/course-outline/status-bar/StatusBar.jsx

+const StatusBarItem = ({ title, children }) => (
+  <div className="d-flex flex-column justify-content-between">
+    <h5>{title}</h5>
+    <div className="d-flex align-items-center">
+      {children}
+    </div>
+  </div>
+);
+
+StatusBarItem.propTypes = {
+  title: PropTypes.string.isRequired,
+  children: PropTypes.node,
+};
+
+StatusBarItem.defaultProps = {
+  children: null,
+};
+


I created this component to avoid the repetition of the code below:

<div className="d-flex flex-column justify-content-between"> <h5>{title}</h5> ....

codecov · 2024-03-11T22:18:15Z

Codecov Report

Attention: Patch coverage is 96.15385% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 91.93%. Comparing base (9a6e12b) to head (dd7fdc2).
Report is 2 commits behind head on master.

Files	Patch %	Lines
src/content-tags-drawer/ContentTagsDrawer.jsx	71.42%	2 Missing ⚠️
src/content-tags-drawer/data/api.js	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #879      +/-   ##
==========================================
+ Coverage   91.87%   91.93%   +0.05%     
==========================================
  Files         572      571       -1     
  Lines       10058    10179     +121     
  Branches     2160     2205      +45     
==========================================
+ Hits         9241     9358     +117     
- Misses        790      794       +4     
  Partials       27       27

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

ChrisChV · 2024-03-12T14:47:51Z

src/course-outline/card-header/CardHeader.jsx


  const isDisabledPublish = (status === ITEM_BADGE_STATUS.live
    || status === ITEM_BADGE_STATUS.publishedNotLive) && !hasChanges;

+  const { data: contentTagCount } = useContentTagsCount(cardId);


@rpenido Will we follow the method of making a call on each card? I'm still leaning toward the approach of fetch tag counts of the entire course.

The current code fetches tags for the entire course, but the hook returns the value only for the requested contentId.

See:

https://github.com/openedx/frontend-app-course-authoring/blob/d1bad89bf1de76d264190eea5ff23953b500789d/src/course-outline/data/apiHooks.jsx#L11-L23

Added some comments here: e6bc92d

Ohh, I understand, thanks 👍

ChrisChV · 2024-03-12T15:23:41Z

@rpenido When open the tag drawer in the course header, the drawer title keeps loading (You can see it in the GIF on the description)

rpenido · 2024-03-12T15:52:33Z

@rpenido When open the tag drawer in the course header, the drawer title keeps loading (You can see it in the GIF on the description)

I totally forgot to fix that. Thank you @ChrisChV!
Fixed here: e39955a

ChrisChV

@rpenido Looks good! 👍

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

arbrandes · 2024-03-13T14:58:13Z

src/course-outline/status-bar/messages.js

@@ -41,6 +41,14 @@ const messages = defineMessages({
    id: 'course-authoring.course-outline.status-bar.highlight-emails.link',
    defaultMessage: 'Learn more',
  },
+  courseTagsTitle: {
+    id: 'course-authoring.course-outline.status-bar.course-tags',
+    defaultMessage: 'Course tags',


This is a friendly advance warning that soon we'll be requiring descriptions to be added to messages: openedx/frontend-build#517. For now I'm not going to block the PR, but it would be nice if you added some. :)

Sure! Done: f0001a4

bradenmacdonald · 2024-03-14T20:23:44Z

src/content-tags-drawer/data/api.js

@@ -30,6 +31,7 @@ export const getTaxonomyTagsApiUrl = (taxonomyId, options = {}) => {
 };
 export const getContentTaxonomyTagsApiUrl = (contentId) => new URL(`api/content_tagging/v1/object_tags/${contentId}/`, getApiBaseUrl()).href;
 export const getXBlockContentDataApiURL = (contentId) => new URL(`/xblock/outline/${contentId}`, getApiBaseUrl()).href;
+export const getCourseContentDataApiURL = (contentId) => new URL(`/api/courses/v1/courses/${contentId}/`, getLmsApiBaseUrl()).href;


Is there a Studio API you can use for this? It's better not to use LMS APIs for the "Course Authoring" frontend if we don't have to.

I'm not sure @bradenmacdonald.
I used the same endpoint that brings the course info to the outline:

https://github.com/openedx/frontend-app-course-authoring/blob/6ae9cdac0088117836138e713606c423bec317d5/src/data/api.js#L12-L17

OK I guess you can leave it for now then, but I've asked on Slack if there's a better way. I'll let you know if I hear anything better.

You can use /api/contentstore/v1/course_settings/:course_id which is a CMS API returns the display_name . But I guess either way is fine.

Nice! Fixed here: 1f14240

…and-the-whole-course

rpenido · 2024-03-20T20:58:57Z

Hi @xitij2000 ! I rebased this PR with the last changes.

xitij2000

This is a big PR with a lot of changes. I've tested it and it seems to work, but I will need to continue the review on monday.

xitij2000 · 2024-03-21T05:08:27Z

src/content-tags-drawer/data/apiHooks.jsx

+      // FixMe: remove code below
+      // queryClient.invalidateQueries({ queryKey: ['contentTaxonomyTagsCount', contentId] });


I guess this is left behind by mistake?

Sure! Sorry for that @xitij2000!
Fixed: 643cc1e

xitij2000

Looks good! Just a minor point, and then it's good to go from my side.

xitij2000 · 2024-03-27T13:10:31Z

src/course-outline/status-bar/StatusBar.jsx

+              <a
+                className="small ml-2"
+                href="#"
+                onClick={() => openManageTagsDrawer()}


Why not make this a button? Additionally, you can simplify the call to onClick={openManageTagsDrawer}

I was following the Figma design. It is possible to have the same appearance using a button component (that way we can remove the eslint warning)?

Fixed the function assignment here: dd7fdc2

Does the figma design specific that it has to be a link? I don't know the advantage of using a link here since it's being used like a button here with an onClick handler and the actual link usage is being supperessed.

The figma is here: https://www.figma.com/file/DmUC2e7BAoQsGDq4KgApPa/Content-Tagging-MVP?type=design&node-id=13585-6454&mode=design&t=UXNUlJJKbdiPmD49-4

openedx-webhooks · 2024-03-28T12:14:31Z

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

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Mar 8, 2024

rpenido force-pushed the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch 2 times, most recently from 6944de6 to 2b09ef1 Compare March 8, 2024 20:01

rpenido changed the title ~~Rpenido/fal 3678 tag sections subsections and the whole course~~ feat: tag sections, subsections, and the whole course Mar 8, 2024

rpenido changed the title ~~feat: tag sections, subsections, and the whole course~~ feat: tag sections, subsections, and the whole course (FC-0053) Mar 8, 2024

rpenido mentioned this pull request Mar 8, 2024

feat: tag sections, subsections, and the whole course (TEMP) open-craft/frontend-app-authoring#30

Closed

rpenido force-pushed the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch 2 times, most recently from 47c2f4f to d928dda Compare March 8, 2024 20:36

rpenido marked this pull request as ready for review March 11, 2024 21:11

rpenido requested a review from a team as a code owner March 11, 2024 21:12

rpenido commented Mar 11, 2024

View reviewed changes

rpenido force-pushed the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch from 1b57202 to c13b2c5 Compare March 11, 2024 22:17

rpenido force-pushed the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch 4 times, most recently from b1c18e1 to d454fc6 Compare March 11, 2024 23:54

feat: tag sections, subsections, and the whole course

d1bad89

rpenido force-pushed the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch from d454fc6 to d1bad89 Compare March 12, 2024 00:01

ChrisChV reviewed Mar 12, 2024

View reviewed changes

docs: add comments to useContentTagsCount

e6bc92d

rpenido force-pushed the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch from f89ea10 to e6bc92d Compare March 12, 2024 14:56

rpenido force-pushed the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch from ac1b94c to dac33de Compare March 12, 2024 16:13

rpenido requested a review from a team as a code owner March 12, 2024 16:13

fix: get course name in content tags drawer

e39955a

rpenido force-pushed the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch from dac33de to e39955a Compare March 12, 2024 18:45

ChrisChV approved these changes Mar 13, 2024

View reviewed changes

arbrandes reviewed Mar 13, 2024

View reviewed changes

feat: add description to messages

f0001a4

bradenmacdonald reviewed Mar 14, 2024

View reviewed changes

rpenido added 2 commits March 15, 2024 16:34

fix: change api url to get course display name

1f14240

Merge branch 'master' into rpenido/fal-3678-tag-sections-subsections-…

d57d5ad

…and-the-whole-course

rpenido force-pushed the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch 3 times, most recently from cbb9879 to 95300f2 Compare March 15, 2024 21:53

refactor: merge useContentTaxonomyTagsCount and useContentTagsCount

a9fd664

rpenido force-pushed the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch from 95300f2 to a9fd664 Compare March 15, 2024 22:19

rpenido requested review from xitij2000 and removed request for a team March 18, 2024 12:37

rpenido added 4 commits March 20, 2024 11:15

Merge branch 'master' into rpenido/fal-3678-tag-sections-subsections-…

888a67c

…and-the-whole-course

style: fix eslint after merge

7ae7f0b

fix: typings

3f96df9

revert: reverting removed code

7f58c5a

rpenido force-pushed the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch from 97f3c87 to 7f58c5a Compare March 20, 2024 15:21

xitij2000 reviewed Mar 22, 2024

View reviewed changes

xitij2000 approved these changes Mar 27, 2024

View reviewed changes

rpenido added 2 commits March 27, 2024 11:05

fix: remove unused code

643cc1e

refactor: change onClick function assignment

dd7fdc2

xitij2000 merged commit f57d40e into openedx:master Mar 28, 2024
6 checks passed

xitij2000 deleted the rpenido/fal-3678-tag-sections-subsections-and-the-whole-course branch March 28, 2024 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: tag sections, subsections, and the whole course (FC-0053) #879

feat: tag sections, subsections, and the whole course (FC-0053) #879

rpenido commented Mar 8, 2024 •

edited

Loading

openedx-webhooks commented Mar 8, 2024 •

edited

Loading

rpenido Mar 11, 2024

codecov bot commented Mar 11, 2024 •

edited

Loading

ChrisChV Mar 12, 2024

rpenido Mar 12, 2024 •

edited

Loading

ChrisChV Mar 12, 2024

ChrisChV commented Mar 12, 2024

rpenido commented Mar 12, 2024 •

edited

Loading

ChrisChV left a comment

arbrandes Mar 13, 2024

rpenido Mar 14, 2024

bradenmacdonald Mar 14, 2024

rpenido Mar 14, 2024

bradenmacdonald Mar 15, 2024

bradenmacdonald Mar 15, 2024

rpenido Mar 15, 2024

rpenido commented Mar 20, 2024

xitij2000 left a comment

xitij2000 Mar 21, 2024

rpenido Mar 27, 2024

xitij2000 left a comment

xitij2000 Mar 27, 2024

rpenido Mar 27, 2024 •

edited

Loading

xitij2000 Mar 28, 2024

rpenido Mar 28, 2024

openedx-webhooks commented Mar 28, 2024

		// FixMe: remove code below
		// queryClient.invalidateQueries({ queryKey: ['contentTaxonomyTagsCount', contentId] });

feat: tag sections, subsections, and the whole course (FC-0053) #879

feat: tag sections, subsections, and the whole course (FC-0053) #879

Conversation

rpenido commented Mar 8, 2024 • edited Loading

Description

More Infomation

Testing Instructions

openedx-webhooks commented Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

rpenido Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChV commented Mar 12, 2024

rpenido commented Mar 12, 2024 • edited Loading

ChrisChV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido commented Mar 20, 2024

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openedx-webhooks commented Mar 28, 2024

rpenido commented Mar 8, 2024 •

edited

Loading

openedx-webhooks commented Mar 8, 2024 •

edited

Loading

codecov bot commented Mar 11, 2024 •

edited

Loading

rpenido Mar 12, 2024 •

edited

Loading

rpenido commented Mar 12, 2024 •

edited

Loading

rpenido Mar 27, 2024 •

edited

Loading