-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature][Calling] CSS call history #710
[Feature][Calling] CSS call history #710
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a first pass, mostly just on the db stuff. It mostly looks fine, but see comments around specifics.
In summary:
- better lifecycle of the db connection (don't throw away the opened connection)
- simplify schema (remove
id
) - simplify cleanup logic, make it part of an insert logic as a single DELETE in a transaction; won't need a
remove
method then - switch to just using Long for start timestamp. it's more correct, and you won't need to do all that time parsing stuff and won't need a threetenbp library include (or desugaring java8+ features)
- I'm fairly sure you're doing all the db operations on the main dispatcher (due to using lifecycleScope), which is not what you want. Use a dedicated single-threaded executor instead (since sqlite is inherently single-threaded and won't like it if we write simultaneously on different threads).
...src/main/java/com/azure/android/communication/ui/calling/data/model/CallHistoryRecordData.kt
Outdated
Show resolved
Hide resolved
...ication-ui/calling/src/main/java/com/azure/android/communication/ui/calling/data/DbHelper.kt
Outdated
Show resolved
Hide resolved
...lling/src/main/java/com/azure/android/communication/ui/calling/data/CallHistoryRepository.kt
Outdated
Show resolved
Hide resolved
...lling/src/main/java/com/azure/android/communication/ui/calling/data/CallHistoryRepository.kt
Outdated
Show resolved
Hide resolved
...lling/src/main/java/com/azure/android/communication/ui/calling/data/CallHistoryRepository.kt
Outdated
Show resolved
Hide resolved
...lling/src/main/java/com/azure/android/communication/ui/calling/data/CallHistoryRepository.kt
Outdated
Show resolved
Hide resolved
...lling/src/main/java/com/azure/android/communication/ui/calling/data/CallHistoryRepository.kt
Outdated
Show resolved
Hide resolved
...ication-ui/calling/src/main/java/com/azure/android/communication/ui/calling/data/DbHelper.kt
Outdated
Show resolved
Hide resolved
...ication-ui/calling/src/main/java/com/azure/android/communication/ui/calling/data/DbHelper.kt
Outdated
Show resolved
Hide resolved
.../calling/src/main/java/com/azure/android/communication/ui/calling/CallCompositeExtentions.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed in person.
...c/main/java/com/azure/android/communication/ui/calling/presentation/CallCompositeActivity.kt
Show resolved
Hide resolved
...ication-ui/calling/src/main/java/com/azure/android/communication/ui/calling/data/DbHelper.kt
Outdated
Show resolved
Hide resolved
...lling/src/main/java/com/azure/android/communication/ui/calling/data/CallHistoryRepository.kt
Outdated
Show resolved
Hide resolved
...lling/src/main/java/com/azure/android/communication/ui/calling/data/CallHistoryRepository.kt
Show resolved
Hide resolved
...lling/src/main/java/com/azure/android/communication/ui/calling/data/CallHistoryRepository.kt
Outdated
Show resolved
Hide resolved
...lling/src/main/java/com/azure/android/communication/ui/calling/data/CallHistoryRepository.kt
Outdated
Show resolved
Hide resolved
...i/calling/src/test/java/com/azure/android/communication/ui/service/CallHistoryServiceTest.kt
Show resolved
Hide resolved
...ain/java/com/azure/android/communication/ui/calling/presentation/manager/DebugInfoManager.kt
Show resolved
Hide resolved
...calling/src/main/java/com/azure/android/communication/ui/calling/redux/state/CallingState.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r+ if you address comments on the CallHistoryRepository.
...lling/src/main/java/com/azure/android/communication/ui/calling/data/CallHistoryRepository.kt
Show resolved
Hide resolved
...lling/src/main/java/com/azure/android/communication/ui/calling/data/CallHistoryRepository.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I focused on the repository/etc parts in the review. The rest looks okay at a glance.
Purpose
Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
Pull Request Checklist
How to Test
What to Check
Verify that the following are valid
Other Information