Skip to content

[MM-63516] Target LegalHold users by groups#137

Merged
davidkrauser merged 26 commits into
mattermost:mainfrom
davidkrauser:specify-with-groups
Jul 8, 2025
Merged

[MM-63516] Target LegalHold users by groups#137
davidkrauser merged 26 commits into
mattermost:mainfrom
davidkrauser:specify-with-groups

Conversation

@davidkrauser

@davidkrauser davidkrauser commented Mar 20, 2025

Copy link
Copy Markdown
Contributor

Summary

This change adds support for specifying LDAP groups when defining Legal Hold users. Previously, administrators could only select individual users. Now, they can specify groups, and all users within those groups will be included in the legal hold report. This makes managing users in a report much easier and more scalable.

Ticket Link

https://mattermost.atlassian.net/browse/MM-63516

Changes

The commits in this PR are roughly grouped by specific change:

New Features

  • Legal Hold Struct Update (62319cf): Added group IDs to the legal hold struct to retrieve users during execution.
  • Legal Hold Execution Update (07e8c0d): Modified execution logic to include all users from specified groups in the report.
  • Group Search API (511d72e): Added an API to search for groups by display name, enabling autocomplete in the web UI.
  • Group Input in UI (522a525, 3cff45a, eea0ae0):
    • Added a group input component with name autocomplete. This is an almost identical copy of the user input component.
    • Added group input fields to the legal hold creation and editing forms.
    • Displayed group information in the legal hold table.
  • Group Info API (8975922): Added an API call to retrieve relevant group info.
    • Without this, we won't have group information to show in the legal hold editing form.

Bug Fixes & Maintenance

  • Job Execution Bug Fix (afa23cd): Fixed an issue where a job execution error could cause an infinite loop. We now stop executing on error.
  • Lint Fixes (12ca57f): Addressed various minor linting issues.

Limitations

The end user is currently limited to 5000 users in a group. This is way more than I expect a user will use, and thus isn't a real limitation. I included this just to ensure we don't have an unbounded group size. It's specified here, and can be updated later if we want to limit it further:

const GroupPageLimit = 100
const GroupPageSize = 50

The real limit is still the total number of posts that can be included in a report, which is specified here:

const PostExportBatchLimit = 10000

Video Demo

Here is a video of the new UI components in action:

legal-hold-group-ui.webm

Update: you can now search by group @-name

This was requested here: #137 (comment)

search-by-name.webm

@davidkrauser davidkrauser changed the title Specify with groups MM-63516 Specify with groups Mar 20, 2025
@davidkrauser davidkrauser changed the title MM-63516 Specify with groups [MM-63516] Specify with groups Mar 20, 2025
@davidkrauser davidkrauser changed the title [MM-63516] Specify with groups [MM-63516] Target LegalHold users by groups Mar 20, 2025
@davidkrauser

Copy link
Copy Markdown
Contributor Author

@fmartingr some of the Playwright Tests are failing on this PR, but I don't believe they are related to my change. For example: ensure server has license is failing.

I know you recently made fixes for Playwright Tests - do you have any ideas on why they may be failing here?

@fmartingr

Copy link
Copy Markdown
Contributor

@fmartingr some of the Playwright Tests are failing on this PR, but I don't believe they are related to my change. For example: ensure server has license is failing.

I know you recently made fixes for Playwright Tests - do you have any ideas on why they may be failing here?

I investigated a bit and there was a change on the trial request this week, country code is now mandatory. I've managed this in a new PR and I'm waiting on directions on how to fill the country field: #138

@fmartingr fmartingr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great work on this! Left a few non-blocking comments/questions.

Comment thread server/legalhold/legal_hold.go
Comment thread server/model/index.go
Comment thread webapp/src/components/legal_hold_table/index.ts Outdated
Comment thread webapp/src/components/legal_hold_table/legal_hold_table.tsx Outdated

@BenCookie95 BenCookie95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks mostly good! Just a few comments

Comment thread webapp/src/components/groups_input/groups_input.jsx
Comment thread webapp/src/components/groups_input/index.ts Outdated
Comment thread webapp/src/components/legal_hold_table/legal_hold_table.tsx Outdated
Comment thread server/store/sqlstore/group.go Outdated
Comment thread server/store/sqlstore/group.go
Comment thread server/store/sqlstore/group.go Outdated
Comment thread server/store/sqlstore/group.go
Comment thread webapp/src/components/groups_input/groups_input.jsx
@BenCookie95 BenCookie95 self-requested a review April 1, 2025 13:33

@BenCookie95 BenCookie95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, nice work

@AayushChaudhary0001

Copy link
Copy Markdown

@davidkrauser We need to run this job whenever we want for testing, for which there is already a PR created #129 . Is there any other way to run this job(one could be manually hitting the API through postman) since this PR is not merged yet?

@davidkrauser

Copy link
Copy Markdown
Contributor Author

@AayushChaudhary0001 we could wait until #129 merges before testing this if that helps - it looks like that one is close to finished. WDYT?

@AayushChaudhary0001

Copy link
Copy Markdown

@davidkrauser It consists of some comments, which should not take much time to fix. Once that is merged, we can quickly test this as well as it would help us in the completion. Let me know if this works for you.

@davidkrauser

Copy link
Copy Markdown
Contributor Author

@AayushChaudhary0001 that would work just fine, thank you.

This fixes:
- A compilation error (userID -> user.Id)
- Broken grid layout on legal hold table view (needed an extra column)
- Broken handling of null user ID array
@davidkrauser

Copy link
Copy Markdown
Contributor Author

@AayushChaudhary0001 This should be ready to be looked at again when you are available. I've merged in the latest from 'main' and fixed up the conflicts.

@davidkrauser davidkrauser merged commit f34abdb into mattermost:main Jul 8, 2025
5 checks passed
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.

4 participants