CMM-933: Don't allow to reply support closed tickets #22342
Conversation
|
Claude finished @adalpari's task —— View job Code Review Complete ✅I've completed a comprehensive review of this PR that prevents users from replying to closed support tickets and adds visual status indicators. Overall, the implementation is well-structured and follows good practices. Here are my findings: ✅ Strengths
🔍 Issues & Recommendations1. Status String Handling (Medium Priority)Location: Issue: The status comparison uses hardcoded strings in different formats:
Recommendation: Consider centralizing status constants to avoid inconsistencies: object SupportConversationStatus {
const val OPEN = "open"
const val PENDING = "pending"
const val ON_HOLD = "on-hold"
const val SOLVED = "solved"
const val CLOSED = "closed"
}This would make the code more maintainable and prevent potential bugs if the API changes status formats. 2. Missing "New" Status Handling (Low Priority)Location: Issue: The screenshots show a "new" status badge, but the Recommendation: Add an explicit case for "new" status if it's a valid state, or document why it's intentionally using default colors. 3. Potential Line Length Violation (Low Priority)Location: Issue: According to the CLAUDE.md file, the maximum line length is 120 characters. While most lines appear compliant, some lines with string concatenation might be close to the limit. Recommendation: Run 4. Accessibility Consideration (Low Priority)Location: Issue: The status badge doesn't have explicit content description for accessibility, relying on the text being read automatically. Recommendation: Consider adding a semantic contentDescription to the status badge for better screen reader support, especially since color is used to convey meaning. 📋 Code Quality Checklist
🎯 Testing RecommendationsBefore merging, please verify:
📝 Minor Observations
✅ SummaryThis is a solid implementation that achieves its goals effectively. The main suggestions are minor improvements around status constant management and ensuring comprehensive status handling. The code is production-ready with the current implementation, and the suggested improvements are optional enhancements for long-term maintainability. Recommendation: ✅ Approve with minor suggestions - The PR can be merged as-is, with the suggestions considered for future improvements. |
Generated by 🚫 Danger |
|
| App Name | Jetpack | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22342-ecb21de | |
| Commit | ecb21de | |
| Direct Download | jetpack-prototype-build-pr22342-ecb21de.apk |
|
| App Name | WordPress | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22342-ecb21de | |
| Commit | ecb21de | |
| Direct Download | wordpress-prototype-build-pr22342-ecb21de.apk |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22342 +/- ##
==========================================
- Coverage 39.12% 39.10% -0.03%
==========================================
Files 2205 2207 +2
Lines 106157 106231 +74
Branches 15049 15054 +5
==========================================
+ Hits 41536 41539 +3
- Misses 61129 61200 +71
Partials 3492 3492 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jkmassel
left a comment
jkmassel
left a comment
There was a problem hiding this comment.
We could probably expose the status in the header of a detail view instead of the message count.
I noted one other issue inline.
Otherwise, this works well.
| @@ -0,0 +1,10 @@ | |||
| package org.wordpress.android.support.he.model | |||
|
|
|||
| object ConversationStatus { | |||
There was a problem hiding this comment.
These status values are more for the support person than for the user – we probably shouldn't expose them directly.
On iOS we have "waiting for support","waiting for user", "solved" and "closed" – we can probably mirror that here?
…osed-tickets # Conflicts: # WordPress/src/main/java/org/wordpress/android/support/he/ui/HEConversationDetailScreen.kt # WordPress/src/main/res/values/strings.xml
|
Description
This PR is preventing the user from answering already closed tickets in support. In addition. we are adding a visual indicator to show the ticket status. At first we started just showing the "Closed" one, but felt odd to just show the status for that state, so we went ahead with all of them.
Testing instructions
@jeremy, sin I assume you already have open and closd tickets, I'm targeting you directly here :)
MODERN_SUPPORTFF