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

feat: allow generating legal hold bundles in filestore directly #70

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

fmartingr
Copy link
Contributor

@fmartingr fmartingr commented Jun 18, 2024

Summary

This pull request adds the feature of allowing users to create the legal hold download bundles directly in the configured filestore rather than downloading them directly in the browser.

  • Adds new logic to allow "locking" a legal hold with a custom name. Using bundle to prevent several uploads at the same time.
  • Adds new configuration option GenerateDownloadOnFilestore(bool) to setup this workflow.
  • Created logic to ensure that the file is uploladed in chunks directly to the filestore, by leveraging a bufio.BufferedWritter and a custom io.Writter implementation to use the model.Filestore underneath.
  • A job to cleanup the files. Bundles are created with a timestamp in the filename and a job running every hour will check if there's any file older than 24h and remove it automatically. A custom job on the plugin is preferred to ensure compatibility with all filestores.
  • Added a bot user to notify the requesting user of updates on the bundle creation.
  • Use a different download API to generate a bundle than to download the file

Notes

I have been experimenting with this feature for several days and before I had a setting to dis/allow this action. But when I had it ready it made no sense, since the page to download the legal holds is the same as the settings, and I had permissions to enable/disable it on the spot:

Screen.Recording.2024-06-21.at.11.59.49.mp4

(You can see the upload button appearing and disappearing)

#44

@fmartingr fmartingr self-assigned this Jun 18, 2024
@fmartingr fmartingr changed the title Feat/download to filestore feat: allow generating legal hold bundles in filestore directly Jun 18, 2024
@fmartingr fmartingr marked this pull request as ready for review June 25, 2024 06:38
@fmartingr fmartingr added the Do Not Merge Should not be merged until this label is removed label Jul 4, 2024
server/api.go Outdated Show resolved Hide resolved
server/api.go Outdated Show resolved Hide resolved
for _, entry := range files {
header := &zip.FileHeader{
Name: entry,
Method: zip.Deflate, // deflate also works, but at a cost
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment makes it seem like we're not using Deflate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this verbatim from the original code, it's also confusing to me tbh.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like a comment that outlasted a code change that it shouldn't have. I'd remove it.

server/api.go Outdated Show resolved Hide resolved
server/store/filebackend/writter.go Outdated Show resolved Hide resolved
server/store/filebackend/writter.go Outdated Show resolved Hide resolved
Comment on lines +68 to +72
<OverlayTrigger

// @ts-ignore
delayShow={300}
placement='top'
Copy link
Contributor

Choose a reason for hiding this comment

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

What typescript error is occurring here?

Copy link
Contributor Author

@fmartingr fmartingr Jul 11, 2024

Choose a reason for hiding this comment

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

Apparently this attribute does not exist. This comes from the rest of the code as well, maybe it's safe to remove?

Type '{ children: Element; delayShow: number; placement: "top"; overlay: Element; }' is not assignable to type 'IntrinsicAttributes & OverlayTriggerProps & { disabled?: boolean | undefined; className?: string | undefined; } & RefAttributes<any>'.
  Property 'delayShow' does not exist on type 'IntrinsicAttributes & OverlayTriggerProps & { disabled?: boolean | undefined; className?: string | undefined; } & RefAttributes<any>'.ts(2322)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If I remember correctly, this ts error is somehow spurious - the props do exist and influence UI behaviour even though ts says they don't. It was a while ago though, so may be misremembering.

Copy link
Collaborator

@grundleborg grundleborg left a comment

Choose a reason for hiding this comment

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

LGTM

for _, entry := range files {
header := &zip.FileHeader{
Name: entry,
Method: zip.Deflate, // deflate also works, but at a cost
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like a comment that outlasted a code change that it shouldn't have. I'd remove it.

"github.com/mattermost/mattermost-server/v6/shared/filestore"
)

// fileBackendWritter is a simple io.Writer that writes to a file using a filestore.FileBackend
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this is spelled Writter rather than Writer?

Comment on lines +68 to +72
<OverlayTrigger

// @ts-ignore
delayShow={300}
placement='top'
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I remember correctly, this ts error is somehow spurious - the props do exist and influence UI behaviour even though ts says they don't. It was a while ago though, so may be misremembering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge Should not be merged until this label is removed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants