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 running a legal hold manually #129

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

Conversation

fmartingr
Copy link
Contributor

@fmartingr fmartingr commented Jan 13, 2025

This pull request introduces new functionalities to manage and execute individual legal holds, along with corresponding tests and necessary refactoring. The most important changes include adding new API endpoints, implementing methods to run and reset legal holds, and updating the data model to include status tracking.

New API Endpoints:

  • Added routes to run and reset the status of a specific legal hold in server/api.go.

Methods for Legal Hold Management:

  • Implemented the runSingleLegalHold and resetLegalHoldStatus methods in server/api.go to handle the new API endpoints.
  • Added RunSingleLegalHold method in server/jobs/legal_hold_job.go to execute a specific legal hold.
  • Executing a Legal Hold will store the last execution date as the current timestamp if the one being used is set in the future. (Automated jobs vs manual jobs)

Data Model Updates:

  • Introduced LegalHoldStatus and added status tracking fields in server/model/legal_hold.go

Interface and Refactoring:

  • Defined LegalHoldJobInterface to standardize the interface for legal hold jobs, facilitating easier testing and future enhancements in server/jobs/legal_hold_job_interface.go.

Important changes

  • Removed the double loop when executing legal holds. This was used to retry a legal hold but also means that if a legal hold fails for any reason the rest of the holds will not be handled while one fails continuously. (src)

UI Changes

Screenshot 2025-01-13 at 15 56 58

New button to refresh the legal hold list manually

Screenshot 2025-01-13 at 15 57 49

Added a button to run a legal hold manually

Screenshot 2025-01-13 at 15 59 26

Show when a current legal hold is running

Screenshot 2025-01-13 at 15 57 42

Disable the run button if the legal hold is already running and the download button if there's no messages in the legal hold

Enabled Disabled
Screenshot 2025-01-13 at 16 02 13 Screenshot 2025-01-13 at 15 57 20

Closes #90
Fixes #89

feat: Add reset status endpoint with five-click trigger for legal hold

fix: Import Client in legal_hold_row.tsx to resolve ReferenceError
@wiggin77
Copy link
Member

/update-branch

@mattermost-build
Copy link

Error trying to update the PR.
Please do it manually.

@wiggin77
Copy link
Member

@fmartingr you'll have to update this PR with latest master.

@fmartingr
Copy link
Contributor Author

@fmartingr you'll have to update this PR with latest master.

Should be up to date.

mattermost-plugin-legal-hold on  feat/run-manually [!?] via 🐹 v1.23.4 via  v16.14.2 via 💎 v2.7.6 took 1m2s
❯ git fetch --all

mattermost-plugin-legal-hold on  feat/run-manually [!?] via 🐹 v1.23.4 via  v16.14.2 via 💎 v2.7.6 took 9s
❯ git merge origin/main
Already up to date.

@fmartingr
Copy link
Contributor Author

Requested re-review while I figure out what's wrong with playwright.

Copy link

@davidkrauser davidkrauser left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for making changes 👍

@fmartingr
Copy link
Contributor Author

@wiggin77 Fixed the problem with the e2e tests 2daf648
From changes in Mattermost upstream: mattermost/mattermost@90e3d28#diff-f5086c95f98abc474eb1d0d1f70e217d79808c31f2417d5a366181a406ab6822

Unsure if this is the correct way to do it though, the pages argument got removed and the system console page is not being set as property.

Copy link
Contributor

@BenCookie95 BenCookie95 left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment about the colours

webapp/src/components/legal_holds_setting.tsx Outdated Show resolved Hide resolved
@fmartingr
Copy link
Contributor Author

I have made some changes to allow backwards comaptibility:

  • Added omitempty to the new fields to ensure atomic updates don't fail.
  • Changed the field that stores the last message retrieved to a bool (from time.Time), this way we only need to check a boolean to know if the hold contains messages or not and it's easier to manage because of the below point.
  • When loading the plugin we now also check if a legal hold has an index file and update the legalHold.HasMessages field accordingly. This allows users to download already existing legal hold data when they upgrade the plugin and prevents problems if the plugin crashes by any reason in the middle of a legal hold.
  • Prevent plugin from failing on load when this block is executed, since most failures would come from atomic updates and that would require manual database intervention logging the errors instead for debugging while allowing the user continuous plugin usage.

cc: @BenCookie95 @davidkrauser Not requesting a re-review, but feel free to do so.

server/plugin.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2: Dev Review Requires review by a core committer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a "run now" button on legal holds in system console
5 participants