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

Change account expiry text to display 'less than a day' #5391

Conversation

rablador
Copy link
Contributor

@rablador rablador commented Nov 2, 2023


This change is Reviewable

@rablador rablador added bug iOS Issues related to iOS labels Nov 2, 2023
Copy link

linear bot commented Nov 2, 2023

IOS-368 Fix "Less than a day left" text on account expiry warning

Supposedly the you-are-running-out-of-time warning banner on the main view (shown when the account has less than three days left) is inconsistent/wrong on iOS. It shows exact time left even when it should show "Less than a day left".

Follow specified design as written down last time, something went wrong. IOS-198

@rablador rablador marked this pull request as ready for review November 2, 2023 09:41
Copy link
Contributor

@pronebird pronebird 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.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@pinkisemils
Copy link
Collaborator

Would you mind adding a screenshot of how it looks now? 🙂

@rablador rablador force-pushed the fix-less-than-a-day-left-text-on-account-expiry-warning-ios-368 branch 2 times, most recently from c976d9c to c396cee Compare November 3, 2023 15:33
Copy link
Contributor Author

@rablador rablador left a comment

Choose a reason for hiding this comment

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

IMG_3570.jpeg
IMG_3569.jpeg

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @pronebird)

Copy link
Contributor

@buggmagnet buggmagnet 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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @pronebird and @rablador)


ios/MullvadVPN/Notifications/Notification Providers/AccountExpiryInAppNotificationProvider.swift line 41 at r2 (raw file):

    var notificationDescriptor: InAppNotificationDescriptor? {
        guard

Screenshot 2023-11-06 at 17.03.22.png
This is what I get when I log with an account that doesn't have time. It looks like the condition that was there previously was necessary.

Let's add tests to :

  • Make sure we don't repeat that mistake again next time we modify this
  • Make sure the behaviour is well defined

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r2.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @rablador)


ios/MullvadVPN/Classes/CustomDateComponentsFormatting.swift line 32 at r2 (raw file):

        guard dateComponents.day != nil else {
            return NSLocalizedString(
                "ACCOUNT_EXPIRY_INAPP_NOTIFICATION_LESS_THAN_ONE_DAY",

Maybe the text identifier should better reflect where it comes from?

@rablador rablador force-pushed the fix-less-than-a-day-left-text-on-account-expiry-warning-ios-368 branch from c396cee to 70f72cd Compare November 7, 2023 14:34
Copy link
Contributor Author

@rablador rablador 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, 2 unresolved discussions (waiting on @buggmagnet and @pronebird)


ios/MullvadVPN/Classes/CustomDateComponentsFormatting.swift line 32 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Maybe the text identifier should better reflect where it comes from?

Indeed


ios/MullvadVPN/Notifications/Notification Providers/AccountExpiryInAppNotificationProvider.swift line 41 at r2 (raw file):

Previously, buggmagnet wrote…

Screenshot 2023-11-06 at 17.03.22.png
This is what I get when I log with an account that doesn't have time. It looks like the condition that was there previously was necessary.

Let's add tests to :

  • Make sure we don't repeat that mistake again next time we modify this
  • Make sure the behaviour is well defined

Done.

@rablador rablador force-pushed the fix-less-than-a-day-left-text-on-account-expiry-warning-ios-368 branch from 70f72cd to df9a89c Compare November 7, 2023 14:58
@mojganii mojganii requested a review from pronebird November 7, 2023 15:27
Copy link
Collaborator

@mojganii mojganii 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, 4 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)


ios/MullvadVPN/Classes/CustomDateComponentsFormatting.swift line 47 at r3 (raw file):

    }

    private static func isLessThanADayLeft(dateComponents: DateComponents) -> Bool {

nit: I would suggest of renaming that to isLessThanOneDay

Code snippet:

fileprivate extension DateComponents {
    var isLessThanOneDayLeft : Bool {
        let year = self.year ?? 0
        let day = self.day ?? 0

        return (year == 0) && (day == 0)
    }
}

ios/MullvadVPN/Notifications/Notification Providers/AccountExpiryInAppNotificationProvider.swift line 33 at r4 (raw file):

            let triggerDate,
            let duration = CustomDateComponentsFormatting.localizedString(
                from: Date(),

it's better to consider the now constant you've defined above

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r3.
Reviewable status: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @rablador)

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @rablador)

@buggmagnet buggmagnet requested a review from mojganii November 8, 2023 10:45
Copy link
Contributor

