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.

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

Ctskf 831 cccd refactor attachment links actual ticket #7174

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Katy600
Copy link
Contributor

@Katy600 Katy600 commented Jul 17, 2024

What

This ticket is very much WIP.

I started by removing the download actions for each controller as Active Storage has it's own controller that provides this functionality.

I then removed any reference to download from the routes config as it will no longer generate a url from here but will be instead generated through the Active Storage resources routes. I also removed download from any before action in the controllers as this action now no longer exists.

I then replaced the previous rails magic routes method in the view partials with the new Active Storage urls. Iit all seemed to work when I ran my local server!! Which made me very happy :).

However - I have been struggling a lot with the tests - they are obviously failing in a number of places!

I decided to remove some failing tests in the Message spec and give them their own active_storage spec as download was no longer part of 'Message'.

This test...

      subject(:get_attachment) { get rails_blob_path(message.attachment, disposition: 'attachment') }

      let(:expected_url) { rails_blob_path(message.attachment, disposition: 'attachment') }

      it 'redirects to the ActiveStorage blob URL' do
        get_attachment
        expect(response).to redirect_to(expected_url)
      end
    end

Was not working for me - and saying that it was getting a
Expected response to be a <3XX: redirect>, but was a <404: Not Found>

I did some googling and did this in the routes file

  #   match '*path', to: 'errors#not_found', via: :all
  # end

  if Rails.env.development?
    scope format: true, constraints: { format: /jpg|png|gif|PNG/ } do
      get '/*anything', to: proc { [404, {}, ['']] }, constraints: lambda { |request| !request.path_parameters[:anything].start_with?('rails/') }
    end
  end

I didn't really understand it at all but it changed my test result to

Expected response to be a redirect to <http://www.example.com/rails/active_storage/blobs/redirect/lots of random letters/attachment.doc?disposition=attachment> 

but was a redirect to ...

<http://www.example.com/rails/active_storage/disk/lots of random letters/attachment.doc>.

I tried to understand what disk was!
It all continued to go a little wrong from here!!

Ticket

CCCD - Refactor attachment links

Why

Active Storage provides its own controller for this purpose and a link can be created with rails_blobs_path or rails_blob_url.

We should move to using Active Storage rather than the controller actions.

How

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant