import: the groups beside a card, largest first
The picker an arrival's groups are ticked in sorted alphabetically,
the groups dialog's order. Sorted by how many are in each group, the
ones most of the book already belongs to lead the list and stand above
the cap, with ties still alphabetical. Store#group_choices counts each
group's members in the same read, from group_members' primary key,
so no card is read to do it. The groups dialog stays alphabetical.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019a8oAqFrgAfiL5UDvELUV9
diff --git a/lib/pro_tacts/admin/import_groups.rb b/lib/pro_tacts/admin/import_groups.rb
index 92d7640..b4ded9c 100644
--- a/lib/pro_tacts/admin/import_groups.rb
+++ b/lib/pro_tacts/admin/import_groups.rb
@@ -43,9 +43,13 @@ module ProTacts
# @rbs @named: Array[String]
# @rbs @capped: Array[String]
- # Alphabetical, the groups dialog's own order: this is a list to
- # find a name in, and nothing here stays put across a rename.
- # `groups` is every group as a choice, no members read
+ # Largest first, and alphabetical among groups the same size:
+ # the groups an arrival is likeliest to belong in are the ones
+ # most of the book already does, so they are the rows the cap
+ # leaves standing, and the filter reaches the rest. The groups
+ # dialog stays alphabetical — there a contact's groups are
+ # being looked over, not guessed at. `groups` is every group as
+ # a choice, counted rather than its members read
# (Store#group_choices); `joined` the ids ticked, `named` the
# groups this card is making that do not exist yet.
#
@@ -62,7 +66,7 @@ module ProTacts
# rather than listed in it.
#: (groups: Array[Store::GroupChoice], joined: Array[String], named: Array[String]) -> void
def initialize(groups:, joined:, named:)
- @groups = groups.sort_by { it.label.downcase }
+ @groups = groups.sort_by { [-it.member_count, it.label.downcase] }
@joined = joined
@named = named
@capped = GroupFilter.capped(@groups.map(&:id), joined:, shown: named.length)
diff --git a/lib/pro_tacts/store.rb b/lib/pro_tacts/store.rb
index db5b3ca..e91be67 100644
--- a/lib/pro_tacts/store.rb
+++ b/lib/pro_tacts/store.rb
@@ -145,12 +145,13 @@ module ProTacts
# and no more. A Group carries the lines it lends and the ids of
# its members, two reads beyond the group row itself (#load_groups);
# a picker is a list of names to tick and needs neither, so
- # #group_choices reads only the row. The label is the same SQL one
- # (#group_label), so a choice and a tag never disagree about what
- # to call a nameless group. Signed in sig/pro_tacts/store.rbs,
- # being a Data class.
+ # #group_choices reads only the row and a count of who is in it,
+ # which the import's picker sorts by (Admin::ImportGroups). The
+ # label is the same SQL one (#group_label), so a choice and a tag
+ # never disagree about what to call a nameless group. Signed in
+ # sig/pro_tacts/store.rbs, being a Data class.
# @rbs skip
- GroupChoice = Data.define(:id, :name, :label)
+ GroupChoice = Data.define(:id, :name, :label, :member_count)
# SQLite has no ON UPDATE, so the column default stamps a row on
# insert and this stamps it again on the way past. Same expression as
@@ -674,18 +675,22 @@ module ProTacts
load_groups(groups.select(*GROUP_COLUMNS, group_label.as(:label)).order(:id).all)
end
- # Every group as a picker needs it: the row and its label, without
- # the lines it lends or the members it holds. #all_groups' one
- # read where a picker would otherwise pay for two, the members read
- # among them being the one that grows with the book — `sync:*`
- # holds every contact, so listing groups to tick would read the
- # whole address book to show none of it (Admin::GroupDialog,
- # Admin::ImportGroups). Ordered by id like #all_groups; a picker
- # sorts by label itself.
+ # Every group as a picker needs it: the row, its label and how many
+ # are in it, without the lines it lends or the members it holds.
+ # #all_groups' one read where a picker would otherwise pay for two,
+ # the members read among them being the one that grows with the
+ # book — `sync:*` holds every contact, so listing groups to tick
+ # would read the whole address book to show none of it
+ # (Admin::GroupDialog, Admin::ImportGroups). The count is a
+ # subquery SQLite answers from group_members' primary key, which
+ # leads with group_id, so no card is read to make it. Ordered by
+ # id like #all_groups; a picker sorts by what it shows.
#: () -> Array[GroupChoice]
def group_choices
- groups.select(*GROUP_COLUMNS, group_label.as(:label)).order(:id).all.map { |row|
- GroupChoice.new(id: row.fetch(:id).to_s, name: row.fetch(:name)&.to_s, label: row.fetch(:label).to_s)
+ member_count = group_members.where(group_id: Sequel[:groups][:id]).select(Sequel.function(:count).*)
+ groups.select(*GROUP_COLUMNS, group_label.as(:label), member_count.as(:member_count)).order(:id).all.map { |row|
+ GroupChoice.new(id: row.fetch(:id).to_s, name: row.fetch(:name)&.to_s, label: row.fetch(:label).to_s,
+ member_count: row.fetch(:member_count).to_i)
}
end
diff --git a/sig/gems/sequel.rbs b/sig/gems/sequel.rbs
index 32b10bc..5bdb978 100644
--- a/sig/gems/sequel.rbs
+++ b/sig/gems/sequel.rbs
@@ -92,6 +92,9 @@ module Sequel
# as the query builder's rather than Enumerable's own #select.
def select: (*untyped columns) -> Dataset
def limit: (Integer count) -> Dataset
+ # A dataset as a subquery under a name, for a column computed in
+ # another dataset's select (Store#group_choices' member count).
+ def as: (Symbol name) -> untyped
# Hash conditions pair the joined table's column with the source's
# (`id: :card_id` is cards.id = birthdays.card_id); the array form
# joins same-named columns.
diff --git a/sig/pro_tacts/store.rbs b/sig/pro_tacts/store.rbs
index 7263602..3e52e5e 100644
--- a/sig/pro_tacts/store.rbs
+++ b/sig/pro_tacts/store.rbs
@@ -50,8 +50,9 @@ module ProTacts
attr_reader id: String
attr_reader name: String?
attr_reader label: String
+ attr_reader member_count: Integer
- def self.new: (id: String, name: String?, label: String) -> instance
+ def self.new: (id: String, name: String?, label: String, member_count: Integer) -> instance
end
class Snapshot
diff --git a/test/pro_tacts/admin/test_import_pages.rb b/test/pro_tacts/admin/test_import_pages.rb
index 8eafedb..f05ad66 100644
--- a/test/pro_tacts/admin/test_import_pages.rb
+++ b/test/pro_tacts/admin/test_import_pages.rb
@@ -663,8 +663,8 @@ class AdminImportPagesTest < Minitest::Test
get "/import/#{id}/0"
- # Alphabetical, and the import's own name takes the eighth row,
- # so Group h and Zulus are the two that go under.
+ # All empty, so alphabetical, and the import's own name takes
+ # the eighth row, so Group h and Zulus are the two that go under.
assert_includes last_response.body, %(<label data-label="zulus" data-capped :hidden="!visible($el)">)
assert_equal 2, last_response.body.scan("data-capped").length
assert_includes last_response.body, "show all 9 groups"
@@ -682,6 +682,29 @@ class AdminImportPagesTest < Minitest::Test
end
end
+ # The groups most of the book is in are the ones an arrival is
+ # likeliest to join, so they lead the list and stand above the cap;
+ # a tie is alphabetical (Admin::ImportGroups). Everyone's book holds
+ # both stored contacts, as Zulus does, and `sync:*` sorts first.
+ def test_the_groups_beside_a_card_are_largest_first
+ with_contacts({"jane" => JANE, "sam" => PLAIN}) do |store|
+ alpha = store.create_group(name: "alpha")
+ beta = store.create_group(name: "beta")
+ zulus = store.create_group(name: "zulus")
+ store.add_member(zulus, "jane")
+ store.add_member(zulus, "sam")
+ store.add_member(beta, "jane")
+ id = upload(JANE)
+
+ get "/import/#{id}/0"
+
+ shown = last_response.body.scan(/name="groups\[\]" value="([^"]+)"/).flatten
+ everyone = store.group_choices.find { it.name == ProTacts::Store::EVERYONE }.id
+
+ assert_equal [everyone, zulus, beta, alpha], shown
+ end
+ end
+
def test_the_groups_beside_a_card_offer_no_such_thing_uncapped
with_contacts({}) do |store|
# Seven, the import's own name being the eighth row.
diff --git a/test/pro_tacts/test_store.rb b/test/pro_tacts/test_store.rb
index b2215ef..225d03d 100644
--- a/test/pro_tacts/test_store.rb
+++ b/test/pro_tacts/test_store.rb
@@ -1345,18 +1345,17 @@ class StoreTest < Minitest::Test
end
# The pickers' read: id, name and the same coalesced label a tag
- # shows, and nothing about who is in a group — a nameless group is
- # its id here as everywhere. A choice a member joins reads the same
- # as one nobody is in, the membership being no part of what a picker
- # shows.
- def test_group_choices_carry_the_label_without_the_membership
+ # shows, and how many are in a group rather than who — a nameless
+ # group is its id here as everywhere, and one nobody is in counts
+ # nought rather than going missing from the list.
+ def test_group_choices_carry_the_label_and_a_member_count
with_store({"aiden" => UUID_CARD}) do |store|
named = store.create_group(name: "Household")
nameless = store.create_group
store.add_member(named, "aiden")
- assert_equal({named => ["Household", "Household"], nameless => [nil, nameless]}.sort.to_h,
- store.group_choices.to_h { [it.id, [it.name, it.label]] })
+ assert_equal({named => ["Household", "Household", 1], nameless => [nil, nameless, 0]}.sort.to_h,
+ store.group_choices.to_h { [it.id, [it.name, it.label, it.member_count]] })
end
end