feat: New delete confirmation modal [FC-0083] #1906

ChrisChV · 2025-05-02T21:36:08Z

Description

A new warning message about units is added when deleting a component.
Uses the units dict on the search document to verify the new state of the delete confirmation modal.
Which edX user roles will this change impact? "Course Author".

Supporting information

Github issue: Delete component that's used in a unit #1685
Depends on: feat: Add units dict to index [FC-0083] edx-platform#36650
Internal ticket: FAL-4099

Testing instructions

Go to the library home of a library
Create 3 components and 2 units.
Add one component to one unit
Add another component to both units.
From the component card in the library home, try to delete the component that is not in any unit. Verify the delete confirmation modal.
Try to delete the component that is in one unit. Verify the delete confirmation modal.
Try to delete the component that is in both units. Verify the delete confirmation modal.
Do the same in the component sidebar and in the block list on the units.

Other information

N/A

openedx-webhooks · 2025-05-02T21:36:12Z

Thanks for the pull request, @ChrisChV!

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-05-02T21:45:54Z

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.82%. Comparing base (8c3fab3) to head (28e5cf4).

Files with missing lines	Patch %	Lines
src/library-authoring/components/ComponentMenu.tsx	50.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #1906    +/-   ##
========================================
  Coverage   93.82%   93.82%            
========================================
  Files        1147     1147            
  Lines       23973    23990    +17     
  Branches     5054     5167   +113     
========================================
+ Hits        22492    22508    +16     
- Misses       1413     1414     +1     
  Partials       68       68

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

rpenido

LGTM 👍
Thank you for your work, @ChrisChV!

I tested this using the instructions from the PR
I read through the code
I checked for accessibility issues
Includes documentation

I left one non-blocking comment about moving some things around.
Let me know what you think!

rpenido · 2025-05-06T18:39:04Z

src/library-authoring/component-info/ComponentInfo.tsx

+  const { client, indexName } = useContentSearchConnection();
+  const {
+    hits: componentHit,
+  } = useContentSearchResults({
+    client,
+    indexName,
+    searchKeywords: '',
+    extraFilter: [`usage_key IN ["${usageKey}"]`],
+    limit: 1,
+    enabled: true,
+    skipBlockTypeFetch: true,
+  });
+


What do you think about creating a useComponentUnits hook for this?
Also, I believe that if you move the fetch call to the ComponentDeleter, you will not need to replicate this on the LibraryUntiBlocks.

I added the useComponentsFromSearchIndex in 917421d.

About moving the fetch all to the ComponentDeleter, I think I would add more calls, in the case of the card component, that information is already available from the search index, and if we add it to the ComponentDeleter, it would be called again

@ChrisChV Can we move the call inside of ComponentMenu while still accepting units prop and only fetch if the prop is not passed?

Or another solution would be to put it inside the ComponentDeleter and change to render it conditionally, like {isConfirmingDelete && <ComponentDeleter usageKey={usageKey} isConfirmingDelete={isConfirmingDelete} cancelDelete={cancelDelete} />}.
That way, the component is only rendered when the dialog opens, saving the extra calls.

Refactored here: 6291f67

navinkarkera · 2025-05-07T05:07:17Z

src/library-authoring/component-info/ComponentInfo.tsx

+            <ComponentMenu
+              usageKey={usageKey}
+              unitsData={(componentHit as ContentHit[])?.[0]?.units}
+            />


@ChrisChV It would be simpler to call useComponentsFromSearchIndex inside of ComponentMenu or ComponentDeleter (preferably former) component instead of passing through props.

Edit: Ahh, I missed the comment from @rpenido above.

Refactored here: 6291f67

ChrisChV · 2025-05-08T01:26:29Z

@navinkarkera This is ready for another review. I asked Product about the final design: #1685 (comment)

navinkarkera

@ChrisChV Looks good 👍

I tested this: (Verified units info in delete component)
I read through the code
I checked for accessibility issues
Includes documentation

