zulip icon indicating copy to clipboard operation
zulip copied to clipboard

slack_import: Improve how Slack thread messages are handled.

Open PieterCK opened this issue 9 months ago • 11 comments

Overview

This PR enhances the handling of Slack thread messages during the Slack export conversion for import. Additionally, it introduces logic to address near-link messages in the import process. Note that some specifications differ from the original details mentioned in the linked GitHub issues due to discussions in the CZO discussion and subsequent iterations.

Fixes

Fixes #27661.

Topic names for Slack threads now include a snippet of the original message of the thread.

Thread topic name is thread date + a snippet of the original message (45 characters in total) if it's more than that, the topic name will cut off image

truncated topic name because the snippet is too long: image

thread topics and their original thread messages: image

documentation changes: /help/import-from-slack

Previous After
image image

Fixes #27626.

Original thread messages are now placed in the main import topic, with an additional cross-linking message referencing its thread topic. Added support to convert thread replies marked with "Also send to #channel".

Converted Slack thread.

Slack Zulip
image image

the original thread message is now in the main "imported from Slack" topic instead of in its branched-off thread topic.

Slack Zulip
image image

Thread reply marked with "Also send to #channel".

Slack
image
Zulip
image

Thread conversations with deleted or missing original thread messages. image

Imported thread topics. image

Fixes #31100.

During export, messages containing near-links are fixed by replacing the old IDs in the near-link with the new IDs after remapping takes place.

Previous

Screencast from 07-30-2024 03:38:47 PM.webm

After

Screencast from 07-30-2024 03:42:02 PM.webm

CZO

Testing:

note if you're trying to manually test this PR

All of the screenshots above are from this Slack export: slack_import2.zip If you need to take a look at the original Slack messages, you can also join the Slack workspace that export here: https://join.slack.com/t/ds-py62195/shared_invite/zt-2nf8pmv06-qRMN67bTvHfDHUqlUzeo9g

Concern:

Here are some areas for improvement I've noticed while working on this PR, all related to the data_import/slack.py code area. None of these are critical, but addressing them would make things a bit nicer. Let me know which ones we should investigate further, and I'll compile them into a GitHub issue.

  • The ZerverFIeldsT type alias is used for both zulip data structure and slack data structure. The user_list variable in data_import/slack.py is a list of Slack user data (from Slack API) using ZerverFieldsT type alias and zerver_userprofile list variable which is the converted user_list also uses the same ZerverFieldsT type alias. I think using something like SlackFieldsT for raw Slack data would make things more readable
  • we're using a users Slack fullname to create zulip mentions when converting messages. Even though Slack user will be converted to Zulip userprofile and have identical fullname anyways, why don't we just use their Zulip full name instead?
  • we’re currently iterating over a list of all users for every mention in a message (get_user_mentions). Instead, we could create a dictionary with the Zulip user ID as the key and the Zulip user profile (zerver_userprofile) as the value.

Follow Up:

  • Replace the linking method to thread topics to use permalinks once #21505 is implemented. discussed here
  • Bumping an old Rocket Chat thread/reply conversion feature here. A couple of users have requested an implementation of a similar thread/reply conversion for Rocket Chat export. CZO
  • Once this PR is merged, changes related to #30465 can be refactored to use the new get_zulip_mention_for_slack_user to reduce duplicative code.
Self-review checklist
  • [x] Self-reviewed the changes for clarity and maintainability (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • [x] Explains differences from previous plans (e.g., issue description).
  • [x] Highlights technical choices and bugs encountered.
  • [ ] Calls out remaining decisions and concerns.
  • [x] Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • [x] Each commit is a coherent idea.
  • [x] Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • [x] Visual appearance of the changes.
  • [ ] Responsiveness and internationalization.
  • [ ] Strings and tooltips.
  • [ ] End-to-end functionality of buttons, interactions and flows.
  • [x] Corner cases, error conditions, and easily imagined bugs.

PieterCK avatar May 22 '24 07:05 PieterCK