Remove proofing costs report, associated data (LG-8028)#7345
Remove proofing costs report, associated data (LG-8028)#7345zachmargolis merged 4 commits intomainfrom
Conversation
changelog: Internal, Reporting, Remove unused reporting code
| # rubocop:disable Lint/UnusedMethodArgument | ||
| def perform(issuer:, result_id:, encrypted_arguments:, trace_id:, user_id: nil) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| @@ -2,9 +2,5 @@ class BackfillAddAcuantResultToProofingCosts < ActiveRecord::Migration[5.2] | |||
| disable_ddl_transaction! | |||
|
|
|||
| def up | |||
There was a problem hiding this comment.
A question from my own ignorance: Could we remove this migration file altogether?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
2 participants
🎫 Ticket
LG-8028
🛠 Summary of changes
Confirmed in Slack this is not used, next up will be dropping this table.
We still use the sp_costs table, so that will be sticking around.