Conversation
[skip changelog]
This builds on Luis's unmerged PR (#9299) and needs a test still. changelog: Internal, Reporting, Monthly report includes all-time user count
|
|
||
| upload_to_s3(account_reuse_data, report_name: 'account_reuse') | ||
| upload_to_s3(total_profiles_data, report_name: 'total_profiles', ) | ||
| upload_to_s3(total_users_all_time_table, report_name: 'total_users_all_time') |
There was a problem hiding this comment.
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.
| let(:expected_count) { 2 } | ||
|
|
||
| it 'includes the suspended user in the count' do | ||
| expect(suspended_user).to be_suspended |
There was a problem hiding this comment.
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.
|
|
||
| it 'includes the suspended user in the count' do | ||
| expect(suspended_user).to be_suspended | ||
| expect(subject.total_user_count_report).to eq(expected_report) |
There was a problem hiding this comment.
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.)
|
Superseded by #9350 |
2 participants
🎫 Ticket
LG-10812
🛠 Summary of changes
This presently builds on an unmerged PR so it won't really make sense on main just yet.