-
-
Notifications
You must be signed in to change notification settings - Fork 235
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
[Bexley] Look up service description #5165
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5165 +/- ##
===========================================
- Coverage 82.49% 69.24% -13.25%
===========================================
Files 406 404 -2
Lines 31708 31634 -74
Branches 5042 5033 -9
===========================================
- Hits 26158 21906 -4252
- Misses 4063 8244 +4181
+ Partials 1487 1484 -3 ☔ View full report in Codecov by Sentry. |
@@ -7,6 +8,7 @@ | |||
ELSIF is_missed_collection; | |||
title = report.title; | |||
title = title.remove('Report missed '); | |||
title = title _ ' (' _ containers.$title _ ')'; |
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.
This feels a bit fragile, to rely upon a title text string lookup. I may be missing something here, but if "Report missed" is being removed from the title, why are we adding it in the first place in Bexley/Waste.pm
? Could we not add it when we construct the title at report creation, also add the type of container to the title there, and then we don't need to add it here or remove any text. I don't think anything expects the title to be of a particular format.
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've not checked whether there is any other objection to not adding 'Report missed' to the service, but does seem to make sense...
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.
One bug, I think, "Various" handling looks good
7994409
to
9768b68
Compare
@chrismytton As I made the fix here, could you give this a once over |
Adds the service description to the report title, so it's e.g. included in the template email. https://mysocietysupport.freshdesk.com/a/tickets/4449
9768b68
to
91bc758
Compare
Adds the service description to the template email so the service and container are shown.
https://mysocietysupport.freshdesk.com/a/tickets/4449
[skip changelog]