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.

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: [FC-0070] implement move xblock modal #1422

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ihor-romaniuk
Copy link
Contributor

@ihor-romaniuk ihor-romaniuk commented Oct 22, 2024

🚨 Dependencies:

Settings

EDX_PLATFORM_REPOSITORY: https://github.com/raccoongang/edx-platform.git
EDX_PLATFORM_VERSION: romaniuk/implement-move-xblock-modal

TUTOR_GROVE_NEW_MFES:
  authoring:
    port: 18000
    repository: https://github.com/raccoongang/frontend-app-course-authoring.git
    version: romaniuk/implement-move-xblock-modal

TUTOR_GROVE_WAFFLE_FLAGS:
  - name: contentstore.new_studio_mfe.use_new_unit_page
    everyone: true

TUTOR_GROVE_MFE_LMS_COMMON_SETTINGS:
  MFE_CONFIG:
    ENABLE_UNIT_PAGE: true

Description

This PR introduces the ability to move an xBlock from one unit to another within a course using a modal window. The feature enables users to select a target unit for the xBlock and confirm the move.

move-xblock.mov

Related Pull Requests

Testing instructions

  • сreate a new course.
  • create several units.
  • open the course unit page.
  • add some xBlocks to the unit.
  • move xBlock to another unit: click three dots icon (xBlock action menu) -> select "Move" -> select new location.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 22, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Oct 22, 2024

Thanks for the pull request, @ihor-romaniuk!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@ihor-romaniuk ihor-romaniuk force-pushed the romaniuk/implement-move-xblock-modal branch from c917065 to 1a9145e Compare November 8, 2024 17:46
@ihor-romaniuk ihor-romaniuk marked this pull request as ready for review November 8, 2024 17:46
@ihor-romaniuk ihor-romaniuk requested a review from a team as a code owner November 8, 2024 17:46
@ihor-romaniuk ihor-romaniuk force-pushed the romaniuk/implement-move-xblock-modal branch from 1a9145e to de9f97c Compare November 8, 2024 18:08
@arbrandes arbrandes self-requested a review November 8, 2024 18:17
@ihor-romaniuk ihor-romaniuk force-pushed the romaniuk/implement-move-xblock-modal branch 3 times, most recently from 0d617b7 to 062e439 Compare November 8, 2024 18:28
@ihor-romaniuk ihor-romaniuk added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Nov 8, 2024
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

Copy link
Contributor

@PKulkoRaccoonGang PKulkoRaccoonGang left a comment

Choose a reason for hiding this comment

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

[inform]: Regarding the failing tests. Apparently after merging one of the last PRs we had problems with tests for the library functionality. I have already fixed this in a new PR
#1493

)).toBeInTheDocument();

