-
-
Notifications
You must be signed in to change notification settings - Fork 473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Historical Trend Speed up #3665
Changes from 1 commit
bd30366
2a4dd1f
277eac1
5892c79
8fa8b31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,5 @@ | ||
class HistoricalTrends::BaseController < ApplicationController | ||
def series(type) | ||
items = [] | ||
|
||
current_organization.items.sort.each do |item| | ||
next if item.line_items.where(itemizable_type: type, item: item).blank? | ||
|
||
dates = Hash.new | ||
|
||
(1..Time.zone.today.month).each do |month| | ||
dates[month] = 0 | ||
end | ||
|
||
total_items(item.line_items, type).each do |line_item| | ||
month = line_item.dig(0).to_date.month | ||
dates[month] = line_item.dig(1) | ||
end | ||
|
||
items << { name: item.name, data: dates.values } | ||
end | ||
|
||
items.sort_by { |hsh| hsh[:name] } | ||
end | ||
|
||
private | ||
|
||
def total_items(line_items, type) | ||
line_items.where(itemizable_type: type) | ||
.group_by_month(:created_at) | ||
.sum(:quantity) | ||
def cached_series(type) | ||
Rails.cache.fetch("#{current_organization.short_name}-historical-#{type}-data") { HistoricalTrendService.new(current_organization.id, type).series } | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
class HistoricalTrends::DistributionsController < HistoricalTrends::BaseController | ||
def index | ||
@series = series('Distribution') | ||
@series = cached_series('Distribution') | ||
@title = 'Monthly Distributions' | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
class HistoricalTrends::DonationsController < HistoricalTrends::BaseController | ||
def index | ||
@series = series('Donation') | ||
@series = cached_series('Donation') | ||
@title = 'Monthly Donations' | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
class HistoricalTrends::PurchasesController < HistoricalTrends::BaseController | ||
def index | ||
@series = series('Purchase') | ||
@series = cached_series('Purchase') | ||
@title = "Monthly Purchases" | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
module HistoricalTrendsHelper | ||
MONTHS = [ | ||
"Jan", | ||
"Feb", | ||
"Mar", | ||
"Apr", | ||
"May", | ||
"Jun", | ||
"Jul", | ||
"Aug", | ||
"Sep", | ||
"Oct", | ||
"Nov", | ||
"Dec" | ||
] | ||
|
||
def last_12_months | ||
current_month = Time.zone.now.month | ||
MONTHS.rotate(current_month) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
class HistoricalDataCacheJob < ApplicationJob | ||
def perform(org_id:, type:) | ||
organization = Organization.find_by(id: org_id) | ||
|
||
Rails.cache.write("#{organization.short_name}-historical-#{type}-data", HistoricalTrendService.new(organization.id, type).series) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
class HistoricalTrendService | ||
def initialize(organization_id, type) | ||
@organization = Organization.find(organization_id) | ||
@type = type | ||
end | ||
|
||
def series | ||
items = [] | ||
|
||
@organization.items.active.sort.each do |item| | ||
next if item.line_items.where(itemizable_type: @type, item: item).blank? | ||
|
||
month_offset = [*1..12].rotate(12 - Time.zone.today.month) | ||
|
||
dates = (1..12).index_with { |i| 0 } | ||
|
||
total_items(item.line_items, @type).each do |line_item| | ||
month = line_item.dig(0).to_date.month | ||
dates[(month_offset.index(month) + 1)] = line_item.dig(1) | ||
end | ||
|
||
items << {name: item.name, data: dates.values, visible: false} | ||
end | ||
|
||
items.sort_by { |hsh| hsh[:name] } | ||
end | ||
|
||
private | ||
|
||
def total_items(line_items, type) | ||
line_items.where(created_at: 1.year.ago.beginning_of_month..Time.current) | ||
.where(itemizable_type: type) | ||
.group_by_month(:created_at) | ||
.sum(:quantity) | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
<div data-controller="highchart" data-highchart-config-value="<%= config %>"> | ||
<div data-highchart-target="chart"></div> | ||
<div class='flex flex-row justify-center w-full space-x-2 py-3'> | ||
<button type="button" class="inline-flex items-center rounded border border-transparent bg-indigo-600 px-2.5 py-1.5 text-xs font-medium text-white shadow-sm hover:bg-indigo-700 focus:outline-none focus:ring-2 focus:ring-indigo-500 focus:ring-offset-2" data-action="click->highchart#selectAll">Select All</button> | ||
<button type="button" class="inline-flex items-center rounded border border-transparent bg-gray-600 px-2.5 py-1.5 text-xs font-medium text-white shadow-sm hover:bg-indigo-700 focus:outline-none focus:ring-2 focus:ring-indigo-500 focus:ring-offset-2" data-action="click->highchart#deselectAll">Deselect All</button> | ||
<div> | ||
<button type="button" class="btn btn-sm btn-secondary" data-action="click->highchart#selectAll">Select All</button> | ||
<button type="button" class="btn btn-sm btn-info" data-action="click->highchart#deselectAll">Deselect All</button> | ||
</div> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
desc "This task is run by a scheduling tool nightly to cache the processor intensive queries" | ||
task :cache_historical_data => :environment do | ||
puts "Caching historical data" | ||
DATA_TYPES = ['Distribution', 'Purchase', 'Donation'] | ||
|
||
orgs = Organization.all | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some way for us to filter out old / inactive organizations from this list? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have we got a definition of what an inactive organization is? (Have we marked them as such somehow?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, but we could define one as (say) one where no one has logged in for over 3 months. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say 4, just in case there's someone working on a quarterly basis. I doubt it, but I haven't ruled it out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Created an |
||
|
||
orgs.each do |org| | ||
DATA_TYPES.each do |type| | ||
puts "Queuing up #{type} cache data for #{org.name}" | ||
HistoricalDataCacheJob.perform_later(org_id: org.id, type: type) | ||
end | ||
end | ||
|
||
puts "Done!" | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
require "rspec" | ||
|
||
RSpec.describe HistoricalTrendsHelper do | ||
describe "#last_12_months" do | ||
it "returns the last 12 months starting from July when the current month is June" do | ||
allow_any_instance_of(Time).to receive(:month).and_return(6) | ||
|
||
expect(last_12_months).to eq(["Jul", "Aug", "Sep", "Oct", "Nov", "Dec", "Jan", "Feb", "Mar", "Apr", "May", "Jun"]) | ||
end | ||
|
||
it "returns the last 12 months starting from Feb when the current month is Jan" do | ||
allow_any_instance_of(Time).to receive(:month).and_return(1) | ||
|
||
expect(last_12_months).to eq(["Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec", "Jan"]) | ||
end | ||
|
||
it "returns the last 12 months starting from Jan when the current month is Dec" do | ||
allow_any_instance_of(Time).to receive(:month).and_return(12) | ||
|
||
expect(last_12_months).to eq(["Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"]) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
require "rails_helper" | ||
|
||
RSpec.describe HistoricalDataCacheJob, type: :job do | ||
include ActiveJob::TestHelper | ||
|
||
let(:organization) { create(:organization) } | ||
let(:type) { "Donation" } | ||
let(:job) { described_class.perform_later(org_id: organization.id, type: type) } | ||
|
||
it "queues the job" do | ||
expect { job }.to have_enqueued_job(described_class) | ||
.with(org_id: organization.id, type: type) | ||
.on_queue("default") | ||
end | ||
|
||
it "caches the historical data" do | ||
expect(Rails.cache).to receive(:write).with("#{organization.short_name}-historical-#{type}-data", anything) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we get a more thorough test that shows the actual data being written? Even if it means stubbing out the HistoricalTrendService. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Stubbed out the response. |
||
perform_enqueued_jobs { job } | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
require "rails_helper" | ||
|
||
RSpec.describe HistoricalTrendService, type: :service do | ||
let(:organization) { create(:organization) } | ||
let(:type) { "Donation" } | ||
let(:service) { described_class.new(organization.id, type) } | ||
|
||
describe "#series" do | ||
let!(:item1) { create(:item, organization: organization, name: "Item 1") } | ||
let!(:item2) { create(:item, organization: organization, name: "Item 2") } | ||
let!(:line_item1) { create(:line_item, item: item1, itemizable_type: type, quantity: 10, created_at: 1.month.ago) } | ||
let!(:line_item2) { create(:line_item, item: item1, itemizable_type: type, quantity: 20, created_at: 2.months.ago) } | ||
let!(:line_item3) { create(:line_item, item: item2, itemizable_type: type, quantity: 30, created_at: 3.months.ago) } | ||
|
||
it "returns an array of items with their monthly data" do | ||
expected_result = [ | ||
{name: "Item 1", data: [0, 0, 0, 0, 0, 0, 0, 0, 0, 20, 10, 0], visible: false}, | ||
{name: "Item 2", data: [0, 0, 0, 0, 0, 0, 0, 0, 30, 0, 0, 0], visible: false} | ||
] | ||
expect(service.series).to eq(expected_result) | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a couple more tests here where we time travel to different months? Just to make sure nothing weird happens around the year end. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. On the first item I created a donation line item for each of the 12 months in the series with the line item 12 months ago having 120, 11 having 110, etc. |
||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using
Rails.logger
here instead ofputs
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched to logger with
info