Merge pull request #11081 from growilabs/refactor/181356-182878-news-inappnotification-stale-item-deletion

refactor(news-inappnotification): 181356-182878 refactor stale item deletion

apps/app/src/features/news/server/services/news-cron-service.spec.ts

@@ -1,17 +1,17 @@
 // Hoisted mocks
 const mocks = vi.hoisted(() => {
   const upsertNewsItems = vi.fn();
-  const deleteNewsItemsByExternalIds = vi.fn();
+  const deleteItemsNotInFeed = vi.fn();
   const mockFetch = vi.fn();
   const getGrowiVersion = vi.fn(() => '7.5.0');
 
   return {
     NewsService: vi.fn(() => ({
       upsertNewsItems,
-      deleteNewsItemsByExternalIds,
+      deleteItemsNotInFeed,
     })),
     upsertNewsItems,
-    deleteNewsItemsByExternalIds,
+    deleteItemsNotInFeed,
     mockFetch,
     getGrowiVersion,
   };
@@ -144,7 +144,10 @@ describe('NewsCronService', () => {
       await service.executeJob();
 
       expect(mocks.upsertNewsItems).toHaveBeenCalledWith(VALID_FEED.items);
-      expect(mocks.deleteNewsItemsByExternalIds).toHaveBeenCalledWith([]);
+      expect(mocks.deleteItemsNotInFeed).toHaveBeenCalledWith([
+        'item-001',
+        'item-002',
+      ]);
     });
 
     test('should NOT update DB when fetch fails', async () => {
@@ -154,7 +157,7 @@ describe('NewsCronService', () => {
       await service.executeJob();
 
       expect(mocks.upsertNewsItems).not.toHaveBeenCalled();
-      expect(mocks.deleteNewsItemsByExternalIds).not.toHaveBeenCalled();
+      expect(mocks.deleteItemsNotInFeed).not.toHaveBeenCalled();
     });
 
     test('should NOT update DB when fetch throws', async () => {
@@ -228,6 +231,51 @@ describe('NewsCronService', () => {
       );
     });
 
+    // Regression for Requirement 1.3: items removed from the feed must be
+    // deleted from the local DB. Earlier code computed `idsToDelete` from
+    // `feedJson.items` only, so DB items absent from the feed were never
+    // cleaned up. The cron must now hand the full set of feed externalIds
+    // to `deleteItemsNotInFeed`, which uses a $nin filter to remove the rest.
+    test('should pass every feed externalId to deleteItemsNotInFeed (regression for stale-item bug)', async () => {
+      process.env.NEWS_FEED_URL = 'https://example.com/feed.json';
+      const feed = {
+        version: '1.0',
+        items: [
+          {
+            id: 'still-present-1',
+            title: { ja_JP: 'still present 1' },
+            publishedAt: '2026-01-01T00:00:00Z',
+          },
+          {
+            id: 'still-present-2',
+            title: { ja_JP: 'still present 2' },
+            publishedAt: '2026-01-02T00:00:00Z',
+          },
+          // Item present in feed but version-filtered out — must remain in
+          // the deletion safelist so it is not wiped from the DB.
+          {
+            id: 'version-filtered',
+            title: { ja_JP: 'version filtered' },
+            publishedAt: '2026-01-03T00:00:00Z',
+            conditions: { growiVersionRegExps: ['^999\\.'] },
+          },
+        ],
+      };
+      mocks.mockFetch.mockResolvedValue(mockResponse(feed));
+
+      await service.executeJob();
+
+      // The argument is the *full* feed externalId list, not the
+      // version-matched subset. Items absent from this list (e.g. an
+      // earlier `removed-from-feed` item still in the DB) will be
+      // deleted by the service via `$nin`.
+      expect(mocks.deleteItemsNotInFeed).toHaveBeenCalledWith([
+        'still-present-1',
+        'still-present-2',
+        'version-filtered',
+      ]);
+    });
+
     test('should skip when response body exceeds size limit (5 MiB)', async () => {
       process.env.NEWS_FEED_URL = 'https://example.com/feed.json';
       // Build a string that exceeds 5 MiB
@@ -240,7 +288,7 @@ describe('NewsCronService', () => {
       await service.executeJob();
 
       expect(mocks.upsertNewsItems).not.toHaveBeenCalled();
-      expect(mocks.deleteNewsItemsByExternalIds).not.toHaveBeenCalled();
+      expect(mocks.deleteItemsNotInFeed).not.toHaveBeenCalled();
     });
 
     test('should abort when top-level shape is invalid', async () => {
@@ -251,7 +299,7 @@ describe('NewsCronService', () => {
       await service.executeJob();
 
       expect(mocks.upsertNewsItems).not.toHaveBeenCalled();
-      expect(mocks.deleteNewsItemsByExternalIds).not.toHaveBeenCalled();
+      expect(mocks.deleteItemsNotInFeed).not.toHaveBeenCalled();
     });
 
     test('should skip individual invalid items but keep valid ones', async () => {

apps/app/src/features/news/server/services/news-cron-service.ts

@@ -134,15 +134,14 @@ export class NewsCronService extends CronService {
         : undefined,
     }));
 
-    const feedIds = new Set(filteredItems.map((item) => item.id));
-
-    // Get all existing external IDs to find which ones are no longer in the feed
-    // We pass all filtered items' IDs — items not in the feed are determined by exclusion
-    const allFeedIds = feedJson.items.map((item) => item.id);
-    const idsToDelete = allFeedIds.filter((id) => !feedIds.has(id));
+    // Pass the full set of feed externalIds so the service can delete any DB
+    // item that is no longer present in the feed (Requirement 1.3). Includes
+    // items filtered out by version match — those remain "in the feed" and
+    // are allowed to age out via the NewsItem TTL.
+    const feedExternalIds = feedJson.items.map((item) => item.id);
 
     const service = new NewsService();
     await service.upsertNewsItems(newsItemInputs);
-    await service.deleteNewsItemsByExternalIds(idsToDelete);
+    await service.deleteItemsNotInFeed(feedExternalIds);
   }
 }

apps/app/src/features/news/server/services/news-service.spec.ts

@@ -421,20 +421,25 @@ describe('NewsService', () => {
     });
   });
 
-  describe('deleteNewsItemsByExternalIds', () => {
-    test('should call deleteMany with externalId filter', async () => {
+  describe('deleteItemsNotInFeed', () => {
+    test('should call deleteMany with $nin filter for items not in feed', async () => {
       mocks.newsItemDeleteMany.mockResolvedValue({ deletedCount: 1 });
 
-      await service.deleteNewsItemsByExternalIds(['ext-001', 'ext-002']);
+      await service.deleteItemsNotInFeed(['ext-001', 'ext-002']);
 
       expect(mocks.newsItemDeleteMany).toHaveBeenCalledWith({
-        externalId: { $in: ['ext-001', 'ext-002'] },
+        externalId: { $nin: ['ext-001', 'ext-002'] },
       });
     });
 
-    test('should do nothing if externalIds is empty', async () => {
-      await service.deleteNewsItemsByExternalIds([]);
-      expect(mocks.newsItemDeleteMany).not.toHaveBeenCalled();
+    test('should call deleteMany with $nin: [] when feed is empty (deletes all cached items)', async () => {
+      mocks.newsItemDeleteMany.mockResolvedValue({ deletedCount: 5 });
+
+      await service.deleteItemsNotInFeed([]);
+
+      expect(mocks.newsItemDeleteMany).toHaveBeenCalledWith({
+        externalId: { $nin: [] },
+      });
     });
   });
 });

apps/app/src/features/news/server/services/news-service.ts

@@ -171,11 +171,14 @@ export class NewsService {
   }
 
   /**
-   * Delete news items that are no longer in the feed
+   * Delete every cached news item whose externalId is NOT in the supplied set.
+   * Caller passes the full list of externalIds present in the latest feed; any DB
+   * item missing from that list is considered stale and removed (Requirement 1.3).
+   *
+   * Note: passing an empty array means "feed has no items" and will delete every
+   * cached news item. Callers must only invoke this after a successful feed fetch.
    */
-  async deleteNewsItemsByExternalIds(externalIds: string[]): Promise<void> {
-    if (externalIds.length === 0) return;
-
-    await NewsItem.deleteMany({ externalId: { $in: externalIds } });
+  async deleteItemsNotInFeed(feedExternalIds: string[]): Promise<void> {
+    await NewsItem.deleteMany({ externalId: { $nin: feedExternalIds } });
   }
 }