Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reintroduce hash sorting for expanded content-item and links #1100

Merged
merged 1 commit into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ source "https://rubygems.org"
gem "rails", "7.0.5"

gem "bootsnap", require: false
gem "deepsort"
gem "gds-api-adapters"
gem "gds-sso"
gem "govuk_app_config"
Expand Down
2 changes: 2 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ GEM
database_cleaner-core (~> 2.0.0)
mongoid
date (3.3.3)
deepsort (0.4.5)
diff-lcs (1.5.0)
docile (1.4.0)
domain_name (0.5.20190701)
Expand Down Expand Up @@ -438,6 +439,7 @@ DEPENDENCIES
ci_reporter_rspec
climate_control
database_cleaner-mongoid
deepsort
factory_bot
gds-api-adapters
gds-sso
Expand Down
10 changes: 10 additions & 0 deletions app/lib/hash_sorter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
require "deepsort"

class HashSorter
def self.sort(hash)
# Deepsort by default will sort keys in an array too -
# we don't want that, as (for instance) order of links
# may be important
hash.deep_sort(array: false)
end
end
7 changes: 4 additions & 3 deletions app/presenters/content_item_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,14 @@ def initialize(item, api_url_method)
end

def as_json(options = nil)
item.as_json(options).slice(*PUBLIC_ATTRIBUTES).merge(
hash = item.as_json(options).slice(*PUBLIC_ATTRIBUTES).merge(
"links" => RESOLVER.resolve(links),
"description" => RESOLVER.resolve(item.description),
"details" => RESOLVER.resolve(item.details),
).tap do |i|
).tap { |i|
i["redirects"] = item["redirects"] if i["schema_name"] == "redirect"
end
}.deep_stringify_keys
HashSorter.sort(hash)
end

private
Expand Down
3 changes: 2 additions & 1 deletion app/presenters/expanded_links_presenter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ def initialize(expanded_links)
end

def present
expanded_links.deep_symbolize_keys.each_with_object({}) do |(type, links), memo|
result = expanded_links.deep_symbolize_keys.each_with_object({}) do |(type, links), memo|
links = Array.wrap(links)
memo[type] = links.map do |link|
link.dup.except(secret_fields).merge(
Expand All @@ -17,6 +17,7 @@ def present
).compact
end
end
HashSorter.sort(result)
end

private
Expand Down
37 changes: 37 additions & 0 deletions spec/lib/hash_sorter_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
require "rails_helper"

describe HashSorter do
describe ".sort" do
let(:links) do
{
group_2: [
{ base_path: "/group-1/link-1", api_path: "/api/content/group-1/link-1" },
{ base_path: "/group-1/link-2", api_path: "/api/content/group-1/link-2" },
{ base_path: "/group-1/link-3", api_path: "/api/content/group-1/link-3" },
],
group_1: [
{ base_path: "/group-2/link-3", api_path: "/api/content/group-2/link-3" },
{ base_path: "/group-2/link-2", api_path: "/api/content/group-2/link-2" },
{ base_path: "/group-2/link-1", api_path: "/api/content/group-2/link-1" },
],
}
end
let(:result) { described_class.sort(links) }

it "sorts the top-level keys" do
expect(result.keys).to eq(%i[group_1 group_2])
end

it "does not sort the arrays" do
expect(result[:group_1].map { |e| e[:base_path] }).to eq(links[:group_1].map { |e| e[:base_path] })
end

it "sorts the keys on Hashes within arrays" do
%i[group_1 group_2].each do |group|
result[group].each do |hash|
expect(hash.keys).to eq(hash.keys.sort)
end
end
end
end
end
4 changes: 2 additions & 2 deletions spec/presenters/content_item_presenter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,11 @@
end

it "inlines the 'text/html' content type in the details" do
expect(presenter.as_json["details"]).to eq(body: "<p>content</p>")
expect(presenter.as_json["details"]).to eq("body" => "<p>content</p>")
end

it "inlines the 'text/html' content type in the links" do
expect(presenter.as_json["links"][:person].first[:details]).to eq(body: "<p>content</p>")
expect(presenter.as_json["links"]["person"].first["details"]).to eq("body" => "<p>content</p>")
end
end

Expand Down
26 changes: 26 additions & 0 deletions spec/presenters/expanded_links_presenter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -205,4 +205,30 @@
end
end
end

describe "sorting the hash" do
let(:links) do
{
group_2: [
{ base_path: "/group-1/link-1", api_path: "/api/content/group-1/link-1" },
{ base_path: "/group-1/link-2", api_path: "/api/content/group-1/link-2" },
{ base_path: "/group-1/link-3", api_path: "/api/content/group-1/link-3" },
],
group_1: [
{ base_path: "/group-2/link-3", api_path: "/api/content/group-2/link-3" },
{ base_path: "/group-2/link-2", api_path: "/api/content/group-2/link-2" },
{ base_path: "/group-2/link-1", api_path: "/api/content/group-2/link-1" },
],
}
end
let(:result) { described_class.new(links).present }

it "sorts the results" do
expect(result.keys).to eq(result.keys.sort)

result.each_key do |group|
expect(result[group].all? { |link| link.keys == link.keys.sort }).to eq(true)
end
end
end
end