Hxl stats view 145 147 #180

leungant · 2017-06-04T19:24:07Z

Change description:

Addresses #145 and #147:

http://.../hxlstats and http://.../hxlstats.json now show array of arrays conforming to Google spreadsheet

Any pointers/assistance much appreciated as I am a bit unfamiliar with the testing requirements.

Instructions:

rake db:migrate

or

rake db:migrate:up VERSION=20170601162837

will bring VERSION up to 20170601162837

The view is available at http://.../hxlstats.json

Sample output

.json:

[["Emotional State", "Stage of journey", "Country drawn in", "Total children affected", "Children who identify as female", "Children who identify as male", "Children who identify as neither female nor male", "Children between the ages of 5-12", "Children between the ages of 13-18", "Children under 5 years old", "Older than 18 years old"],
 ["#impact+indicator+code", "#affected+children", "#country+code", "#affected+children+total", "#affected+children+female", "#affected+children+male", "#affected+children+indicator", "#affected+children+age_5_12", "#affected+children+age_13_18", "#affected+children+age_under5", "#affected+children+age_18plus", "#affected+children+age_unknown"], 
[1, "At home", "AF", 1, 0, 0, 1, 0, 0, 1, 0, 1], 
[3, "At home", "AF", 2, 0, 1, 1, 0, 0, 2, 0, 2], 
[3, "In temporary shelter", "AF", 1, 0, 1, 0, 0, 0, 1, 0, 2], 
[3, "At home", "FR", 1, 0, 0, 1, 1, 0, 0, 0, 1], 
[2, "In temporary shelter", "AF", 1, 0, 1, 0, 1, 0, 0, 0, 1]]

krissy

Looking great so far @leungant, and local tests of results are exciting! 🕺

Output is missing a single but quite key row, i.e. the HXL hashtags, but that should be a quick fix!

Also some notes on how we can refactor this a bit and make you fight with Rubocop less. Please bug me if you'd like to go through anything together, pair or delegate some tasks to me. Thanks again for jumping on this so quick! :D

krissy · 2017-06-10T16:09:05Z

app/assets/javascripts/hxlstats.coffee

@@ -0,0 +1,3 @@
+# Place all the behaviors and hooks related to the matching controller here.


Pretty sure these were scaffolded so we can get rid of any unused files. Will mark any below we can remove with an amazing hashtag like #ScaffoldScrap! ✌️

krissy · 2017-06-10T16:09:23Z

app/assets/stylesheets/hxlstats.scss

@@ -0,0 +1,3 @@
+// Place all the styles related to the hxlstats controller here.


#ScaffoldScrap

krissy · 2017-06-10T16:09:51Z

app/controllers/hxlstats_controller.rb

@@ -0,0 +1,69 @@
+class HxlstatsController < ApplicationController
+  # Class to collect and aggregate data to parse to the HXL proxy/endpoint
+  def new


#ScaffoldScrap the new, create and index methods

krissy · 2017-06-10T16:10:04Z

app/decorators/hxlstats_decorator.rb

@@ -0,0 +1,13 @@
+class HxlstatsDecorator < Draper::Decorator


#ScaffoldScrap

krissy · 2017-06-10T16:10:11Z

app/helpers/hxlstats_helper.rb

@@ -0,0 +1,2 @@
+module HxlstatsHelper


#ScaffoldScrap

krissy · 2017-06-10T16:20:24Z

app/views/hxlstats/show.html.erb

@@ -0,0 +1,11 @@
+<%# country moodrating stage_of_journey count gender1 gender2 gender3 age1 age2 age3 age4 %> 


Can remove comments

krissy · 2017-06-10T16:45:25Z

app/models/hxl_stats_view.rb

+    true
+  end
+
+  def self.to_csv # Not actually used! But could be convenient for data analysts. was %w{ } but rubocop!


