Skip to content
35 changes: 31 additions & 4 deletions app/services/reporting/total_user_count_report.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,21 @@ def initialize(report_date = Time.zone.today)

def total_user_count_report
[
['All-time user count'],
[total_user_count],
['Metric', 'Value', 'Time Range Start', 'Time Range End'],
['All-time user count', total_user_count, '-', report_date.strftime('%F')],
['Total verified users', verified_user_count, '-', report_date.strftime('%F')],
[
'Total annual users',
annual_total_user_count,
annual_start_date.strftime('%F'),
end_date.strftime('%F'),
],
]
end

def total_user_count_emailable_report
EmailableReport.new(
title: 'Total user count (all-time)',
title: 'Total user counts',
table: total_user_count_report,
filename: 'total_user_count',
)
Expand All @@ -27,8 +34,28 @@ def total_user_count_emailable_report

def total_user_count
Reports::BaseReport.transaction_with_timeout do
User.where('created_at <= ?', report_date).count
User.where('created_at <= ?', end_date).count
end
end

def verified_user_count
Reports::BaseReport.transaction_with_timeout do
Profile.where(active: true).where('activated_at <= ?', end_date).count
end
end

def annual_total_user_count
Reports::BaseReport.transaction_with_timeout do
User.where(created_at: annual_start_date..end_date).count
end
end

def annual_start_date
(report_date - 1.year).beginning_of_day
end

def end_date
report_date.beginning_of_day
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.

I am boldly just assuming that the request for "verified users" means "IdV users," but hopefully @zachmargolis or someone can confirm if that is how others would interpret the term?

And to slightly further open the can of worms: I'm including the active: true limit to be consistent with the handbook, but the other report generated in this class is "total users all time" so I think one could make an argument that we shouldn't filter only on active: true profiles.

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.

yeah that's a correct interpretation and yes, we should keep the handbook query here

Copy link
Copy Markdown
Contributor Author

@n1zyy n1zyy Oct 18, 2023

Choose a reason for hiding this comment

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

I worked with @olatifflexion to steal his code incorporate his work on LG-11150 into this, rather than having him deal with merge conflicts.

We confirmed with Cale in the original doc that "annual" here refers to the previous 12 months, not fiscal/calendar year.

end
end
45 changes: 16 additions & 29 deletions spec/jobs/reports/monthly_key_metrics_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@
let(:account_deletion_rate_s3_path) { "#{report_folder}/account_deletion_rate.csv" }
let(:total_user_count_s3_path) { "#{report_folder}/total_user_count.csv" }
let(:monthly_active_users_count_s3_path) { "#{report_folder}/monthly_active_users_count.csv" }
let(:expected_s3_paths) do
[
account_reuse_s3_path,
total_profiles_s3_path,
account_deletion_rate_s3_path,
total_user_count_s3_path,
document_upload_proofing_s3_path,
monthly_active_users_count_s3_path,
]
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.

I built an array of all these expected S3 files so that...

let(:s3_metadata) do
{
body: anything,
Expand Down Expand Up @@ -91,35 +101,12 @@
end

it 'uploads a file to S3 based on the report date' do
expect(subject).to receive(:upload_file_to_s3_bucket).with(
path: total_user_count_s3_path,
**s3_metadata,
).exactly(1).time.and_call_original

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

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

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

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

expect(subject).to receive(:upload_file_to_s3_bucket).with(
path: monthly_active_users_count_s3_path,
**s3_metadata,
).exactly(1).time.and_call_original
expected_s3_paths.each do |path|
expect(subject).to receive(:upload_file_to_s3_bucket).with(
path: path,
**s3_metadata,
).exactly(1).time.and_call_original
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.

...we can replace this with a happy little loop.


subject.perform(report_date)
end
Expand Down
87 changes: 51 additions & 36 deletions spec/services/reporting/total_user_count_report_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,69 +5,84 @@
let(:report_date) do
Date.new(2021, 3, 1).in_time_zone('UTC')
end

let(:expected_report) do
[
['All-time user count'],
[expected_count],
['Metric', 'Value', 'Time Range Start', 'Time Range End'],
['All-time user count', expected_total_count, '-', '2021-03-01'],
['Total verified users', expected_verified_count, '-', '2021-03-01'],
['Total annual users', expected_annual_count, '2020-03-01', '2021-03-01'],
]
end

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

before { travel_to report_date }

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

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
shared_examples 'a report with the specified counts' do
it 'returns a report with the expected counts' 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'
context 'with only a non-verified user' do
before { create(:user) }
let(:expected_total_count) { 1 }
let(:expected_verified_count) { 0 }
let(:expected_annual_count) { expected_total_count }

it_behaves_like 'a report with the specified counts'
end

context 'with a suspended user' do
let!(:suspended_user) { create(:user, :suspended) }
context 'with 2 users, 1 from more than a year ago' do
let!(:recent_user) { create(:user) }
let!(:old_user) { create(:user, created_at: report_date - 13.months) }

it 'has a suspended user' do
expect(suspended_user).to be_suspended
expect(User.count).to eq 1
let(:expected_total_count) { 2 }
let(:expected_verified_count) { 0 }
let(:expected_annual_count) { 1 }

it_behaves_like 'a report with the specified counts'
end

context 'with one verified and one non-verified user' do
before do
create(:user)
user2 = create(:user)
# MW: The :verified trait doesn't set active: true. This feels confusing.
create(:profile, :active, user: user2)
end
let(:expected_total_count) { 2 }
let(:expected_verified_count) { 1 }
let(:expected_annual_count) { expected_total_count }

it_behaves_like 'a report with that user counted'
it_behaves_like 'a report with the specified counts'
end

context 'with an unconfirmed user' do
let!(:unconfirmed_user) { create(:user, :unconfirmed) }
# The suspended and fraud-rejection examples are meant to highlight this
# developer's understanding of the requirements and resulting behavior,
# not to express immutable business logic.
context 'with one user, who is suspended' do
before { create(:user, :suspended) }

it 'has an unconfirmed user' do
expect(unconfirmed_user).to_not be_confirmed
expect(User.count).to eq 1
end
# A suspended user is still a total user:
let(:expected_total_count) { 1 }
let(:expected_verified_count) { 0 }
let(:expected_annual_count) { 1 }

it_behaves_like 'a report with that user counted'
it_behaves_like 'a report with the specified counts'
end

context 'with a user rejected for fraud' do
let!(:fraud_user) { create(:user, :fraud_rejection) }
context 'with one user, who was rejected for fraud' do
before { 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
# A user with a fraud rejection is still a total user
let(:expected_total_count) { 1 }
let(:expected_verified_count) { 0 }
let(:expected_annual_count) { 1 }

it_behaves_like 'a report with that user counted'
it_behaves_like 'a report with the specified counts'
end
end
end