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

[#1005] Hide individual attachments #7391

Merged
merged 7 commits into from
Nov 1, 2022
Merged

Conversation

gbp
Copy link
Member

@gbp gbp commented Oct 24, 2022

Relevant issue(s)

Supersedes #7327
Fixes #1005

What does this do?

Allows admins to edit attachment prominence and reason.

Add prominence checks for attachments when:

  • viewing requests/messages
  • viewing attachments
  • ZIP downloads
  • Xapian indexing

Why was this needed?

Easier admin of responses and attachments.

Implementation notes

Screenshots

Notes to reviewer

@gbp
Copy link
Member Author

gbp commented Oct 24, 2022

Need to address points made in previous review #7327 (review)

@gbp gbp force-pushed the 1005-hide-individual-attachments-2 branch from caafffe to 5fa6885 Compare October 26, 2022 11:41
@gbp
Copy link
Member Author

gbp commented Oct 26, 2022

From #7327 (review)

This doesn't cover the case where the FoiAttachment is "normal", but it's IncomingMessage is reduced prominence. In this situation you can still view the attachment if you know its direct URL (which search engines will). I think it needs to be like:

can_view_with_prominence?(attachment.prominence, attachment.incoming_message.info_request) &&
  can?(:read, attachment.incoming_message)

This is pretty critical behaviour, so I think it needs tests, somewhere….

This has now been fixed and separated out into its own commit in: 3f6a3f1

@gbp gbp force-pushed the 1005-hide-individual-attachments-2 branch 3 times, most recently from e778ace to 23baf65 Compare October 26, 2022 16:50
@gbp
Copy link
Member Author

gbp commented Oct 26, 2022

From #7327 (review)

As an admin user I though I should be able to see reduced-prominence main body parts below the prominence reason?

Fixed in 6c302bf

@gbp gbp marked this pull request as ready for review October 26, 2022 16:52
@gbp gbp requested a review from garethrees October 26, 2022 16:52
Copy link
Member

@garethrees garethrees left a comment

Choose a reason for hiding this comment

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

Looks great! So much work to get here, but everything much nicer as a result 🥷

I haven't been able to test every permutation of every visibility options per action, but given the much clearer Ability checks I'm pretty confident.

app/views/request/_attachments.html.erb Outdated Show resolved Hide resolved
app/views/request/_attachments.html.erb Outdated Show resolved Hide resolved
app/models/incoming_message.rb Outdated Show resolved Hide resolved
gbp added 6 commits November 1, 2022 15:32
Allowing us to check if an attachment can be `read` by checking its
prominence and if the incoming message and info request are also
readable.
We need to check attachment prominence when rendering correspondence on
the `request#show` action and views.
We need to check attachment prominence when serving them directly or
when converted to HTML.
We need to check attachment prominence when adding them to request ZIP
downloads or the text version of the correspondence.
We need to check attachment prominence while updating the Xapian search
index for events.

To achieve the `IncomingMessae#get_body_for_quoting` method needed to be
split, as it was being used for both indexing and displaying the body as
text.
Update an attachment prominence from the admin and record an
`edit_attachment` event on the relevant request.
@gbp gbp force-pushed the 1005-hide-individual-attachments-2 branch from af23784 to 5939b59 Compare November 1, 2022 15:41
This tests 27 different permutations of different prominences values for
requests, messages and attachments objects.

Proving that viewing requests and attachments is correctly limited and
the correct prominence reason shown.
@gbp gbp force-pushed the 1005-hide-individual-attachments-2 branch from 5939b59 to 0eaac67 Compare November 1, 2022 15:48
@gbp gbp merged commit 64a4de7 into develop Nov 1, 2022
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.

Allow individual attachments to incoming messages to be hidden
2 participants