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 18.1.0.2 beta (includes beta 1 and 2) plus release notes into develop #17080

Merged
merged 16 commits into from
Aug 25, 2021

Conversation

mokagio
Copy link
Contributor

@mokagio mokagio commented Aug 24, 2021

Includes:

18.1.0.1

  • Enabled the "Recommend app" feature via feature flag by @dvdchr (#17065)

18.1.0.2

Nothing to test, if CI Shows green, we're good.

Regression Notes

  1. Potential unintended areas of impact
    N.A.

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

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


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

dvdchr and others added 8 commits August 23, 2021 17:23
…end-app

Recommend App: Enable feature flag to public
There's clearly a bug in my rough implementation of the logic to decide
whether to commit the Jetpack metadata. Likely due to manually checking
the Git commands text output instead of a more appropriate usage of Git
machine readable output like @AliSoftware suggested here:
#17047 (comment)

These would get deleted anyway or updated with the localizations once
ready upon a new beta creation or release finalization, but I thought it
best to track it here from a workflow consistency point of view.
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 24, 2021

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Aug 24, 2021

You can trigger an installable build for these changes by visiting CircleCI here.

Copy link
Contributor Author

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

  • Version update in .xcconfig
  • Updates to the localization files (.strings and Fastlane metadata) automatically pulled for the new translations that have already been approved in GlotPress
  • Diffs from the PRs that made it into this beta

Notice that all the release_notes.txt were deleted because we don't have translations yet—It's only the first day after the GlotPress upload run.

@mokagio mokagio marked this pull request as ready for review August 24, 2021 07:04
@mokagio mokagio requested a review from a team August 24, 2021 07:21
@mokagio mokagio added this to the 18.1 ❄️ milestone Aug 24, 2021
@mokagio mokagio enabled auto-merge August 24, 2021 07:21
@mokagio
Copy link
Contributor Author

mokagio commented Aug 24, 2021

I enabled auto-merge before seeing if the tests passed because it's dinner time for me and I don't want to forget 🍝

Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

I notice that you didn't regenerate the strings via ios_localize_project as discussed/suggested in p1629722771248800/1629715763.241600-slack-CC7L49W13 after the need for one of the original string copy to be updated – see also https://wordpress.slack.com/archives/C02RQC4LY/p1629710931010500 ; which means this new string, if it was ready, wouldn't be imported in GlotPress.

Is this intentional (i.e. the discussion wasn't settled and the new string not ready yet) or an oversight?

@@ -1 +1 @@
Erstelle deine neue Website
Erstelle eine Website oder ein Blog
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a heads up that this will exceed the 30 char limit. (I remember you fixing it manually last time and asking translators for a fix at the source in some P2 or Slack, but apparently they didn't tackle it yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. This keeps happening. Your prompt was the push I needed to suggest these 30-characters-compliant translations into GlotPress myself. 🤞

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. Let's merge it as is for now and hope the fixed copy gets accepted in GlotPress by the time you'll do the final release then 👍

@@ -1 +1 @@
Construisez un site web
Construisez un site web ou un blog
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@@ -1 +1 @@
Bir web sitesi oluşturun
Bir web sitesi veya blog oluşturun
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

Copy link
Contributor

Choose a reason for hiding this comment

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

image

@peril-wordpress-mobile
Copy link

Messages
📖 This PR has the 'Releases' label: some checks will be skipped.

Generated by 🚫 dangerJS

@mokagio mokagio changed the title Merge 18.1.0.1 beta into develop Merge 18.1.0.2 beta (includes beta 1 and 2) plus release notes into develop Aug 25, 2021
@mokagio
Copy link
Contributor Author

mokagio commented Aug 25, 2021

Thanks @AliSoftware. The localizations not being updated was intentional, as I was waiting fro input on the PR that changed one.

Since your comment, I got that info, merged the PR and shipped a new beta. The diff does reflect that.

Can I ask for another review? 😄 🙏 Thanks!

@mokagio mokagio disabled auto-merge August 25, 2021 04:34
@mokagio mokagio enabled auto-merge August 25, 2021 04:34
Copy link
Contributor Author

@mokagio mokagio left a comment

Choose a reason for hiding this comment

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

  • Version update in .xcconfig
  • Updates to the localization files (.strings and Fastlane metadata) automatically pulled for the new translations that have already been approved in GlotPress
  • Diffs from the PRs that made it into this beta
  • AppStoreStrings.po changed with editorialized release notes

Comment on lines +7100 to +7101
/* Error message displayed when unable to close user account due to having active atomic site. */
"This user account cannot be closed while it has active atomic sites." = "This user account cannot be closed while it has active atomic sites.";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice that this and all the other non-English US localization of this string, match the original version not the new one updated from the change in WordPress/Classes/ViewRelated/Me/Account Settings/AccountSettingsViewController.swift.

That's expected because these files were downloaded GlotPress during the download_localized_strings_and_metadata step of the new_beta_release. Because the change to this string hasn't been merged into develop yet, GlotPress still has the original version.

Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@@ -1 +1 @@
Erstelle deine neue Website
Erstelle eine Website oder ein Blog
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. This keeps happening. Your prompt was the push I needed to suggest these 30-characters-compliant translations into GlotPress myself. 🤞

@mokagio mokagio merged commit 71bb1a7 into develop Aug 25, 2021
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

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

✅ Version bump
✅ strings updated, including the last minute post-code-freeze change
✅ release notes being different for WP and JP and matching the copies suggested by editorial

:shipit:

@@ -288,7 +288,7 @@ private class AccountSettingsController: SettingsController {
return NSLocalizedString("You're not authorized to close the account.",
comment: "Error message displayed when unable to close user account due to being unauthorized.")
case "atomic-site":
return NSLocalizedString("This user account cannot be closed while it has active atomic sites.",
return NSLocalizedString("This user account could not be closed. Please contact us by going to Me > Help & Support screen.",
Copy link
Contributor

@AliSoftware AliSoftware Aug 25, 2021

Choose a reason for hiding this comment

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

Personal note: I'd have said "can't be closed automatically" or something like this, to avoid making the user think that "it won't be possible to close it at all" but instead tell them "you need to contact us so we can do some stuff in our end to close it", which would be less worrying than "can't be closed [at all]" imho 🤷‍♂️

Comment on lines +7100 to +7101
/* Error message displayed when unable to close user account due to having active atomic site. */
"This user account cannot be closed while it has active atomic sites." = "This user account cannot be closed while it has active atomic sites.";
Copy link
Contributor

Choose a reason for hiding this comment

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

👌

@@ -1 +1 @@
Erstelle deine neue Website
Erstelle eine Website oder ein Blog
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. Let's merge it as is for now and hope the fixed copy gets accepted in GlotPress by the time you'll do the final release then 👍

@@ -1 +1 @@
Bir web sitesi oluşturun
Bir web sitesi veya blog oluşturun
Copy link
Contributor

Choose a reason for hiding this comment

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

image

@mokagio mokagio mentioned this pull request Aug 30, 2021
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants