Copy a list into a new list, with all of its cards

Thanks to xet7 !

models/lib/listCopyPlan.js

@@ -0,0 +1,104 @@
+'use strict';
+
+// Pure decision for List.copy(boardId, swimlaneId, cardIdMap) — no DB access, so
+// it is unit-testable (tests/listCopySwimlane.test.cjs). models/lists.js applies
+// it. This is the copy-side twin of models/lib/listMovePlan.js, and it exists
+// because List.copy carried both of the faults #6670 fixed in List.move, plus
+// one of its own.
+//
+// 1. WHICH CARDS TRAVEL. copy() selected them with
+//
+//      { swimlaneId: this.swimlaneId || null, listId: oldId, archived: false }
+//
+//    A list that is not bound to a swimlane — an empty or missing swimlaneId,
+//    which is what every list on a board predating per-swimlane lists still has
+//    — turned that into `swimlaneId: null`, while its cards carry the real
+//    swimlaneIds of the swimlanes they are in. The selector therefore matched
+//    NOTHING and the copy produced an empty list. Even for a bound list the
+//    filter could only ever remove cards that are in the list being copied.
+//
+//    A list is the unit of a copy: every card in it travels, exactly as in
+//    List.move (`for (const card of await this.cards())`). So the selector is
+//    the list's own cards, unfiltered by swimlane.
+//
+// 2. WHICH LIST IS WRITTEN INTO. copy() searched the TARGET board for a list
+//    with this title to reuse, without first asking whether the target board IS
+//    this list's own board. On a same-board copy that search finds THIS LIST,
+//    so `_id` became the original: the "copy" wrote the cards back into the
+//    source list, doubling them, and returned the source list's id — which the
+//    REST endpoint then repositioned, moving the original the user had asked to
+//    copy. Same shape as the #6670 bug in List.move.
+//
+//    Reusing a same-titled list is only meaningful across boards. A copy on the
+//    same board is always a NEW list, the way Swimlane.copy already creates one
+//    ("Always create a new list for the copied swimlane, even if title already
+//    exists"), so:
+//
+//      same board                -> 'create' (a new list, bound to the swimlane)
+//      other board, name exists  -> 'merge'  (cards copied into that list)
+//      other board, name is new  -> 'create'
+//
+// An empty targetSwimlaneId is not an error: the copy is board-wide, which is
+// what shared lists were before per-swimlane lists and is still a legitimate
+// layout. It is also the default of the REST endpoint
+// (POST /api/boards/:boardId/lists/:listId/copy with no `toSwimlaneId`), which
+// is where fixing (1) raises a question that could not come up while the copy
+// was empty: WHERE do the cards land. Pinning every card to the target swimlane
+// would, for a board-wide same-board copy, dump the cards of three swimlanes
+// into none at all. So when no swimlane is asked for and the copy stays on the
+// same board, each card keeps the swimlane it is in and the duplicate looks
+// like the original. Across boards it cannot: the source card's swimlaneId
+// belongs to the OTHER board and would be orphaned there, so those cards take
+// the copy's own (possibly empty) swimlane.
+
+function planListCopy({
+  listId,
+  listBoardId,
+  targetBoardId,
+  targetSwimlaneId,
+  existingListId,
+} = {}) {
+  const swimlaneId = typeof targetSwimlaneId === 'string' ? targetSwimlaneId : '';
+
+  // Every card of the source list, whatever swimlane each one is in. Scoping
+  // this by the list's own swimlaneId is what emptied the copy of a board-wide
+  // list, and can never do anything but drop cards that belong to the list.
+  const cardSelector = { listId, archived: false };
+
+  const sameBoard = listBoardId === targetBoardId;
+  // See the note above: only a same-board copy can keep the cards where they
+  // are, and only when the caller named no swimlane to put them in.
+  const keepCardSwimlanes = sameBoard && swimlaneId === '';
+
+  if (!sameBoard && existingListId) {
+    // The destination board's own list keeps its own swimlane binding; only the
+    // cards are copied into it.
+    return {
+      action: 'merge',
+      listId: existingListId,
+      swimlaneId,
+      keepCardSwimlanes,
+      cardSelector,
+    };
+  }
+
+  return {
+    action: 'create',
+    listId: null,
+    swimlaneId,
+    keepCardSwimlanes,
+    cardSelector,
+  };
+}
+
+// The swimlaneId one copied card gets. Kept beside the plan so the rule lives
+// in one place and models/lists.js only applies it.
+function copiedCardSwimlaneId(plan, card) {
+  if (plan && plan.keepCardSwimlanes) {
+    const own = card && card.swimlaneId;
+    return typeof own === 'string' ? own : '';
+  }
+  return (plan && plan.swimlaneId) || '';
+}
+
+module.exports = { planListCopy, copiedCardSwimlaneId };

