Add "House Download" page #15245

octopusinvitro · 2016-08-30T14:14:01Z

What does this do?

This PR creates dedicated download pages for a single-legislature-data download.

Why was this needed?

The per-country Download page is too cluttered and confusing, so we want to split it up into separate Download pages per legislature instead.

Relevant Issue(s)

Part of #13599. Part of #15256.

Implementation notes

Adds:

A new page class, HouseDownload
A route in the app.rb
A new template (copy-pasted the country download one, removed the loop on legislatures and updated the relevant stuff to make it a legislature download page)
Tests for what we send to the template from the new page

Screenshots

Notes to Reviewer

There is a lot of duplication here with the Country Download page (particularly the template). However, we will be removing that page before this one goes public, so it's OK to live with the duplication for a little while.

Notes to Merger

This should not go fully live (i.e. on EveryPolitician.org until all the tickets in the first section of #15256 are complete, however it is safe to merge before then as no other public pages will link to here, and so it will not yet get spidered.

tmtmtmtm · 2016-08-30T14:30:12Z

lib/page/house_download.rb

+
+    def legislative_periods
+      house.legislative_periods
+    end


All the tests pass if I change this implementation to house.legislative_periods.take(1), which probably isn't so good…

tmtmtmtm · 2016-08-31T11:38:17Z

lib/page/house_download.rb

+    def initialize(country:, house:, index:)
+      @country = country
+      @house   = house
+      @index   = index


Why do you need to instantiate with a @country here?

davewhiteland · 2016-08-31T11:46:38Z

views/house_download.erb

@@ -0,0 +1,198 @@
+<div class="page-section page-section--grey text-center">
+  <div class="container">
+    <h3>Download data for politicians in <%= @page.house.name %>, <%= @page.country.name %>.</h3>


I think this might be simpler as "Download data for politicians in this legislature." because

you've already got the house and country name immediately above this line anyway

it might an article (e.g., "...politicians in the Senate..." but I'm not sure that automatically works for every house name we have (it might))

So I suggest you simplify this with no variables in the string, and if it causes upset subsequently raise an issue to fix/refine it.

Yeah, as we did in the House page 👍

tmtmtmtm · 2016-08-31T14:49:48Z

Generally looking good, though I'm a little concerned that we have a "Want politicians who aren’t from [this country]?" section, but don't explain what to do if you want politicians from a different house in the same country. It doesn't necessarily need to be solved in this PR, but if it's going to happen separately we should at least link to the issue for resolving that. (Likewise for un-hiding the terms.)
--> Update: issue covering this: #15259

@davewhiteland perhaps this PR is a good test case for rewriting the description in the style that the new PR template should take?

tmtmtmtm · 2016-09-01T07:45:42Z

Review summary

Meets acceptance criteria?

This does not yet meet the criteria for closing One download page per legislature #13599 or release house and house/download pages #15256. It is merely a step towards each of them. But other than some things that are already scheduled to be cleaned up in other tickets, it's doing the right thing.

UX

Slight issue with getting to equivalent page for other legislatures in this country, but that can be done in another PR.

Architecture

one new class — follows existing patterns with no new interface/naming decisions.
new Page is instantiated with suitable (and minimal) arguments.

Tests

all new public methods tested
existing tests all still pass

Style

rubocop clean
- though adds 7 new "line too long" warnings
no warnings from flay
acceptable flog scores
- Highest = 10.2 for HouseDownload#subject test set-up

Git/GitHub

Good PR title
Good PR description
fixup!s squashed

@davewhiteland I've rewritten the Issue description more in line with the style I think we should move to, and cross-linked the things that are now more clearly able to be done post-merge, than as pre-requisites to merge. So I think this is good to go! 👍

@octopusinvitro If you want squash down the fixup! commit before merging, go ahead. Otherwise, let's

This template was based on the country download template. It allows to download data for a single legislature.

The terms information describe was misleading as it was testing for a single term. This commit adds a test for the total number of terms and tests the information of two different terms, 113 and 114

Removed because they are already in the main header

Removed because it is already in the main heading

After rebasing master onto this branch and running the tests, some were failing due to the recent simplification of the signature of the term_table_url method. This commit updates the template to use the new signature.

This was omitted when migrating download pages from country to house level. Refer to everypolitician#15245, everypolitician#15255.

tmtmtmtm requested a deployment to everypolitician-viewe-pr-15245 August 30, 2016 14:14 Pending

octopusinvitro added 4 - Done (Ready for Review) 3 - WIP and removed 4 - Done (Ready for Review) labels Aug 30, 2016

octopusinvitro assigned tmtmtmtm and davewhiteland Aug 30, 2016

tmtmtmtm requested a deployment to everypolitician-viewe-pr-15245 August 30, 2016 14:14 Pending

octopusinvitro unassigned davewhiteland Aug 30, 2016

tmtmtmtm requested a deployment to everypolitician-viewe-pr-15245 August 30, 2016 14:26 Pending

tmtmtmtm requested a deployment to everypolitician-viewe-pr-15245 August 30, 2016 14:27 Pending

tmtmtmtm reviewed Aug 30, 2016
View reviewed changes

tmtmtmtm assigned octopusinvitro and unassigned tmtmtmtm Aug 30, 2016

tmtmtmtm requested a deployment to everypolitician-viewe-pr-15245 August 30, 2016 15:12 Pending

tmtmtmtm requested a deployment to everypolitician-viewe-pr-15245 August 30, 2016 15:13 Pending

octopusinvitro assigned tmtmtmtm Aug 30, 2016

octopusinvitro added 4 - Done (Ready for Review) and removed 3 - WIP labels Aug 30, 2016

tmtmtmtm mentioned this pull request Aug 30, 2016

House page #15246

Merged

octopusinvitro removed their assignment Aug 31, 2016

tmtmtmtm reviewed Aug 31, 2016
View reviewed changes

tmtmtmtm assigned davewhiteland Aug 31, 2016

davewhiteland reviewed Aug 31, 2016
View reviewed changes

tmtmtmtm requested a deployment to everypolitician-viewe-pr-15245 August 31, 2016 11:54 Pending

tmtmtmtm requested a deployment to everypolitician-viewe-pr-15245 August 31, 2016 12:10 Pending

tmtmtmtm requested a deployment to everypolitician-viewe-pr-15245 August 31, 2016 12:11 Pending

This was referenced Aug 31, 2016

Link to house/download #15255

Merged

TODO w/c 2016-08-30 everypolitician/everypolitician#492

Closed

davewhiteland mentioned this pull request Aug 31, 2016

release house and house/download pages #15256

Closed

12 tasks

tmtmtmtm changed the title ~~House download page~~ Add "House Download" page Sep 1, 2016

octopusinvitro force-pushed the house-download branch from e465b44 to b535df1 Compare September 1, 2016 10:31

tmtmtmtm requested a deployment to everypolitician-viewe-pr-15245 September 1, 2016 10:31 Pending

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15245 September 1, 2016 10:31 Inactive

octopusinvitro added 11 commits September 1, 2016 12:02

Add a page class for a single legislature download

9388e9c

Add a route to load the single house download page

ba898e2

Add test for the single legislature template

7e4127f

Create a new template for the legislature download page

8942077

This template was based on the country download template. It allows to download data for a single legislature.

Make the headings tests read more naturally

4c07b7f

Add a test for the download of the first term file

5bb0052

Add a test for the total number of terms displayed

fe8abf8

The terms information describe was misleading as it was testing for a single term. This commit adds a test for the total number of terms and tests the information of two different terms, 113 and 114

Remove country as a constructor argument in HouseDownload

e93cd03

Remove the house and country name from the banner

a7f5dc5

Removed because they are already in the main header

Remove the house name from the legislature heading

8b098c1

Removed because it is already in the main heading

Update to the new signature of term_table_url

2413c8f

After rebasing master onto this branch and running the tests, some were failing due to the recent simplification of the signature of the term_table_url method. This commit updates the template to use the new signature.

octopusinvitro force-pushed the house-download branch from b535df1 to 2413c8f Compare September 1, 2016 11:07

tmtmtmtm temporarily deployed to everypolitician-viewe-pr-15245 September 1, 2016 11:07 Inactive

tmtmtmtm merged commit 40dacf4 into master Sep 1, 2016

octopusinvitro deleted the house-download branch September 1, 2016 13:12

tmtmtmtm removed the 4 - Done (Ready for Review) label Sep 10, 2016

wfdd added a commit to wfdd/viewer-sinatra that referenced this pull request Sep 26, 2016

Update download page URL on term table

d31c38e

This was omitted when migrating download pages from country to house level. Refer to everypolitician#15245, everypolitician#15255.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "House Download" page #15245

Add "House Download" page #15245

octopusinvitro commented Aug 30, 2016 •

edited by tmtmtmtm

Loading

tmtmtmtm Aug 30, 2016

tmtmtmtm Aug 31, 2016

davewhiteland Aug 31, 2016 •

edited

Loading

octopusinvitro Aug 31, 2016

tmtmtmtm commented Aug 31, 2016 •

edited by octopusinvitro

Loading

tmtmtmtm commented Sep 1, 2016 •

edited by octopusinvitro

Loading

Add "House Download" page #15245

Add "House Download" page #15245

Conversation

octopusinvitro commented Aug 30, 2016 • 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 Aug 30, 2016

Choose a reason for hiding this comment

tmtmtmtm Aug 31, 2016

Choose a reason for hiding this comment

davewhiteland Aug 31, 2016 • edited Loading

Choose a reason for hiding this comment

octopusinvitro Aug 31, 2016

Choose a reason for hiding this comment

tmtmtmtm commented Aug 31, 2016 • edited by octopusinvitro Loading

tmtmtmtm commented Sep 1, 2016 • edited by octopusinvitro Loading

Review summary

Meets acceptance criteria?

UX

Architecture

Tests

Style

Git/GitHub

octopusinvitro commented Aug 30, 2016 •

edited by tmtmtmtm

Loading

davewhiteland Aug 31, 2016 •

edited

Loading

tmtmtmtm commented Aug 31, 2016 •

edited by octopusinvitro

Loading

tmtmtmtm commented Sep 1, 2016 •

edited by octopusinvitro

Loading