Skip to content

Remove proofing costs report, associated data (LG-8028) #7345

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zachmargolis merged 4 commits into from
Nov 16, 2022
Merged

Remove proofing costs report, associated data (LG-8028) #7345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e4ec611
Remove proofing costs report, associated data (LG-8028)
zachmargolis Nov 15, 2022
b94634a
Remove a few more stragglers
zachmargolis Nov 15, 2022
8417968
lint on unused user_id
zachmargolis Nov 15, 2022
9a12ae4
Remove ProofingCost model reference in an old migration
zachmargolis Nov 15, 2022
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
5 changes: 3 additions & 2 deletions app/jobs/address_proofing_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ class AddressProofingJob < ApplicationJob

discard_on JobHelpers::StaleJobHelper::StaleJobError

def perform(user_id:, issuer:, result_id:, encrypted_arguments:, trace_id:)
# rubocop:disable Lint/UnusedMethodArgument
def perform(issuer:, result_id:, encrypted_arguments:, trace_id:, user_id: nil)
Comment on lines +8 to +9
Copy link
Copy Markdown
Contributor

@aduth aduth Nov 15, 2022

Choose a reason for hiding this comment

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

I'm a bit confused by what I'm seeing in #7346 changes, but just to confirm: The unused argument here is temporary during deployment, and we'll circle back to remove it and the Rubocop inline disabling?

Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Nov 15, 2022

Choose a reason for hiding this comment

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

Yeah the two PRs got out of sync a bit as I was patching more issues

My current thinking is, maybe we don't want to remove the user_id as an argument? Since if we ever wanted to add analytics logging, it would be useful to have, etc

timer = JobHelpers::Timer.new

raise_stale_job! if stale_job?(enqueued_at)
Expand All @@ -25,7 +26,6 @@ def perform(user_id:, issuer:, result_id:, encrypted_arguments:, trace_id:)
Db::SpCost::AddSpCost.call(
service_provider, 2, :lexis_nexis_address, transaction_id: proofer_result.transaction_id
)
Db::ProofingCost::AddUserProofingCost.call(user_id, :lexis_nexis_address)

document_capture_session = DocumentCaptureSession.new(result_id: result_id)
document_capture_session.store_proofing_result(proofer_result.to_h)
Expand All @@ -39,6 +39,7 @@ def perform(user_id:, issuer:, result_id:, encrypted_arguments:, trace_id:)
}.to_json,
)
end
# rubocop:enable Lint/UnusedMethodArgument

private

Expand Down
21 changes: 0 additions & 21 deletions app/jobs/reports/proofing_costs_report.rb
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions app/models/proofing_cost.rb
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion app/services/db/add_document_verification_and_selfie_costs.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ def call(client_response)

def add_cost(token)
Db::SpCost::AddSpCost.call(service_provider, 2, token)
Db::ProofingCost::AddUserProofingCost.call(user_id, token)
end
end
end
32 changes: 0 additions & 32 deletions app/services/db/proofing_cost/add_user_proofing_cost.rb
View file
Open in desktop

This file was deleted.

47 changes: 0 additions & 47 deletions app/services/db/proofing_cost/proofing_costs_summary.rb
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion app/services/gpo_confirmation_maker.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ def generate_otp
end

def update_proofing_cost
Db::ProofingCost::AddUserProofingCost.call(profile&.user&.id, :gpo_letter)
Db::SpCost::AddSpCost.call(service_provider, 2, :gpo_letter)
end

Expand Down
5 changes: 0 additions & 5 deletions app/services/idv/send_phone_confirmation_otp.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ def send_otp
domain: IdentityConfig.store.domain_name,
country_code: parsed_phone.country,
)
add_cost
otp_sent_response
end

Expand All @@ -71,10 +70,6 @@ def otp_sent_response
)
end

def add_cost
Db::ProofingCost::AddUserProofingCost.call(user.id, :phone_otp)
end

def extra_analytics_attributes
{
otp_delivery_preference: delivery_method,
Expand Down
1 change: 0 additions & 1 deletion app/services/idv/steps/doc_auth_base_step.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ def user_id

def add_cost(token, transaction_id: nil)
Db::SpCost::AddSpCost.call(current_sp, 2, token, transaction_id: transaction_id)
Db::ProofingCost::AddUserProofingCost.call(user_id, token)
end

def add_costs(result)
Expand Down
6 changes: 0 additions & 6 deletions config/initializers/job_configurations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,6 @@
cron: cron_24h,
args: -> { [Time.zone.today] },
},
# Proofing Costs Report to S3
proofing_costs: {
class: 'Reports::ProofingCostsReport',
cron: cron_24h,
args: -> { [Time.zone.today] },
},
# Combined Invoice Supplement Report to S3
combined_invoice_supplement_report: {
class: 'Reports::CombinedInvoiceSupplementReport',
Expand Down
4 changes: 0 additions & 4 deletions db/primary_migrate/20200803211145_backfill_add_acuant_result_to_proofing_costs.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,5 @@ class BackfillAddAcuantResultToProofingCosts < ActiveRecord::Migration[5.2]
disable_ddl_transaction!

def up
Copy link
Copy Markdown
Contributor

@aduth aduth Nov 15, 2022

Choose a reason for hiding this comment

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

A question from my own ignorance: Could we remove this migration file altogether?

Copy link
Copy Markdown
Contributor

@aduth aduth Nov 15, 2022

Choose a reason for hiding this comment

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

Related question: We only need to do anything here because the Ruby class implemlentation wouldn't exist, right? So it's not a problem that other migrations would continue to reference the (later-deleted) tables, since they'd be present at that point in the migration timeline?

identity-idp/db/primary_migrate/20220906112214_add_threatmetrix_to_proofing_cost.rb

Line 5 in d5081da

add_column :proofing_costs, :threatmetrix_count, :integer

Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Nov 15, 2022

Choose a reason for hiding this comment

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

Yeah exactly, it's because the ProofingCost model was removed, so this blew up with a name error.

Another option would have been to redo this as raw SQL because at the time it runs, the table would exist.

I guess we could remove the class, but the timestamp slug would still exist in the schema_migrations table? But it's not like we check that very often so it's probably not a big deal.

Do you think it would be clearer to remove entirely?

Copy link
Copy Markdown
Contributor

@aduth aduth Nov 16, 2022

Choose a reason for hiding this comment

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

I was mostly curious to improve my understanding of the migrations, but I don't have a strong feeling one way or the other. I think practically speaking this is almost certainly something we won't need. I'd defer to what you think is best, and generally would be fine to keep it as-is.

Copy link
Copy Markdown
Contributor Author

@zachmargolis zachmargolis Nov 16, 2022

Choose a reason for hiding this comment

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

ok! Ill choose to leave it, because I think for future us, it's easier to look back at the history of a mostly empty file that does exist, rather than search history for a file that used to exist

ProofingCost.unscoped.in_batches do |relation|
relation.update_all acuant_result_count: 0
sleep(0.01)
end
end
end
54 changes: 0 additions & 54 deletions spec/features/reports/proofing_costs_report_spec.rb
View file
Open in desktop

This file was deleted.

9 changes: 1 addition & 8 deletions spec/jobs/address_proofing_job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
{ applicant_pii: applicant_pii }.to_json,
)
end
let(:user_id) { SecureRandom.random_number(1000) }
let(:service_provider) { create(:service_provider) }
let(:applicant_pii) do
{
Expand All @@ -27,7 +26,6 @@
result_id: document_capture_session.result_id,
encrypted_arguments: encrypted_arguments,
trace_id: trace_id,
user_id: user_id,
issuer: service_provider.issuer,
)

Expand All @@ -45,7 +43,6 @@
result_id: document_capture_session.result_id,
encrypted_arguments: encrypted_arguments,
trace_id: trace_id,
user_id: user_id,
issuer: service_provider.issuer,
)
end
Expand Down Expand Up @@ -86,15 +83,11 @@
end

it 'adds cost data' do
expect { perform }.
to(change { SpCost.count }.by(1).and(change { ProofingCost.count }.by(1)))
expect { perform }.to(change { SpCost.count }.by(1))

sp_cost = SpCost.last
expect(sp_cost.issuer).to eq(service_provider.issuer)
expect(sp_cost.transaction_id).to eq(conversation_id)

proofing_cost = ProofingCost.last
expect(proofing_cost.user_id).to eq(user_id)
end
end

Expand Down
5 changes: 0 additions & 5 deletions spec/services/idv/steps/verify_wait_step_show_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,6 @@
expect(sp_costs[0].transaction_id).to eq(resolution_transaction_id)
sp_costs = SpCost.where(issuer: issuer, cost_type: 'threatmetrix')
expect(sp_costs[0].transaction_id).to eq(threatmetrix_transaction_id)

proofing_cost = ProofingCost.last
expect(proofing_cost.user_id).to eq(user.id)
expect(proofing_cost.threatmetrix_count).to eq(1)
expect(proofing_cost.lexis_nexis_resolution_count).to eq(1)
end

it 'clears pii from session' do
Expand Down