From 398562bc12a341bc799f8f056222880cfec9026c Mon Sep 17 00:00:00 2001 From: Sean Rankine Date: Tue, 14 Jul 2026 13:15:37 +0100 Subject: [PATCH 1/7] Add allowed brands for organisations Brands are a global list, but each organisation should only be able to use brands it has been granted. Super admins can now add and remove an organisation's brands from the organisation page. The add page uses an autocomplete so it works with hundreds of brands. --- .../organisation_brands_controller.rb | 51 +++++ app/controllers/organisations_controller.rb | 2 +- .../organisations/brand_input.rb | 29 +++ app/models/brand.rb | 2 + app/models/organisation.rb | 3 + app/models/organisation_brand.rb | 6 + app/policies/organisation_policy.rb | 4 + app/views/organisation_brands/new.html.erb | 31 +++ app/views/organisations/show.html.erb | 29 +++ config/locales/en.yml | 18 ++ .../input_objects/organisation_brand.yml | 10 + config/routes.rb | 4 +- ...260714090000_create_organisation_brands.rb | 11 + db/schema.rb | 14 +- db/seeds.rb | 1 + spec/factories/models/organisation_brands.rb | 6 + spec/models/organisation_brand_spec.rb | 32 +++ .../organisation_brands_controller_spec.rb | 197 ++++++++++++++++++ .../requests/organisations_controller_spec.rb | 7 + 19 files changed, 454 insertions(+), 3 deletions(-) create mode 100644 app/controllers/organisation_brands_controller.rb create mode 100644 app/input_objects/organisations/brand_input.rb create mode 100644 app/models/organisation_brand.rb create mode 100644 app/views/organisation_brands/new.html.erb create mode 100644 config/locales/input_objects/organisation_brand.yml create mode 100644 db/migrate/20260714090000_create_organisation_brands.rb create mode 100644 spec/factories/models/organisation_brands.rb create mode 100644 spec/models/organisation_brand_spec.rb create mode 100644 spec/requests/organisation_brands_controller_spec.rb diff --git a/app/controllers/organisation_brands_controller.rb b/app/controllers/organisation_brands_controller.rb new file mode 100644 index 000000000..22cc05035 --- /dev/null +++ b/app/controllers/organisation_brands_controller.rb @@ -0,0 +1,51 @@ +class OrganisationBrandsController < WebController + after_action :verify_authorized + + def new + authorize organisation, :can_manage_organisation_brands? + + @brand_input = Organisations::BrandInput.new(organisation:) + end + + def create + authorize organisation, :can_manage_organisation_brands? + + @brand_input = Organisations::BrandInput.new(brand_input_params) + + if @brand_input.submit + redirect_to organisation_path(organisation), success: t(".success", brand_name: @brand_input.brand.name) + else + render :new, status: :unprocessable_content + end + end + + def destroy + authorize organisation, :can_manage_organisation_brands? + + organisation_brand = organisation.organisation_brands.find_by!(brand_id: params[:id]) + organisation_brand.destroy! + + redirect_to organisation_path(organisation), success: t(".success", brand_name: organisation_brand.brand.name) + end + +private + + def organisation + @organisation ||= Organisation.find(params[:organisation_id]) + end + + def brand_input_params + params.require(:organisations_brand_input).permit(:brand_id).merge(organisation:).tap do |p| + # We have to take steps to detect when the autocomplete component is + # empty. We use the value of rawAttribute, which is the text input when JS + # is enabled. When it's empty, the user has cleared it. + if p.key?(:brand_id) && brand_id_raw && brand_id_raw.empty? + p[:brand_id] = nil + end + end + end + + def brand_id_raw + params.dig(:organisations_brand_input, :brand_id_raw) + end +end diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index 19aa94ae5..167575f36 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -23,7 +23,7 @@ def index def show authorize Organisation, :can_view_organisations? - @organisation = Organisation.includes(:organisation_domains, mou_signatures: :user).find(params[:id]) + @organisation = Organisation.includes(:organisation_domains, :brands, mou_signatures: :user).find(params[:id]) end private diff --git a/app/input_objects/organisations/brand_input.rb b/app/input_objects/organisations/brand_input.rb new file mode 100644 index 000000000..a7fc5c554 --- /dev/null +++ b/app/input_objects/organisations/brand_input.rb @@ -0,0 +1,29 @@ +module Organisations + class BrandInput < BaseInput + attr_accessor :organisation, :brand_id + + validates :brand_id, presence: true + validate :brand_is_available, if: -> { brand_id.present? } + + def submit + return false if invalid? + + organisation.organisation_brands.create!(brand:) + true + end + + def brand + @brand ||= Brand.find_by(id: brand_id) + end + + def available_brands + Brand.where.not(id: organisation.brands.select(:id)).order(:name) + end + + private + + def brand_is_available + errors.add(:brand_id, :inclusion) if brand.nil? || organisation.brands.exists?(brand.id) + end + end +end diff --git a/app/models/brand.rb b/app/models/brand.rb index b73a032aa..e03751124 100644 --- a/app/models/brand.rb +++ b/app/models/brand.rb @@ -1,4 +1,6 @@ class Brand < ApplicationRecord + has_many :organisation_brands, dependent: :destroy + validates :slug, presence: true, uniqueness: true, format: { with: /\A[a-z0-9]+(?:-[a-z0-9]+)*\z/, allow_blank: true } validates :name, presence: true end diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 57e857604..0be2fb64a 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -8,6 +8,9 @@ class Organisation < ApplicationRecord has_many :mou_signatures has_many :organisation_domains, dependent: :destroy + has_many :organisation_brands, dependent: :destroy + has_many :brands, -> { order(:name) }, through: :organisation_brands + scope :not_closed, -> { where(closed: false) } scope :with_users, -> { joins(:users).distinct.order(:name) } diff --git a/app/models/organisation_brand.rb b/app/models/organisation_brand.rb new file mode 100644 index 000000000..c9652bb96 --- /dev/null +++ b/app/models/organisation_brand.rb @@ -0,0 +1,6 @@ +class OrganisationBrand < ApplicationRecord + belongs_to :organisation + belongs_to :brand + + validates :brand_id, uniqueness: { scope: :organisation_id } +end diff --git a/app/policies/organisation_policy.rb b/app/policies/organisation_policy.rb index 48ca57e1e..675a9fb1c 100644 --- a/app/policies/organisation_policy.rb +++ b/app/policies/organisation_policy.rb @@ -2,4 +2,8 @@ class OrganisationPolicy < ApplicationPolicy def can_view_organisations? user.super_admin? end + + def can_manage_organisation_brands? + user.super_admin? + end end diff --git a/app/views/organisation_brands/new.html.erb b/app/views/organisation_brands/new.html.erb new file mode 100644 index 000000000..580985050 --- /dev/null +++ b/app/views/organisation_brands/new.html.erb @@ -0,0 +1,31 @@ +<% set_page_title(title_with_error_prefix(t(".title"), @brand_input.errors.any?)) %> +<% content_for :back_link, govuk_back_link_to(organisation_path(@brand_input.organisation)) %> + +
+
+ <%= form_with(model: @brand_input, url: organisation_brands_path(@brand_input.organisation)) do |f| %> + <% if @brand_input.errors.any? %> + <%= f.govuk_error_summary %> + <% end %> + +

