-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
In-App Updates: Prompt update after phased release is complete #23218
Conversation
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
4c09a91
to
89e1488
Compare
d5afcc7
to
4a68936
Compare
@@ -24,7 +25,8 @@ final class AppUpdateCoordinator { | |||
remoteConfigStore: RemoteConfigStore = RemoteConfigStore(), | |||
isJetpack: Bool = AppConfiguration.isJetpack, | |||
isLoggedIn: Bool = AccountHelper.isLoggedIn, | |||
isInAppUpdatesEnabled: Bool = RemoteFeatureFlag.inAppUpdates.enabled() | |||
isInAppUpdatesEnabled: Bool = RemoteFeatureFlag.inAppUpdates.enabled(), | |||
delayInDays: Int = 7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we delay it by more days, just to be safe? I'd probably go with a month to let users a chance to update manually and to make sure the automatic update has enough time to run. I don't think we need to start bombarding them with the updates right away, do we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This value doesn't represent how many days to wait in between showing prompts.
It represents how many days to wait until a version has been released to show a prompt to update. I chose 7 days because phased release is complete after 7 days. This delay ensures that the prompt isn't shown during the phased release period - it prevents showing the prompt to users who have automatic updates enabled.
As discussed in p1716217030223529-slack-C072JBZL84U, my plan is change the flexible update interval remote config value from 5 to 90 days.
func currentVersionHasBeenReleased(for days: Int) -> Bool { | ||
let secondsInDay: TimeInterval = 86_400 | ||
let secondsSinceRelease = -currentVersionReleaseDate.timeIntervalSinceNow | ||
return secondsSinceRelease > Double(days) * secondsInDay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) there is a convenience API for this:
guard let days = Calendar.current.dateComponents([.day], from: lhs, to: rhs).day else {
return wpAssertionFailure("should-never-happen")
}
return days > 1
warning: I have not tested it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed this in #23221 🙇♀️
Part of https://github.com/Automattic/wordpress-mobile/issues/55
Part of https://github.com/Automattic/wordpress-mobile/issues/56
Description
How to test
Flexible update
Preconditions
In-App Updates
remote feature flagTest 1.1
Test 1.2
delayInDays
default value to 1Blocking update
Preconditions
In-App Updates
remote feature flagIn-App Update Blocking Version
remote config value to 24.7Test 2.1
Test 2.2
delayInDays
default value to 1Regression Notes
Potential unintended areas of impact
Showing app update prompt
What I did to test those areas of impact (or what existing automated tests I relied on)
Added unit test
What automated tests I added (or what prevented me from doing so)
AppUpdateCoordinatorTests
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: