-
-
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
[Northumberland] Fetch 'detailed_information' (extra details) to/from Alloy & assigned user to Alloy #5147
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5147 +/- ##
=========================================
Coverage ? 82.51%
=========================================
Files ? 406
Lines ? 31732
Branches ? 5049
=========================================
Hits ? 26185
Misses ? 4056
Partials ? 1491 ☔ View full report in Codecov by Sentry. |
224a88b
to
828c26e
Compare
bb6d52d
to
10ac3cd
Compare
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.
All clear and makes sense :) A few suggestions, shout if don't make sense
&& ( $problem->get_extra_metadata('detailed_information') // '' ) | ||
ne $old_detailed_information | ||
) { | ||
$update_params{extra}{detailed_information} = 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.
I guess issue here is if someone edits the extra detail, and the assignment, that has two updates rather than combining them in the one. Do we think that matters? I'm not sure. I like the way you've hooked it round planned_reports to catch any usage there, I wonder if there's some way that can be hooked in here.
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 ^.^ I didn't think it mattered - and trying to consolidate them into one update seemed like it would be a headache, given that the assigned user logic kind of exists in its own bubble.
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.
Am still wondering, though, what I should do about the updates displaying on the /report/<id> page :-/
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.
You can see in report/updates.html there's a "IF update.get_extra_metadata triage_report" type thing to skip those updates. Perhaps we take this opportunity to add a staff_only
flag to updates (for there as well) and have it use that to skip these updates. Seems easier than trying to decide based on contents (given the updates in the template can include questionnaire replies as well etc).
Not sure that's easy for the update on detailed information, though, hmm.
10ac3cd
to
818dabf
Compare
c02aed2
to
de608ee
Compare
de608ee
to
702d107
Compare
For https://mysocietysupport.freshdesk.com/a/tickets/3082 / https://github.com/mysociety/societyworks/issues/4507.
[skip changelog]