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

Offline Mode: Switch to local feature flag #23105

Merged
merged 6 commits into from
Apr 29, 2024

Conversation

momo-ozawa
Copy link
Contributor

Description

  • Switches syncPublishing flag to a local feature flag
  • Adds FeatureFlagRolloutStore to facilitate rolling out the flag to a specified percentage of users

How to test

  • Change FeatureFlag.swift L45 to return FeatureFlagRolloutStore().isRolloutEnabled(for: self) in order to test the percentage rollout mechanism
  • Set a breakpoint on L43 of FeatureFlagRolloutStore.swift
  • Run the app with the syncPublishing rollout percentage set to 1%
  • ✅ The sync publishing feature flag should disabled (highly likely, but not impossible that it's enabled if you make it into the 1% rollout group)
  • When the app hits the breakpoint, note the assigned rollout group and release the breakpoint
  • Run the app with the syncPublishing rollout percentage set to a value higher than assigned rollout group / 10 (e.g. if the assigned group was 120, set the rollout percentage to 13)
  • ✅ The sync publishing feature flag should enabled

Regression Notes

  1. Potential unintended areas of impact
    sync publishing feature flag

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

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

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • 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.

@dangermattic
Copy link
Collaborator

dangermattic commented Apr 26, 2024

2 Warnings
⚠️ View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️ This PR is assigned to the milestone 24.8 ❄️. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 26, 2024

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23105-6c81ca8
Version24.7
Bundle IDorg.wordpress.alpha
Commit6c81ca8
App Center BuildWPiOS - One-Offs #9712
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 26, 2024

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23105-6c81ca8
Version24.7
Bundle IDcom.jetpack.alpha
Commit6c81ca8
App Center Buildjetpack-installable-builds #8756
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

Copy link
Contributor

@kean kean left a comment

Choose a reason for hiding this comment

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

Works well!

/// If the feature flag hasn't been assigned to a rollout group yet, assigns the flag to a group.
///
private func rolloutGroup(for featureFlag: RolloutConfigurableFlag) -> Int {
let key = key(for: featureFlag)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to assign more than one group (one per flag). The same group assignment can be used for all flags. If there is only one group key, it might also be beneficial to cache it in memory for further invocations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! 830970b

/// To set a percentage rollout, return a value between 1 and 100 inclusive.
/// If a percentage rollout isn't applicable for the flag, return nil.
///
var rolloutPercentage: Int? {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Suggestion) I would suggest modeling it using floats 0.0...<1.0 to allow more granularity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! 6c81ca8

@dvdchr dvdchr modified the milestones: 24.8, 24.9 Apr 29, 2024
@dvdchr
Copy link
Contributor

dvdchr commented Apr 29, 2024

Hi @momo-ozawa 👋 , I'm bumping this PR's milestone to 24.9 since I'm starting code freeze. Feel free to re-target this to the release branch if this is a blocker or intended for 24.8.

@kean kean modified the milestones: 24.9, 24.8 ❄️ Apr 29, 2024
@kean kean changed the base branch from trunk to release/24.8 April 29, 2024 11:43
@kean kean self-requested a review April 29, 2024 11:46
@momo-ozawa momo-ozawa merged commit 5d74206 into release/24.8 Apr 29, 2024
27 of 28 checks passed
@momo-ozawa momo-ozawa deleted the task/sync-publishing-feature-flag branch April 29, 2024 14:06
@momo-ozawa
Copy link
Contributor Author

Thanks @dvdchr, I've merged this into release/24.8

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.

5 participants