Merge pull request #3134 from alphagov/optimise-details-graphql

Optimise `details` handling for GraphQL

app/graphql/types/edition_type.rb

@@ -135,7 +135,7 @@ class WhipOrganisation < Types::BaseObject
 
       field :body, String
       field :brand, String
-      field :change_history, GraphQL::Types::JSON, extras: [:parent]
+      field :change_history, GraphQL::Types::JSON
       field :current, Boolean
       field :default_news_image, Image
       field :display_date, GraphQL::Types::ISO8601DateTime
@@ -162,7 +162,7 @@ class WhipOrganisation < Types::BaseObject
     field :content_id, ID
     field :current, Boolean
     field :description, String
-    field :details, Details
+    field :details, Details, extras: [:lookahead]
     field :document_type, String
     field :ended_on, GraphQL::Types::ISO8601DateTime
     field :first_published_at, GraphQL::Types::ISO8601DateTime, null: false
@@ -187,16 +187,18 @@ class WhipOrganisation < Types::BaseObject
     field :web_url, String
     field :withdrawn_notice, WithdrawnNotice
 
-    def change_history
-      Presenters::ChangeHistoryPresenter.new(object).change_history
-    end
+    def details(lookahead:)
+      requested_details_fields = lookahead.selections.map(&:name)
+      object.details = object.details.slice(*requested_details_fields)
+
+      change_history_presenter = Presenters::ChangeHistoryPresenter.new(object) if requested_details_fields.include?(:change_history)
+      content_embed_presenter = Presenters::ContentEmbedPresenter.new(object)
 
-    def details
       Presenters::ContentTypeResolver.new("text/html").resolve(
         Presenters::DetailsPresenter.new(
           object.details,
-          nil,
-          Presenters::ContentEmbedPresenter.new(object),
+          change_history_presenter,
+          content_embed_presenter,
         ).details,
       )
     end

app/presenters/content_embed_presenter.rb

@@ -5,6 +5,8 @@ def initialize(edition)
     end
 
     def render_embedded_content(details)
+      return details unless target_content_ids
+
       details.each_pair do |field, value|
         next if value.blank?
 
@@ -16,13 +18,15 @@ def render_embedded_content(details)
 
   private
 
+    def target_content_ids
+      @target_content_ids ||= @edition
+                                .links
+                                .where(link_type: "embed")
+                                .pluck(:target_content_id)
+    end
+
     def embedded_editions
       @embedded_editions ||= begin