Let's remove this one. Ideally for any of our HDX integrations, this app will only act as an input source for the HXL Proxy, and any exports / filtering / analysis will occur in the HXL Proxy itself. Better to keep things minimal rather than keep methods in that most likely won't be used but would still need maintenance with view changes etc.

krissy · 2017-06-10T16:46:16Z

app/views/hxlstats/index.html.erb

@@ -0,0 +1,23 @@
+<h1> hello </h1>


Not sure if this is used for anything? If not, we can remove too.

krissy · 2017-06-10T17:09:00Z

app/controllers/hxlstats_controller.rb

+
+  # rubocop:disable MethodLength
+  def show
+    # Prepare output results array


Thanks for working all this grouping logic out! Tested locally and so exciting to finally have an endpoint with data.
👏

A couple of suggestions for cleaning it up a little bit, keeping Rubocop happy and also making testing a bit easier:

Move everything from line 12 to 59, i.e. creating the results array, out of the controller. One suggestion is making it a public class method in the HxlStatsView model, like results_by_emotional_state. Or another option is to create a new service object / Plain Old Ruby Object (PORO) like drawings_by_emotional_state_query in a new directory like app/models/queries, to save from bloating the model also.

Once the logic is extracted, you can extract each block below into its own private method, which will be easier to read / maintain / identify the purpose of each block by explicit method naming - this last bit also makes it less necessary to feel like you need to prefix comments on each block, as the methods should tell the tale. AND this will make you fight with Rubocop less and remove the need for # rubocop:disable MethodLength I see stuck up there :)

Once the logic is extracted outside of the controller, testing becomes a bit clearer/easier. For controller specs (not detrimental if these are missing btw), you can set up just a couple of tests that focus on asserting the response of the show method (i.e. returns html if normal request, returns data if json specified) and just stub the value of the results generation method. Then you can focus on testing the actual results generation in the model / service object, using FactoryGirl for HxlStatsView.

As always, buzz or let me know if you'd like to go through this or pair!

Mostly done, in the most minimal way possible ;) thanks for the comments!

krissy · 2017-06-10T17:10:30Z

app/controllers/hxlstats_controller.rb

