Fix: summary screen crashes if multiple histories are present for the…

… patient Steps to reproduce: 1. Send patients to the server without any medical history. This can be done by using an HTTP client or running Android tests for syncing patients 2. Open the patient's summary screen on two phones 3. Answer different medical history questions 4. Exit summary screen 5. Trigger a sync on both phones 6. Open summary screen again for the same patient 7. The app will crash.
simpledotorg · Oct 6, 2018 · 6a1f719 · 6a1f719
1 parent 3f07963
commit 6a1f719
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 14 deletions.
diff --git a/.../androidTest/java/org/simple/clinic/medicalhistory/MedicalHistoryRepositoryAndroidTest.kt b/.../androidTest/java/org/simple/clinic/medicalhistory/MedicalHistoryRepositoryAndroidTest.kt
@@ -104,4 +104,26 @@ class MedicalHistoryRepositoryAndroidTest {
     assertThat(emptyHistory.hasDiabetes).isFalse()
     assertThat(emptyHistory.syncStatus).isEqualTo(SyncStatus.DONE)
   }
+
+  @Test
+  fun when_multiple_medical_histories_are_present_for_a_patient_then_only_the_last_edited_one_should_be_returned() {
+    val patientUuid = UUID.randomUUID()
+
+    val olderHistory = testData.medicalHistory(
+        patientUuid = patientUuid,
+        createdAt = Instant.now(clock).minusMillis(100),
+        updatedAt = Instant.now(clock).minusMillis(100))
+
+    val newerHistory = testData.medicalHistory(
+        patientUuid = patientUuid,
+        createdAt = Instant.now(clock).minusMillis(100),
+        updatedAt = Instant.now(clock))
+
+    repository.save(olderHistory, updateTime = {olderHistory.updatedAt})
+        .andThen(repository.save(newerHistory, updateTime = {newerHistory.updatedAt}))
+        .blockingAwait()
+
+    val foundHistory = repository.historyForPatientOrDefault(patientUuid).blockingFirst()
+    assertThat(foundHistory.uuid).isEqualTo(newerHistory.uuid)
+  }
 }
diff --git a/app/src/main/java/org/simple/clinic/medicalhistory/MedicalHistory.kt b/app/src/main/java/org/simple/clinic/medicalhistory/MedicalHistory.kt
@@ -55,7 +55,18 @@ data class MedicalHistory(
     @Query("DELETE FROM MedicalHistory")
     fun clear()
 
-    @Query("SELECT * FROM MedicalHistory WHERE patientUuid = :patientUuid")
+    /**
+     * The last updated medical history is returned because it's possible
+     * to have multiple medical histories present for the same patient.
+     * See [MedicalHistoryRepository.historyForPatientOrDefault] to
+     * understand when this will happen.
+     */
+    @Query("""
+      SELECT * FROM MedicalHistory
+      WHERE patientUuid = :patientUuid
+      ORDER BY updatedAt DESC
+      LIMIT 1
+    """)
     fun historyForPatient(patientUuid: PatientUuid): Flowable<List<MedicalHistory>>
   }
 }
diff --git a/app/src/main/java/org/simple/clinic/medicalhistory/MedicalHistoryRepository.kt b/app/src/main/java/org/simple/clinic/medicalhistory/MedicalHistoryRepository.kt
@@ -3,7 +3,6 @@ package org.simple.clinic.medicalhistory
 import io.reactivex.Completable
 import io.reactivex.Observable
 import io.reactivex.Single
-import org.simple.clinic.BuildConfig
 import org.simple.clinic.medicalhistory.sync.MedicalHistoryPayload
 import org.simple.clinic.patient.PatientUuid
 import org.simple.clinic.patient.SyncStatus
@@ -37,11 +36,7 @@ class MedicalHistoryRepository @Inject constructor(
         .toObservable()
         .map { histories ->
           if (histories.size > 1) {
-            if (BuildConfig.DEBUG) {
-              throw AssertionError("Multiple histories are present for $patientUuid: $histories")
-            } else {
-              throw AssertionError("Multiple histories are present")
-            }
+            throw AssertionError("DAO shouldn't have returned multiple histories for the same patient")
           }
           if (histories.isEmpty()) {
             // This patient's MedicalHistory hasn't synced yet. We're okay with overriding
@@ -71,11 +66,11 @@ class MedicalHistoryRepository @Inject constructor(
     return save(listOf(medicalHistory))
   }
 
-  fun save(history: MedicalHistory): Completable {
+  fun save(history: MedicalHistory, updateTime: () -> Instant = { Instant.now(clock) }): Completable {
     return Completable.fromAction {
       val dirtyHistory = history.copy(
           syncStatus = SyncStatus.PENDING,
-          updatedAt = Instant.now(clock))
+          updatedAt = updateTime())
       dao.save(dirtyHistory)
     }
   }

diff --git a/app/src/main/java/org/simple/clinic/summary/PatientSummaryScreenController.kt b/app/src/main/java/org/simple/clinic/summary/PatientSummaryScreenController.kt
@@ -28,6 +28,7 @@ import org.simple.clinic.summary.PatientSummaryCaller.SEARCH
 import org.simple.clinic.util.Just
 import org.simple.clinic.util.exhaustive
 import org.simple.clinic.widgets.UiEvent
+import org.threeten.bp.Clock
 import javax.inject.Inject
 
 typealias Ui = PatientSummaryScreen
@@ -38,7 +39,8 @@ class PatientSummaryScreenController @Inject constructor(
     private val bpRepository: BloodPressureRepository,
     private val prescriptionRepository: PrescriptionRepository,
     private val medicalHistoryRepository: MedicalHistoryRepository,
-    private val timestampGenerator: RelativeTimestampGenerator
+    private val timestampGenerator: RelativeTimestampGenerator,
+    private val clock: Clock
 ) : ObservableTransformer<UiEvent, UiChange> {
 
   private lateinit var disposable: Disposable

diff --git a/...t/java/org/simple/clinic/medicalhistory/newentry/NewMedicalHistoryScreenControllerTest.kt b/...t/java/org/simple/clinic/medicalhistory/newentry/NewMedicalHistoryScreenControllerTest.kt
@@ -43,7 +43,7 @@ class NewMedicalHistoryScreenControllerTest {
   fun setUp() {
     controller = NewMedicalHistoryScreenController(medicalHistoryRepository, patientRepository)
 
-    whenever(medicalHistoryRepository.save(any(), any())).thenReturn(Completable.complete())
+    whenever(medicalHistoryRepository.save(any<UUID>(), any())).thenReturn(Completable.complete())
     whenever(patientRepository.ongoingEntry()).thenReturn(Single.never())
 
     uiEvents.compose(controller).subscribe { uiChange -> uiChange(screen) }

diff --git a/app/src/test/java/org/simple/clinic/summary/PatientSummaryScreenControllerTest.kt b/app/src/test/java/org/simple/clinic/summary/PatientSummaryScreenControllerTest.kt
@@ -36,7 +36,9 @@ import org.simple.clinic.patient.PatientRepository
 import org.simple.clinic.util.Just
 import org.simple.clinic.util.None
 import org.simple.clinic.widgets.UiEvent
+import org.threeten.bp.Clock
 import org.threeten.bp.Instant
+import org.threeten.bp.ZoneOffset.UTC
 import java.util.UUID
 
 @RunWith(JUnitParamsRunner::class)
@@ -48,6 +50,7 @@ class PatientSummaryScreenControllerTest {
   private val prescriptionRepository = mock<PrescriptionRepository>()
   private val medicalHistoryRepository = mock<MedicalHistoryRepository>()
   private val patientUuid = UUID.randomUUID()
+  private val clock = Clock.fixed(Instant.now(), UTC)
 
   private val uiEvents = PublishSubject.create<UiEvent>()
   private val reporter = MockReporter()
@@ -62,7 +65,8 @@ class PatientSummaryScreenControllerTest {
         bpRepository,
         prescriptionRepository,
         medicalHistoryRepository,
-        timestampGenerator)
+        timestampGenerator,
+        clock)
 
     uiEvents
         .compose(controller)
@@ -265,7 +269,7 @@ class PatientSummaryScreenControllerTest {
         hasDiabetes = false,
         updatedAt = Instant.now())
     whenever(medicalHistoryRepository.historyForPatientOrDefault(patientUuid)).thenReturn(Observable.just(medicalHistory))
-    whenever(medicalHistoryRepository.save(any<MedicalHistory>())).thenReturn(Completable.complete())
+    whenever(medicalHistoryRepository.save(any<MedicalHistory>(), any())).thenReturn(Completable.complete())
 
     uiEvents.onNext(PatientSummaryScreenCreated(patientUuid, caller = PatientSummaryCaller.SEARCH))
     uiEvents.onNext(SummaryMedicalHistoryAnswerToggled(question, selected = true))
@@ -277,7 +281,7 @@ class PatientSummaryScreenControllerTest {
         hasHadStroke = question == HAS_HAD_A_STROKE,
         hasHadKidneyDisease = question == HAS_HAD_A_KIDNEY_DISEASE,
         hasDiabetes = question == HAS_DIABETES)
-    verify(medicalHistoryRepository).save(updatedMedicalHistory)
+    verify(medicalHistoryRepository).save(eq(updatedMedicalHistory), any())
   }
 
   @Suppress("unused")