+ <%= @brand_input.organisation.name_with_abbreviation %> + - + <%= t(".title") %> +

+ + <%= render DfE::Autocomplete::View.new( + f, + attribute_name: :brand_id, + form_field: f.govuk_collection_select(:brand_id, @brand_input.available_brands, :id, :name, + class: ['govuk-!-width-three-quarters'], + options: { prompt: t('.prompt') }, + label: { text: t('.label'), size: 'm' }) + ) %> + + <%= f.govuk_submit t(".submit") %> + <% end %> +
+
+ +<%= init_autocomplete_script(show_all_values: false, raw_attribute: true, source: false) %> diff --git a/app/views/organisations/show.html.erb b/app/views/organisations/show.html.erb index ad3938560..f59057d5f 100644 --- a/app/views/organisations/show.html.erb +++ b/app/views/organisations/show.html.erb @@ -105,5 +105,34 @@ <% else %>

<%= t("organisations.show.domains.none") %>

<% end %> + +

<%= t("organisations.show.brands.heading") %>

+ <% if @organisation.brands.any? %> + <%= render ScrollingWrapperComponent::View.new(aria_label: t("organisations.show.brands.heading")) do %> + <%= govuk_table do |table| %> + <%= table.with_head do |head| + head.with_row do |row| + row.with_cell(header: true, text: t("organisations.show.brands.table_headings.name")) + row.with_cell(header: true, text: t("organisations.show.brands.table_headings.actions")) + end + end %> + + <%= table.with_body do |body| %> + <% @organisation.brands.each do |brand| %> + <%= body.with_row do |row| %> + <%= row.with_cell { govuk_link_to(brand.name, brand_path(brand)) } %> + <%= row.with_cell do %> + <%= govuk_button_to(t("organisations.show.brands.remove"), organisation_brand_path(@organisation, brand), method: :delete, secondary: true) %> + <% end %> + <% end %> + <% end %> + <% end %> + <% end %> + <% end %> + <% else %> +

<%= t("organisations.show.brands.none") %>

