Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

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

Closed
wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
55276c8
feat: Send message after update tags on content
ChrisChV Jan 17, 2024
786a04e
feat: Send messages with updated tags count from manage tags drawer
ChrisChV Jan 18, 2024
78db929
docs: Comments added
ChrisChV Jan 22, 2024
04f5bf9
docs: Comments added
ChrisChV Jan 22, 2024
e60c559
test: Fix coverage
ChrisChV Jan 22, 2024
32a5f9f
Merge branch 'master' into chris/FAL-3601-ux-refinements-v2
ChrisChV Feb 12, 2024
2170664
refactor: Send Json message on tags drawer update
ChrisChV Feb 12, 2024
076ef75
Merge branch 'master' into chris/FAL-3601-ux-refinements-v2
ChrisChV Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/content-tags-drawer/data/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export const getTaxonomyTagsApiUrl = (taxonomyId, options = {}) => {
return url.href;
};
export const getContentTaxonomyTagsApiUrl = (contentId) => new URL(`api/content_tagging/v1/object_tags/${contentId}/`, getApiBaseUrl()).href;
export const getContentTaxonomyTagsCountApiUrl = (contentId) => new URL(`api/content_tagging/v1/object_tag_counts/${contentId}/?count_implicit`, getApiBaseUrl()).href;
export const getXBlockContentDataApiURL = (contentId) => new URL(`/xblock/outline/${contentId}`, getApiBaseUrl()).href;
export const getLibraryContentDataApiUrl = (contentId) => new URL(`/api/libraries/v2/blocks/${contentId}/`, getApiBaseUrl()).href;

Expand All @@ -54,6 +55,16 @@ export async function getContentTaxonomyTagsData(contentId) {
return camelCaseObject(data[contentId]);
}

/**
* Get the count of the tags that are applied to the content object
* @param {string} contentId The id of the content object to fetch the applied tags for
* @returns {Promise<Object>}
*/
export async function getContentTaxonomyTagsCountData(contentId) {
const { data } = await getAuthenticatedHttpClient().get(getContentTaxonomyTagsCountApiUrl(contentId));
return camelCaseObject(data[contentId]);
}

/**
* Fetch meta data (eg: display_name) about the content object (unit/compoenent)
* @param {string} contentId The id of the content object (unit/component)
Expand Down
13 changes: 13 additions & 0 deletions src/content-tags-drawer/data/api.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,12 @@ import {
import {
getTaxonomyTagsApiUrl,
getContentTaxonomyTagsApiUrl,
getContentTaxonomyTagsCountApiUrl,
getXBlockContentDataApiURL,
getLibraryContentDataApiUrl,
getTaxonomyTagsData,
getContentTaxonomyTagsData,
getContentTaxonomyTagsCountData,
getContentData,
updateContentTaxonomyTags,
} from './api';
Expand Down Expand Up @@ -88,6 +90,17 @@ describe('content tags drawer api calls', () => {
expect(result).toEqual(contentTaxonomyTagsMock[contentId]);
});

it('should get content taxonomy tags count', async () => {
const contentId = 'block-v1:SampleTaxonomyOrg1+STC1+2023_1+type@vertical+block@aaf8b8eb86b54281aeeab12499d2cb0b';
const dataMock = {};
dataMock[contentId] = 1000;
axiosMock.onGet(getContentTaxonomyTagsCountApiUrl(contentId)).reply(200, dataMock);
const result = await getContentTaxonomyTagsCountData(contentId);

expect(axiosMock.history.get[0].url).toEqual(getContentTaxonomyTagsCountApiUrl(contentId));
expect(result).toEqual(1000);
});

it('should get content data for course component', async () => {
const contentId = 'block-v1:SampleTaxonomyOrg1+STC1+2023_1+type@vertical+block@aaf8b8eb86b54281aeeab12499d2cb0b';
axiosMock.onGet(getXBlockContentDataApiURL(contentId)).reply(200, contentDataMock);
Expand Down
27 changes: 27 additions & 0 deletions src/content-tags-drawer/data/apiHooks.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// @ts-check
import { useMemo } from 'react';
import { getConfig } from '@edx/frontend-platform';
import {
useQuery,
useQueries,
Expand All @@ -9,6 +10,7 @@
import {
getTaxonomyTagsData,
getContentTaxonomyTagsData,
getContentTaxonomyTagsCountData,
getContentData,
updateContentTaxonomyTags,
} from './api';
Expand Down Expand Up @@ -142,5 +144,30 @@
onSettled: () => {
queryClient.invalidateQueries({ queryKey: ['contentTaxonomyTags', contentId] });
},
onSuccess: () => {

Check warning on line 147 in src/content-tags-drawer/data/apiHooks.jsx

View check run for this annotation

Codecov / codecov/patch

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

Added line #L147 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

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

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."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated here: 78db929

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks!

if (window.top != null) {
// Sends content tags to the parent page if is called from a iframe.
getContentTaxonomyTagsData(contentId).then((data) => {
const messageJson = {

Check warning on line 151 in src/content-tags-drawer/data/apiHooks.jsx

View check run for this annotation

Codecov / codecov/patch

src/content-tags-drawer/data/apiHooks.jsx#L150-L151 

Added lines #L150 - L151 were not covered by tests
contentId,
...data,
};
window.top?.postMessage(

Check warning on line 155 in src/content-tags-drawer/data/apiHooks.jsx

View check run for this annotation

Codecov / codecov/patch

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

Added line #L155 was not covered by tests
`[Manage tags drawer] Tags updated: ${JSON.stringify(messageJson)}`,
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

getConfig().STUDIO_BASE_URL,
);
});
getContentTaxonomyTagsCountData(contentId).then((data) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

const messageJson = {

Check warning on line 161 in src/content-tags-drawer/data/apiHooks.jsx

View check run for this annotation

Codecov / codecov/patch

src/content-tags-drawer/data/apiHooks.jsx#L160-L161 

Added lines #L160 - L161 were not covered by tests
contentId,
count: data,
};
window.top?.postMessage(

Check warning on line 165 in src/content-tags-drawer/data/apiHooks.jsx

View check run for this annotation

Codecov / codecov/patch

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

Added line #L165 was not covered by tests
`[Manage tags drawer] Count updated: ${JSON.stringify(messageJson)}`,
getConfig().STUDIO_BASE_URL,
);
});
}
},
});
};