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

Use GraphQL for rendering Role page #3845

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
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
12 changes: 9 additions & 3 deletions app/controllers/roles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,14 @@ class RolesController < ApplicationController
around_action :switch_locale

def show
@role = Role.find!(request.path)
setup_content_item_and_navigation_helpers(@role)
render :show, locals: { role: @role }
if Features.graphql_feature_enabled?
role = RoleGraphql.find!(request.path)
setup_content_item_and_navigation_helpers(role)
render :show, locals: { role: RoleGraphqlPresenter.new(role.content_item) }
else
role = Role.find!(request.path)
setup_content_item_and_navigation_helpers(role)
render :show, locals: { role: RolePresenter.new(role.content_item.content_item_data) }
end
end
end
116 changes: 0 additions & 116 deletions app/models/role.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
require "active_model"

class Role
include ActiveModel::Model

attr_reader :content_item

def initialize(content_item)
Expand All @@ -13,116 +9,4 @@ def self.find!(base_path)
content_item = ContentItem.find!(base_path)
new(content_item)
end

def base_path
content_item_data["base_path"]
end

def title
content_item_data["title"]
end

def responsibilities
details["body"]
end

def organisations
links["ordered_parent_organisations"]
end

def currently_occupied?
current_holder.present?
end

def current_holder
@current_holder ||= role_holders(current: true)
.first
&.dig("links", "person")
&.first
end

def current_holder_biography
current_holder["details"]["body"]
end

def past_holders
@past_holders ||= role_holders(current: false)
.reverse
.map do |rh|
rh.dig("links", "person")
&.first
&.tap do |hash|
hash["details"]["start_year"] = Time.zone.parse(rh["details"]["started_on"]).year
hash["details"]["end_year"] = Time.zone.parse(rh["details"]["ended_on"]).year
end
end
end

def link_to_person
current_holder["base_path"]
end

def translations
available_translations.map do |translation|
translation
.slice(:locale, :base_path)
.merge(
text: language_name(translation[:locale]),
active: locale == translation[:locale],
)
end
end

def locale
content_item_data["locale"]
end

def announcements
@announcements ||= AnnouncementsPresenter.new(slug, slug_prefix: "ministers", filter_key: "roles")
end

def supports_historical_accounts?
details["supports_historical_accounts"]
end

def past_holders_url
{
"prime-minister" => "/government/history/past-prime-ministers",
"chancellor-of-the-exchequer" => "/government/history/past-chancellors",
"secretary-of-state-for-foreign-commonwealth-and-development-affairs" => "/government/history/past-foreign-secretaries",
"foreign-secretary" => "/government/history/past-foreign-secretaries",
}[slug]
end

private

def content_item_data
content_item.content_item_data
end

def links
content_item_data["links"]
end

def details
content_item_data["details"]
end

def language_name(language)
I18n.t("shared.language_name", locale: language)
end

def available_translations
links["available_translations"].map(&:symbolize_keys)
end

def slug
base_path.split("/").last
end

def role_holders(current:)
links.fetch("role_appointments", [])
.find_all { |rh| rh["details"]["current"] == current }
.sort_by { |rh| rh["details"]["started_on"] }
end
end
14 changes: 14 additions & 0 deletions app/models/role_graphql.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
class RoleGraphql
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this (and the other for the world index) should be namespaced to keep them all together in one place? E.g. Graphql::Role?

attr_reader :content_item

def initialize(content_item)
@content_item = content_item
end

def self.find!(base_path)
query = Graphql::RoleQuery.new(base_path).query

edition = Services.publishing_api.graphql_query(query).dig("data", "edition")
new(edition)
end
end
49 changes: 49 additions & 0 deletions app/presenters/role_graphql_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
class RoleGraphqlPresenter < RolePresenter
def initialize(content_item_data)
super(content_item_data.deep_transform_keys(&:underscore))
end

def responsibilities
content_item_data["responsibilities"]
end

def current_holder
person_hash = content_item_data
.fetch("current_role_appointment")
&.dig("person")

return unless person_hash

CurrentRoleHolder.new(**person_hash)
end

def past_holders
content_item_data.fetch("past_role_appointments")
.sort_by { |role_appointment_hash| role_appointment_hash["started_on"] }
.reverse
.map do |role_appointment_hash|
person_hash = role_appointment_hash["person"]

PastRoleHolder.new(
**person_hash.merge({
"start_year" => Time.zone.parse(role_appointment_hash["started_on"]).year,
"end_year" => Time.zone.parse(role_appointment_hash["ended_on"]).year,
}),
)
end
end

def supports_historical_accounts?
content_item_data["supports_historical_accounts"]
end

