Allow any cobrand to display public asset ID on a report #3920
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3920 +/- ##
==========================================
- Coverage 82.64% 74.27% -8.37%
==========================================
Files 354 291 -63
Lines 24362 19603 -4759
Branches 3685 3682 -3
==========================================
- Hits 20133 14560 -5573
- Misses 3081 3866 +785
- Partials 1148 1177 +29
Continue to review full report at Codecov.
|
nephila-nacrea
force-pushed
the
issues/public/1959-show-asset-num-report-page
branch
4 times, most recently
from
9e391f2
to
28d610d
Compare
nephila-nacrea
changed the title
nephila-nacrea
changed the title
|
Looking at this again I am not sure if it would be easier to have the config in the cobrand module rather than general.yml (which will have to be deployed beforehand on mysociety-servers). But I am leaving it up for review for now. |
| @@ -1,5 +1,5 @@ | |||
| [% IF problem.bodies_str %] | |||
| [% INCLUDE 'report/_council_sent_info.html' %] | |||
| [% IF problem.whensent %] | |||
There was a problem hiding this comment.
This doesn't need to check whensent - we have the asset ID as soon as the report is made, so can show it.
Also we don't want to print out "Not reported to council" at this point if it's not been sent, this is nothing to do with asset IDs and would change lots of existing reports. This is presumably trying to keep the Oxfordshire behaviour, so you need to do that but also not change base behaviour more than is required - so eg the base template including _cobrand_ref.html and nothing else, then Oxfordshire having its own version of that template printing out the "Not reported to council".
The logic before wasn't entirely inconsistent - the bodies_str check Oxfordshire's main_after was doing matched the _main_sent_info check base was doing. But hopefully the above makes it clearer - base _main_after can be simple, always try and show asset IDs, nothing more, fixmystreet.com's version can show the extra stuff for Oxfordshire reports, and Oxfordshire can similarly change that to keep its current behaviour.
There was a problem hiding this comment.
Have tried to get this back to a similar setup as before, though I don't like having the same large chunk of HTML in two files >.<
nephila-nacrea
force-pushed
the
issues/public/1959-show-asset-num-report-page
branch
from
28d610d
to
3b89956
Compare
There was a problem hiding this comment.
Almost, just needs couple more tweaks to template layout and one hook fix.
I think you want:
- templates/web/base/report/_cobrand_ref.html - your current _cobrand_ref_default.html
- templates/web/base/report/_main_after.html - includes report/_cobrand_ref.html
- templates/web/fixmystreet.com/report/_main_after.html - your current base _main_after.html
- templates/web/fixmystreet.com/report/_cobrand_ref_oxfordshire.html - not in base
Others fine as is. Do ask if that doesn't make sense :)
nephila-nacrea
force-pushed
the
issues/public/1959-show-asset-num-report-page
branch
3 times, most recently
from
829cc22
to
4b77b1a
Compare
| [% asset_id = problem.public_asset_id; IF asset_id %] | ||
| <p><strong>Asset ID:</strong> [% asset_id %]</p> | ||
| [% END %] | ||
| [% IF column_no %] |
There was a problem hiding this comment.
No need to change it back, but note this was deliberate indentation on our part, not a mistake; it can be quite nice in templates to have HTML indented in 4, and the logic to also be indented in 4 but offset by 2, which makes it easy to follow when as here there are conditional and non-conditional parts of the HTML output.
nephila-nacrea
force-pushed
the
issues/public/1959-show-asset-num-report-page
branch
from
4b77b1a
to
0fcd606
Compare
nephila-nacrea
changed the title
|
Merged to master but the way I merged seems to mean Github did not pick up on it. |
nephila-nacrea
force-pushed
the
issues/public/1959-show-asset-num-report-page
branch
from
0fcd606
to
f557eaa
Compare
Fixes https://github.com/mysociety/societyworks/issues/1959.
Oxfordshire already has functionality for displaying a publicly viewable 'asset ID' (among other data) on each report; this PR extends this functionality so it can be applied to other cobrands, should they so wish. A list of extra fields that can be used as the asset ID can be defined under COBRAND_FEATURES in general.yml.