From bcb95424c393ac2a6dab89c01fe3f05b11d7f0a3 Mon Sep 17 00:00:00 2001 From: Zsolt Tasnadi Date: Wed, 19 Aug 2026 14:23:20 +0200 Subject: [PATCH] A store can be listed or not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Soft deletion already said "this store is gone". What was missing is "not yet" — a catalog still being set up, or one pulled from the picker for a while without losing the row and its history. `GET /api/stores` now answers with the active ones only. The client is deliberately told nothing about the flag. It has no state for "there but switched off", and giving it one would mean every client release having an opinion about it; an inactive store is simply absent, which is a case the client already handles because it is the same as never having existed. The payload stays two fields, and a spec holds it there. Default true, so the migration lists every store that exists today. One that silently emptied the registry would be a client with nothing to install from. No index: a handful of rows, read once per client on first run. In the admin the flag is what the page is *for*, so it is not just a checkbox on the form: Active is the default scope, the index shows listed/hidden as a status tag with a one-click toggle beside Edit, and the two batch actions do it in bulk. A request spec covers all of it, because none of it is reachable from a model spec — the pipelines resource shipped without `permit_params` and every edit raised, which is the same layer and the same lesson. The last example toggles in the admin and then reads /api/stores, since a change here that the registry does not reflect is the only failure that actually matters. Co-Authored-By: Claude Opus 5 (1M context) --- apps/api/app/admin/stores.rb | 43 ++++++++- apps/api/app/models/store.rb | 7 +- apps/api/app/services/store_service.rb | 5 +- .../20260819121824_add_active_to_stores.rb | 17 ++++ apps/api/db/schema.rb | 3 +- .../controllers/stores_controller_spec.rb | 20 ++++ apps/api/spec/factories/stores.rb | 4 + apps/api/spec/requests/admin_stores_spec.rb | 91 +++++++++++++++++++ 8 files changed, 184 insertions(+), 6 deletions(-) create mode 100644 apps/api/db/migrate/20260819121824_add_active_to_stores.rb create mode 100644 apps/api/spec/requests/admin_stores_spec.rb diff --git a/apps/api/app/admin/stores.rb b/apps/api/app/admin/stores.rb index 19e7b72..a07d2d0 100644 --- a/apps/api/app/admin/stores.rb +++ b/apps/api/app/admin/stores.rb @@ -1,8 +1,14 @@ ActiveAdmin.register Store do - permit_params :name, :catalog_url + permit_params :name, :catalog_url, :active menu priority: 5, label: "🛒 Stores" + # Active first and by default: the registry is what clients actually read, so the + # useful question on opening this page is "what is being offered right now". + scope("Active", default: true) { |scope| scope.active } + scope("Inactive") { |scope| scope.where(active: false) } + scope :all + index do selectable_column id_column @@ -10,12 +16,38 @@ ActiveAdmin.register Store do column :catalog_url do |store| link_to store.catalog_url, store.catalog_url, target: "_blank", rel: "noopener" end + column :active do |store| + status_tag store.active? ? "listed" : "hidden", class: store.active? ? :ok : :warning + end column :updated_at - actions + actions defaults: true do |store| + # One click, because this is the thing an admin comes to this page to do — and + # the alternative is Edit, a checkbox and Save for a single boolean. + link_to store.active? ? "Hide" : "List", + toggle_admin_store_path(store), + method: :put + end end filter :name filter :catalog_url + filter :active + + member_action :toggle, method: :put do + resource.update!(active: !resource.active?) + redirect_back fallback_location: admin_stores_path, + notice: "#{resource.name} is now #{resource.active? ? 'listed' : 'hidden'}" + end + + batch_action :list do |ids| + Store.where(id: ids).update_all(active: true, updated_at: Time.current) + redirect_to admin_stores_path, notice: "#{ids.size} store(s) listed" + end + + batch_action :hide do |ids| + Store.where(id: ids).update_all(active: false, updated_at: Time.current) + redirect_to admin_stores_path, notice: "#{ids.size} store(s) hidden" + end show do attributes_table do @@ -24,6 +56,9 @@ ActiveAdmin.register Store do row :catalog_url do |store| link_to store.catalog_url, store.catalog_url, target: "_blank", rel: "noopener" end + row :active do |store| + status_tag store.active? ? "listed" : "hidden", class: store.active? ? :ok : :warning + end row :created_at row :updated_at end @@ -31,7 +66,8 @@ ActiveAdmin.register Store do "Listed by GET /api/stores, which the graphical client reads on first run. A name " \ "and a catalog are the whole record: the client carries its own store engine " \ "and configures itself from this much, deriving the store's slug from the " \ - "catalog host." + "catalog host. An inactive store is left out of that answer entirely — the " \ + "client has no state for 'there but switched off'." end end @@ -39,6 +75,7 @@ ActiveAdmin.register Store do f.inputs do f.input :name, hint: "What the client shows in its store picker" f.input :catalog_url, hint: "Base URL of the WarpEngine catalog, e.g. https://teletypegames.org" + f.input :active, hint: "Off keeps the record but leaves it out of GET /api/stores" end f.actions end diff --git a/apps/api/app/models/store.rb b/apps/api/app/models/store.rb index ee067e3..6b89cfe 100644 --- a/apps/api/app/models/store.rb +++ b/apps/api/app/models/store.rb @@ -17,9 +17,14 @@ class Store < ApplicationRecord default_scope { where(deleted_at: nil) } + # Two different "not listed". Soft deletion is "this store is gone"; `active` is + # "not right now" — a catalog still being set up, or one pulled from the picker for + # a while. The client cannot tell the difference and should not have to: it lists + # whatever /api/stores hands it. + scope :active, -> { where(active: true) } scope :ordered, -> { order(:name) } def self.ransackable_attributes(auth_object = nil) - %w[id name catalog_url created_at updated_at] + %w[id name catalog_url active created_at updated_at] end end diff --git a/apps/api/app/services/store_service.rb b/apps/api/app/services/store_service.rb index 30a498d..0ab0c7f 100644 --- a/apps/api/app/services/store_service.rb +++ b/apps/api/app/services/store_service.rb @@ -1,5 +1,8 @@ class StoreService + # Only the active ones. An inactive store is simply absent from the registry — the + # client has no state for "there but switched off", and inventing one would mean + # every client release having an opinion about it. def index - StoreSerializer.render_as_hash(Store.ordered) + StoreSerializer.render_as_hash(Store.active.ordered) end end diff --git a/apps/api/db/migrate/20260819121824_add_active_to_stores.rb b/apps/api/db/migrate/20260819121824_add_active_to_stores.rb new file mode 100644 index 0000000..7825dcd --- /dev/null +++ b/apps/api/db/migrate/20260819121824_add_active_to_stores.rb @@ -0,0 +1,17 @@ +class AddActiveToStores < ActiveRecord::Migration[8.1] + # A store that exists but should not be offered yet. + # + # Soft deletion already covered "this store is gone"; what was missing is "not yet" — + # a catalog being set up, or one taken out of the picker for a while without losing + # the row and its history. The client has no notion of either: it lists whatever + # /api/stores hands it, so the filtering has to happen here. + # + # Default true, so every store that exists today keeps being listed. A migration that + # silently emptied the registry would be a client with nothing to install from. + # + # No index: this table holds a handful of rows and is read once per client on first + # run. An index on a two-valued column would be ceremony. + def change + add_column :stores, :active, :boolean, null: false, default: true + end +end diff --git a/apps/api/db/schema.rb b/apps/api/db/schema.rb index 273869f..1325fc5 100644 --- a/apps/api/db/schema.rb +++ b/apps/api/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_08_19_120000) do +ActiveRecord::Schema[8.1].define(version: 2026_08_19_121824) do create_table "admin_users", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.datetime "created_at", null: false t.datetime "deleted_at", precision: 3 @@ -300,6 +300,7 @@ ActiveRecord::Schema[8.1].define(version: 2026_08_19_120000) do end create_table "stores", id: { type: :bigint, unsigned: true }, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| + t.boolean "active", default: true, null: false t.string "catalog_url", null: false t.datetime "created_at", precision: 3, null: false t.datetime "deleted_at", precision: 3 diff --git a/apps/api/spec/controllers/stores_controller_spec.rb b/apps/api/spec/controllers/stores_controller_spec.rb index 8fb0c05..fdbe6eb 100644 --- a/apps/api/spec/controllers/stores_controller_spec.rb +++ b/apps/api/spec/controllers/stores_controller_spec.rb @@ -32,5 +32,25 @@ RSpec.describe Api::StoresController, type: :request do expect(JSON.parse(response.body)).to be_empty end + + # Two different "not listed", and a client can tell neither apart from the store + # never having existed — which is the point. It has no state for "there but + # switched off", so an inactive store is simply absent. + it "leaves out inactive stores" do + create(:store, name: "Listed") + create(:store, name: "Not yet", catalog_url: "https://soon.example", active: false) + + get "/api/stores" + + expect(JSON.parse(response.body).map { |s| s["name"] }).to eq([ "Listed" ]) + end + + it "still says nothing about the flag itself" do + create(:store) + + get "/api/stores" + + expect(JSON.parse(response.body).first.keys).to contain_exactly("name", "catalogUrl") + end end end diff --git a/apps/api/spec/factories/stores.rb b/apps/api/spec/factories/stores.rb index 02f0acb..3c30943 100644 --- a/apps/api/spec/factories/stores.rb +++ b/apps/api/spec/factories/stores.rb @@ -2,5 +2,9 @@ FactoryBot.define do factory :store do name { "Teletype Games" } catalog_url { "https://teletypegames.org" } + + trait :inactive do + active { false } + end end end diff --git a/apps/api/spec/requests/admin_stores_spec.rb b/apps/api/spec/requests/admin_stores_spec.rb new file mode 100644 index 0000000..b0d5b99 --- /dev/null +++ b/apps/api/spec/requests/admin_stores_spec.rb @@ -0,0 +1,91 @@ +require "rails_helper" +require "warden/test/helpers" + +# The admin is where the flag is actually used, and none of it is reachable from a model +# spec: the scopes, the toggle action and the batch actions are ActiveAdmin plumbing that +# only exists once the host's admin is running. The pipelines resource shipped with a +# missing `permit_params` and every edit raised — same layer, same lesson. +RSpec.describe "Admin stores", type: :request do + include Warden::Test::Helpers + + let(:admin) { AdminUser.create!(email: "stores-spec@example.org", password: "password123") } + let!(:listed) { create(:store, name: "Listed", catalog_url: "https://listed.example") } + let!(:hidden) { create(:store, :inactive, name: "Hidden", catalog_url: "https://hidden.example") } + + before do + Warden.test_mode! + login_as(admin, scope: :admin_user) + end + + after { Warden.test_reset! } + + # Same reasoning as the pipelines spec: a request spec has no rendered form to take a + # CSRF token from, and the token is not what is under test. + around do |example| + protection = ActionController::Base.allow_forgery_protection + ActionController::Base.allow_forgery_protection = false + example.run + ActionController::Base.allow_forgery_protection = protection + end + + it "opens with the active ones" do + get "/admin/stores" + + expect(response).to have_http_status(:ok) + expect(response.body).to include("Listed") + expect(response.body).not_to include("Hidden") + end + + it "shows the hidden ones under their own scope" do + get "/admin/stores", params: { scope: "inactive" } + + expect(response.body).to include("Hidden") + end + + it "renders a store's page" do + get "/admin/stores/#{hidden.id}" + + expect(response).to have_http_status(:ok) + end + + # The one this page exists for: the form saves the flag rather than dropping it as an + # unpermitted attribute, which fails silently — the page redirects and nothing changes. + it "saves the flag from the form" do + put "/admin/stores/#{listed.id}", params: { store: { active: "0" } } + + expect(response).to have_http_status(:found) + expect(listed.reload).not_to be_active + end + + it "toggles from the index in one click" do + put "/admin/stores/#{listed.id}/toggle" + + expect(response).to have_http_status(:found) + expect(listed.reload).not_to be_active + + put "/admin/stores/#{listed.id}/toggle" + expect(listed.reload).to be_active + end + + it "lists and hides in bulk" do + post "/admin/stores/batch_action", + params: { batch_action: "list", collection_selection: [ hidden.id ] } + + expect(hidden.reload).to be_active + + post "/admin/stores/batch_action", + params: { batch_action: "hide", collection_selection: [ hidden.id ] } + + expect(hidden.reload).not_to be_active + end + + # The registry is the whole point of the flag, so the two are checked together: a + # change made here has to be what the client sees. + it "is what /api/stores answers with" do + put "/admin/stores/#{listed.id}/toggle" + + get "/api/stores" + + expect(JSON.parse(response.body)).to be_empty + end +end