From 1c5c5ddfbb678ce21b8634f8091052eb89437212 Mon Sep 17 00:00:00 2001 From: Mark Longair Date: Mon, 16 Jan 2017 16:46:09 +0000 Subject: [PATCH 1/7] Upgrade everypolitician-popolo to the latest version --- Gemfile.lock | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 00aed0f7f..ba9557347 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,8 +1,9 @@ GIT remote: https://github.com/everypolitician/everypolitician-popolo.git - revision: 4e6e75ddfece32889243595e7305ba9007e3c8b4 + revision: 40dc460d8340df9ae78469b755ce73b4fd074ce3 specs: - everypolitician-popolo (0.5.0) + everypolitician-popolo (0.8.0) + require_all GIT remote: https://github.com/everypolitician/everypolitician-ruby.git From 7219f8e5b65b33227b18d04910dfe458db670d3b Mon Sep 17 00:00:00 2001 From: Mark Longair Date: Mon, 16 Jan 2017 15:24:43 +0000 Subject: [PATCH 2/7] PersonCard: use the 'memberships' attribute of the person The displayed positions should only be for a particular term, however, so we need to pass 'term' as well, to select memberships for that term. This isn't a bad thing, though, because in subsequent commits 'term' will be useful for getting the legislature, country, proxy image URL and the top identifiers. --- lib/page/term_table.rb | 10 +--------- lib/person_cards.rb | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/lib/page/term_table.rb b/lib/page/term_table.rb index c57b24246..a5bcb2090 100644 --- a/lib/page/term_table.rb +++ b/lib/page/term_table.rb @@ -60,7 +60,7 @@ def people PersonCard.new( person: person, proxy_image: image_proxy_url(person.id), - memberships: person_memberships(person), + term: term, top_identifiers: top_identifiers ) end @@ -92,20 +92,12 @@ def top_identifiers .map { |s, _ids| s } end - def person_memberships(person) - membership_lookup[person.id] - end - def image_proxy_url(id) 'https://mysociety.github.io/politician-image-proxy' \ "/#{country.slug}/#{house.slug}/#{id}/140x140.jpeg" end # Caches for faster lookup - def membership_lookup - @membership_lookup ||= current_term_memberships.group_by(&:person_id) - end - def area_lookup @area_lookup ||= popolo.areas.group_by(&:id) end diff --git a/lib/person_cards.rb b/lib/person_cards.rb index 3befcf36a..234037897 100644 --- a/lib/person_cards.rb +++ b/lib/person_cards.rb @@ -1,13 +1,13 @@ # frozen_string_literal: true class PersonCard - attr_reader :proxy_image, :memberships + attr_reader :proxy_image # TODO: pass fewer arguments! - def initialize(person:, proxy_image:, memberships:, top_identifiers:) + def initialize(person:, proxy_image:, term:, top_identifiers:) @person = person - @memberships = memberships # should be able to get from Person @proxy_image = proxy_image # get from Legislature + @term = term @top_identifiers = top_identifiers # get from Legislature end @@ -39,9 +39,17 @@ def identifiers Section::Identifiers.new(person, top_identifiers: top_identifiers).data end + def memberships + person.memberships.where(legislative_period_id: term.id) + end + private - attr_reader :person, :top_identifiers + attr_reader :person, :term, :top_identifiers + + def legislature + term.legislature + end class Section CardLine = Struct.new(:type, :value, :link) From 1a4ee9de4cc299a03393ffc40bad45f45b41add6 Mon Sep 17 00:00:00 2001 From: Mark Longair Date: Tue, 17 Jan 2017 14:14:52 +0000 Subject: [PATCH 3/7] PersonCard: It's no longer necessary to pass the proxy_image URL We can derive the proxy image URL from the country, legislature and person ID, and the former two can be found from the term. --- lib/page/term_table.rb | 6 ------ lib/person_cards.rb | 9 ++++++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/page/term_table.rb b/lib/page/term_table.rb index a5bcb2090..1769f34c7 100644 --- a/lib/page/term_table.rb +++ b/lib/page/term_table.rb @@ -59,7 +59,6 @@ def people @people ||= people_for_current_term.sort_by { |e| [e.sort_name, e.name] }.map do |person| PersonCard.new( person: person, - proxy_image: image_proxy_url(person.id), term: term, top_identifiers: top_identifiers ) @@ -92,11 +91,6 @@ def top_identifiers .map { |s, _ids| s } end - def image_proxy_url(id) - 'https://mysociety.github.io/politician-image-proxy' \ - "/#{country.slug}/#{house.slug}/#{id}/140x140.jpeg" - end - # Caches for faster lookup def area_lookup @area_lookup ||= popolo.areas.group_by(&:id) diff --git a/lib/person_cards.rb b/lib/person_cards.rb index 234037897..a11b24403 100644 --- a/lib/person_cards.rb +++ b/lib/person_cards.rb @@ -1,16 +1,19 @@ # frozen_string_literal: true class PersonCard - attr_reader :proxy_image # TODO: pass fewer arguments! - def initialize(person:, proxy_image:, term:, top_identifiers:) + def initialize(person:, term:, top_identifiers:) @person = person - @proxy_image = proxy_image # get from Legislature @term = term @top_identifiers = top_identifiers # get from Legislature end + def proxy_image + 'https://mysociety.github.io/politician-image-proxy' \ + "/#{legislature.country.slug}/#{legislature.slug}/#{id}/140x140.jpeg" + end + def id person.id end From 4c12a77195d3f000fa220bf2b9e6ded06e6d767e Mon Sep 17 00:00:00 2001 From: Mark Longair Date: Tue, 17 Jan 2017 19:22:38 +0000 Subject: [PATCH 4/7] Move various methods from TermTable to LegislativePeriod These methods: top_identifiers current_term_memberships current_term_people_ids people_for_current_term ... all operate on data specific to a term; those that are still needed can be moved to the LegislativePeriod and then they can be used wherever a term object is available. --- lib/everypolitician_extensions.rb | 21 +++++++++++++++++++++ lib/page/term_table.rb | 25 +------------------------ 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/lib/everypolitician_extensions.rb b/lib/everypolitician_extensions.rb index ba7a9962c..74f424ca7 100644 --- a/lib/everypolitician_extensions.rb +++ b/lib/everypolitician_extensions.rb @@ -37,6 +37,27 @@ def memberships_at_end mem.end_date.to_s.empty? || mem.end_date == end_date end end + + def people_ids + @people_ids ||= Set.new(memberships.map(&:person_id)) + end + + def people + @people ||= legislature.popolo.persons.select do |p| + people_ids.include?(p.id) + end + end + + def top_identifiers + @top_identifiers ||= people + .map(&:identifiers) + .compact + .flatten + .reject { |i| i[:scheme] == 'everypolitician_legacy' } + .group_by { |i| i[:scheme] } + .sort_by { |s, ids| [-ids.size, s] } + .map { |s, _ids| s } + end end module MembershipExtension diff --git a/lib/page/term_table.rb b/lib/page/term_table.rb index 1769f34c7..20ed969e3 100644 --- a/lib/page/term_table.rb +++ b/lib/page/term_table.rb @@ -56,7 +56,7 @@ def group_data end def people - @people ||= people_for_current_term.sort_by { |e| [e.sort_name, e.name] }.map do |person| + @people ||= term.people.sort_by { |e| [e.sort_name, e.name] }.map do |person| PersonCard.new( person: person, term: term, @@ -80,17 +80,6 @@ def popolo @popolo ||= house.popolo end - def top_identifiers - @tidx ||= people_for_current_term - .map(&:identifiers) - .compact - .flatten - .reject { |i| i[:scheme] == 'everypolitician_legacy' } - .group_by { |i| i[:scheme] } - .sort_by { |s, ids| [-ids.size, s] } - .map { |s, _ids| s } - end - # Caches for faster lookup def area_lookup @area_lookup ||= popolo.areas.group_by(&:id) @@ -99,17 +88,5 @@ def area_lookup def org_lookup @org_lookup ||= popolo.organizations.group_by(&:id) end - - def current_term_memberships - @ctm ||= term.memberships - end - - def current_term_people_ids - @ctpids ||= Set.new(current_term_memberships.map(&:person_id)) - end - - def people_for_current_term - @pct ||= popolo.persons.select { |p| current_term_people_ids.include?(p.id) } - end end end From 881038c92a86783efabf68b525ad1e6c9f9d1bf3 Mon Sep 17 00:00:00 2001 From: Mark Longair Date: Wed, 18 Jan 2017 11:14:12 +0000 Subject: [PATCH 5/7] Remove the unnecessary LegislativePeriodExtension#people_ids method Thanks to @tmtmtmtm for pointing out that the people method can be much simpler, making the people_ids method unnecessary. --- lib/everypolitician_extensions.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/everypolitician_extensions.rb b/lib/everypolitician_extensions.rb index 74f424ca7..c8ef4d70d 100644 --- a/lib/everypolitician_extensions.rb +++ b/lib/everypolitician_extensions.rb @@ -38,14 +38,8 @@ def memberships_at_end end end - def people_ids - @people_ids ||= Set.new(memberships.map(&:person_id)) - end - def people - @people ||= legislature.popolo.persons.select do |p| - people_ids.include?(p.id) - end + @people ||= memberships.map(&:person).uniq(&:id) end def top_identifiers From 595f5b6e018416b9ea0de19669fc742bc0e78897 Mon Sep 17 00:00:00 2001 From: Mark Longair Date: Tue, 17 Jan 2017 19:29:06 +0000 Subject: [PATCH 6/7] PersonCard: get top_identifiers from term instead This is now directly available from the LegislativePeriod object (called 'term') so there's no need to pass it to the initializer. --- lib/page/term_table.rb | 1 - lib/person_cards.rb | 11 ++++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/page/term_table.rb b/lib/page/term_table.rb index 20ed969e3..77820e313 100644 --- a/lib/page/term_table.rb +++ b/lib/page/term_table.rb @@ -60,7 +60,6 @@ def people PersonCard.new( person: person, term: term, - top_identifiers: top_identifiers ) end end diff --git a/lib/person_cards.rb b/lib/person_cards.rb index a11b24403..404710a7b 100644 --- a/lib/person_cards.rb +++ b/lib/person_cards.rb @@ -1,12 +1,9 @@ # frozen_string_literal: true class PersonCard - - # TODO: pass fewer arguments! - def initialize(person:, term:, top_identifiers:) + def initialize(person:, term:) @person = person @term = term - @top_identifiers = top_identifiers # get from Legislature end def proxy_image @@ -48,7 +45,11 @@ def memberships private - attr_reader :person, :term, :top_identifiers + attr_reader :person, :term + + def top_identifiers + term.top_identifiers + end def legislature term.legislature From b2e507b3591f099c31cfa361c1e45a57adc8d4eb Mon Sep 17 00:00:00 2001 From: Mark Longair Date: Tue, 17 Jan 2017 21:18:49 +0000 Subject: [PATCH 7/7] Fix Rubocop errors Rubocop complained about the following things: lib/page/term_table.rb:90:11: C: Align the elements of a hash literal if they span more than one line. person: person, ^^^^^^^^^^^^^^^^^^^^^^^ lib/page/term_table.rb:91:11: C: Align the elements of a hash literal if they span more than one line. term: term, ^^^^^^^^^^^^^^^^^^^^^ lib/page/term_table.rb:91:32: C: Avoid comma after the last parameter of a method call. term: term, ^ This commit fixes those errors in a way that seems to fit the local style in this file. --- lib/page/term_table.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/page/term_table.rb b/lib/page/term_table.rb index 77820e313..0efdade8e 100644 --- a/lib/page/term_table.rb +++ b/lib/page/term_table.rb @@ -58,8 +58,8 @@ def group_data def people @people ||= term.people.sort_by { |e| [e.sort_name, e.name] }.map do |person| PersonCard.new( - person: person, - term: term, + person: person, + term: term ) end end