Add asset_manager_id to file attachments

[richardTowers]

This needs to follow on from alphagov/publishing-api#2994

As described there, the lack of asset_manager_id in the data for file asset attachments in content store makes it hard to request the attachments on the server side. Instead, we have to redirect users to the assets, which has led to ugly workarounds like CSV previews being rendered by frontend but served on assets.publishing.service.gov.uk.

Adding asset_manager_id should allow the frontends to request attachments directly, using the API client:

GdsApi.asset_manager.media(asset_manager_id, filename)

This should make it much more convenient to preview assets, or use them for other rendering purposes (e.g. showing a CSV as a line graph).

EDIT: This description has been updated by @unoduetre to not include details which are no longer relevant.

⚠️ This repo is Continuously Deployed: make sure you follow the guidance ⚠️

[catalinailie]

I'm not sure attachment_data.id is the ID we want. GdsApi.asset_manager expects the asset's ID, and that one, in whitehall, is Asset.asset_manager_id.

I think this is the reason why we have this scenario with preview_url formed in whitehall using attachment_data.id and then overwritten in government-frontend with the URL that has this asset_manager_id in it (formed here I think, but I might be wrong).

[unoduetre]

Yes, you're right. It should be asset_manager_id of the relevant asset (an object of the Asset class). The issue is that there can be more than one asset per attachment (an object of the Attachment class). (The path to assets is attachment.attachment_data.assets) But the solution is to get the asset with the variant "original", which means the original file as uploaded. The combination of assetable_id, assetable_type and variant is unique (there is a validation for that). assetable_type in this case is "AttachmentData", assetable_id is the id of the object of the AttachmentData class (the id from the database). And the variant should be specified as "original". In this way we can get the unique asset_manager_id.

There is a method called get_asset in AssetManagerStorage but it's currently private. But we can make it public. asset_manager_id can be retrieved this way: attachment.attachment_data.file.file.send(:get_asset).asset_manager_id
Of course in the final solution we should create a couple more methods, so there are no deep dependencies present in the final code.

[catalinailie]

The changes look good, I only have one small suggestion.
And really nice that you covered the changes with tests ⭐

[minhngocd]

AttachmentData has assets (usually only one now, for file attachments), which also store asset_manager_id. Can we not make use of that instead of taking it from file which I understand is more the actual file uploader from CarrierWave?
Are we looking to attach this ID to the content item when publishing it through to content store? if so, I think AttachmentData > Asset > Asset Manager ID might be more straight forward and actually persisted in the database

[catalinailie]

@minhngocd Ohh interesting, it make sense. I don't know why we haven't thought of that.
Yes, we are looking to attach this ID to the content item when publishing.

[minhngocd]

Another thought i was also having is: what's the need for the asset manager ID? If you're getting it from asset manager via the GDS API, would that not be equivalent to using the attachment URL that comes through with the content item?

e.g. https://www.integration.publishing.service.gov.uk/api/content/government/publications/pension-credit-toolkit
File attachments currently come through with the content item as

{
        "accessible": false,
        "alternative_format_contact_email": "[email protected]",
        "attachment_type": "file",
        "command_paper_number": "",
        "content_type": "application/zip",
        "file_size": 465016,
        "filename": "pension-credit-customers-english.zip",
        "hoc_paper_number": "",
        "id": "8524176",
        "isbn": "",
        "locale": "en",
        "title": "Pension Credit social media images directed at potential customers",
        "unique_reference": "",
        "unnumbered_command_paper": false,
        "unnumbered_hoc_paper": false,
        "url": "https://assets.publishing.service.gov.uk/media/6756cb787ec19a3516f79a0e/pension-credit-customers-english.zip"
      },

The url is a direct link to the asset on asset manager. can that not be used to render previews? Sorry if I'm not getting something - happy to jump on a call and discuss

[catalinailie]

The url is a direct link to the asset on asset manager. can that not be used to render previews?

This is the current behaviour. Government-frontend uses this asset URL for creating the /media/id/filename/preview url.
But for our change, we need to extract the asset_manager_id and filename to create the new preview /csv-preview/id/filename url that will be served from gov.uk domain. And parsing an URL to get an ID isn't the best approach. It will be nicer if that logic moves away from government-frontend and we just use the asset_manager_id from content item.

[minhngocd]

ahhh ok thanks for explaining! yes, in which case i think exposing it from AttachmentData > Assets through to the publishing_api_details on Attachment would be the most straightforward

[minhngocd]

Suggestion, when we expose it, maybe it can be an array of hash object?
like

assets: [{
  filename: 
  asset_manager_id:
}]

Because the relationship between AttachmentData and Assets is one to many due to the multiple versions of image uploads. If we do an array of objects, we can return everything rather than arbitrarily selecting the first asset to send.

[catalinailie]

We will be sending this asset_manager_id only for csv file attachments. I might be wrong, but I guess the AttachmentData will have only one Asset linked to it, right? So, in this case, picking the first Asset is actually the only one.

[catalinailie]

Ups, I requested a review from the wrong user

[minhngocd]

Personally, I feel like implementing this specifically for the CSV scenario and only returning the first asset manager id feels too narrow to one use-case. This is then also not explicit in the naming as well since the variable being exposed is asset_manager_id I would expect it to return all asset manager IDs associated with the file, not just the first CSV ones.

can we not return something like

assets: [{
  asset_manager_id: abc
  filename: abc_1.csv
}]

And then it's up to the consumer of the content item to decide to use the first one if it's a CSV based off the content_type of the attachment?

[minhngocd]

Otherwise it feels like we're building consumer logic into the provider

[catalinailie]

Yeah, I agree the name is not very explicit for what that value is used.
The initial idea was to send this asset_manager_id (or a better name) only for csv attachments. In this way, government-frontend knows when to add the preview URL to the page.
It would be nice not to have any logic in govenrment-frontend for checking if the file is a csv or not and to have this logic in the publishing side since this check is already there.

[catalinailie]

Also, will it be useful for other types of assets to have this information sent? I'm mindful there are quite a lot of assets and sending this information for all of them is not very useful since we need it only for csv files.

[minhngocd]

Discussed offline with @catalinailie , As an FYI update for folks reading the PR. We decided we would require returning all assets array as the file is not necessarily always the first one.

Asset.where(assetable_type: "AttachmentData").pluck(:assetable_id).group_by{|e|e}.select{|k,v|v.size>1}.count
=> 782044

There are 780k+ attachment data records where there are multiple variants of asset on an AttachmentData (still roughly 400k, discounting the duplicates)

example of a live published edition:

[#<Asset:0x0000ffff710ae9e0
  id: 3812782,
  asset_manager_id: "67598fff9f669f2e28ce2b01",
  assetable_type: "AttachmentData",
  assetable_id: 1286037,
  filename: "thumbnail_redacted..">,
 #<Asset:0x0000ffff710ae8a0
  id: 3812783,
  asset_manager_id: "67598fff3bb681ccb0d346b7",
  assetable_type: "AttachmentData",
  assetable_id: 1286037,
  filename: "redacted...">]

Which belongs to a published edition, still live - and the first entry seems to be the thumbnail

[minhngocd]

Looks good! Thanks @catalinailie !
I guess the failing tests are related to publishing API changes to the schema going through.

[nacnudus]

Just to note that this will be useful to data analysts, who have been asked by other departments to provide lists of attachments, when they were last updated, and what page they are attached to. Getting this information will require a join between the Publishing API and the Asset Manager databases, for all types of attachment (pdf, csv, etc.).