@buggmagnet buggmagnet 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, 4 unresolved discussions (waiting on @mojganii and @rablador)


ios/MullvadVPN/Classes/CustomDateComponentsFormatting.swift line 47 at r3 (raw file):

Previously, mojganii wrote…

nit: I would suggest of renaming that to isLessThanOneDay

The current name is fine


ios/MullvadVPN/Notifications/Notification Providers/AccountExpiryInAppNotificationProvider.swift line 13 at r4 (raw file):

import MullvadTypes

struct AccountExpiry {

Let's put this in a separate file


ios/MullvadVPN/Notifications/Notification Providers/AccountExpiryInAppNotificationProvider.swift line 19 at r4 (raw file):

        guard let expiryDate else { return nil }

        return Calendar.current.date(

It's better to use autoupdatingCurrent to track user made time changes.
...
Although considering we use current everywhere else, I will create a janitor task instead to change all references to autoupdatingCurrent

@rablador rablador force-pushed the fix-less-than-a-day-left-text-on-account-expiry-warning-ios-368 branch from df9a89c to 7d5ed04 Compare November 8, 2023 12:44
Copy link
Contributor Author

@rablador rablador 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: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @buggmagnet, @mojganii, and @pronebird)


ios/MullvadVPN/Classes/CustomDateComponentsFormatting.swift line 47 at r3 (raw file):

Previously, buggmagnet wrote…

The current name is fine

I'll change the name, but keep the function in CustomDateComponentsFormatting since it's probably only relevant in this context.


ios/MullvadVPN/Notifications/Notification Providers/AccountExpiryInAppNotificationProvider.swift line 13 at r4 (raw file):

Previously, buggmagnet wrote…

Let's put this in a separate file

Done.


ios/MullvadVPN/Notifications/Notification Providers/AccountExpiryInAppNotificationProvider.swift line 19 at r4 (raw file):

Previously, buggmagnet wrote…

It's better to use autoupdatingCurrent to track user made time changes.
...
Although considering we use current everywhere else, I will create a janitor task instead to change all references to autoupdatingCurrent

I'll keep it as-is then.


ios/MullvadVPN/Notifications/Notification Providers/AccountExpiryInAppNotificationProvider.swift line 33 at r4 (raw file):

Previously, mojganii wrote…

it's better to consider the now constant you've defined above

Done.

Copy link
Contributor

@buggmagnet buggmagnet 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: 2 of 6 files reviewed, 5 unresolved discussions (waiting on @mojganii, @pronebird, and @rablador)


ios/MullvadVPN.xcodeproj/project.pbxproj line 440 at r5 (raw file):

		7A6B4F592AB8412E00123853 /* TunnelMonitorTimings.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A6B4F582AB8412E00123853 /* TunnelMonitorTimings.swift */; };
		7A6F2FA52AFA3CB2006D0856 /* AccountExpiryTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A6F2FA42AFA3CB2006D0856 /* AccountExpiryTests.swift */; };
		7A6F2FA72AFBB9AE006D0856 /* AccountExpiry.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A6F2FA62AFBB9AE006D0856 /* AccountExpiry.swift */; };

You forgot to make AccountExpiry part of the MullvadVPNTests target, the tests don't compile anymore

Copy link
Collaborator

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 of 6 files reviewed, 2 unresolved discussions (waiting on @pronebird and @rablador)

@buggmagnet buggmagnet force-pushed the fix-less-than-a-day-left-text-on-account-expiry-warning-ios-368 branch from 7d5ed04 to d3ce67f Compare November 10, 2023 12:09
@buggmagnet buggmagnet force-pushed the fix-less-than-a-day-left-text-on-account-expiry-warning-ios-368 branch from d3ce67f to 5b24541 Compare November 10, 2023 12:09
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 5 files at r3, 3 of 4 files at r5.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @mojganii, @pronebird, and @rablador)


ios/MullvadVPN.xcodeproj/project.pbxproj line 440 at r5 (raw file):

Previously, buggmagnet wrote…

You forgot to make AccountExpiry part of the MullvadVPNTests target, the tests don't compile anymore

Went and fixed it after talking with @rablador since it was the only thing blocking this PR.

@buggmagnet buggmagnet merged commit f7a1323 into main Nov 10, 2023
3 of 4 checks passed
@buggmagnet buggmagnet deleted the fix-less-than-a-day-left-text-on-account-expiry-warning-ios-368 branch November 10, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants