feat: import Slack bot users as Mattermost bot accounts#79
Merged
feat: import Slack bot users as Mattermost bot accounts#79
Conversation
Prevents test utilities from being compiled and published with the services/slack package while keeping them importable by any test.
… issues - Terminate orphaned containers on error in Postgres and Mattermost setup - Validate Docker Hub HTTP status before decoding response - Add json:"-" tag to SlackChannel.Type to clarify it is computed - Only set DeleteAt on user export when user is actually deleted
Reset all flag values and Changed state on the shared RootCmd before each Execute() call, preventing flag pollution across subtests. Also fix duplicate step numbering in test comments.
Slack bot users are now exported as native Mattermost bot records (type "bot" with BotImportData) instead of regular user accounts. This ensures bots get proper Bot records with IsBot=true, avoids email validation issues for bots, and correctly attributes bot posts. Adds --bot-owner CLI flag (required when bots are present) to set the owner of all imported bots. Bots are skipped during Sanitise and PopulateUserMemberships. Placeholder bots created for missing bot references in posts are also exported as bot type. Includes unit tests for all new functions and e2e tests verifying bot import, bot posts, --bot-owner validation, and deleted bots.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Slack bot-import support: introduces a Changes
Sequence DiagramsequenceDiagram
participant User as "User"
participant CLI as "Transform CLI"
participant Transformer as "Slack Transformer"
participant Intermediate as "Intermediate Layer"
participant Exporter as "Exporter"
participant Output as "Import File"
User->>CLI: run `mmetl transform slack --bot-owner <owner>`
CLI->>Transformer: invoke TransformSlack(..., botOwner)
Transformer->>Intermediate: TransformUsers (detect bots, create placeholders)
Intermediate->>Intermediate: mark users with IsBot / DisplayName
Transformer->>Exporter: Export(outputPath, botOwner)
Exporter->>Exporter: validate botOwner if bots exist
Exporter->>Exporter: write regular users then bot import lines (with owner)
Exporter->>Output: emit import file (users + bots)
Output-->>User: produced import archive ready for upload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Set ImagePlatform to linux/amd64 for the Mattermost container so Docker pulls the correct image on ARM hosts (no arm64 variant exists) - Normalize ZIP entry paths with filepath.ToSlash for cross-platform compliance - Respect context cancellation in the import job polling loop
wiggin77
reviewed
Mar 5, 2026
wiggin77
reviewed
Mar 5, 2026
wiggin77
reviewed
Mar 5, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
commands/transform_slack_e2e_test.go (1)
436-437: UsetransformLogFileconstant for log cleanup calls.These repeated string literals can drift over time; reusing the existing constant keeps cleanup consistent.
Also applies to: 509-510, 571-572, 601-602, 636-637
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/transform_slack_e2e_test.go` around lines 436 - 437, Replace hardcoded "transform-slack.log" string literals used for cleanup with the existing transformLogFile constant so the defer os.Remove calls reference transformLogFile (e.g., update the defer os.Remove("transform-slack.log") usages around the transform test cleanup points to defer os.Remove(transformLogFile)); ensure all occurrences noted (lines near the defer calls previously using the literal) are updated to use transformLogFile to keep log file name centralized.testhelper/testhelper.go (1)
429-440: AlignGetBotByUsernamedocstring with actual behavior.The comment says this returns
nilwhen the user doesn't exist or isn't a bot, but the current implementation propagates API errors for those cases. Either update the comment or normalize expected not-found/non-bot errors tonil, nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testhelper/testhelper.go` around lines 429 - 440, The docstring for GetBotByUsername is out of sync with behavior: update GetBotByUsername to normalize "not found" and "not-a-bot" API errors into a nil, nil return instead of propagating errors, or alternatively update the comment to reflect that errors are propagated; to implement the normalization path, call th.Client.GetBotIncludeDeleted(ctx, user.Id, "") as currently done, inspect the returned error and if it indicates a 404/not-found or a specific "not a bot" response from the API (from Client.GetBotIncludeDeleted's error type/contents), return nil, nil; otherwise return bot, err as before. Ensure the function comment matches the chosen behavior (either keep the current comment and convert the errors to nil,nil, or change the comment to state that API errors are returned).services/slack/intermediate_test.go (1)
937-1027: Add a regression case for bots with emptyProfile.BotID.Please add a case where
IsBot == trueandProfile.BotID == ""to lock down expected fallback behavior and prevent accidental map-key collisions on empty IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/slack/intermediate_test.go` around lines 937 - 1027, Add a regression sub-test inside TestTransformBotUsers that creates a SlackUser with IsBot == true and Profile.BotID == "" and calls TransformUsers; assert the bot is stored under its SlackUser.Id (e.g., "U003") in slackTransformer.Intermediate.UsersById, that the entry has IsBot == true and expected display/name fields, and assert there is no entry at the empty-string key (""), to lock down the fallback behavior for empty Profile.BotID and prevent map-key collisions. Reference TransformUsers, SlackUser, SlackProfile.BotID and Intermediate.UsersById when adding the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commands/transform.go`:
- Around line 145-155: The current check for botOwner only rejects an empty
string but allows whitespace-only values; update the validation after computing
hasBots (iterating slackTransformer.Intermediate.UsersById) to trim botOwner
(using strings.TrimSpace) and treat a trimmed-empty value as missing — i.e., if
hasBots and strings.TrimSpace(botOwner) == "" return the same fmt.Errorf about
missing --bot-owner so whitespace-only inputs are rejected.
In `@services/slack/export.go`:
- Around line 430-436: ExportUsers currently assumes callers supply a non-empty
botOwner which can produce invalid bot import lines; enforce this invariant
locally by validating botOwner inside ExportUsers (either at the start or
immediately before writing bots in the loop) and return a clear error if
botOwner is empty instead of proceeding. Update the loop that calls
GetImportLineFromBot and ExportWriteLine to first check botOwner (or validate
per-bot) and fail fast with a descriptive error; reference ExportUsers,
GetImportLineFromBot, and ExportWriteLine when making the change.
In `@services/slack/intermediate.go`:
- Around line 189-193: When marking a Slack user as a bot in the block that
checks user.IsBot, guard against an empty user.Profile.BotID before assigning it
to newUser.Id; if user.Profile.BotID is empty, fall back to using user.ID (or
another unique identifier already present on the Slack user object) so you don't
set newUser.Id to an empty string and risk overwriting existing entries. Update
the user.IsBot/newUser.DisplayName logic in the same branch accordingly so
newUser.Id is always a non-empty unique key.
---
Nitpick comments:
In `@commands/transform_slack_e2e_test.go`:
- Around line 436-437: Replace hardcoded "transform-slack.log" string literals
used for cleanup with the existing transformLogFile constant so the defer
os.Remove calls reference transformLogFile (e.g., update the defer
os.Remove("transform-slack.log") usages around the transform test cleanup points
to defer os.Remove(transformLogFile)); ensure all occurrences noted (lines near
the defer calls previously using the literal) are updated to use
transformLogFile to keep log file name centralized.
In `@services/slack/intermediate_test.go`:
- Around line 937-1027: Add a regression sub-test inside TestTransformBotUsers
that creates a SlackUser with IsBot == true and Profile.BotID == "" and calls
TransformUsers; assert the bot is stored under its SlackUser.Id (e.g., "U003")
in slackTransformer.Intermediate.UsersById, that the entry has IsBot == true and
expected display/name fields, and assert there is no entry at the empty-string
key (""), to lock down the fallback behavior for empty Profile.BotID and prevent
map-key collisions. Reference TransformUsers, SlackUser, SlackProfile.BotID and
Intermediate.UsersById when adding the test.
In `@testhelper/testhelper.go`:
- Around line 429-440: The docstring for GetBotByUsername is out of sync with
behavior: update GetBotByUsername to normalize "not found" and "not-a-bot" API
errors into a nil, nil return instead of propagating errors, or alternatively
update the comment to reflect that errors are propagated; to implement the
normalization path, call th.Client.GetBotIncludeDeleted(ctx, user.Id, "") as
currently done, inspect the returned error and if it indicates a 404/not-found
or a specific "not a bot" response from the API (from
Client.GetBotIncludeDeleted's error type/contents), return nil, nil; otherwise
return bot, err as before. Ensure the function comment matches the chosen
behavior (either keep the current comment and convert the errors to nil,nil, or
change the comment to state that API errors are returned).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b5b4733-76c5-4cdc-9a07-fa609ba732a9
📒 Files selected for processing (11)
commands/transform.gocommands/transform_slack_e2e_test.godocs/cli/mmetl_transform_slack.mdservices/slack/export.goservices/slack/export_test.goservices/slack/intermediate.goservices/slack/intermediate_test.goservices/slack/parse.gotesthelper/slack_fixtures.gotesthelper/slack_fixtures_test.gotesthelper/testhelper.go
- Change IsBotMessage() to check BotId != "" instead of relying on subtype, ensuring posts from bots are correctly routed regardless of their subtype field - Move bot message case before plain message in TransformPosts switch to prevent bot posts from being misattributed to regular users - Guard against empty BotID in TransformUsers, falling back to user ID - Trim whitespace on --bot-owner flag to reject whitespace-only values - Add defensive validation in ExportUsers to fail fast if bots exist but botOwner is empty - Add regression test for empty BotID fallback behavior - Use transformLogFile constant in e2e test cleanup - Fix GetBotByUsername docstring to reflect actual error propagation - Remove Description from bot export (no longer set)
Mattermost's importBot silently accepts unknown owner usernames by falling back to plugin-owner semantics (storing the raw string as OwnerId). This test documents that behaviour so we notice if server-side semantics change.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
services/slack/export.go (1)
421-432: Fail fast before writing any lines when bot owner is missing.At Line 430, the error check runs after regular users are already written in Lines 422-427, so callers can end up with partial JSONL output on failure.
🔧 Proposed refactor
func (t *Transformer) ExportUsers(writer io.Writer, botOwner string) error { // Collect users from map and sort them by username for deterministic output users := make([]*IntermediateUser, 0, len(t.Intermediate.UsersById)) bots := make([]*IntermediateUser, 0) for _, user := range t.Intermediate.UsersById { if user.IsBot { bots = append(bots, user) } else { users = append(users, user) } } // Sort by username to ensure consistent ordering sort.Slice(users, func(i, j int) bool { return users[i].Username < users[j].Username }) sort.Slice(bots, func(i, j int) bool { return bots[i].Username < bots[j].Username }) + // Validate before writing to avoid partial output on error. + if len(bots) > 0 && botOwner == "" { + return errors.New("cannot export bots without a bot owner: please provide the --bot-owner flag") + } + // Write regular users first (bot owner must exist before bots) for _, user := range users { line := GetImportLineFromUser(user, t.TeamName) if err := ExportWriteLine(writer, line); err != nil { return err } } // Write bots after users - if len(bots) > 0 && botOwner == "" { - return errors.New("cannot export bots without a bot owner: please provide the --bot-owner flag") - } for _, bot := range bots { line := GetImportLineFromBot(bot, botOwner) if err := ExportWriteLine(writer, line); err != nil { return err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/slack/export.go` around lines 421 - 432, Move the bot-owner validation so it runs before any output is written: check the existing variables (bots and botOwner) at the top of the export flow and return the error if bots exist but botOwner == "" before calling GetImportLineFromUser or ExportWriteLine; in other words, perform the "cannot export bots without a bot owner" check prior to the loop that iterates over users and writes lines to writer to avoid producing partial JSONL output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Line 4: The CI config sets the global auto-review flag "enabled: false" which
disables automatic PR reviews; if this was unintentional, change the "enabled"
key back to true (or restore the previous .coderabbit.yaml value) and commit the
corrected value so auto-review is re-enabled, otherwise add a brief comment in
the PR explaining the intentional scope and reason for turning off auto-reviews
to avoid silent coverage loss.
In `@services/slack/intermediate.go`:
- Line 203: The Debugf call currently logs raw PII (t.Logger.Debugf(...,
newUser.Email)); do not log newUser.Email directly. Replace that log with a
non-sensitive identifier (e.g., newUser.ID or Slack user ID) or a masked/hashed
email computed via a helper (e.g., maskEmail or hashPII) and log that value
instead; update the call site (t.Logger.Debugf) to emit the safe identifier and
add a small helper function to mask or hash emails if you need deterministic
correlation.
---
Nitpick comments:
In `@services/slack/export.go`:
- Around line 421-432: Move the bot-owner validation so it runs before any
output is written: check the existing variables (bots and botOwner) at the top
of the export flow and return the error if bots exist but botOwner == "" before
calling GetImportLineFromUser or ExportWriteLine; in other words, perform the
"cannot export bots without a bot owner" check prior to the loop that iterates
over users and writes lines to writer to avoid producing partial JSONL output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4d9c0df-d5ff-4ec5-8c6c-c950842a0f69
📒 Files selected for processing (9)
.coderabbit.yamlcommands/transform.gocommands/transform_slack_e2e_test.goservices/slack/export.goservices/slack/export_test.goservices/slack/intermediate.goservices/slack/intermediate_test.goservices/slack/models.gotesthelper/testhelper.go
Bot users may not have email addresses, making the previous log message unhelpful. Username is always available and more useful for identifying imported users.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/slack/intermediate.go`:
- Around line 672-705: The bot-message branch currently only runs when
post.IsBotMessage() (which requires a non-empty BotId); update the logic to also
treat posts with subtype "bot_message" as bot posts even when BotId is empty:
e.g., change the branch condition or add an additional branch that checks
post.SubType == "bot_message" and then create a fallback author (call
CreateIntermediateBotUser with a generated/placeholder ID or map empty BotId to
a unique key), build the IntermediatePost (same fields used: author.Username,
channel.Name, post.Text, reactions, timestamp), call AddFilesToPost and
AddAttachmentsToPost with the same props handling, and finally call
AddPostToThreads so these bot posts are not dropped or misclassified; reference
symbols: IsBotMessage(), CreateIntermediateBotUser(), IntermediatePost,
AddFilesToPost(), AddAttachmentsToPost(), AddPostToThreads().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f5ab78ef-83e8-4a63-811b-3a2ded4a792e
📒 Files selected for processing (1)
services/slack/intermediate.go
…Username fields Some Slack exports contain messages with subtype "bot_message" but no BotId. Expand IsBotMessage() to also match on SubType and add a fallback chain (User → BotUsername → timestamp placeholder) so these messages are no longer silently dropped during import.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commands/transform_slack_e2e_test.go`:
- Line 706: The string literal assigned to fakeOwner ("fake_user_non_existant")
is misspelled; change it to the correct spelling (e.g., "fake_user_nonexistent")
to satisfy golangci-lint (misspell). Locate the assignment to fakeOwner in
transform_slack_e2e_test.go and update only the string value so tests and
references to the fakeOwner variable remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ebaa7353-a70d-41f9-911a-d5d4218e09e1
📒 Files selected for processing (4)
commands/transform_slack_e2e_test.goservices/slack/intermediate.goservices/slack/intermediate_test.goservices/slack/models.go
🚧 Files skipped from review as they are similar to previous changes (1)
- services/slack/models.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
commands/transform_slack_e2e_test.go (2)
522-540: Cache bot user lookups before iterating posts.
AssertUserExistsis called inside the loop for matched messages. Pull both bot lookups outside the loop to reduce repeated integration/API calls.Refactor sketch
// 6. Verify we can find all three posts + deployBotUser := th.AssertUserExists(ctx, "deploybot") + alertBotUser := th.AssertUserExists(ctx, "alertbot") var foundDeploy, foundHuman, foundAlert bool for _, postID := range posts.Order { post := posts.Posts[postID] @@ if strings.Contains(post.Message, "Deployment started for v2.0.0") { foundDeploy = true - // Verify the post is attributed to the bot user - deployBotUser := th.AssertUserExists(ctx, "deploybot") assert.Equal(t, deployBotUser.Id, post.UserId, "deploy post should be attributed to deploybot user") } if strings.Contains(post.Message, "Alert: CPU usage above 90%") { foundAlert = true - // Verify the post is attributed to the alert bot user - alertBotUser := th.AssertUserExists(ctx, "alertbot") assert.Equal(t, alertBotUser.Id, post.UserId, "alert post should be attributed to alertbot user") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/transform_slack_e2e_test.go` around lines 522 - 540, The loop currently calls th.AssertUserExists(ctx, "deploybot") and th.AssertUserExists(ctx, "alertbot") inside the posts iteration causing repeated integration/API calls; call th.AssertUserExists once for each bot before the for _, postID := range posts.Order loop (store in deployBotUser and alertBotUser variables), then inside the loop use those cached deployBotUser.Id and alertBotUser.Id when asserting post.UserId for the deploy and alert matches (leave foundDeploy/foundHuman/foundAlert logic unchanged).
687-690: Avoid starting a second test environment inside the same top-level E2E test.This subtest spins a new
SetupHelperwhile the outer helper is still alive, which increases container/resource overlap and can hurt E2E stability/runtime. Prefer splitting this scenario into a separate top-level test (with its own helper lifecycle) so only one environment is active at a time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@commands/transform_slack_e2e_test.go` around lines 687 - 690, The subtest starting with t.Run("import succeeds with non-existent bot owner username", func(t *testing.T) { ... }) creates a new testhelper.SetupHelper while the outer test's helper is still active; remove the nested SetupHelper and instead extract this subtest into its own top-level test function (e.g., TestImportSucceedsWithNonExistentBotOwnerUsername) that calls testhelper.SetupHelper() at the start and defers th.TearDown(), then copy the subtest body into that new top-level test and delete the inner t.Run to ensure only one test environment is active at a time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@commands/transform_slack_e2e_test.go`:
- Around line 522-540: The loop currently calls th.AssertUserExists(ctx,
"deploybot") and th.AssertUserExists(ctx, "alertbot") inside the posts iteration
causing repeated integration/API calls; call th.AssertUserExists once for each
bot before the for _, postID := range posts.Order loop (store in deployBotUser
and alertBotUser variables), then inside the loop use those cached
deployBotUser.Id and alertBotUser.Id when asserting post.UserId for the deploy
and alert matches (leave foundDeploy/foundHuman/foundAlert logic unchanged).
- Around line 687-690: The subtest starting with t.Run("import succeeds with
non-existent bot owner username", func(t *testing.T) { ... }) creates a new
testhelper.SetupHelper while the outer test's helper is still active; remove the
nested SetupHelper and instead extract this subtest into its own top-level test
function (e.g., TestImportSucceedsWithNonExistentBotOwnerUsername) that calls
testhelper.SetupHelper() at the start and defers th.TearDown(), then copy the
subtest body into that new top-level test and delete the inner t.Run to ensure
only one test environment is active at a time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18302312-b511-45f9-a21f-ffdb6b8736ae
📒 Files selected for processing (1)
commands/transform_slack_e2e_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
"bot"type records (BotImportData) instead of regular user accounts, giving them properBotrecords withIsBot=true--bot-ownerCLI flag to specify the Mattermost user who owns all imported bots (required when bots are present in the export)--default-email-domainor--skip-empty-emailswas required just because of botsChanges
Core logic (
services/slack/intermediate.go):IntermediateUsergainsIsBotandDisplayNamefieldsTransformUserssets bot fields and skipsSanitisefor botsPopulateUserMembershipsskips bots (bots don't get team/channel memberships inBotImportData)CreateIntermediateBotUsercreates placeholder bots for missing bot references in postsExport (
services/slack/export.go):GetImportLineFromBotproduces{"type":"bot","bot":{...}}linesExportUserspartitions users and bots, writing users first (so the bot owner exists before bots are imported)CLI (
commands/transform.go):--bot-ownerflag with validation: errors if bots exist but flag is not setNote
Mattermost's server-side
importBotfunction does not currently processdelete_at(source), so deleted bots will be imported but not marked as deactivated. The export file does include the correctdelete_atvalue for when server support is added.Test plan
TransformUserswith bots,CreateIntermediateBotUser,PopulateUserMembershipsskipping bots,GetImportLineFromBot,ExportUsersordering--bot-ownervalidation (required/not required), deleted bot exportSummary by CodeRabbit
New Features
Tests
Documentation