LG-13387 Remove SP cost tracking logic from VerifyInfoConcern#10786
Merged
LG-13387 Remove SP cost tracking logic from VerifyInfoConcern#10786
VerifyInfoConcern#10786Conversation
mitchellhenke
approved these changes
Jun 10, 2024
Contributor
Author
|
I am marking this "do not merge" until Tuesday's deploy when #10767 should be deployed. |
jmhooper
added a commit
that referenced
this pull request
Jun 11, 2024
I was looking at the changes in #10786 for the move of SP cost tracking from the `VerifyInfoConcern` to the `ProgressiveProofer` when I noticed this condition that was not accounted for: https://github.com/18F/identity-idp/pull/10786/files#diff-5d8b1370b1c579364bb79a0813ca9bf9a15c65e7a974829886ab3e00f3441cedL310 This logic prevents a AAMVA cost from being added if an AAMVA exception occurs. Presumably this is because we are not billed for AAMVA exception which happen frequently. This commit applies that logic to the `ProgressiveProofer` where SP costs are now tracked. [skip changelog]
In #10767 we changed the `ProgressiveProofer` class to begin tracking SP costs closer to where costly events occur. The logic for tracking costs was left in the `VerifyInfoConcern` to deal with the 50/50 state. When the changes in #10767 are fully deployed this can be merged to clean up dead code. [skip changelog]
jmhooper
force-pushed
the
jmhooper-remove-verify-info-cost-tracking
branch
from
fdc5f22 to
e3113e9
Compare
jmhooper
added a commit
that referenced
this pull request
Jun 11, 2024
I was looking at the changes in #10786 for the move of SP cost tracking from the `VerifyInfoConcern` to the `ProgressiveProofer` when I noticed this condition that was not accounted for: https://github.com/18F/identity-idp/pull/10786/files#diff-5d8b1370b1c579364bb79a0813ca9bf9a15c65e7a974829886ab3e00f3441cedL310 This logic prevents a AAMVA cost from being added if an AAMVA exception occurs. Presumably this is because we are not billed for AAMVA exception which happen frequently. This commit applies that logic to the `ProgressiveProofer` where SP costs are now tracked. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 13, 2024
The `VerifyInfoConcern` used to add SP costs to the database by inspecting the result from the `ResolutionProofingJob`. We have made the following changes in an effort to move the logic for tracking SP costs into the resolution proofing job: - In #10743 we added a conditional to the `VerifyInfoConcern` to stop writing SP costs if a `sp_costs_added` flag is present on the result - In #10767 we started adding costs in the `ResolutionProofingJob` and had the job set the `sp_costs_added` flag - In #10786 we removed the code that added the SP costs in the `VerifyInfoConcern` This commit includes the changes for the final step: Removing the `sp_costs_added` flag now that nothing is reading it. This commit can be safely merged into main when all of the above changes are fully deployed. [skip changelog]
brandemix
pushed a commit
to brandemix/18F-identity-idp
that referenced
this pull request
Jun 17, 2024
I was looking at the changes in 18F#10786 for the move of SP cost tracking from the `VerifyInfoConcern` to the `ProgressiveProofer` when I noticed this condition that was not accounted for: https://github.com/18F/identity-idp/pull/10786/files#diff-5d8b1370b1c579364bb79a0813ca9bf9a15c65e7a974829886ab3e00f3441cedL310 This logic prevents a AAMVA cost from being added if an AAMVA exception occurs. Presumably this is because we are not billed for AAMVA exception which happen frequently. This commit applies that logic to the `ProgressiveProofer` where SP costs are now tracked. [skip changelog]
brandemix
pushed a commit
to brandemix/18F-identity-idp
that referenced
this pull request
Jun 17, 2024
…10786) In 18F#10767 we changed the `ProgressiveProofer` class to begin tracking SP costs closer to where costly events occur. The logic for tracking costs was left in the `VerifyInfoConcern` to deal with the 50/50 state. When the changes in 18F#10767 are fully deployed this can be merged to clean up dead code. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 20, 2024
The `VerifyInfoConcern` used to add SP costs to the database by inspecting the result from the `ResolutionProofingJob`. We have made the following changes in an effort to move the logic for tracking SP costs into the resolution proofing job: - In #10743 we added a conditional to the `VerifyInfoConcern` to stop writing SP costs if a `sp_costs_added` flag is present on the result - In #10767 we started adding costs in the `ResolutionProofingJob` and had the job set the `sp_costs_added` flag - In #10786 we removed the code that added the SP costs in the `VerifyInfoConcern` This commit includes the changes for the final step: Removing the `sp_costs_added` flag now that nothing is reading it. This commit can be safely merged into main when all of the above changes are fully deployed. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 20, 2024
The `VerifyInfoConcern` used to add SP costs to the database by inspecting the result from the `ResolutionProofingJob`. We have made the following changes in an effort to move the logic for tracking SP costs into the resolution proofing job: - In #10743 we added a conditional to the `VerifyInfoConcern` to stop writing SP costs if a `sp_costs_added` flag is present on the result - In #10767 we started adding costs in the `ResolutionProofingJob` and had the job set the `sp_costs_added` flag - In #10786 we removed the code that added the SP costs in the `VerifyInfoConcern` This commit includes the changes for the final step: Removing the `sp_costs_added` flag now that nothing is reading it. This commit can be safely merged into main when all of the above changes are fully deployed. [skip changelog]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #10767 we changed the
ProgressiveProoferclass to begin tracking SP costs closer to where costly events occur. The logic for tracking costs was left in theVerifyInfoConcernto deal with the 50/50 state. When the changes in #10767 are fully deployed this can be merged to clean up dead code.[skip changelog]