+  def show
+    # Prepare output results array
+    @results = []
+    @results.append(["Emotional State", "Stage of journey", "Country drawn in", "Total children affected",


As well as these headings, we need another row with the HXL tagged headings (e.g. #impact +indicator +code) as per the spreadsheet. Both the headings and HXL headings can probably live as constants in the HxlStatsView model, or service object as suggested above.

When to have spaces in the tags and when not?

Deferring named constants to when we start using in multiple places.

…ethod from class

leungant · 2017-06-11T10:15:14Z

Appreciate any further thoughts!

… swap around

krissy

Great! Have added a few more comments. Let me know if you'd like to go over anything. 🍍

krissy · 2017-06-24T15:14:37Z

app/views/hxlstats/new.html.haml

@@ -0,0 +1,2 @@
+%h1 Hxlstats#new


We can remove this too as we don't have a new route set up anymore.

krissy · 2017-06-24T15:20:47Z

app/models/hxl_stats_view.rb

@@ -0,0 +1,109 @@
+HXLSTATS_COLUMN_HEADERS =


Any reason line 1 - 66 are outside of the class? Let's pop them back in. The constants (in this case HXLSTATS_COLUMN_HEADERS and HXL_STATS_TAGS) usually would live right at the top of the class (but inside it), and the methods get_counts_by_gender and get_counts_by_age can be private methods within the class. I.e. stick them under the keyword private.

krissy · 2017-06-24T15:27:56Z

app/models/hxl_stats_view.rb

+def get_counts_by_gender(hxlstatsmajorgroup)
+  # Group by gender and aggregate
+  gender_totals = Hash.new(0)
+  @hxlstatsgroupsgender = hxlstatsmajorgroup.group_by(&:gender) # shorthand for { |hxlstat| hxlstat.gender }


No need for an instance variable here as it will only be used locally within the method. So @hxlstatsgroupsgender -> hxlstatsgroupsgender

krissy · 2017-06-24T15:28:12Z

app/models/hxl_stats_view.rb

+# rubocop:disable MethodLength
+def get_counts_by_age(hxlstatsmajorgroup)
+  # Processing Age aggregations
+  @hxlstatsgroupsage = hxlstatsmajorgroup.group_by(&:age) # shorthand for { |hxlstat| hxlstat.age }


No need for an instance variable here as it will only be used locally within the method. So @hxlstatsgroupsage -> hxlstatsgroupsage

krissy · 2017-06-24T15:30:16Z

app/models/hxl_stats_view.rb

+
+  def self.hxl_stats_counts
+    results = []
+    @hxlstats = HxlStatsView.all


Same re replacing instance variable with local variable - @hxlstats -> hxlstats

krissy · 2017-06-24T15:30:27Z

app/models/hxl_stats_view.rb

+    @hxlstats = HxlStatsView.all
+
+    # First group by major categories, before aggregating independently for gender and age and recombining
+    @hxlstatsgroups = @hxlstats.group_by { |hxlstat| [hxlstat.mood_rating, hxlstat.stage_of_journey, hxlstat.country] }


Same re replacing instance variable with local variable - @hxlstatsgroups -> hxlstatsgroups

krissy · 2017-06-24T15:35:34Z

app/models/hxl_stats_view.rb

+  end
+
+  def self.results_by_emotional_state
+    @results = []


Same re replacing instance variable with local variable - @results -> results.

krissy · 2017-06-24T15:44:57Z

app/models/hxl_stats_view.rb

+    true
+  end
+
+  @gender_lookup = { 'male' => 0,


We already conveniently have the gender mapping available as an enum in the Drawing model. We can reference that in the hxl_stats_counts method rather than create a new hash here to avoid repetition.

I.e.

Drawing.genders # Response => {"not_specified"=>0, "female"=>1, "male"=>2, "other"=>3} Drawing.genders["female"] # Response => 1

What value does hxl_stats_counts return for drawings with 'other' i.e. 3? We might need to update the logic so anything with 0 or 3 falls into 'other' for now.

krissy · 2017-06-25T12:32:18Z

app/models/hxl_stats_view.rb

+     "#affected+children+age_under5",
+     "#affected+children+age_18plus"].freeze
+
+  private_class_method :get_counts_by_gender


Ah the 'private class method' debacle! Yeah, wish this was a bit less painful in Rails.

We can use the private_class_method option, however this needs to be declared after the methods themselves or Rails complains. A second, think less PITA option, is to use: class << self around all of the methods. It's usually clearer to use the def self.method_name style to define class methods, however when all of the methods within a class are public, wrapping everything in class << self is fine and more readable. Also, this way you can use the private keyword as normal.

I.e.

class << self def results_by_emotional_state ... end private def hxl_stats_counts ... end def get_counts_by_gender(hxlstatsmajorgroup) ... end def get_counts_by_age(hxlstatsmajorgroup) ... end end

krissy · 2017-06-25T12:34:10Z

app/models/hxl_stats_view.rb

+    gender_totals # hash of key gender to v counts
+  end
+
+  # rubocop:disable MethodLength


Rubocop doesn't need this methodlength exception for this method.

Also, you can run rubocop locally without having to wait for CI to build. Just cd into the root of the project and type rubocop. Or you can do it for a specific file by typing the path after it.

krissy · 2017-06-25T12:35:16Z

db/migrate/20170601162837_create_hxl_stats_view.rb

@@ -0,0 +1,24 @@
+class CreateHxlStatsView < ActiveRecord::Migration
+  def up 
+    execute <<-SQL


Run locally, some rubocop bits:

db/migrate/20170601162837_create_hxl_stats_view.rb:2:9: C: Trailing whitespace detected. def up ^ db/migrate/20170601162837_create_hxl_stats_view.rb:5:33: C: Trailing whitespace detected. SELECT d.id AS drawing_id, ^ db/migrate/20170601162837_create_hxl_stats_view.rb:8:29: C: Trailing whitespace detected. o.id AS org_id, ^^ db/migrate/20170601162837_create_hxl_stats_view.rb:11:29: C: Trailing whitespace detected. users u, ^ db/migrate/20170601162837_create_hxl_stats_view.rb:24:1: C: 1 trailing blank lines detected.

…unknown age groups.

krissy · 2017-08-28T11:22:56Z

@leungant I reviewed the last couple of changes and QA'ed them locally. Had to add an extra 'unknown ages' column to the spreadsheet and HXL logic as age isn't compulsory in the form. All else looking fine so will get this merged!

…got time fo dat.

leungant added 2 commits June 4, 2017 21:08

HXL stats db schema changes

505fc25

#147 related controller/view items for HXL stats reporting

024f664

leungant added backend Ruby/Rails SQL labels Jun 4, 2017

leungant requested review from steverabino and removed request for steverabino June 4, 2017 19:24

leungant added 4 commits June 4, 2017 21:34

Travis wants format!

4d54556

Oh wow Rubocop

f31ddf3

Oh wow Rubocop

cc492d2

Oh wow Rubocop

a7f42be

krissy reviewed Jun 10, 2017

View reviewed changes

leungant added 11 commits June 10, 2017 22:13

Krissy code review

b6709ba

Krissy review

4fa5948

Krissy review

f9c11db

Krissy review

5abb632

KrisView

6a0542b

rubocop

e379e8a

rubocop

2f6449a

rubocop

058e291

rubocop

0cbc42f

...

a0ea11a

Separate non-class dependent (though very data structure dependent) m…

bfb52b7

…ethod from class

leungant added 3 commits June 11, 2017 14:24

Fixed bug where genders are actually m-0 f-1 other-2 in db, needed to…

0447c5d

… swap around

Fixed indicator tag for "other gender"

15c4541

rubocop!

96cc7b9

leungant added status: please review status: ready to merge labels Jun 13, 2017

krissy reviewed Jun 24, 2017

View reviewed changes

leungant added 2 commits June 24, 2017 23:34

krissy review, use Drawing.gender for gender mapping

9e46293

rubocop

40234d2

krissy reviewed Jun 25, 2017

View reviewed changes

leungant added 2 commits July 15, 2017 14:48

Krissy's input for rubocop errors

5766dc6

Clean up some messy comments

93be3a7

leungant removed the status: ready to merge label Jul 25, 2017

[145, 147] Small QA/code review tweaks. Need extra column to capture …

b2353dc

…unknown age groups.

Disable some rubocop checks for CI, because volunteer projects ain't …

ac24ae8

…got time fo dat.

krissy merged commit 5775c18 into master Aug 28, 2017

krissy deleted the hxl_stats_view-145-147 branch August 28, 2017 11:40

		@@ -0,0 +1,3 @@
		# Place all the behaviors and hooks related to the matching controller here.

		@@ -0,0 +1,3 @@
		// Place all the styles related to the hxlstats controller here.

		@@ -0,0 +1,11 @@
		<%# country moodrating stage_of_journey count gender1 gender2 gender3 age1 age2 age3 age4 %>

Hxl stats view 145 147 #180

Hxl stats view 145 147 #180

Conversation

leungant commented Jun 4, 2017 • edited by krissy Loading

Change description:

Instructions:

Sample output

krissy 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

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

leungant Jun 10, 2017 • edited Loading

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

leungant Jun 10, 2017 • edited Loading

Choose a reason for hiding this comment

leungant commented Jun 11, 2017 • edited Loading

krissy left a comment • edited by leungant Loading

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

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

krissy commented Aug 28, 2017

leungant commented Jun 4, 2017 •

edited by krissy

Loading

leungant Jun 10, 2017 •

edited

Loading

leungant Jun 10, 2017 •

edited

Loading

leungant commented Jun 11, 2017 •

edited

Loading

krissy left a comment •

edited by leungant

Loading