MS-496: Pay button text

[vscmaster]

https://plexteq-ottu.atlassian.net/browse/MS-496 Second part

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[vscmaster]

@claude review

[claude]

Claude finished @vscmaster's task in 1m 49s —— View job

---

Code Review Complete

• Read and analyze changed files
• Review against CLAUDE.md security criteria
• Check for CHD exposure risks
• Verify channel security patterns
• Review logging practices
• Check Sample app patterns
• Provide comprehensive feedback

---

Summary

This PR adds a PayButtonText feature allowing merchants to customize the payment button text in English and Arabic. The implementation adds a new optional field across Dart/Android/iOS layers.

Security Assessment: ✅ No critical issues – This change does not introduce CHD exposure or violate PCI boundaries.

Code Quality: ⚠️ Pre-existing issues remain – The PR does not fix any of the significant logging/security issues documented in CLAUDE.md.

---

Detailed Findings

✅ Security Analysis (No Blockers)

A) Boundary Proof: "Wrapper Only"

• PASS: PayButtonText contains only UI strings (en/ar localization), no CHD
• PASS: Field is optional, follows existing patterns
• PASS: Data flows from Dart → Native SDKs via JSON in creationParams

B) Platform Channels

• NO NEW RISKS: PayButtonText is serialized/deserialized like other config fields
• PRE-EXISTING ISSUE (unchanged by this PR):
  • apiKey still in creationParams payload (checkout_arguments.dart:9, checkout_widget.dart:33-35)
  • No error handling on channel calls – unhandled PlatformException would propagate apiKey

C) Flutter Logging / Error Handling

• NO NEW RISKS: PayButtonText not logged in new code
• PRE-EXISTING ISSUES (unchanged by this PR):
  • SDK: print("OttuCheckoutWidget.dispose()") in checkout_widget.dart:26
  • Android: 27 Log.* calls with NO debug/release guard in CheckoutView.kt
  • iOS: New Logger.swift added but uses OSLog (appropriate for iOS)
  • Sample app: LogInterceptor(responseBody: true, requestBody: true) always active

---

⚠️ iOS Code Refactoring

The PR includes a massive 429-line refactoring of CheckoutPlatformView.swift (+256/-173 lines). This is NOT documented in the PR description ("MS-496: Pay button text - Second part").

Concerns:

1. Scope creep: The refactoring is not mentioned in the Jira ticket reference
2. Review difficulty: Large-scale formatting/structural changes mixed with feature addition
3. Risk: Harder to identify if bugs were introduced in the refactoring vs the feature

Recommendation:

When making large refactorings, separate them into distinct commits or PRs from feature additions. This makes review safer and rollback easier if issues arise.

---

📝 Code Quality Issues (Non-Blocking)

1. Commented-Out Code

File: lib/src/pay_button_text.dart:19-26

/*
  extention Language on PayButtonText {
  forLanguage(String language) =
switch (language) {
case "en" : en;
case "ar" : ar;
}
}*/

• Typo: "extention" → "extension"
• Dead code should be removed, not committed
• If needed for future reference, use git history

Impact: Low – just clutter, but reduces code cleanliness

---

2. Sample App Hardcoded Test Values

File: Sample/lib/feature/home/home_screen_cubit.dart:260-262

payButtonText: state.useCustomText == true
    ? PayButtonText(en: "Checkout", ar: "الدفع")
    : null,

Issue: Hardcoded test strings for the custom button text feature.

Recommendation: Either:

• Add text input fields in the Sample UI for custom button text (better demo)
• Use more obvious placeholder text like "Custom EN Text"/"نص مخصص" to make it clear these are test values

Impact: Low – Sample app only, but could be misleading for merchants copying this code

---

Pre-Existing Security Issues (Reminder)

These issues exist in the codebase but are NOT introduced by this PR:

Critical (from CLAUDE.md):

1. Android logging: 27 Log.* calls with NO debug/release guards – logs may include apiKey/sessionId
2. Sample app teaches insecure patterns:
   • LogInterceptor with responseBody: true, requestBody: true always active
   • API key explicitly logged at home_screen_cubit.dart:97
   • android:usesCleartextTraffic="true" in Sample manifest
3. No test coverage: Zero tests exist for any layer
4. No static scanning: No CI checks for PAN-like patterns or sensitive field leakage

---

Recommendations

For This PR:

1. ✅ Approve – The PayButtonText feature is safe and follows existing patterns
2. 🔧 Remove commented code at pay_button_text.dart:19-26
3. 📖 Document the iOS refactoring in commit messages or PR description

For Future Work:

1. Address logging risks documented in CLAUDE.md (especially Android Log.* calls)
2. Add test coverage for channel serialization/deserialization
3. Separate refactorings from feature PRs for cleaner review
4. Consider adding CI lint rules to prevent print/debug logging in SDK code

---

Verdict: ✅ Safe to merge with minor cleanup recommended