From 14454bdd45db4eb469cf7cf5fd6ab23dc0699fcf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 25 Apr 2024 02:11:00 +0200 Subject: [PATCH] Include data in chart tags instead of using AJAX Now that we've moved the logic to generate the events data to a model, and we've got access to the model in the component rendering the chart, we can render the data inside the chart instead of doing an extra AJAX request to get the same data. Originally, this was problaby done this way so the page wouldn't take several seconds to load while preparing the data for the chart when there are thousands of dates being displayed. With an AJAX call, the page would load as fast as usual, and then the chart would render after a few seconds. However, we can have an even better performance improvement in this scenario if we use a Set instead of an Array. The method `Array#include?`, which we were calling for every date in the data, is much slower that `Set#merge`. So now both the page and the chart load as fast as expected. We could also use something like: ``` def add (...) shared_keys.push(*collection.keys) end def build (...) shared_keys.uniq.each do |k| (...) end def shared_keys @shared_keys ||= [] end ``` Or other approaches to avoid using `Array#include?`. The performance would be similar to the one we get when using `Set`. We're using a `Set` because it makes more obvious that `shared_keys` is supposed to contain unique elements. We've had some tests failing in the past due to these AJAX requests being triggered automatically during the tests and no expectations checking the requests have finished, so now we're reducing the amount of flaky tests. --- app/assets/javascripts/stats.js | 6 ++-- app/components/admin/stats/chart_component.rb | 2 +- app/controllers/admin/api/stats_controller.rb | 9 ----- app/models/ahoy/data_source.rb | 8 ++--- config/routes/admin.rb | 4 --- .../admin/stats/chart_component_spec.rb | 13 +++++++ .../admin/api/stats_controller_spec.rb | 36 ------------------- 7 files changed, 18 insertions(+), 60 deletions(-) delete mode 100644 app/controllers/admin/api/stats_controller.rb create mode 100644 spec/components/admin/stats/chart_component_spec.rb delete mode 100644 spec/controllers/admin/api/stats_controller_spec.rb diff --git a/app/assets/javascripts/stats.js b/app/assets/javascripts/stats.js index fe7e46d8b..924c4a2dc 100644 --- a/app/assets/javascripts/stats.js +++ b/app/assets/javascripts/stats.js @@ -5,14 +5,12 @@ var buildGraph; buildGraph = function(el) { - var conf, url; - url = $(el).data("graph"); + var conf; conf = { bindto: el, data: { x: "x", - url: url, - mimeType: "json" + json: $(el).data("graph") }, axis: { x: { diff --git a/app/components/admin/stats/chart_component.rb b/app/components/admin/stats/chart_component.rb index 2aeb1fd23..dbfde1678 100644 --- a/app/components/admin/stats/chart_component.rb +++ b/app/components/admin/stats/chart_component.rb @@ -16,7 +16,7 @@ class Admin::Stats::ChartComponent < ApplicationComponent end def chart_tag - tag.div("data-graph": admin_api_stats_path(event: event)) + tag.div("data-graph": chart.data_points.to_json) end def title diff --git a/app/controllers/admin/api/stats_controller.rb b/app/controllers/admin/api/stats_controller.rb deleted file mode 100644 index 577d11fff..000000000 --- a/app/controllers/admin/api/stats_controller.rb +++ /dev/null @@ -1,9 +0,0 @@ -class Admin::Api::StatsController < Admin::Api::BaseController - def show - if params[:event].present? - render json: Ahoy::Chart.new(params[:event]).data_points - else - render json: {}, status: :bad_request - end - end -end diff --git a/app/models/ahoy/data_source.rb b/app/models/ahoy/data_source.rb index e1430c5e8..a29902593 100644 --- a/app/models/ahoy/data_source.rb +++ b/app/models/ahoy/data_source.rb @@ -9,7 +9,7 @@ module Ahoy # chart def add(name, collection) collections.push data: collection, name: name - collection.each_key { |key| add_key key } + shared_keys.merge(collection.keys) end def build @@ -36,11 +36,7 @@ module Ahoy end def shared_keys - @shared_keys ||= [] - end - - def add_key(key) - shared_keys.push(key) unless shared_keys.include? key + @shared_keys ||= Set.new end end end diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 5ffbf5eba..4e72dae01 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -241,10 +241,6 @@ namespace :admin do end end - namespace :api do - resource :stats, only: :show - end - resources :geozones, only: [:index, :new, :create, :edit, :update, :destroy] namespace :site_customization do diff --git a/spec/components/admin/stats/chart_component_spec.rb b/spec/components/admin/stats/chart_component_spec.rb new file mode 100644 index 000000000..c6d0bdafb --- /dev/null +++ b/spec/components/admin/stats/chart_component_spec.rb @@ -0,0 +1,13 @@ +require "rails_helper" + +describe Admin::Stats::ChartComponent do + it "renders a tag with JSON data" do + create(:user, :level_three, verified_at: "2009-09-09") + chart = Ahoy::Chart.new(:level_3_user) + + render_inline Admin::Stats::ChartComponent.new(chart) + + expect(page).to have_css "h2", exact_text: "Level 3 users verified (1)" + expect(page).to have_css "[data-graph='{\"x\":[\"2009-09-09\"],\"Level 3 users verified\":[1]}']" + end +end diff --git a/spec/controllers/admin/api/stats_controller_spec.rb b/spec/controllers/admin/api/stats_controller_spec.rb deleted file mode 100644 index 472fbda86..000000000 --- a/spec/controllers/admin/api/stats_controller_spec.rb +++ /dev/null @@ -1,36 +0,0 @@ -require "rails_helper" - -describe Admin::Api::StatsController, :admin do - describe "GET index" do - context "events or visits not present" do - it "responds with bad_request" do - get :show - - expect(response).not_to be_ok - expect(response).to have_http_status 400 - end - end - - context "events present" do - before do - time_1 = Time.zone.local(2015, 01, 01) - time_2 = Time.zone.local(2015, 01, 02) - time_3 = Time.zone.local(2015, 01, 03) - - create(:proposal, created_at: time_1) - create(:proposal, created_at: time_1) - create(:proposal, created_at: time_2) - create(:debate, created_at: time_1) - create(:debate, created_at: time_3) - create(:debate, created_at: time_3) - end - - it "returns single events formated for working with c3.js" do - get :show, params: { event: "proposal_created" } - - expect(response).to be_ok - expect(response.parsed_body).to eq "x" => ["2015-01-01", "2015-01-02"], "Citizen proposals created" => [2, 1] - end - end - end -end