Merge pull request #1821 from alphagov/re-enable-value-in-selections

Re-enable value in selections

Author: thomasiles

app/components/question/selection_component/view.rb

@@ -61,14 +61,14 @@ def none_of_the_above_question_field
       end
 
       def radio_button_options
-        question.answer_settings.selection_options.map.with_index do |option, index|
-          form_builder.govuk_radio_button :selection, option.name, label: { text: option.name }, link_errors: index.zero?
+        question.selection_options_with_none_of_the_above.map.with_index do |option, index|
+          form_builder.govuk_radio_button :selection, option.value, label: { text: option.name }, link_errors: index.zero?
         end
       end
 
       def checkbox_options
         question.answer_settings.selection_options.map.with_index do |option, index|
-          form_builder.govuk_check_box :selection, option.name, label: { text: option.name }, link_errors: index.zero?
+          form_builder.govuk_check_box :selection, option.value, label: { text: option.name }, link_errors: index.zero?
         end
       end
     end

app/models/question/selection.rb

@@ -19,9 +19,9 @@ def allow_multiple_answers?
     end
 
     def show_answer
-      return selection_without_blanks.join(", ") if allow_multiple_answers?
+      return selection_without_blanks.map { |selected| name_from_value(selected) }.join(", ") if allow_multiple_answers?
 
-      selection
+      selection_name
     end
 
     def show_answer_in_email
@@ -39,6 +39,20 @@ def show_answer_in_json(*)
       hash
     end
 
+    def selection_name
+      return nil if selection.nil?
+      return "" if selection.blank?
+
+      name_from_value(selection)
+    end
+
+    # Show the selection option name, which can be different to the value. Value
+    # should stay the same across FormDocuments in different languages.
+    def name_from_value(selected)
+      @options_by_value ||= selection_options_with_none_of_the_above.index_by(&:value)
+      @options_by_value[selected]&.name
+    end
+
     def show_optional_suffix
       false
     end
@@ -77,11 +91,11 @@ def clear_none_of_the_above_answer_if_not_selected
     end
 
     def allowed_options
-      selection_options_with_none_of_the_above.map(&:name)
+      selection_options_with_none_of_the_above.map(&:value)
     end
 
     def none_of_the_above_option
-      OpenStruct.new(name: I18n.t("page.none_of_the_above"))
+      OpenStruct.new(name: I18n.t("page.none_of_the_above"), value: I18n.t("page.none_of_the_above"))
     end
 
     def selection_without_blanks

spec/components/question/selection_component/view_spec.rb

@@ -55,7 +55,8 @@
       let(:question) { build :single_selection_question, is_optional:, selection_options: }
 
       let(:selection_options) do
-        Array.new(30).map { |_index| OpenStruct.new(name: Faker::Lorem.sentence) }
+        option_text = Faker::Lorem.sentence
+        Array.new(30).map { |_index| OpenStruct.new(name: option_text, value: option_text) }
       end
 
       it "renders the question text as a heading" do

spec/factories/models/pages.rb

@@ -26,7 +26,7 @@
     trait :with_selections_settings do
       transient do
         only_one_option { "true" }
-        selection_options { [DataStruct.new(name: "Option 1"), DataStruct.new(name: "Option 2")] }
+        selection_options { [DataStruct.new(name: "Option 1", value: "Option 1"), DataStruct.new(name: "Option 2", value: "Option 2")] }
       end
 
       answer_type { "selection" }

spec/factories/models/question/selection.rb

@@ -15,7 +15,7 @@
 
     transient do
       only_one_option { "true" }
-      selection_options { [DataStruct.new(name: "Option 1"), DataStruct.new(name: "Option 2")] }
+      selection_options { [DataStruct.new(name: "Option 1", value: "Option 1"), DataStruct.new(name: "Option 2", value: "Option 2")] }
       none_of_the_above_question { nil }
     end
 

spec/factories/v2_step.rb

@@ -44,7 +44,7 @@
       trait :with_selections_settings do
         transient do
           only_one_option { "true" }
-          selection_options { [{ name: "Option 1" }, { name: "Option 2" }] }
+          selection_options { [{ name: "Option 1", value: "Option 1" }, { name: "Option 2", value: "Option 2" }] }
         end
 
         answer_type { "selection" }

spec/features/fill_in_autocomplete_question_spec.rb

@@ -1,9 +1,9 @@
 require "rails_helper"
 
 feature "Fill in and submit a form with an autocomplete question", type: :feature do
-  let(:steps) { [(build :v2_question_page_step, :with_selections_settings, id: 1, routing_conditions: [], question_text:, selection_options:)] }
+  let(:steps) { [build(:v2_question_page_step, :with_selections_settings, id: 1, routing_conditions: [], question_text:, selection_options:)] }
   let(:form) { build :v2_form_document, :live?, form_id: 1, name: "Fill in this form", steps:, start_page: 1 }
-  let(:selection_options) { Array.new(31).each_with_index.map { |_element, index| { name: "Answer #{index}" } } }
+  let(:selection_options) { Array.new(31).each_with_index.map { |_element, index| { name: "Answer #{index}", value: "Answer #{index}" } } }
   let(:question_text) { Faker::Lorem.question }
   let(:answer_text) { "Answer 1" }
   let(:reference) { Faker::Alphanumeric.alphanumeric(number: 8).upcase }

spec/models/question/selection_spec.rb

@@ -3,7 +3,10 @@
 RSpec.describe Question::Selection, type: :model do
   subject(:question) { build :selection, only_one_option:, selection_options:, is_optional:, question_text: }
 
-  let(:selection_options) { [OpenStruct.new({ name: "option 1" }), OpenStruct.new({ name: "option 2" })] }
+  # We use different values for name and value to make it clear which is which in the tests.
+  # Name would be the text in current locale and value doesn't change between locales.
+  # For English forms they would be the same, but for Welsh forms they would be different.
+  let(:selection_options) { [OpenStruct.new({ name: "display option 1", value: "option 1" }), OpenStruct.new({ name: "display option 2", value: "option 2" })] }
   let(:is_optional) { false }
   let(:only_one_option) { "false" }
   let(:question_text) { Faker::Lorem.question }
@@ -64,21 +67,21 @@
 
     context "when selection has a value" do
       before do
-        question.selection = %w[something]
+        question.selection = ["option 1"]
       end
 
       it "shows the answer" do
-        expect(question.show_answer).to eq("something")
+        expect(question.show_answer).to eq("display option 1")
       end
 
       it "shows the answer in show_answer_in_csv" do
-        expect(question.show_answer_in_csv).to eq(Hash[question_text, "something"])
+        expect(question.show_answer_in_csv).to eq(Hash[question_text, "display option 1"])
       end
 
       it "returns a hash for show_answer_in_json" do
         expect(question.show_answer_in_json).to eq({
-          selections: %w[something],
-          answer_text: "something",
+          selections: ["option 1"],
+          answer_text: "display option 1",
         })
       end
     end
@@ -152,21 +155,21 @@
 
       context "when selection has a value" do
         before do
-          question.selection = %w[something]
+          question.selection = ["option 1"]
         end
 
         it "shows the answer" do
-          expect(question.show_answer).to eq("something")
+          expect(question.show_answer).to eq("display option 1")
         end
 
         it "shows the answer in show_answer_in_csv" do
-          expect(question.show_answer_in_csv).to eq(Hash[question_text, "something"])
+          expect(question.show_answer_in_csv).to eq(Hash[question_text, "display option 1"])
         end
 
         it "returns a hash for show_answer_in_json" do
           expect(question.show_answer_in_json).to eq({
-            selections: %w[something],
-            answer_text: "something",
+            selections: ["option 1"],
+            answer_text: "display option 1",
           })
         end
       end
@@ -210,20 +213,20 @@
 
     context "when selection has a value" do
       before do
-        question.selection = "something"
+        question.selection = "option 1"
       end
 
       it "shows the answer" do
-        expect(question.show_answer).to eq("something")
+        expect(question.show_answer).to eq("display option 1")
       end
 
       it "shows the answer in show_answer_in_csv" do
-        expect(question.show_answer_in_csv).to eq(Hash[question_text, "something"])
+        expect(question.show_answer_in_csv).to eq(Hash[question_text, "display option 1"])
       end
 
       it "returns a hash for show_answer_in_json" do
         expect(question.show_answer_in_json).to eq({
-          answer_text: "something",
+          answer_text: "display option 1",
         })
       end
     end
@@ -280,20 +283,20 @@
 
       context "when selection has a value" do
         before do
-          question.selection = "something"
+          question.selection = "option 1"
         end
 
         it "shows the answer" do
-          expect(question.show_answer).to eq("something")
+          expect(question.show_answer).to eq("display option 1")
         end
 
         it "shows the answer in show_answer_in_csv" do
-          expect(question.show_answer_in_csv).to eq(Hash[question_text, "something"])
+          expect(question.show_answer_in_csv).to eq(Hash[question_text, "display option 1"])
         end
 
         it "returns a hash for show_answer_in_json" do
           expect(question.show_answer_in_json).to eq({
-            answer_text: "something",
+            answer_text: "display option 1",
           })
         end
       end
@@ -430,7 +433,7 @@
 
   describe "#selection_options_with_none_of_the_above" do
     let(:only_one_option) { "true" }
-    let(:none_of_the_above_option) { OpenStruct.new(name: I18n.t("page.none_of_the_above")) }
+    let(:none_of_the_above_option) { OpenStruct.new(name: I18n.t("page.none_of_the_above"), value: I18n.t("page.none_of_the_above")) }
 
     context "when the user can select 'None of the above'" do
       let(:is_optional) { true }
@@ -463,15 +466,15 @@
 
   describe "#autocomplete_component?" do
     context "when there are 30 selection options" do
-      let(:selection_options) { Array.new(30).map { |_index| OpenStruct.new(name: Faker::Lorem.sentence) } }
+      let(:selection_options) { Array.new(30).map { |_index| OpenStruct.new(name: Faker::Lorem.sentence, value: Faker::Lorem.sentence) } }
 
       it "returns false" do
         expect(question.autocomplete_component?).to be false
       end
     end
 
     context "when there are more than 30 selection options" do
-      let(:selection_options) { Array.new(31).map { |_index| OpenStruct.new(name: Faker::Lorem.sentence) } }
+      let(:selection_options) { Array.new(31).map { |_index| OpenStruct.new(name: Faker::Lorem.sentence, value: Faker::Lorem.sentence) } }
 
       it "returns true" do
         expect(question.autocomplete_component?).to be true