groups: the editor's save leaves membership alone
apply_group_edit read a members list the editor's form never sends,
fell back to the group's own, and Store#edit_group diffed that against
itself. Membership is the members screen's (edit_members), so
edit_group is now the rename and the lines.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Kqo9sKAi4DnKCQkrtYoAwM
change
commit fe9e591daf3973cd3cd9dffb6d76d471a7a089e7
author Claude <noreply@anthropic.com>
date
parent 36657285
diff --git a/lib/pro_tacts/store.rb b/lib/pro_tacts/store.rb
index 09aab1b..a66feef 100644
--- a/lib/pro_tacts/store.rb
+++ b/lib/pro_tacts/store.rb
@@ -726,25 +726,22 @@ module ProTacts
     end
 
     # The editor's whole save, as one transaction: a failure part-way
-    # leaves the group as it was rather than half renamed. Leavers go
-    # first and joiners last, so no card is logged for lines it was
-    # about to stop or had yet to start serving.
-    #: (String id, name: String?, lines: Array[String], members: Array[String]) -> void
-    def edit_group(id, name:, lines:, members:)
+    # leaves the group as it was rather than half renamed. Membership
+    # is #edit_members', the members screen's own save.
+    #: (String id, name: String?, lines: Array[String]) -> void
+    def edit_group(id, name:, lines:)
       refuse_sync_rename(id, name)
       @database.transaction do
-        current = member_ids([id])
-        (current - members).each { remove_member(id, it) }
         rename(id, name)
         set_group_lines(id, lines)
-        (members - current).each { add_member(id, it) }
       end
     end
 
     # The members screen's save, #regroup's shape over a group: what
     # it toggled rather than the set it shows, one transaction so a
-    # failure part-way leaves the membership as it was. Leavers first
-    # and joiners last, #edit_group's reason.
+    # failure part-way leaves the membership as it was. Leavers go
+    # first and joiners last, so no card is logged for lines it was
+    # about to stop or had yet to start serving.
     #: (String id, join: Array[String], leave: Array[String]) -> void
     def edit_members(id, join:, leave:)
       @database.transaction do
@@ -855,7 +852,7 @@ module ProTacts
       end
     end
 
-    # A card's side of #edit_group: one transaction, leavers first and
+    # A card's side of #edit_members: one transaction, leavers first and
     # joiners last for that method's reason. A named create makes its
     # group to join inside the same transaction, so a taken name
     # (#create_group) moves nothing.
diff --git a/lib/pro_tacts/web/groups.rb b/lib/pro_tacts/web/groups.rb
index 6c5ac49..3ca0af8 100644
--- a/lib/pro_tacts/web/groups.rb
+++ b/lib/pro_tacts/web/groups.rb
@@ -113,26 +113,14 @@ module ProTacts
         return group_edit_screen(group, notice: "This group changed since the page loaded; nothing was saved.")
       end
 
-      # Only ids that name a card: a membership row is a foreign key,
-      # and a doctored id is ordinary bad input rather than a 500. A
-      # POST carrying no list keeps the membership it found, as an
-      # absent field keeps a card's lines (Admin::CardForm.edited_values);
-      # the editor's own form never carries one (Admin::GroupsEdit).
-      submitted = r.params["members"]
-      members =
-        if submitted.is_a?(Array)
-          submitted.map(&:to_s) & store.contact_ids
-        else
-          group.members
-        end #: Array[String]
-
-      # No name at all keeps the one it has, the members' posture
-      # above: a book's group shows its name as text (Admin::GroupsEdit),
-      # where a blank one is a request to go nameless.
+      # No name at all keeps the one it has: a book's group shows its
+      # name as text (Admin::GroupsEdit), where a blank one is a
+      # request to go nameless. Membership is the members screen's
+      # (#apply_members); the editor's form carries none.
       name = r.params.key?("name") ? r.params["name"].to_s : group.name
 
       begin
-        store.edit_group(id, name:, lines: Admin::CardForm.group_lines(group, r.params), members:)
+        store.edit_group(id, name:, lines: Admin::CardForm.group_lines(group, card_form_params))
       rescue Sequel::UniqueConstraintViolation
         # A taken name (db/migrations/008_group_names.rb). The save is
         # one transaction, so nothing of it landed.
