-
Notifications
You must be signed in to change notification settings - Fork 8
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
DPL-865 handle sprint client response #2130
base: develop
Are you sure you want to change the base?
DPL-865 handle sprint client response #2130
Conversation
[release] Merge Develop into Master
[release] Merge Develop into Master
[release] Merge Develop into Master
…-handle-sprint-client-response
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2130 +/- ##
===========================================
+ Coverage 80.78% 80.80% +0.02%
===========================================
Files 476 476
Lines 18185 18206 +21
Branches 269 269
===========================================
+ Hits 14691 14712 +21
Misses 3492 3492
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Looks good, couple advisories. I haven't tested locally.
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.
I like Bens suggested changes.
My minor one was the error message if sprint is down, can we do something to intercept and change that into something more friendly (might need a separate story).
…r-handle-sprint-client-response
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.
Nice improvements, thanks.
Not sure the user will like the 'try again later' messages, but this is much better than getting no label and nothing at all from LIMS to indicate why. And they can raise a clearer support message if needed.
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.
Looks good, codecov complaining about tests I have mentioned.
expect(SPrintClient).to receive(:send_print_request).with( | ||
printer_sprint.name, | ||
label_template_name_sprint, | ||
labels_sprint.values | ||
) | ||
expect(pj.print_to_sprint).to eq(true) | ||
end | ||
|
||
it 'will not execute if the SPrintClient is down' do | ||
response = Net::HTTPBadGateway.new(1.0, '502', nil) |
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.
Could we also test the errors for Net::HTTPUnprocessableEntity and Net::HTTPInternalServerError as they have separate routes / error messages.
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.
I just added two more tests, but Codecov is still complaining?
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.
Nice, they look good. Think codecov is complaining about coverage in extract_error_message method
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.
Fixed it. Thanks.
Closes #1352
Changes proposed in this pull request
Instructions for Reviewers
[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to
main
]- Check story numbers included
- Check for debug code
- Check version