Simplify the instantiation of PersonCard objects #15603

mhl · 2017-01-17T20:04:23Z

What does this do?

This pull request simplifies the dependencies of the PersonCard class,
so that it only needs to take a Person and a LegislativePeriod when
instantiating. It also moves some functionality from TermTable to
LegislativePeriod ~~(by monkey-patching the latter class).~~

Why was this needed?

In the future we would like to reuse these models in other projects, and
these changes make that reuse easier.

Relevant Issue(s)

I couldn't find one, but some of these changes were suggested by "TODO"
comments in the existing source.

Implementation notes

I'm not 100% sure if the monkey-patching of LegislativePeriod is sensible
or not - maybe similar changes in the original class in everypolitician-ruby
would be more useful in the long run, but I thought this was OK.

Screenshots

The appearance of the site should be unchanged by these.

Notes to Reviewer

I've run the recursive wget on the site before and after this change, and
the results are identical (according to diff -r) so I don't think this has
broken anything.

I'm not sure if the upgrade of everypolitician-popolo is actually required
any more (I've gone through several variants of this change) but it seems
like a good idea in any case.

Notes to Merger

None.

tmtmtmtm

Thanks for doing this @mhl — it's been on my wishlist for quite some time! I have a very slight concern that upgrading to the latest everypolitician-popolo leaves us with a bunch of local workarounds to things that are no longer true with that version, but as the tests all pass, and there are no changes when generating the static site, it's probably OK to go ahead with this and then have look separately at cleaning those up.

A few comments in-place for things that are largely stylistic (at this stage in this project I'm keen to maintain consistency), or where having upgraded everypolitician-popolo I think you should be able to simplify some things even more, but the underlying approach looks good.

tmtmtmtm · 2017-01-17T20:31:20Z

lib/page/term_table.rb

+
+module Everypolitician
+  class LegislativePeriod
+    def people_ids


It looks like this method is only used within people here, so could be private. (If needed at all — see later comment)

tmtmtmtm · 2017-01-17T20:35:51Z

lib/page/term_table.rb

+      @people_ids ||= Set.new(memberships.map(&:person_id))
+    end
+
+    def people


As we now have the latest everypolitician-popolo, I think this could just be memberships.map(&:person).uniq(&:id), now, with no need for the people_ids method.

tmtmtmtm · 2017-01-17T20:52:30Z

lib/person_cards.rb

-    @top_identifiers = top_identifiers # get from Legislature
+    @term            = term
+    @legislature     = term.legislature
+    @top_identifiers = term.top_identifiers


Apologies that the coding style isn't well documented anywhere, but in this project we try to maintain very strong encapsulation — restricting instance variables to only things passed directly to the constructor, and even then always accessing those through (ideally private) accessor methods, rather than directly. We also try to avoid doing work in the constructor other than initialising those variables. So here, I'd prefer legislature and top_identifiers to be methods.

NB: this is now better documented: https://github.com/everypolitician/viewer-sinatra/blob/master/CONTRIBUTING.md

tmtmtmtm · 2017-01-17T21:21:40Z

lib/person_cards.rb

@@ -39,6 +41,10 @@ def identifiers
    Section::Identifiers.new(person, top_identifiers: top_identifiers).data
  end

+  def memberships
+    @person.memberships.select { |m| m.term.id == @term.id }


Rather than rolling your own select here, it's probably better to go through the public search interface: i.e. @person.memberships.where(legislative_period_id: term.id)

tmtmtmtm · 2017-01-17T21:24:30Z

lib/page/term_table.rb

@@ -3,6 +3,35 @@
 require_relative '../everypolitician_extensions'
 require_relative '../person_cards'

+# Reopen the LegislativePeriod class to add helper methods; we could
+# consider adding some of these to the class definition in
+# everypolitician-ruby instead.


Rather than re-opening here, I think it would be better to add these into lib/everypolitician_extensions.rb, especially as we're already extending LegislativePeriod there.

Thanks, I hadn't spotted that (despite actually using the memberships method defined there!)

mhl · 2017-01-18T14:27:47Z

Thanks for all those helpful comments, @tmtmtmtm - it was too confusing to amend these via fixups so I've pushed a new version of the branch.

I've tried diffing this generated version with the other and there are some changes:

Sometimes the order of two people with an identical sort_name is reversed (e.g. Octavio Jara Wolff and Sergio Jara Catalán)
Sometime the top identifiers are in a different order. In the examples I've looked at this is similarly because there there the same number of those identifiers.

These are both due to the following returning people in different orders:

# On master:
popolo.persons.select { |p| Set.new(term.memberships.map(&:person_id)).include?(p.id) }

# On simplify-personcard
term.memberships.map(&:person).uniq(&:id)

The top identifiers and persons are futher sorted before use, so this only has an effect where the sort key is the same for two different items. The differences in output are pretty inconsequential, I think, but to potentially reduce the time spent in looking into similar problems in the future, I'd be tempted to add:

diff --git a/lib/everypolitician_extensions.rb b/lib/everypolitician_extensions.rb
index 6928a06..c8ef4d7 100644
--- a/lib/everypolitician_extensions.rb
+++ b/lib/everypolitician_extensions.rb
@@ -49,7 +49,7 @@ module EveryPolitician
                            .flatten
                            .reject { |i| i[:scheme] == 'everypolitician_legacy' }
                            .group_by { |i| i[:scheme] }
-                           .sort_by { |_s, ids| -ids.size }
+                           .sort_by { |s, ids| [-ids.size, s] }
                            .map { |s, _ids| s }
     end
   end
diff --git a/lib/page/term_table.rb b/lib/page/term_table.rb
index 0e9e0b4..4e0c28d 100644
--- a/lib/page/term_table.rb
+++ b/lib/page/term_table.rb
@@ -56,7 +56,7 @@ module Page
     end
 
     def people
-      @people ||= term.people.sort_by(&:sort_name).map do |person|
+      @people ||= term.people.sort_by{|e| [e.sort_name, e.name]}.map do |person|
         PersonCard.new(
           person: person,
           term:   term

What do you think?

tmtmtmtm · 2017-01-18T16:47:19Z

@mhl yes, I think stabilising the sort order is a good idea.

tmtmtmtm · 2017-01-21T11:17:29Z

lib/person_cards.rb

+  def top_identifiers
+    term.top_identifiers
+  end
+


Sorry for not catching this earlier, but I'm not convinced that these need to be public, do they?

Good point - thanks. I've force-pushed a version that changes those two methods to be private.

Oh, except probably memberships should be private too, grr

Ah, no - it's used by the term_table.erb view.

@tmtmtmtm I think / hope this is OK now!

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.

We can derive the proxy image URL from the country, legislature and person ID, and the former two can be found from the term.

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.

@tmtmtmtm

Thanks to @tmtmtmtm for pointing out that the people method can be much simpler, making the people_ids method unnecessary.

This is now directly available from the LegislativePeriod object (called 'term') so there's no need to pass it to the initializer.

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.

tmtmtmtm

👍

Woot! Thanks @mhl

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15603 January 17, 2017 20:04 Inactive

mhl added the 3 - WIP label Jan 17, 2017

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15603 January 17, 2017 21:20 Inactive

tmtmtmtm suggested changes Jan 17, 2017

View reviewed changes

mhl force-pushed the simplify-personcard branch from 6006c59 to b0275cc Compare January 18, 2017 12:26

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15603 January 18, 2017 12:26 Inactive

octopusinvitro mentioned this pull request Jan 18, 2017

ToDo list theyworkforyou/shineyoureye-sinatra#6

Closed

30 tasks

mhl force-pushed the simplify-personcard branch 2 times, most recently from 008cc48 to 3c27ddf Compare January 19, 2017 11:12

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15603 January 19, 2017 11:12 Inactive

mhl force-pushed the simplify-personcard branch from 3c27ddf to e1e3ce0 Compare January 19, 2017 11:22

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15603 January 19, 2017 11:22 Inactive

Upgrade everypolitician-popolo to the latest version

1c5c5dd

mhl force-pushed the simplify-personcard branch from e1e3ce0 to b486efa Compare January 19, 2017 13:38

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15603 January 19, 2017 13:38 Inactive

tmtmtmtm reviewed Jan 21, 2017

View reviewed changes

mhl added 6 commits January 26, 2017 10:53

PersonCard: It's no longer necessary to pass the proxy_image URL

1a4ee9d

We can derive the proxy image URL from the country, legislature and person ID, and the former two can be found from the term.

Remove the unnecessary LegislativePeriodExtension#people_ids method

881038c

Thanks to @tmtmtmtm for pointing out that the people method can be much simpler, making the people_ids method unnecessary.

PersonCard: get top_identifiers from term instead

595f5b6

This is now directly available from the LegislativePeriod object (called 'term') so there's no need to pass it to the initializer.

mhl force-pushed the simplify-personcard branch from b486efa to b2e507b Compare January 26, 2017 10:55

tmtmtmtm deployed to everypolitician-viewe-pr-15603 January 26, 2017 10:55 View deployment

tmtmtmtm approved these changes Jan 26, 2017

View reviewed changes

tmtmtmtm merged commit 8e9823e into master Jan 26, 2017

tmtmtmtm removed the 3 - WIP label Jan 26, 2017

tmtmtmtm deleted the simplify-personcard branch December 16, 2017 16:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify the instantiation of PersonCard objects #15603

Simplify the instantiation of PersonCard objects #15603

mhl commented Jan 17, 2017 •

edited by tmtmtmtm

Loading

tmtmtmtm left a comment

tmtmtmtm Jan 17, 2017

tmtmtmtm Jan 17, 2017

tmtmtmtm Jan 17, 2017

tmtmtmtm Jan 18, 2017

tmtmtmtm Jan 17, 2017

tmtmtmtm Jan 17, 2017

mhl Jan 18, 2017

mhl commented Jan 18, 2017

tmtmtmtm commented Jan 18, 2017

tmtmtmtm Jan 21, 2017

mhl Jan 26, 2017

mhl Jan 26, 2017

mhl Jan 26, 2017

tmtmtmtm left a comment

Simplify the instantiation of PersonCard objects #15603

Simplify the instantiation of PersonCard objects #15603

Conversation

mhl commented Jan 17, 2017 • edited by tmtmtmtm Loading

What does this do?

Why was this needed?

Relevant Issue(s)

Implementation notes

Screenshots

Notes to Reviewer

Notes to Merger

tmtmtmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhl commented Jan 18, 2017

tmtmtmtm commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmtmtmtm left a comment

Choose a reason for hiding this comment

mhl commented Jan 17, 2017 •

edited by tmtmtmtm

Loading