4619 sis workers #11663
4619 sis workers #11663
Conversation
Generated by 🚫 Danger |
| @@ -4,11 +4,11 @@ | |||
| require_relative '../support/iam_session_helper' | |||
There was a problem hiding this comment.
The changes I made to these specs are difficult to read in the diff. I decided to remove some of the tests because I didn't think they were adding anything except complexity that made these tests needlessly difficult to refactor. They were testing feature flag and other appointment behaviors that are already tested in the appointments list request specs. I'm sure I added some of these tests myself, but at this time I'd rather keep them simple and do the more thorough testing in the integration (request) specs. It may be easier to read these specs by just looking at the file since the diff is so complex.
| @@ -18,8 +18,6 @@ | |||
|
|
|||
| after(:all) { VCR.configure { |c| c.cassette_library_dir = @original_cassette_dir } } | |||
|
|
|||
| let(:user) { FactoryBot.build(:iam_user) } | |||
There was a problem hiding this comment.
In all of these tests, I've switched to using User instead of IAMUser as the base case because 1) it's simpler to set up 2) it's the way of the future. So I've done most of the testing with User and also confirmed that it works with IAMUser.
| @@ -7,15 +7,19 @@ class PreCacheAppointmentsJob | |||
|
|
|||
| sidekiq_options(retry: false) | |||
|
|
|||
| class MissingUserError < StandardError; end | |||
There was a problem hiding this comment.
Adding these error classes because I want to see it in sentry if this is happening and at least one of these jobs effectively hides the error from us. We can probably remove this if we find that it never occurs.
There was a problem hiding this comment.
If that works, that'd be a great way to fight the noise that Sentry puts out.
| Mobile::AppointmentsCacheInterface.new.fetch_appointments(user: user, fetch_cache: false) | ||
|
|
||
| Rails.logger.warn('mobile appointments pre-cache success', user_uuid: uuid) | ||
| Rails.logger.info('mobile appointments pre-cache success', user_uuid: uuid) |
There was a problem hiding this comment.
IDK why we use info at the start of the job then warn that it succeeded.
| @@ -7,15 +7,19 @@ class PreCacheAppointmentsJob | |||
|
|
|||
| sidekiq_options(retry: false) | |||
|
|
|||
| class MissingUserError < StandardError; end | |||
There was a problem hiding this comment.
If that works, that'd be a great way to fight the noise that Sentry puts out.
| it 'caches the expected appointments' do | ||
| VCR.use_cassette('appointments/get_facilities', match_requests_on: %i[method uri]) do | ||
| VCR.use_cassette('appointments/get_cc_appointments_default', match_requests_on: %i[method uri]) do | ||
| VCR.use_cassette('appointments/get_appointments_default', match_requests_on: %i[method uri]) do |
There was a problem hiding this comment.
some of these specs are still using v0 appointments. In the current draft PR for ripping out v0 appointments, we're removing all v0 specs with future tickets to make v2 versions of them. so these will be removed soon. Could change them to v2 now while your working with them or do it later.
Summary
When we added SIS login, we didn't notice that our background jobs has logic for looking up IAM users. This has been causing background workers to fail for SIS users but the issue wasn't obvious because:
As a result, the only error found in sentry (which led us to this bug) is this one that was only occurring in staging:
http://sentry.vfs.va.gov/organizations/vsp/issues/176633/?project=3&query=is%3Aunresolved+mobile&statsPeriod=14d
Related issue(s)
Testing done
Specs
What areas of the site does it impact?
Mobile background jobs
Acceptance criteria
Requested Feedback
Any and all.