[CM-1752] Collect email on Away Mode | iOS SDK #452
Conversation
WalkthroughThe pull request introduces several changes to enhance the handling of email collection in "Away Mode" within the application. It adds new localization strings for user prompts related to email collection, modifies the logic for displaying UI elements based on user interactions, and updates method signatures to return more detailed data structures. Additionally, a new asset for an email icon is introduced, and the Changes
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 SwiftLintSources/Kommunicate/Classes/KMConversationService.swiftThank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (9)
Sources/Kommunicate/Classes/Controllers/KMConversationController+MessageCharacterLimit.swift (1)
12-12: Consider improving variable naming and logic clarityThe variable name
emailAndAwayViewHiddenmight be misleading as it represents the inverse ofemailCollectionAwayModeEnabled. Consider renaming and restructuring for better clarity.- let emailAndAwayViewHidden = viewModel.emailCollectionAwayModeEnabled ? false : isAwayMessageViewHidden + let shouldShowHeaderView = viewModel.emailCollectionAwayModeEnabled || !isAwayMessageViewHiddenSources/Resources/Assets/Localizable.strings (1)
110-115: Consider making the error message more specific and actionable.While the new localization strings are well-structured and clear, the invalid email message could be more specific and actionable, consistent with other error messages in the file.
Consider updating the invalid email message to be more specific:
-InvalidEmailMessageOnAwayMode = "It seems you have entered an invalid email"; +InvalidEmailMessageOnAwayMode = "Please enter a valid email address";This change would:
- Be more consistent with other error messages in the file
- Provide clearer direction to users
- Match the direct style of other validation messages
Sources/Kommunicate/Classes/Views/AwayMessageView.swift (2)
13-18: LGTM! Well-structured localization implementation.Good practice using an enum for localized strings and leveraging the default configuration. The keys are descriptive and follow a consistent naming pattern.
Consider adding documentation comments for the LocalizedText enum to describe the purpose of each localized string.
35-40: Consider consolidating duplicate padding values.The padding values for
EmailMessageLabelare identical toMessageLabel. Consider creating shared padding constants if these values are intended to remain the same.enum Padding { + enum Common { + static let leading: CGFloat = 20.0 + static let trailing: CGFloat = 20.0 + } enum MessageLabel { static let top: CGFloat = 5.0 - static let leading: CGFloat = 20.0 - static let trailing: CGFloat = 20.0 + static let leading: CGFloat = Common.leading + static let trailing: CGFloat = Common.trailing } enum EmailMessageLabel { static let top: CGFloat = 8.0 - static let leading: CGFloat = 20.0 - static let trailing: CGFloat = 20.0 + static let leading: CGFloat = Common.leading + static let trailing: CGFloat = Common.trailing } }Example/Kommunicate/Base.lproj/Localizable.strings (1)
181-182: Consider aligning error message with existing email validation style.For consistency with existing email validation messages in the file (e.g.,
PreChatViewEmailInvalidError), consider updating the error message.CollectEmailMessageOnAwayMode = "How may we reach you?"; -InvalidEmailMessageOnAwayMode = "It seems you have entered an invalid email"; +InvalidEmailMessageOnAwayMode = "Please enter correct email address";Sources/Kommunicate/Classes/KMConversationService.swift (1)
279-296: Consider using custom error types for better error handling.The current implementation uses generic
APIError.messageNotPresent. Consider creating specific error types for different scenarios to provide more detailed error information.enum APIError: Error { + case emptyMessageList + case invalidMessageFormat + case missingRequiredField(String) case messageNotPresent case jsonConversion } func makeAwayMessageFrom(json: [String: Any]) -> Result<[String: Any], Error> { guard let data = json["data"] as? [String: Any], let messageList = data["messageList"] as? [Any] else { - return .failure(APIError.jsonConversion) + return .failure(APIError.missingRequiredField("messageList")) } if messageList.isEmpty { - return .failure(APIError.messageNotPresent) + return .failure(APIError.emptyMessageList) } // ... rest of the code ... guard let firstMessage = messageList.first as? [String: Any], let message = firstMessage["message"] as? String else { - return .failure(APIError.messageNotPresent) + return .failure(APIError.invalidMessageFormat) }Sources/Kommunicate/Classes/KMConversationViewController.swift (3)
100-104: Consider adding documentation for handleAwayUINoChange.The method handles a specific edge case but lacks documentation explaining its purpose and when it's called.
Add documentation like this:
+/// Handles UI updates when the away message view visibility hasn't changed but the agent/bot is online. +/// This ensures the UI stays consistent with the agent's online status. private func handleAwayUINoChange() {
342-356: Improve error handling in messageStatusAndFetchBotType.The success case handling for the away message data could be more robust:
- Consider extracting the dictionary parsing into a separate method
- Add logging for invalid data scenarios
Consider refactoring like this:
case let .success(awayMessageData): + guard let parsedData = parseAwayMessageData(awayMessageData) else { + print("Invalid away message data format") + self.isAwayMessageViewHidden = true + return + } + self.updateAwayMessageUI(with: parsedData) +private func parseAwayMessageData(_ data: Any) -> (message: String, emailCollection: Bool)? { + guard + let awayMessageData = data as? [String: Any], + let message = awayMessageData["message"] as? String, + !message.isEmpty, + let emailCollection = awayMessageData["collectEmailOnAwayMessage"] as? Bool + else { return nil } + return (message, emailCollection) +}
455-479: LGTM: Well-structured email collection overrides.The email collection related overrides (
showEmailCollectionUI,showInvalidEmail,awayModeEmailUpdated) are well-implemented:
- They properly call super implementations
- They check for the
collectEmailOnAwayModeflag- They maintain UI state consistency
However, consider adding unit tests for these methods to ensure the email collection behavior works correctly in all scenarios.
Would you like me to help generate unit tests for these methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Sources/Resources/Assets/KMAssets.xcassets/km_email_icon.imageset/alk_email_icon.pdfis excluded by!**/*.pdf
📒 Files selected for processing (7)
Example/Kommunicate/Base.lproj/Localizable.strings(1 hunks)Sources/Kommunicate/Classes/Controllers/KMConversationController+MessageCharacterLimit.swift(1 hunks)Sources/Kommunicate/Classes/KMConversationService.swift(2 hunks)Sources/Kommunicate/Classes/KMConversationViewController.swift(3 hunks)Sources/Kommunicate/Classes/Views/AwayMessageView.swift(5 hunks)Sources/Resources/Assets/KMAssets.xcassets/km_email_icon.imageset/Contents.json(1 hunks)Sources/Resources/Assets/Localizable.strings(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Sources/Resources/Assets/KMAssets.xcassets/km_email_icon.imageset/Contents.json
🔇 Additional comments (10)
Sources/Kommunicate/Classes/Controllers/KMConversationController+MessageCharacterLimit.swift (1)
14-17: 🛠️ Refactor suggestion
Simplify header height calculation logic
The current implementation uses nested ternary operations which can be hard to read and maintain. Consider restructuring the logic for better clarity.
- chatBar.headerViewHeight = emailAndAwayViewHidden ? 0 : awayMessageheight
- } else {
- chatBar.headerViewHeight = emailAndAwayViewHidden ? MessageCharacterLimitManager.charLimitViewHeight : awayMessageheight + MessageCharacterLimitManager.charLimitViewHeight
+ let baseHeight = shouldShowHeaderView ? awayMessageheight : 0
+ } else {
+ let charLimitHeight = MessageCharacterLimitManager.charLimitViewHeight
+ let baseHeight = shouldShowHeaderView ? awayMessageheight : 0
+ chatBar.headerViewHeight = baseHeight + charLimitHeightAdditionally, verify that adding these heights together doesn't cause layout issues.
Sources/Resources/Assets/Localizable.strings (1)
110-115: Verify translations for new strings.
New localization strings have been added for the email collection feature.
Let's check if these strings exist in other localization files:
✅ Verification successful
Let me check if there are any other language-specific localization files that might need these translations.
New strings are properly added to both localization files
The verification shows that both new strings (CollectEmailMessageOnAwayMode and InvalidEmailMessageOnAwayMode) are present in both localization files:
Example/Kommunicate/Base.lproj/Localizable.stringsSources/Resources/Assets/Localizable.strings
The project appears to use only these two localization files, with Base.lproj serving as the base localization. No language-specific .lproj directories were found, indicating that the app currently doesn't support multiple languages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for missing translations of the new strings
# Expected: These strings should exist in all localization files
# Find all Localizable.strings files
fd -e strings Localizable | while read -r file; do
echo "Checking $file for new strings..."
# Check for both new strings
grep -A 1 "CollectEmailMessageOnAwayMode\|InvalidEmailMessageOnAwayMode" "$file" || echo "Missing translations in $file"
done
Length of output: 951
Script:
#!/bin/bash
# Find all language-specific Localizable.strings files
fd -e strings Localizable | while read -r file; do
# Extract directory name to identify language
dir=$(dirname "$file")
echo "=== $dir ==="
# Show file content and structure
ls -la "$dir"
done
Length of output: 1281
Sources/Kommunicate/Classes/Views/AwayMessageView.swift (1)
Line range hint 109-155: LGTM! Well-structured view setup and constraints.
The view hierarchy setup and constraints are well-organized. Good practice setting initial visibility states explicitly.
Example/Kommunicate/Base.lproj/Localizable.strings (1)
179-182: LGTM! Well-organized localization additions.
The new section for Away Mode email collection is properly organized with a clear comment header and follows the file's existing structure.
Sources/Kommunicate/Classes/KMConversationService.swift (3)
138-138: LGTM: Return type change enables Away Mode email collection.
The updated return type allows the method to include additional metadata about email collection along with the away message.
274-277: LGTM: Proper validation for empty message list.
The check for empty message list prevents null message responses and provides clear error feedback.
138-138: Verify that API consumers handle the new response format.
The return type change from String to [String: Any] is a breaking change. Ensure that all consumers of awayMessageFor and makeAwayMessageFrom are updated to handle the new response format.
Also applies to: 266-296
✅ Verification successful
Let me analyze the results. I can see that:
awayMessageForis used inKMConversationViewController.swiftand the consumer is already handling the response using a switch statement withsuccess(awayMessageData)casemakeAwayMessageFromis only used internally withinKMConversationService.swift- The code in
KMConversationViewController.swiftappears to be updated to handle the dictionary response format based on theguardstatement following the success case
Let me verify the complete handling in the view controller to ensure it's properly consuming the new format.
API consumers are properly handling the new response format
The verification shows that:
- The only consumer
KMConversationViewController.swiftis already updated to handle the new[String: Any]response format - It correctly extracts
messageandcollectEmailOnAwayMessagefields from the dictionary response makeAwayMessageFromis only used internally withinKMConversationService.swift
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of awayMessageFor to verify they handle the new response format
rg -l "awayMessageFor\(" | while read -r file; do
echo "=== $file ==="
rg -A 5 "awayMessageFor\(" "$file"
done
# Search for usages of makeAwayMessageFrom to verify they handle the new response format
rg -l "makeAwayMessageFrom\(" | while read -r file; do
echo "=== $file ==="
rg -A 5 "makeAwayMessageFrom\(" "$file"
done
Length of output: 1891
Script:
#!/bin/bash
# Get more context around the usage in view controller
rg -B 2 -A 10 "awayMessageFor\(" "Sources/Kommunicate/Classes/KMConversationViewController.swift"
Length of output: 958
Sources/Kommunicate/Classes/KMConversationViewController.swift (3)
87-87: LGTM: New property tracks agent/bot online status.
The isAssignedAgentOrBotOnline property is well-named and appropriately scoped as private.
106-109: LGTM: Clear and focused UI update method.
The handleAwayUIUpdate method correctly guards against email collection mode and delegates to showAwayMessage.
91-98: Verify the state management in property observer.
The property observer for isAwayMessageViewHidden updates isAssignedAgentOrBotOnline and triggers UI updates. While the logic is sound, we should ensure this doesn't create unintended state changes.
Run the following script to check for potential state management issues:
✅ Verification successful
Let me gather more context about the usage and dependencies of these properties.
Property observer implementation is safe and consistent
The state management in the property observer is well-implemented and doesn't create unintended state changes. Here's why:
isAssignedAgentOrBotOnlineis only modified in this one location through theisAwayMessageViewHiddenproperty observerisAwayMessageViewHiddenchanges are properly guarded against redundant updates- The UI updates through
handleAwayUIUpdate()andhandleAwayUINoChange()are consistently triggered based on the state changes - All other modifications to
isAwayMessageViewHiddenare intentional state changes based on user actions or contact status updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other places where isAssignedAgentOrBotOnline is modified
# to ensure there are no conflicting updates
rg "isAssignedAgentOrBotOnline\s*=\s*" --type swift
Length of output: 181
Script:
#!/bin/bash
# Check for all references to both properties and their related UI handlers
rg "isAwayMessageViewHidden|isAssignedAgentOrBotOnline|handleAwayUI" --type swift -A 2
Length of output: 7012
| private let emailMessageLabel: UILabel = { | ||
| let label = UILabel() | ||
| label.font = UIFont(name: "HelveticaNeue-Light", size: 16) | ||
| label.textColor = .kmDynamicColor(light: UIColor(netHex: 0x676262), dark: .lightGray) | ||
| label.numberOfLines = 1 | ||
|
|
||
| let attachment = NSTextAttachment() | ||
| attachment.image = UIImage(named: "km_email_icon", in: Bundle.kommunicate, compatibleWith: nil)?.withRenderingMode(.alwaysTemplate) | ||
| attachment.bounds = CGRect(x: 0, y: -5, width: 30, height: 20) | ||
| let attachmentString = NSAttributedString(attachment: attachment) | ||
| let completeText = NSMutableAttributedString(string: "") | ||
| completeText.append(attachmentString) | ||
| completeText.append(NSAttributedString(string: " " + LocalizedText.CollectEmailMessageOnAwayMode)) | ||
| label.attributedText = completeText | ||
| label.textColor = .kmDynamicColor(light: UIColor(netHex: 0x676262), dark: .lightGray) | ||
| return label | ||
| }() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor email message label setup for better maintainability.
The email message label setup contains complex UI logic that could benefit from some improvements:
- Extract image attachment setup into a separate method
- Define constants for magic numbers
- Consider caching the attributed string if it doesn't change frequently
private let emailMessageLabel: UILabel = {
let label = UILabel()
label.font = UIFont(name: "HelveticaNeue-Light", size: 16)
label.textColor = .kmDynamicColor(light: UIColor(netHex: 0x676262), dark: .lightGray)
label.numberOfLines = 1
-
- let attachment = NSTextAttachment()
- attachment.image = UIImage(named: "km_email_icon", in: Bundle.kommunicate, compatibleWith: nil)?.withRenderingMode(.alwaysTemplate)
- attachment.bounds = CGRect(x: 0, y: -5, width: 30, height: 20)
- let attachmentString = NSAttributedString(attachment: attachment)
- let completeText = NSMutableAttributedString(string: "")
- completeText.append(attachmentString)
- completeText.append(NSAttributedString(string: " " + LocalizedText.CollectEmailMessageOnAwayMode))
- label.attributedText = completeText
+ label.attributedText = createEmailMessageAttributedText()
return label
}()
+private struct EmailIconMetrics {
+ static let offsetY: CGFloat = -5
+ static let width: CGFloat = 30
+ static let height: CGFloat = 20
+}
+
+private func createEmailMessageAttributedText() -> NSAttributedString {
+ let attachment = NSTextAttachment()
+ attachment.image = UIImage(named: "km_email_icon",
+ in: Bundle.kommunicate,
+ compatibleWith: nil)?.withRenderingMode(.alwaysTemplate)
+ attachment.bounds = CGRect(x: 0,
+ y: EmailIconMetrics.offsetY,
+ width: EmailIconMetrics.width,
+ height: EmailIconMetrics.height)
+ let completeText = NSMutableAttributedString()
+ completeText.append(NSAttributedString(attachment: attachment))
+ completeText.append(NSAttributedString(string: " " + LocalizedText.CollectEmailMessageOnAwayMode))
+ return completeText
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private let emailMessageLabel: UILabel = { | |
| let label = UILabel() | |
| label.font = UIFont(name: "HelveticaNeue-Light", size: 16) | |
| label.textColor = .kmDynamicColor(light: UIColor(netHex: 0x676262), dark: .lightGray) | |
| label.numberOfLines = 1 | |
| let attachment = NSTextAttachment() | |
| attachment.image = UIImage(named: "km_email_icon", in: Bundle.kommunicate, compatibleWith: nil)?.withRenderingMode(.alwaysTemplate) | |
| attachment.bounds = CGRect(x: 0, y: -5, width: 30, height: 20) | |
| let attachmentString = NSAttributedString(attachment: attachment) | |
| let completeText = NSMutableAttributedString(string: "") | |
| completeText.append(attachmentString) | |
| completeText.append(NSAttributedString(string: " " + LocalizedText.CollectEmailMessageOnAwayMode)) | |
| label.attributedText = completeText | |
| label.textColor = .kmDynamicColor(light: UIColor(netHex: 0x676262), dark: .lightGray) | |
| return label | |
| }() | |
| private let emailMessageLabel: UILabel = { | |
| let label = UILabel() | |
| label.font = UIFont(name: "HelveticaNeue-Light", size: 16) | |
| label.textColor = .kmDynamicColor(light: UIColor(netHex: 0x676262), dark: .lightGray) | |
| label.numberOfLines = 1 | |
| label.attributedText = createEmailMessageAttributedText() | |
| return label | |
| }() | |
| private struct EmailIconMetrics { | |
| static let offsetY: CGFloat = -5 | |
| static let width: CGFloat = 30 | |
| static let height: CGFloat = 20 | |
| } | |
| private func createEmailMessageAttributedText() -> NSAttributedString { | |
| let attachment = NSTextAttachment() | |
| attachment.image = UIImage(named: "km_email_icon", | |
| in: Bundle.kommunicate, | |
| compatibleWith: nil)?.withRenderingMode(.alwaysTemplate) | |
| attachment.bounds = CGRect(x: 0, | |
| y: EmailIconMetrics.offsetY, | |
| width: EmailIconMetrics.width, | |
| height: EmailIconMetrics.height) | |
| let completeText = NSMutableAttributedString() | |
| completeText.append(NSAttributedString(attachment: attachment)) | |
| completeText.append(NSAttributedString(string: " " + LocalizedText.CollectEmailMessageOnAwayMode)) | |
| return completeText | |
| } |
There was a problem hiding this comment.
Can't do the reactor as the given code require lazy var implementation for emailMessageLabel that can create some thread related issue as well as increase memory use.
There was a problem hiding this comment.
@AbhijeetRanjan308, understood. Thank you for explaining the concerns. Avoiding the refactor makes sense if it would introduce threading issues and increase memory usage.
✏️ Learnings added
Learnt from: AbhijeetRanjan308
PR: Kommunicate-io/Kommunicate-iOS-SDK#452
File: Sources/Kommunicate/Classes/Views/AwayMessageView.swift:52-68
Timestamp: 2024-11-18T09:24:38.559Z
Learning: Changing `emailMessageLabel` to a `lazy var` in `AwayMessageView.swift` can introduce threading issues and increase memory usage. Avoid suggesting such refactors in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| func switchToEmailUI(emailUIEnabled: Bool) { | ||
| messageLabel.isHidden = emailUIEnabled | ||
| emailMessageLabel.isHidden = !emailUIEnabled | ||
| } | ||
|
|
||
| func showInvalidEmailError() { | ||
| let attachment = NSTextAttachment() | ||
| attachment.image = UIImage(named: "km_email_icon", in: Bundle.kommunicate, compatibleWith: nil)?.withRenderingMode(.alwaysTemplate) | ||
| attachment.bounds = CGRect(x: 0, y: -5, width: 30, height: 20) | ||
| let attachmentString = NSAttributedString(attachment: attachment) | ||
| let completeText = NSMutableAttributedString(string: "") | ||
| completeText.append(attachmentString) | ||
| completeText.append(NSAttributedString(string: " " + LocalizedText.InvalidEmailMessageOnAwayMode)) | ||
| emailMessageLabel.attributedText = completeText | ||
| emailMessageLabel.textColor = .red | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in email error handling.
The showInvalidEmailError method duplicates the image attachment logic from the label setup. Consider reusing the attachment creation logic.
+private func createEmailIconAttachment() -> NSTextAttachment {
+ let attachment = NSTextAttachment()
+ attachment.image = UIImage(named: "km_email_icon",
+ in: Bundle.kommunicate,
+ compatibleWith: nil)?.withRenderingMode(.alwaysTemplate)
+ attachment.bounds = CGRect(x: 0,
+ y: EmailIconMetrics.offsetY,
+ width: EmailIconMetrics.width,
+ height: EmailIconMetrics.height)
+ return attachment
+}
func showInvalidEmailError() {
- let attachment = NSTextAttachment()
- attachment.image = UIImage(named: "km_email_icon", in: Bundle.kommunicate, compatibleWith: nil)?.withRenderingMode(.alwaysTemplate)
- attachment.bounds = CGRect(x: 0, y: -5, width: 30, height: 20)
+ let attachment = createEmailIconAttachment()
let attachmentString = NSAttributedString(attachment: attachment)
let completeText = NSMutableAttributedString(string: "")
completeText.append(attachmentString)
completeText.append(NSAttributedString(string: " " + LocalizedText.InvalidEmailMessageOnAwayMode))
emailMessageLabel.attributedText = completeText
emailMessageLabel.textColor = .red
}Committable suggestion skipped: line range outside the PR's diff.
Summary
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores