groups: a tick on the filter's offer commits the name
The offer's box rode the filter inside an x-if, so clearing the filter removed the ticked name from the page and the save made no group. Assisted-by: GLM-5.3 via pi
diff --git a/lib/pro_tacts/admin/group_dialog.rb b/lib/pro_tacts/admin/group_dialog.rb
index c0882a8..94d802e 100644
--- a/lib/pro_tacts/admin/group_dialog.rb
+++ b/lib/pro_tacts/admin/group_dialog.rb
@@ -18,11 +18,12 @@ module ProTacts
# form here, because Enter in a form's text box would save
# mid-filter and there is somewhere outside to put it.
#
- # Closing the popover puts the cap and the filter back. Alpine's
- # state belongs to the element, and a popover is hidden rather
- # than rebuilt, so without the reset a dialog reopens on whatever
- # the last visit left in it — a filter typed ten minutes ago, or
- # the whole list still unrolled.
+ # Closing the popover puts the cap, the filter, and any names
+ # ticked into making back. Alpine's state belongs to the element,
+ # and a popover is hidden rather than rebuilt, so without the
+ # reset a dialog reopens on whatever the last visit left in it — a
+ # filter typed ten minutes ago, the whole list still unrolled, or
+ # a name committed and then left there by a Cancel.
class GroupDialog < Phlex::HTML
ID = "edit-groups" #: String
FORM = "edit-groups-form" #: String
@@ -50,7 +51,7 @@ module ProTacts
def view_template
dialog(id: ID, popover: "auto", x_data: GroupFilter::STATE,
- "@toggle": "if ($event.newState === 'closed') { all = false; filter = '' }") do
+ "@toggle": "if ($event.newState === 'closed') { all = false; filter = ''; named = [] }") do
header { "Groups" }
div(class: "field-stack") do
render GroupFilter.new(autofocus: true)
@@ -64,6 +65,7 @@ module ProTacts
end
input(type: "hidden", name: "was[]", value: group.id) if joined
end
+ render GroupFilter::Named.new
render GroupFilter::Fresh.new
end
render GroupFilter::Empty.new(any: @groups.any?)
diff --git a/lib/pro_tacts/admin/group_filter.rb b/lib/pro_tacts/admin/group_filter.rb
index 1bc70f0..c593db2 100644
--- a/lib/pro_tacts/admin/group_filter.rb
+++ b/lib/pro_tacts/admin/group_filter.rb
@@ -36,16 +36,22 @@ module ProTacts
# cap adds to `shows`: a capped row joins the list once the
# filter is typed into or the whole list is asked for, and a
# filter is the one of the two that can reach a single group
- # without the rest.
- STATE = "{ filter: '', all: false, " \
+ # without the rest. `named` is the names a tick on the offer
+ # committed to making (Named): `creatable` and `none` read it
+ # directly rather than off the rows' `data-label`s, which those
+ # two only re-read when the filter changes — a name committed
+ # since would be invisible to both until the next keystroke.
+ STATE = "{ filter: '', all: false, named: [], " \
"get needle() { return this.filter.trim().toLowerCase() }, " \
"get labels() { return [...this.$refs.options.querySelectorAll('[data-label]')]" \
".map(row => row.dataset.label) }, " \
"shows(label) { return label.includes(this.needle) }, " \
"visible(row) { return this.shows(row.dataset.label) && " \
"(this.all || this.needle !== '' || !('capped' in row.dataset)) }, " \
- "get none() { return !this.labels.some(label => this.shows(label)) }, " \
- "get creatable() { return this.needle !== '' && !this.labels.includes(this.needle) } }" #: String
+ "get none() { return !this.labels.some(label => this.shows(label)) && " \
+ "!this.named.some(name => name.toLowerCase().includes(this.needle)) }, " \
+ "get creatable() { return this.needle !== '' && !this.labels.includes(this.needle) && " \
+ "!this.named.some(name => name.toLowerCase() === this.needle) } }" #: String
# What a filterable row wears: the label to match on, read off
# the element rather than out of a closure, whether the cap holds
@@ -109,21 +115,48 @@ module ProTacts
end
# The filter itself, offered as a group to make, under the rows
- # it matched none of. Inside the form, being a field of it, and
- # unticked: a filter is as often half a name typed to find a
- # group as it is a new one, and a save must not make "boo" on
- # the way to Booles.
+ # it matched none of, and unticked: a filter is as often half a
+ # name typed to find a group as it is a new one, and a save must
+ # not make "boo" on the way to Booles. A tick is the commit
+ # rather than a box the save reads: the name stops riding the
+ # filter — which clearing would otherwise carry it off on, and
+ # so would typing one more letter past the tick — and stands as
+ # one of Named's rows instead.
class Fresh < Phlex::HTML
def view_template
template(x_if: "creatable") do
label do
- input(type: "checkbox", name: "new", ":value": "filter.trim()")
+ input(type: "checkbox",
+ "@change": "if ($event.target.checked) named.push(filter.trim())")
span(class: "gl-muted", style: "font-style: italic;", x_text: "filter.trim()")
end
end
end
end
+ # The names a tick on Fresh committed, standing rows in the
+ # shape the import gives the names a refused save sent back
+ # (Admin::ImportGroups): ticked, submitting `named[]`, and
+ # independent of the filter — which is the whole difference
+ # from the offer, whose row lives on the filter's own text. An
+ # untick withdraws the name, back to the offer if the filter
+ # still says it. Never capped, being ticked, and hidden by the
+ # filter like any row — said straight to `shows` rather than
+ # through `data-label`/`visible($el)`, those reading the DOM
+ # the server rendered, and this row being made client-side
+ # where the name is in hand.
+ class Named < Phlex::HTML
+ def view_template
+ template(x_for: "name in named") do
+ label(":hidden": "!shows(name.toLowerCase())") do
+ input(type: "checkbox", name: "named[]", ":value": "name", checked: true,
+ "@change": "named = named.filter(n => n !== name)")
+ span(class: "gl-muted", style: "font-style: italic;", x_text: "name")
+ end
+ end
+ end
+ end
+
# What the list says when the filter matches nothing and is not
# a name either — which, the filter being blank, is also what an
# empty book says.
diff --git a/lib/pro_tacts/admin/import_groups.rb b/lib/pro_tacts/admin/import_groups.rb
index b4ded9c..64c3649 100644
--- a/lib/pro_tacts/admin/import_groups.rb
+++ b/lib/pro_tacts/admin/import_groups.rb
@@ -100,6 +100,11 @@ module ProTacts
span(class: "gl-muted", style: "font-style: italic;") { name }
end
end
+ # The same row client-side, for names the offer's tick has
+ # committed since this page loaded: the tick is what
+ # commits (Admin::GroupFilter), so the name stands here
+ # rather than riding the filter that named it.
+ render GroupFilter::Named.new
render GroupFilter::Fresh.new
end
render GroupFilter::Empty.new(any: @groups.any? || @named.any?)
diff --git a/lib/pro_tacts/store.rb b/lib/pro_tacts/store.rb
index e91be67..58b050f 100644
--- a/lib/pro_tacts/store.rb
+++ b/lib/pro_tacts/store.rb
@@ -817,13 +817,13 @@ module ProTacts
end
# A card's side of #edit_group: one transaction, leavers first and
- # joiners last for that method's reason. A name creates a group to
- # join inside the same transaction, so a taken name (#create_group)
- # moves nothing.
- #: (String card_id, join: Array[String], leave: Array[String], ?create: String?) -> void
- def regroup(card_id, join:, leave:, create: nil)
+ # 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.
+ #: (String card_id, join: Array[String], leave: Array[String], ?create: Array[String]) -> void
+ def regroup(card_id, join:, leave:, create: [])
@database.transaction do
- join += [create_group(name: create)] if create
+ join += create.map { create_group(name: it) }
leave.each { remove_member(it, card_id) }
join.each { add_member(it, card_id) }
end
diff --git a/lib/pro_tacts/web/contacts.rb b/lib/pro_tacts/web/contacts.rb
index 4a84d21..26fc7b0 100644
--- a/lib/pro_tacts/web/contacts.rb
+++ b/lib/pro_tacts/web/contacts.rb
@@ -207,13 +207,26 @@ module ProTacts
known = store.group_choices.map(&:id)
checked = ids_in(r.params["groups"]) & known
was = ids_in(r.params["was"]) & known
- name = r.params["new"].to_s.strip
+ # The names the picker's offer committed, each made with this
+ # save (Admin::GroupFilter::Named) — a tick is what commits a
+ # name, so there is no single `new` riding the filter to read.
+ names = ids_in(r.params["named"]).map(&:strip).reject(&:empty?)
begin
- store.regroup(id, join: checked - was, leave: was - checked, create: (name unless name.empty?))
+ store.regroup(id, join: checked - was, leave: was - checked, create: names)
rescue Sequel::UniqueConstraintViolation
# A taken name (db/migrations/008_group_names.rb), refused with
- # the rest of the save in regroup's transaction.
- return contact_screen(contact, notice: "Another group is already named #{name}; nothing was saved.")
+ # the rest of the save in regroup's transaction. The page's own
+ # list is what keeps the picker from offering a taken name, so
+ # a save that finds one is looking at a list gone stale — the
+ # name is asked of the store rather than of the form.
+ clash = names.find { |name| store.group_choices.any? { it.name == name } }
+ notice =
+ if clash
+ "Another group is already named #{clash}; nothing was saved."
+ else
+ "A group already holds one of those names; nothing was saved."
+ end
+ return contact_screen(contact, notice:)
end
r.redirect "/contacts/#{id}", 303
end
diff --git a/lib/pro_tacts/web/import.rb b/lib/pro_tacts/web/import.rb
index 370cf67..ba42dec 100644
--- a/lib/pro_tacts/web/import.rb
+++ b/lib/pro_tacts/web/import.rb
@@ -306,8 +306,9 @@ module ProTacts
# #apply_groups' own rule, and the boxes are the whole answer —
# none ticked is none joined, the group for the import included,
# which is a box like any other here (#import_picker). A name
- # typed into the filter joins the names already standing for
- # this contact, and is made by the write below
+ # typed into the filter is committed by its tick into the
+ # standing names (Admin::GroupFilter), so it survives the
+ # filter being cleared, and is made by the write below
# (Import::Write#group_id).
#
# Read before the refusals rather than after, so a card sent
@@ -316,9 +317,7 @@ module ProTacts
# them until the write.
choices = store.group_choices
ticked = ids_in(r.params["groups"]) & choices.map(&:id)
- standing = ids_in(r.params["named"]).map(&:strip).reject(&:empty?)
- fresh = r.params["new"].to_s.strip
- wanted = (fresh.empty? ? standing : standing + [fresh]).uniq
+ wanted = ids_in(r.params["named"]).map(&:strip).reject(&:empty?)
# Everyone's book is one of those boxes, and the only one the
# write cannot take as an id: on the screen where unticking it
# matters least — the first card into an empty book — the group
diff --git a/test/pro_tacts/admin/test_contacts_pages.rb b/test/pro_tacts/admin/test_contacts_pages.rb
index 8bb3600..4a3cb35 100644
--- a/test/pro_tacts/admin/test_contacts_pages.rb
+++ b/test/pro_tacts/admin/test_contacts_pages.rb
@@ -450,12 +450,17 @@ class AdminContactsPagesTest < Minitest::Test
end
end
- def test_the_groups_dialog_offers_the_filter_as_an_unchecked_new_group
+ def test_the_groups_dialogs_filter_offer_commits_its_name_on_the_tick
with_contacts({"ada" => ADA}) do
get "/contacts/ada"
dialog = last_response.body[%r{<dialog id="edit-groups".*?</dialog>}].to_s
- assert_includes dialog, %(<input type="checkbox" name="new" :value="filter.trim()">)
+ assert_includes dialog,
+ %(<input type="checkbox" @change="if ($event.target.checked) named.push(filter.trim())">)
+ # And somewhere for the committed name to stand, inside the form
+ # so the save carries it.
+ assert_includes dialog, %(<template x-for="name in named">)
+ assert_includes dialog, %(name="named[]" :value="name" checked)
end
end
@@ -528,7 +533,7 @@ class AdminContactsPagesTest < Minitest::Test
get "/contacts/ada"
assert_includes last_response.body,
- %(@toggle="if ($event.newState === 'closed') { all = false; filter = '' }")
+ %(@toggle="if ($event.newState === 'closed') { all = false; filter = ''; named = [] }")
end
end
@@ -595,16 +600,27 @@ class AdminContactsPagesTest < Minitest::Test
with_contacts({"ada" => ADA}) do |store|
booles = FixtureData.seed_group(store, name: "Booles")
- post "/contacts/ada/groups", groups: [booles], new: " Clarks "
+ post "/contacts/ada/groups", groups: [booles], named: [" Clarks "]
assert_equal 303, last_response.status
assert_equal ["Booles", "Clarks"], store.groups_of("ada").map(&:label).sort
end
end
+ # Several, because the commit is per name: a filter names one group
+ # at a time, and nothing stops a person naming two before the save.
+ def test_several_new_groups_are_created_with_one_save
+ with_contacts({"ada" => ADA}) do |store|
+ post "/contacts/ada/groups", named: ["Clarks", " Babbages "]
+
+ assert_equal 303, last_response.status
+ assert_equal ["Babbages", "Clarks"], store.groups_of("ada").map(&:label).sort
+ end
+ end
+
def test_a_blank_new_group_creates_nothing
with_contacts({"ada" => ADA}) do |store|
- post "/contacts/ada/groups", new: " "
+ post "/contacts/ada/groups", named: [" "]
assert_equal 303, last_response.status
assert_empty store.all_groups
@@ -618,7 +634,7 @@ class AdminContactsPagesTest < Minitest::Test
booles = FixtureData.seed_group(store, name: "Booles")
FixtureData.seed_group(store, name: "Clarks")
- post "/contacts/ada/groups", groups: [booles], new: "Clarks"
+ post "/contacts/ada/groups", groups: [booles], named: ["Clarks"]
assert_equal 200, last_response.status
assert_includes last_response.body, "Another group is already named Clarks; nothing was saved."
diff --git a/test/pro_tacts/admin/test_import_pages.rb b/test/pro_tacts/admin/test_import_pages.rb
index c397452..d1bff35 100644
--- a/test/pro_tacts/admin/test_import_pages.rb
+++ b/test/pro_tacts/admin/test_import_pages.rb
@@ -502,7 +502,7 @@ class AdminImportPagesTest < Minitest::Test
with_contacts({}) do |store|
id = upload(JANE)
- save(id, 0, first: "Jane", last: "Booles", "named" => [], "new" => " Clarks ")
+ save(id, 0, first: "Jane", last: "Booles", "named" => [" Clarks "])
clarks = store.all_groups.find { it.name == "Clarks" }
@@ -616,7 +616,7 @@ class AdminImportPagesTest < Minitest::Test
post "/import/#{id}/0",
"etag" => "not the etag", "first" => "Jane", "middle" => "", "last" => "Booles",
- "nickname" => "", "note" => "", "groups" => [school], "new" => "Clarks"
+ "nickname" => "", "note" => "", "groups" => [school], "named" => ["Clarks"]
assert_includes last_response.body,
%(<input type="checkbox" name="groups[]" value="#{school}" checked>school)
@@ -644,7 +644,12 @@ class AdminImportPagesTest < Minitest::Test
assert_includes last_response.body, %(<input type="search" placeholder="Filter or add groups")
assert_includes last_response.body, %(data-label="school")
- assert_includes last_response.body, %(<input type="checkbox" name="new" :value="filter.trim()">)
+ # The offer's tick commits the name into a standing row rather
+ # than leaving it on the filter, where clearing would lose it.
+ assert_includes last_response.body,
+ %(<input type="checkbox" @change="if ($event.target.checked) named.push(filter.trim())">)
+ assert_includes last_response.body,
+ %(<input type="checkbox" name="named[]" :value="name" checked @change="named = named.filter(n => n !== name)">)
end
end
diff --git a/test/pro_tacts/test_store.rb b/test/pro_tacts/test_store.rb
index 225d03d..fa1ceee 100644
--- a/test/pro_tacts/test_store.rb
+++ b/test/pro_tacts/test_store.rb
@@ -1468,11 +1468,11 @@ class StoreTest < Minitest::Test
end
end
- def test_a_regroup_creates_the_group_it_names_and_joins_it
+ def test_a_regroup_creates_the_groups_it_names_and_joins_them
with_store({"aiden" => AIDEN}) do |store|
- store.regroup("aiden", join: [], leave: [], create: "Clarks")
+ store.regroup("aiden", join: [], leave: [], create: ["Clarks", "Booles"])
- assert_equal ["Clarks"], store.groups_of("aiden").map(&:label)
+ assert_equal ["Booles", "Clarks"], store.groups_of("aiden").map(&:label).sort
end
end