diff --git a/app/services/reporting/total_user_count_report.rb b/app/services/reporting/total_user_count_report.rb index 453ccc4ad48..68720658c1d 100644 --- a/app/services/reporting/total_user_count_report.rb +++ b/app/services/reporting/total_user_count_report.rb @@ -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', ) @@ -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 end end diff --git a/spec/jobs/reports/monthly_key_metrics_report_spec.rb b/spec/jobs/reports/monthly_key_metrics_report_spec.rb index a341b1bea42..5f2c80dcc0e 100644 --- a/spec/jobs/reports/monthly_key_metrics_report_spec.rb +++ b/spec/jobs/reports/monthly_key_metrics_report_spec.rb @@ -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 let(:s3_metadata) do { body: anything, @@ -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 subject.perform(report_date) end diff --git a/spec/services/reporting/total_user_count_report_spec.rb b/spec/services/reporting/total_user_count_report_spec.rb index 0c0525b515a..4427bf442b1 100644 --- a/spec/services/reporting/total_user_count_report_spec.rb +++ b/spec/services/reporting/total_user_count_report_spec.rb @@ -5,10 +5,13 @@ 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 @@ -16,58 +19,70 @@ 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