Refactor variance chart to make view responsible for sorting measures

app/controllers/dashboard_controller.rb

@@ -1,24 +1,16 @@
 class DashboardController < SqmApplicationController
 
   def index
-    @measure_graph_row_presenters = measure_ids
+    @variance_chart_row_presenters = Measure.all.map(&method(:presenter_for_measure))
-                                      .map { |measure_id| Measure.find_by_measure_id measure_id }
-                                      .map(&method(:presenter_for_measure))
-                                      .sort
-                                      .reverse
 
     @category_presenters = SqmCategory.sorted.map { |sqm_category| CategoryPresenter.new(category: sqm_category) }
   end
 
   private
 
-  def measure_ids
-    Measure.all.map(&:measure_id)
-  end
-
   def presenter_for_measure(measure)
     score = SurveyItemResponse.score_for_measure(measure: measure, school: @school, academic_year: @academic_year)
 
-    MeasureGraphRowPresenter.new(measure: measure, score: score)
+    VarianceChartRowPresenter.new(measure: measure, score: score)
   end
 end

app/helpers/variance_helper.rb

@@ -42,12 +42,4 @@ module VarianceHelper
   def zone_width_percentage
     100.0/zones.size
   end
-
-  def measures_with_insufficient_data(presenters:)
-    presenters.filter { |presenter| presenter.score == nil }
-  end
-
-  def measures_with_sufficient_data(presenters:)
-    presenters.filter { |presenter| presenter.score != nil }
-  end
 end

app/presenters/variance_chart_row_presenter.rb

@@ -1,4 +1,4 @@
-class MeasureGraphRowPresenter
+class VarianceChartRowPresenter
   include Comparable
 
   attr_reader :score
@@ -49,7 +49,7 @@ class MeasureGraphRowPresenter
   end
 
   def <=>(other_presenter)
-    order <=> other_presenter.order
+    other_presenter.order <=> order
   end
 
   private

app/views/dashboard/_variance_chart.html.erb

@@ -1,8 +1,10 @@
-<% unless measures_with_insufficient_data(presenters: presenters).empty? %>
+<% displayed_presenters = presenters.filter { |p| p.sufficient_data? }.sort %>
-<p class="caption mb-5">Note: The following measures are not displayed due to limited availability of school admin data and/or low survey response rates: <%= measures_with_insufficient_data(presenters: presenters).map(&:measure_name).join('; ') %>.</p>
+<% not_displayed_presenters = presenters - displayed_presenters %>
+
+<% if not_displayed_presenters.present? %>
+  <p class="caption mb-5">Note: The following measures are not displayed due to limited availability of school admin data and/or low survey response rates: <%= not_displayed_presenters.map(&:measure_name).join('; ') %>.</p>
 <% end %>
 
-<% displayed_presenters = measures_with_sufficient_data(presenters: presenters) %>
 <svg width="100%" height=<%= graph_height(displayed_presenters.size) %> xmlns="http://www.w3.org/2000/svg">
 
   <filter id="inset-shadow" x="-50%" y="-50%" width="200%" height="200%">
@@ -25,7 +27,7 @@
     x="<%= label_width_percentage %>%"
     y="0"
     width="<%= graph_width_percentage %>%"
-    height=<%= graph_background_height(number_of_rows: displayed_presenters.size) %>
+    height="<%= graph_background_height(number_of_rows: displayed_presenters.size) %>"
     filter="url(#inset-shadow)"
   >
     <g id="zone-headings">
@@ -87,7 +89,7 @@
           x="<%= presenter.x_offset %>"
           y="<%= index * measure_row_height + (measure_row_height - measure_row_bar_height) / 2 %>"
           width="<%= presenter.bar_width %>"
-          height=<%= measure_row_bar_height %>
+          height="<%= measure_row_bar_height %>"
           data-for-measure-id="<%= presenter.measure_id %>"
           stroke="none"
           />

app/views/dashboard/index.html.erb

@@ -75,5 +75,5 @@
 <div class="card">
   <h2 class="sub-header-2 mb-4">Distance From Benchmark</h2>
 
-  <%= render partial: "variance_graph", locals: { presenters: @measure_graph_row_presenters} %>
+  <%= render partial: "variance_chart", locals: { presenters: @variance_chart_row_presenters} %>
 </div>

spec/features/school_dashboard_feature_spec.rb

@@ -142,16 +142,6 @@ def district_admin_sees_district_change
   expect(page).to have_current_path(expected_path)
 end
 
-def district_admin_sees_measures_in_correct_order
-  def index_of_row_for(measure_id:)
-    expect(page).to have_css("[data-for-measure-id='#{measure_id}']")
-    page.all('rect.measure-row-bar').find_index { |item| item['data-for-measure-id'] == "#{measure_id}" }
-  end
-
-  expect(index_of_row_for(measure_id: '2A-i')).to be < index_of_row_for(measure_id: '1A-i')
-  expect(index_of_row_for(measure_id: '1A-i')).to be < index_of_row_for(measure_id: '4C-i')
-end
-
 def district_admin_sees_dashboard_content
   expect(page).to have_select('academic-year', selected: '2020 – 2021')
   expect(page).to have_select('district', selected: 'Winchester')
@@ -163,8 +153,6 @@ def district_admin_sees_dashboard_content
   district_admin_sees_problem_solving_emphasis
 
   page.assert_selector('.measure-row-bar', count: 5)
-
-  district_admin_sees_measures_in_correct_order
 end
 
 def district_admin_sees_browse_content

spec/presenters/variance_chart_row_presenter_spec.rb

@@ -1,6 +1,6 @@
 require 'rails_helper'
 
-RSpec.describe MeasureGraphRowPresenter do
+describe VarianceChartRowPresenter do
 
   let(:watch_low_benchmark) { 2.9 }
   let(:growth_low_benchmark) { 3.1 }
@@ -18,7 +18,7 @@ RSpec.describe MeasureGraphRowPresenter do
   }
 
   let(:presenter) {
-    MeasureGraphRowPresenter.new measure: measure, score: score
+    VarianceChartRowPresenter.new measure: measure, score: score
   }
 
   shared_examples_for 'measure_name' do
@@ -124,22 +124,18 @@ RSpec.describe MeasureGraphRowPresenter do
   end
 
   context 'sorting scores' do
-    it 'selects a shorter bar width before a longer bar' do
+    it 'selects a longer bar before a shorter bar for measures in the approval/ideal zones' do
-      this_presenter = MeasureGraphRowPresenter.new measure: measure, score: 3.7
+      approval_presenter = VarianceChartRowPresenter.new measure: measure, score: 3.7
-      other_presenter = MeasureGraphRowPresenter.new measure: measure, score: 4.4
+      ideal_presenter = VarianceChartRowPresenter.new measure: measure, score: 4.4
-      expect(this_presenter <=> other_presenter).to be < 0
+      expect(ideal_presenter <=> approval_presenter).to be < 0
-    end
+      expect([approval_presenter, ideal_presenter].sort).to eq [ideal_presenter, approval_presenter]
-
+    end
-    it 'selects a warning bar before a ideal bar' do
+
-      this_presenter = MeasureGraphRowPresenter.new measure: measure, score: 1.0
+    it 'selects a warning bar below a ideal bar' do
-      other_presenter = MeasureGraphRowPresenter.new measure: measure, score: 5.0
+      warning_presenter = VarianceChartRowPresenter.new measure: measure, score: 1.0
-      expect(this_presenter <=> other_presenter).to be < 0
+      ideal_presenter = VarianceChartRowPresenter.new measure: measure, score: 5.0
-    end
+      expect(warning_presenter <=> ideal_presenter).to be > 0
-
+      expect([warning_presenter, ideal_presenter].sort).to eq [ideal_presenter, warning_presenter]
-    it 'selects an ideal bar after a warning bar' do
-      this_presenter = MeasureGraphRowPresenter.new measure: measure, score: 4.4
-      other_presenter = MeasureGraphRowPresenter.new measure: measure, score: 1.0
-      expect(this_presenter <=> other_presenter).to be > 0
     end
   end
 end

spec/views/dashboard/index.html.erb_spec.rb

@@ -9,18 +9,18 @@ describe 'dashboard/index.html.erb' do
 
   before :each do
     assign :category_presenters, []
-    assign :measure_graph_row_presenters, measure_graph_row_presenters
+    assign :variance_chart_row_presenters, variance_chart_row_presenters
 
     render
   end
 
   context 'when some presenters have a nil score' do
-    let(:measure_graph_row_presenters) {
+    let(:variance_chart_row_presenters) {
       [
-        MeasureGraphRowPresenter.new(measure: support_for_teaching, score: nil),
+        VarianceChartRowPresenter.new(measure: support_for_teaching, score: nil),
-        MeasureGraphRowPresenter.new(measure: create(:measure, name: 'Should Be Displayed', measure_id: 'should-be-displayed'), score: rand),
+        VarianceChartRowPresenter.new(measure: create(:measure, name: 'Should Be Displayed', measure_id: 'should-be-displayed'), score: rand),
-        MeasureGraphRowPresenter.new(measure: effective_leadership, score: nil),
+        VarianceChartRowPresenter.new(measure: effective_leadership, score: nil),
-        MeasureGraphRowPresenter.new(measure: professional_qualifications, score: nil)
+        VarianceChartRowPresenter.new(measure: professional_qualifications, score: nil)
       ]
     }
 
@@ -40,9 +40,9 @@ describe 'dashboard/index.html.erb' do
   end
 
   context 'when all the presenters have a non-nil score' do
-    let(:measure_graph_row_presenters) {
+    let(:variance_chart_row_presenters) {
       [
-        MeasureGraphRowPresenter.new(measure: create(:measure, name: 'Display Me', measure_id: 'display-me'), score: rand)
+        VarianceChartRowPresenter.new(measure: create(:measure, name: 'Display Me', measure_id: 'display-me'), score: rand)
       ]
     }
 

spec/views/dashboard/variance_chart.html.erb_spec.rb

@@ -0,0 +1,26 @@
+require 'rails_helper'
+
+describe 'dashboard/_variance_chart.html.erb' do
+  subject { Nokogiri::HTML(rendered) }
+
+  let(:higher_scoring_measure) { create(:measure) }
+  let(:lower_scoring_measure) { create(:measure) }
+
+  before :each do
+    presenters = [
+      VarianceChartRowPresenter.new(measure: lower_scoring_measure, score: 1),
+      VarianceChartRowPresenter.new(measure: higher_scoring_measure, score: 5)
+    ]
+
+    render partial: 'variance_chart', locals: { presenters: presenters }
+  end
+
+  it 'displays higher scoring measures above lower scoring measures' do
+    measure_row_bars = subject.css("rect.measure-row-bar")
+
+    higher_scoring_measure_index = measure_row_bars.find_index { |bar| bar['data-for-measure-id'] == higher_scoring_measure.measure_id }
+    lower_scoring_measure_index = measure_row_bars.find_index { |bar| bar['data-for-measure-id'] == lower_scoring_measure.measure_id }
+
+    expect(higher_scoring_measure_index).to be < lower_scoring_measure_index
+  end
+end