import: choose the groups the arrivals join
Before: the review screen offered one group, made on the way in and
named for the moment. A book's existing groups were reachable only by
typing a name you already knew, and a typo made a second group rather
than joining the one meant.

After: that box is still there, and under it every group this book
already has, each a box to tick. Both are joins and neither is
required -- an import is as often "these are the people from the
school list" as it is a batch that only needs finding again, and a
card in no group is still in everyone's book.

For the whole file rather than per contact: the walk's pair of cards
is for fixing what one contact carries, and a group is not something
the file says anything about. Once the arrivals have landed, the group
they landed in is how to find them and a group's own editor is where
its members are moved.

How: Import::Land takes `join`, the ids of groups that already exist,
beside the `group` it resolves or creates by name; the confirm filters
the submitted ids against the store's own, apply_groups' rule, so a
doctored id is dropped rather than reaching Store#add_member's `sole`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KrREeoPLXx1qsYYq7qkL8h
change
commit fb493dc316e374c871490e6897bd33c58923915e
author Claude <noreply@anthropic.com>
date
parent a27f9f27
diff --git a/AGENTS.md b/AGENTS.md
index 702303e..fa4d3aa 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -40,7 +40,8 @@ profiles, so leave those to the user.
 Importing is not a task at all: a .vcf is uploaded at `/import` in the
 app, and reviewed there contact by contact — the card as the file wrote
 it beside the card that is landing, with the lines no screen here shows
-struck — before the lot is landed through the store
+struck — before the lot is landed through the store, into a group made
+for the import and whichever of this book's groups the review ticked
 (docs/plans/2026-09-21-import-a-vcf.md). The `import:*` tasks, the Swift
 reader they drove Contacts.app with, and the plan directories they wrote
 are gone.
diff --git a/docs/plans/2026-09-21-import-a-vcf.md b/docs/plans/2026-09-21-import-a-vcf.md
index bea5e01..093d1ce 100644
--- a/docs/plans/2026-09-21-import-a-vcf.md
+++ b/docs/plans/2026-09-21-import-a-vcf.md
@@ -110,7 +110,8 @@ So the review is a walk, four screens rather than a submission:
    right, the contact editor, pre-filled with what was read and
    pointed at the import. Read the spouse's name off the left card and
    type it into the note on the right, in your own words.
-4. **Confirm**, and the cards land as the walk left them.
+4. **Confirm**, under whichever groups the arrivals should join, and
+   the cards land as the walk left them.
 
 The right-hand card is `Admin::ContactsEdit` itself, not a copy of it.
 A second editor for imports would be a place where the two could
@@ -165,10 +166,21 @@ dialog make:
 2. `Store#put` with `client: true`, so a card this creates joins
    everyone's book the way a client's create does
    (`2026-09-15-client-creates-join-everyone.md`).
-3. `Store#regroup` into the import's group, made if it is missing. The
-   group is named `import-<timestamp>` by default and can be renamed or
-   emptied on the review screen: what landed together can be found
-   together, and undone together.
+3. `Store#regroup` into whichever groups the review screen settled on.
+   There are two ways to name one there and they are both joins. The
+   text box makes a group, named `import-<timestamp>` by default — what
+   landed together can be found together, and undone together — and is
+   emptiable, a name this server already has joining that group rather
+   than colliding with it. Beside it every group this book already has,
+   as a box to tick, because an import is as often "these are the people
+   from the school list" as it is a batch that only needs finding again.
+   Neither is required: a card in no group is still in everyone's book.
+
+   Ticked for the whole file rather than per contact. The walk's pair of
+   cards is for fixing what one contact carries, and a group is not
+   something the file says anything about; once the arrivals have landed
+   the group they landed in is how to find them, and a group's own
+   editor is where its members are moved (`Admin::GroupsEdit`).
 
 Importing is not idempotent and cannot be. A `.vcf` carries no id this
 server minted, so a second upload of the same file is a second set of
diff --git a/lib/pro_tacts/admin/import_review.rb b/lib/pro_tacts/admin/import_review.rb
index e6c34be..0dbc825 100644
--- a/lib/pro_tacts/admin/import_review.rb
+++ b/lib/pro_tacts/admin/import_review.rb
@@ -1,6 +1,7 @@
 require "pro_tacts/admin/phlex"
 
 require "pro_tacts/admin/format"
+require "pro_tacts/admin/group_label"
 require "pro_tacts/admin/layout"
 require "pro_tacts/admin/list_item"
 
@@ -28,17 +29,22 @@ module ProTacts
       # @rbs @rows: Array[[::ProTacts::Contact, Integer]]
       # @rbs @unknown: Array[::ProTacts::Import::Vcf::Unknown]
       # @rbs @group: String
+      # @rbs @groups: Array[::ProTacts::Store::Group]
       # @rbs @notice: String?
 
       # `rows` is the contact each card will land as, paired with how
       # many of its original's lines are not coming with it. `upload`
       # is the staged import every link and the confirm carry.
-      #: (upload: String, rows: Array[[::ProTacts::Contact, Integer]], unknown: Array[::ProTacts::Import::Vcf::Unknown], group: String, ?notice: String?) -> void
-      def initialize(upload:, rows:, unknown:, group:, notice: nil)
+      # `group` is the name the import offers to make; `groups` is
+      # every group this book already has, alphabetical because this
+      # is a list to find a name in (the groups dialog's own order).
+      #: (upload: String, rows: Array[[::ProTacts::Contact, Integer]], unknown: Array[::ProTacts::Import::Vcf::Unknown], group: String, groups: Array[::ProTacts::Store::Group], ?notice: String?) -> void
+      def initialize(upload:, rows:, unknown:, group:, groups:, notice: nil)
         @upload = upload
         @rows = rows
         @unknown = unknown
         @group = group
+        @groups = groups.sort_by { it.label.downcase }
         @notice = notice
       end
 
@@ -63,20 +69,48 @@ module ProTacts
             h1(class: "type-h2", style: "margin: 0;") { count(@rows.length, "contact") }
             losses
             form(action: "/import/#{@upload}/land", method: "post", class: "field-stack") do
-              # The group everything lands in, named for the moment by
+              # A group made for this import, named for the moment by
               # default so one import can be found — or undone — apart
-              # from the next. Editable, and emptiable: a blank name
-              # puts the arrivals in nobody's group but everyone's book.
+              # from the next. Editable, and emptiable; a name this
+              # book already uses joins that group rather than making
+              # a second one by the same name (Import::Land#group_id).
               label(class: "field") do
-                span(class: "type-label") { "group" }
+                span(class: "type-label") { "new group" }
                 input(type: "text", name: "group", value: @group, placeholder: "no group")
               end
+              existing_groups
               button(type: "submit", data: {variant: "primary"}) { "import #{count(@rows.length, "contact")}" }
             end
           end
         end
       end
 
+      # The groups this book already has, every one of them a box the
+      # arrivals can join: an import is as often "these are the people
+      # from the school list" as it is a batch that only needs finding
+      # again, and a name typed into the box above is a name you have
+      # to know. Ticked alongside that name rather than instead of it
+      # — both are joins, and neither is required.
+      #
+      # Unchecked to begin with, every time. The import's own group is
+      # the default this screen argues for; putting four hundred
+      # arrivals somewhere else is a thing to say, not a box to leave
+      # as it was found.
+      #: () -> void
+      def existing_groups
+        return if @groups.empty?
+
+        div(class: "field-stack") do
+          span(class: "type-label") { "add to" }
+          @groups.each do |group|
+            label do
+              input(type: "checkbox", name: "groups[]", value: group.id)
+              render GroupLabel.new(group:)
+            end
+          end
+        end
+      end
+
       # What the file is losing, by property rather than by line: a
       # reader deciding whether to open any of the rows below wants to
       # know it is `X-SOCIALPROFILE` and `PRODID` going, not that
diff --git a/lib/pro_tacts/import/land.rb b/lib/pro_tacts/import/land.rb
index cf70749..090aad8 100644
--- a/lib/pro_tacts/import/land.rb
+++ b/lib/pro_tacts/import/land.rb
@@ -21,6 +21,7 @@ module ProTacts
     class Land
       # @rbs @store: Store
       # @rbs @group: String?
+      # @rbs @join: Array[String]
       # @rbs @group_ids: Hash[String, String]
 
       # The group an import offers to put its arrivals in, named for
@@ -32,16 +33,23 @@ module ProTacts
         "import-#{now.utc.strftime("%Y%m%dT%H%M%SZ")}"
       end
 
-      # `group` is the group every arrival joins, or none.
-      #: (Store store, Array[VCard] cards, ?group: String?) -> Array[Contact]
-      def self.call(store, cards, group: nil)
-        new(store, group:).call(cards)
+      # `group` is a group named rather than chosen — this server's
+      # own by that name, or a new one — and `join` the ids of groups
+      # it already has. Both, because the review screen offers both: a
+      # group named for the moment, so that what landed together can
+      # be found together, and the groups these people actually belong
+      # in. Either, or neither: nothing here is required, and a card
+      # in no group is still in everyone's book (#land).
+      #: (Store store, Array[VCard] cards, ?group: String?, ?join: Array[String]) -> Array[Contact]
+      def self.call(store, cards, group: nil, join: [])
+        new(store, group:, join:).call(cards)
       end
 
-      #: (Store store, ?group: String?) -> void
-      def initialize(store, group: nil)
+      #: (Store store, ?group: String?, ?join: Array[String]) -> void
+      def initialize(store, group: nil, join: [])
         @store = store
         @group = group
+        @join = join
         @group_ids = {} #: Hash[String, String]
       end
 
@@ -64,7 +72,11 @@ module ProTacts
         @store.put(id, identified(card, id), client: true)
 
         group = @group
-        @store.regroup(id, join: [group_id(group)], leave: []) if group
+        # The named group is resolved per card rather than up front,
+        # for #group_id's reason; the chosen ids need no resolving,
+        # the review screen having read them off this same store.
+        join = group ? [*@join, group_id(group)] : @join
+        @store.regroup(id, join:, leave: []) unless join.empty?
 
         # Read back rather than kept from the put, because the group
         # above moved what the card serves. Nothing can have taken it
diff --git a/lib/pro_tacts/web/import.rb b/lib/pro_tacts/web/import.rb
index edab387..decf37a 100644
--- a/lib/pro_tacts/web/import.rb
+++ b/lib/pro_tacts/web/import.rb
@@ -126,6 +126,7 @@ module ProTacts
         rows:,
         unknown: Import::Vcf.unknown(originals),
         group: Import::Land.default_group,
+        groups: store.all_groups,
         notice:,
       )
     end
@@ -235,7 +236,11 @@ module ProTacts
 
       _originals, landing = staged
       group = r.params["group"].to_s.strip
-      landed = Import::Land.call(store, landing, group: group.empty? ? nil : group)
+      # Only ids naming a group, #apply_groups' own rule: Store#add_member
+      # reads the group with `sole`, so a doctored one would be a 500
+      # rather than the bad input it is.
+      join = ids_in(r.params["groups"]) & store.all_groups.map(&:id)
+      landed = Import::Land.call(store, landing, group: group.empty? ? nil : group, join:)
       Import::Staged.close(upload)
 
       import_screen(landed:)
diff --git a/sig/pro_tacts/admin.rbs b/sig/pro_tacts/admin.rbs
index 9724a28..721570f 100644
--- a/sig/pro_tacts/admin.rbs
+++ b/sig/pro_tacts/admin.rbs
@@ -46,7 +46,7 @@ module ProTacts
     end
 
     class ImportReview
-      def self.call: (upload: String, rows: Array[[Contact, Integer]], unknown: Array[::ProTacts::Import::Vcf::Unknown], group: String, ?notice: String?) -> String
+      def self.call: (upload: String, rows: Array[[Contact, Integer]], unknown: Array[::ProTacts::Import::Vcf::Unknown], group: String, groups: Array[::ProTacts::Store::Group], ?notice: String?) -> String
     end
 
     # Rendered into ContactsEdit rather than called, so it is the one
diff --git a/test/pro_tacts/admin/test_import_pages.rb b/test/pro_tacts/admin/test_import_pages.rb
index 72ad0fc..f306d54 100644
--- a/test/pro_tacts/admin/test_import_pages.rb
+++ b/test/pro_tacts/admin/test_import_pages.rb
@@ -261,6 +261,40 @@ class AdminImportPagesTest < Minitest::Test
     end
   end
 
+  # The groups this book already has, offered beside the name the
+  # import would make: an import is as often people who belong in a
+  # group that exists as it is a batch that only needs finding again.
+  def test_the_review_offers_the_groups_this_book_already_has
+    with_contacts({}) do |store|
+      school = store.create_group(name: "school")
+
+      upload(JANE)
+      follow_redirect!
+
+      assert_includes last_response.body, %(<input type="text" name="group" value="import-)
+      assert_includes last_response.body, %(<input type="checkbox" name="groups[]" value="#{school}">school)
+    end
+  end
+
+  # Ticked and typed are both joins. The id that names no group is
+  # dropped rather than carried into Store#add_member, which reads a
+  # group with `sole` and would answer bad input with a 500.
+  def test_confirming_joins_the_groups_that_were_ticked
+    with_contacts({}) do |store|
+      school = store.create_group(name: "school")
+      id = upload(JANE)
+
+      post "/import/#{id}/land", "group" => "import-20260921T031655Z", "groups" => [school, "zzzz"]
+
+      assert_equal 200, last_response.status
+      landed = store.contacts.fetch(0)
+
+      assert_equal [landed.id], store.group(school).members
+      assert_equal ["import-20260921T031655Z", "school", ProTacts::Store::EVERYONE],
+                   store.all_groups.select { it.members.include?(landed.id) }.map(&:name).sort
+    end
+  end
+
   def test_confirming_lands_the_contacts_as_they_stand
     with_contacts({}) do |store|
       id = upload(JANE + PLAIN)
diff --git a/test/pro_tacts/import/test_land.rb b/test/pro_tacts/import/test_land.rb
index 7fd4138..4d1c8a5 100644
--- a/test/pro_tacts/import/test_land.rb
+++ b/test/pro_tacts/import/test_land.rb
@@ -81,6 +81,35 @@ class ImportLandTest < Minitest::Test
     end
   end
 
+  # The groups the review screen ticked, joined alongside the name it
+  # typed: an import is as often "these are the people from the school
+  # list" as it is a batch that only needs finding again.
+  def test_arrivals_join_the_groups_the_review_chose
+    with_contacts({}) do |store|
+      school = store.create_group(name: "school")
+
+      landed = Land.call(store, Vcf.cards(JANE), group: "import-20260921T031655Z", join: [school])
+      id = landed.fetch(0).id
+
+      assert_equal [id], store.group(school).members
+      assert_equal ["import-20260921T031655Z", "school", ProTacts::Store::EVERYONE],
+                   store.all_groups.select { it.members.include?(id) }.map(&:name).sort
+    end
+  end
+
+  # Either, or neither: the chosen groups stand on their own when no
+  # name is typed.
+  def test_arrivals_join_a_chosen_group_with_no_group_named
+    with_contacts({}) do |store|
+      school = store.create_group(name: "school")
+
+      landed = Land.call(store, Vcf.cards(JANE), join: [school])
+
+      assert_equal landed.map(&:id), store.group(school).members
+      assert_equal %w[school], store.all_groups.map(&:name).reject { it == ProTacts::Store::EVERYONE }
+    end
+  end
+
   def test_no_group_name_lands_the_cards_in_everyones_book_alone
     land do |landed, store|
       assert_equal [ProTacts::Store::EVERYONE], store.all_groups.map(&:name)