-
-
Notifications
You must be signed in to change notification settings - Fork 272
[WW][Sutton] Add flow for container request cancellations. #5742
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
[WW][Sutton] Add flow for container request cancellations. #5742
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5742 +/- ##
==========================================
- Coverage 82.64% 82.61% -0.04%
==========================================
Files 457 458 +1
Lines 35710 35761 +51
Branches 5821 5828 +7
==========================================
+ Hits 29514 29545 +31
- Misses 4501 4516 +15
- Partials 1695 1700 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
702a876 to
3bd730f
Compare
dracos
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.
Only real request is to make the Sutton specific things the default, with a feature flag; other two are just question/comments :)
dracos
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.
Only the one thing, sorry :)
4e1eaac to
254d96f
Compare
dracos
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.
Thanks :)
71bdd27 to
f316a4c
Compare
|
New commits address bugs from https://github.com/mysociety/societyworks/issues/5269. |
dracos
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.
Looks good :)
|
@dracos taking the approach of letting the update logic filter out the echo cancellation because of the matching text & user, as discussed. |
a8f77b7 to
cfe14a5
Compare
dracos
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.
👍
58594b8 to
b67f7fb
Compare
|
New changes:
|
dracos
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.
Looks fine - I guess only question is if in this state we don't want to have an update at all (but then, I think we won't because the text and state will match so it's about to get hidden anyway).
| if ($p->category eq 'Request new container' | ||
| && $request->{status} eq 'cancelled' | ||
| && $p->state eq 'cancelled') { | ||
| $updates->suppress_alerts(1); |
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.
Just checking (I think we discussed this, but have forgotten) - this is just in case it's not already caught/hidden by the 'identical update' checking (now that we use the template)? Do we want to only suppress the alert, or if we're here, skip the update entirely?
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.
Yeah on staging the update was still getting through, I guess because the template was not picked up so text wasn't matching, but I thought rather than figure that out better to just be explicit anyway. The reason I went for suppression rather than skipping was in case we wanted to keep a log on the report of when we get confirmation Echo that it was actually cancelled.
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.
Oh yes, the template isn't being picked up because there isn't a state change (so it's not even looking); I wonder if it should be in that case, but that seems like a bigger thing.
Remove unused 'reference' parameter and fix doc text.
b67f7fb to
097c9a3
Compare
[skip changelog]
for https://github.com/mysociety/societyworks/issues/4805