Skip to content

Comments

Chore/wpb 20631 update welcome message#52

Merged
MarianKijewski merged 2 commits intomainfrom
chore/WPB-20631_update_welcome_message
Jan 19, 2026
Merged

Chore/wpb 20631 update welcome message#52
MarianKijewski merged 2 commits intomainfrom
chore/WPB-20631_update_welcome_message

Conversation

@MarianKijewski
Copy link
Collaborator


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

The capabilities of the App were not clear once it's added to the conversation

Solutions

Greet, describe App's purpose and usage when added to a conversation.

Testing

Test Coverage

  • I have added automated test to this contribution

How to Test

Verify that there's a message, once an App is added to the conversation.


References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@MarianKijewski MarianKijewski requested a review from a team as a code owner January 19, 2026 08:01
@claude
Copy link

claude bot commented Jan 19, 2026

PR Review: Update Welcome Message

Summary

This PR improves the welcome message sent when the Poll App is added to a conversation. The changes enhance user experience by providing a clearer introduction and explanation of the app's capabilities.

✅ Strengths

  1. Improved User Experience: The new welcome message is more friendly and informative, clearly explaining the app's purpose before diving into usage instructions.

  2. Better Code Organization: Consolidating the welcome message components into clearly named constants (WELCOME_TEXT, USAGE, WELCOME_MESSAGE) improves readability and maintainability.

  3. Consistent Messaging: Using the same USAGE constant across both the welcome message and help command ensures consistency.

🔍 Code Quality Observations

Positive:

  • The refactoring follows existing Kotlin conventions in the codebase
  • Constants are properly organized in the companion object
  • The string concatenation approach is consistent with patterns used elsewhere in the project
  • KDoc comments remain accurate and descriptive

Minor Considerations:

  1. Visibility Modifiers (src/main/kotlin/com/wire/apps/polls/services/UserCommunicationService.kt:29-32):

    • USAGE and WELCOME_MESSAGE are declared as public (const val) while WELCOME_TEXT is private (private const val)
    • Consider whether USAGE and WELCOME_MESSAGE need to be public. If they're only used within this class, making them private would better encapsulate the implementation details.
    • Recommendation: Add private modifier to maintain consistency:
      private const val USAGE = "To create poll please text: " +
          "`/poll \"Question\" \"Option 1\" \"Option 2\"`. To display usage write `/poll help`"
      private const val WELCOME_MESSAGE = WELCOME_TEXT + USAGE
  2. String Formatting (src/main/kotlin/com/wire/apps/polls/services/UserCommunicationService.kt:25-28):

    • The multi-line string concatenation with + operator works but could be more readable using triple-quoted strings or trimIndent()
    • However, the current approach is consistent with the USAGE constant style, so this is acceptable.

🧪 Test Coverage

Observation: The existing test file (UserCommunicationServiceTest.kt) doesn't include tests for the sayHello() method or message content verification.

Recommendation: Consider adding a test to verify the welcome message format, especially since this is user-facing content:

@Test
fun `when app says hello, then send welcome message with usage instructions`() = runTest {
    // This would verify the message structure and content
    userCommunicationService.sayHello(manager, Stub.id())
    
    coVerify {
        proxySenderService.send(
            manager = manager,
            message = match { it.text.contains("Hi, I'm the Poll App") }
        )
    }
}

However, given that the PR checklist shows "I have not added automated test to this contribution," this appears to be an intentional decision for this change.

🔒 Security

No security concerns identified. The changes are purely presentational and don't introduce any:

  • Input validation issues
  • Injection vulnerabilities
  • Data exposure risks
  • Authentication/authorization changes

⚡ Performance

No performance impact. The change from building strings at runtime to using compile-time constants (if anything) slightly improves performance.

📝 Documentation

The usage instructions in the code match the README.md commands, which is good for consistency. The welcome message now serves as inline documentation for users.

🎯 Recommendations Summary

Optional Improvements:

  1. Consider making USAGE and WELCOME_MESSAGE private for better encapsulation
  2. Consider adding basic test coverage for the welcome message content (low priority)

Approval Status: ✅ Ready to merge

The changes are clean, improve user experience, and don't introduce any bugs or issues. The minor suggestions above are optional refinements that could be addressed in a follow-up PR if desired.


Review generated by Claude Code

@MarianKijewski
Copy link
Collaborator Author

image

Copy link

@bbaarriiss bbaarriiss left a comment

Choose a reason for hiding this comment

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

Appoved with a simple, optional todo.

@@ -22,13 +22,14 @@ class UserCommunicationService(
private val version: String
) {
private companion object : KLogging() {

Choose a reason for hiding this comment

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

Btw, I don't think we need KLogging() here. It could be nice to remove it as well, hopefully it will not break sth unexpected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for mentioning that. After wanting to verify why it's there I've noticed that this dependency is very outdated (2.0.6 v.s. 7.0.14). I've created a Jira ticket for it for further investigation of why it's the case: WPB-22879

@MarianKijewski MarianKijewski merged commit 76a9009 into main Jan 19, 2026
8 checks passed
@MarianKijewski MarianKijewski deleted the chore/WPB-20631_update_welcome_message branch January 19, 2026 10:15
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.

2 participants