Update in app expiry notifications over time

[kl]

Changes the in app account expiry notifications to a flow that emits at the starts of the notification threshold (currently 3 days before expiry), and then emits every time the notification update duration is passed (currently 1 day).

---

This change is 

[linear]

DROID-1348 Ensure we update InAppAccountExpiry notification

[Rawa]

Reviewed 16 of 16 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @kl)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/extensions/ResourcesExtensions.kt line 26 at r2 (raw file):

fun Period.isNegative() =
    normalizedStandard().let {

Would any of these work instead?

    normalizedStandard().toStandardSeconds() < 0

or

    normalizedStandard().toStandardDuration().millis < 0

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryTickerFlow.kt line 14 at r2 (raw file):

    tickStart: Duration,
    updateInterval: (expiry: DateTime) -> Duration,
): Flow<Period> = flow {

Since I'm a bit senile, remind me, why did we say Period instead of Duration?

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryTickerFlow.kt line 41 at r2 (raw file):

    var delayCount = calculateDelaysNeeded(expiry.millisFromNow(), currentUpdateInterval)

    while (delayCount > 0) {

I haven't fully thought this through, but it feels like it should work to make use of the do {} while loop https://kotlinlang.org/docs/control-flow.html#while-loops

    do {
        // emit()
        // newUpdate interval
        // calulate new delay
        // evaluate hasAnotherEmission
    } while(hasAnotherEmission)
    emit(Period.ZERO)

Then we should be able to remove these lines:

   // Always emit at start of flow.
    emit(Period(DateTime.now(), expiry))

    // Delay until the next update interval.
    delay(expiry.millisFromNow() % currentUpdateInterval)

    var delayCount = calculateDelaysNeeded(expiry.millisFromNow(), currentUpdateInterval)

whilst making the code more readable.

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryTickerFlow.kt line 64 at r2 (raw file):

// Note that the returned delays may add upp to less than the remaining time, for example
// if we have 100ms remaining and currentUpdateIntervalMillis is 40ms this function will return 2.
private fun calculateDelaysNeeded(millisUntilExpiry: Long, currentUpdateIntervalMillis: Long): Int {

Shouldn't be a problem but we should consider to return Long since we are diving with Long to avoid any overflow.

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryTickerFlow.kt line 68 at r2 (raw file):

        0
    } else {
        (millisUntilExpiry / currentUpdateIntervalMillis).toInt()

This is floorDiv I believe.

[Pururun]

Reviewed all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @kl)

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/AccountExpiryInAppNotificationUseCaseTest.kt line 57 at r2 (raw file):

    @Test
    fun `initial state should be empty`() = runTest {
        // Arrange, Act, Assert

This comment is kind of useless, so I think it should be either removed into test { } or removed.

[kl]

Reviewable status: 7 of 16 files reviewed, 6 unresolved discussions (waiting on @Pururun and @Rawa)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/extensions/ResourcesExtensions.kt line 26 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Would any of these work instead?

    normalizedStandard().toStandardSeconds() < 0

or

    normalizedStandard().toStandardDuration().millis < 0

No but we can do this instead fun Period.isNegative() = toStandardDuration().millis < 0

---

android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/AccountExpiryInAppNotificationUseCaseTest.kt line 57 at r2 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

This comment is kind of useless, so I think it should be either removed into test { } or removed.

Done.

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryTickerFlow.kt line 14 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

Since I'm a bit senile, remind me, why did we say Period instead of Duration?

Period is used here

fun Resources.getExpiryQuantityString(accountExpiry: Period): String {
    return if (accountExpiry.isNegative() || accountExpiry == Period.ZERO) {
        getString(R.string.out_of_time)
    } else if (accountExpiry.years > 0) {
        getRemainingText(this, R.plurals.years_left, accountExpiry.years)
    } else if (accountExpiry.months >= 3) {
        getRemainingText(this, R.plurals.months_left, accountExpiry.months)
    } else if (accountExpiry.months > 0 || accountExpiry.days >= 1) {
        getRemainingText(this, R.plurals.days_left, accountExpiry.days)
    } else {
        getString(R.string.less_than_a_day_left)
    }
}

because Duration doesn't have methods to convert to years or months

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryTickerFlow.kt line 68 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

This is floorDiv I believe.

Looks like floorDiv can return negative numbers, but we can do this instead:

private fun calculateDelaysNeeded(millisUntilExpiry: Long, currentUpdateIntervalMillis: Long): Long =
    millisUntilExpiry.coerceAtLeast(0) / currentUpdateIntervalMillis

[Rawa]

Reviewed 9 of 9 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @albin-mullvad, @kl, and @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/extensions/ResourcesExtensions.kt line 26 at r2 (raw file):

Previously, kl (Kalle Lindström) wrote…

No but we can do this instead fun Period.isNegative() = toStandardDuration().millis < 0

🙌 Very nice!

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryTickerFlow.kt line 14 at r2 (raw file):

Previously, kl (Kalle Lindström) wrote…

Period is used here

fun Resources.getExpiryQuantityString(accountExpiry: Period): String {
    return if (accountExpiry.isNegative() || accountExpiry == Period.ZERO) {
        getString(R.string.out_of_time)
    } else if (accountExpiry.years > 0) {
        getRemainingText(this, R.plurals.years_left, accountExpiry.years)
    } else if (accountExpiry.months >= 3) {
        getRemainingText(this, R.plurals.months_left, accountExpiry.months)
    } else if (accountExpiry.months > 0 || accountExpiry.days >= 1) {
        getRemainingText(this, R.plurals.days_left, accountExpiry.days)
    } else {
        getString(R.string.less_than_a_day_left)
    }
}

because Duration doesn't have methods to convert to years or months

Ah, yes. no toStandardMonth() or toStandardYear(). Kind of sucks, I guess we could add it ourselves as extension functions but it would be a bit less accurate. I'm fine with Period. @Pururun @albin-mullvad do you have any opinion about this?

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryTickerFlow.kt line 41 at r2 (raw file):

Previously, Rawa (David Göransson) wrote…

I haven't fully thought this through, but it feels like it should work to make use of the do {} while loop https://kotlinlang.org/docs/control-flow.html#while-loops

    do {
        // emit()
        // newUpdate interval
        // calulate new delay
        // evaluate hasAnotherEmission
    } while(hasAnotherEmission)
    emit(Period.ZERO)

Then we should be able to remove these lines:

   // Always emit at start of flow.
    emit(Period(DateTime.now(), expiry))

    // Delay until the next update interval.
    delay(expiry.millisFromNow() % currentUpdateInterval)

    var delayCount = calculateDelaysNeeded(expiry.millisFromNow(), currentUpdateInterval)

whilst making the code more readable.

Discussed offline. 👏

---

android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryTickerFlow.kt line 68 at r2 (raw file):

Previously, kl (Kalle Lindström) wrote…

Looks like floorDiv can return negative numbers, but we can do this instead:

private fun calculateDelaysNeeded(millisUntilExpiry: Long, currentUpdateIntervalMillis: Long): Long =
    millisUntilExpiry.coerceAtLeast(0) / currentUpdateIntervalMillis

Looks good to me 👍

[Rawa]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kl and @Pururun)

[Rawa]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)

[Pururun]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[Pururun]

Just remember to squash commits

Reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved