Skip to content

Commit

Permalink
Update "show as HTML" attachment controller action
Browse files Browse the repository at this point in the history
This updates `AttachmentController#show_as_html` to do attachment
masking in the background.

Our "view attachments in the browser" feature, or "view as HTML", is
doing a lot of processing inline in the web processes and this sometimes
causes memory issues on the server and results in oom-kill running.

Uses the same logic as the `#show` action to check an attachment is
masked before proceeding with the action. This means it will time outs
after 5 seconds and then redirect the `AttachmentMaskController` to wait
for the job to complete before rendering a view saying the attachment is
now viewable.

Fixes #7977
  • Loading branch information
gbp committed Oct 25, 2023
1 parent 3f70a99 commit 1113772
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 14 deletions.
2 changes: 1 addition & 1 deletion app/controllers/attachments_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ class AttachmentsController < ApplicationController
before_action :authenticate_attachment
before_action :authenticate_attachment_as_html, only: :show_as_html

around_action :ensure_masked, only: :show
around_action :ensure_masked
around_action :cache_attachments, only: :show_as_html

def show
Expand Down
11 changes: 11 additions & 0 deletions app/views/attachment_masks/_download.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<% @title = _('Attachment available for download.') %>
<h1><%= @title %></h1>

<p>
<%= _('The attachment has now been processed and is available for ' \
'download.') %>
</p>

<p>
<%= link_to _('Download attachment'), @referer, class: 'button' %>
</p>
11 changes: 11 additions & 0 deletions app/views/attachment_masks/_viewing.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<% @title = _('Attachment available for viewing.') %>
<h1><%= @title %></h1>

<p>
<%= _('The attachment has now been processed and is available for ' \
'viewing.') %>
</p>

<p>
<%= link_to _('View attachment'), @referer, class: 'button' %>
</p>
16 changes: 5 additions & 11 deletions app/views/attachment_masks/done.html.erb
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
<% @title = _('Attachment available for download.') %>
<h1><%= @title %></h1>

<p>
<%= _('The attachment has now been processed and is available for ' \
'download.') %>
</p>

<p>
<%= link_to _('Download attachment'), @referer, class: 'button' %>
</p>
<% if @referer =~ %r(/request/\d+/response/\d+/attach/html/) %>
<%= render partial: 'viewing' %>
<% else %>
<%= render partial: 'download' %>
<% end %>
63 changes: 63 additions & 0 deletions spec/controllers/attachments_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,69 @@ def show_as_html(params = {})
.to raise_error(ActiveRecord::RecordNotFound)
end

context 'when attachment has not been masked' do
let(:attachment) do
FactoryBot.create(
:body_text, :unmasked,
incoming_message: message,
prominence: attachment_prominence
)
end

context 'when masked attachment is available before timing out' do
before do
allow(IncomingMessage).to receive(
:get_attachment_by_url_part_number_and_filename!
).and_return(attachment)
allow(attachment).to receive(:masked?).and_return(false, true)
end

it 'queues FoiAttachmentMaskJob' do
expect(FoiAttachmentMaskJob).to receive(:perform_later).
with(attachment)
show_as_html
end

it 'redirects to show action' do
show_as_html
expect(response).to redirect_to(request.fullpath)
end
end

context 'when response times out waiting for masked attachment' do
before do
allow(Timeout).to receive(:timeout).and_raise(Timeout::Error)
end

it 'queues FoiAttachmentMaskJob' do
expect(FoiAttachmentMaskJob).to receive(:perform_later).
with(attachment)
show_as_html
end

it 'redirects to wait for attachment mask route' do
allow_any_instance_of(FoiAttachment).to receive(:to_signed_global_id).
and_return('ABC')

verifier = double('ActiveSupport::MessageVerifier')
allow(controller).to receive(:verifier).and_return(verifier)
allow(verifier).to receive(:generate).with(
get_attachment_as_html_path(
incoming_message_id: attachment.incoming_message_id,
id: info_request.id,
part: attachment.url_part_number,
file_name: attachment.filename
)
).and_return('DEF')

show_as_html
expect(response).to redirect_to(
wait_for_attachment_mask_path('ABC', referer: 'DEF')
)
end
end
end

context 'when request is embargoed' do
let(:info_request) { FactoryBot.create(:embargoed_request) }

Expand Down
14 changes: 12 additions & 2 deletions spec/integration/incoming_mail_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,25 +64,35 @@
it "generates a valid HTML verson of plain text attachments" do
receive_incoming_mail('incoming-request-two-same-name.email',
email_to: info_request.incoming_email)
visit get_attachment_as_html_path(
attachment_path = get_attachment_as_html_path(
incoming_message_id: info_request.incoming_messages.first.id,
id: info_request.id,
part: 2,
file_name: 'hello world.txt.html',
skip_cache: 1)

visit attachment_path
perform_enqueued_jobs

visit attachment_path
expect(page.response_headers['Content-Type']).to eq("text/html; charset=utf-8")
expect(page).to have_content "Second hello"
end

it "generates a valid HTML verson of PDF attachments" do
receive_incoming_mail('incoming-request-pdf-attachment.email',
email_to: info_request.incoming_email)
visit get_attachment_as_html_path(
attachment_path = get_attachment_as_html_path(
incoming_message_id: info_request.incoming_messages.first.id,
id: info_request.id,
part: 2,
file_name: 'fs 50379341.pdf.html',
skip_cache: 1)

visit attachment_path
perform_enqueued_jobs

visit attachment_path
expect(page.response_headers['Content-Type']).to eq("text/html; charset=utf-8")
expect(page).to have_content "Walberswick Parish Council"
end
Expand Down

0 comments on commit 1113772

Please sign in to comment.