Fix remove selected Item loop

packages/core/src/collection/index.ts

@@ -979,26 +979,29 @@ export class Collection<DataType extends Object = DefaultItem> {
     for (const groupKey in this.groups) {
       const group = this.getGroup(groupKey, { notExisting: true });
       if (!group?.has(oldItemKey)) continue;
-      group.replace(oldItemKey, newItemKey, { background: config.background });
+      group?.replace(oldItemKey, newItemKey, { background: config.background });
     }
 
     // Update ItemKey in Selectors
     for (const selectorKey in this.selectors) {
       const selector = this.getSelector(selectorKey, { notExisting: true });
-      if (!selector) continue;
-
-      // Reselect Item in existing Selector which has selected the newItemKey
-      if (selector.hasSelected(newItemKey)) {
-        selector.select(newItemKey, {
+      if (selector == null) continue;
+
+      // Reselect Item in Selector that has selected the newItemKey
+      // Necessary because the reference placeholder Item got removed
+      // and replaced with the new Item (Item of which the primaryKey was renamed)
+      // -> needs to find new Item with the same itemKey
+      if (selector.hasSelected(newItemKey, false)) {
+        selector.reselect({
           force: true, // Because ItemKeys are the same
           background: config.background,
         });
       }
 
-      // Select newItemKey in existing Selector which has selected the oldItemKey
-      if (selector.hasSelected(oldItemKey))
+      // Select newItemKey in Selector that has selected the oldItemKey
+      if (selector.hasSelected(oldItemKey, false))
         selector.select(newItemKey, {
-          background: config?.background,
+          background: config.background,
         });
     }
 
@@ -1034,12 +1037,12 @@ export class Collection<DataType extends Object = DefaultItem> {
     itemKeys: ItemKey | Array<ItemKey>
   ): {
     fromGroups: (groups: Array<ItemKey> | ItemKey) => Collection<DataType>;
-    everywhere: () => Collection<DataType>;
+    everywhere: (config?: RemoveItemsConfigInterface) => Collection<DataType>;
   } {
     return {
       fromGroups: (groups: Array<ItemKey> | ItemKey) =>
         this.removeFromGroups(itemKeys, groups),
-      everywhere: () => this.removeItems(itemKeys),
+      everywhere: (config) => this.removeItems(itemKeys, config || {}),
     };
   }
 
@@ -1088,13 +1091,22 @@ export class Collection<DataType extends Object = DefaultItem> {
    * @public
    * Removes Item completely from Collection
    * @param itemKeys - ItemKey/s of Item/s
+   * @param config - Config
    */
-  public removeItems(itemKeys: ItemKey | Array<ItemKey>): this {
+  public removeItems(
+    itemKeys: ItemKey | Array<ItemKey>,
+    config: RemoveItemsConfigInterface = {}
+  ): this {
+    config = defineConfig(config, {
+      notExisting: false,
+      removeSelector: false,
+    });
     const _itemKeys = normalizeArray<ItemKey>(itemKeys);
 
     _itemKeys.forEach((itemKey) => {
-      const item = this.getItem(itemKey, { notExisting: true });
+      const item = this.getItem(itemKey, { notExisting: config.notExisting });
       if (item == null) return;
+      const wasPlaceholder = item.isPlaceholder;
 
       // Remove Item from Groups
       for (const groupKey in this.groups) {
@@ -1108,14 +1120,21 @@ export class Collection<DataType extends Object = DefaultItem> {
       // Remove Item from Collection
       delete this.data[itemKey];
 
-      // Reselect Item in Selectors (to create new dummyItem that holds reference)
+      // Reselect or remove Selectors representing the removed Item
       for (const selectorKey in this.selectors) {
         const selector = this.getSelector(selectorKey, { notExisting: true });
-        if (selector?.hasSelected(itemKey))
-          selector?.select(itemKey, { force: true });
+        if (selector?.hasSelected(itemKey, false)) {
+          if (config.removeSelector) {
+            // Remove Selector
+            this.removeSelector(selector?._key ?? 'unknown');
+          } else {
+            // Reselect Item in Selector (to create new dummyItem to hold a reference to this removed Item)
+            selector?.reselect({ force: true });
+          }
+        }
       }
 
-      this.size--;
+      if (!wasPlaceholder) this.size--;
     });
 
     return this;
@@ -1302,6 +1321,18 @@ export interface CollectionPersistentConfigInterface {
   defaultStorageKey?: StorageKey;
 }
 
+/*
+ * @param notExisting - If not existing Items like placeholder Items can be removed.
+ * Keep in mind that sometimes it won't remove the Item entirely
+ * because another Instance (like a Selector) needs to keep reference to it.
+ * https://github.com/agile-ts/agile/pull/152
+ * @param - If Selectors that have selected an Item to be removed, should be removed too
+ */
+export interface RemoveItemsConfigInterface {
+  notExisting?: boolean;
+  removeSelector?: boolean;
+}
+
 /**
  * @param patch - If Data gets patched into existing Item
  * @param background - If assigning Data happens in background

packages/core/src/collection/selector.ts

@@ -143,8 +143,8 @@ export class Selector<DataType extends Object = DefaultItem> extends State<
   /**
    * @public
    * Reselects current Item
-   * Might help if the Selector failed to properly select an Item.
-   * You can check with 'hasSelected()' if an Item got properly selected.
+   * Might help if the Selector failed to select an Item correctly.
+   * You can check with 'hasSelected()' if an Item got correctly selected.
    * @param config - Config
    */
   public reselect(config: StateRuntimeJobConfigInterface = {}): this {
@@ -196,13 +196,17 @@ export class Selector<DataType extends Object = DefaultItem> extends State<
   /**
    * Checks if Selector has correctly selected the Item at the passed itemKey
    * @param itemKey - ItemKey
+   * @param correctlySelected - If it should consider only correctly selected Items
    */
-  public hasSelected(itemKey: ItemKey): boolean {
-    return (
-      this._itemKey === itemKey &&
-      this.item != null &&
-      this.item.selectedBy.has(this._key as any)
-    );
+  public hasSelected(itemKey: ItemKey, correctlySelected = true): boolean {
+    if (correctlySelected) {
+      return (
+        this._itemKey === itemKey &&
+        this.item != null &&
+        this.item.selectedBy.has(this._key as any)
+      );
+    }
+    return this._itemKey === itemKey;
   }
 
   //=========================================================================================================

packages/core/src/runtime/subscription/sub.controller.ts

@@ -283,7 +283,7 @@ export class SubController {
 
     Agile.logger.if
       .tag(['runtime', 'subscription'])
-      .info('15:01:03', callbackSubscriptionContainer);
+      .info(LogCodeManager.getLog('15:01:03'), callbackSubscriptionContainer);
 
     return callbackSubscriptionContainer;
   }

packages/core/tests/unit/collection/collection.test.ts

@@ -1903,6 +1903,9 @@ describe('Collection Tests', () => {
         dummySelector1.select = jest.fn();
         dummySelector2.select = jest.fn();
         dummySelector3.select = jest.fn();
+        dummySelector1.reselect = jest.fn();
+        dummySelector2.reselect = jest.fn();
+        dummySelector3.reselect = jest.fn();
       });
 
       it('should update ItemKey in Collection, Selectors and Groups (default config)', () => {
@@ -1935,7 +1938,7 @@ describe('Collection Tests', () => {
           background: false,
         });
         expect(dummySelector2.select).not.toHaveBeenCalled();
-        expect(dummySelector3.select).toHaveBeenCalledWith('newDummyItem', {
+        expect(dummySelector3.reselect).toHaveBeenCalledWith({
           force: true,
           background: false,
         });
@@ -1975,7 +1978,7 @@ describe('Collection Tests', () => {
         expect(dummySelector1.select).toHaveBeenCalledWith('newDummyItem', {
           background: true,
         });
-        expect(dummySelector3.select).toHaveBeenCalledWith('newDummyItem', {
+        expect(dummySelector3.reselect).toHaveBeenCalledWith({
           force: true,
           background: true,
         });
@@ -2012,7 +2015,7 @@ describe('Collection Tests', () => {
         expect(dummySelector1.select).toHaveBeenCalledWith('newDummyItem', {
           background: false,
         });
-        expect(dummySelector3.select).toHaveBeenCalledWith('newDummyItem', {
+        expect(dummySelector3.reselect).toHaveBeenCalledWith({
           force: true,
           background: false,
         });
@@ -2109,11 +2112,26 @@ describe('Collection Tests', () => {
         expect(collection.removeItems).not.toHaveBeenCalled();
       });
 
-      it('should remove Items from everywhere', () => {
+      it('should remove Items from everywhere (default config)', () => {
         collection.remove(['test1', 'test2']).everywhere();
 
         expect(collection.removeFromGroups).not.toHaveBeenCalled();
-        expect(collection.removeItems).toHaveBeenCalledWith(['test1', 'test2']);
+        expect(collection.removeItems).toHaveBeenCalledWith(
+          ['test1', 'test2'],
+          {}
+        );
+      });
+
+      it('should remove Items from everywhere (specific config)', () => {
+        collection
+          .remove(['test1', 'test2'])
+          .everywhere({ removeSelector: true, notExisting: true });
+
+        expect(collection.removeFromGroups).not.toHaveBeenCalled();
+        expect(collection.removeItems).toHaveBeenCalledWith(
+          ['test1', 'test2'],
+          { removeSelector: true, notExisting: true }
+        );
       });
     });
 
@@ -2194,15 +2212,22 @@ describe('Collection Tests', () => {
       let dummyGroup2: Group<ItemInterface>;
       let dummyItem1: Item<ItemInterface>;
       let dummyItem2: Item<ItemInterface>;
+      let placeholderItem: Item<ItemInterface>;
 
       beforeEach(() => {
         dummyItem1 = new Item(collection, { id: 'dummyItem1', name: 'Jeff' });
         dummyItem1.persistent = new StatePersistent(dummyItem1);
         dummyItem2 = new Item(collection, { id: 'dummyItem2', name: 'Hans' });
         dummyItem2.persistent = new StatePersistent(dummyItem2);
+        placeholderItem = new Item(
+          collection,
+          { id: 'placeholderItem', name: 'placeholder' },
+          { isPlaceholder: true }
+        );
         collection.data = {
           dummyItem1: dummyItem1,
           dummyItem2: dummyItem2,
+          placeholderItem: placeholderItem,
         };
         collection.size = 2;
 
@@ -2234,16 +2259,19 @@ describe('Collection Tests', () => {
         dummyGroup1.remove = jest.fn();
         dummyGroup2.remove = jest.fn();
 
-        dummySelector1.select = jest.fn();
-        dummySelector2.select = jest.fn();
+        dummySelector1.reselect = jest.fn();
+        dummySelector2.reselect = jest.fn();
+
+        collection.removeSelector = jest.fn();
       });
 
-      it('should remove Item from Collection, Groups and Selectors', () => {
+      it('should remove Item from Collection, Groups and reselect Selectors (default config)', () => {
         collection.removeItems('dummyItem1');
 
         expect(collection.data).not.toHaveProperty('dummyItem1');
         expect(collection.data).toHaveProperty('dummyItem2');
         expect(collection.size).toBe(1);
+        expect(collection.removeSelector).not.toHaveBeenCalled();
 
         expect(dummyItem1.persistent?.removePersistedValue).toHaveBeenCalled();
         expect(
@@ -2253,18 +2281,19 @@ describe('Collection Tests', () => {
         expect(dummyGroup1.remove).toHaveBeenCalledWith('dummyItem1');
         expect(dummyGroup2.remove).not.toHaveBeenCalled();
 
-        expect(dummySelector1.select).toHaveBeenCalledWith('dummyItem1', {
+        expect(dummySelector1.reselect).toHaveBeenCalledWith({
           force: true,
         });
-        expect(dummySelector2.select).not.toHaveBeenCalled();
+        expect(dummySelector2.reselect).not.toHaveBeenCalled();
       });
 
-      it('should remove Items from Collection, Groups and Selectors', () => {
+      it('should remove Items from Collection, Groups and reselect Selectors (default config)', () => {
         collection.removeItems(['dummyItem1', 'dummyItem2', 'notExistingItem']);
 
         expect(collection.data).not.toHaveProperty('dummyItem1');
         expect(collection.data).not.toHaveProperty('dummyItem2');
         expect(collection.size).toBe(0);
+        expect(collection.removeSelector).not.toHaveBeenCalled();
 
         expect(dummyItem1.persistent?.removePersistedValue).toHaveBeenCalled();
         expect(dummyItem2.persistent?.removePersistedValue).toHaveBeenCalled();
@@ -2274,13 +2303,54 @@ describe('Collection Tests', () => {
         expect(dummyGroup2.remove).not.toHaveBeenCalledWith('dummyItem1');
         expect(dummyGroup2.remove).toHaveBeenCalledWith('dummyItem2');
 
-        expect(dummySelector1.select).toHaveBeenCalledWith('dummyItem1', {
+        expect(dummySelector1.reselect).toHaveBeenCalledWith({
           force: true,
         });
-        expect(dummySelector2.select).toHaveBeenCalledWith('dummyItem2', {
+        expect(dummySelector2.reselect).toHaveBeenCalledWith({
           force: true,
         });
       });
+
+      it('should remove Item from Collection, Groups and remove Selectors (removeSelector = true)', () => {
+        collection.removeItems('dummyItem1', { removeSelector: true });
+
+        expect(collection.data).not.toHaveProperty('dummyItem1');
+        expect(collection.data).toHaveProperty('dummyItem2');
+        expect(collection.size).toBe(1);
+        expect(collection.removeSelector).toHaveBeenCalledTimes(1);
+        expect(collection.removeSelector).toHaveBeenCalledWith(
+          dummySelector1._key
+        );
+
+        expect(dummyItem1.persistent?.removePersistedValue).toHaveBeenCalled();
+        expect(
+          dummyItem2.persistent?.removePersistedValue
+        ).not.toHaveBeenCalled();
+
+        expect(dummyGroup1.remove).toHaveBeenCalledWith('dummyItem1');
+        expect(dummyGroup2.remove).not.toHaveBeenCalled();
+
+        expect(dummySelector1.reselect).not.toHaveBeenCalled();
+        expect(dummySelector2.reselect).not.toHaveBeenCalled();
+      });
+
+      it("shouldn't remove placeholder Items from Collection (default config)", () => {
+        collection.removeItems(['dummyItem1', 'placeholderItem']);
+
+        expect(collection.data).toHaveProperty('placeholderItem');
+        expect(collection.data).not.toHaveProperty('dummyItem1');
+        expect(collection.size).toBe(1);
+      });
+
+      it('should remove placeholder Items from Collection (config.notExisting = true)', () => {
+        collection.removeItems(['dummyItem1', 'placeholderItem'], {
+          notExisting: true,
+        });
+
+        expect(collection.data).not.toHaveProperty('placeholderItem');
+        expect(collection.data).not.toHaveProperty('dummyItem1');
+        expect(collection.size).toBe(1);
+      });
     });
 
     describe('setData function tests', () => {

packages/core/tests/unit/collection/selector.test.ts

@@ -651,21 +651,37 @@ describe('Selector Tests', () => {
         expect(selector.hasSelected('dummyItemKey')).toBeTruthy();
       });
 
-      it("should return false if Selector hasn't selected itemKey correctly (itemKey == undefined)", () => {
+      it("should return false if Selector hasn't selected itemKey correctly (itemKey = undefined)", () => {
         expect(selector.hasSelected('notSelectedItemKey')).toBeFalsy();
       });
 
-      it("should return false if Selector hasn't selected itemKey correctly (item == undefined)", () => {
+      it("should return false if Selector hasn't selected itemKey correctly (itemKey = undefined, correctlySelected = false)", () => {
+        expect(selector.hasSelected('notSelectedItemKey', false)).toBeFalsy();
+      });
+
+      it("should return false if Selector hasn't selected itemKey correctly (item = undefined)", () => {
         selector.item = undefined;
 
         expect(selector.hasSelected('dummyItemKey')).toBeFalsy();
       });
 
+      it("should return true if Selector hasn't selected itemKey correctly (item = undefined, correctlySelected = false)", () => {
+        selector.item = undefined;
+
+        expect(selector.hasSelected('dummyItemKey', false)).toBeTruthy();
+      });
+
       it("should return false if Selector has selected itemKey correctly and Item isn't isSelected", () => {
         if (selector.item) selector.item.selectedBy = new Set();
 
         expect(selector.hasSelected('dummyItemKey')).toBeFalsy();
       });
+
+      it("should return true if Selector has selected itemKey correctly and Item isn't isSelected (correctlySelected = false)", () => {
+        if (selector.item) selector.item.selectedBy = new Set();
+
+        expect(selector.hasSelected('dummyItemKey', false)).toBeTruthy();
+      });
     });
 
     describe('rebuildSelector function tests', () => {