From ad7dd85524136a02b30531c19f0b811f3ffb125c Mon Sep 17 00:00:00 2001 From: Liam Morley Date: Thu, 28 Oct 2021 12:05:41 -0400 Subject: [PATCH] Refactor variance chart to make view responsible for sorting measures --- app/controllers/dashboard_controller.rb | 12 ++----- app/helpers/variance_helper.rb | 8 ----- ...ter.rb => variance_chart_row_presenter.rb} | 4 +-- ...nce_graph.erb => _variance_chart.html.erb} | 12 ++++--- app/views/dashboard/index.html.erb | 2 +- .../features/school_dashboard_feature_spec.rb | 12 ------- ...b => variance_chart_row_presenter_spec.rb} | 32 ++++++++----------- spec/views/dashboard/index.html.erb_spec.rb | 16 +++++----- .../dashboard/variance_chart.html.erb_spec.rb | 26 +++++++++++++++ 9 files changed, 60 insertions(+), 64 deletions(-) rename app/presenters/{measure_graph_row_presenter.rb => variance_chart_row_presenter.rb} (96%) rename app/views/dashboard/{_variance_graph.erb => _variance_chart.html.erb} (87%) rename spec/presenters/{measure_graph_row_presenter_spec.rb => variance_chart_row_presenter_spec.rb} (77%) create mode 100644 spec/views/dashboard/variance_chart.html.erb_spec.rb diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index fa4bd1ee..fac83f40 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -1,24 +1,16 @@ class DashboardController < SqmApplicationController def index - @measure_graph_row_presenters = measure_ids - .map { |measure_id| Measure.find_by_measure_id measure_id } - .map(&method(:presenter_for_measure)) - .sort - .reverse + @variance_chart_row_presenters = Measure.all.map(&method(:presenter_for_measure)) @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 diff --git a/app/helpers/variance_helper.rb b/app/helpers/variance_helper.rb index 5ff88e2a..e66fbcb3 100644 --- a/app/helpers/variance_helper.rb +++ b/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 diff --git a/app/presenters/measure_graph_row_presenter.rb b/app/presenters/variance_chart_row_presenter.rb similarity index 96% rename from app/presenters/measure_graph_row_presenter.rb rename to app/presenters/variance_chart_row_presenter.rb index f11ab77d..fff4025b 100644 --- a/app/presenters/measure_graph_row_presenter.rb +++ b/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 diff --git a/app/views/dashboard/_variance_graph.erb b/app/views/dashboard/_variance_chart.html.erb similarity index 87% rename from app/views/dashboard/_variance_graph.erb rename to app/views/dashboard/_variance_chart.html.erb index c6305e3b..7f891dc5 100644 --- a/app/views/dashboard/_variance_graph.erb +++ b/app/views/dashboard/_variance_chart.html.erb @@ -1,8 +1,10 @@ -<% unless measures_with_insufficient_data(presenters: presenters).empty? %> -

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('; ') %>.

+<% displayed_presenters = presenters.filter { |p| p.sufficient_data? }.sort %> +<% not_displayed_presenters = presenters - displayed_presenters %> + +<% if not_displayed_presenters.present? %> +

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('; ') %>.

<% end %> -<% displayed_presenters = measures_with_sufficient_data(presenters: presenters) %> xmlns="http://www.w3.org/2000/svg"> @@ -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)" > @@ -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" /> diff --git a/app/views/dashboard/index.html.erb b/app/views/dashboard/index.html.erb index cfbc8e80..955f26dd 100644 --- a/app/views/dashboard/index.html.erb +++ b/app/views/dashboard/index.html.erb @@ -75,5 +75,5 @@

Distance From Benchmark

- <%= render partial: "variance_graph", locals: { presenters: @measure_graph_row_presenters} %> + <%= render partial: "variance_chart", locals: { presenters: @variance_chart_row_presenters} %>
diff --git a/spec/features/school_dashboard_feature_spec.rb b/spec/features/school_dashboard_feature_spec.rb index 7e72007f..98d9ab79 100644 --- a/spec/features/school_dashboard_feature_spec.rb +++ b/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 diff --git a/spec/presenters/measure_graph_row_presenter_spec.rb b/spec/presenters/variance_chart_row_presenter_spec.rb similarity index 77% rename from spec/presenters/measure_graph_row_presenter_spec.rb rename to spec/presenters/variance_chart_row_presenter_spec.rb index 916bcb6a..4fee05ab 100644 --- a/spec/presenters/measure_graph_row_presenter_spec.rb +++ b/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 - this_presenter = MeasureGraphRowPresenter.new measure: measure, score: 3.7 - other_presenter = MeasureGraphRowPresenter.new measure: measure, score: 4.4 - expect(this_presenter <=> other_presenter).to be < 0 - end - - it 'selects a warning bar before a ideal bar' do - this_presenter = MeasureGraphRowPresenter.new measure: measure, score: 1.0 - other_presenter = MeasureGraphRowPresenter.new measure: measure, score: 5.0 - expect(this_presenter <=> other_presenter).to be < 0 - end - - 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 + it 'selects a longer bar before a shorter bar for measures in the approval/ideal zones' do + approval_presenter = VarianceChartRowPresenter.new measure: measure, score: 3.7 + ideal_presenter = VarianceChartRowPresenter.new measure: measure, score: 4.4 + expect(ideal_presenter <=> approval_presenter).to be < 0 + expect([approval_presenter, ideal_presenter].sort).to eq [ideal_presenter, approval_presenter] + end + + it 'selects a warning bar below a ideal bar' do + warning_presenter = VarianceChartRowPresenter.new measure: measure, score: 1.0 + ideal_presenter = VarianceChartRowPresenter.new measure: measure, score: 5.0 + expect(warning_presenter <=> ideal_presenter).to be > 0 + expect([warning_presenter, ideal_presenter].sort).to eq [ideal_presenter, warning_presenter] end end end diff --git a/spec/views/dashboard/index.html.erb_spec.rb b/spec/views/dashboard/index.html.erb_spec.rb index 14c1c895..3ab818b4 100644 --- a/spec/views/dashboard/index.html.erb_spec.rb +++ b/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), - MeasureGraphRowPresenter.new(measure: create(:measure, name: 'Should Be Displayed', measure_id: 'should-be-displayed'), score: rand), - MeasureGraphRowPresenter.new(measure: effective_leadership, score: nil), - MeasureGraphRowPresenter.new(measure: professional_qualifications, score: nil) + VarianceChartRowPresenter.new(measure: support_for_teaching, score: nil), + VarianceChartRowPresenter.new(measure: create(:measure, name: 'Should Be Displayed', measure_id: 'should-be-displayed'), score: rand), + VarianceChartRowPresenter.new(measure: effective_leadership, 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) ] } diff --git a/spec/views/dashboard/variance_chart.html.erb_spec.rb b/spec/views/dashboard/variance_chart.html.erb_spec.rb new file mode 100644 index 00000000..1c6af8bf --- /dev/null +++ b/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