private

def available_translations
content_item_data["available_translations"]
end

def ordered_parent_organisations
content_item_data["ordered_parent_organisations"]
end
end
150 changes: 150 additions & 0 deletions app/presenters/role_presenter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
class RolePresenter
def initialize(content_item_data)
@content_item_data = content_item_data
end

def base_path
content_item_data["base_path"]
end

def title
content_item_data["title"]
end

def responsibilities
details["body"]
end

OrganisationPresenter = Data.define(:title, :base_path) do
def initialize(attributes)
super(attributes.slice(*members.map(&:to_s)))
end
end
Comment on lines +18 to +22
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced these sub-presenters add much value here and seem a little inconsistent with other presenters (noticed you'd noted about this in the commit message). Is there a way we can achieve the same without them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My aim with these sub-presenters is for the "presented" data to be used like objects with attribute reader methods instead of like hashes. Yeah, I see what you're saying, I guess that goal itself might already be the definition of inconsistency if the existing presenters all provide hash access.

As I (hope I) mentioned in the commit, the motivation for objects instead of hashes ^ is to be able to avoid having to massage the new GraphQL JSON data into nested hashes that're structured identically to the Content Store JSON.

So, we could always just take the route I avoided and massage the GraphQL JSON into nested hashes identical to the pre-existing Content Store ones. It wouldn't be a terrible thing (I mean, it's standard practice when working with unfamiliar or legacy code, and it potentially leaves less of a mess if we remove the GraphQL code later).

On the other hand, if the object access is preferred, off the top of my head the options are:

  1. define presenter classes
  2. use Structs
  3. use OpenStructs
  4. use Data objects

What appealed to me about Data objects was the minimal boilerplate (we could go further and try to hide that common initialize somewhere less distracting) and that compared to the struct types, it doesn't define setter methods out-of-the-box.

I hope the Navigation team will spot this change and have some thoughts, too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


def organisations
ordered_parent_organisations.map do |organisation_hash|
OrganisationPresenter.new(**organisation_hash)
end
end

def currently_occupied?
current_holder.present?
end

CurrentRoleHolder = Data.define(:title, :base_path, :biography) do
def initialize(attributes)
super(attributes.slice(*members.map(&:to_s)))
end
end

def current_holder
@current_holder ||= begin
person_hash = role_holders(current: true)
.first
&.dig("links", "person")
&.first

unless person_hash.nil?
person_hash["biography"] = person_hash.fetch("details").fetch("body")
CurrentRoleHolder.new(**person_hash)
end
end
end

def current_holder_biography
current_holder&.biography
end

PastRoleHolder = Data.define(:title, :base_path, :start_year, :end_year) do
def initialize(attributes)
super(attributes.slice(*members.map(&:to_s)))
end
end

def past_holders
@past_holders ||= role_holders(current: false)
.reverse
.map { |rh|
person_hash = rh.dig("links", "person")&.first

next if person_hash.nil?

PastRoleHolder.new(
**person_hash.merge({
"start_year" => Time.zone.parse(rh["details"]["started_on"]).year,
"end_year" => Time.zone.parse(rh["details"]["ended_on"]).year,
}),
)
}.compact
end

def link_to_person
current_holder&.base_path
end

def translations
available_translations.map do |translation|
translation
.slice(:locale, :base_path)
.merge(
text: language_name(translation[:locale]),
active: locale == translation[:locale],
)
end
end

def locale
content_item_data["locale"]
end

def announcements
@announcements ||= AnnouncementsPresenter.new(slug, slug_prefix: "ministers", filter_key: "roles")
end

def supports_historical_accounts?
details["supports_historical_accounts"]
end

def past_holders_url
{
"prime-minister" => "/government/history/past-prime-ministers",
"chancellor-of-the-exchequer" => "/government/history/past-chancellors",
"secretary-of-state-for-foreign-commonwealth-and-development-affairs" => "/government/history/past-foreign-secretaries",
"foreign-secretary" => "/government/history/past-foreign-secretaries",
}[slug]
end

private

attr_reader :content_item_data

def links
content_item_data["links"]
end

def details
content_item_data["details"]
end

def language_name(language)
I18n.t("shared.language_name", locale: language)
end

def available_translations
links["available_translations"].map(&:symbolize_keys)
end

def slug
base_path.split("/").last
end

def role_holders(current:)
links.fetch("role_appointments", [])
.find_all { |rh| rh["details"]["current"] == current }
.sort_by { |rh| rh["details"]["started_on"] }
end

def ordered_parent_organisations
links["ordered_parent_organisations"]
end
end
Loading
Loading