Skip to content

Conversation

@AndyButland
Copy link
Contributor

Prerequisites

  • I have added steps to test this contribution in the description below

Fixes: #20943

Description

As noted on the linked issue, if a picked item has been deleted the index can no longer be relied upon to be correct as an indicator of the item to be removed. This updates to use the UDI of the item instead.

Testing

See reproduction steps in linked issue.

Copilot AI review requested due to automatic review settings November 25, 2025 09:23
Copilot finished reviewing on behalf of AndyButland November 25, 2025 09:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request addresses an issue where removing items from the multi-node content picker fails when a previously picked item has been deleted, as the array index no longer reliably corresponds to the correct item. The fix changes the removal logic from index-based to identifier-based (UDI/ID) lookup.

  • Refactored the remove function to accept and search by identifier (UDI or ID) instead of array index
  • Updated the HTML template to pass node.udi to the remove function instead of $index
  • Added null-check logic to handle cases where the identifier is not found
  • Modified behavior to set model.value to null when removing the last item (aligning with clear() behavior)

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/Umbraco.Web.UI.Client/src/views/propertyeditors/contentpicker/contentpicker.controller.js Refactored remove function from index-based to identifier-based removal with indexOf lookup and null-check handling
src/Umbraco.Web.UI.Client/src/views/propertyeditors/contentpicker/contentpicker.html Updated remove callback to pass node.udi instead of $index
src/Umbraco.Web.UI.Client/test/unit/app/propertyeditors/content-picker-controller.spec.js Updated test to pass identifier value (1231) instead of array index (1) to the remove function
Comments suppressed due to low confidence (1)

src/Umbraco.Web.UI.Client/test/unit/app/propertyeditors/content-picker-controller.spec.js:88

  • The test coverage for the updated remove function is incomplete. Consider adding tests for:
  1. Removing a non-existent item (should handle gracefully when the UDI/ID is not found)
  2. Removing the last remaining item (should set model.value to null)
  3. Attempting to remove when allowRemove is false

These edge cases are now important since the removal logic has changed from index-based to identifier-based lookup.

        it("Removing an item should update renderModel, ids and model.value", function(){
            scope.remove(1231);
            scope.$apply();
            expect(scope.renderModel.length).toBe(2);
            expect(scope.model.value).toBe("1233,23121");
        });

@leekelleher leekelleher self-requested a review December 1, 2025 12:19
Copy link
Member

@leekelleher leekelleher left a comment

Choose a reason for hiding this comment

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

Tested out, works as described. 🚀

@leekelleher leekelleher merged commit 467db77 into v13/dev Dec 1, 2025
20 checks passed
@leekelleher leekelleher deleted the v13/bugfix/remove-mntp-entries-by-udi branch December 1, 2025 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants