[#1005] Hide individual attachments #7327
Conversation
gbp
force-pushed
the
1005-prep-for-attachment-prominence
branch
from
363c9e1 to
0b630ce
Compare
gbp
force-pushed
the
1005-hide-individual-attachments
branch
from
782edd7 to
9fad5b6
Compare
There was a problem hiding this comment.
This is looking great!!
Some minor comments inline, and a few more here.
Reduced prominence content is included in zip downloads and RSS feed (will also need to check a batch zip download):
Minor, but in some cases the attachment logo overflows its container:
I don't quite understand what's going on with b0bcb87, so maybe one worth talking over when we next catch up.
| <table class="table table-striped table-condensed"> | ||
| <tbody> | ||
| <% @foi_attachment.for_admin_column( | ||
| :filename, :content_type, :charset, :display_size |
There was a problem hiding this comment.
Any reason not to render out the full admin column set?
| redirect_to edit_admin_incoming_message_path(@incoming_message) | ||
|
|
||
| else | ||
| render action: 'edit' |
There was a problem hiding this comment.
Should this come with some sort of indication that the update was unsuccessful?
There was a problem hiding this comment.
We don't do this in the other admin controllers.
There was a problem hiding this comment.
I don't think we do when we show model validation errors, but doesn't look like we're doing that here so a fail would be completely silent. I think we only need a error: 'Save unsuccessful' flash on the end of the render call.
gbp
force-pushed
the
1005-prep-for-attachment-prominence
branch
from
0b630ce to
9eb2ee5
Compare
Move specs into shared example block so these can be included within other model's specs.
Take into account hidden or requester only prominence.
Requires a `message` local variable in order to be rendered.
This reverts commit 02c7b9d.
Update the attachment caching to take place after the authentication. In changing this a regression for returning a 'Directory listing not allowed' error is introduced. In order to fix the routes have been tightened up so we can simply check for the present of `file_name` parameter. Fixes #6477
We need to check attachment prominence when: 1. serving them directly or when converted to HTML 2. generating the Xapian index.
This is shared across all classes which include `MessageProminence` so can be extracted.
gbp
force-pushed
the
1005-hide-individual-attachments
branch
from
857545f to
1e1e401
Compare
gbp
changed the base branch from
1005-prep-for-attachment-prominence
to
develop
Rename variable `message` to `prominenceable` as this will allow us to use this partial for attachments as well as messages.
|
@garethrees this should be working now. There is loads of fixups do you want to see those or should I go ahead and rebase before you re-review? |
|
Specs are green locally not sure why GitHub actions aren't running. |
Happy to review as-is.
Might be because of the merge conflict? |
|
On running I get:
Fixed with: diff --git a/app/helpers/prominence_helper.rb b/app/helpers/prominence_helper.rb
index 2a26944b4..c81994e99 100644
--- a/app/helpers/prominence_helper.rb
+++ b/app/helpers/prominence_helper.rb
@@ -98,7 +98,7 @@ def default_prominence_reason
end
end
- class InfoRequest::Prominence::Helper < Base # :nodoc:
+ class InfoRequest::Prominence::Helper < ProminenceHelper::Base # :nodoc:
def user
prominenceable.user
end
@@ -166,6 +166,8 @@ def sign_in_notice(*args)
::OutgoingMessage::Prominence::Helper = IncomingMessage::Prominence::Helper
+ module FoiAttachment::Prominence ; end
+
class FoiAttachment::Prominence::Helper < Base # :nodoc:
def user
prominenceable.incoming_message.info_request.user |
Missed a file, pushed a fixup |
There was a problem hiding this comment.
As an admin user I though I should be able to see reduced-prominence main body parts below the prominence reason?
I think this is getting quite a big and complicated PR, so maybe let's pair on this on Monday to get some of the more incidental parts merged, and then get left with just the main body of work that adds attachment prominence.
|
Superseded by #7391 |
Relevant issue(s)
Requires #7324
Fixes #1005
What does this do?
FoiAttachmentWhy was this needed?
Easier admin of responses and attachments.
Screenshots
Incoming message admin
Attachment admin
Hidden attachment as public / Hidden attachment as requester
Hidden attachment as site admin
Requester only attachment as public
Requester only attachment as requester
Requester only attachment as site admin
Hidden main body attachment as public / Hidden main body attachment as requester
Hidden main body attachment as site admin
Requester only main body attachment as public
Requester only main body attachment as requester
Requester only main body attachment as site admin