const currentSection = courseOutlineInfoMock.child_info.children[1];
const currentSectionItemText = getByRole('button', {
Copy link
Contributor

Choose a reason for hiding this comment

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

[optional]: We are looking for a button here, aren't we? Let's call this variable currentSectionItemBtn

Suggested change
const currentSectionItemText = getByRole('button', {
const currentSectionItemBtn = getByRole('button', {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

name: `${currentSection.display_name} ${moveModalMessages.moveModalOutlineItemCurrentLocationText.defaultMessage} ${moveModalMessages.moveModalOutlineItemViewText.defaultMessage}`,
});
expect(currentSectionItemText).toBeInTheDocument();
fireEvent.click(currentSectionItemText);
Copy link
Contributor

Choose a reason for hiding this comment

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

[optional]: I see that in this file there are tests using fireEvent, but let's use a more correct approach with userEvent

https://blog.mimacom.com/react-testing-library-fireevent-vs-userevent/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

fireEvent.click(currentSectionItemText);

const currentSubsection = currentSection.child_info.children[0];
const currentSubsectionText = getByRole('button', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const currentSubsectionText = getByRole('button', {
const currentSectionItemBtn = getByRole('button', {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

<span
className="category-text"
aria-hidden="true"
data-testId="move-xblock-modal-category"
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit]: data-testid instead of data-testId

Suggested change
data-testId="move-xblock-modal-category"
data-testid="move-xblock-modal-category"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Comment on lines 1131 to 1140
await waitFor(() => {
expect(getByText(unitDisplayName)).toBeInTheDocument();
});

axiosMock
.onGet(getOutlineInfo(courseId))
.reply(200, courseOutlineInfoMock);
await executeThunk(getCourseOutlineInfoQuery(courseId), store.dispatch);

window.dispatchEvent(messageEvent);
Copy link
Contributor

Choose a reason for hiding this comment

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

[optional]: Since we have an asynchronous state update in this section of code, we will most likely get a warning in the test related to wrapping such sections of code in act(). Let's wrap the blocks that asynchronously update states in this test and the following ones in this PR.

Something like this:

Suggested change
await waitFor(() => {
expect(getByText(unitDisplayName)).toBeInTheDocument();
});
axiosMock
.onGet(getOutlineInfo(courseId))
.reply(200, courseOutlineInfoMock);
await executeThunk(getCourseOutlineInfoQuery(courseId), store.dispatch);
window.dispatchEvent(messageEvent);
await act(async () => {
await waitFor(() => {
expect(getByText(unitDisplayName)).toBeInTheDocument();
});
axiosMock.onGet(getOutlineInfo(courseId)).reply(200, courseOutlineInfoMock);
await executeThunk(getCourseOutlineInfoQuery(courseId), store.dispatch);
window.dispatchEvent(messageEvent);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Comment on lines 161 to 163
await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(sections[1].display_name, 'i') })));
await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(subsections[1].display_name, 'i') })));
await waitFor(() => userEvent.click(within(breadcrumbs).getByText(sections[1].display_name)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(sections[1].display_name, 'i') })));
await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(subsections[1].display_name, 'i') })));
await waitFor(() => userEvent.click(within(breadcrumbs).getByText(sections[1].display_name)));
await waitFor(() => {
userEvent.click(getByRole('button', { name: new RegExp(sections[1].display_name, 'i') }));
userEvent.click(getByRole('button', { name: new RegExp(subsections[1].display_name, 'i') }));
userEvent.click(within(breadcrumbs).getByText(sections[1].display_name));
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Comment on lines 107 to 138
it('correctly navigates through the structure list', async () => {
const { getByText, getByRole, getByTestId } = renderComponent();
const breadcrumbs: HTMLElement = getByTestId('move-xblock-modal-breadcrumbs');
const categoryIndicator: HTMLElement = getByTestId('move-xblock-modal-category');

expect(within(breadcrumbs).getByText(messages.moveModalBreadcrumbsBaseCategory.defaultMessage)).toBeInTheDocument();
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsSections.defaultMessage),
).toBeInTheDocument();
sections.map((section) => (
expect(getByText(section.display_name)).toBeInTheDocument()
));

await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(sections[1].display_name, 'i') })));
await waitFor(() => {
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsSubsections.defaultMessage),
).toBeInTheDocument();
expect(within(breadcrumbs).getByText(sections[1].display_name)).toBeInTheDocument();
subsections.map((subsection) => (
expect(getByRole('button', { name: new RegExp(subsection.display_name, 'i') })).toBeInTheDocument()
));
});

await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(subsections[1].display_name, 'i') })));
await waitFor(() => {
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsUnits.defaultMessage),
).toBeInTheDocument();
expect(within(breadcrumbs).getByText(subsections[1].display_name)).toBeInTheDocument();
units.map((unit) => (
expect(getByRole('button', { name: new RegExp(unit.display_name, 'i') })).toBeInTheDocument()
));
});

await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(units[0].display_name, 'i') })));
await waitFor(() => {
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsComponents.defaultMessage),
).toBeInTheDocument();
expect(within(breadcrumbs).getByText(units[0].display_name)).toBeInTheDocument();
components.forEach((component) => {
if (component.display_name) {
expect(getByText(component.display_name)).toBeInTheDocument();
}
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

[optional]: I tried locally to remove some waitFor and unnecessary types and it works. Please check if you need to use waitFor so often in other tests in this file.

Suggested change
it('correctly navigates through the structure list', async () => {
const { getByText, getByRole, getByTestId } = renderComponent();
const breadcrumbs: HTMLElement = getByTestId('move-xblock-modal-breadcrumbs');
const categoryIndicator: HTMLElement = getByTestId('move-xblock-modal-category');
expect(within(breadcrumbs).getByText(messages.moveModalBreadcrumbsBaseCategory.defaultMessage)).toBeInTheDocument();
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsSections.defaultMessage),
).toBeInTheDocument();
sections.map((section) => (
expect(getByText(section.display_name)).toBeInTheDocument()
));
await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(sections[1].display_name, 'i') })));
await waitFor(() => {
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsSubsections.defaultMessage),
).toBeInTheDocument();
expect(within(breadcrumbs).getByText(sections[1].display_name)).toBeInTheDocument();
subsections.map((subsection) => (
expect(getByRole('button', { name: new RegExp(subsection.display_name, 'i') })).toBeInTheDocument()
));
});
await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(subsections[1].display_name, 'i') })));
await waitFor(() => {
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsUnits.defaultMessage),
).toBeInTheDocument();
expect(within(breadcrumbs).getByText(subsections[1].display_name)).toBeInTheDocument();
units.map((unit) => (
expect(getByRole('button', { name: new RegExp(unit.display_name, 'i') })).toBeInTheDocument()
));
});
await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(units[0].display_name, 'i') })));
await waitFor(() => {
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsComponents.defaultMessage),
).toBeInTheDocument();
expect(within(breadcrumbs).getByText(units[0].display_name)).toBeInTheDocument();
components.forEach((component) => {
if (component.display_name) {
expect(getByText(component.display_name)).toBeInTheDocument();
}
});
});
});
it('correctly navigates through the structure list', async () => {
const { getByText, getByRole, getByTestId } = renderComponent();
const breadcrumbs = getByTestId('move-xblock-modal-breadcrumbs');
const categoryIndicator = getByTestId('move-xblock-modal-category');
expect(within(breadcrumbs).getByText(messages.moveModalBreadcrumbsBaseCategory.defaultMessage)).toBeInTheDocument();
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsSections.defaultMessage)
).toBeInTheDocument();
sections.forEach((section) => {
expect(getByText(section.display_name)).toBeInTheDocument();
});
userEvent.click(getByRole('button', { name: new RegExp(sections[1].display_name, 'i') }));
await waitFor(() => {
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsSubsections.defaultMessage)
).toBeInTheDocument();
expect(within(breadcrumbs).getByText(sections[1].display_name)).toBeInTheDocument();
subsections.forEach((subsection) => {
expect(getByRole('button', { name: new RegExp(subsection.display_name, 'i') })).toBeInTheDocument();
});
});
userEvent.click(getByRole('button', { name: new RegExp(subsections[1].display_name, 'i') }));
await waitFor(() => {
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsUnits.defaultMessage)
).toBeInTheDocument();
expect(within(breadcrumbs).getByText(subsections[1].display_name)).toBeInTheDocument();
units.forEach((unit) => {
expect(getByRole('button', { name: new RegExp(unit.display_name, 'i') })).toBeInTheDocument();
});
});
userEvent.click(getByRole('button', { name: new RegExp(units[0].display_name, 'i') }));
await waitFor(() => {
expect(
within(categoryIndicator).getByText(messages.moveModalBreadcrumbsComponents.defaultMessage)
).toBeInTheDocument();
expect(within(breadcrumbs).getByText(units[0].display_name)).toBeInTheDocument();
components.forEach((component) => {
if (component.display_name) {
expect(getByText(component.display_name)).toBeInTheDocument();
}
});
});
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Comment on lines 179 to 181
await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(sections[1].display_name, 'i') })));
await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(subsections[1].display_name, 'i') })));
await waitFor(() => userEvent.click(getByRole('button', { name: new RegExp(units[7].display_name, 'i') })));
Copy link
Contributor

