CMM-885 support HE attachments

[adalpari]

Description

This PR is adding support to upload attachments inside HE tickes in the new support flow.

Showing the attachments as well, turned out to create a big PR, so the showing part has been moved to #22333
If you want to give a try to both PRs, you can test them consecutively.

Things to discuss/improve:

• Limit the number of attachments that can be uploaded

Testing instructions

1. Log in with a WP.COM account and enable MODERN_SUPPORT
2. Create a support conversation with HE and include attachments (NOTE: heavy videos are failing)
3. Open the conversation and include more attachments
4. Open that conversation in Zendesk (search by the ticket number you will get in your email) and check you can see the attachments. (Alternatively, if you are testing the second iteration PR, you can check them in the mobile app as well)

NOTE: don0t forget to mark your ticket as solved in Zendesk

Screen_recording_20251103_113859.mp4

[claude]

Claude finished @adalpari's task —— View job

---

Code Review for PR #22323 ✅

I've completed a comprehensive review of the attachment support implementation for HE tickets. Here are my findings:

---

✅ Positive Aspects

1. Good Architecture: The attachment state management in ViewModel is well-designed with proper StateFlow usage
2. Clean UI Components: The fullscreen image preview with zoom/pan gestures is well-implemented
3. User Experience: Draft message persistence in the bottom sheet is a nice touch
4. Proper Cleanup: Temporary files are cleaned up after upload/failure
5. Good Test Coverage: The ViewModel tests have been updated to include attachment scenarios

---

🔴 Critical Issues

1. Resource Leak in Activity (HESupportActivity.kt:54)

The photoPickerLauncher callback doesn't handle errors when addAttachments fails. If the ViewModel throws an exception or the URI is invalid, the user gets no feedback.

Impact: Silent failures, poor UX

Recommendation:

photoPickerLauncher = registerForActivityResult(...) { result ->
    if (result.resultCode == RESULT_OK && result.data != null) {
        val uris = result.data?.getStringArrayExtra(MediaPickerConstants.EXTRA_MEDIA_URIS)
        uris?.let { uriStrings ->
            try {
                val newUris = uriStrings.map { it.toUri() }
                viewModel.addAttachments(newUris)
            } catch (e: Exception) {
                // Show error snackbar
            }
        }
    }
}

2. Exception Handling Too Broad (HESupportViewModel.kt:189-209)

The Uri.toTempFile() extension function uses @Suppress("TooGenericExceptionCaught", "TooGenericExceptionThrown") and throws a generic Exception:

?: throw Exception("Failed to open input stream for attachment")

Issues:

• Violates the project's code quality guidelines (see CLAUDE.md: "No Deprecated APIs: Avoid using deprecated methods")
• Suppresses Detekt warnings rather than addressing them
• Throws generic Exception instead of specific exception types
• Makes debugging difficult

Recommendation:

// Create specific exception classes
class AttachmentException(message: String, cause: Throwable? = null) : Exception(message, cause)

private suspend fun Uri.toTempFile(): File = withContext(ioDispatcher) {
    try {
        val inputStream = application.contentResolver.openInputStream(this@toTempFile)
            ?: throw AttachmentException("Cannot access file: permission denied or file not found")
        // ... rest of implementation
    } catch (e: SecurityException) {
        throw AttachmentException("Permission denied to access file", e)
    } catch (e: IOException) {
        throw AttachmentException("Failed to copy file: ${e.message}", e)
    }
}

3. Missing Input Validation (HESupportRepository.kt:96-136)

The repository accepts attachments: List<String> without validating:

• File paths exist
• Files are readable
• File sizes are reasonable
• File types are allowed

Impact: Server errors, poor error messages, potential security issues

Recommendation: Add validation in the repository layer before making API calls

---

⚠️ High Priority Issues

4. Incomplete Test Coverage (HESupportViewModelTest.kt)

The test file signature shows tests pass attachments but they're all emptyList(). The new attachment functionality isn't tested:

• addAttachments() - not tested
• removeAttachment() - not tested
• clearAttachments() - not tested
• copyUrisToTempFiles() - not tested
• Uri.toTempFile() - not tested
• Error scenarios during file copy - not tested

Recommendation: Add comprehensive tests for the attachment functionality:

@Test
fun `addAttachments adds URIs to attachments list`() = test {
    val uri1 = Uri.parse("content://test/1")
    val uri2 = Uri.parse("content://test/2")
    
    viewModel.addAttachments(listOf(uri1, uri2))
    
    assertThat(viewModel.attachments.value).containsExactly(uri1, uri2)
}

@Test
fun `removeAttachment removes specific URI`() = test {
    val uri1 = Uri.parse("content://test/1")
    val uri2 = Uri.parse("content://test/2")
    viewModel.addAttachments(listOf(uri1, uri2))
    
    viewModel.removeAttachment(uri1)
    
    assertThat(viewModel.attachments.value).containsExactly(uri2)
}

// Add more tests...

5. No Attachment Limit Enforcement (Activity & ViewModel)

The PR description mentions "Limit the number of attachments that can be uploaded" as a discussion point, and a previous review comment about a hardcoded limit was addressed by removal. However, there's now no limit at all.

Issues:

• Server may have limits that aren't enforced client-side
• Memory issues with large attachments
• Poor UX if server rejects due to too many attachments

Recommendation:

1. Add a constant for max attachments (check with backend team for limits)
2. Enforce in ViewModel when adding attachments
3. Show appropriate UI feedback when limit is reached

6. File Extension Detection Fallback (HESupportViewModel.kt:232)

