Rules: 'remove all members' action now removes the card's assignees (#2674).

The rule member actions operate on a card's ASSIGNEES: addMember calls card.assignMember() and removeMember calls card.unassignMember() (both write card.assignees). The 'remove all' branch (username === '*') however iterated card.members -- a different collection -- so it unassigned nobody a rule had assigned and 'remove every member' silently did nothing. Iterate the card's assignees (card.getAssignees()) instead, snapshotting the ids first since unassignMember pulls from that same array. The single-user add/remove were already symmetric (the earlier #2674 fix that made them assign/unassign); this completes it for the wildcard case.

Tests: tests/ruleRemoveMemberAction.test.cjs (6 checks) pins the add/remove symmetry, that the '*' branch reads assignees (not card.members) and still unassigns each, and the defensive skip for an unresolved username.

Thanks to sfahrenholz and xet7 !

Fixes #2674,

server/rulesHelper.js

@@ -428,9 +428,15 @@ export const RulesHelper = {
     }
     if (action.actionType === 'removeMember') {
       if (action.username === '*') {
-        const members = card.members || [];
-        for (let i = 0; i < members.length; i++) {
-          await card.unassignMember(members[i]);
+        // #2674: "remove every member" must iterate the card's ASSIGNEES — the
+        // same collection assignMember/unassignMember act on (addMember above
+        // uses card.assignMember). It previously read card.members, so it never
+        // removed anyone the rule had added and "remove all" appeared to do
+        // nothing. Snapshot the ids first: unassignMember $pulls from assignees.
+        const assignees =
+          (typeof card.getAssignees === 'function' ? card.getAssignees() : card.assignees) || [];
+        for (let i = 0; i < assignees.length; i++) {
+          await card.unassignMember(assignees[i]);
         }
       } else {
         const member = await ReactiveCache.getUser({ username: action.username });

tests/ruleRemoveMemberAction.test.cjs

@@ -0,0 +1,80 @@
+'use strict';
+
+// Regression guard for #2674 ("Rule: remove USER from card when move" — a member
+// added by a move-to rule was never removed by a move-from rule, and the
+// "remove every member" action removed nobody).
+// Run: node tests/ruleRemoveMemberAction.test.cjs
+//
+// The rule member actions operate on the card's ASSIGNEES: `addMember` calls
+// card.assignMember() and `removeMember` calls card.unassignMember() (both write
+// card.assignees). The "remove all" branch (username === '*') must therefore
+// iterate the card's assignees too. It previously iterated card.MEMBERS, a
+// different collection, so it never removed anyone a rule had assigned — the
+// action silently did nothing (@mweiss237's "remove every member" report).
+//
+// This is a source guard because performAction() is Meteor-coupled (ReactiveCache,
+// Card model methods) and cannot run under plain Node; the invariant we protect is
+// that add and remove act on the SAME collection.
+
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+
+const src = fs.readFileSync(
+  path.join(__dirname, '..', 'server', 'rulesHelper.js'),
+  'utf8',
+);
+
+let passed = 0;
+function check(name, fn) { fn(); passed += 1; console.log('  ok -', name); }
+
+// Isolate the removeMember action block so assertions are specific to it.
+const removeBlock = (() => {
+  const start = src.indexOf("action.actionType === 'removeMember'");
+  assert.ok(start > -1, 'removeMember action must exist');
+  return src.slice(start, start + 1100);
+})();
+const addBlock = (() => {
+  const start = src.indexOf("action.actionType === 'addMember'");
+  assert.ok(start > -1, 'addMember action must exist');
+  return src.slice(start, start + 400);
+})();
+
+check('addMember assigns (writes card.assignees via assignMember)', () => {
+  assert.ok(/card\.assignMember\(/.test(addBlock),
+    'addMember must call card.assignMember()');
+});
+
+check('removeMember (single user) unassigns the same collection', () => {
+  assert.ok(/card\.unassignMember\(member\._id\)/.test(removeBlock),
+    'removeMember must call card.unassignMember(member._id)');
+});
+
+check('#2674: "remove all" (username === "*") iterates ASSIGNEES, not members', () => {
+  // The bug: iterating card.members while unassigning from card.assignees.
+  assert.ok(/getAssignees\(\)|card\.assignees/.test(removeBlock),
+    'the "*" branch must read the card assignees');
+  assert.ok(!/const members = card\.members/.test(removeBlock),
+    'the "*" branch must NOT iterate card.members (wrong collection)');
+});
+
+check('#2674: "remove all" still unassigns each entry (removal actually happens)', () => {
+  const star = removeBlock.slice(removeBlock.indexOf("=== '*'"));
+  assert.ok(/for \([^)]*\)[\s\S]*card\.unassignMember\(/.test(star),
+    'the "*" branch must loop and call card.unassignMember() for each assignee');
+});
+
+check('symmetry: add and remove act on the SAME concept (assign/unassign)', () => {
+  // add -> assignMember, remove -> unassignMember: a user added by a move-to
+  // rule is removed by a move-from rule (the core #2674 report).
+  assert.ok(/assignMember\(/.test(addBlock) && /unassignMember\(/.test(removeBlock),
+    'add uses assignMember, remove uses unassignMember');
+});
+
+check('negative: an unresolved username is skipped, not crashed on (#2674 defensive)', () => {
+  assert.ok(/user "\$\{action\.username\}" not found; skipping/.test(removeBlock) ||
+            /not found; skipping/.test(removeBlock),
+    'a missing user must warn+skip rather than throw on undefined._id');
+});
+
+console.log(`\nruleRemoveMemberAction: ${passed} checks passed`);