models/lists.js

@@ -10,6 +10,7 @@ import {
   listCardsSelector,
   otherSwimlaneIdsForFirstActive,
 } from '/models/lib/swimlaneFilter';
+import { planListCopy, copiedCardSwimlaneId } from './lib/listCopyPlan';
 import { planListMove } from './lib/listMovePlan';
 const { SimpleSchema } = require('/imports/simpleSchema');
 
@@ -287,33 +288,52 @@ Lists.attachSchema(
 
 Lists.helpers({
   async copy(boardId, swimlaneId, cardIdMap = null) {
+    // What this does is decided in models/lib/listCopyPlan.js, where it can be
+    // unit-tested - the copy-side twin of List.move's planner. Two faults lived
+    // in these lines, and both are described there:
+    //
+    // - the cards were selected by `swimlaneId: this.swimlaneId || null`, so a
+    //   board-wide list (an empty or missing swimlaneId, which is every list on
+    //   a board predating per-swimlane lists) asked for cards with NO swimlane
+    //   while its cards carry real ones - the selector matched nothing and the
+    //   copy came out EMPTY;
+    // - the target board was searched for a same-titled list to reuse without
+    //   first asking whether that board IS this list's own board. On a
+    //   same-board copy the search finds THIS list, so the "copy" wrote the
+    //   cards back into the source list and returned the source list's id.
     const oldId = this._id;
-    const oldSwimlaneId = this.swimlaneId || null;
-    this.boardId = boardId;
-    this.swimlaneId = swimlaneId;
+    const sameBoard = boardId === this.boardId;
+    const existing = sameBoard
+      ? null
+      : await ReactiveCache.getList({
+        boardId,
+        title: this.title,
+        archived: false,
+      });
 
-    let _id = null;
-    const existingListWithSameName = await ReactiveCache.getList({
-      boardId,
-      title: this.title,
-      archived: false,
+    const plan = planListCopy({
+      listId: oldId,
+      listBoardId: this.boardId,
+      targetBoardId: boardId,
+      targetSwimlaneId: swimlaneId,
+      existingListId: existing ? existing._id : null,
     });
-    if (existingListWithSameName) {
-      _id = existingListWithSameName._id;
-    } else {
+
+    let _id = plan.listId;
+    if (plan.action === 'create') {
+      this.boardId = boardId;
+      this.swimlaneId = plan.swimlaneId; // Set the target swimlane for the copied list
       delete this._id;
-      this.swimlaneId = swimlaneId; // Set the target swimlane for the copied list
       _id = await Lists.insertAsync(this);
     }
 
-    // Copy all cards in list
-    const cards = await ReactiveCache.getCards({
-      swimlaneId: oldSwimlaneId,
-      listId: oldId,
-      archived: false,
-    });
+    // Copy all cards in list. Every card of the source list travels, whatever
+    // swimlane each one is in - a list is the unit of a copy, exactly as in
+    // List.move - and each one lands in the swimlane the plan gives it: the
+    // chosen one, or its own when a same-board copy named no swimlane.
+    const cards = await ReactiveCache.getCards(plan.cardSelector);
     for (const card of cards) {
-      await card.copy(boardId, swimlaneId, _id, cardIdMap);
+      await card.copy(boardId, copiedCardSwimlaneId(plan, card), _id, cardIdMap);
     }
 
     return _id;

tests/listCopySwimlane.test.cjs

@@ -0,0 +1,267 @@
+'use strict';
+
+// Guard: copying a list actually copies its cards, and a copy is a new list.
+// Run: node tests/listCopySwimlane.test.cjs
+//
+// The copy-side twin of tests/listMoveSwimlane.test.cjs. List.copy carried both
+// of the faults #6670 fixed in List.move, and one of its own:
+//
+//   const oldSwimlaneId = this.swimlaneId || null;
+//   ...
+//   const existingListWithSameName = await ReactiveCache.getList({
+//     boardId, title: this.title, archived: false });
+//   if (existingListWithSameName) { _id = existingListWithSameName._id; }
+//   ...
+//   const cards = await ReactiveCache.getCards({
+//     swimlaneId: oldSwimlaneId, listId: oldId, archived: false });
+//
+// 1. THE COPY CAME OUT EMPTY. A list that is not bound to a swimlane - an empty
+//    or missing swimlaneId, which is what every list on a board predating
+//    per-swimlane lists still has - turns `this.swimlaneId || null` into
+//    `null`, so the selector asks for cards that have NO swimlane. The cards of
+//    such a list carry the real swimlaneIds of the swimlanes they are in, so it
+//    matched nothing and the copy produced a list with no cards at all.
+//
+// 2. THE "COPY" WROTE INTO THE ORIGINAL. The by-title search asks whether the
+//    target board has a list of this name, without first asking whether the
+//    target board IS this list's own board. On a same-board copy it finds THIS
+//    list, so `_id` became the original: the cards were copied back into the
+//    source list, doubling them, and the source list's id was returned - which
+//    POST /api/boards/:boardId/lists/:listId/copy then repositioned, moving the
+//    very list the user asked to copy. That is the #6670 shape exactly.
+//
+// The decision now lives in models/lib/listCopyPlan.js so it can be tested
+// here; models/lists.js applies it.
+
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
+
+const ROOT = path.join(__dirname, '..');
+const {
+  planListCopy,
+  copiedCardSwimlaneId,
+} = require(path.join(ROOT, 'models', 'lib', 'listCopyPlan'));
+
+const listsModel = fs.readFileSync(path.join(ROOT, 'models', 'lists.js'), 'utf8');
+
+// Source scans look at CODE, not prose: the comments in List.copy quote the
+// selector that used to be wrong, and a scan that counted those would fail on
+// the very explanation of the fix.
+const withoutComments = source => source
+  .replace(/\/\*[\s\S]*?\*\//g, '')
+  .replace(/^[ \t]*\/\/.*$/gm, '');
+
+const copySource = () => {
+  const stripped = withoutComments(listsModel);
+  const start = stripped.indexOf('  async copy(boardId, swimlaneId, cardIdMap = null) {');
+  const end = stripped.indexOf('  async move(boardId, swimlaneId) {');
+  assert.ok(start >= 0 && end > start, 'models/lists.js must still have List.copy');
+  return stripped.slice(start, end);
+};
+
+let passed = 0;
+const test = (name, run) => {
+  run();
+  passed++;
+  if (process.env.VERBOSE) console.log(`  ok - ${name}`);
+};
+
+const BOARD = 'board1';
+const OTHER = 'board2';
+const LIST = 'listA';
+
+// ---- which cards travel (defect 1) ------------------------------------------
+
+// THE regression: a board-wide list is the common case on any board that
+// predates per-swimlane lists, and its copy contained nothing.
+test('a board-wide list copies its cards, which are in real swimlanes', () => {
+  const plan = planListCopy({
+    listId: LIST,
+    listBoardId: BOARD,
+    targetBoardId: BOARD,
+    targetSwimlaneId: 'swimlane2',
+  });
+  assert.deepEqual(plan.cardSelector, { listId: LIST, archived: false });
+  const card = { _id: 'c1', swimlaneId: 'swimlane1' };
+  assert.equal(copiedCardSwimlaneId(plan, card), 'swimlane2',
+    'and they land in the swimlane the copy was asked for');
+});
+
+test('the card selector never scopes by swimlane at all (negative)', () => {
+  for (const listSwimlaneId of ['', null, undefined, 'swimlane1']) {
+    const plan = planListCopy({
+      listId: LIST,
+      listBoardId: BOARD,
+      listSwimlaneId,
+      targetBoardId: OTHER,
+      targetSwimlaneId: 'swimlane9',
+    });
+    assert.ok(!('swimlaneId' in plan.cardSelector),
+      'scoping the source list-s own cards by a swimlane can only drop cards ' +
+      'that belong to the list - and emptied the copy of a board-wide one');
+    assert.deepEqual(plan.cardSelector, { listId: LIST, archived: false });
+  }
+});
+
+test('archived cards are still left behind', () => {
+  const plan = planListCopy({
+    listId: LIST,
+    listBoardId: BOARD,
+    targetBoardId: OTHER,
+    targetSwimlaneId: 's',
+  });
+  assert.equal(plan.cardSelector.archived, false,
+    'a copy takes the list as it is shown, not its archive');
+});
+
+// ---- which list is written into (defect 2) ----------------------------------
+
+// THE other regression, the #6670 shape: the by-title search finds THIS list on
+// a same-board copy, so the "copy" wrote into the original.
+test('a same-board copy is never a merge, even with itself (negative)', () => {
+  const plan = planListCopy({
+    listId: LIST,
+    listBoardId: BOARD,
+    targetBoardId: BOARD,
+    targetSwimlaneId: 'swimlane2',
+    existingListId: LIST, // what the by-title search finds on the same board
+  });
+  assert.notEqual(plan.action, 'merge',
+    'merging a list into itself copied the cards back into the source list');
+  assert.equal(plan.action, 'create');
+  assert.equal(plan.listId, null, 'a copy is a NEW list');
+  assert.equal(plan.swimlaneId, 'swimlane2');
+});
+
+test('a same-board copy creates a new list even beside a same-titled one', () => {
+  const plan = planListCopy({
+    listId: LIST,
+    listBoardId: BOARD,
+    targetBoardId: BOARD,
+    targetSwimlaneId: '',
+    existingListId: 'anotherListOnThisBoardCalledTheSame',
+  });
+  assert.equal(plan.action, 'create',
+    'reusing a same-titled list is only meaningful across boards');
+  assert.equal(plan.listId, null);
+});
+
+test('merging into the destination board-s own list still happens', () => {
+  const plan = planListCopy({
+    listId: LIST,
+    listBoardId: BOARD,
+    targetBoardId: OTHER,
+    targetSwimlaneId: 'otherSwimlane',
+    existingListId: 'listOnOtherBoard',
+  });
+  assert.equal(plan.action, 'merge');
+  assert.equal(plan.listId, 'listOnOtherBoard', 'the cards join the list already there');
+});
+
+test('a list whose name is new on the other board is created there, bound', () => {
+  const plan = planListCopy({
+    listId: LIST,
+    listBoardId: BOARD,
+    targetBoardId: OTHER,
+    targetSwimlaneId: 'otherSwimlane',
+    existingListId: null,
+  });
+  assert.equal(plan.action, 'create');
+  assert.equal(plan.listId, null);
+  assert.equal(plan.swimlaneId, 'otherSwimlane');
+});
+
+// ---- where the copied cards land --------------------------------------------
+
+test('a missing or non-string swimlane normalises to board-wide', () => {
+  for (const value of [undefined, null, 0, {}]) {
+    const plan = planListCopy({
+      listId: LIST,
+      listBoardId: BOARD,
+      targetBoardId: OTHER,
+      targetSwimlaneId: value,
+    });
+    assert.equal(plan.swimlaneId, '', `${JSON.stringify(value)} must not reach the database`);
+    assert.equal(copiedCardSwimlaneId(plan, { swimlaneId: 'swimlane1' }), '',
+      'a card may not keep a swimlaneId that belongs to the source board');
+  }
+});
+
+// The REST endpoint's own default: POST .../copy with no toSwimlaneId, on the
+// board the list is already on.
+test('a same-board copy with no swimlane keeps each card where it is', () => {
+  const plan = planListCopy({
+    listId: LIST,
+    listBoardId: BOARD,
+    targetBoardId: BOARD,
+    targetSwimlaneId: undefined,
+  });
+  assert.equal(plan.keepCardSwimlanes, true);
+  assert.equal(copiedCardSwimlaneId(plan, { swimlaneId: 'swimlane1' }), 'swimlane1');
+  assert.equal(copiedCardSwimlaneId(plan, { swimlaneId: 'swimlane2' }), 'swimlane2',
+    'a duplicate of a board-wide list looks like the original');
+  assert.equal(copiedCardSwimlaneId(plan, {}), '',
+    'a card that had no swimlane still has none');
+});
+
+// Negative: keeping the source card's swimlaneId across boards would point at a
+// swimlane on the OTHER board - an orphaned card the board-open repair has to
+// rescue, which is what the #6670 argument-order bug produced in List.move.
+test('a cross-board copy never keeps the source swimlaneId (negative)', () => {
+  for (const targetSwimlaneId of ['', 'otherSwimlane', undefined]) {
+    const plan = planListCopy({
+      listId: LIST,
+      listBoardId: BOARD,
+      targetBoardId: OTHER,
+      targetSwimlaneId,
+    });
+    assert.equal(plan.keepCardSwimlanes, false);
+    assert.equal(copiedCardSwimlaneId(plan, { swimlaneId: 'swimlaneOnSourceBoard' }),
+      typeof targetSwimlaneId === 'string' ? targetSwimlaneId : '');
+  }
+});
+
+// ---- how models/lists.js applies it -----------------------------------------
+
+test('List.copy uses the plan and only searches other boards by title', () => {
+  const copy = copySource();
+  assert.match(listsModel,
+    /import \{ planListCopy, copiedCardSwimlaneId \} from '\.\/lib\/listCopyPlan';/,
+    'the planner must be the one place the decision lives');
+  assert.match(copy, /const sameBoard = boardId === this\.boardId;/,
+    'the by-title search must be skipped on the same board, where it finds itself');
+  assert.match(copy, /sameBoard\s*\n?\s*\?\s*null/,
+    'and it must resolve to nothing there');
+  assert.match(copy, /planListCopy\(\{/);
+  assert.match(copy, /ReactiveCache\.getCards\(plan\.cardSelector\)/,
+    'the cards must be selected by the plan-s selector');
+  assert.match(copy, /copiedCardSwimlaneId\(plan, card\)/,
+    'and each card must land in the swimlane the plan gives it');
+});
+
+// Negative: the exact selector that emptied the copy must be gone, and no
+// swimlaneId may be hand-built into the card query again.
+test('List.copy no longer scopes its cards by the list-s swimlane (negative)', () => {
+  const copy = copySource();
+  assert.doesNotMatch(copy, /this\.swimlaneId \|\| null/,
+    'a board-wide list has no swimlaneId, and its cards have real ones');
+  assert.doesNotMatch(copy, /oldSwimlaneId/,
+    'the source swimlane must not come back as a card filter');
+  assert.doesNotMatch(copy, /getCards\(\{/,
+    'the card selector belongs to models/lib/listCopyPlan.js, not to a literal here');
+});
+
+// Negative: nothing may reach the copy branch that writes into a list found by
+// title on this list's OWN board.
+test('no same-titled list is looked up before the board is compared (negative)', () => {
+  const copy = copySource();
+  assert.doesNotMatch(copy, /existingListWithSameName/,
+    'the unconditional by-title search is what found THIS list');
+  const searchIndex = copy.indexOf('ReactiveCache.getList({');
+  assert.ok(searchIndex > 0, 'a cross-board copy still looks for a same-titled list');
+  assert.ok(copy.indexOf('const sameBoard') < searchIndex,
+    'the board comparison must come first, or the search finds this list again');
+});
+
+console.log(`listCopySwimlane: ${passed} tests passed`);

tests/listMoveSwimlane.test.cjs

@@ -51,6 +51,18 @@ const withoutComments = source => source
   .replace(/\/\*[\s\S]*?\*\//g, '')
   .replace(/^[ \t]*\/\/.*$/gm, '');
 
+// Cut List.move out of a source string. It takes the offsets from the SAME
+// string it slices: taking them from the un-stripped file and slicing the
+// stripped one used to work only by accident, and any edit above move() - such
+// as the sibling fix in List.copy - slid the window off the method and failed
+// the two scans below on code they never looked at.
+const moveSource = (source = listsModel) => {
+  const start = source.indexOf('  async move(boardId, swimlaneId) {');
+  const end = source.indexOf('orphanedCardsSwimlaneIds(swimlaneId) {');
+  assert.ok(start >= 0 && end > start, 'models/lists.js must still have List.move');
+  return source.slice(start, end);
+};
+
 let passed = 0;
 const test = (name, run) => {
   run();
@@ -169,10 +181,7 @@ test('a list whose name is new on the other board is created there, bound', () =
 // ---- how models/lists.js applies it -----------------------------------------
 
 test('List.move uses the plan and only searches other boards by title', () => {
-  const move = listsModel.slice(
-    listsModel.indexOf('  async move(boardId, swimlaneId) {'),
-    listsModel.indexOf('orphanedCardsSwimlaneIds(swimlaneId) {'));
-  assert.ok(move.length > 0, 'models/lists.js must still have List.move');
+  const move = moveSource();
   assert.match(listsModel, /import \{ planListMove \} from '\.\/lib\/listMovePlan';/,
     'the planner must be the one place the decision lives');
   assert.match(move, /const sameBoard = boardId === this\.boardId;/,
@@ -201,9 +210,7 @@ test('no card is moved into a swimlane that is really a list id (negative)', ()
 // Negative: the cards must not be filtered by the TARGET board's swimlaneId -
 // on a cross-board move that matches nothing and leaves every card behind.
 test('the cards that travel are the list-s own, unfiltered (negative)', () => {
-  const move = withoutComments(listsModel).slice(
-    listsModel.indexOf('  async move(boardId, swimlaneId) {'),
-    listsModel.indexOf('orphanedCardsSwimlaneIds(swimlaneId) {'));
+  const move = moveSource(withoutComments(listsModel));
   assert.match(move, /for \(const card of await this\.cards\(\)\)/,
     'every card in the list travels with it');
   assert.doesNotMatch(move, /this\.cards\(swimlaneId\)/,