-
Notifications
You must be signed in to change notification settings - Fork 4
Removed send_message_batch functionality #802
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
Conversation
steventux
left a comment
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.
We need to remove smb (send message batch) job here: https://github.com/NHSDigital/dtos-manage-breast-screening/blob/main/scripts/python/smoke_test/notifications_smoke_test.py#L26
Smoke tests don't run on review apps but they do run on dev and preprod.
Cheers @steventux - missed that one. 🤦 Done now |
steventux
left a comment
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.
If you are removing the ApiClient in this PR, it probably makes sense to also remove the message batch presenter.
Have we coordinate with Manage around removing internals like the client and presenter? I know we don't need to hang on to unused code but these things might be useful sooner rather than later. It might be more prudent just to remove the commands for now.
Fair point, maybe I got a little over-eager. :D I'll put the ApiClient back and let Manage decide its fate |
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 think the RoutingPlan configuration class might also be a candidate for removal as it is very tailored to episode type but not necessarily in this PR. I assume we will have a few internals like this we can review later. The main thing is to remove unnecessary commands.
Only other outlier I can think of is how the removal of this might affect monitoring?
|
Yeah, I think we need to have a think about monitoring on the whole anyway given the changes in approach. And agreed re: RoutingPlan. |
This removes the send_message_batch functionality which is no longer being used, as well as the ApiClient class which was only being used by send_message_batch.
Also updates documentation to reflect both send_message_batch and retries being removed.
Original ticket here.