-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[JP Content Migration Flow] Check Wordpress version compatibility #17536
Conversation
📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17536-6dd4699.apk
|
📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17536-6dd4699.apk
|
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.
Thanks for the PR, @antonis . All the scenarios work as expected.
I've left a couple of comments with optional suggestions so you can see it they make sense, but I'm approving it as it is.
@@ -21,4 +21,18 @@ class WordPressPublicData @Inject constructor() { | |||
Wasabi.type -> Wasabi.value | |||
else -> throw IllegalArgumentException("Failed to get Jetpack package ID: build flavor not found.") | |||
} | |||
|
|||
fun currentPackageVersion() = packageManagerWrapper.getPackageInfo(currentPackageId())?.versionName |
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.
Nitpick: would it make sense to add the explicit return type for this public function?
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.
Good point 👍
Added with 398ff12
val rawVersion = currentPackageVersion() | ||
|
||
// Clean app semantic versioning info. E.g 21.2-rc-3 turns to 21.2 | ||
val wordPressVersion = rawVersion?.split("-")?.getOrNull(0) ?: rawVersion ?: "" |
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 was wondering if it makes sense to use RegEx here too? I don't see any problem with this approach, I just wanted to suggest in case you find it more readable.
e.g.
val rawVersion = "21.2-rc-3"
val majorMinorRegex = "^(\\d*)(\\.(\\d*))".toRegex()
val wordPressVersion =
majorMinorRegex.find(rawVersion) // 21.2
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.
That's indeed cleaner. Thank you for the suggestion @RenanLukas 🙇
I've added this with 02fec9c
} | ||
|
||
@Test | ||
fun `Invalid versions should return a null non semantic version`() { |
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.
Nice tests 👍
Generated by 🚫 dangerJS |
Description
This PR adds checks for confirming that the WordPress app is installed and is compatible with the migration flow
To test:
Enable the Jetpack Content Migration Flow
Enable the following flags in the
build.gradle
WordPress app not present
Not eligible WordPress app version
Eligible WordPress app version
minimumSupportedVersion
to21.2
inJetpackAppMigrationFlowUtils.kt
Regression Notes
Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
Added unit tests
PR submission checklist:
RELEASE-NOTES.txt
if necessary.