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

Merge hotfix release/20.6.1 into current release/20.7 #17136

Merged
merged 16 commits into from
Sep 8, 2022

Conversation

AliSoftware
Copy link
Contributor

@AliSoftware AliSoftware commented Sep 6, 2022

Merging release/20.6.1 back into release/20.7 after doing a hotfix.

The hotfix contained the following fixes:

Warning: A Play Store bug is currently preventing us from submitting the hotfix. [internal ref: pbArwn-5g8-p2]

Since this bug is likely on Google Play Store's side, we don't expect any new commits to be needed on release/20.6.1 to fix it (we'll just resubmit as soon as Google fixed it on their end), so this PR should be good to be reviewed and merged once the conflicts are resolved, even if the hotfix's .aab couldn't be submitted to Play Store yet. And merging it sooner than later would unblock us from preparing the 20.7-rc-2 beta ASAP to be ready when it finally becomes possible to upload .aab on Play Store Console again.

TODO:

  • Submit 20.6.1 for review (once Play Store bug solved)
  • Fix conflicts on this PR (see below; waiting for @ovitrif to chime in on the MeFragment.kt conflict) then undraft this PR
  • Once this PR is merged, we will also need to do a new beta, and bump its versionCode manually (see below)

Conflict Resolution

MeFragment.kt

<<<<<<< merge/20.6.1-into-20.7
            jetpackBadge.root.isVisible = true
            if (jetpackBrandingUtils.shouldShowJetpackPoweredBottomSheet()) {
                jetpackBadge.footerJetpackBadge.jetpackPoweredBadge.setOnClickListener {
                    jetpackBrandingUtils.trackBadgeTapped(ME)
                    viewModel.showJetpackPoweredBottomSheet()
                }
=======
            jetpackBadge.isVisible = true
            jetpackBadge.setOnClickListener {
                jetpackBrandingUtils.trackBadgeTapped(ME)
                viewModel.showJetpackPoweredBottomSheet()
>>>>>>> release/20.7

cc @ovitrif for confirming which resolution to pick for this conflict

build.gradle

<<<<<<< merge/20.6.1-into-20.7
    wordPressLoginVersion = '0.18.0'
    gutenbergMobileVersion = 'v1.81.2'
=======
    wordPressLoginVersion = '0.19.0'
    gutenbergMobileVersion = 'v1.82.0'
>>>>>>> release/20.7

We should pick the lines from release/20.7, but cc @geriux to double-check that a v1.82.1 is in the works to include the bug fix (that landed in 1.81.2), and that v1.82.1 will be shipped in a future beta of WP/JP Android

version.properties

<<<<<<< merge/20.6.1-into-20.7
versionName=20.6.1
versionCode=1268
=======
versionName=20.7-rc-1
versionCode=1267
>>>>>>> release/20.7

We will pick the values from release/20.7 for now, but will have to manually bump versionCode to 1269 (instead of the 1268 that our tooling will bump from seeing 1267 in trunk) when we'll do the new beta once this PR lands.
(There's no point in choosing versionCode=1268 or versionCode=1269 in advance while resolving this conflict, because our tooling we update it based on what's in trunk when we'll run fastlane new_beta_release…)


%%{ init: {
    "gitGraph": { "mainBranchName": "trunk" }
}}%%
gitGraph
       commit id:"20.6 code freeze"
       branch release_20.6
       commit id:"beta fixes…"
       commit id:"finalize_release" tag:"20.6"
       branch release_20.6.1 order:4
       checkout trunk
       commit id:"features…"
       merge release_20.6
       commit id:" "
       branch release_20.7
       checkout release_20.7
       commit id:"code_freeze"
       commit id:"(not published)" tag:"20.7-rc-1" type: REVERSE
       checkout release_20.6.1
       commit id:"fix: #17123"
       commit id:"fix: #17124" tag:"20.6.1 (todo)"
       branch merge_20.6.1-into-20.7
       checkout merge_20.6.1-into-20.7
       commit id:"fix conflict" type: HIGHLIGHT
       checkout release_20.7
       merge merge_20.6.1-into-20.7
       commit id:"(TODO: new beta)" tag:"20.7-rc-2" type: REVERSE
Loading

@AliSoftware AliSoftware added the Releases Label related to managing releases label Sep 6, 2022
@AliSoftware AliSoftware added this to the 20.7 ❄️ milestone Sep 6, 2022
@AliSoftware AliSoftware self-assigned this Sep 6, 2022
@geriux
Copy link
Contributor

geriux commented Sep 6, 2022

cc @geriux to double-check that a v1.82.1 is in the works to include the bug fix (that landed in 1.81.2), and that v1.82.1 will be shipped in a future beta of WP/JP Android

Thanks for the ping, yes a 1.82.1 beta is in the works to include this fix as well. 🙇

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Sep 6, 2022

You can test the WordPress changes on this Pull Request by downloading an installable build (wordpress-installable-build-pr17136-0b40fae.apk), or scanning this QR code:

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Sep 6, 2022

You can test the Jetpack changes on this Pull Request by downloading an installable build (jetpack-installable-build-pr17136-0b40fae.apk), or scanning this QR code:

@ovitrif
Copy link
Contributor

ovitrif commented Sep 7, 2022

MeFragment.kt

<<<<<<< merge/20.6.1-into-20.7
            jetpackBadge.root.isVisible = true
            if (jetpackBrandingUtils.shouldShowJetpackPoweredBottomSheet()) {
                jetpackBadge.footerJetpackBadge.jetpackPoweredBadge.setOnClickListener {
                    jetpackBrandingUtils.trackBadgeTapped(ME)
                    viewModel.showJetpackPoweredBottomSheet()
                }
=======
            jetpackBadge.isVisible = true
            jetpackBadge.setOnClickListener {
                jetpackBrandingUtils.trackBadgeTapped(ME)
                viewModel.showJetpackPoweredBottomSheet()
>>>>>>> release/20.7

cc @ovitrif for confirming which resolution to pick for this conflict

@AliSoftware It's unclear to me why we have jetpackBadge.root.isVisible = true vs jetpackBadge.isVisible = true.

If I look at the changelog of the hotfix PR the changeset is different and some updates are already in. Initially for that PR I created a branch on top of release/20.6.1 before committing changes for the hotfix.

Aside from the unclear thingie, we should pick the first version, i.e. the one with the if (jetpackBrandingUtils.shouldShowJetpackPoweredBottomSheet() condition.

I'll investigate this further on a local merge attempt 👍 .

@ovitrif
Copy link
Contributor

ovitrif commented Sep 7, 2022

MeFragment.kt

<<<<<<< merge/20.6.1-into-20.7
            jetpackBadge.root.isVisible = true
            if (jetpackBrandingUtils.shouldShowJetpackPoweredBottomSheet()) {
                jetpackBadge.footerJetpackBadge.jetpackPoweredBadge.setOnClickListener {
                    jetpackBrandingUtils.trackBadgeTapped(ME)
                    viewModel.showJetpackPoweredBottomSheet()
                }
=======
            jetpackBadge.isVisible = true
            jetpackBadge.setOnClickListener {
                jetpackBrandingUtils.trackBadgeTapped(ME)
                viewModel.showJetpackPoweredBottomSheet()
>>>>>>> release/20.7

I'll investigate this further on a local merge attempt 👍 .

There was a change that updated the same code in this PR: #17095

This change is part of release/20.7 and it wasn't yet in release/20.6.1 on top of which I created the branch for the hotfix.

Practically, this means none of the two options are correct because the code binding to the badge is different on 20.7 → therefore a "manual merge" is required to update the code to fix the references.

the final code of the enclosing if condition should be:

if (jetpackBrandingUtils.shouldShowJetpackBranding()) {
    jetpackBadge.isVisible = true
    if (jetpackBrandingUtils.shouldShowJetpackPoweredBottomSheet()) {
        jetpackBadge.setOnClickListener {
            jetpackBrandingUtils.trackBadgeTapped(ME)
            viewModel.showJetpackPoweredBottomSheet()
        }
    }
}

I'm not sure how we acting in such situations though. This code will only build when merge/20.6.1-into-20.7 is merged into release/20.7.

I've collected all changes in a merge example branch: merge/20.6.1-into-20.7-me-fragment-conflict-fix.
This one should build.

@spencertransier spencertransier marked this pull request as ready for review September 7, 2022 21:53
@AliSoftware
Copy link
Contributor Author

AliSoftware commented Sep 7, 2022

Thanks @ovitrif for checking the conflict and @spencertransier for merging the branch to solve it and mark PR ready.

I'm surprised though that the diff from 0b40fae (merge conflict resolution from @spencertransier ) does not contain all the same things as the comparison of the branch that @ovitrif created in merge/20.6.1-into-20.7...merge/20.6.1-into-20.7-me-fragment-conflict-fix — which also contains some changes in XML files as well in its diff.

I'm not sure if it's just a GitHub glitch in what diff it's actually showing me in each case (and what branches they compare) and just because I'm looking at this from my phone and not digging too much in details so maybe it's just a red herring in practice… or if that's a genuine thing that got missed during the conflict resolution.

My gut feeling would say it's the former (ie red herring and all is ok) — also because @ovitrif only mentioned MeFragment.kt in his comment above but not the XML files I see in his branch's diff — and all is good; but just in case it's the latter and to be on the safe side, it would be nice to get a final approval from @ovitrif that the conflict resolution that landed in 0b40fae didn't miss anything 😅

@AliSoftware
Copy link
Contributor Author

AliSoftware commented Sep 7, 2022

Update: Thinking about it a bit more I think the layout and dimens XML I see in @ovitrif 's link are because it's comparing against the merge branch, while those xml changes are already in the release/20.7 branch so it's normal they don't appear in Spencer commits… strengthening my belief that my comment above is a red herring and result is all ok and as expected after all…?

Copy link
Contributor

@antonis antonis 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 for wrangling this @AliSoftware 🙇
I've tested the generated builds and the apps behave as expected. The code changes also look consistent to me and include all the changes from #17124 🎉
Note that the Gutenberg version upgrade is not included in this PR but this should be ok according to this comment.

@mokagio mokagio merged commit 3e7181e into release/20.7 Sep 8, 2022
@mokagio mokagio deleted the merge/20.6.1-into-20.7 branch September 8, 2022 07:19
@mokagio
Copy link
Contributor

mokagio commented Sep 8, 2022

Merging after @antonis' approval, despite the bit of back and forth on whether the diff is correct. From reading the conversation, I get the feeling that the diff is indeed correct.

All in all, I think it's a safer bet to merge and unblock shipping a new build, rather than waiting any longer for additional confirmation, particularly because we haven't shipped a 20.7 beta yet and we're getting close to half-way through its code freeze cycle.

@ovitrif
Copy link
Contributor

ovitrif commented Sep 8, 2022

Update: Thinking about it a bit more I think the layout and dimens XML I see in @ovitrif 's link are because it's comparing against the merge branch, while those xml changes are already in the release/20.7 branch so it's normal they don't appear in Spencer commits… strengthening my belief that my comment above is a red herring and result is all ok and as expected after all…?

That's true, my example branch was trying to make a buildable intermediate "version" of '20.6.1' that includes the code changes that led to the initial merge conflict; but in reality only the change from MeFragment was needed 👍, everything else was already there in 20.7 release branch.

mokagio added a commit that referenced this pull request Sep 8, 2022
See rationale from
#17136,
namely:

> We will pick the values from release/20.7 for now, but will have to
> manually bump versionCode to 1269 (instead of the 1268 that our tooling
> will bump from seeing 1267 in trunk) when we'll do the new beta once
> this PR lands.
> (There's no point in choosing versionCode=1268 or versionCode=1269 in
> advance while resolving this conflict, because our tooling we update it
> based on what's in trunk when we'll run fastlane new_beta_release…)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Releases Label related to managing releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants