Skip to content

Commit 190557f

Browse files
hermannakosclaude
hermannakos
and
claude
authored
[MBL-19275][Android] Fix file attachment duplicate uploads and infinite loading (#3295)
Fix race condition causing duplicate file uploads and infinite loading indicators in SpeedGrader comments by passing file paths atomically with upload events. refs: MBL-19275 affects: Teacher release note: Fixed an issue where file attachments in SpeedGrader comments could be uploaded multiple times and get stuck in an infinite loading state test plan: - Unit tests: All 20 tests pass including 5 new tests for file upload flows - Manual testing: Build and run Teacher app on emulator - Verify file uploads in SpeedGrader comments work correctly without duplicates 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <[email protected]>
1 parent c7f5da0 commit 190557f

File tree

5 files changed

+224
-20
lines changed

5 files changed

+224
-20
lines changed

‎libs/pandautils/src/main/java/com/instructure/pandautils/features/file/upload/FileUploadDialogFragment.kt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,8 @@ class FileUploadDialogFragment : BaseCanvasDialogFragment() {
256256
fileUploadEventHandler.postEvent(
257257
FileUploadEvent.UploadStarted(
258258
action.id,
259-
action.liveData
259+
action.liveData,
260+
action.selectedUris
260261
)
261262
)
262263
}

‎libs/pandautils/src/main/java/com/instructure/pandautils/features/file/upload/FileUploadEventHandler.kt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@ sealed class FileUploadEvent {
2929
data class FileSelected(val filePaths: List<String>) : FileUploadEvent()
3030
data class UploadStarted(
3131
val uuid: UUID?,
32-
val workInfoLiveData: LiveData<WorkInfo>
32+
val workInfoLiveData: LiveData<WorkInfo>,
33+
val filePaths: List<String>
3334
) : FileUploadEvent()
3435
}
3536

‎libs/pandautils/src/main/java/com/instructure/pandautils/features/speedgrader/grade/comments/SpeedGraderCommentsSection.kt‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -208,16 +208,20 @@ fun SpeedGraderCommentsSection(
208208
if (state.fileSelectorDialogData != null) {
209209
val fragmentManager = LocalContext.current.getFragmentActivity().supportFragmentManager
210210

211-
val bundle = FileUploadDialogFragment.createTeacherSubmissionCommentBundle(
212-
state.fileSelectorDialogData.courseId,
213-
state.fileSelectorDialogData.assignmentId,
214-
state.fileSelectorDialogData.userId,
215-
state.fileSelectorDialogData.attempt
216-
)
211+
// Check if dialog is already showing to prevent duplicates
212+
val existingDialog = fragmentManager.findFragmentByTag(FileUploadDialogFragment.TAG)
213+
if (existingDialog == null) {
214+
val bundle = FileUploadDialogFragment.createTeacherSubmissionCommentBundle(
215+
state.fileSelectorDialogData.courseId,
216+
state.fileSelectorDialogData.assignmentId,
217+
state.fileSelectorDialogData.userId,
218+
state.fileSelectorDialogData.attempt
219+
)
217220

218-
FileUploadDialogFragment.newInstance(bundle).show(
219-
fragmentManager, FileUploadDialogFragment.TAG + UUID.randomUUID()
220-
)
221+
FileUploadDialogFragment.newInstance(bundle).show(
222+
fragmentManager, FileUploadDialogFragment.TAG
223+
)
224+
}
221225
}
222226
}
223227
}

‎libs/pandautils/src/main/java/com/instructure/pandautils/features/speedgrader/grade/comments/SpeedGraderCommentsViewModel.kt‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ class SpeedGraderCommentsViewModel @Inject constructor(
206206
fileUploadEventHandler.events.collect { event ->
207207
when (event) {
208208
is FileUploadEvent.UploadStarted -> {
209-
onFileUploadStarted(event.workInfoLiveData)
209+
onFileUploadStarted(event.workInfoLiveData, event.filePaths)
210210
}
211211
is FileUploadEvent.FileSelected -> {
212212
selectedFilePaths = event.filePaths
@@ -347,7 +347,7 @@ class SpeedGraderCommentsViewModel @Inject constructor(
347347
}
348348

349349
is SpeedGraderCommentsAction.FileUploadStarted -> {
350-
onFileUploadStarted(action.workInfoLiveData)
350+
onFileUploadStarted(action.workInfoLiveData, selectedFilePaths.orEmpty())
351351
}
352352

353353
is SpeedGraderCommentsAction.FilesSelected -> {
@@ -356,7 +356,7 @@ class SpeedGraderCommentsViewModel @Inject constructor(
356356
}
357357
}
358358

359-
private fun onFileUploadStarted(workInfoLiveData: LiveData<WorkInfo>) {
359+
private fun onFileUploadStarted(workInfoLiveData: LiveData<WorkInfo>, filePaths: List<String>) {
360360
_uiState.update { state ->
361361
state.copy(
362362
fileSelectorDialogData = null,
@@ -366,8 +366,8 @@ class SpeedGraderCommentsViewModel @Inject constructor(
366366
// Subscribe to the worker's LiveData to observe its state
367367
viewModelScope.launch {
368368
workInfoLiveData.asFlow().collect { workInfo ->
369-
when (workInfo.state) {
370-
WorkInfo.State.RUNNING -> createPendingFileComment(workInfo)
369+
when (workInfo?.state) {
370+
WorkInfo.State.RUNNING -> createPendingFileComment(workInfo, filePaths)
371371
WorkInfo.State.SUCCEEDED -> handleFileUploadSuccess(workInfo)
372372
WorkInfo.State.FAILED -> handleFileUploadFailure(workInfo)
373373
else -> {}
@@ -376,15 +376,15 @@ class SpeedGraderCommentsViewModel @Inject constructor(
376376
}
377377
}
378378

379-
private suspend fun createPendingFileComment(workInfo: WorkInfo) {
379+
private suspend fun createPendingFileComment(workInfo: WorkInfo, filePaths: List<String>) {
380380
var fileUploadInput = fileUploadInputDao.findByWorkerId(workInfo.id.toString())
381381
if (fileUploadInput == null) {
382382
fileUploadInput = FileUploadInputEntity(
383383
workerId = workInfo.id.toString(),
384384
courseId = courseId,
385385
assignmentId = assignmentId,
386386
userId = studentId,
387-
filePaths = selectedFilePaths.orEmpty(),
387+
filePaths = filePaths,
388388
action = FileUploadWorker.ACTION_TEACHER_SUBMISSION_COMMENT,
389389
attemptId = selectedAttemptId.takeIf { assignmentEnhancementsEnabled }
390390
)
@@ -399,7 +399,7 @@ class SpeedGraderCommentsViewModel @Inject constructor(
399399
this.workerId = workInfo.id
400400
this.status = CommentSendStatus.SENDING
401401
this.workerInputData = FileUploadWorkerData(
402-
selectedFilePaths.orEmpty(),
402+
filePaths,
403403
courseId,
404404
assignmentId,
405405
studentId
@@ -647,7 +647,7 @@ class SpeedGraderCommentsViewModel @Inject constructor(
647647
fileUploadInputDao.insert(inputData)
648648

649649
WorkManager.getInstance(context).apply {
650-
onFileUploadStarted(getWorkInfoByIdLiveData(worker.id))
650+
onFileUploadStarted(getWorkInfoByIdLiveData(worker.id), filePaths)
651651
enqueue(worker)
652652
}
653653
}

‎libs/pandautils/src/test/java/com/instructure/pandautils/features/speedgrader/grade/comments/SpeedGraderCommentsViewModelTest.kt‎

Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,12 @@
1717
package com.instructure.pandautils.features.speedgrader.grade.comments
1818

1919
import androidx.arch.core.executor.testing.InstantTaskExecutorRule
20+
import androidx.lifecycle.MutableLiveData
2021
import androidx.lifecycle.SavedStateHandle
22+
import androidx.work.WorkInfo
2123
import com.instructure.canvasapi2.SubmissionCommentsQuery
2224
import com.instructure.canvasapi2.utils.ApiPrefs
25+
import com.instructure.pandautils.features.file.upload.FileUploadEvent
2326
import com.instructure.pandautils.features.file.upload.FileUploadEventHandler
2427
import com.instructure.pandautils.features.speedgrader.SpeedGraderSelectedAttemptHolder
2528
import com.instructure.pandautils.room.appdatabase.daos.AttachmentDao
@@ -28,19 +31,23 @@ import com.instructure.pandautils.room.appdatabase.daos.FileUploadInputDao
2831
import com.instructure.pandautils.room.appdatabase.daos.MediaCommentDao
2932
import com.instructure.pandautils.room.appdatabase.daos.PendingSubmissionCommentDao
3033
import com.instructure.pandautils.room.appdatabase.daos.SubmissionCommentDao
34+
import com.instructure.pandautils.room.appdatabase.entities.FileUploadInputEntity
3135
import com.instructure.pandautils.room.appdatabase.entities.PendingSubmissionCommentEntity
3236
import com.instructure.pandautils.room.appdatabase.model.PendingSubmissionCommentWithFileUploadInput
3337
import com.instructure.pandautils.views.RecordingMediaType
3438
import io.mockk.coEvery
39+
import io.mockk.coVerify
3540
import io.mockk.every
3641
import io.mockk.mockk
42+
import io.mockk.slot
3743
import junit.framework.TestCase.assertEquals
3844
import kotlinx.coroutines.Dispatchers
3945
import kotlinx.coroutines.ExperimentalCoroutinesApi
4046
import kotlinx.coroutines.flow.MutableSharedFlow
4147
import kotlinx.coroutines.flow.MutableStateFlow
4248
import kotlinx.coroutines.flow.flowOf
4349
import kotlinx.coroutines.test.UnconfinedTestDispatcher
50+
import kotlinx.coroutines.test.advanceUntilIdle
4451
import kotlinx.coroutines.test.resetMain
4552
import kotlinx.coroutines.test.runTest
4653
import kotlinx.coroutines.test.setMain
@@ -49,6 +56,7 @@ import org.junit.Assert
4956
import org.junit.Before
5057
import org.junit.Rule
5158
import org.junit.Test
59+
import java.util.UUID
5260

5361
@ExperimentalCoroutinesApi
5462
class SpeedGraderCommentsViewModelTest {
@@ -372,4 +380,194 @@ class SpeedGraderCommentsViewModelTest {
372380
Thread.sleep(100)
373381
assertEquals(0, viewModel.uiState.value.comments.size)
374382
}
383+
384+
@Test
385+
fun `FileUploadEvent UploadStarted uses file paths from event`() = runTest {
386+
val fileUploadEventsFlow = MutableSharedFlow<FileUploadEvent>(replay = 1)
387+
coEvery { fileUploadEventHandler.events } returns fileUploadEventsFlow
388+
389+
val workInfoLiveData = MutableLiveData<WorkInfo>()
390+
val workInfo = mockk<WorkInfo>(relaxed = true)
391+
val workerId = UUID.randomUUID()
392+
every { workInfo.id } returns workerId
393+
every { workInfo.state } returns WorkInfo.State.RUNNING
394+
395+
val expectedFilePaths = listOf("/path/to/file1.pdf", "/path/to/file2.jpg")
396+
val fileUploadInputSlot = slot<FileUploadInputEntity>()
397+
398+
coEvery { fileUploadInputDao.findByWorkerId(any()) } returns null
399+
coEvery { fileUploadInputDao.insert(capture(fileUploadInputSlot)) } returns Unit
400+
coEvery { pendingSubmissionCommentDao.findByPageId(any()) } returns null
401+
coEvery { pendingSubmissionCommentDao.insert(any()) } returns 1L
402+
403+
createViewModel()
404+
405+
// Emit the UploadStarted event with file paths
406+
fileUploadEventsFlow.emit(
407+
FileUploadEvent.UploadStarted(
408+
uuid = workerId,
409+
workInfoLiveData = workInfoLiveData,
410+
filePaths = expectedFilePaths
411+
)
412+
)
413+
414+
advanceUntilIdle()
415+
416+
// Trigger the worker state change
417+
workInfoLiveData.postValue(workInfo)
418+
419+
advanceUntilIdle()
420+
421+
// Verify that FileUploadInputEntity was created with the correct file paths from the event
422+
coVerify { fileUploadInputDao.insert(any()) }
423+
assertEquals(expectedFilePaths, fileUploadInputSlot.captured.filePaths)
424+
}
425+
426+
@Test
427+
fun `FileUploadEvent UploadStarted creates pending comment with correct file paths`() = runTest {
428+
val fileUploadEventsFlow = MutableSharedFlow<FileUploadEvent>(replay = 1)
429+
coEvery { fileUploadEventHandler.events } returns fileUploadEventsFlow
430+
431+
val workInfoLiveData = MutableLiveData<WorkInfo>()
432+
val workInfo = mockk<WorkInfo>(relaxed = true)
433+
val workerId = UUID.randomUUID()
434+
every { workInfo.id } returns workerId
435+
every { workInfo.state } returns WorkInfo.State.RUNNING
436+
437+
val expectedFilePaths = listOf("/path/to/file1.pdf")
438+
val pendingCommentSlot = slot<PendingSubmissionCommentEntity>()
439+
440+
coEvery { fileUploadInputDao.findByWorkerId(any()) } returns null
441+
coEvery { fileUploadInputDao.insert(any()) } returns Unit
442+
coEvery { pendingSubmissionCommentDao.findByPageId(any()) } returns null
443+
coEvery { pendingSubmissionCommentDao.insert(capture(pendingCommentSlot)) } returns 1L
444+
445+
createViewModel()
446+
447+
// Emit the UploadStarted event
448+
fileUploadEventsFlow.emit(
449+
FileUploadEvent.UploadStarted(
450+
uuid = workerId,
451+
workInfoLiveData = workInfoLiveData,
452+
filePaths = expectedFilePaths
453+
)
454+
)
455+
456+
advanceUntilIdle()
457+
458+
// Trigger worker state
459+
workInfoLiveData.postValue(workInfo)
460+
461+
advanceUntilIdle()
462+
463+
// Verify pending comment was created with the correct file paths
464+
coVerify { pendingSubmissionCommentDao.insert(any()) }
465+
// The workerInputData is not stored directly in the entity - it's constructed from fileUploadInput
466+
// So we can't test it here. Instead, we verify that the entity was created successfully.
467+
assertEquals("domain-3-1-2", pendingCommentSlot.captured.pageId)
468+
}
469+
470+
@Test
471+
fun `FileUploadEvent UploadStarted does not create duplicate pending comments`() = runTest {
472+
val fileUploadEventsFlow = MutableSharedFlow<FileUploadEvent>(replay = 1)
473+
coEvery { fileUploadEventHandler.events } returns fileUploadEventsFlow
474+
475+
val workInfoLiveData = MutableLiveData<WorkInfo>()
476+
val workInfo = mockk<WorkInfo>(relaxed = true)
477+
val workerId = UUID.randomUUID()
478+
every { workInfo.id } returns workerId
479+
every { workInfo.state } returns WorkInfo.State.RUNNING
480+
481+
val filePaths = listOf("/path/to/file.pdf")
482+
val existingFileUploadInput = FileUploadInputEntity(
483+
workerId = workerId.toString(),
484+
filePaths = filePaths,
485+
courseId = 3L,
486+
assignmentId = 1L,
487+
userId = 2L,
488+
action = "teacher_submission_comment"
489+
)
490+
491+
val existingPendingComment = PendingSubmissionCommentWithFileUploadInput(
492+
pendingSubmissionCommentEntity = PendingSubmissionCommentEntity(
493+
pageId = "domain-3-1-2"
494+
),
495+
fileUploadInput = existingFileUploadInput
496+
)
497+
498+
coEvery { fileUploadInputDao.findByWorkerId(workerId.toString()) } returns existingFileUploadInput
499+
coEvery { pendingSubmissionCommentDao.findByPageId(any()) } returns listOf(existingPendingComment)
500+
501+
createViewModel()
502+
503+
// Emit the UploadStarted event
504+
fileUploadEventsFlow.emit(
505+
FileUploadEvent.UploadStarted(
506+
uuid = workerId,
507+
workInfoLiveData = workInfoLiveData,
508+
filePaths = filePaths
509+
)
510+
)
511+
512+
advanceUntilIdle()
513+
514+
// Trigger worker state
515+
workInfoLiveData.postValue(workInfo)
516+
517+
advanceUntilIdle()
518+
519+
// Verify no duplicate was created
520+
coVerify(exactly = 0) { fileUploadInputDao.insert(any()) }
521+
coVerify(exactly = 0) { pendingSubmissionCommentDao.insert(any()) }
522+
}
523+
524+
@Test
525+
fun `FileSelected event updates selectedFilePaths variable`() = runTest {
526+
val fileUploadEventsFlow = MutableSharedFlow<FileUploadEvent>(replay = 1)
527+
coEvery { fileUploadEventHandler.events } returns fileUploadEventsFlow
528+
529+
createViewModel()
530+
531+
val expectedFilePaths = listOf("/path/to/selected/file.pdf")
532+
533+
// Emit FileSelected event
534+
fileUploadEventsFlow.emit(
535+
FileUploadEvent.FileSelected(filePaths = expectedFilePaths)
536+
)
537+
538+
advanceUntilIdle()
539+
540+
// The internal selectedFilePaths variable should be updated
541+
// This is tested indirectly by ensuring the next upload uses these paths
542+
viewModel.handleAction(
543+
SpeedGraderCommentsAction.FileUploadStarted(
544+
workInfoLiveData = MutableLiveData()
545+
)
546+
)
547+
548+
advanceUntilIdle()
549+
550+
// Verify the action was handled (the implementation uses selectedFilePaths)
551+
Assert.assertFalse(viewModel.uiState.value.showAttachmentTypeDialog)
552+
}
553+
554+
@Test
555+
fun `DialogDismissed event clears file selector dialog`() = runTest {
556+
val fileUploadEventsFlow = MutableSharedFlow<FileUploadEvent>(replay = 1)
557+
coEvery { fileUploadEventHandler.events } returns fileUploadEventsFlow
558+
559+
createViewModel()
560+
561+
// First show the dialog
562+
viewModel.handleAction(SpeedGraderCommentsAction.ChooseFilesClicked)
563+
Assert.assertNotNull(viewModel.uiState.value.fileSelectorDialogData)
564+
565+
// Emit DialogDismissed event
566+
fileUploadEventsFlow.emit(FileUploadEvent.DialogDismissed)
567+
568+
advanceUntilIdle()
569+
570+
// Verify dialog data is cleared
571+
Assert.assertNull(viewModel.uiState.value.fileSelectorDialogData)
572+
}
375573
}

0 commit comments

Comments
 (0)