feat: Send message after update tags on content [FC-0036] #800

ChrisChV · 2024-01-17T18:33:45Z

Description

Blocked: Check this #800 (comment)

This adds a feature to send tags of a content after the update mutation.

Supporting information

Internal ticket: FAL-3601
Github issue: openedx/modular-learning#167
Related PR: openedx/edx-platform#34059

Testing instructions

Follow the testing instructions of Tagging UX refinement v2 [FC-0036] edx-platform#34059

openedx-webhooks · 2024-01-17T18:33:51Z

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

openedx-webhooks · 2024-01-17T18:33:53Z

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

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

codecov · 2024-01-18T14:01:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0fc0ce0) 89.28% compared to head (076ef75) 89.28%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #800   +/-   ##
=======================================
  Coverage   89.28%   89.28%           
=======================================
  Files         551      551           
  Lines        9740     9744    +4     
  Branches     2101     2101           
=======================================
+ Hits         8696     8700    +4     
  Misses        996      996           
  Partials       48       48

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

yusuf-musleh

👍 LGTM! Great work @ChrisChV ! Just had 1 suggestion

I tested this: (I testing by following the steps in the related edx-platform PR)
I read through the code
I checked for accessibility issues
~~Includes documentation~~
~~I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.~~

yusuf-musleh · 2024-01-19T07:28:05Z

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

