Skip to content
Closed
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 @@ -11,9 +11,11 @@ def perform(date = Time.zone.today)

account_reuse_table = account_reuse_report.account_reuse_report
total_profiles_table = account_reuse_report.total_identities_report
total_users_all_time_table = total_user_queries.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(total_users_all_time_table, report_name: 'total_users_all_time')
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.

Much of this is actually from Luis's PR; you can view my commits individually if you need to separate them out. I'll rebase this Friday as appropriate.


email_tables = [
[
Expand All @@ -28,6 +30,10 @@ def perform(date = Time.zone.today)
{ title: 'Total proofed identities' },
*total_profiles_table,
],
[
{ title: 'All-time user total' },
total_users_all_time_table,
],
]

email_message = "Report: #{REPORT_NAME} #{date}"
Expand All @@ -53,9 +59,15 @@ def emails
emails
end


def account_reuse_report
@account_reuse_report ||= Reporting::AccountReuseAndTotalIdentitiesReport.new(report_date)
end

def total_user_queries
@total_user_queries ||= 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
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@

module Reporting
class AccountReuseAndTotalIdentitiesReport
attr_reader :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
54 changes: 54 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,54 @@
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe Reporting::TotalUserCountReport do
let(:report_date) { Time.zone.today + 1.day }
let!(:user) { create(:user) }
let(:expected_report) do
[
['All-time user count'],
[expected_count],
]
end

subject { described_class.new(report_date) }

describe '#total_user_count_report' do
let(:expected_count) { 1 }

it 'returns the expected data' do
expect(subject.total_user_count_report).to eq(expected_report)
end

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

it 'includes the suspended user in the count' do
expect(suspended_user).to be_suspended
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.

Aside: I just recently learned about how much magic you can do with built-in rspec matchers. If a class responds to foo?, then you can just expect(subject).to be_foo. https://rubydoc.info/gems/rspec-expectations/RSpec/Matchers#be-instance_method

This was kind of scaffolding for myself to confirm that my 'suspended' user was indeed suspended, but I'm inclined to leave it in for clarity.

expect(subject.total_user_count_report).to eq(expected_report)
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'm not super proud of this pattern. I'm essentially testing total_user_count, which I made private (because it should be), in a roundabout way. The diff should hopefully make it clear enough if it fails, but it's a little hamfisted.

All that said, I'm not testing these because I think they might break; I'm testing them to make explicit what I intend as the expected behavior. Should a suspended, unconfirmed, or rejected-for-fraud user be included in "Total users all-time", or is that surprising? Given that we have previously received requests for "active users," I am interpreting "Total users all-time" to include non-active users, but will the consumers of our report agree? (I worry that this turns into a large can of worms otherwise.)

end
end

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

it 'includes the unconfirmed user in the count' do
expect(unconfirmed_user).not_to be_confirmed
expect(subject.total_user_count_report).to eq(expected_report)
end
end

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

it 'includes them in the count because they are still a user' do
expect(fraud_user).to be_fraud_rejection
expect(subject.total_user_count_report).to eq(expected_report)
end
end
end
end