-
Notifications
You must be signed in to change notification settings - Fork 62
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.
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
migrate usage if IcnJob to UserAccountJob, with Flipper #18538
Conversation
…ts-api into mm/1350-v1
…ts-api into mm/1350-v1
@nathanbwright Can you add a tad more info to your PR description with an estimated timeline/end date for removing the flipper (You say 100%... but that's always the goal :) ). Thanks! |
@ryan-mcneil Added the following:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Backend-review-group approval confirmed. |
* updates documentation RE new job * attempts to replace all instances of ICNJob with UserAccountJob * adds va_notify_user_account_job flipper * updates in_progress_form_reminder and specs * updates one_time_in_progress_reminder and spec * updates user_account_job and spec * lint * 2nd attempt to clean lint * runs bundle exec rubocop * extrapolates abstracted logic that relates to a temporary flipper
Summary
before this PR:
IcnJob
, which accepted an ICN number as a parameter. A newUserAccountJob
had previously been created, though it hadn't been implemented throughout the moduleafter this PR:
:va_notify_user_account_job
to aid in the controlled velocity and rollout of this newUserAccountJob
. All instances (3) ofIcnJob
have been replaced by theUserAccountJob
. Also, the README for VANotify was updated.Yes
The only instances of using the
IcnJob
were internal to VANotifyNA
This solution works because it does not pass an ICN as a parameter, which removes the potential for an ICN to appear within a sidekiq log
VANotify
Success criteria being targeted is 100% usage of the
UserAccountJob
instead ofIcnJob
The plan for rollout would be a typical rollout cadence using a flipper, 25%, 50%, and then 100%, with 2 days at each interval coupled with monitoring of effectiveness/errors/logs etc. Rolling out from 0 -> 100% over the course of 6 business days.
Related issue(s)
https://github.com/department-of-veterans-affairs/vanotify-team/issues/1350
This is a link to the PR which created the
UserAccountJob
#18215
NA
Testing done
The flipper is used as part of some basic if/else logic; If the Flipper is enabled, user the
UserAccountJob
, otherwise, use theIcnJob
, while we rollout the new job. Hence, existing/passing tests for theIcnJob
are used to verify flipper off scenarios.Screenshots
Note: Optional
What areas of the site does it impact?
(Describe what parts of the site are impacted andifcode touched other areas)
Acceptance criteria
Requested Feedback
(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?
In the
OneTimeInProgressReminder
, user_account.id is passed as the initial argument; Coulduser_account_id
have been passed instead?