-        target_content_ids = @edition
-         .links
-         .where(link_type: "embed")
-         .pluck(:target_content_id)
-
         embedded_edition_ids = ::Queries::GetEditionIdsWithFallbacks.call(
           target_content_ids,
           locale_fallback_order: [@edition.locale, Edition::DEFAULT_LOCALE].uniq,

app/presenters/details_presenter.rb

@@ -11,40 +11,33 @@ def initialize(content_item_details, change_history_presenter, content_embed_pre
     end
 
     def details
-      @details ||=
-        begin
-          updated = content_embed(content_item_details).presence || content_item_details
-          updated = recursively_transform_govspeak(updated)
-          updated[:change_history] = change_history if change_history.present?
-          updated
-        end
+      updated = content_embed(content_item_details).presence || content_item_details
+      updated = recursively_transform_govspeak(updated)
+      updated[:change_history] = change_history if change_history.present?
+      updated
     end
 
   private
 
-    def govspeak_content?(value)
-      wrapped = Array.wrap(value)
-      wrapped.all? { |hsh| hsh.is_a?(Hash) } &&
-        wrapped.one? { |hsh| hsh[:content_type] == "text/govspeak" } &&
-        wrapped.none? { |hsh| hsh[:content_type] == "text/html" }
-    end
-
-    def html_content?(value)
-      wrapped = Array.wrap(value)
-      wrapped.all? { |hsh| hsh.is_a?(Hash) } &&
-        wrapped.one? { |hsh| hsh[:content_type] == "text/html" }
+    def parsed_content(array_of_hashes)
+      if array_of_hashes.one? { |hash| hash[:content_type] == "text/html" }
+        array_of_hashes
+      elsif array_of_hashes.one? { |hash| hash[:content_type] == "text/govspeak" }
+        render_govspeak(array_of_hashes)
+      end
     end
 
     def recursively_transform_govspeak(obj)
-      return obj if !obj.respond_to?(:map) || html_content?(obj)
-      return render_govspeak(obj) if govspeak_content?(obj)
-
-      if obj.is_a?(Hash)
+      if obj.is_a?(Array) && obj.all?(Hash) && (parsed_obj = parsed_content(obj))
+        parsed_obj
+      elsif obj.is_a?(Array)
+        obj.map { |o| recursively_transform_govspeak(o) }
+      elsif obj.is_a?(Hash)
         obj.transform_values do |value|
           recursively_transform_govspeak(value)
         end
       else
-        obj.map { |o| recursively_transform_govspeak(o) }
+        obj
       end
     end
 

app/validators/well_formed_content_types_validator.rb

@@ -23,7 +23,11 @@ def validate!(object)
   end
 
   def validate_hash!(hash)
-    validate_array!(hash.values)
+    if hash.keys == %i[content_type content]
+      @error_messages << "fields with content types should be presented in an array"
+    else
+      validate_array!(hash.values)
+    end
   end
 
   def validate_array!(array)

spec/graphql/types/edition_type_spec.rb

@@ -1,5 +1,5 @@
 RSpec.describe "Types::EditionType" do
-  include GraphQL::Testing::Helpers.for(PublishingApiSchema)
+  include GraphQL::Testing::Helpers
 
   describe "#withdrawn_notice" do
     context "when the edition is withdrawn" do
@@ -12,6 +12,7 @@
 
         expect(
           run_graphql_field(
+            PublishingApiSchema,
             "Edition.withdrawn_notice",
             edition,
           ),
@@ -23,6 +24,7 @@
       it "returns nil" do
         expect(
           run_graphql_field(
+            PublishingApiSchema,
             "Edition.withdrawn_notice",
             create(:edition),
           ),
@@ -38,8 +40,10 @@
 
         expect(
           run_graphql_field(
+            PublishingApiSchema,
             "Edition.details",
             edition,
+            lookahead: OpenStruct.new(selections: [OpenStruct.new(name: :body)]),
           )[:body],
         ).to eq("some text")
       end
@@ -56,8 +60,10 @@
 
         expect(
           run_graphql_field(
+            PublishingApiSchema,
             "Edition.details",
             edition,
+            lookahead: OpenStruct.new(selections: [OpenStruct.new(name: :body)]),
           )[:body],
         ).to eq("<p>some other text</p>")
       end
@@ -73,8 +79,10 @@
 
         expect(
           run_graphql_field(
+            PublishingApiSchema,
             "Edition.details",
             edition,
+            lookahead: OpenStruct.new(selections: [OpenStruct.new(name: :body)]),
           )[:body],
         ).to eq("<p>some text</p>\n")
       end

spec/presenters/details_presenter_spec.rb

@@ -108,25 +108,6 @@
       it { is_expected.to match(expected_result) }
     end
 
-    context "when we're passed hashes rather than arrays" do
-      let(:edition_details) do
-        {
-          body: { content_type: "text/govspeak", content: "**hello**" },
-        }
-      end
-
-      let(:expected_result) do
-        {
-          body: [
-            { content_type: "text/govspeak", content: "**hello**" },
-            { content_type: "text/html", content: "<p><strong>hello</strong></p>\n" },
-          ],
-        }
-      end
-
-      it { is_expected.to match(expected_result) }
-    end
-
     context "when we're passed an image hash" do
       let(:edition_details) do
         { image: { content_type: "image/png", content: "some content" } }

spec/validators/well_formed_content_types_validator_spec.rb

@@ -49,6 +49,13 @@ def initialize
     expect(record.errors).to be_empty
   end
 
+  it "rejects hashes containing content types that are not wrapped in an array" do
+    value = { content_type: "text/html", content: "<p>content</p>" }
+    subject.validate_each(record, attribute, value)
+    expect(record.errors).to be_present
+    expect(record.errors.messages_for(:some_attribute)).to eq(["fields with content types should be presented in an array"])
+  end
+
   it "rejects values where the content key is missing" do
     value = [{ content_type: "text/html" }]
     subject.validate_each(record, attribute, value)