Merge "Fix ordering for event processing." into udc-mainline-prod

5 files changed, 129 insertions, 52 deletions

diff --git a/service/java/com/android/safetycenter/SafetyCenterRefreshTracker.java b/service/java/com/android/safetycenter/SafetyCenterRefreshTracker.java
index 17688063a..d98127300 100644
--- a/service/java/com/android/safetycenter/SafetyCenterRefreshTracker.java
+++ b/service/java/com/android/safetycenter/SafetyCenterRefreshTracker.java
@@ -155,8 +155,7 @@ public final class SafetyCenterRefreshTracker {
      */
     public boolean reportSourceRefreshCompleted(
             String refreshBroadcastId,
-            String sourceId,
-            @UserIdInt int userId,
+            SafetySourceKey safetySourceKey,
             boolean successful,
             boolean dataChanged) {
         RefreshInProgress refreshInProgress =
@@ -165,9 +164,9 @@ public final class SafetyCenterRefreshTracker {
             return false;
         }
 
-        SafetySourceKey sourceKey = SafetySourceKey.of(sourceId, userId);
         Duration duration =
-                refreshInProgress.markSourceRefreshComplete(sourceKey, successful, dataChanged);
+                refreshInProgress.markSourceRefreshComplete(
+                        safetySourceKey, successful, dataChanged);
         int refreshReason = refreshInProgress.getReason();
         int requestType = RefreshReasons.toRefreshRequestType(refreshReason);
 
@@ -175,8 +174,8 @@ public final class SafetyCenterRefreshTracker {
             int sourceResult = toSystemEventResult(successful);
             SafetyCenterStatsdLogger.writeSourceRefreshSystemEvent(
                     requestType,
-                    sourceId,
-                    UserUtils.isManagedProfile(userId, mContext),
+                    safetySourceKey.getSourceId(),
+                    UserUtils.isManagedProfile(safetySourceKey.getUserId(), mContext),
                     duration,
                     sourceResult,
                     refreshReason,
diff --git a/service/java/com/android/safetycenter/data/SafetyCenterDataManager.java b/service/java/com/android/safetycenter/data/SafetyCenterDataManager.java
index 3925b64aa..018fedf41 100644
--- a/service/java/com/android/safetycenter/data/SafetyCenterDataManager.java
+++ b/service/java/com/android/safetycenter/data/SafetyCenterDataManager.java
@@ -133,7 +133,7 @@ public final class SafetyCenterDataManager {
                 safetySourceData, safetySourceId, packageName, userId)) {
             return false;
         }
-        SafetySourceKey key = SafetySourceKey.of(safetySourceId, userId);
+        SafetySourceKey safetySourceKey = SafetySourceKey.of(safetySourceId, userId);
 
         // Must fetch refresh reason before calling processSafetyEvent because the latter may
         // complete and clear the current refresh.
@@ -143,16 +143,15 @@ public final class SafetyCenterDataManager {
             refreshReason = mSafetyCenterRefreshTracker.getRefreshReason();
         }
 
-        boolean sourceDataDiffers =
-                mSafetySourceDataRepository.setSafetySourceData(
-                        safetySourceData, safetySourceId, userId);
+        // It is important to process the event first as it relies on the data available prior to
+        // changing it.
+        boolean sourceDataWillChange =
+                !mSafetySourceDataRepository.sourceHasData(safetySourceKey, safetySourceData);
         boolean eventCausedChange =
                 processSafetyEvent(
-                        safetySourceId,
-                        safetyEvent,
-                        userId,
-                        /* isError= */ false,
-                        sourceDataDiffers);
+                        safetySourceKey, safetyEvent, /* isError= */ false, sourceDataWillChange);
+        boolean sourceDataDiffers =
+                mSafetySourceDataRepository.setSafetySourceData(safetySourceKey, safetySourceData);
         boolean safetyCenterDataChanged = sourceDataDiffers || eventCausedChange;
 
         if (safetyCenterDataChanged) {
@@ -160,7 +159,12 @@ public final class SafetyCenterDataManager {
         }
 
         mSafetySourceStateCollectedLogger.writeSourceUpdatedAtom(
-                key, safetySourceData, refreshReason, sourceDataDiffers, userId, safetyEvent);
+                safetySourceKey,
+                safetySourceData,
+                refreshReason,
+                sourceDataDiffers,
+                userId,
+                safetyEvent);
 
         return safetyCenterDataChanged;
     }
@@ -204,7 +208,7 @@ public final class SafetyCenterDataManager {
             return false;
         }
         SafetyEvent safetyEvent = safetySourceErrorDetails.getSafetyEvent();
-        SafetySourceKey key = SafetySourceKey.of(safetySourceId, userId);
+        SafetySourceKey safetySourceKey = SafetySourceKey.of(safetySourceId, userId);
 
         // Must fetch refresh reason before calling processSafetyEvent because the latter may
         // complete and clear the current refresh.
@@ -214,16 +218,15 @@ public final class SafetyCenterDataManager {
             refreshReason = mSafetyCenterRefreshTracker.getRefreshReason();
         }
 
-        boolean sourceDataDiffers =
-                mSafetySourceDataRepository.reportSafetySourceError(
-                        safetySourceErrorDetails, safetySourceId, userId);
+        // It is important to process the event first as it relies on the data available prior to
+        // changing it.
+        boolean sourceDataWillChange = !mSafetySourceDataRepository.sourceHasError(safetySourceKey);
         boolean eventCausedChange =
                 processSafetyEvent(
-                        safetySourceId,
-                        safetyEvent,
-                        userId,
-                        /* isError= */ true,
-                        sourceDataDiffers);
+                        safetySourceKey, safetyEvent, /* isError= */ true, sourceDataWillChange);
+        boolean sourceDataDiffers =
+                mSafetySourceDataRepository.reportSafetySourceError(
+                        safetySourceKey, safetySourceErrorDetails);
         boolean safetyCenterDataChanged = sourceDataDiffers || eventCausedChange;
 
         if (safetyCenterDataChanged) {
@@ -231,7 +234,7 @@ public final class SafetyCenterDataManager {
         }
 
         mSafetySourceStateCollectedLogger.writeSourceUpdatedAtom(
-                key,
+                safetySourceKey,
                 /* safetySourceData= */ null,
                 refreshReason,
                 sourceDataDiffers,
@@ -488,11 +491,10 @@ public final class SafetyCenterDataManager {
     }
 
     private boolean processSafetyEvent(
-            String safetySourceId,
+            SafetySourceKey safetySourceKey,
             SafetyEvent safetyEvent,
-            @UserIdInt int userId,
             boolean isError,
-            boolean sourceDataChanged) {
+            boolean sourceDataWillChange) {
         int type = safetyEvent.getType();
         switch (type) {
             case SafetyEvent.SAFETY_EVENT_TYPE_REFRESH_REQUESTED:
@@ -502,7 +504,7 @@ public final class SafetyCenterDataManager {
                     return false;
                 }
                 return mSafetyCenterRefreshTracker.reportSourceRefreshCompleted(
-                        refreshBroadcastId, safetySourceId, userId, !isError, sourceDataChanged);
+                        refreshBroadcastId, safetySourceKey, !isError, sourceDataWillChange);
             case SafetyEvent.SAFETY_EVENT_TYPE_RESOLVING_ACTION_SUCCEEDED:
             case SafetyEvent.SAFETY_EVENT_TYPE_RESOLVING_ACTION_FAILED:
                 String safetySourceIssueId = safetyEvent.getSafetySourceIssueId();
@@ -517,9 +519,9 @@ public final class SafetyCenterDataManager {
                 }
                 SafetyCenterIssueKey safetyCenterIssueKey =
                         SafetyCenterIssueKey.newBuilder()
-                                .setSafetySourceId(safetySourceId)
+                                .setSafetySourceId(safetySourceKey.getSourceId())
                                 .setSafetySourceIssueId(safetySourceIssueId)
-                                .setUserId(userId)
+                                .setUserId(safetySourceKey.getUserId())
                                 .build();
                 SafetyCenterIssueActionId safetyCenterIssueActionId =
                         SafetyCenterIssueActionId.newBuilder()
diff --git a/service/java/com/android/safetycenter/data/SafetySourceDataRepository.java b/service/java/com/android/safetycenter/data/SafetySourceDataRepository.java
index 15e420dce..cdb8709d6 100644
--- a/service/java/com/android/safetycenter/data/SafetySourceDataRepository.java
+++ b/service/java/com/android/safetycenter/data/SafetySourceDataRepository.java
@@ -75,9 +75,8 @@ final class SafetySourceDataRepository {
     }
 
     /**
-     * Sets the latest {@link SafetySourceData} for the given {@code safetySourceId}, {@link
-     * SafetyEvent}, {@code packageName} and {@code userId}, and returns {@code true} if this caused
-     * any changes which would alter {@link SafetyCenterData}.
+     * Sets the latest {@link SafetySourceData} for the given {@link SafetySourceKey}, and returns
+     * {@code true} if this caused any changes which would alter {@link SafetyCenterData}.
      *
      * <p>This method does not perform any validation, {@link
      * SafetyCenterDataManager#setSafetySourceData(SafetySourceData, String, SafetyEvent, String,
@@ -90,18 +89,16 @@ final class SafetySourceDataRepository {
      * <p>This method may modify the {@link SafetyCenterIssueDismissalRepository}.
      */
     boolean setSafetySourceData(
-            @Nullable SafetySourceData safetySourceData,
-            String safetySourceId,
-            @UserIdInt int userId) {
-        SafetySourceKey key = SafetySourceKey.of(safetySourceId, userId);
-        boolean sourceDataDiffers = !Objects.equals(safetySourceData, mSafetySourceData.get(key));
-        boolean removedSourceError = mSafetySourceErrors.remove(key);
+            SafetySourceKey safetySourceKey, @Nullable SafetySourceData safetySourceData) {
+        boolean sourceDataDiffers =
+                !Objects.equals(safetySourceData, mSafetySourceData.get(safetySourceKey));
+        boolean removedSourceError = mSafetySourceErrors.remove(safetySourceKey);
 
         if (sourceDataDiffers) {
-            setSafetySourceDataInternal(key, safetySourceData);
+            setSafetySourceDataInternal(safetySourceKey, safetySourceData);
         }
 
-        setLastUpdatedNow(key);
+        setLastUpdatedNow(safetySourceKey);
         return sourceDataDiffers || removedSourceError;
     }
 
@@ -143,19 +140,33 @@ final class SafetySourceDataRepository {
     }
 
     /**
-     * Reports the given {@link SafetySourceErrorDetails} for the given {@code safetySourceId} and
-     * {@code userId}, and returns {@code true} if this changed the repository's data.
+     * Returns whether the repository has the given {@link SafetySourceData} for the given {@link
+     * SafetySourceKey}.
+     */
+    boolean sourceHasData(
+            SafetySourceKey safetySourceKey, @Nullable SafetySourceData safetySourceData) {
+        if (mSafetySourceErrors.contains(safetySourceKey)) {
+            // Any error will cause the SafetySourceData to be discarded in favor of an error
+            // message, so it can't possibly match the SafetySourceData passed in parameter.
+            return false;
+        }
+        return Objects.equals(safetySourceData, mSafetySourceData.get(safetySourceKey));
+    }
+
+    /**
+     * Reports the given {@link SafetySourceErrorDetails} for the given {@link SafetySourceKey}, and
+     * returns {@code true} if this changed the repository's data.
      *
      * <p>This method does not perform any validation, {@link
      * SafetyCenterDataManager#reportSafetySourceError(SafetySourceErrorDetails, String, String,
      * int)} should be called wherever validation is required.
      */
     boolean reportSafetySourceError(
-            SafetySourceErrorDetails safetySourceErrorDetails,
-            String safetySourceId,
-            @UserIdInt int userId) {
+            SafetySourceKey safetySourceKey, SafetySourceErrorDetails safetySourceErrorDetails) {
         SafetyEvent safetyEvent = safetySourceErrorDetails.getSafetyEvent();
-        Log.w(TAG, "Error reported from source: " + safetySourceId + ", for event: " + safetyEvent);
+        Log.w(
+                TAG,
+                "Error reported from source: " + safetySourceKey + ", for event: " + safetyEvent);
 
         int safetyEventType = safetyEvent.getType();
         if (safetyEventType == SafetyEvent.SAFETY_EVENT_TYPE_RESOLVING_ACTION_FAILED
@@ -163,9 +174,9 @@ final class SafetySourceDataRepository {
             return false;
         }
 
-        SafetySourceKey sourceKey = SafetySourceKey.of(safetySourceId, userId);
-        mSourceStates.put(sourceKey, SAFETY_SOURCE_STATE_COLLECTED__SOURCE_STATE__SOURCE_ERROR);
-        return setSafetySourceError(sourceKey);
+        mSourceStates.put(
+                safetySourceKey, SAFETY_SOURCE_STATE_COLLECTED__SOURCE_STATE__SOURCE_ERROR);
+        return setSafetySourceError(safetySourceKey);
     }
 
     /**
diff --git a/tests/hostside/safetycenter/helper-app/src/android/safetycenter/hostside/device/SafetySourceStateCollectedLoggingHelperTests.kt b/tests/hostside/safetycenter/helper-app/src/android/safetycenter/hostside/device/SafetySourceStateCollectedLoggingHelperTests.kt
index d020aac0a..4d945aad7 100644
--- a/tests/hostside/safetycenter/helper-app/src/android/safetycenter/hostside/device/SafetySourceStateCollectedLoggingHelperTests.kt
+++ b/tests/hostside/safetycenter/helper-app/src/android/safetycenter/hostside/device/SafetySourceStateCollectedLoggingHelperTests.kt
@@ -27,16 +27,21 @@ import com.android.safetycenter.testing.Coroutines.TIMEOUT_SHORT
 import com.android.safetycenter.testing.SafetyCenterApisWithShellPermissions.reportSafetySourceErrorWithPermission
 import com.android.safetycenter.testing.SafetyCenterFlags
 import com.android.safetycenter.testing.SafetyCenterTestConfigs
+import com.android.safetycenter.testing.SafetyCenterTestConfigs.Companion.SINGLE_SOURCE_ID
 import com.android.safetycenter.testing.SafetyCenterTestConfigs.Companion.SOURCE_ID_1
 import com.android.safetycenter.testing.SafetyCenterTestConfigs.Companion.SOURCE_ID_2
 import com.android.safetycenter.testing.SafetyCenterTestConfigs.Companion.SOURCE_ID_3
+import com.android.safetycenter.testing.SafetyCenterTestData
 import com.android.safetycenter.testing.SafetyCenterTestHelper
 import com.android.safetycenter.testing.SafetyCenterTestRule
 import com.android.safetycenter.testing.SafetySourceIntentHandler.Request
 import com.android.safetycenter.testing.SafetySourceIntentHandler.Response
 import com.android.safetycenter.testing.SafetySourceReceiver
+import com.android.safetycenter.testing.SafetySourceReceiver.Companion.executeSafetyCenterIssueActionWithPermissionAndWait
 import com.android.safetycenter.testing.SafetySourceReceiver.Companion.refreshSafetySourcesWithReceiverPermissionAndWait
 import com.android.safetycenter.testing.SafetySourceTestData
+import com.android.safetycenter.testing.SafetySourceTestData.Companion.CRITICAL_ISSUE_ACTION_ID
+import com.android.safetycenter.testing.SafetySourceTestData.Companion.CRITICAL_ISSUE_ID
 import org.junit.Before
 import org.junit.Rule
 import org.junit.Test
@@ -139,6 +144,16 @@ class SafetySourceStateCollectedLoggingHelperTests {
         )
     }
 
+    @Test
+    fun resolvingAction_success() {
+        simulateResolvingActionWith(Response.SetData(safetySourceTestData.information))
+    }
+
+    @Test
+    fun resolvingAction_error() {
+        simulateResolvingActionWith(Response.Error)
+    }
+
     private fun simulateRefresh(
         source1Response: Response?,
         source2Response: Response?,
@@ -167,4 +182,22 @@ class SafetySourceStateCollectedLoggingHelperTests {
         safetyCenterManager.refreshSafetySourcesWithReceiverPermissionAndWait(refreshReason)
         listener.waitForSafetyCenterRefresh()
     }
+
+    private fun simulateResolvingActionWith(response: Response) {
+        safetyCenterTestHelper.setConfig(safetyCenterTestConfigs.singleSourceConfig)
+        safetyCenterTestHelper.setData(
+            SINGLE_SOURCE_ID,
+            safetySourceTestData.criticalWithResolvingGeneralIssue
+        )
+        SafetySourceReceiver.setResponse(Request.ResolveAction(SINGLE_SOURCE_ID), response)
+
+        safetyCenterManager.executeSafetyCenterIssueActionWithPermissionAndWait(
+            SafetyCenterTestData.issueId(SINGLE_SOURCE_ID, CRITICAL_ISSUE_ID),
+            SafetyCenterTestData.issueActionId(
+                SINGLE_SOURCE_ID,
+                CRITICAL_ISSUE_ID,
+                CRITICAL_ISSUE_ACTION_ID
+            )
+        )
+    }
 }
diff --git a/tests/hostside/safetycenter/src/android/safetycenter/hostside/SafetyCenterSystemEventReportedLoggingHostTest.kt b/tests/hostside/safetycenter/src/android/safetycenter/hostside/SafetyCenterSystemEventReportedLoggingHostTest.kt
index 0baf1f5d5..2589c7a47 100644
--- a/tests/hostside/safetycenter/src/android/safetycenter/hostside/SafetyCenterSystemEventReportedLoggingHostTest.kt
+++ b/tests/hostside/safetycenter/src/android/safetycenter/hostside/SafetyCenterSystemEventReportedLoggingHostTest.kt
@@ -194,6 +194,38 @@ class SafetyCenterSystemEventReportedLoggingHostTest : BaseHostJUnit4Test() {
             .isEqualTo(1) // Only source 1
     }
 
+    @Test
+    fun resolveAction_success_resolvingActionSuccessEvent() {
+        helperAppRule.runTest(
+            ".SafetySourceStateCollectedLoggingHelperTests",
+            "resolvingAction_success"
+        )
+
+        val resolvingActionEvent =
+            ReportUtils.getEventMetricDataList(device)
+                .mapNotNull { it.atom.safetyCenterSystemEventReported }
+                .single { it.eventType == EventType.INLINE_ACTION }
+
+        assertThat(resolvingActionEvent.result).isEqualTo(Result.SUCCESS)
+        assertThat(resolvingActionEvent.encodedIssueTypeId).isNotEqualTo(0)
+    }
+
+    @Test
+    fun resolveAction_error_resolvingActionErrorEvent() {
+        helperAppRule.runTest(
+            ".SafetySourceStateCollectedLoggingHelperTests",
+            "resolvingAction_error"
+        )
+
+        val resolvingActionEvent =
+            ReportUtils.getEventMetricDataList(device)
+                .mapNotNull { it.atom.safetyCenterSystemEventReported }
+                .single { it.eventType == EventType.INLINE_ACTION }
+
+        assertThat(resolvingActionEvent.result).isEqualTo(Result.ERROR)
+        assertThat(resolvingActionEvent.encodedIssueTypeId).isNotEqualTo(0)
+    }
+
     companion object {
         private const val REFRESH_REASON_PAGE_OPEN = 100L
         private const val REFRESH_REASON_BUTTON_CLICK = 200L