Improve stats interface #1932
Closed
Improve stats interface #1932
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We would now like to differenciate between 70-year-old people and 90-year-old people.
This way we can easily see the h3 tag's parent is the h2 tag.
We try to make the method return data which is easier to handle in the view.
The rest of the `before` block still uses instance variables, but at least the rest of the file doesn't use instance variables anymore.
So related methods are on the same line.
Naming it "Dropdown" was misleading.
Turbolinks doesn't get on well with Foundation's Sticky, and so we need to manually trigger the event on Turbolinks' `page:load`.
So it can be reused in advanced statistics.
We're going to use it in many places, so removing duplication is useful.
This way we don't need to manually define the width we think the image we insert in the `::before` pseudoclass is going to take.
The name was confusing because it seemed to return a list of age groups.
This class improve tables layout on mobile screen sizes.
It was still using "web" as origin.
The factories were creating strange database relations:
* The voter belonged to a poll, to a booth and to an officer
* The booth belonged to a different poll
* The officer belonged to a different booth
The code uses an unusual syntax because the following code:
association :booth_assignment,
factory: :poll_booth_assignment,
poll: poll
Would generate the following error:
ActiveRecord::AssociationTypeMismatch: Poll(#46976420451940) expected,
got FactoryBot::Declaration::Implicit
Adding the option to assign a poll to a poll recount factory meant we didn't need to create so much data. Also note we're removing the `create(:poll_voter, origin: "booth")` code, since it isn't used in the stats calculations.
While we already had "one test to rule all stats", testing each method individually makes reading, adding and changing tests easier. Note we need to make all methods being tested public. We could also test them using methods like `stats.generate[:total_valid_votes]` instead of `stats.total_valid_votes`, but then the tests would be more difficult to read.
We didn't use metaprogramming from the start because the `null_percentage_web` method had a particular behaviour. However, the behaviour (due to a typo) didn't really matter because there are no null web votes, and so the `null_percentage_web` is always zero.
So far we don't know about implemenation details.
Note we currently don't have a way to get the votes by mail, so that section is always blank.
It was only defined in the custom routes file.
So now stats by gender and age are replaced by shared participation stats (which also includes stats by district), reusing the code already used in poll stats, and advanced statistics (which used to be at the top of the page) are now displayed after partipation stats.
We were using custom translations, but now this code is going to be included in the main CONSUL repository.
Data equalizer was used to show a border to separate sidebar from the content. Now is the same with only css avoiding use js.
houndci-bot
reviewed
Mar 18, 2019
| end | ||
|
|
||
| end | ||
|
|
||
| context "#headings" do | ||
|
|
||
| it "returns headings data" do | ||
| heading_stats = @stats[:headings][@heading.id] | ||
| heading_stats = stats[:headings][@heading.id] |
There was a problem hiding this comment.
RSpec/InstanceVariable: Use let instead of an instance variable. (http://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/InstanceVariable)
| end | ||
|
|
||
| let(:stats) { Budget::Stats.new(@budget).generate } |
There was a problem hiding this comment.
RSpec/InstanceVariable: Use let instead of an instance variable. (http://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/InstanceVariable)
| end | ||
| second_total = Poll::Answer.where(answer: "En blanco", question: poll.questions.second).pluck(:author_id).count | ||
| second_total -= (Poll::Answer.where(answer: poll.questions.first.question_answers.where(given_order: 1).first.title, question: poll.questions.first).pluck(:author_id) & Poll::Answer.where(answer: "En blanco", question: poll.questions.second).pluck(:author_id)).uniq.count | ||
| second_total -= (Poll::Answer.where(answer: poll.questions.first.question_answers.where(given_order: 2).first.title, question: poll.questions.first).pluck(:author_id) & Poll::Answer.where(answer: "En blanco", question: poll.questions.second).pluck(:author_id)).uniq.count |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [275/100] (https://github.com/bbatsov/ruby-style-guide#80-character-limits)
| calculate_percentage(total_web_null, total_web_valid) | ||
| end | ||
| second_total = Poll::Answer.where(answer: "En blanco", question: poll.questions.second).pluck(:author_id).count | ||
| second_total -= (Poll::Answer.where(answer: poll.questions.first.question_answers.where(given_order: 1).first.title, question: poll.questions.first).pluck(:author_id) & Poll::Answer.where(answer: "En blanco", question: poll.questions.second).pluck(:author_id)).uniq.count |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [275/100] (https://github.com/bbatsov/ruby-style-guide#80-character-limits)
| def null_percentage_web | ||
| calculate_percentage(total_web_null, total_web_valid) | ||
| end | ||
| second_total = Poll::Answer.where(answer: "En blanco", question: poll.questions.second).pluck(:author_id).count |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [116/100] (https://github.com/bbatsov/ruby-style-guide#80-character-limits)
| } | ||
|
|
||
| &.mail::before { | ||
| content: image-url('stats_mail.png'); |
| } | ||
|
|
||
| &.booth::before { | ||
| content: image-url('stats_booth.png'); |
| } | ||
|
|
||
| &.web::before { | ||
| content: image-url('stats_web.png'); |
| } | ||
|
|
||
| &.mail::before { | ||
| content: image-url('stats_mail.png'); |
| } | ||
|
|
||
| &.booth::before { | ||
| content: image-url('stats_booth.png'); |
|
I changed the compare branch instead of the base branch while opening this pull request 😅. Wrong branch! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Objectives
Visual Changes
Before these changes:
After these changes:
Does this PR need a Backport to CONSUL?
Yes, backport when backporting everything related to stats.