src/library-authoring/component-info/ComponentInfo.tsx

src/library-authoring/components/ComponentDeleter.tsx

ChrisChV · 2025-05-17T19:24:12Z

@bradenmacdonald This is ready for your review as CC

ChrisChV added 3 commits May 2, 2025 14:50

feat: New units message on delete confirm modal when deleting components

9b4ea0a

test: Tests for new delete confirmation messages

ffff000

Merge branch 'master' into chris/FAL-4099-new-delete-confirmation-modal

f6ff398

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 2, 2025

openedx-webhooks added this to Contributions May 2, 2025

ChrisChV marked this pull request as draft May 2, 2025 21:36

github-project-automation bot moved this to Needs Triage in Contributions May 2, 2025

ChrisChV changed the title ~~Chris/fal 4099 new delete confirmation modal~~ feat: New delete confirmation modal [FC-0083] May 2, 2025

ChrisChV marked this pull request as ready for review May 2, 2025 22:11

ChrisChV mentioned this pull request May 2, 2025

feat: Add units dict to index [FC-0083] openedx/edx-platform#36650

Merged

rpenido approved these changes May 6, 2025

View reviewed changes

mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions May 6, 2025

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label May 6, 2025

refactor: useComponentsFromSearchIndex added

917421d

navinkarkera suggested changes May 7, 2025

View reviewed changes

ChrisChV added 2 commits May 7, 2025 18:05

Merge branch 'master' into chris/FAL-4099-new-delete-confirmation-modal

f93b3f4

refactor: Move fetch units to ComponentDeleter

6291f67

ChrisChV mentioned this pull request May 8, 2025

Delete component that's used in a unit #1685

Closed

navinkarkera approved these changes May 8, 2025

View reviewed changes

src/library-authoring/component-info/ComponentInfo.tsx Outdated Show resolved Hide resolved

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

style: Delete unnecesary code

28e5cf4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: New delete confirmation modal [FC-0083] #1906

feat: New delete confirmation modal [FC-0083] #1906

ChrisChV commented May 2, 2025 •

edited

Loading

openedx-webhooks commented May 2, 2025 •

edited

Loading

codecov bot commented May 2, 2025 •

edited

Loading

rpenido left a comment

rpenido May 6, 2025

ChrisChV May 6, 2025

navinkarkera May 7, 2025

rpenido May 7, 2025

ChrisChV May 8, 2025

navinkarkera May 7, 2025 •

edited

Loading

ChrisChV May 8, 2025

ChrisChV commented May 8, 2025

navinkarkera left a comment

ChrisChV commented May 17, 2025

feat: New delete confirmation modal [FC-0083] #1906

Are you sure you want to change the base?

feat: New delete confirmation modal [FC-0083] #1906

Conversation

ChrisChV commented May 2, 2025 • edited Loading

Description

Supporting information

Testing instructions

Other information

openedx-webhooks commented May 2, 2025 • edited Loading

codecov bot commented May 2, 2025 • edited Loading

Codecov Report

rpenido left a comment

Choose a reason for hiding this comment

rpenido May 6, 2025

Choose a reason for hiding this comment

ChrisChV May 6, 2025

Choose a reason for hiding this comment

navinkarkera May 7, 2025

Choose a reason for hiding this comment

rpenido May 7, 2025

Choose a reason for hiding this comment

ChrisChV May 8, 2025

Choose a reason for hiding this comment

navinkarkera May 7, 2025 • edited Loading

Choose a reason for hiding this comment

ChrisChV May 8, 2025

Choose a reason for hiding this comment

ChrisChV commented May 8, 2025

navinkarkera left a comment

Choose a reason for hiding this comment

ChrisChV commented May 17, 2025

ChrisChV commented May 2, 2025 •

edited

Loading

openedx-webhooks commented May 2, 2025 •

edited

Loading

codecov bot commented May 2, 2025 •

edited

Loading

navinkarkera May 7, 2025 •

edited

Loading