Merge pull request #209 from alphagov/2449-add-source-filter-to-conversation

davidgisbey · web-flow · commit edf8ab1458e5 · 2025-05-07T12:01:37.000+01:00
Add ability to filter questions in Admin UI by source
diff --git a/app/controllers/admin/questions_controller.rb b/app/controllers/admin/questions_controller.rb
@@ -20,6 +20,7 @@ def filter_params
     params.permit(
       :search,
       :status,
+      :source,
       { start_date_params: %i[day month year], end_date_params: %i[day month year] },
       :answer_feedback_useful,
       :question_routing_label,
diff --git a/app/controllers/api/v0/conversations_controller.rb b/app/controllers/api/v0/conversations_controller.rb
@@ -4,7 +4,7 @@ class Api::V0::ConversationsController < ApplicationController
   before_action :find_conversation, only: %i[show update answer answer_feedback]
 
   def create
-    conversation = Conversation.new(signon_user: current_user)
+    conversation = Conversation.new(signon_user: current_user, source: :api)
     form = Form::CreateQuestion.new(question_params.merge(conversation:))
 
     if form.valid?
@@ -70,7 +70,7 @@ def answer_feedback
   def find_conversation
     @conversation = Conversation
                     .includes(questions: { answer: %i[sources feedback] })
-                    .where(signon_user_id: current_user.id)
+                    .where(signon_user_id: current_user.id, source: :api)
                     .find(params[:conversation_id])
   end
 
diff --git a/app/controllers/conversations_controller.rb b/app/controllers/conversations_controller.rb
@@ -111,7 +111,9 @@ def user_question_params
   def find_conversation
     return if cookies[:conversation_id].blank?
 
-    @conversation = Conversation.includes(:user).active.find_by!(id: cookies[:conversation_id], user: current_early_access_user)
+    @conversation = Conversation.includes(:user)
+                                .active
+                                .find_by!(id: cookies[:conversation_id], user: current_early_access_user, source: :web)
     set_conversation_cookie(@conversation)
   rescue ActiveRecord::RecordNotFound
     cookies.delete(:conversation_id)
diff --git a/app/helpers/admin/questions_helper.rb b/app/helpers/admin/questions_helper.rb
@@ -85,7 +85,7 @@ def question_show_summary_list_rows(question, answer, question_number, total_que
       end
 
       signon_user = conversation.signon_user
-      if signon_user.present?
+      if signon_user.present? && conversation.source_api?
         rows << {
           field: "API user",
           value: safe_join([
@@ -97,6 +97,11 @@ def question_show_summary_list_rows(question, answer, question_number, total_que
         }
       end
 
+      rows << {
+        field: "Source",
+        value: conversation.source_api? ? "API" : conversation.source.humanize,
+      }
+
       rows << if answer.present?
                 [
                   {
diff --git a/app/models/admin/filters/questions_filter.rb b/app/models/admin/filters/questions_filter.rb
@@ -1,6 +1,7 @@
 class Admin::Filters::QuestionsFilter < Admin::Filters::BaseFilter
   attribute :status
   attribute :search
+  attribute :source
   attribute :start_date_params, default: {}
   attribute :end_date_params, default: {}
   attribute :conversation_id
@@ -30,6 +31,7 @@ def results
                       .left_outer_joins(:answer)
       scope = search_scope(scope)
       scope = status_scope(scope)
+      scope = source_scope(scope)
       scope = start_date_scope(scope)
       scope = end_date_scope(scope)
       scope = answer_feedback_useful_scope(scope)
@@ -93,6 +95,12 @@ def status_scope(scope)
     end
   end
 
+  def source_scope(scope)
+    return scope if source.blank?
+
+    scope.joins(:conversation).where(conversations: { source: })
+  end
+
   def start_date_scope(scope)
     return scope if errors[:start_date_params].present? || start_date.nil?
 
@@ -126,7 +134,7 @@ def user_scope(scope)
   def signon_user_scope(scope)
     return scope if signon_user_id.blank? || user_id.present?
 
-    scope.joins(:conversation).where(conversation: { signon_user_id: signon_user_id })
+    scope.joins(:conversation).where(conversation: { signon_user_id: signon_user_id, source: :api })
   end
 
   def question_routing_label_scope(scope)
diff --git a/app/models/conversation.rb b/app/models/conversation.rb
@@ -6,6 +6,13 @@ class Conversation < ApplicationRecord
 
   scope :active, -> { where(Question.active.where("questions.conversation_id = conversations.id").arel.exists) }
 
+  enum :source,
+       {
+         api: "api",
+         web: "web",
+       },
+       prefix: true
+
   def questions_for_showing_conversation
     Question.where(conversation: self)
             .includes(answer: %i[feedback sources])
diff --git a/app/views/admin/questions/_question_filters.html.erb b/app/views/admin/questions/_question_filters.html.erb
@@ -74,6 +74,30 @@
     end,
   } %>
 
+  <%= render "govuk_publishing_components/components/select", {
+    id: "source",
+    name: "source",
+    label: "Source",
+    heading_size: "s",
+    full_width: true,
+    options: [
+      {
+        text: "",
+        value: "",
+      },
+      {
+        text: "Web",
+        value: "web",
+        selected: params[:source] == "web",
+      },
+      {
+        text: "API",
+        value: "api",
+        selected: params[:source] == "api",
+      },
+    ],
+  } %>
+
   <%= render "govuk_publishing_components/components/fieldset", {
     legend_text: "Start date",
     heading_size: "s",
diff --git a/db/migrate/20250506133220_add_source_to_conversations.rb b/db/migrate/20250506133220_add_source_to_conversations.rb
@@ -0,0 +1,6 @@
+class AddSourceToConversations < ActiveRecord::Migration[8.0]
+  def change
+    create_enum "conversation_source", %w[web api]
+    add_column :conversations, :source, :conversation_source, null: false, default: "web"
+  end
+end
diff --git 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.0].define(version: 2025_05_01_150559) do
+ActiveRecord::Schema[8.0].define(version: 2025_05_06_133220) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
   enable_extension "pg_catalog.plpgsql"
@@ -19,6 +19,7 @@
   # Custom types defined in this database.
   # Note that some types may not work with other database engines. Be careful if changing database.
   create_enum "answer_status", ["answered", "banned", "clarification", "error_answer_guardrails", "error_answer_service_error", "error_context_length_exceeded", "error_jailbreak_guardrails", "error_non_specific", "error_question_routing_guardrails", "error_timeout", "guardrails_answer", "guardrails_forbidden_terms", "guardrails_jailbreak", "guardrails_question_routing", "unanswerable_llm_cannot_answer", "unanswerable_no_govuk_content", "unanswerable_question_routing"]
+  create_enum "conversation_source", ["web", "api"]
   create_enum "deleted_early_access_user_deletion_type", ["unsubscribe", "admin"]
   create_enum "deleted_waiting_list_user_deletion_type", ["unsubscribe", "admin", "promotion"]
   create_enum "early_access_user_source", ["admin_added", "instant_signup", "admin_promoted", "delayed_signup"]
@@ -97,6 +98,7 @@
     t.datetime "updated_at", null: false
     t.uuid "early_access_user_id"
     t.uuid "signon_user_id"
+    t.enum "source", default: "web", null: false, enum_type: "conversation_source"
     t.index ["created_at"], name: "index_conversations_on_created_at"
     t.index ["early_access_user_id"], name: "index_conversations_on_early_access_user_id"
     t.index ["signon_user_id"], name: "index_conversations_on_signon_user_id"
diff --git a/spec/factories/conversation_factory.rb b/spec/factories/conversation_factory.rb
@@ -34,5 +34,9 @@
         create(:question, :with_answer, conversation:)
       end
     end
+
+    trait :api do
+      source { :api }
+    end
   end
 end
diff --git a/spec/helpers/admin/questions_helper_spec.rb b/spec/helpers/admin/questions_helper_spec.rb
@@ -54,6 +54,7 @@
         "Question created at",
         "Question",
         "Show search results",
+        "Source",
         "Status",
       ]
 
@@ -72,6 +73,7 @@
         "Question",
         "Show search results",
         "Early access user",
+        "Source",
         "Rephrased question",
         "Status",
         "Answer created at",
@@ -176,7 +178,7 @@
 
     it "returns a row with a link to filter the questions table by the signon user" do
       signon_user = create(:signon_user)
-      conversation.update!(signon_user:)
+      conversation.update!(signon_user:, source: :api)
       result = helper.question_show_summary_list_rows(question, nil, 1, 1)
 
       row = result.find { |r| r[:field] == "API user" }
@@ -187,6 +189,27 @@
           href: admin_questions_path(signon_user_id: conversation.signon_user.id),
         )
     end
+
+    it "returns a row with the source of the conversation" do
+      result = helper.question_show_summary_list_rows(question, nil, 1, 1)
+
+      row = result.find { |r| r[:field] == "Source" }
+      expect(row[:value]).to eq("Web")
+
+      conversation.update!(source: :api)
+      result = helper.question_show_summary_list_rows(question, nil, 1, 1)
+
+      row = result.find { |r| r[:field] == "Source" }
+      expect(row[:value]).to eq("API")
+    end
+
+    it "doesn't return a signon user row if the conversation wasn't created via the API" do
+      signon_user = create(:signon_user)
+      conversation.update!(signon_user:)
+
+      result = helper.question_show_summary_list_rows(question, nil, 1, 1)
+      expect(returned_keys(result)).not_to include("API user")
+    end
   end
 
   describe "#decode_and_mark_unicode_tag_segments" do
diff --git a/spec/models/admin/filters/questions_filter_spec.rb b/spec/models/admin/filters/questions_filter_spec.rb
@@ -130,6 +130,17 @@
       expect(filter.results).to eq([question2])
     end
 
+    it "filters the results by conversation source" do
+      question1 = create(:question)
+      question2 = create(:question, conversation: build(:conversation, :api))
+
+      filter = described_class.new(source: "web")
+      expect(filter.results).to eq([question1])
+
+      filter = described_class.new(source: "api")
+      expect(filter.results).to eq([question2])
+    end
+
     it "filters the results by start date" do
       question = create(:question)
       create(:question, created_at: 2.days.ago)
@@ -223,8 +234,8 @@
     it "filters the results by signon user" do
       alice = create(:signon_user, email: "alice@example.com")
       bob = create(:signon_user, email: "bob@example.com")
-      alice_question = create(:question, conversation: create(:conversation, signon_user: alice))
-      bob_question = create(:question, conversation: create(:conversation, signon_user: bob))
+      alice_question = create(:question, conversation: create(:conversation, :api, signon_user: alice))
+      bob_question = create(:question, conversation: create(:conversation, :api, signon_user: bob))
 
       filter = described_class.new(signon_user_id: alice.id)
       expect(filter.results).to eq([alice_question])
@@ -233,6 +244,15 @@
       expect(filter.results).to eq([bob_question])
     end
 
+    it "filters out results assoicated with a signon user that weren't created via the API" do
+      signon_user = create(:signon_user)
+      create(:question, conversation: create(:conversation, signon_user:))
+
+      filter = described_class.new(signon_user_id: signon_user.id)
+
+      expect(filter.results).to eq([])
+    end
+
     it "doesn't filter the results by signon user if signon_user_id and user_id are passed in" do
       alice = create(:signon_user, email: "alice@example.com")
       bob = create(:early_access_user, email: "bob@example.com")
diff --git a/spec/requests/api/v0/conversations_spec.rb b/spec/requests/api/v0/conversations_spec.rb
@@ -1,32 +1,54 @@
 RSpec.describe "Api::V0::ConversationsController" do
   let(:api_user) { create(:signon_user, :conversation_api) }
-  let(:conversation) { create(:conversation, signon_user: api_user) }
+  let(:conversation) { create(:conversation, :api, signon_user: api_user) }
   let(:question) { create(:question, conversation:) }
 
   before do
     login_as(api_user)
   end
 
-  shared_examples "responds with forbidden if user doesn't have conversation-api permission" do |routes:|
-    before { login_as(create(:signon_user)) }
-
+  shared_examples "limits access based on permissions and conversation source" do |routes:|
     routes.each do |route|
+      method = route[:method]
+      path = route[:path]
+      route_params = route[:route_params] || []
+      params = route[:params] || {}
+
       describe "responds with forbidden if user doesn't have conversation-api permission" do
-        it "returns with 403 for #{route[:method]} #{route[:path]}" do
+        before { login_as(create(:signon_user)) }
+
+        it "returns 403 for #{method} #{path}" do
           process(
-            route[:method.to_sym],
-            public_send(route[:path].to_sym, *(route[:route_params] || [])),
-            params: route[:params] || {},
+            method.to_sym,
+            public_send(path.to_sym, *route_params),
+            params: params,
             as: :json,
           )
 
           expect(response).to have_http_status(:forbidden)
         end
       end
+
+      next if path == :api_v0_create_conversation_path
+
+      describe "ensures the conversation was created by the API" do
+        it "returns 404 when conversation source is not :api for #{method} #{path}" do
+          conversation.update!(source: :web)
+
+          process(
+            method.to_sym,
+            public_send(path.to_sym, *route_params),
+            params: params,
+            as: :json,
+          )
+
+          expect(response).to have_http_status(:not_found)
+        end
+      end
     end
   end
 
-  it_behaves_like "responds with forbidden if user doesn't have conversation-api permission",
+  it_behaves_like "limits access based on permissions and conversation source",
                   routes: [
                     {
                       path: :api_v0_create_conversation_path,
@@ -143,6 +165,13 @@
         conversation = Conversation.includes(:signon_user).last
         expect(conversation.signon_user).to eq(api_user)
       end
+
+      it "sets the conversations source to :api" do
+        post api_v0_create_conversation_path, params: payload, as: :json
+
+        conversation = Conversation.last
+        expect(conversation.source).to eq("api")
+      end
     end
 
     context "when the question is invalid" do
diff --git a/spec/requests/conversations_spec.rb b/spec/requests/conversations_spec.rb
@@ -35,7 +35,7 @@
     let(:route_params) { [SecureRandom.uuid] }
   end
 
-  it_behaves_like "requires a conversation", routes: { answer_question_path: %i[get], answer_feedback_path: %i[post] } do
+  it_behaves_like "requires a conversation created via the chat interface", routes: { answer_question_path: %i[get], answer_feedback_path: %i[post] } do
     let(:route_params) { [SecureRandom.uuid] }
   end
 
diff --git a/spec/support/conversation_request_examples.rb b/spec/support/conversation_request_examples.rb
diff --git a/spec/system/admin/user_filters_questions_spec.rb b/spec/system/admin/user_filters_questions_spec.rb

Original file line number	Diff line number	Diff line change
`@@ -85,7 +85,7 @@ def question_show_summary_list_rows(question, answer, question_number, total_que`
`85`	`85`	`end`
`86`	`86`
`87`	`87`	`signon_user = conversation.signon_user`
`88`		`- if signon_user.present?`
	`88`	`+ if signon_user.present? && conversation.source_api?`
`89`	`89`	`rows << {`
`90`	`90`	`field: "API user",`
`91`	`91`	`value: safe_join([`
`@@ -97,6 +97,11 @@ def question_show_summary_list_rows(question, answer, question_number, total_que`
`97`	`97`	`}`
`98`	`98`	`end`
`99`	`99`
	`100`	`+ rows << {`
	`101`	`+ field: "Source",`
	`102`	`+ value: conversation.source_api? ? "API" : conversation.source.humanize,`
	`103`	`+ }`
	`104`	`+`
`100`	`105`	`rows << if answer.present?`
`101`	`106`	`[`
`102`	`107`	`{`