+ <% end %> + + <%= govuk_button_link_to t("organisations.show.brands.add"), new_organisation_brand_path(@organisation), secondary: true %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 745c769b7..f15315e05 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1491,6 +1491,16 @@ en: If you pasted the web address, check you copied the entire address. title: Page not found + organisation_brands: + create: + success: "%{brand_name} has been added to this organisation’s brands" + destroy: + success: "%{brand_name} has been removed from this organisation’s brands" + new: + label: Brand + prompt: Select a brand + submit: Add brand + title: Add a brand organisations: boolean: 'false': 'No' @@ -1531,6 +1541,14 @@ en: admin_users: heading: Organisation admins none: This organisation does not have any organisation admins. + brands: + add: Add a brand + heading: Brands + none: This organisation does not have any brands. Forms in this organisation use the default GOV.UK branding. + remove: Remove + table_headings: + actions: Actions + name: Name domains: heading: Email domains none: This organisation does not have any email domains. diff --git a/config/locales/input_objects/organisation_brand.yml b/config/locales/input_objects/organisation_brand.yml new file mode 100644 index 000000000..c7ccb08c0 --- /dev/null +++ b/config/locales/input_objects/organisation_brand.yml @@ -0,0 +1,10 @@ +--- +en: + activemodel: + errors: + models: + organisations/brand_input: + attributes: + brand_id: + blank: Select a brand + inclusion: Select a brand from the list of available brands diff --git a/config/routes.rb b/config/routes.rb index bc58cd57e..6cf6a3baf 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -217,7 +217,9 @@ resources :mou_signatures, only: %i[index], path: "mous" - resources :organisations, only: %i[index show] + resources :organisations, only: %i[index show] do + resources :brands, controller: :organisation_brands, only: %i[new create destroy] + end resources :brands, only: %i[index show new create edit update] diff --git a/db/migrate/20260714090000_create_organisation_brands.rb b/db/migrate/20260714090000_create_organisation_brands.rb new file mode 100644 index 000000000..4b382bcac --- /dev/null +++ b/db/migrate/20260714090000_create_organisation_brands.rb @@ -0,0 +1,11 @@ +class CreateOrganisationBrands < ActiveRecord::Migration[8.1] + def change + create_table :organisation_brands do |t| + t.references :organisation, null: false, foreign_key: true + t.references :brand, null: false, foreign_key: true + t.timestamps + end + + add_index :organisation_brands, %i[organisation_id brand_id], unique: true, name: "index_organisation_brands_unique" + end +end diff --git a/db/schema.rb b/db/schema.rb index a419bb51d..bbea686ba 100644 --- a/db/schema.rb +++ b/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_07_10_080000) do +ActiveRecord::Schema[8.1].define(version: 2026_07_14_090000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -231,6 +231,16 @@ t.index ["user_id"], name: "index_mou_signatures_on_user_id_unique_without_organisation_id", unique: true, where: "(organisation_id IS NULL)", comment: "Users can only sign a single MOU without an organisation" end + create_table "organisation_brands", force: :cascade do |t| + t.bigint "brand_id", null: false + t.datetime "created_at", null: false + t.bigint "organisation_id", null: false + t.datetime "updated_at", null: false + t.index ["brand_id"], name: "index_organisation_brands_on_brand_id" + t.index ["organisation_id", "brand_id"], name: "index_organisation_brands_unique", unique: true + t.index ["organisation_id"], name: "index_organisation_brands_on_organisation_id" + end + create_table "organisation_domains", force: :cascade do |t| t.datetime "created_at", null: false t.string "domain", null: false @@ -342,6 +352,8 @@ add_foreign_key "memberships", "users", column: "added_by_id" add_foreign_key "mou_signatures", "organisations" add_foreign_key "mou_signatures", "users" + add_foreign_key "organisation_brands", "brands" + add_foreign_key "organisation_brands", "organisations" add_foreign_key "organisation_domains", "organisations" add_foreign_key "page_translations", "pages" add_foreign_key "pages", "forms" diff --git a/db/seeds.rb b/db/seeds.rb index 4a06fb129..6fad255d9 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -21,6 +21,7 @@ ) gds.organisation_domains.create! domain: "digital.cabinet-office.gov.uk" gds.organisation_domains.create! domain: "dsit.gov.uk" + gds.brands = Brand.all # Create default super-admin default_user = User.create!({ email: "example@example.com", diff --git a/spec/factories/models/organisation_brands.rb b/spec/factories/models/organisation_brands.rb new file mode 100644 index 000000000..0b1b392d7 --- /dev/null +++ b/spec/factories/models/organisation_brands.rb @@ -0,0 +1,6 @@ +FactoryBot.define do + factory :organisation_brand do + organisation + brand + end +end diff --git a/spec/models/organisation_brand_spec.rb b/spec/models/organisation_brand_spec.rb new file mode 100644 index 000000000..4c40ee790 --- /dev/null +++ b/spec/models/organisation_brand_spec.rb @@ -0,0 +1,32 @@ +require "rails_helper" + +RSpec.describe OrganisationBrand do + describe "validations" do + it "has a valid factory" do + expect(build(:organisation_brand)).to be_valid + end + + it "is invalid without an organisation" do + organisation_brand = described_class.new(brand: create(:brand)) + expect(organisation_brand).not_to be_valid + end + + it "is invalid without a brand" do + organisation_brand = described_class.new(organisation: create(:organisation)) + expect(organisation_brand).not_to be_valid + end + + it "is invalid when the brand is already added to the organisation" do + organisation_brand = create(:organisation_brand) + duplicate = described_class.new(organisation: organisation_brand.organisation, brand: organisation_brand.brand) + expect(duplicate).not_to be_valid + end + + it "is valid when the brand is added to a different organisation" do + organisation_brand = create(:organisation_brand) + other_organisation = create(:organisation, slug: "other-org") + other_organisation_brand = build(:organisation_brand, organisation: other_organisation, brand: organisation_brand.brand) + expect(other_organisation_brand).to be_valid + end + end +end diff --git a/spec/requests/organisation_brands_controller_spec.rb b/spec/requests/organisation_brands_controller_spec.rb new file mode 100644 index 000000000..96f17af8b --- /dev/null +++ b/spec/requests/organisation_brands_controller_spec.rb @@ -0,0 +1,197 @@ +require "rails_helper" + +RSpec.describe OrganisationBrandsController, type: :request do + let(:organisation) { create :organisation, slug: "department-for-testing" } + let(:brand) { create :brand, slug: "cheshire-east", name: "Cheshire East Council" } + + describe "#new" do + let(:path) { new_organisation_brand_path(organisation) } + + context "when the user is not a super admin" do + before do + login_as_standard_user + + get path + end + + it "returns http code 403 and renders forbidden" do + expect(response).to have_http_status(:forbidden) + expect(response).to render_template("errors/forbidden") + end + end + + context "when the user is a super admin" do + let!(:added_brand) { create :brand, name: "Already Added Council" } + let!(:available_brand) { create :brand, name: "Available Council" } + + before do + create :organisation_brand, organisation:, brand: added_brand + + login_as_super_admin_user + + get path + end + + it "returns http code 200 and renders the new view" do + expect(response).to have_http_status(:ok) + expect(response).to render_template("organisation_brands/new") + end + + it "includes brands that have not been added to the organisation" do + expect(response.body).to include(available_brand.name) + end + + it "does not include brands that have already been added to the organisation" do + expect(response.body).not_to include(added_brand.name) + end + end + end + + describe "#create" do + let(:path) { organisation_brands_path(organisation) } + let(:params) { { organisations_brand_input: { brand_id: brand.id } } } + + context "when the user is not a super admin" do + before do + login_as_standard_user + + post(path, params:) + end + + it "returns http code 403 and renders forbidden" do + expect(response).to have_http_status(:forbidden) + expect(response).to render_template("errors/forbidden") + end + + it "does not add the brand to the organisation" do + expect(organisation.brands).to be_empty + end + end + + context "when the user is a super admin" do + before do + login_as_super_admin_user + end + + it "adds the brand to the organisation" do + expect { + post(path, params:) + }.to change { organisation.brands.count }.by(1) + end + + it "redirects to the organisation page with a success message" do + post(path, params:) + + expect(response).to redirect_to(organisation_path(organisation)) + expect(flash[:success]).to eq(I18n.t("organisation_brands.create.success", brand_name: brand.name)) + end + + context "when no brand is selected" do + let(:params) { { organisations_brand_input: { brand_id: "" } } } + + it "re-renders the page with an error" do + post(path, params:) + + expect(response).to have_http_status(:unprocessable_content) + expect(response.body).to include(I18n.t("activemodel.errors.models.organisations/brand_input.attributes.brand_id.blank")) + end + end + + context "when the brand does not exist" do + let(:params) { { organisations_brand_input: { brand_id: "999999" } } } + + it "re-renders the page with an error" do + post(path, params:) + + expect(response).to have_http_status(:unprocessable_content) + expect(response.body).to include(I18n.t("activemodel.errors.models.organisations/brand_input.attributes.brand_id.inclusion")) + end + end + + context "when the brand has already been added to the organisation" do + before do + create :organisation_brand, organisation:, brand: + end + + it "re-renders the page with an error" do + expect { + post(path, params:) + }.not_to(change { organisation.brands.count }) + + expect(response).to have_http_status(:unprocessable_content) + expect(response.body).to include(I18n.t("activemodel.errors.models.organisations/brand_input.attributes.brand_id.inclusion")) + end + end + + context "when the autocomplete text input has been cleared" do + let(:params) { { organisations_brand_input: { brand_id: brand.id, brand_id_raw: "" } } } + + it "ignores the stale select value and re-renders the page with an error" do + expect { + post(path, params:) + }.not_to(change { organisation.brands.count }) + + expect(response).to have_http_status(:unprocessable_content) + expect(response.body).to include(I18n.t("activemodel.errors.models.organisations/brand_input.attributes.brand_id.blank")) + end + end + end + end + + describe "#destroy" do + let(:path) { organisation_brand_path(organisation, brand) } + + before do + create :organisation_brand, organisation:, brand: + end + + context "when the user is not a super admin" do + before do + login_as_standard_user + + delete path + end + + it "returns http code 403 and renders forbidden" do + expect(response).to have_http_status(:forbidden) + expect(response).to render_template("errors/forbidden") + end + + it "does not remove the brand from the organisation" do + expect(organisation.brands).to include(brand) + end + end + + context "when the user is a super admin" do + before do + login_as_super_admin_user + end + + it "removes the brand from the organisation without deleting the brand" do + expect { + delete path + }.to change { organisation.brands.count }.by(-1) + + expect(Brand.exists?(brand.id)).to be true + end + + it "redirects to the organisation page with a success message" do + delete path + + expect(response).to redirect_to(organisation_path(organisation)) + expect(flash[:success]).to eq(I18n.t("organisation_brands.destroy.success", brand_name: brand.name)) + end + + context "when the brand is not one of the organisation's brands" do + let(:other_brand) { create :brand } + let(:path) { organisation_brand_path(organisation, other_brand) } + + it "returns http code 404" do + delete path + + expect(response).to have_http_status(:not_found) + end + end + end + end +end diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 8af46f3e4..c0711ce5f 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -166,6 +166,7 @@ def organisation_row_order(response) let(:path) { organisation_path(organisation) } let!(:organisation_domain) { create :organisation_domain, organisation: } + let!(:organisation_brand) { create :organisation_brand, organisation: } include_examples "unauthorized user is forbidden" @@ -188,6 +189,12 @@ def organisation_row_order(response) expect(response.body).to include(I18n.t("mou_signatures.index.agreement_type.#{organisation.mou_signatures.first.agreement_type}")) expect(response.body).to include(organisation_domain.domain) end + + it "shows the organisation's brands with a link to add a brand" do + expect(response.body).to include(organisation_brand.brand.name) + expect(response.body).to include(I18n.t("organisations.show.brands.add")) + expect(response.body).to include(new_organisation_brand_path(organisation)) + end end end end From d7856a1af5fa5a3c8bcd16c14fc6239b8a9297c1 Mon Sep 17 00:00:00 2001 From: Sean Rankine Date: Tue, 14 Jul 2026 13:15:48 +0100 Subject: [PATCH 2/7] Offer only organisation's brands on brand task The form brand picker offered every brand configured in settings.yml to any form. Source the options from the brands allowed for the form's organisation instead, so organisations can only apply brands they have been granted. Options keep using the brand slug, so existing form brand_id values are unaffected. --- app/input_objects/forms/brand_input.rb | 19 +++---- config/settings.yml | 9 ---- spec/factories/models/forms.rb | 2 +- spec/input_objects/forms/brand_input_spec.rb | 57 +++++++++++++++++--- spec/requests/forms/brand_controller_spec.rb | 5 ++ 5 files changed, 67 insertions(+), 25 deletions(-) diff --git a/app/input_objects/forms/brand_input.rb b/app/input_objects/forms/brand_input.rb index 2a56bec7e..dc557df8a 100644 --- a/app/input_objects/forms/brand_input.rb +++ b/app/input_objects/forms/brand_input.rb @@ -3,21 +3,22 @@ class Forms::BrandInput < BaseInput attr_accessor :form, :brand_id - validates :brand_id, inclusion: { in: ->(_input) { Forms::BrandInput.allowed_brand_ids } }, allow_blank: true + validates :brand_id, inclusion: { in: ->(input) { input.allowed_brand_ids } }, allow_blank: true - class << self - def brands - Settings.branding.available_brands - end + def brands + organisation = form.group&.organisation + return Brand.none if organisation.nil? - def allowed_brand_ids - brands.map(&:id) - end + organisation.brands + end + + def allowed_brand_ids + brands.map(&:slug) end def brand_options default_option = BrandOption.new("", I18n.t("helpers.label.forms_brand_input.brand_id.options.default")) - [default_option] + self.class.brands.map { |brand| BrandOption.new(brand.id, brand.name) } + [default_option] + brands.map { |brand| BrandOption.new(brand.slug, brand.name) } end def submit diff --git a/config/settings.yml b/config/settings.yml index 832b978cc..64913349e 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -9,15 +9,6 @@ features: send_filler_answers: enabled_by_group: true -# Brands that a form can be styled with in forms-runner. A form with no -# brand_id uses the default GOV.UK branding. -branding: - available_brands: - - id: cheshire-east - name: Cheshire East Council - - id: south-gloucestershire - name: South Gloucestershire Council - forms_api: # Authentication key to authenticate with forms-api auth_key: development_key diff --git a/spec/factories/models/forms.rb b/spec/factories/models/forms.rb index 1c467ea0c..fb0445f86 100644 --- a/spec/factories/models/forms.rb +++ b/spec/factories/models/forms.rb @@ -24,7 +24,7 @@ brand_id { nil } trait :with_brand do - brand_id { Settings.branding.available_brands.first.id } + brand_id { "test-brand" } end trait :with_group do diff --git a/spec/input_objects/forms/brand_input_spec.rb b/spec/input_objects/forms/brand_input_spec.rb index 0f0ce21b8..69cdf6e92 100644 --- a/spec/input_objects/forms/brand_input_spec.rb +++ b/spec/input_objects/forms/brand_input_spec.rb @@ -1,8 +1,18 @@ require "rails_helper" RSpec.describe Forms::BrandInput, type: :model do + let(:organisation) { create(:organisation) } + let(:group) { create(:group, organisation:) } let(:form) do - create(:form, :live) + create(:form, :live, :with_group, group:) + end + + let(:cheshire_east) { create(:brand, slug: "cheshire-east", name: "Cheshire East Council") } + let(:south_gloucestershire) { create(:brand, slug: "south-gloucestershire", name: "South Gloucestershire Council") } + + before do + create(:organisation_brand, organisation:, brand: cheshire_east) + create(:organisation_brand, organisation:, brand: south_gloucestershire) end describe "validations" do @@ -14,15 +24,29 @@ end end - context "when given a brand_id from the configured list" do + context "when given a brand_id from the organisation's brands" do it "validates successfully" do - brand_input = described_class.new(form:, brand_id: Settings.branding.available_brands.first.id) + brand_input = described_class.new(form:, brand_id: "cheshire-east") expect(brand_input).to be_valid end end - context "when given a brand_id that is not in the configured list" do + context "when given a brand_id that is not one of the organisation's brands" do + it "returns a validation error" do + create(:brand, slug: "not-allowed-for-organisation") + + brand_input = described_class.new(form:, brand_id: "not-allowed-for-organisation") + + brand_input.validate(:brand_id) + + expect(brand_input.errors.full_messages_for(:brand_id)).to include( + "Brand Select a brand", + ) + end + end + + context "when given a brand_id that does not exist" do it "returns a validation error" do brand_input = described_class.new(form:, brand_id: "not-a-brand") @@ -36,11 +60,32 @@ end describe "#brand_options" do - it "starts with the GOV.UK default option followed by the configured brands" do + it "starts with the GOV.UK default option followed by the organisation's brands" do brand_input = described_class.new(form:) expect(brand_input.brand_options.first).to have_attributes(id: "", name: "GOV.UK (default)") - expect(brand_input.brand_options.drop(1).map(&:id)).to eq(Settings.branding.available_brands.map(&:id)) + expect(brand_input.brand_options.drop(1).map(&:id)).to eq %w[cheshire-east south-gloucestershire] + end + + context "when the organisation has no brands" do + let(:other_organisation) { create(:organisation, slug: "other-org") } + let(:group) { create(:group, organisation: other_organisation) } + + it "only includes the GOV.UK default option" do + brand_input = described_class.new(form:) + + expect(brand_input.brand_options.map(&:id)).to eq [""] + end + end + + context "when the form is not in a group" do + let(:form) { create(:form, :live) } + + it "only includes the GOV.UK default option" do + brand_input = described_class.new(form:) + + expect(brand_input.brand_options.map(&:id)).to eq [""] + end end end diff --git a/spec/requests/forms/brand_controller_spec.rb b/spec/requests/forms/brand_controller_spec.rb index 796e910ee..888222b32 100644 --- a/spec/requests/forms/brand_controller_spec.rb +++ b/spec/requests/forms/brand_controller_spec.rb @@ -8,6 +8,11 @@ let(:custom_branding_enabled) { true } before do + cheshire_east = create(:brand, slug: "cheshire-east", name: "Cheshire East Council") + south_gloucestershire = create(:brand, slug: "south-gloucestershire", name: "South Gloucestershire Council") + create(:organisation_brand, organisation: standard_user.organisation, brand: cheshire_east) + create(:organisation_brand, organisation: standard_user.organisation, brand: south_gloucestershire) + Membership.create!(group_id: group.id, user: standard_user, added_by: standard_user) GroupForm.create!(form_id: form.id, group_id: group.id) From e702710fa9ac6b17943ec8a414675cd9d108b9e6 Mon Sep 17 00:00:00 2001 From: Sean Rankine Date: Tue, 14 Jul 2026 15:28:12 +0100 Subject: [PATCH 3/7] Explain brand removal does not affect forms Removing a brand from an organisation stops new forms selecting it, but forms already using the brand keep it. Make this clear on the organisation page so super admins know what removing a brand does. --- app/views/organisations/show.html.erb | 1 + config/locales/en.yml | 1 + spec/requests/organisations_controller_spec.rb | 1 + 3 files changed, 3 insertions(+) diff --git a/app/views/organisations/show.html.erb b/app/views/organisations/show.html.erb index f59057d5f..c6ba109c4 100644 --- a/app/views/organisations/show.html.erb +++ b/app/views/organisations/show.html.erb @@ -108,6 +108,7 @@

