Allow asset to be replaced with another asset #499
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chrislo
approved these changes
Feb 27, 2018
| @@ -135,6 +135,24 @@ | |||
| end | |||
| end | |||
|
|
|||
| context 'and replacement_id is does not match an existing asset' do | |||
When a Whitehall attachment is "replaced", requests for its URL result in a redirect to the attachment which replaced it. We're going to need to implement this functionality in Asset Manager in order to serve Whitehall attachments without changing the externally visible behaviour. This is a first step in that direction. Note that as of Rails v5, a belongs_to association is required by default. Hence why I've had to specify the `optional: true` option. I didn't see any point in implementing the inverse relation at this point, because it's not obvious we're going to need it. Note that some of the model examples I've added to the Asset spec may not be needed, but I wanted to make sure I understood how this association was persisted by Mongoid.
This implements this bit of behaviour [1] in Whitehall's AttachmentsController#fail method. Note that I plan to address the call to `expires_headers` in a subsequent commit. Note also that I've implemented this for both Whitehall and Mainstream assets for completeness, even though the immediate requirement is only to do this for Whitehall assets. [1]: https://github.com/alphagov/whitehall/blob/d32f0b1a3fa331c32dd92bd61174900026a0e41d/app/controllers/attachments_controller.rb#L45-L47
The header now set in WhitehallMediaController#download is intended to match that set in Whitehall's AttachmentsController#fail method [1] for anonymous users. I haven't addressed the case where the user is signed in, because this is a more general problem which I've captured in this GitHub issue [2]. I've done something similar in MediaController#download, but using the default expiry for Mainstream assets for consistency. Hopefully we can bring these two values into line in the not too distant future. [1]: https://github.com/alphagov/whitehall/blob/446e60b80fee198540e5abe3a7077a5a7f5f63e5/app/controllers/attachments_controller.rb#L46 [2]: #494
If asset has a replacement then include its ID in the JSON response. The convention seems to be to include such attributes in the response by default; the only exception being if there is some kind of security consideration. In this case it seems fine to include the new attribute.
This commit makes it possible to specify a replacement_id when creating or updating an asset. Although the most likely use case is when *updating* an existing asset, I've also allowed it when creating a new asset for completeness. Although the immediate need for this if only for Whitehall assets, I've also allowed it for Mainstream assets, because it seems generally useful. Note that I suspect we could probably replace the MediaController#redirect_to_current_filename behaviour with this more general mechanism at some point.
If Asset#replacement_id is specified, it should match an existing asset, otherwise we should fail fast with a 422 Unprocessable Entity response.
|
I've fixed the typo spotted by @chrislo in the last commit and I'm rebasing against |
floehopper
force-pushed
the
allow-asset-to-be-replaced-with-another-asset
branch
from
5e8d60e
to
4f00fd6
Compare
floehopper
added a commit
to alphagov/whitehall
that referenced
this pull request
Mar 1, 2018
We want to update Asset Manager whenever an attachment is replaced in Whitehall so that Asset Manager can redirect requests for the corresponding asset to the replacement asset. This is a first step in that direction, although none of it is wired up yet. These changes require the functionality in this Asset Manager PR [1]. Unfortunately, since we're not yet storing the Asset Manager asset ID in Whitehall, we're forced to lookup both the asset being replaced and the asset which is replacing it. This does make me wonder whether we're making lives harder by not storing the asset ID. [1]: alphagov/asset-manager#499
floehopper
added a commit
to alphagov/whitehall
that referenced
this pull request
Mar 1, 2018
We want to update Asset Manager whenever an attachment is replaced in Whitehall so that Asset Manager can redirect requests for the corresponding asset to the replacement asset. This is a first step in that direction, although none of it is wired up yet. These changes require the functionality in this Asset Manager PR [1]. Unfortunately, since we're not yet storing the Asset Manager asset ID in Whitehall, we're forced to lookup both the asset being replaced and the asset which is replacing it. This does make me wonder whether we're making lives harder by not storing the asset ID. [1]: alphagov/asset-manager#499
floehopper
added a commit
to alphagov/whitehall
that referenced
this pull request
Mar 2, 2018
We want to update Asset Manager whenever an attachment is replaced in Whitehall so that Asset Manager can redirect requests for the corresponding asset to the replacement asset. This is a first step in that direction, although none of it is wired up yet. These changes require the functionality in this Asset Manager PR [1]. Unfortunately, since we're not yet storing the Asset Manager asset ID in Whitehall, we're forced to lookup both the asset being replaced and the asset which is replacing it. This does make me wonder whether we're making lives harder by not storing the asset ID. [1]: alphagov/asset-manager#499
ChrisBAshton
added a commit
that referenced
this pull request
Nov 12, 2019
Had hoped I could use #499 But my attempts to set a `replacement_id` on an asset on integration did not cause it to redirect.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a Whitehall attachment is "replaced", requests for its URL result in a redirect to the attachment which replaced it. This PR implements this functionality in Asset Manager so that we'll be able to serve Whitehall attachments from Asset Manager without changing the externally visible behaviour.
This PR introduces the
Asset#replacementassociation. If an asset has such a replacement then requests for it are redirected to the path for the replacement. This is equivalent to the behaviour implemented in Whitehall'sAttachmentsController#failmethod.This PR also allows a
replacement_idto be specified when creating or updating an asset via the API. I plan to call this when an attachment is replaced in Whitehall.Notes:
I have not addressed the issue of setting a different value for the
Cache-Controlresponse header when the user is not signed in, because I think this is a wider concern which has been captured in this GitHub issue.I have not implemented the optimisation which avoids multiple redirects where a replacement asset is itself replaced. For the moment, we can retain this behaviour by leaving it in Whitehall, but it would probably make sense to move it into Asset Manager at some point so we don't need to implement it in other publisher apps. See this GitHub issue for more details.
This work is based on the work in Improvements to cache control #497. The first commit is a squashed version of that PR and should be removed/rebased-out before merging this PR.