Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Redirect asset requests to asset host #3627

Merged
merged 4 commits into from Dec 20, 2017
Merged

Redirect asset requests to asset host #3627

merged 4 commits into from Dec 20, 2017

Commits on Dec 20, 2017

  1. Rename extension to format in public_upload route segment 

    This `public_upload` route was unnecessarily complicated.

Rails will treat anything after the final period in a wildcard route as
the format[1] by default.  By adding the `:extension` dynamic segment we
were essentially duplicating that default behaviour. This meant that if
we were to request a file containing more than one period, e.g.
/government/uploads/path/to/file.name.png then `params[:path]` would
contain 'path/to/file', `params[:extension]` would contain 'name' and
`params[:format]` would contain 'png'.

This caused me a problem when trying to redirect asset requests to the
canonical asset host as calling `url_for(params)` (using the params
above) would end up dropping the format of the file.

I attempted to rename the `extension` segment in the other
asset/attachment routes but struggled to rename it for the attachment
preview route: In that case, the format is actually HTML even though the
extension will (always?) be CSV.

[1]:
http://guides.rubyonrails.org/routing.html#route-globbing-and-wildcard-segments
    @chrisroos
    chrisroos committed Dec 20, 2017
    Configuration menu
    Copy the full SHA
    98bb15f View commit details
    Browse the repository at this point in the history

  2. Use shortcut to enforce format for public_upload route 

    As per the Route globbing and wildcard segments docs[1]:

> If you want to make the format segment mandatory, so it cannot be
omitted, you can supply  format: true like this:

We'd already made format mandatory by including the :format dynamic
segment in the route. Using the `format: true` option appears to be the
Rails way, though.

[1]:
http://guides.rubyonrails.org/routing.html#route-globbing-and-wildcard-segments
    @chrisroos
    chrisroos committed Dec 20, 2017
    Configuration menu
    Copy the full SHA
    9b34f61 View commit details
    Browse the repository at this point in the history

  3. Redirect asset requests to asset host 

    It's currently possible to request Whitehall assets (and attachments) at
/government/uploads via both www.gov.uk and the asset host (e.g.
assets.publishing.gov.uk). We want all asset requests to go via the
asset host so that we can make changes to the config in a single place.

I'm using `Plek#public_asset_host` as it returns "The public-facing
asset base URL"[1]. I've tested this on integration and confirmed that
it returns
'https://assets-origin.integration.publishing.service.gov.uk'.

I initially tried to do the redirect in the routes file but it was
proving to be quite hard so I've gone with a method in the
`PublicUploadsController` instead.

I had to update upload_access_test to simulate making requests against
the asset host. Otherwise all the tests failed because they were being
redirect to the asset host.

Note that attachments are still available on both www.gov.uk and the
asset host. I expect we'll update this when we start working on the
migration of attachments to Asset Manager.

[1]:
https://github.com/alphagov/plek/blob/4d63a574ab36e9e8833ff14471482d1cbb110e61/lib/plek.rb#L97
    @chrisroos
    chrisroos committed Dec 20, 2017
    Configuration menu
    Copy the full SHA
    0aa298a View commit details
    Browse the repository at this point in the history

  4. Remove unused assert_redirected_to_placeholder_page method 

    This was made redundant in 616744b (Sep
2013).

I noticed it when fixing some tests in this file in the previous commit.
    @chrisroos
    chrisroos committed Dec 20, 2017
    Configuration menu
    Copy the full SHA
    cd37d37 View commit details
    Browse the repository at this point in the history