<%= t("organisations.show.brands.heading") %>

<% if @organisation.brands.any? %> +

<%= t("organisations.show.brands.guidance") %>

<%= render ScrollingWrapperComponent::View.new(aria_label: t("organisations.show.brands.heading")) do %> <%= govuk_table do |table| %> <%= table.with_head do |head| diff --git a/config/locales/en.yml b/config/locales/en.yml index f15315e05..b1ab3afe7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1543,6 +1543,7 @@ en: none: This organisation does not have any organisation admins. brands: add: Add a brand + guidance: Forms that already use a brand will continue to use it, even if the brand is removed from this organisation. heading: Brands none: This organisation does not have any brands. Forms in this organisation use the default GOV.UK branding. remove: Remove diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index c0711ce5f..ff1878f40 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -192,6 +192,7 @@ def organisation_row_order(response) it "shows the organisation's brands with a link to add a brand" do expect(response.body).to include(organisation_brand.brand.name) + expect(response.body).to include(I18n.t("organisations.show.brands.guidance")) expect(response.body).to include(I18n.t("organisations.show.brands.add")) expect(response.body).to include(new_organisation_brand_path(organisation)) end From c6267c9b1b84d328a37aa0f39b7cd7fdc4aa4203 Mon Sep 17 00:00:00 2001 From: Sean Rankine Date: Tue, 14 Jul 2026 15:30:00 +0100 Subject: [PATCH 4/7] Extract helper for clearing empty autocompletes The same empty-rawAttribute detection was copied in three places across UsersController and OrganisationBrandsController. Move it to WebController so every autocomplete form clears stale values the same way. --- .../organisation_brands_controller.rb | 11 +------- app/controllers/users_controller.rb | 25 +++---------------- app/controllers/web_controller.rb | 10 ++++++++ 3 files changed, 15 insertions(+), 31 deletions(-) diff --git a/app/controllers/organisation_brands_controller.rb b/app/controllers/organisation_brands_controller.rb index 22cc05035..fc3b90348 100644 --- a/app/controllers/organisation_brands_controller.rb +++ b/app/controllers/organisation_brands_controller.rb @@ -36,16 +36,7 @@ def organisation def brand_input_params params.require(:organisations_brand_input).permit(:brand_id).merge(organisation:).tap do |p| - # We have to take steps to detect when the autocomplete component is - # empty. We use the value of rawAttribute, which is the text input when JS - # is enabled. When it's empty, the user has cleared it. - if p.key?(:brand_id) && brand_id_raw && brand_id_raw.empty? - p[:brand_id] = nil - end + clear_param_if_autocomplete_empty(p, :brand_id, params.dig(:organisations_brand_input, :brand_id_raw)) end end - - def brand_id_raw - params.dig(:organisations_brand_input, :brand_id_raw) - end end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index ff4f415b1..18a5d43d9 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -54,14 +54,7 @@ def filtered_users def user_params params.require(:user).permit(:has_access, :name, :role, :organisation_id).tap do |p| - # We have to take steps to detect when the autocomplete component is - # empty. We use the value of rawAttribute, which is the text input when JS - # is enabled. When it's empty, the user has cleared it. This isn't needed - # when the no JS select is used but we have to allow organisation_id to be - # nil still. - if p.key?(:organisation_id) && organisation_id_raw && organisation_id_raw.empty? - p[:organisation_id] = nil - end + clear_param_if_autocomplete_empty(p, :organisation_id, params.dig(:user, :organisation_id_raw)) end end @@ -69,19 +62,9 @@ def user @user ||= User.find(params[:id]) end - def organisation_id_raw - params.dig(:user, :organisation_id_raw) - end - def filter_params - filters = params[:filter]&.permit(:name, :email, :organisation_id, :role, :has_access) || {} - - # if the text in the organisation input has been cleared, don't use the last selected organisation_id - organisation_id_raw = params.dig(:filter, :organisation_id_raw) - if filters.key?(:organisation_id) && organisation_id_raw && organisation_id_raw.empty? - filters[:organisation_id] = nil - end - - filters + params[:filter]&.permit(:name, :email, :organisation_id, :role, :has_access)&.tap do |filters| + clear_param_if_autocomplete_empty(filters, :organisation_id, params.dig(:filter, :organisation_id_raw)) + end || {} end end diff --git a/app/controllers/web_controller.rb b/app/controllers/web_controller.rb index 960e1dbe9..f7bad77b6 100644 --- a/app/controllers/web_controller.rb +++ b/app/controllers/web_controller.rb @@ -74,6 +74,16 @@ def authenticate_user! private + # We have to take steps to detect when the autocomplete component is empty. + # We use the value of rawAttribute, which is the text input when JS is + # enabled. When it's empty, the user has cleared it, so any previously + # selected value is stale and should be discarded. This isn't needed when + # the no JS select is used but we have to allow the attribute to be nil + # still. + def clear_param_if_autocomplete_empty(permitted_params, attribute, raw_value) + permitted_params[attribute] = nil if permitted_params.key?(attribute) && raw_value && raw_value.empty? + end + def authenticate_and_check_access authenticate_user! From 94b011e2749df68ed9e6808b5a723e0e546d0d82 Mon Sep 17 00:00:00 2001 From: Sean Rankine Date: Tue, 14 Jul 2026 15:31:18 +0100 Subject: [PATCH 5/7] Define available brands once in brand input The custom validation re-implemented the "not already added" rule that available_brands already expresses, so the two could drift. Validate against available_brands directly instead. --- app/input_objects/organisations/brand_input.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/input_objects/organisations/brand_input.rb b/app/input_objects/organisations/brand_input.rb index a7fc5c554..235cf99f6 100644 --- a/app/input_objects/organisations/brand_input.rb +++ b/app/input_objects/organisations/brand_input.rb @@ -23,7 +23,7 @@ def available_brands private def brand_is_available - errors.add(:brand_id, :inclusion) if brand.nil? || organisation.brands.exists?(brand.id) + errors.add(:brand_id, :inclusion) unless available_brands.exists?(id: brand_id) end end end From 716e54f6ea7dbbb349082a488afbf6a4cfdfbac9 Mon Sep 17 00:00:00 2001 From: Sean Rankine Date: Tue, 14 Jul 2026 15:33:36 +0100 Subject: [PATCH 6/7] Avoid loading brand records for validation Validating a brand id only needs slugs, so pluck them rather than instantiating every Brand. Memoise the brands relation as it is used by both validation and the options list. --- app/input_objects/forms/brand_input.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/input_objects/forms/brand_input.rb b/app/input_objects/forms/brand_input.rb index dc557df8a..be6540c9c 100644 --- a/app/input_objects/forms/brand_input.rb +++ b/app/input_objects/forms/brand_input.rb @@ -6,14 +6,14 @@ class Forms::BrandInput < BaseInput validates :brand_id, inclusion: { in: ->(input) { input.allowed_brand_ids } }, allow_blank: true def brands - organisation = form.group&.organisation - return Brand.none if organisation.nil? - - organisation.brands + @brands ||= begin + organisation = form.group&.organisation + organisation.nil? ? Brand.none : organisation.brands + end end def allowed_brand_ids - brands.map(&:slug) + brands.pluck(:slug) end def brand_options From ea669ad79a905caf7daf98c30f6810eaa4a1bab9 Mon Sep 17 00:00:00 2001 From: Sean Rankine Date: Tue, 14 Jul 2026 16:52:33 +0100 Subject: [PATCH 7/7] Clarify available brands on organisation page Show the guidance even when an organisation has no brands, and reword it to explain that adding a brand makes it available for the organisation's forms to use. --- app/views/organisations/show.html.erb | 2 +- config/locales/en.yml | 6 +++--- spec/views/organisations/show.html.erb_spec.rb | 5 +++++ 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/views/organisations/show.html.erb b/app/views/organisations/show.html.erb index c6ba109c4..ef75385d2 100644 --- a/app/views/organisations/show.html.erb +++ b/app/views/organisations/show.html.erb @@ -107,8 +107,8 @@ <% end %>

