LG 8441 Prioritize ready enrollments by JackRyan1989 · Pull Request #8488 · 18F/identity-idp

JackRyan1989 · 2023-05-25T20:30:51Z

🎫 Ticket

Note: this ticket follows work @NavaTim completed in LG-8440. Pull Request here.

🛠 Summary of changes

Created two new sub-classes of GetUspsProofingResultsJob
Each subclass is set to be called at different frequencies via cron
Each subclass processes different enrollments: enrollments that have been marked 'ready' by our SQS job, and those not marked 'ready'.
Tests have been created for each subclass
Refactored GetUspsProofingResultsJob
Pulled config checks into separate methods
Turned percent error calculation into own method
Moved email related methods into new module
Moved analytics related methods into new module
Created wrapper methods for each analytics method
Removed an error handler method with duplicate internals to other error handler method

📜 Testing Plan

To test locally:

Set the following config values to true in your local dev application.yml file
in_person_proofing_enabled
in_person_enrollments_ready_job_enabled
Check that the GetUspsReadyProofingResultsJob and GetUspsWaitingProofingResultsJob are performed in the workers.log file located at log/workers.log

…nrollment

…-prioritize-ready-enrollments

…om:18F/identity-idp into jryan/lg-8441-prioritize-ready-enrollments

…ck enrollments that are ready

app/jobs/get_usps_waiting_proofing_results_job.rb

config/initializers/job_configurations.rb

app/services/analytics_events.rb

dawei-nava · 2023-05-30T14:25:07Z

app/models/in_person_enrollment.rb

+  def self.needs_status_check_on_waiting_enrollments(check_interval)
+    where(ready_for_status_check: false).
+      and(
+        where(status: :pending),


These two methods have same body. Is the second one status should be :waiting?

@dawei-nava No actually. These two methods are very subtly different. The first where check is the only difference. needs_status_check_on_waiting_enrollments should check for enrollments that are not ready for status check, whereas needs_status_check_on_ready_enrollments checks for enrollments that are ready for a status check.

They are still both pending enrollments, as the user has not yet visited the Post Office and they haven't been expired after the 30 day cutoff yet.

Sorry, i did not notice the ready_for_status_check differences. So if the only difference is ready_for_status_check true of false, just like @NavaTim said, at least these two methods may at least get consolidated into one with boolean as second parameter?

@dawei-nava Yes, I'm going to follow @NavaTim's suggestion. I think that'll hopefully reduce any potential confusion.

dawei-nava · 2023-05-30T14:25:49Z

app/models/in_person_enrollment.rb

+      check_interval.cover?(status_check_attempted_at)
+    )
+  end
+


Same here just method name is different?

@dawei-nava see my answer above.

Got it, only difference is a flag.

app/services/analytics_events.rb

NavaTim · 2023-05-30T18:58:50Z

app/jobs/job_helpers/usps_proofing_results_job/analytics_helper.rb

@@ -0,0 +1,83 @@
+module JobHelpers
+  module UspsProofingResultsJob
+    module AnalyticsHelper


We should consider passing this in as an instance instead of including it as a mixin.

@NavaTim I'm curious, why do you think that's the better approach?

Passing it in as an object makes it easier to test and enforce a contract. That in turn can help increase the confidence that we're not breaking anything when we update either the analytics or the job logic, and the complexity introduced by inheritance is removed.

So @NavaTim I'm running into an issue where fake_analytics.rb is not aware of the analytics calls I'm making from this helper class. I'm not sure if I'm going to need to make a new Fake_Analytics class to be aware of these methods or not. If I need to refactor Fake_analytics.rb to make this change work, I might be inclined to shelve this refactor until a later time and fully separate the work. I think scope creep is starting to become an issue here for me.

It looks like you're making the classes stateful, but it's better to keep them stateless, esp. in a process like this job where many different records are processed that differ in that type of state. I would suggest completely hiding the fact that a new Analytics instance may be constructed inside AnalyticsHelper by taking the User object as a parameter for each of the methods (as you had in the module version).

I expect this will require some changes to tests since all of this logic was previously encapsulated in the job. I would suggest initially going about this by treating the original test as an integration rather than unit test & making sure that it passes with real (not mock) instances of the new classes until after you've completed the refactor.

All that said - I don't think this type of change is a prerequisite for merging the code. It's more important that the job can be configured (and by default is configured) as functionally identical to the original job, but can also be reconfigured to use the ready_for_status_check flag instead.

@NavaTim Yeah, I'm now spending more time on this refactor than the actual requirements of the ticket, so I think I'm going to abandon them for the time being and open a separate ticket.

app/jobs/get_usps_waiting_proofing_results_job.rb

NavaTim · 2023-05-30T19:03:28Z

app/jobs/get_usps_proofing_results_job.rb

+  rescue Faraday::ClientError, Faraday::ServerError, Faraday::Error => err
+    # 4xx or 5xx status code.
+    # These are unexpected but will have some sort of response body that we can try to log data from
+    # Faraday Timeouts, failed connections, parsing errors, and other HTTP errors.
+    # These generally won't have a response body
    handle_client_or_server_error(err, enrollment)


The comment on the new code doesn't cohesively combine the previous comments giving context on these errors.

config/application.yml.default

app/jobs/get_usps_proofing_results_job.rb

dawei-nava · 2023-05-30T20:45:53Z

app/models/in_person_enrollment.rb

Generally speaking I feel we are using our entity models(ApplicationRecord/ActiveRecord) as service/query object, just different ways in different language.

…field

NavaTim

Overall looks good, but I want to confirm the reason for the change in metric rounding before approving.

app/jobs/get_usps_proofing_results_job.rb

spec/models/in_person_enrollment_spec.rb

NavaTim and others added 11 commits May 16, 2023 12:13

LG-8440: Install aws-sdk-ruby; create migration for status check field

d3bbe31

initial class creation and job configuration making

f0fb023

thinking comments

44884ca

subclassing done

d36ab78

Adding appropriate subclass and tests for new methods for in person e…

37052df

…nrollment

add second job

491fa01

Merge branch 'main' of github.com:18F/identity-idp into jryan/lg-8441…

a0ed76e

…-prioritize-ready-enrollments

Trim down specs

a8cb46a

Use helper methods for config checks

2a87ff3

Create new helper modules

fc418f0

Refactor to use helper modules

5a36273

JackRyan1989 requested review from a team and tomas-nava May 25, 2023 20:30

JackRyan1989 self-assigned this May 25, 2023

jack-ryan-nava-pbc added 5 commits May 25, 2023 16:36

Merge branch 'main' into jryan/lg-8441-prioritize-ready-enrollments

b7fdb13

Undoing yarn bomb

1a9e8fc

Merge branch 'jryan/lg-8441-prioritize-ready-enrollments' of github.c…

3ad8abf

…om:18F/identity-idp into jryan/lg-8441-prioritize-ready-enrollments

undoing mystery changes to packages

6c0b27e

changelog: Upcoming Features, In-person Proofing, Add two jobs to che…

4876ac0

…ck enrollments that are ready

svalexander reviewed May 26, 2023

View reviewed changes

app/jobs/get_usps_waiting_proofing_results_job.rb Show resolved Hide resolved

svalexander reviewed May 26, 2023

View reviewed changes

config/initializers/job_configurations.rb Outdated Show resolved Hide resolved

svalexander reviewed May 26, 2023

View reviewed changes

app/services/analytics_events.rb Outdated Show resolved Hide resolved

correct class name in job config and whack with linter

876efc2

dawei-nava reviewed May 30, 2023

View reviewed changes

JackRyan1989 changed the title ~~LG 8441 prioritize ready enrollments - [WIP]~~ LG 8441 prioritize ready enrollments May 30, 2023

jack-ryan-nava-pbc added 3 commits May 30, 2023 10:51

appeasing linter gods

a09e2b2

proper documentation and not nil for job name

070fccb

moving not nil values up

64f7051

JackRyan1989 marked this pull request as ready for review May 30, 2023 16:17

JackRyan1989 requested a review from a team May 30, 2023 16:17

NavaTim reviewed May 30, 2023

View reviewed changes

dawei-nava reviewed May 30, 2023

View reviewed changes

jack-ryan-nava-pbc added 13 commits May 31, 2023 11:00

Cleaning up application

d45f009

cleaning up analytics events

f0f0d77

chaining methods instead of rewriting queries

cac58b0

Updating analytics to remove alteration of job name and instead as a …

3e8a7e9

…field

update error comment

ac168b8

linty lint lint

84d71da

Refactor modules to classes

2a3aff6

Merge branch 'main' into jryan/lg-8441-prioritize-ready-enrollments

ebd40d5

Abandon module refactor

ac715f6

Linty lint lint

5fde952

merging in main

81663f3

Added in check for in_person_enrollments_ready_job_enabled

2de24da

lint fix

b67b88c

JackRyan1989 requested a review from a team June 5, 2023 18:15

JackRyan1989 changed the title ~~LG 8441 prioritize ready enrollments~~ LG 8441 Prioritize ready enrollments Jun 5, 2023

NavaTim reviewed Jun 5, 2023

View reviewed changes

app/jobs/get_usps_proofing_results_job.rb Outdated Show resolved Hide resolved

spec/models/in_person_enrollment_spec.rb Show resolved Hide resolved

spec/models/in_person_enrollment_spec.rb Show resolved Hide resolved

lint and decimal fix

75383cc

NavaTim approved these changes Jun 5, 2023

View reviewed changes

JackRyan1989 merged commit b73775d into main Jun 5, 2023

JackRyan1989 deleted the jryan/lg-8441-prioritize-ready-enrollments branch June 5, 2023 20:59

solipet mentioned this pull request Jun 6, 2023

Deploy RC 286 to Prod #8539

Merged

Conversation

JackRyan1989 commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NavaTim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JackRyan1989 commented May 25, 2023 •

edited

Loading