// Default to jpg if we can't determine the extension
return "jpg"

Issues:

• Silently assumes JPG for all unknown file types
• Could cause issues with PDFs, videos, or other file types
• Server might reject incorrectly labeled files

Recommendation: Either reject unknown file types or use a generic extension like "bin"

---

📝 Medium Priority Issues

7. Accessibility Improvements Needed

• AttachmentFullscreenImagePreview.kt:94: Image description is generic "Attachment image" - should include filename
• HEConversationDetailScreen.kt:414: No content description for attachment filenames

8. Missing Documentation

Functions like copyUrisToTempFiles(), removeTempFiles(), and Uri.toTempFile() lack KDoc comments explaining:

• What they do
• When they're called
• What exceptions they throw
• Thread safety considerations

9. Hardcoded String (HESupportRepository.kt:25)

private const val APPLICATION_ID = "jetpack"

This should be configurable or determined from build variant, as the repository builds both WordPress and Jetpack apps.

---

💡 Low Priority / Suggestions

10. Consider Using Sealed Class for File Operations
    Similar to MessageSendResult, file operations could return a sealed class result instead of throwing exceptions.

11. Performance: Large images loaded directly into memory could cause OOM on older devices. Consider adding:

    • File size checks before upload
    • Image compression for large images
    • Progress indicators for large uploads

12. UX Enhancement: When attachment download starts (HESupportActivity.kt:206-219), there's no indication when download completes/fails

---

📋 Summary

Must Fix Before Merge:

• ❌ Fix exception handling (Issue Implementing WordPress.com Notifications Activities #2) - creates technical debt
• ❌ Add attachment functionality tests (Issue Fix for bug #403 #4) - critical for maintaining quality
• ❌ Add input validation (Issue WordPress.com Notifications in Android #3) - prevents runtime errors

Should Fix Before Merge:

• ⚠️ Add attachment limit enforcement (Issue Fix for bug #403 (new patch) #5)
• ⚠️ Improve file extension detection (Issue Select first row of notifications after refresh on tablet #6)
• ⚠️ Add error handling in Activity (Issue Menu Drawer Item refactor #1)

Can Address in Follow-up:

• Documentation improvements (Issue Added Swipe feature in Posts Activity #8)
• Accessibility enhancements (Issue Funky chars around comment reply #7)
• Performance optimizations (Issue Images that aren't able to load show the WordPress logo placeholder #11)

---

🎯 Testing Recommendations

Before merging, ensure testing covers:

1. ✅ Uploading images in new ticket creation (from testing instructions)
2. ✅ Uploading images in conversation replies (from testing instructions)
3. ✅ Viewing image attachments from Zendesk (from testing instructions)
4. ⚠️ Large file handling (>10MB)
5. ⚠️ Multiple file types (PDF, PNG, JPG)
6. ⚠️ Permissions denied scenarios
7. ⚠️ Network failure during upload
8. ⚠️ File no longer exists when uploading
9. ⚠️ Low storage space scenarios

---

Overall, this is a solid implementation with good architectural decisions. The main concerns are around error handling robustness and test coverage for the new attachment functionality. Once the critical issues are addressed, this will be ready to merge.

---

[adalpari]

3. Missing Input Validation (HESupportRepository.kt:96-136)

The repository accepts attachments: List<String> without validating:

• File paths exist
• Files are readable
• File sizes are reasonable
• File types are allowed

We are just failing if the file is not supported. We can malke the error handling more fine grained in next iterations, but it's out fo scope for this one.

[adalpari]

4. Incomplete Test Coverage (HESupportViewModelTest.kt)

It's coming

[adalpari]

5. No Attachment Limit Enforcement (Activity & ViewModel)

Totally right, we need to set a max number of attachments.

[codecov]

Codecov Report

❌ Patch coverage is 12.73063% with 473 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.21%. Comparing base (7f795cb) to head (3a4c1ab).
⚠️ Report is 1 commits behind head on trunk.

Files with missing lines	Patch %	Lines
.../support/he/ui/AttachmentFullscreenImagePreview.kt	0.00%	108 Missing ⚠️
...ndroid/support/he/ui/HEConversationDetailScreen.kt	0.00%	100 Missing ⚠️
...id/support/he/ui/HEConversationReplyBottomSheet.kt	0.00%	69 Missing ⚠️
...ess/android/support/he/ui/TicketMainContentView.kt	0.00%	69 Missing ⚠️
...ess/android/support/he/util/TempAttachmentsUtil.kt	0.00%	39 Missing ⚠️
...ess/android/support/he/util/HEConversationUtils.kt	0.00%	32 Missing ⚠️
...rdpress/android/support/he/ui/HENewTicketScreen.kt	0.00%	24 Missing ⚠️
...droid/support/he/repository/HESupportRepository.kt	21.73%	18 Missing ⚠️
...wordpress/android/support/he/ui/SupportCategory.kt	0.00%	7 Missing ⚠️
...rdpress/android/support/he/model/SupportMessage.kt	25.00%	6 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #22323      +/-   ##
==========================================
- Coverage   39.33%   39.21%   -0.12%     
==========================================
  Files        2198     2201       +3     
  Lines      105412   105788     +376     
  Branches    14984    15009      +25     
==========================================
+ Hits        41462    41485      +23     
- Misses      60458    60811     +353     
  Partials     3492     3492              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jkmassel]

Generally looks good – I don't see anything concerning so I'm approving this as part of #22333

[sonarqubecloud]

Please retry analysis of this Pull-Request directly on SonarQube Cloud