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.

Sign up for GitHub

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

Jump to bottom

[MM-60513] Import profile pictures from Slack. #57

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[MM-60513] Import profile pictures from Slack. #57

wants to merge 3 commits into from
+82 −18

Conversation

wetneb
Copy link
Contributor

@wetneb wetneb commented Sep 20, 2024 •
edited by isacikgoz
Loading

Summary

This makes it possible to import profile pictures from Slack.

The code expects a Slack zip archive produced by slack-advanced-exporter with the additional PR:
grundleborg/slack-advanced-exporter#37

Given that slack-advanced-exporter doesn't seem actively developed, I am not sure if the PR there has any chances of getting through, so which makes it unlikely that this PR can also get accepted.
For this reason, it's perhaps a better idea to do the downloading in mmetl directly…

Ticket Link

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

Closes #53.

@wetneb wetneb mentioned this pull request Sep 20, 2024
Open
@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Oct 14, 2024
@hanzei hanzei requested review from fmartingr and isacikgoz October 14, 2024 10:27
@isacikgoz isacikgoz changed the title Import profile pictures from Slack. [MM-60513] Import profile pictures from Slack. Oct 15, 2024
@wetneb
Import profile pictures from Slack.
956b26f 
Closes mattermost#53.

This expects a Slack zip archive produced by slack-advanced-exporter
with the additional PR:
grundleborg/slack-advanced-exporter#37
@wetneb wetneb force-pushed the import_profile_pictures branch from dbfae1d to 956b26f Compare October 15, 2024 13:21
fmartingr
fmartingr approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@fmartingr fmartingr left a comment

Choose a reason for hiding this comment

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

This LGTM.

commands/transform.go Outdated Show resolved Hide resolved
@wetneb
Copy link
Contributor Author

wetneb commented Oct 16, 2024 •
edited
Loading

Thanks for the review!

In case this was unclear from my description above: the dump format which this PR expects is something that I made up.

It is generated by grundleborg/slack-advanced-exporter#37 but does not correspond (as far as I know) to anything produced by Slack itself. Slack just provides the URLs of the profile pictures without including them in the dump - at least for the dumps I could get generate myself.

So unless you are happy with documenting that mmetl expects a dump format computed by my fork of slack-advanced-exporter, I would recommend against merging this as it stands. So it's worth reconsidering whether it wouldn't be okay for mmetl to do the downloading of the images itself, and the broader relationship to slack-advanced-exporter given that it's unmaintained and still referenced in the migration docs.

Marking the PR as draft to make this clearer

@wetneb wetneb marked this pull request as draft October 16, 2024 12:00
@mattermost-build
Copy link

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@agnivade
Copy link
Member

ping @isacikgoz

@isacikgoz
Copy link
Member

@wetneb I'd say let's document that for the profile pictures, they can use your fork of slack-advanced-exporter. Since it's a community driven project, it wouldn't do harm to use yours.

isacikgoz
isacikgoz reviewed Dec 13, 2024
View reviewed changes
commands/transform.go Outdated
@@ -93,6 +93,19 @@ func transformSlackCmdF(cmd *cobra.Command, args []string) error {
}
}

if !skipAttachments {
Copy link
Member

Choose a reason for hiding this comment

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

We may want to activate this path only if the flag is set. (eg. --include-profile-pictures)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency with the other flags I have introduced it as --skip-profile-pictures, with a default set to false.

@isacikgoz isacikgoz self-requested a review December 13, 2024 14:15
@wetneb wetneb marked this pull request as ready for review December 14, 2024 19:25
@wetneb wetneb force-pushed the import_profile_pictures branch from 332b07f to fdb7f68 Compare December 14, 2024 19:35
@wetneb
Copy link
Contributor Author

wetneb commented Dec 14, 2024

As you want. Once this is released, the docs to be updated are at https://docs.mattermost.com/onboard/migrate-from-slack.html.

@wetneb @fmartingr
Update commands/transform.go
91442cc 
Co-authored-by: Felipe Martin <[email protected]>
isacikgoz
isacikgoz approved these changes Dec 19, 2024
View reviewed changes
Copy link
Member

@isacikgoz isacikgoz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @wetneb

@isacikgoz isacikgoz added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Dec 19, 2024
@isacikgoz
Copy link
Member

As you want. Once this is released, the docs to be updated are at https://docs.mattermost.com/onboard/migrate-from-slack.html.

Feel free to get the PR ready, we can merge once a new version of mmetl gets published.

@mattermost-build
Copy link

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

wetneb added a commit to wetneb/docs that referenced this pull request Dec 31, 2024
@wetneb
Update migrate-from-slack.rst to include profile pictures
13bdfc8 
Documents mattermost/mmetl#57.
@wetneb wetneb mentioned this pull request Dec 31, 2024
Draft
@wetneb
Copy link
Contributor Author

wetneb commented Dec 31, 2024

I have opened a PR to update the docs here: mattermost/docs#7674

@fmartingr fmartingr requested a review from isacikgoz January 7, 2025 16:57
@mattermost-build
Copy link

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmartingr fmartingr fmartingr approved these changes

@isacikgoz isacikgoz Awaiting requested review from isacikgoz

Assignees
No one assigned
Labels
3: Reviews Complete All reviewers have approved the pull request Lifecycle/1:stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support importing profile pictures from Slack
6 participants
@wetneb @mattermost-build @agnivade @isacikgoz @fmartingr @hanzei