<%= t("organisations.show.brands.heading") %>

+

<%= t("organisations.show.brands.guidance") %>

<% if @organisation.brands.any? %> -

<%= t("organisations.show.brands.guidance") %>

<%= render ScrollingWrapperComponent::View.new(aria_label: t("organisations.show.brands.heading")) do %> <%= govuk_table do |table| %> <%= table.with_head do |head| diff --git a/config/locales/en.yml b/config/locales/en.yml index b1ab3afe7..f1c676704 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1543,9 +1543,9 @@ en: none: This organisation does not have any organisation admins. brands: add: Add a brand - guidance: Forms that already use a brand will continue to use it, even if the brand is removed from this organisation. - heading: Brands - none: This organisation does not have any brands. Forms in this organisation use the default GOV.UK branding. + guidance: Add brands to make them available for forms in this organisation to use. Forms that already use a brand will continue to use it, even if you remove the brand from this organisation. + heading: Available brands + none: This organisation does not have any available brands. Forms use the default GOV.UK branding. remove: Remove table_headings: actions: Actions diff --git a/spec/views/organisations/show.html.erb_spec.rb b/spec/views/organisations/show.html.erb_spec.rb index b89d16136..3773a3cd3 100644 --- a/spec/views/organisations/show.html.erb_spec.rb +++ b/spec/views/organisations/show.html.erb_spec.rb @@ -64,6 +64,11 @@ expect(rendered).to have_text(I18n.t("organisations.show.admin_users.none")) expect(rendered).to have_text(I18n.t("organisations.show.mou_signatures.none")) expect(rendered).to have_text(I18n.t("organisations.show.domains.none")) + expect(rendered).to have_text(I18n.t("organisations.show.brands.none")) + end + + it "shows brands guidance when there are no brands" do + expect(rendered).to have_text(I18n.t("organisations.show.brands.guidance")) end it "shows the MOU guidance with links to the agreements" do