diff --git a/service/src/main/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommand.java b/service/src/main/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommand.java index ebfd108e9..0e870e414 100644 --- a/service/src/main/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommand.java +++ b/service/src/main/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommand.java @@ -112,7 +112,7 @@ public class ProcessPushNotificationFeedbackCommand extends AbstractSinglePassCr } @VisibleForTesting - boolean deviceNeedsUpdate(final Device device) { + boolean pushFeedbackIntervalElapsed(final Device device) { // After we get an indication that a device may have uninstalled the Signal app (`uninstalledFeedbackTimestamp` is // non-zero), check back in after a few days to see what ultimately happened. return device.getUninstalledFeedbackTimestamp() != 0 && @@ -129,6 +129,11 @@ public class ProcessPushNotificationFeedbackCommand extends AbstractSinglePassCr return Instant.ofEpochMilli(device.getLastSeen()).plus(MAX_TOKEN_REFRESH_DELAY).isBefore(clock.instant()); } + @VisibleForTesting + boolean deviceNeedsUpdate(final Device device) { + return pushFeedbackIntervalElapsed(device) && (device.isEnabled() || device.getLastSeen() > device.getUninstalledFeedbackTimestamp()); + } + @VisibleForTesting static Optional getUserAgent(final Device device) { if (StringUtils.isNotBlank(device.getApnId())) { diff --git a/service/src/test/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommandTest.java b/service/src/test/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommandTest.java index 9c4279948..e5fc3c4c9 100644 --- a/service/src/test/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommandTest.java +++ b/service/src/test/java/org/whispersystems/textsecuregcm/workers/ProcessPushNotificationFeedbackCommandTest.java @@ -20,6 +20,7 @@ import reactor.core.publisher.Flux; import java.time.Clock; import java.time.Instant; import java.time.ZoneId; +import java.util.ArrayList; import java.util.List; import java.util.Optional; import java.util.concurrent.CompletableFuture; @@ -40,6 +41,8 @@ class ProcessPushNotificationFeedbackCommandTest { private ProcessPushNotificationFeedbackCommand processPushNotificationFeedbackCommand; + private static final Instant CURRENT_TIME = Instant.now(); + private static class TestProcessPushNotificationFeedbackCommand extends ProcessPushNotificationFeedbackCommand { private final CommandDependencies commandDependencies; @@ -80,7 +83,7 @@ class ProcessPushNotificationFeedbackCommandTest { return CompletableFuture.completedFuture(account); }); - clock = Clock.fixed(Instant.now(), ZoneId.systemDefault()); + clock = Clock.fixed(CURRENT_TIME, ZoneId.systemDefault()); processPushNotificationFeedbackCommand = new TestProcessPushNotificationFeedbackCommand(clock, accountsManager, true); @@ -95,6 +98,7 @@ class ProcessPushNotificationFeedbackCommandTest { final Account accountWithActiveDevice = mock(Account.class); { final Device device = mock(Device.class); + when(device.isEnabled()).thenReturn(true); when(accountWithActiveDevice.getDevices()).thenReturn(List.of(device)); } @@ -102,31 +106,44 @@ class ProcessPushNotificationFeedbackCommandTest { final Account accountWithUninstalledDevice = mock(Account.class); { final Device uninstalledDevice = mock(Device.class); + when(uninstalledDevice.isEnabled()).thenReturn(true); when(uninstalledDevice.getUninstalledFeedbackTimestamp()) .thenReturn(clock.instant().minus(ProcessPushNotificationFeedbackCommand.MAX_TOKEN_REFRESH_DELAY.multipliedBy(2)).toEpochMilli()); when(accountWithUninstalledDevice.getDevices()).thenReturn(List.of(uninstalledDevice)); } + final Account accountWithAlreadyDisabledUninstalledDevice = mock(Account.class); + { + final Device previouslyDisabledUninstalledDevice = mock(Device.class); + when(previouslyDisabledUninstalledDevice.isEnabled()).thenReturn(false); + when(previouslyDisabledUninstalledDevice.getUninstalledFeedbackTimestamp()) + .thenReturn(clock.instant().minus(ProcessPushNotificationFeedbackCommand.MAX_TOKEN_REFRESH_DELAY.multipliedBy(2)).toEpochMilli()); + + when(accountWithAlreadyDisabledUninstalledDevice.getDevices()).thenReturn(List.of(previouslyDisabledUninstalledDevice)); + } + processPushNotificationFeedbackCommand.crawlAccounts( - Flux.just(accountWithActiveDevice, accountWithUninstalledDevice).parallel()); + Flux.just(accountWithActiveDevice, accountWithUninstalledDevice, accountWithAlreadyDisabledUninstalledDevice) + .parallel()); if (isDryRun) { verify(accountsManager, never()).updateAsync(any(), any()); } else { verify(accountsManager, never()).updateAsync(eq(accountWithActiveDevice), any()); verify(accountsManager).updateAsync(eq(accountWithUninstalledDevice), any()); + verify(accountsManager, never()).updateAsync(eq(accountWithAlreadyDisabledUninstalledDevice), any()); } } @Test - void deviceNeedsUpdate() { + void pushFeedbackIntervalElapsed() { { final Device deviceWithMaturePushNotificationFeedback = mock(Device.class); when(deviceWithMaturePushNotificationFeedback.getUninstalledFeedbackTimestamp()) .thenReturn(clock.instant().minus(ProcessPushNotificationFeedbackCommand.MAX_TOKEN_REFRESH_DELAY.multipliedBy(2)).toEpochMilli()); - assertTrue(processPushNotificationFeedbackCommand.deviceNeedsUpdate(deviceWithMaturePushNotificationFeedback)); + assertTrue(processPushNotificationFeedbackCommand.pushFeedbackIntervalElapsed(deviceWithMaturePushNotificationFeedback)); } { @@ -134,13 +151,13 @@ class ProcessPushNotificationFeedbackCommandTest { when(deviceWithRecentPushNotificationFeedback.getUninstalledFeedbackTimestamp()) .thenReturn(clock.instant().minus(ProcessPushNotificationFeedbackCommand.MAX_TOKEN_REFRESH_DELAY.dividedBy(2)).toEpochMilli()); - assertFalse(processPushNotificationFeedbackCommand.deviceNeedsUpdate(deviceWithRecentPushNotificationFeedback)); + assertFalse(processPushNotificationFeedbackCommand.pushFeedbackIntervalElapsed(deviceWithRecentPushNotificationFeedback)); } { final Device deviceWithoutPushNotificationFeedback = mock(Device.class); - assertFalse(processPushNotificationFeedbackCommand.deviceNeedsUpdate(deviceWithoutPushNotificationFeedback)); + assertFalse(processPushNotificationFeedbackCommand.pushFeedbackIntervalElapsed(deviceWithoutPushNotificationFeedback)); } } @@ -164,6 +181,72 @@ class ProcessPushNotificationFeedbackCommandTest { } } + @ParameterizedTest + @MethodSource + void deviceNeedsUpdate(final Device device, final boolean expectNeedsUpdate) { + assertEquals(expectNeedsUpdate, processPushNotificationFeedbackCommand.deviceNeedsUpdate(device)); + } + + private static List deviceNeedsUpdate() { + final long maturePushFeedbackTimestamp = + CURRENT_TIME.minus(ProcessPushNotificationFeedbackCommand.MAX_TOKEN_REFRESH_DELAY.multipliedBy(2)) + .toEpochMilli(); + + final List arguments = new ArrayList<>(); + + { + // Device is active, enabled, and has no push feedback + final Device device = mock(Device.class); + when(device.isEnabled()).thenReturn(true); + when(device.getUninstalledFeedbackTimestamp()).thenReturn(0L); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); + + arguments.add(Arguments.of(device, false)); + } + + { + // Device is active, but not enabled, and has no push feedback + final Device device = mock(Device.class); + when(device.isEnabled()).thenReturn(false); + when(device.getUninstalledFeedbackTimestamp()).thenReturn(0L); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); + + arguments.add(Arguments.of(device, false)); + } + + { + // Device is active, enabled, and has "mature" push feedback + final Device device = mock(Device.class); + when(device.isEnabled()).thenReturn(true); + when(device.getUninstalledFeedbackTimestamp()).thenReturn(maturePushFeedbackTimestamp); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); + + arguments.add(Arguments.of(device, true)); + } + + { + // Device is active, but not enabled, and has "mature" push feedback + final Device device = mock(Device.class); + when(device.isEnabled()).thenReturn(false); + when(device.getUninstalledFeedbackTimestamp()).thenReturn(maturePushFeedbackTimestamp); + when(device.getLastSeen()).thenReturn(CURRENT_TIME.toEpochMilli()); + + arguments.add(Arguments.of(device, true)); + } + + { + // Device is inactive, not enabled, and has "mature" push feedback + final Device device = mock(Device.class); + when(device.isEnabled()).thenReturn(false); + when(device.getUninstalledFeedbackTimestamp()).thenReturn(maturePushFeedbackTimestamp); + when(device.getLastSeen()).thenReturn(maturePushFeedbackTimestamp); + + arguments.add(Arguments.of(device, false)); + } + + return arguments; + } + @SuppressWarnings("OptionalUsedAsFieldOrParameterType") @ParameterizedTest @MethodSource