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

Fix/15322 upgrade WordPress-Android to android 11 api level 30 #15331

Merged
merged 16 commits into from
Oct 7, 2021

Conversation

ravishanker
Copy link
Contributor

@ravishanker ravishanker commented Sep 16, 2021

Fixes #15322

Please see paqN3M-mr-p2 for details on mandatory requirement to upgrade to Android 11 by November 2021. Also this paqN3M-n4-p2, for details on planned approach to upgrade

  • upgrade targetSdkVersion and compileSdkVersion to 30
  • fix and tweak a couple of files

NOTE: The following PRs need to be merged first before this can be merged. Build will fail as well till then. Most of the changes are in stories lib.

To test:

  • camera: taking pictures and recording videos
  • saving: create new pics and videos
  • media: using the media picker (local media, WP media in the WP app, 3rd party app media, remote media).
  • Gutenberg (creating a post from zero, then edit, editing an existing Post created on another device)

Regression Notes

  1. Potential unintended areas of impact

  2. What I did to test those areas of impact (or what existing automated tests I relied on)

  3. What automated tests I added (or what prevented me from doing so)

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Upgrade to android 11 (API level 30)
Tweak for stories related changes
@ravishanker ravishanker added this to the Future milestone Sep 16, 2021
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Sep 16, 2021

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

buildToolVersion = '29.0.2'

minSdkVersion = 24
targetSdkVersion = 29
targetSdkVersion = 30
Copy link
Contributor

@ParaskP7 ParaskP7 Sep 16, 2021

Choose a reason for hiding this comment

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

Suggestion (💡): Since the buildToolVersion is not needed and actually not used anymore, we can remove that as well. Afterwards, with some reordering this might look like:

    minSdkVersion = 24
    compileSdkVersion = 30
    targetSdkVersion = 30

PS: This is the same comment copy-pasted from stories-android.

Copy link
Contributor Author

@ravishanker ravishanker Sep 17, 2021

Choose a reason for hiding this comment

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

  • done, 🙏

@ParaskP7 ParaskP7 self-requested a review September 16, 2021 07:36
@mzorz
Copy link
Contributor

mzorz commented Sep 16, 2021

👋 question, apparently bumping the targetSdk version to 30 involves some more thorough analysis.

Have we done an assessment about the work involved? If so, could you explain? I'm not entirely sure from reading the description in this PR, whether this has been discussed elsewhere and there's a plan in place or not. Please provide details if these exist 🙏. I have later discovered there is actually a plan in paqN3M-n4-p2, which is based on the guidelines provided by the Android documentation on Android 11 migration. It would be nice to provide this in the PR description as well 👍. Or better so, copying the steps here to have everything contained here (as commented here), so this gives context about how this PR fits into your plans right away.

Back to the work at hand, I wonder if scoped Storage is taken into account, it doesn't seem a trivial thing to do 🤔 .

While this is in the works, and just as a preventive measure to add to the Not ready for merge label added here: to avoid potentially bumping the targetSdk to 30 without taking care of that, I'd suggest we make this one a feature branch and convert this PR (which targets develop) to draft - merging this PR by mistake would incur in potential problems with scoped Storage and things I'm not entirely aware of (unless a plan is in place). Wdyt @ravishanker @ParaskP7 @malinajirka ?

@ParaskP7
Copy link
Contributor

👋 @mzorz !

While this is in the works, and just as a preventive measure to add to the Not ready for merge label added here: to avoid potentially bumping the targetSdk to 30 without taking care of that, I'd suggest we make this one a feature branch and convert this PR (which targets develop) to draft - merging this PR by mistake would incur in potential problems with scoped Storage and things I'm not entirely aware of (unless a plan is in place). Wdyt @ravishanker @ParaskP7 @malinajirka ?

I 💯 agree with that.

As I understand, atm @ravishanker is doing all the prep work to understand what is what. As soon as this work is done and given the fact that this will not be completed during this hack week timeframe, which is expected, I suggest this whole endeavour gets to be accompanying by a Project Thread, a master GitHub Issue and a Feature Branch, which will hold all the necessary information about what needs to be done and its progress status.

@mzorz
Copy link
Contributor

mzorz commented Sep 16, 2021

As I understand, atm @ravishanker is doing all the prep work to understand what is what. As soon as this work is done and given the fact that this will not be completed during this hack week timeframe, which is expected, I suggest this whole endeavour gets to be accompanying by a Project Thread, a master GitHub Issue and a Feature Branch, which will hold all the necessary information about what needs to be done and its progress status.

Awesome, thank you for giving me the context I needed @ParaskP7 ! 🙌 😄

@ravishanker ravishanker changed the title Fix/15322 upgrade to android 11 api level 30 Fix/15322 upgrade WordPress-Android to android 11 api level 30 Sep 17, 2021
@ravishanker
Copy link
Contributor Author

👋 @mzorz !

While this is in the works, and just as a preventive measure to add to the Not ready for merge label added here: to avoid potentially bumping the targetSdk to 30 without taking care of that, I'd suggest we make this one a feature branch and convert this PR (which targets develop) to draft - merging this PR by mistake would incur in potential problems with scoped Storage and things I'm not entirely aware of (unless a plan is in place). Wdyt @ravishanker @ParaskP7 @malinajirka ?

I 💯 agree with that.

As I understand, atm @ravishanker is doing all the prep work to understand what is what. As soon as this work is done and given the fact that this will not be completed during this hack week timeframe, which is expected, I suggest this whole endeavour gets to be accompanying by a Project Thread, a master GitHub Issue and a Feature Branch, which will hold all the necessary information about what needs to be done and its progress status.

I've created a project and linked to all this PRs. I'm not sure if this is what you had in mind @ParaskP7. Thanks

@ravishanker
Copy link
Contributor Author

ravishanker commented Sep 17, 2021

👋 question, apparently bumping the targetSdk version to 30 involves some more thorough analysis.

* See this https://developer.android.com/about/versions/11/behavior-changes-all

* and specifically this https://developer.android.com/about/versions/11/privacy/storage

Have we done an assessment about the work involved? If so, could you explain? I'm not entirely sure from reading the description in this PR, whether this has been discussed elsewhere and there's a plan in place or not. Please provide details if these exist 🙏. I have later discovered there is actually a plan in paqN3M-n4-p2, which is based on the guidelines provided by the Android documentation on Android 11 migration. It would be nice to provide this in the PR description as well 👍. Or better so, copying the steps here to have everything contained here (as commented here), so this gives context about how this PR fits into your plans right away.

Back to the work at hand, I wonder if scoped Storage is taken into account, it doesn't seem a trivial thing to do 🤔 .

While this is in the works, and just as a preventive measure to add to the Not ready for merge label added here: to avoid potentially bumping the targetSdk to 30 without taking care of that, I'd suggest we make this one a feature branch and convert this PR (which targets develop) to draft - merging this PR by mistake would incur in potential problems with scoped Storage and things I'm not entirely aware of (unless a plan is in place). Wdyt @ravishanker @ParaskP7 @malinajirka ?

Sounds like plan. Thank you 🙌

  • Created a project board for tracking

Is there anything to be done for Gutenberg? I've not touched Gutenberg at all so far.

removed buildToolVersion
put versions in min, compile and target Sdk order
@mzorz
Copy link
Contributor

mzorz commented Sep 17, 2021

Is there anything to be done for Gutenberg? I've not touched Gutenberg at all so far.

Pinging @hypest here 👋 who would be best to take a look into this? Context: SDK 30 bump on WPAndroid (paqN3M-n4-p2), and Android 11 migration.

@ParaskP7
Copy link
Contributor

👋 @ravishanker !

I've created a project and linked to all this PRs. I'm not sure if this is what you had in mind @ParaskP7. Thanks

Thanks for creating the project, this will definitely help us with tracking the Android 11 upgrade todos and progress! ❤️

In addition to that, I was having in mind a Project Thread, like a P2 post for more visibility (see pbArwn-2zs-p2 as an example), just like we do for every other such project. Also, in addition to the GitHub Project you created (kudos btw), I was thinking of having a parent GitHub Issue, which will hold all the sub-issues and their project (see this as an example). PS: All those are just suggestions, so feel free to pick and apply whatever you think makes sense. 🥇

@hypest
Copy link
Contributor

hypest commented Sep 17, 2021

Pinging @hypest here 👋 who would be best to take a look into this? Context: SDK 30 bump on WPAndroid (paqN3M-n4-p2), and Android 11 migration.

Thanks for the ping @mzorz and for the question @ravishanker ! It'd be cool if the folks currently working on this migration continue to hammer on it including having a look at the Android bits related to gutenberg-mobile and the we can definitely offer assistance. In other words, this is not yet prioritized high for the gb-mobile teams to devote people on but, happy to re-evaluate! In the meantime, feel free to ping me or @mchowning for navigating the gb-mobile of things!

@hypest
Copy link
Contributor

hypest commented Sep 29, 2021

NOTE: The following PRs need to be merged first before this can be merged. Build will fail as well till then. Most of the changes are in stories lib.

Stories
FluxC
Login Flow

👋 @ravishanker , I wonder, to make it easier to develop other deps and testing, what do you think about updating the "pointers" in this PR to point to the API 30 branches on those other deps? Like, is there any particular reason this PR doesn't bump the stories-android git submodule to point to its PR? That will help the build succeed without much manual work on everyone's side. Thanks!

Update, I went ahead and did the submodule bump in the test PR I created for the gb-mobile API 30 work.

@hypest
Copy link
Contributor

hypest commented Sep 30, 2021

👋 @ravishanker, the gutenberg-mobile side of things is ready at this stage. But, we won't merge the Gutenberg side PR before all the API 30 work is ready to merge, to avoid possibly introducing issues in Gutenberg. Let's coordinate the merges so, please give me a ping when everything else is ready too. We'll then merge the Gutenberg PR, the gutenberg-mobile PR and my WPAndroid PR too, what we usually call a "merge domino", typical when working in the gutenberg-mobile project. Let me know if that doesn't make much sense, thanks!

ravishanker and others added 8 commits October 6, 2021 15:53
Updated with develop-# after merging login-flow PR
with develop-# after merging related FluxC branch
…android-api-30

Gutenberg mobile Android target API 30
Update with develop-# after merging Gutenberg and gutenberg-mobile upgrade PRs
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Oct 7, 2021

You can test the changes on this Pull Request by downloading the APKs:

Update with stories-android version
fix :WordPress:lintWordpressVanillaRelease issue
Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

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

Thank you @ravishanker !
I smoke tested the app and after merging the Stories library PR I updated the ext.storiesVersion value in build.gradle to latest commit in develop here: 523d865

IMPORTANT Please note I haven't tested the Login framework changes that seem to be part of this PR introduced in 4fe9cf9, I'm approving this as part of what I have tested only (Stories) 👍

@ravishanker ravishanker merged commit 70a122f into develop Oct 7, 2021
@ravishanker ravishanker deleted the fix/15322-upgrade-to-android-11-api-level-30 branch October 7, 2021 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade WPAndroid to Android 11 (API level 30)
4 participants