@@ -149,8 +137,8 @@ module ProTacts
     # the diff of what came back against what the page loaded, so a
     # membership moved elsewhere since stays as it is, and a stale
     # page has nothing to revert and no snapshot to refuse. Only ids
-    # that name a card — a membership row is a foreign key
-    # (#apply_group_edit's reason).
+    # that name a card: a membership row is a foreign key, and a
+    # doctored id is ordinary bad input rather than a 500.
     #: (untyped r, String id) -> String?
     def apply_members(r, id)
       group = store.group(id)
diff --git a/test/pro_tacts/admin/test_groups_pages.rb b/test/pro_tacts/admin/test_groups_pages.rb
index bc847fe..d2eb2ff 100644
--- a/test/pro_tacts/admin/test_groups_pages.rb
+++ b/test/pro_tacts/admin/test_groups_pages.rb
@@ -235,23 +235,22 @@ class AdminGroupsPagesTest < Minitest::Test
     end
   end
 
-  # The whole save: the name, a line, and the membership in one POST,
-  # and every card whose served bytes moved told so in the change log.
-  def test_a_save_writes_the_name_the_lines_and_the_members
+  # The whole save: the name and a line in one POST, and every card
+  # whose served bytes moved told so in the change log. Membership is
+  # the members screen's, so a list sent here moves nobody.
+  def test_a_save_writes_the_name_and_the_lines_but_not_the_members
     with_contacts(BOOLES) do |store|
       id = household(store)
       group = store.group(id)
 
       post "/groups/#{id}", version: group.version, name: "The Booles", note: "Gate code 1912.",
-                            members: ["", "george", "ada"]
+                            members: ["", "ada"]
 
       assert_equal 303, last_response.status
       saved = store.group(id)
       assert_equal "The Booles", saved.name
       assert_equal [ADDRESS, "NOTE:Gate code 1912."], saved.lines
-      assert_equal %w[ada george], saved.members
-      assert_includes store.contact("ada").vcard.to_s, ADDRESS
-      refute_includes store.contact("mary").vcard.to_s, ADDRESS
+      assert_equal %w[george mary], saved.members
       assert_equal "group", store.changes_of("mary").first.action
     end
   end
@@ -262,7 +261,7 @@ class AdminGroupsPagesTest < Minitest::Test
       group = store.group(id)
       digest = group.reading.addresses.fetch(0).line.digest
 
-      post "/groups/#{id}", version: group.version, note: "Gate code 1854.", members: ["", "george", "mary"],
+      post "/groups/#{id}", version: group.version, note: "Gate code 1854.",
                             address: {digest => {street: "8 Calculus Close", locality: "London",
                                                  region: "England", postal_code: "NW1 1AB",
                                                  country: "United Kingdom"}},
@@ -281,7 +280,7 @@ class AdminGroupsPagesTest < Minitest::Test
       stale = store.group(id).version
       store.rename_group(id, name: "The Booles")
 
-      post "/groups/#{id}", version: stale, name: "Booles", members: [""]
+      post "/groups/#{id}", version: stale, name: "Booles"
 
       assert_equal 200, last_response.status
       assert_includes last_response.body, "This group changed since the page loaded; nothing was saved."
@@ -295,8 +294,7 @@ class AdminGroupsPagesTest < Minitest::Test
       store.create_group(name: "Clarks")
       id = household(store)
 
-      post "/groups/#{id}", version: store.group(id).version, name: "Clarks", note: "Gate code 1854.",
-                            members: ["", "ada"]
+      post "/groups/#{id}", version: store.group(id).version, name: "Clarks", note: "Gate code 1854."
 
       assert_equal 200, last_response.status
       assert_includes last_response.body, "Another group is already named Clarks; nothing was saved."
@@ -311,7 +309,7 @@ class AdminGroupsPagesTest < Minitest::Test
     with_contacts(BOOLES) do |store|
       id = FixtureData.seed_group(store, name: ProTacts::Group::EVERYONE, members: %w[george])
 
-      post "/groups/#{id}", version: store.group(id).version, name: "Everyone", members: ["", "george", "mary"]
+      post "/groups/#{id}", version: store.group(id).version, name: "Everyone"
 
       assert_equal 200, last_response.status
       assert_includes last_response.body, "The sync:* group names a book and keeps its name; nothing was saved."
@@ -337,7 +335,7 @@ class AdminGroupsPagesTest < Minitest::Test
     with_contacts(BOOLES) do |store|
       id = FixtureData.seed_group(store, name: ProTacts::Group::EVERYONE, members: %w[george])
 
