Fix #1106: Addition of Work Manager for uploading logs #1680
Conversation
# Conflicts: # app/src/main/java/org/oppia/app/application/ApplicationComponent.kt
domain/src/main/java/org/oppia/domain/oppialogger/OppiaLogUploadWorker.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/domain/oppialogger/OppiaLogUploadWorker.kt
Outdated
Show resolved
Hide resolved
| private const val OPPIA_EXCEPTION_WORK = "OPPIA_EXCEPTION_WORK_REQUEST" | ||
| private const val OPPIA_EVENT_WORK = "OPPIA_EVENT_WORK_REQUEST" | ||
|
|
||
| class OppiaLogUploader() { |
There was a problem hiding this comment.
I am a bit unsure what the purpose of this class is. This is not an uploader since its not doing any uploading. It looks like its only creating the worker requests right? Can we rename this class accordingly?
There was a problem hiding this comment.
Aah, I named it so because the work requests are after all the ones that will start the process of uploading. Changed it to OppiaLogUploadWorkRequest, is that fine now ?
There was a problem hiding this comment.
Keeping this open, for further communication in future.
# Conflicts: # app/src/main/java/org/oppia/app/application/OppiaApplication.kt
# Conflicts: # app/src/main/java/org/oppia/app/application/OppiaApplication.kt
There was a problem hiding this comment.
Thanks @Sarthak2601! PTAL at my comments & reply before I approve. Also, I think you'll need to make Bazel changes and that will require my approval per code owners.
domain/src/main/java/org/oppia/domain/oppialogger/analytics/AnalyticsController.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/domain/oppialogger/exceptions/ExceptionsController.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/domain/oppialogger/loguploader/LogUploaderModule.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/domain/oppialogger/loguploader/LogUploadWorker.kt
Show resolved
Hide resolved
| ): LogUploadChildWorkerFactory | ||
|
|
||
| @Binds | ||
| fun bindWorkerFactory(logUploadWorkerFactory: LogUploadWorkerFactory): WorkerFactory |
There was a problem hiding this comment.
I understand why the factory is needed, but I don't quite understand why we can't inject LogUploadWorkerFactory where we're currently injecting WorkerFactory (though I don't actually see this injected anywhere--are you sure this binding is being used?)
domain/src/main/java/org/oppia/domain/oppialogger/loguploader/LogUploadChildWorkerFactory.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/domain/oppialogger/loguploader/LogUploadWorkerTest.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| fun testWorker_logException_withoutNetwork_enqueueRequest_verifySuccess() { |
There was a problem hiding this comment.
To be clear: I'm actually suggesting that we verify that we don't enqueue the request if we log with internet connectivity on (since it should just log as normal). Am I maybe misunderstanding why we shouldn't do that?
|
@BenHenning for the code owners, I've added artifacts in Bazel and it's building perfectly. PTAL. Also, I used this article for work manager - dagger integration. |
|
@Sarthak2601 left 1 new comment & replies to most existing threads. PTAL. |
There was a problem hiding this comment.
Approving to unblock, but submission is conditioned on my comment threads being resolved. @rt4914 & @anandwana001 please verify these.
be initialized (these tests shouldn't use OppiaApplication anymore). Includes some fixes for various tests that may have had flakiness/syncing issues before, and removed one test activity that wasn't actually used. One test was also ignored because it isn't testing the correct thing and it fails when the correct assertion is added, so follow-up work is needed.
Explanation
Fixes #1106 - Implements work manager for uploading events and exceptions to Firebase in presence of network connection.
Checklist