Skip to content

Conversation

ddouglascarr
Copy link

@ddouglascarr ddouglascarr commented Jul 21, 2020

The PR adds a requires_watermark boolean field to the attachments model. When an attachment is uploaded with this set to True, the attachment is uploaded to a different location in the private_attachments/ namespace.

It is up to the consuming application to make sure this location is private.

This enables watermarking by providing:

  • a means to ensure that the source document for watermarking is in a private location
  • a boolean that, when checked, indicates to the consumer the file needs to be accessed differently

The file will not be accessible once uploaded. (If you click on the link, you should be denied access). It is up to the consuming application to provide a way to watermark the source file, and provide a means to download it.

@dannyshaw
Copy link

I do wonder if this could just be called is_private making it a more generic solution for non public attachments.

Another field would be necessary to indicate requiring watermarking, or a combination of is_private and a pdf extension could assume watermarking.

#nonblocking

@ddouglascarr
Copy link
Author

@dannyshaw

I considered this. You'd need to enforce the is_private when requires_watermark is True. At which point you're basically at using requires_watermark. We can extend it in the future if needed if other private attachments are required.

"""
Uploads a sample file for the given user.
"""
opts = {} if opts == None else opts
Copy link

@MattFisher MattFisher Jul 23, 2020

Choose a reason for hiding this comment

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

Comparisons with None need to use is. Edit: this is a PEP8 guideline not a requirement so #non-blocking

Suggested change
opts = {} if opts == None else opts
opts = opts or {}

@MattFisher
Copy link

Flagging that the migration will lock+rewrite the attachments table, which probably isn't a big deal.

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.

3 participants