Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Align internal and external notification expiry #6822

Merged

Conversation

kl
Copy link
Contributor

@kl kl commented Sep 19, 2024


This change is Reviewable

@kl kl added the Android Issues related to Android label Sep 19, 2024
Copy link

linear bot commented Sep 19, 2024

Rawa
Rawa previously approved these changes Sep 20, 2024
Copy link
Contributor

@Rawa Rawa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryNotificationProvider.kt line 62 at r1 (raw file):

    companion object {
        private val REMAINING_TIME_FOR_REMINDERS = Duration.standardDays(3)

I think that we either should reference the other constant or use the same since otherwise it is quite easy to forget that they should be the same.

Copy link
Contributor Author

@kl kl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryNotificationProvider.kt line 62 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I think that we either should reference the other constant or use the same since otherwise it is quite easy to forget that they should be the same.

I agree but ACCOUNT\_EXPIRY\_CLOSE\_TO\_EXPIRY\_THRESHOLD is defined in the main module file AccountExpiryConstant.kt and REMAINING_TIME_FOR_REMINDERS is defined in the service module. So we cannot use the ACCOUNT\_EXPIRY\_CLOSE\_TO\_EXPIRY\_THRESHOLD constant in the AccountExpiryNotificationProvider file. Should we move the AccountExpiryConstant.kt file to the service module and have the use case reference the constants from there?

Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryNotificationProvider.kt line 62 at r1 (raw file):

Previously, kl (Kalle Lindström) wrote…

I agree but ACCOUNT\_EXPIRY\_CLOSE\_TO\_EXPIRY\_THRESHOLD is defined in the main module file AccountExpiryConstant.kt and REMAINING_TIME_FOR_REMINDERS is defined in the service module. So we cannot use the ACCOUNT\_EXPIRY\_CLOSE\_TO\_EXPIRY\_THRESHOLD constant in the AccountExpiryNotificationProvider file. Should we move the AccountExpiryConstant.kt file to the service module and have the use case reference the constants from there?

I think that is fine. Ideally we should have some common constant module similar to the model module, but that is beyond the scope of this PR.

@kl kl force-pushed the align-notification-threshold-inapp-android-notification-droid-1347 branch from e52d569 to 60f44df Compare September 25, 2024 08:42
Copy link
Contributor Author

@kl kl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @Pururun and @Rawa)


android/service/src/main/kotlin/net/mullvad/mullvadvpn/service/notifications/accountexpiry/AccountExpiryNotificationProvider.kt line 62 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I think that is fine. Ideally we should have some common constant module similar to the model module, but that is beyond the scope of this PR.

Done.

@kl kl force-pushed the align-notification-threshold-inapp-android-notification-droid-1347 branch from 60f44df to d798035 Compare September 25, 2024 08:53
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Rawa Rawa force-pushed the align-notification-threshold-inapp-android-notification-droid-1347 branch from d798035 to 808d6c9 Compare September 25, 2024 09:24
@Rawa Rawa force-pushed the align-notification-threshold-inapp-android-notification-droid-1347 branch from 808d6c9 to cc357be Compare September 25, 2024 14:13
@Rawa Rawa merged commit 21361ee into main Sep 25, 2024
25 checks passed
@Rawa Rawa deleted the align-notification-threshold-inapp-android-notification-droid-1347 branch September 25, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants