Keep madmin from writing blank shopkeeper tokens

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## [Unreleased]
 
+- Fix saving a shopkeeper in madmin failing with 500 (`PG::UniqueViolation` on `index_shopkeepers_on_reset_password_token` or `index_shopkeepers_on_confirmation_token`). The edit form sends empty token fields as `""`, so a second shopkeeper with no reset or confirmation token collided with the first. madmin's shopkeeper form no longer shows `reset_password_token` and `confirmation_token` (read only on the show page), and a migration clears the `""` already stored. An earlier version of this fix normalized blank tokens to NULL in `Shopkeeper`; that also rewrote finder values, so `where(reset_password_token: "")` matched every shopkeeper without a token, and it was removed. Ported from myturntagapi, where it hit production (#158)
 - API change: `Shop#time_zone` and `Shopkeeper#time_zone` must be a Rails time zone name (`ActiveSupport::TimeZone.all.map(&:name)`, e.g. `Tokyo`, `London`); anything else, IANA ids such as `Asia/Tokyo` included, gets 422 "Time zone is not included in the list" from `PATCH /shops/:id`, `POST /shops`, sign-up and `PATCH /shopkeeper_auth` instead of being stored. `test/config/time_zones_test.rb` pins the list against `test/fixtures/files/rails_time_zones.tsv`, which the iOS and Android apps embed for their time zone pickers (#155)
 - Add optional Sentry error monitoring (`sentry-ruby` / `sentry-rails` 7.1). Off unless `SENTRY_DSN` is set (a Kamal secret read from `NATIVEAPPTEMPLATEAPI_SENTRY_DSN`, optional); sends only from production, only unhandled errors (no 4xx, no tracing, profiling or breadcrumbs), with the shopkeeper's ID as the only user data, the account ID as a tag, devise_token_auth headers dropped, email addresses masked and `filter_parameters` applied. The README's "Error monitoring (Sentry)" covers turning it on, what is sent and the recommended Sentry privacy settings. New tests: `test/config/sentry_test.rb`, `test/integration/sentry_context_test.rb` (#154)
 - API change: tell the apps when the organization in the URL is gone. An `/<account uuid>/api/...` request for a deleted account now gets `404 {"code":404,"error_code":"account_not_found","error_message":...}` JSON (was a `302` to `/`), and a signed-in shopkeeper who is no longer a member gets `403 {"code":403,"error_code":"not_account_member","error_message":...}` (was Pundit's generic 401, which the apps treated as a sign-out). Messages are in English and Japanese and follow the usual locale rules. Pundit denials inside an organization the shopkeeper belongs to stay 401; non-API paths still redirect (#153)

app/madmin/resources/shopkeeper_resource.rb

@@ -4,11 +4,13 @@ class ShopkeeperResource < Madmin::Resource
   attribute :provider
   attribute :uid
   attribute :encrypted_password
-  attribute :reset_password_token
+  # Read only: the form would send "" for an empty token, which breaks its unique index
+  attribute :reset_password_token, form: false
   attribute :reset_password_sent_at
   attribute :allow_password_change
   attribute :remember_created_at
-  attribute :confirmation_token
+  # Read only: the form would send "" for an empty token, which breaks its unique index
+  attribute :confirmation_token, form: false
   attribute :confirmed_at
   attribute :confirmation_sent_at
   attribute :unconfirmed_email

app/models/shopkeeper.rb

@@ -17,6 +17,11 @@ class Shopkeeper < ApplicationRecord
   has_many :application_push_devices, as: :owner, class_name: "ApplicationPushDevice", dependent: :destroy
   has_many :notifications, as: :recipient, dependent: :destroy, class_name: "Noticed::Notification"
 
+  # No `normalizes` on the token columns: it also rewrites finder values, and
+  # turning "" into nil makes where(reset_password_token: "") match every
+  # shopkeeper without a token. madmin keeps the tokens out of its edit form
+  # instead (ShopkeeperResource), so it never writes "" into their unique indexes.
+
   attribute :token, :string
   attribute :client, :string
   attribute :expiry, :string

db/migrate/20261017000000_nullify_blank_shopkeeper_tokens.rb

@@ -0,0 +1,12 @@
+# Saving a shopkeeper in madmin stored "" in empty token fields; a second ""
+# then broke the unique indexes. Shopkeeper now normalizes blanks to NULL, and
+# this clears the "" already stored.
+class NullifyBlankShopkeeperTokens < ActiveRecord::Migration[8.1]
+  def up
+    execute "UPDATE shopkeepers SET reset_password_token = NULL WHERE reset_password_token = ''"
+    execute "UPDATE shopkeepers SET confirmation_token = NULL WHERE confirmation_token = ''"
+  end
+
+  def down
+  end
+end

test/integration/madmin_test.rb

@@ -61,6 +61,30 @@ class MadminTest < ActionDispatch::IntegrationTest
     assert_equal "Renamed by admin", @shop.reload.name
   end
 
+  # The edit form sends every field it shows, not just the one the admin changed, so
+  # empty token fields arrive as "". Ported from myturntagapi, where in production
+  # (2026-10-17) saving a shopkeeper whose reset_password_token was nil failed with
+  # PG::UniqueViolation on index_shopkeepers_on_reset_password_token,
+  # "Key (reset_password_token)=() already exists", because an earlier save had
+  # stored "" on another shopkeeper.
+  test "saves the shopkeeper edit form as the browser submits it" do
+    shopkeepers(:two).update_columns(reset_password_token: "", confirmation_token: "")
+    @shopkeeper.update_columns(reset_password_token: nil, confirmation_token: nil)
+    sign_in_admin
+    get ShopkeeperResource.edit_path(@shopkeeper)
+    assert_response :success
+
+    form = Nokogiri::HTML(response.body).at_css("form[action$='/shopkeepers/#{@shopkeeper.id}']")
+    fields = form_fields(form).map do |name, value|
+      (name == "shopkeeper[name]") ? [name, "Renamed by admin"] : [name, value]
+    end
+
+    patch madmin_shopkeeper_path(@shopkeeper), params: URI.encode_www_form(fields),
+      headers: {"CONTENT_TYPE" => "application/x-www-form-urlencoded"}
+    assert_redirected_to madmin_shopkeeper_path(@shopkeeper)
+    assert_equal "Renamed by admin", @shopkeeper.reload.name
+  end
+
   test "blobs and variant records hide write actions" do
     sign_in_admin
 
@@ -99,6 +123,23 @@ class MadminTest < ActionDispatch::IntegrationTest
 
   private
 
+  # [name, value] pairs a browser sends for the form: filled inputs and textareas,
+  # checked boxes and the selected options of each select
+  def form_fields(form)
+    form.css("input, textarea, select").flat_map do |field|
+      name = field["name"]
+      next [] if name.blank? || field["disabled"] || %w[submit button].include?(field["type"])
+
+      case field.name
+      when "select" then field.css("option[selected]").map { |option| [name, option["value"] || option.text] }
+      when "textarea" then [[name, field.text]]
+      else
+        next [] if %w[checkbox radio].include?(field["type"]) && !field["checked"]
+        [[name, field["value"].to_s]]
+      end
+    end
+  end
+
   def sign_in_admin
     post admin_session_path, params: {email: @admin_user.email, password: "password"}
   end

test/models/shopkeeper_test.rb

@@ -315,4 +315,17 @@ class ShopkeeperTest < ActiveSupport::TestCase
     shopkeeper.name = "Renamed"
     assert shopkeeper.valid?
   end
+
+  # A token column must never be normalized: normalization also runs on finder values,
+  # so turning "" into nil would make where(reset_password_token: "") match every
+  # shopkeeper with no token (WHERE reset_password_token IS NULL)
+  test "a blank token finds no shopkeeper" do
+    shopkeepers(:one).update_columns(reset_password_token: nil, confirmation_token: nil)
+
+    # where, as Devise's lookups do (orm_adapter's find_first); find_by binds nil as
+    # "= NULL" instead and would match nothing either way
+    assert_not Shopkeeper.where(reset_password_token: "").exists?
+    assert_not Shopkeeper.where(confirmation_token: "").exists?
+    assert_nil Shopkeeper.with_reset_password_token("")
+  end
 end