Fix parent_document_url not being sent to asset manager for consultation outcome attachments [WHIT-3466]#11469
Fix parent_document_url not being sent to asset manager for consultation outcome attachments [WHIT-3466]#11469eYinka wants to merge 6 commits into
Conversation
ChrisBAshton
left a comment
There was a problem hiding this comment.
Approach looks good 👍 nice one getting to the bottom of it.
In essence, parent_document_url is only reliably set at document publish time. Extending attachable_url to handle non-edition attachables with a public URL would be great as a follow-up work.
Interested in what that looks like. Care to write up a backlog item for it? 🙏
|
|
||
| delegate :unpublishing, to: :parent_attachable | ||
|
|
||
| delegate :public_url, to: :parent_attachable |
There was a problem hiding this comment.
Please roll this out to CallForEvidence too - near-identical document type.
306219f to
2bdc282
Compare
|
Had a quick chat with Ola - changed PR to draft and added a do not merge label to this as I think running the publish job would incorrectly publish draft assets. We'll be looking into using the Metadata updater. |
Just to clarify, the rake task isn't on this PR yet. My original intention was to make a separate PR after merging this update. Nonetheless, let's see if we can do a global fix for both cases in this PR. My rough thought for the known limitation would be adding something like: to whitehall/app/models/attachment_data.rb Lines 176 to 183 in 0f5896d |
2bdc282 to
cba8239
Compare
| end | ||
| end | ||
|
|
||
| def url_for(edition) |
There was a problem hiding this comment.
I am certain this method exists elsewhere - more or less the same
There was a problem hiding this comment.
I've checked for this but didn't find any instance.
…type attachments When a consultation or call for evidence is published, `PublishAttachmentAssetJob` updates each attachment's metadata in asset manager. For attachments belonging to a ConsultationOutcome, ConsultationPublicFeedback, or CallForEvidenceOutcome, `parent_document_url` was never sent because the job guards with `respond_to?(:public_url)`, and neither ConsultationResponse nor CallForEvidenceResponse implemented that method. I've extended ConsultationResponse and CallForEvidenceResponse, so the PublishAttachmentAssetJob can use their parent document's public URL for associated attachments' `parent_document_url`. Known limitation: When saving a draft edition of a Consultation Outcome/Public Feedback or Call for Evidence Outcome, the `attachable_url` in AttachmentData is used to get the `parent_document_url` to be sent to Asset Manager. However, this will still return `nil`. This is because that method calls `visible_edition_for` which filters to Edition subclasses only, and neither ConsultationResponse nor CallForEvidenceResponse is an Edition. As a result, `parent_document_url` is only reliably set at publish time, not on every metadata update. Extending `attachable_url` to handle non-edition attachables with a public URL would be great as a follow-up work.
e4c0568 to
5ae5179
Compare
The previous code did not work at all for policy groups, and consultation and call for evidence responses, which are not editions. Since the access limitation restrictions should already kick in before this asset payload is constructed, there is no reason to check for visibility at this level. We have removed the calls to `visible_edition` in favour of just using the `significant_attachable`. This should correctly return either visible attachables or the draft attachable. This works correctly for all attachable types, including responses and policy groups. In order to correctly construct the draft or live attachable URL, we need to determine if it is in draft state. Policy groups are immediately published, so this does not apply to them. For responses, we want to send the parent edition's URL, so we can check the parent state. I have made the `parent_attachable` method public, so we can identify response-like classes - this method is only defined on `ConsultationResponse` and `CallForEvidenceResponse` at the moment. For context, the method was previously never called with a user argument to check the visibility for. The line `visible_edition.blank? && draft_edition` was particularly confusing, since the visible edition should also return the draft_edition, if called with a user. It also invalidated the whole point of ever checking for visibility, when falling back to the draft_edition anyway.
5ae5179 to
9af1987
Compare
Because ConsultationResponse, CallForEvidenceResponse, and PolicyGroup are attachables that are not editionable, they don't have a parent document URL in Asset Manager. This task backfills the parent document URL for attachments associated with these attachables. Run with `bundle exec rake backfill_parent_document_url_for_non_editionable_attachables`
When a consultation or call for evidence is published,
PublishAttachmentAssetJobupdates each attachment's metadata in asset manager. For attachments belonging to aConsultationOutcome,ConsultationPublicFeedback, orCallForEvidenceOutcome,parent_document_urlwas never sent because the job guards withrespond_to?(:public_url), and neitherConsultationResponsenorCallForEvidenceResponseimplemented that method.Similarly, when we save a draft or update the metadata of an attachment,
AssetManagerAttachmentMetadataJobcallsattachable_urlonAttachmentDatato get theparent_document_url. This also returnednilfor these types becauseattachable_urlonly handled Edition subclasses and Policy Group; neitherConsultationResponsenorCallForEvidenceResponsewas covered.We've made two fixes:
Extended
ConsultationResponseandCallForEvidenceResponseto delegatepublic_urlto theirparent_attachable, soPublishAttachmentAssetJobcan use their parent document's public URL forparent_document_url.Extended
attachable_urlinAttachmentDatato handle non-edition attachables that respond toparent_attachable, soAssetManagerAttachmentMetadataJobnow correctly setsparent_document_urlat every metadata update, including for draft consultations and calls for evidence.Finally, I've added a backfill rake task (
backfill_parent_document_url_for_non_editionable_attachables) which enqueuesAssetManagerAttachmentMetadataJobfor all existing attachment data records belonging toConsultationResponse,CallForEvidenceResponse, andPolicyGroup, so that historically missingparent_document_urlvalues are corrected in asset manager.Issue was found through a support ticket: https://gov-uk.atlassian.net/browse/WHIT-3466
This application is owned by the Whitehall Experience team. Please let us know in #govuk-whitehall-experience-tech when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.