Merge pull request #274 from alphagov/limit-and-return-active-questions-in-api

Limit and only return active questions in API

Author: davidgisbey

app/controllers/api/v0/conversations_controller.rb

@@ -16,7 +16,7 @@ def create
   end
 
   def show
-    answered_questions = @conversation.questions.joins(:answer)
+    answered_questions = @conversation.questions_for_showing_conversation(only_answered: true)
     pending_question = @conversation.questions.unanswered.last
 
     render(
@@ -69,7 +69,7 @@ def answer_feedback
 
   def find_conversation
     @conversation = Conversation
-                    .includes(questions: { answer: %i[sources feedback] })
+                    .active
                     .where(signon_user_id: current_user.id, source: :api)
                     .find(params[:conversation_id])
   end

app/models/conversation.rb

@@ -13,10 +13,11 @@ class Conversation < ApplicationRecord
        },
        prefix: true
 
-  def questions_for_showing_conversation
-    Question.where(conversation: self)
-            .includes(answer: %i[feedback sources])
-            .active
-            .last(Rails.configuration.conversations.max_question_count)
+  def questions_for_showing_conversation(only_answered: false)
+    scope = Question.where(conversation: self)
+                  .includes(answer: %i[feedback sources])
+                  .active
+    scope = scope.joins(:answer) if only_answered
+    scope.last(Rails.configuration.conversations.max_question_count)
   end
 end

spec/models/conversations_spec.rb

@@ -33,13 +33,28 @@
   describe ".questions_for_showing_conversation" do
     let(:conversation) { create(:conversation) }
 
+    before do
+      allow(Rails.configuration.conversations).to receive(:max_question_count).and_return(2)
+    end
+
     it "returns the last N active questions based on the configuration value" do
       create(:question, conversation:)
       expected = 2.times.map do |_|
         create(:question, conversation:)
       end
-      allow(Rails.configuration.conversations).to receive(:max_question_count).and_return(2)
       expect(conversation.reload.questions_for_showing_conversation).to eq(expected)
     end
+
+    context "when only_answered is true" do
+      it "returns the last N active answered questions based on the configuration value" do
+        create(:question, :with_answer, conversation:)
+        expected = 2.times.map do |_|
+          create(:question, :with_answer, conversation:)
+        end
+        create(:question, conversation:)
+
+        expect(conversation.reload.questions_for_showing_conversation(only_answered: true)).to eq(expected)
+      end
+    end
   end
 end

spec/requests/api/v0/conversations_spec.rb

@@ -164,6 +164,12 @@
       expect(response).to have_http_status(:not_found)
     end
 
+    it "returns a 404 if the conversation has expired" do
+      conversation = create(:conversation, :api, :expired, signon_user: api_user)
+      get api_v0_show_conversation_path(conversation)
+      expect(response).to have_http_status(:not_found)
+    end
+
     it "returns a 404 if the conversation is not associated with the user" do
       different_user = create(:signon_user, :conversation_api)
       conversation = create(:conversation, signon_user: different_user)
@@ -234,6 +240,16 @@
   end
 
   describe "PUT :update" do
+    let(:conversation) do
+      create(
+        :conversation,
+        :api,
+        signon_user:
+        api_user,
+        questions: [create(:question, :with_answer)],
+      )
+    end
+
     context "when the params are valid" do
       let(:user_question) { "What is the capital of France?" }
       let(:params) { { user_question: } }