Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions app/jobs/reports/monthly_key_metrics_report.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ def perform(date = Time.zone.today)
account_reuse_table = account_reuse_report.account_reuse_report
total_profiles_table = account_reuse_report.total_identities_report
account_deletion_rate_table = account_deletion_rate_report.account_deletion_report
total_user_count_table = total_user_count_report.total_user_count_report

upload_to_s3(account_reuse_table, report_name: 'account_reuse')
upload_to_s3(total_profiles_table, report_name: 'total_profiles')
upload_to_s3(account_deletion_rate_table, report_name: 'account_deletion_rate')
upload_to_s3(total_user_count_table, report_name: 'total_user_count')

email_tables = [
[
Expand All @@ -38,6 +40,12 @@ def perform(date = Time.zone.today)
},
*account_deletion_rate_table,
],
[
{
title: 'Total user count (all-time)',
},
*total_user_count_table,
],
]

email_message = "Report: #{REPORT_NAME} #{date}"
Expand Down Expand Up @@ -71,6 +79,10 @@ def account_deletion_rate_report
@account_deletion_rate_report ||= Reporting::AccountDeletionRateReport.new(report_date)
end

def total_user_count_report
@total_user_count_report ||= Reporting::TotalUserCountReport.new(report_date)
end

def upload_to_s3(report_body, report_name: nil)
_latest, path = generate_s3_paths(REPORT_NAME, 'csv', subname: report_name, now: report_date)

Expand Down
24 changes: 24 additions & 0 deletions app/services/reporting/total_user_count_report.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# frozen_string_literal: true

module Reporting
class TotalUserCountReport
attr_reader :report_date

def initialize(report_date = Time.zone.today)
@report_date = report_date
end

def total_user_count_report
[
['All-time user count'],
[total_user_count],
]
end

private

def total_user_count
User.where('created_at <= ?', report_date).count
end
end
end
35 changes: 18 additions & 17 deletions spec/jobs/reports/monthly_key_metrics_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@
let(:report_folder) do
'int/monthly-key-metrics-report/2021/2021-03-02.monthly-key-metrics-report'
end
let(:account_reuse_s3_path) do
"#{report_folder}/account_reuse.csv"
end
let(:total_profiles_s3_path) do
"#{report_folder}/total_profiles.csv"
end
let(:account_deletion_rate_s3_path) do
"#{report_folder}/account_deletion_rate.csv"
let(:account_reuse_s3_path) { "#{report_folder}/account_reuse.csv" }
let(:total_profiles_s3_path) { "#{report_folder}/total_profiles.csv" }
let(:account_deletion_rate_s3_path) { "#{report_folder}/account_deletion_rate.csv" }
let(:total_user_count_s3_path) { "#{report_folder}/total_user_count.csv" }
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized this didn't actually need to be do...end blocks for length so I compacted them. Tell me if you hate this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compact is good!

let(:s3_metadata) do
{
body: anything,
content_type: 'text/csv',
bucket: 'reports-bucket.1234-us-west-1',
}
end

before do
Expand Down Expand Up @@ -80,23 +82,22 @@
it 'uploads a file to S3 based on the report date' do
expect(subject).to receive(:upload_file_to_s3_bucket).with(
path: account_reuse_s3_path,
body: anything,
content_type: 'text/csv',
bucket: 'reports-bucket.1234-us-west-1',
**s3_metadata,
).exactly(1).time.and_call_original

expect(subject).to receive(:upload_file_to_s3_bucket).with(
path: total_profiles_s3_path,
body: anything,
content_type: 'text/csv',
bucket: 'reports-bucket.1234-us-west-1',
**s3_metadata,
).exactly(1).time.and_call_original

expect(subject).to receive(:upload_file_to_s3_bucket).with(
path: account_deletion_rate_s3_path,
body: anything,
content_type: 'text/csv',
bucket: 'reports-bucket.1234-us-west-1',
**s3_metadata,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I'd try to DRY this up a little rather than copying it for a 4th time.

).exactly(1).time.and_call_original

expect(subject).to receive(:upload_file_to_s3_bucket).with(
path: total_user_count_s3_path,
**s3_metadata,
).exactly(1).time.and_call_original

subject.perform(report_date)
Expand Down
73 changes: 73 additions & 0 deletions spec/services/reporting/total_user_count_report_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
require 'rails_helper'
require 'csv'

RSpec.describe Reporting::TotalUserCountReport do
let(:report_date) do
Date.new(2021, 3, 1).in_time_zone('UTC')
end
let(:expected_report) do
[
['All-time user count'],
[expected_count],
]
end

subject(:report) { described_class.new(report_date) }

before { travel_to report_date }
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly copypasta; we don't assert the actual date here. But it nicely avoids the hassle I ran into previously where I had to set created_at to Time.now - 1.minute to work around some weirdness, so I left it with just following the existing pattern.


shared_examples 'a report with that user counted' do
let(:expected_count) { 1 }
it 'includes that user in the count' do
expect(subject.total_user_count_report).to eq expected_report
end
end
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a big savings, but IMHO it better expresses the intent vs. repeating expect(subject.total_user_count_report).to eq expected_report a bunch of times.


describe '#total_user_count_report' do
context 'with no users' do
let(:expected_count) { 0 }

it 'returns a report with a count of zero' do
expect(subject.total_user_count_report).to eq expected_report
end
end

context 'with one ordinary user' do
let!(:user) { create(:user) }
it_behaves_like 'a report with that user counted'
end

context 'with a suspended user' do
let!(:suspended_user) { create(:user, :suspended) }

it 'has a suspended user' do
expect(suspended_user).to be_suspended
expect(User.count).to eq 1
end

it_behaves_like 'a report with that user counted'
end

context 'with an unconfirmed user' do
let!(:unconfirmed_user) { create(:user, :unconfirmed) }

it 'has an unconfirmed user' do
expect(unconfirmed_user).to_not be_confirmed
expect(User.count).to eq 1
end

it_behaves_like 'a report with that user counted'
end

context 'with a user rejected for fraud' do
let!(:fraud_user) { create(:user, :fraud_rejection) }

it 'has a user rejected for fraud' do
expect(fraud_user).to be_fraud_rejection
expect(User.count).to eq 1
end

it_behaves_like 'a report with that user counted'
end
end
end