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

Extract FoiAttachment::Dsn & restore functionality #8015

Merged
merged 2 commits into from
Jan 3, 2024

Conversation

garethrees
Copy link
Member

@garethrees garethrees commented Nov 16, 2023

What does this do?

Extract FoiAttachment::Dsn & restore functionality

Why was this needed?

The list of DSN codes was constantly getting in the way when viewing foi_attachment.rb. Also, it was doing nothing, so might as well fix it.

Screenshots

BEFORE

Screenshot 2023-11-24 at 15 58 52

AFTER

Screenshot 2023-11-24 at 15 58 21

Notes to reviewer

@garethrees garethrees force-pushed the extract-dsn-to-message branch from 8c314f0 to baf1cd6 Compare November 16, 2023 20:43
@garethrees garethrees force-pushed the extract-dsn-to-message branch 2 times, most recently from 0c592f3 to 26b204c Compare November 24, 2023 16:03
@garethrees garethrees marked this pull request as ready for review November 24, 2023 16:05
@garethrees garethrees force-pushed the extract-dsn-to-message branch from 26b204c to fb23623 Compare November 24, 2023 16:07
app/models/foi_attachment/dsn.rb Outdated Show resolved Hide resolved
app/models/foi_attachment.rb Show resolved Hide resolved
app/models/foi_attachment.rb Outdated Show resolved Hide resolved
app/views/request/_attachments.html.erb Outdated Show resolved Hide resolved
app/models/foi_attachment/dsn.rb Outdated Show resolved Hide resolved
Copy link
Member

@gbp gbp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, much better than all being in FoiAttachment.

Tested by sending a mail to the holding pen with:

mail = Mail.new do
  body 'Hello World'
  add_file filename: 'DSN', content: File.read('spec/fixtures/files/attachment.delivery_status'), content_type: 'message/delivery-status'
end
RequestMailer.receive mail.to_s

I guess we could move the em tag into the view which is probably the correct thing to do but happy to leave it as is.

Merge once you're happy.

diff --git a/app/models/foi_attachment.rb b/app/models/foi_attachment.rb
index d1381eb5a..596c37c22 100644
--- a/app/models/foi_attachment.rb
+++ b/app/models/foi_attachment.rb
@@ -164,7 +164,7 @@ def extra_note
     dsn = DeliveryStatusNotification.new(body)
     return unless dsn.status && dsn.message
 
-    "<em>DSN: #{dsn.status} #{dsn.message}</em>".html_safe
+    "DSN: #{dsn.status} #{dsn.message}"
   end
 
   # Called by controller so old filenames still work
diff --git a/app/views/request/_attachments.html.erb b/app/views/request/_attachments.html.erb
index 8022ec812..83dbc48aa 100644
--- a/app/views/request/_attachments.html.erb
+++ b/app/views/request/_attachments.html.erb
@@ -32,7 +32,7 @@
               <% if a.has_body_as_html? && incoming_message.info_request.prominence(:decorate => true).is_public? %>
                 <%= link_to "View as HTML", attachment_path(a, :html => true) %>
               <% end %>
-              <%= a.extra_note %>
+              <% if note = a.extra_note %><em><%= note %></em><% end %>
             </p>
           <% end %>
         <% end %>

Copy link
Member

@gbp gbp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. dns.rb is still present

ls app/models/foi_attachment
delivery_status_notification.rb dsn.rb prominence.rb

@gbp gbp merged commit caf8676 into develop Jan 3, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants