feat: container delete confirmation modal #2145

navinkarkera · 2025-06-13T20:31:29Z

Description

Update container delete confirmation modal based on #1982 and #1981

Useful information to include:

Which edX user roles will this change impact? Common user roles are "Learner", "Course Author"

Supporting information

Testing instructions

Checkout feat: container links api edx-platform#36911
Delete section, subsections and units and restore them. Verify different combinations displayed in Confirmation Dialog for Deleting Section/ Subsection #1982

Other information

Include anything else that will help reviewers and consumers understand the change.

Does this change depend on other changes elsewhere?
Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.

openedx-webhooks · 2025-06-13T20:31:34Z

Thanks for the pull request, @navinkarkera!

This repository is currently maintained by @bradenmacdonald.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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.

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.

codecov · 2025-06-14T15:29:18Z

Codecov Report

Attention: Patch coverage is 98.01980% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.14%. Comparing base (71fa247) to head (a2b3056).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/course-libraries/data/api.ts	80.00%	1 Missing ⚠️
.../library-authoring/containers/ContainerDeleter.tsx	98.55%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #2145    +/-   ##
========================================
  Coverage   94.13%   94.14%            
========================================
  Files        1164     1164            
  Lines       24539    24589    +50     
  Branches     5220     5381   +161     
========================================
+ Hits        23101    23150    +49     
+ Misses       1370     1362     -8     
- Partials       68       77     +9

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pomegranited

👍 This is working great @navinkarkera . I left a few nits, including one UX suggestion that's open for discussion.

I tested this on standalone containers of all types, containers with parents, and units used in courses with and without parents. Deleting and restoring all types worked as expected, and the expected messages were shown in the confirmation modal.
I read through the code
I checked for accessibility issues by using my keyboard to navigate
Includes documentation
User-facing strings are extracted for translation -- and templated beautifully :)

pomegranited · 2025-06-16T20:07:00Z

src/library-authoring/components/ContainerCard.tsx

-        containerId={containerKey}
-        displayName={displayName}
-      />
+      {isConfirmingDelete && (


nit: This change looks redundant with this one -- Can we revert one or the other?

pomegranited · 2025-06-16T21:20:45Z

src/library-authoring/components/ContainerDeleter.tsx

+  const { hits } = useContentFromSearchIndex([containerId]);
+  const containerData = (hits as ContainerHit[])?.[0];
+  const {
+    data: dataDownstreamLinks,


nit: While these container entity links are loading, I think we should indicate this somehow, e.g. with a spinner or by disabling the Delete button until they're loaded.

Open for discussion -- I don't know how much of an issue this might be on a real service, but I think it could prevent accidental deletion of linked containers.

pomegranited · 2025-06-16T21:24:13Z

src/library-authoring/components/messages.ts

+  },
+  deleteUnitCourseMessage: {
+    id: 'course-authoring.library-authoring.unit.delete-course-usage-message',
+    defaultMessage: 'This unit is used {courseCount, plural, one {{courseCountText} time} other {{courseCountText} times}} in courses, and will stop receiving updates there.',


nit: Any particular reason why this message has a courseCountText variable and the others don't?

Good catch, they all should have it.

pomegranited · 2025-06-16T21:27:18Z

src/library-authoring/components/ContainerDeleter.tsx

+        return {
+          title: intl.formatMessage(messages.deleteSectionWarningTitle),
+          parentMessage: '',
+          // Update below fields when sections are linked to courses


nit: Do we actually need this comment (or the ones below)? It looks like this code will work without modification once sections are linked to courses, and it already does work for Units in courses.

pomegranited · 2025-06-17T18:18:00Z

src/library-authoring/components/ContainerDeleter.tsx

@@ -1,13 +1,16 @@
-import { useCallback, useContext } from 'react';


nit: I think it make more sense for this ContainerDeleter component to live under containers/ instead of components/?

pomegranited · 2025-06-17T19:12:04Z

src/library-authoring/components/ContainerDeleter.tsx


-  // TODO: support other container types besides 'unit'
-  const deleteWarningTitle = intl.formatMessage(messages.deleteUnitWarningTitle);
  const deleteText = intl.formatMessage(messages.deleteUnitConfirm, {
    unitName: <b>{displayName}</b>,


Since we're now fetching the containerData from the search index, we don't need to pass in the displayName anymore; we can just use the containerData.displayName.

Had to fix a minor bug with invalidating single search index queries for this to work properly on name change.

src/library-authoring/components/ContainerDeleter.test.tsx

src/library-authoring/components/messages.ts

ChrisChV

I found these issues:

When you delete a section or subsection, the message in the toast says "Unit deleted"
For some reason I can't see the status when the unit is in a course, am I missing some configuration?
Missing update after adding a unit to subsection: Steps:
- Create a Unit.
- Open the delete confirmation modal an close it
- Add the unit to a subsection.
- Open the delete confirmation. Bug: The message of the parent subsection doesn't appear

pomegranited · 2025-06-18T22:05:03Z

src/library-authoring/components/ContainerDeleter.test.tsx

+      expect((await screen.findAllByText((_, element) => textMatch.test(element?.textContent || ''))).length).toBeGreaterThan(0);
+    });
+  });
+});


I think we can also reinstate this skipped test.

navinkarkera · 2025-06-24T14:47:02Z

For some reason I can't see the status when the unit is in a course, am I missing some configuration?

Did you checkout openedx/edx-platform#36911? We also probably need to reindex just to be sure.

I have fixed all other issues that you have listed above.

ChrisChV · 2025-06-24T17:02:45Z

For some reason I can't see the status when the unit is in a course, am I missing some configuration?

Did you checkout openedx/edx-platform#36911? We also probably need to reindex just to be sure.

@navinkarkera Sorry, it was an old bad migration in my local.

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Jun 13, 2025

openedx-webhooks added this to Contributions Jun 13, 2025

github-project-automation bot moved this to Needs Triage in Contributions Jun 13, 2025

navinkarkera force-pushed the navin/fal-4167/delete-section-subsection branch from 99684b7 to aa37548 Compare June 14, 2025 13:10

navinkarkera changed the title ~~Navin/fal 4167/delete section subsection~~ feat: container delete confirmation modal Jun 14, 2025

navinkarkera marked this pull request as ready for review June 14, 2025 15:35

navinkarkera mentioned this pull request Jun 14, 2025

feat: container links api openedx/edx-platform#36911

Merged

pomegranited approved these changes Jun 16, 2025

View reviewed changes

pomegranited reviewed Jun 17, 2025

View reviewed changes

ChrisChV reviewed Jun 17, 2025

View reviewed changes

src/library-authoring/components/ContainerDeleter.test.tsx Outdated Show resolved Hide resolved

src/library-authoring/components/messages.ts Outdated Show resolved Hide resolved

src/library-authoring/components/messages.ts Outdated Show resolved Hide resolved

ChrisChV requested changes Jun 17, 2025

View reviewed changes

pomegranited reviewed Jun 18, 2025

View reviewed changes

mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions Jun 20, 2025

navinkarkera added 13 commits June 24, 2025 14:44

feat: container delete modal

4ae73f9

fix: lint issues

09630c2

refactor: container delete messaging

40fbd89

test: container deleter modal

dace6e2

fix: show loading spinner in delete modal

17f90a9

fix: variable name in messages

cb10faa

chore: remove redundant comment

00ad09a

refactor: move container components under containers folder

0c8f75b

fix: invalidating single search query

d8f1820

chore: fix lint issues

448f0ad

refactor: messages

3515dfa

test: container info for subsection and sections

64716e9

fix: toast messages in delete operation

7c54c7b

navinkarkera force-pushed the navin/fal-4167/delete-section-subsection branch from a33b25d to 7c54c7b Compare June 24, 2025 14:14

navinkarkera added 2 commits June 24, 2025 16:24

chore: fix lint issues

1ed89dc

fix: library id in single index query

a2b3056

navinkarkera force-pushed the navin/fal-4167/delete-section-subsection branch from 7022a11 to a2b3056 Compare June 24, 2025 14:45

navinkarkera requested a review from ChrisChV June 24, 2025 14:47

ChrisChV approved these changes Jun 24, 2025

View reviewed changes

ChrisChV merged commit 4905f3b into openedx:master Jun 24, 2025
7 checks passed

github-project-automation bot moved this from In Eng Review to Done in Contributions Jun 24, 2025

		@@ -1,13 +1,16 @@
		import { useCallback, useContext } from 'react';

feat: container delete confirmation modal #2145

feat: container delete confirmation modal #2145

Uh oh!

Conversation

navinkarkera commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Other information

Uh oh!

openedx-webhooks commented Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChrisChV left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navinkarkera commented Jun 24, 2025

Uh oh!

ChrisChV commented Jun 24, 2025

Uh oh!

Uh oh!

Uh oh!

navinkarkera commented Jun 13, 2025 •

edited

Loading

openedx-webhooks commented Jun 13, 2025 •

edited

Loading

codecov bot commented Jun 14, 2025 •

edited

Loading

ChrisChV left a comment •

edited

Loading