Retrofit Migration - #25
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe app migrates backend communication from direct OkHttp and Gson handling to Retrofit with Kotlin serialization. Models now expose camelCase properties while preserving existing snake_case JSON fields. Repositories, notifications, session handling, and UI adapters use the updated models. ChangesNetworking and serialization migration
Merge Risk: ⚪ Minimal · up to This PR migrates the networking implementation to Retrofit and updates related parameter names; no actionable merge-blocking risk remains based on the supplied evidence. Sequence Diagram(s)sequenceDiagram
participant App
participant CourseGrabRepository
participant CourseGrabService
participant Retrofit
participant Backend
App->>CourseGrabRepository: request session, course, tracking, or notification operation
CourseGrabRepository->>CourseGrabService: call typed suspend endpoint
CourseGrabService->>Retrofit: serialize request and create HTTP call
Retrofit->>Backend: send authenticated request
Backend-->>Retrofit: return serialized ApiResponse
Retrofit-->>CourseGrabService: deserialize response with Json
CourseGrabService-->>CourseGrabRepository: return ApiResponse
CourseGrabRepository-->>App: return Result
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description includes an overview, emulator test coverage, and next steps. It omits the required Changes Made section and does not provide implementation details for the Retrofit migration or parameter-name fixes. Resolution Add a Changes Made section that describes the Retrofit service, serialization, dependency, networking-module, repository, and model updates. Keep the emulator test details and state whether Related PRs or Screenshots & Videos are not applicable by deleting those sections if appropriate. Full details: Docstring CoverageExplanation Docstring coverage is 6.06% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 33 functions across 16 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
85d26e9 to
b82da4d
Compare
| ) No newline at end of file | ||
| @SerialName("session_token") val sessionToken: String? = null, | ||
| @SerialName("update_token") val updateToken: String? = null, | ||
| @SerialName("session_expiration") val sessionExpiration: Long? = null |
There was a problem hiding this comment.
Does backend send a JSON number instead of numeric string now?
| } | ||
|
|
||
| private companion object { | ||
| const val TAG = "NotificationService" |
There was a problem hiding this comment.
nit: I'm pretty sure this is ok but curious - do we expect another TAG value other than "NotificationService"? I'm wondering what the difference between this v.s. just passing in the value directly to the log on line 46.
conniecliu
left a comment
There was a problem hiding this comment.
Aside from the two comments I think everything else lgtm!
Overview
Migrated to Retrofit and fixed parameter names.
Test Coverage
Next Steps
Summary by CodeRabbit
New Features
Bug Fixes