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

Jump to bottom

feat: Enable taxonomy/tagging feature in MFE by default #971

Closed
wants to merge 11 commits into from
Closed

feat: Enable taxonomy/tagging feature in MFE by default #971

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5fbfe66
refactor: change ENABLE_TAGGING_TAXONOMY_LIST_PAGE waffle flag to DIS…
rpenido Apr 30, 2024
22084d4
feat: create useStudioHomeData hook
rpenido May 2, 2024
c0404ae
test: add queryclientprovider to fix errors
rpenido May 2, 2024
0c6951b
test: add queryclientprovider
rpenido May 2, 2024
b0cd867
chore: update test mock
bradenmacdonald May 7, 2024
5f32f47
chore: update with latest master
bradenmacdonald May 7, 2024
d85522b
feat: finish implementing disable flag for taxonomy/tagging features
bradenmacdonald May 7, 2024
29daee5
perf: don't refetch the "studio home data" too often
bradenmacdonald May 7, 2024
e82c2de
test: increase test coverage
bradenmacdonald May 8, 2024
5d37761
fix: Hide unit tags sidebar if taxonomies disabled
yusuf-musleh May 8, 2024
5b19f45
chore: linting
yusuf-musleh May 8, 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
6 changes: 3 additions & 3 deletions README.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,14 +253,14 @@ Requirements

* ``edx-platform`` Waffle flags:

* ``new_studio_mfe.use_tagging_taxonomy_list_page``: this feature flag must be enabled.
* ``content_tagging.disable_tagging_feature``: this feature flag must NOT be checked.

Configuration
-------------

In additional to the standard settings, the following local configuration items are required:
In additional to the standard settings, the following local configuration items could be used to disable the tagging feature.

* ``ENABLE_TAGGING_TAXONOMY_PAGES``: must be enabled in order to actually present the new Tagging/Taxonomy pages.
* ``DISABLE_TAGGING_FEATURE``: can be checked in order to hide the Tagging/Taxonomy pages.


Developing
Expand Down
4 changes: 2 additions & 2 deletions src/course-outline/card-header/CardHeader.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ const CardHeader = ({
{(isVertical || isSequential) && (
<CardStatus status={status} showDiscussionsEnabledBadge={showDiscussionsEnabledBadge} />
)}
{ getConfig().ENABLE_TAGGING_TAXONOMY_PAGES === 'true' && contentTagCount > 0 && (
{ !([true, 'true'].includes(getConfig().DISABLE_TAGGING_FEATURE)) && contentTagCount > 0 && (
<TagCount count={contentTagCount} onClick={openManageTagsDrawer} />
)}
<Dropdown data-testid={`${namePrefix}-card-header__menu`} onClick={onClickMenuButton}>
Expand Down Expand Up @@ -176,7 +176,7 @@ const CardHeader = ({
>
{intl.formatMessage(messages.menuConfigure)}
</Dropdown.Item>
{getConfig().ENABLE_TAGGING_TAXONOMY_PAGES === 'true' && (
{!([true, 'true'].includes(getConfig().DISABLE_TAGGING_FEATURE)) && (
<Dropdown.Item
data-testid={`${namePrefix}-card-header__menu-manage-tags-button`}
onClick={openManageTagsDrawer}
Expand Down
10 changes: 5 additions & 5 deletions src/course-outline/card-header/CardHeader.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ describe('<CardHeader />', () => {
it('only shows Manage tags menu if the waffle flag is enabled', async () => {
setConfig({
...getConfig(),
ENABLE_TAGGING_TAXONOMY_PAGES: 'false',
DISABLE_TAGGING_FEATURE: 'true',
});
renderComponent();
const menuButton = await screen.findByTestId('subsection-card-header__menu-button');
Expand All @@ -196,7 +196,7 @@ describe('<CardHeader />', () => {
it('shows ContentTagsDrawer when the menu is clicked', async () => {
setConfig({
...getConfig(),
ENABLE_TAGGING_TAXONOMY_PAGES: 'true',
DISABLE_TAGGING_FEATURE: 'false',
});
renderComponent();
const menuButton = await screen.findByTestId('subsection-card-header__menu-button');
Expand Down Expand Up @@ -296,7 +296,7 @@ describe('<CardHeader />', () => {
it('should render tag count if is not zero and the waffle flag is enabled', async () => {
setConfig({
...getConfig(),
ENABLE_TAGGING_TAXONOMY_PAGES: 'true',
DISABLE_TAGGING_FEATURE: 'false',
});
mockGetTagsCount.mockResolvedValue({ 12345: 17 });
renderComponent();
Expand All @@ -306,7 +306,7 @@ describe('<CardHeader />', () => {
it('shouldn render tag count if the waffle flag is disabled', async () => {
setConfig({
...getConfig(),
ENABLE_TAGGING_TAXONOMY_PAGES: 'false',
DISABLE_TAGGING_FEATURE: 'true',
});
mockGetTagsCount.mockResolvedValue({ 12345: 17 });
renderComponent();
Expand All @@ -316,7 +316,7 @@ describe('<CardHeader />', () => {
it('should not render tag count if is zero', () => {
setConfig({
...getConfig(),
ENABLE_TAGGING_TAXONOMY_PAGES: 'true',
DISABLE_TAGGING_FEATURE: 'false',
});
mockGetTagsCount.mockResolvedValue({ 12345: 0 });
renderComponent();
Expand Down
2 changes: 1 addition & 1 deletion src/course-outline/status-bar/StatusBar.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ const StatusBar = ({
</Hyperlink>
</div>
</StatusBarItem>
{getConfig().ENABLE_TAGGING_TAXONOMY_PAGES === 'true' && (
{!([true, 'true'].includes(getConfig().DISABLE_TAGGING_FEATURE)) && (
<StatusBarItem title={intl.formatMessage(messages.courseTagsTitle)}>
<div className="d-flex align-items-center">
<TagCount count={courseTagCount} />
Expand Down
4 changes: 2 additions & 2 deletions src/course-outline/status-bar/StatusBar.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ describe('<StatusBar />', () => {
it('renders the tag count if the waffle flag is enabled', async () => {
setConfig({
...getConfig(),
ENABLE_TAGGING_TAXONOMY_PAGES: 'true',
DISABLE_TAGGING_FEATURE: 'false',
});
const { findByText } = renderComponent();

Expand All @@ -157,7 +157,7 @@ describe('<StatusBar />', () => {
it('doesnt renders the tag count if the waffle flag is disabled', () => {
setConfig({
...getConfig(),
ENABLE_TAGGING_TAXONOMY_PAGES: 'false',
DISABLE_TAGGING_FEATURE: 'true',
});
const { queryByText } = renderComponent();

Expand Down
2 changes: 1 addition & 1 deletion src/header/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export const getToolsMenuItems = ({ studioBaseUrl, courseId, intl }) => ([
href: `${studioBaseUrl}/export/${courseId}`,
title: intl.formatMessage(messages['header.links.exportCourse']),
},
...(getConfig().ENABLE_TAGGING_TAXONOMY_PAGES === 'true'
...(!([true, 'true'].includes(getConfig().DISABLE_TAGGING_FEATURE))
? [{
href: `${studioBaseUrl}/api/content_tagging/v1/object_tags/${courseId}/export/`,
title: intl.formatMessage(messages['header.links.exportTags']),
Expand Down
4 changes: 2 additions & 2 deletions src/header/utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('header utils', () => {
it('should include export tags option', () => {
setConfig({
...getConfig(),
ENABLE_TAGGING_TAXONOMY_PAGES: 'true',
DISABLE_TAGGING_FEATURE: 'false',
});
const actualItemsTitle = getToolsMenuItems(props).map((item) => item.title);
expect(actualItemsTitle).toEqual([
Expand All @@ -46,7 +46,7 @@ describe('header utils', () => {
it('should not include export tags option', () => {
setConfig({
...getConfig(),
ENABLE_TAGGING_TAXONOMY_PAGES: 'false',
DISABLE_TAGGING_FEATURE: 'true',
});
const actualItemsTitle = getToolsMenuItems(props).map((item) => item.title);
expect(actualItemsTitle).toEqual([
Expand Down
4 changes: 2 additions & 2 deletions src/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const App = () => {
{getConfig().ENABLE_ACCESSIBILITY_PAGE === 'true' && (
<Route path="/accessibility" element={<AccessibilityPage />} />
)}
{getConfig().ENABLE_TAGGING_TAXONOMY_PAGES === 'true' && (
{!([true, 'true'].includes(getConfig().DISABLE_TAGGING_FEATURE)) && (
<>
<Route path="/taxonomies" element={<TaxonomyLayout />}>
<Route index element={<TaxonomyListPage />} />
Expand Down Expand Up @@ -121,7 +121,7 @@ initialize({
ENABLE_UNIT_PAGE: process.env.ENABLE_UNIT_PAGE || 'false',
ENABLE_ASSETS_PAGE: process.env.ENABLE_ASSETS_PAGE || 'false',
ENABLE_VIDEO_UPLOAD_PAGE_LINK_IN_CONTENT_DROPDOWN: process.env.ENABLE_VIDEO_UPLOAD_PAGE_LINK_IN_CONTENT_DROPDOWN || 'false',
ENABLE_TAGGING_TAXONOMY_PAGES: process.env.ENABLE_TAGGING_TAXONOMY_PAGES || 'false',
DISABLE_TAGGING_FEATURE: process.env.DISABLE_TAGGING_FEATURE || 'false',
Copy link
Contributor

Choose a reason for hiding this comment

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

This always seems to be falling back to false since the env variable is not being set, regardless of whether the flags is set in the edx-platform or not. So the "Manage Tags" and tags counts always show regardless if the new waffle flag is set or not.

The previous flag has the following env variables set:

https://github.com/openedx/frontend-app-course-authoring/blob/65f45f72f08c1f38ced5d8d302fbdd2406da228d/.env#L36

https://github.com/openedx/frontend-app-course-authoring/blob/65f45f72f08c1f38ced5d8d302fbdd2406da228d/.env.development#L39

https://github.com/openedx/frontend-app-course-authoring/blob/65f45f72f08c1f38ced5d8d302fbdd2406da228d/.env.test#L34

Since it currently doens't seem to be reading it from the edx-platform backend, I'm not sure what the general practice is for this, however maybe we could utilize the taxonomiesEnabled flag that is passed in from the backend:

https://github.com/openedx/frontend-app-course-authoring/blob/65f45f72f08c1f38ced5d8d302fbdd2406da228d/src/studio-home/tabs-section/index.jsx#L32-L37

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @yusuf-musleh!

I think getting the data from the backend is the right approach, but we need to fetch a lot of data to use this selector.

I created a new hook (using react-query) that solves that problem. The only issue is that we need this info in the index.jsx, where we don't have the necessary configs just to call a hook.

We may need to use some kind of "lazy load" for the routes. Maybe @bradenmacdonald could help us with some ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

hey guys, I don't know if there is a good pattern for this now. I think we should just stick to the simplest option and have an environment variable that disables the MFE features (getConfig().DISABLE_TAGGING_FEATURE) and a waffle flag that controls the legacy UI (content_tagging.disabled).

The waffle flag is already set on edx.org, in anticipation of this PR, but we'll also have to get them to set the env var once it's confirmed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I like @rpenido's approach and I think it's even better if we can get it to work. Which index.jsx is the problem? The studio home page or the taxonomies 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.

The root index, where we declare the <App>.

One "easy" approach to avoid this problem (needing a provider at the App level) is to move the check to the destination pages, showing an error message or redirecting to another place if the flag is disabled. I think this is not the best solution, but it will work.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine. We want most people using this, and having it disabled will be fairly rare. The important thing is that reading this flag doesn't slow down the frontend, so shouldn't introduce an additional blocking API request on every page.

Copy link
Contributor

Choose a reason for hiding this comment

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

One "easy" approach to avoid this problem (needing a provider at the App level) is to move the check to the destination pages, showing an error message or redirecting to another place if the flag is disabled.

So let's go with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I implemented that ^

@yusuf-musleh could you please review/test this PR again?

ENABLE_HOME_PAGE_COURSE_API_V2: process.env.ENABLE_HOME_PAGE_COURSE_API_V2 === 'true',
ENABLE_CHECKLIST_QUALITY: process.env.ENABLE_CHECKLIST_QUALITY || 'true',
}, 'CourseAuthoringConfig');
Expand Down
Loading