-      post "/groups/#{id}", version: store.group(id).version, note: "Gate code 1854.", members: ["", "george"]
+      post "/groups/#{id}", version: store.group(id).version, note: "Gate code 1854."
 
       assert_equal 303, last_response.status
       assert_equal ProTacts::Group::EVERYONE, store.group(id).name
@@ -345,22 +343,6 @@ class AdminGroupsPagesTest < Minitest::Test
     end
   end
 
-  # No list at all is a POST that says nothing about membership; a
-  # doctored id names no card and is dropped rather than a 500.
-  def test_membership_moves_only_for_a_list_of_real_cards
-    with_contacts(BOOLES) do |store|
-      id = household(store)
-
-      post "/groups/#{id}", version: store.group(id).version, note: "Gate code 1854."
-      assert_equal %w[george mary], store.group(id).members
-
-      post "/groups/#{id}", version: store.group(id).version, note: "Gate code 1854.",
-                            members: ["", "george", "nobody"]
-      assert_equal 303, last_response.status
-      assert_equal %w[george], store.group(id).members
-    end
-  end
-
   ## The photo
 
   JPEG = "\xFF\xD8\xFF\xE0\x00\x10JFIF\x00\xFF".b #: String
diff --git a/test/pro_tacts/test_store.rb b/test/pro_tacts/test_store.rb
index c39d604..3dc04a7 100644
--- a/test/pro_tacts/test_store.rb
+++ b/test/pro_tacts/test_store.rb
@@ -1712,7 +1712,8 @@ class StoreTest < Minitest::Test
       store.set_group_lines(id, [HOUSEHOLD_ADDRESS])
       store.add_member(id, "aiden")
       store.remove_member(id, "znorth")
-      store.edit_group(id, name: "Booles", lines: [HOUSEHOLD_ADDRESS], members: ["aiden"])
+      store.edit_group(id, name: "Booles", lines: [HOUSEHOLD_ADDRESS])
+      store.edit_members(id, join: [], leave: ["znorth"])
 
       assert_equal logged, store.group_changes_of(id).length
     end
@@ -1741,9 +1742,10 @@ class StoreTest < Minitest::Test
     with_store({"aiden" => AIDEN, "znorth" => ZED}) do |store|
       id = FixtureData.seed_group(store, name: "Booles", members: ["aiden"], lines: [HOUSEHOLD_ADDRESS])
 
-      store.edit_group(id, name: "The Booles", lines: [HOUSEHOLD_NOTE], members: ["znorth"])
+      store.edit_group(id, name: "The Booles", lines: [HOUSEHOLD_NOTE])
+      store.edit_members(id, join: ["znorth"], leave: ["aiden"])
 
-      assert_equal %w[join lines rename leave create], store.group_changes_of(id).map(&:action)
+      assert_equal %w[join leave lines rename create], store.group_changes_of(id).map(&:action)
     end
   end
 
@@ -1919,7 +1921,7 @@ class StoreTest < Minitest::Test
       assert_raises(ProTacts::Store::SyncGroupRename) { store.rename_group(everyone, name: " ") }
       assert_raises(ProTacts::Store::SyncGroupRename) { store.rename_group(alpha, name: "sync:Zoë Chen") }
       assert_raises(ProTacts::Store::SyncGroupRename) do
-        store.edit_group(everyone, name: "Everyone", lines: [], members: %w[aiden])
+        store.edit_group(everyone, name: "Everyone", lines: [])
       end
 
       assert_equal ProTacts::Group::EVERYONE, store.group(everyone).name
@@ -1933,9 +1935,10 @@ class StoreTest < Minitest::Test
     with_store({"aiden" => AIDEN}) do |store|
       everyone = FixtureData.seed_group(store, name: ProTacts::Group::EVERYONE, members: %w[aiden])
 
-      store.edit_group(everyone, name: ProTacts::Group::EVERYONE, lines: [], members: [])
+      store.edit_group(everyone, name: ProTacts::Group::EVERYONE, lines: [HOUSEHOLD_NOTE])
 
-      assert_empty store.group(everyone).members
+      assert_equal ProTacts::Group::EVERYONE, store.group(everyone).name
+      assert_equal [HOUSEHOLD_NOTE], store.group(everyone).lines
     end
   end