+            ...data,
+          };
+          window.top?.postMessage(
+            `[Manage tags drawer] Tags updated: ${JSON.stringify(messageJson)}`,


I like this new message format, it might make sense to also change the closeManageTagsDrawer message to match the same format to keep things consistent?

I think it would be better to add it when necessary, when it is used. Also, as Braden says, these messages are temporary.

I'm not sure why this message data is being passed as a string and then processed. You can simply pass JSON data. This is used in a couple of places and it generally follows the format of: window.top?.postMessage({ type: 'authoring.events.tags.updated', payload: data })

Then you can use code like this to handle it on the other side.

bradenmacdonald · 2024-01-19T22:20:28Z

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

+            getConfig().STUDIO_BASE_URL,
+          );
+        });
+        getContentTaxonomyTagsCountData(contentId).then((data) => {


It would be simpler to just send a message that the count has changed, with only the contentId and to let the parent frame call the API to retrieve the updated count if it wants to. That will keep the code here simpler. What do you think?

I have also thought the same thing, but I feel that it is simpler to make and handle the API calls here and just send the data to be displayed in the template. Also I don't think there is much problem with the code here since it is temporary

Yep, I just don't want people to copy this pattern when they move it into the MFE. If we just use React-Query in the MFE it will update without any special code, as long as we do the "query invalidation" correctly.

bradenmacdonald · 2024-01-19T22:23:29Z

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

@@ -142,5 +144,30 @@ export const useContentTaxonomyTagsUpdater = (contentId, taxonomyId) => {
    onSettled: () => {
      queryClient.invalidateQueries({ queryKey: ['contentTaxonomyTags', contentId] });
    },
+    onSuccess: () => {


This notification is just temporary. In the future, the course outline page and course unit page will be part of the frontend-app-course-authoring, so there will be no need to send a change notification nor to refresh the counts - React Query will handle that all automatically :)

So: I want to make sure everyone who sees this code understands that. Please add a comment that says something like "In the future, when the Course Outline Page and Unit Page are integrated into this MFE, they should just use React Query to load the tag counts, and React Query will automatically refresh those counts when the mutation invalidates them. So this postMessage is just a temporary feature to support the legacy Django template courseware page."

Updated here: 78db929

Nice, thanks!

bradenmacdonald · 2024-01-22T20:32:08Z

@ChrisChV Can this PR merge before the edx-platform PR?

ChrisChV · 2024-01-22T23:35:54Z

@ChrisChV Can this PR merge before the edx-platform PR?

@bradenmacdonald Yes, in this case the order does not matter. Nothing breaks

xitij2000

The code generally looks good, but I don't like the usage of postMessage. You can send JSON data with postmessage, so I don't see a reason to mash it into a string and then use regex on the other side. These are messages that the user will not see so we can use something that's easier to parse.

xitij2000 · 2024-02-06T10:43:15Z

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

+            ...data,
+          };
+          window.top?.postMessage(
+            `[Manage tags drawer] Tags updated: ${JSON.stringify(messageJson)}`,


I'm not sure why this message data is being passed as a string and then processed. You can simply pass JSON data. This is used in a couple of places and it generally follows the format of: window.top?.postMessage({ type: 'authoring.events.tags.updated', payload: data })

Then you can use code like this to handle it on the other side.

ChrisChV · 2024-02-12T20:10:04Z

@xitij2000 Thanks for the review. Yes, you are right, I will make the changes

ChrisChV · 2024-02-12T21:43:08Z

@xitij2000 I made the changes on this commit: 2170664

Also I open this PR with edx-platform changes: openedx/edx-platform#34223

xitij2000

This is not working for me on tutor without changes described in openedx/edx-platform#34223

Once that is fixed, this is good to go.

bradenmacdonald · 2024-02-20T20:12:03Z

Good news/bad news: With the major progress we're making lately on the MFE pages, and the plan to shift focus entirely to the MFEs for now, I think it may make more sense to pause this PR. If the outline/unit MFE pages don't end up being ready for Redwood, we can merge this, but it's looking pretty likely that those will be done by Redwood and we won't need this at all.

ChrisChV · 2024-02-21T13:43:25Z

Good news/bad news: With the major progress we're making lately on the MFE pages, and the plan to shift focus entirely to the MFEs for now, I think it may make more sense to pause this PR. If the outline/unit MFE pages don't end up being ready for Redwood, we can merge this, but it's looking pretty likely that those will be done by Redwood and we won't need this at all.

Ok, make sense 👍

bradenmacdonald · 2024-03-19T20:12:00Z

Closing this as we're focusing development on the MFE.

openedx-webhooks · 2024-03-19T20:12:03Z

@ChrisChV Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

feat: Send message after update tags on content

55276c8

ChrisChV marked this pull request as draft January 17, 2024 18:33

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

ChrisChV changed the title ~~feat: Send message after update tags on content~~ feat: Send message after update tags on content [FC-0036] Jan 17, 2024

ChrisChV mentioned this pull request Jan 17, 2024

Tagging UX refinement v2 [FC-0036] openedx/edx-platform#34059

Merged

feat: Send messages with updated tags count from manage tags drawer

786a04e

ChrisChV marked this pull request as ready for review January 18, 2024 16:58

yusuf-musleh approved these changes Jan 19, 2024

View reviewed changes

bradenmacdonald reviewed Jan 19, 2024

View reviewed changes

ChrisChV added 2 commits January 22, 2024 03:05

docs: Comments added

78db929

docs: Comments added

04f5bf9

test: Fix coverage

e60c559

bradenmacdonald mentioned this pull request Jan 25, 2024

[Tagging] New Tag Sidebar Widget - Unit Page (in MFE) openedx/modular-learning#187

Closed

xitij2000 requested changes Feb 6, 2024

View reviewed changes

ChrisChV and others added 2 commits February 12, 2024 15:16

Merge branch 'master' into chris/FAL-3601-ux-refinements-v2

32a5f9f

refactor: Send Json message on tags drawer update

2170664

ChrisChV mentioned this pull request Feb 12, 2024

refactor: Update tags drawer listener for a Json message openedx/edx-platform#34223

Closed

xitij2000 approved these changes Feb 19, 2024

View reviewed changes

Merge branch 'master' into chris/FAL-3601-ux-refinements-v2

076ef75

bradenmacdonald closed this Mar 19, 2024

bradenmacdonald mentioned this pull request May 1, 2024

Legacy Unit Page UI: Update tag counts when changes are saved in the tag drawer openedx/modular-learning#216

Closed

ChrisChV mentioned this pull request May 2, 2024

[FC-0036] feat: Send messages after update tags #975

Merged

bradenmacdonald deleted the chris/FAL-3601-ux-refinements-v2 branch July 12, 2024 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Send message after update tags on content [FC-0036] #800

feat: Send message after update tags on content [FC-0036] #800

ChrisChV commented Jan 17, 2024 •

edited

Loading

openedx-webhooks commented Jan 17, 2024 •

edited

Loading

openedx-webhooks commented Jan 17, 2024

codecov bot commented Jan 18, 2024 •

edited

Loading

yusuf-musleh left a comment •

edited

Loading

yusuf-musleh Jan 19, 2024

ChrisChV Jan 22, 2024

xitij2000 Feb 6, 2024

bradenmacdonald Jan 19, 2024

ChrisChV Jan 22, 2024

bradenmacdonald Jan 22, 2024

bradenmacdonald Jan 19, 2024

ChrisChV Jan 22, 2024

bradenmacdonald Jan 22, 2024

bradenmacdonald commented Jan 22, 2024

ChrisChV commented Jan 22, 2024 •

edited

Loading

xitij2000 left a comment

xitij2000 Feb 6, 2024

ChrisChV commented Feb 12, 2024

ChrisChV commented Feb 12, 2024

xitij2000 left a comment

bradenmacdonald commented Feb 20, 2024

ChrisChV commented Feb 21, 2024

bradenmacdonald commented Mar 19, 2024

openedx-webhooks commented Mar 19, 2024

feat: Send message after update tags on content [FC-0036] #800

feat: Send message after update tags on content [FC-0036] #800

Conversation

ChrisChV commented Jan 17, 2024 • edited Loading

Blocked: Check this #800 (comment)

openedx-webhooks commented Jan 17, 2024 • edited Loading

openedx-webhooks commented Jan 17, 2024

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

yusuf-musleh left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Jan 22, 2024

ChrisChV commented Jan 22, 2024 • edited Loading

xitij2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisChV commented Feb 12, 2024

ChrisChV commented Feb 12, 2024

xitij2000 left a comment

Choose a reason for hiding this comment

bradenmacdonald commented Feb 20, 2024

ChrisChV commented Feb 21, 2024

bradenmacdonald commented Mar 19, 2024

openedx-webhooks commented Mar 19, 2024

ChrisChV commented Jan 17, 2024 •

edited

Loading

openedx-webhooks commented Jan 17, 2024 •

edited

Loading

codecov bot commented Jan 18, 2024 •

edited

Loading

yusuf-musleh left a comment •

edited

Loading

ChrisChV commented Jan 22, 2024 •

edited

Loading