Choose a reason for hiding this comment

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

[optional]: It seems to me that a wrapper of one waitFor will work the same as 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

import MoveModal from './index';
import messages from './messages';

interface CourseOutlineChildInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

[question]: Just a clarification, can we use these TS types inside the component or maybe we are already using them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reused from the component

Comment on lines 11 to 18
export const getXBlockType = (category: string): string => {
switch (true) {
case category === CATEGORIES_KEYS.chapter:
return CATEGORIES_KEYS.section;
case category === CATEGORIES_KEYS.sequential:
return CATEGORIES_KEYS.subsection;
case category === CATEGORIES_KEYS.vertical:
return CATEGORIES_KEYS.unit;
default:
return category;
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

[optional]: I consider switch(true) an anti-pattern if you also suggest using the concatenated approach :)

Suggested change
export const getXBlockType = (category: string): string => {
switch (true) {
case category === CATEGORIES_KEYS.chapter:
return CATEGORIES_KEYS.section;
case category === CATEGORIES_KEYS.sequential:
return CATEGORIES_KEYS.subsection;
case category === CATEGORIES_KEYS.vertical:
return CATEGORIES_KEYS.unit;
default:
return category;
}
};
export const getXBlockType = (category: string): string => {
const categoryMap: { [key: string]: string } = {
[CATEGORIES_KEYS.chapter]: CATEGORIES_KEYS.section,
[CATEGORIES_KEYS.sequential]: CATEGORIES_KEYS.subsection,
[CATEGORIES_KEYS.vertical]: CATEGORIES_KEYS.unit,
};
return categoryMap[category] || category;
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 93.87097% with 19 lines in your changes missing coverage. Please review.

Project coverage is 93.02%. Comparing base (9b4cf87) to head (7429b68).

Files with missing lines Patch % Lines
src/course-unit/context/iFrameContext.tsx 56.25% 7 Missing ⚠️
src/course-unit/data/thunk.js 82.14% 5 Missing ⚠️
src/course-unit/hooks.jsx 72.22% 5 Missing ⚠️
src/course-unit/context/hooks.tsx 85.71% 1 Missing ⚠️
src/course-unit/move-modal/hooks.tsx 98.93% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1422      +/-   ##
==========================================
+ Coverage   93.01%   93.02%   +0.01%     
==========================================
  Files        1048     1059      +11     
  Lines       20480    20789     +309     
  Branches     4402     4402              
==========================================
+ Hits        19049    19339     +290     
- Misses       1361     1382      +21     
+ Partials       70       68       -2     

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

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@PKulkoRaccoonGang PKulkoRaccoonGang added create-sandbox open-craft-grove should create a sandbox environment from this PR and removed create-sandbox open-craft-grove should create a sandbox environment from this PR labels Nov 12, 2024
/**
* Get an object containing course outline data.
* @param {string} courseId - The identifier of the course.
* @returns {Promise<Object>}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible for you to specify the actual shape of the data returned, instead of just Object ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

* Move a unit item to new unit.
* @param {string} sourceLocator - The ID of the item to be moved.
* @param {string} targetParentLocator - The ID of the XBlock associated with the item.
* @returns {Promise<Object>} - A promise that resolves to the response data from the server.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible for you to specify the actual shape of the data returned, instead of just Object ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

EmptyMessage.propTypes = {
category: PropTypes.string.isRequired,
categoryText: PropTypes.string.isRequired,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

You can omit propTypes from .tsx files if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

@ihor-romaniuk ihor-romaniuk force-pushed the romaniuk/implement-move-xblock-modal branch from 54ddf6b to 7429b68 Compare November 13, 2024 11:13
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U
Projects
Status: Waiting on Author
Development

Successfully merging this pull request may close these issues.

6 participants