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

[NT-1646][NT-1647]:Email verification flow|Skip Link feature flags #1030

Merged
merged 8 commits into from
Nov 3, 2020

Conversation

Arkariang
Copy link
Contributor

@Arkariang Arkariang commented Nov 2, 2020

📲 What

  • Refactor for the current existing ConfigUtils file
  • Added new method to check if a concrete feature flag is enabled or not.
  • Break down pre-existing tests in smaller ones
  • Added new tests for the new method

🛠 How

  • added @file:JvmName("ConfigExtension") as first line for the kotlin extension. That will allow us to use that extansion on a Java class, in the same way we will use an Utils class.

👀 See

  • Actives feature flags in Staging
    Screen Shot 2020-11-03 at 8 45 11 AM

| | |

Story 📖

NT-1646
NT-1647

- tests for the new method `isFeatureFlagEnabled(String)`
- Added method to check if a concrete FeatureFlag is enabled
@Arkariang Arkariang reopened this Nov 3, 2020
@Arkariang Arkariang changed the title NT-1646:verification flow feature flag [NT-1646][NT-1647]:Email verification flow|Skip Link feature flags Nov 3, 2020
Copy link
Contributor

@leighdouglas leighdouglas left a comment

Choose a reason for hiding this comment

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

Made some comments with suggested method names for the tests you added. Otherwise looks great and awesome job figuring out this java/kotlin-extension work! 😄

Pair("ios_go_rewardless", true),
Pair("ios_native_checkout", true)))

assertEquals(JSONArray().apply {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you gave this test three active experiments. Shouldn't we set these other two experiments to false to check to make sure they aren't coming back as apart of the enabledFeatureFlag field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the test is correct, as the other two experiments are for ios, and they are active. That's the scenario is being tested, even though they are active we are interested only in the android ones.

@@ -100,7 +101,7 @@ abstract class TrackingClient(@param:ApplicationContext private val context: Con
}

override fun currentVariants(): JSONArray? {
return ConfigUtils.currentVariants(this.config)
return this.config?.currentVariants()
Copy link
Contributor

Choose a reason for hiding this comment

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

if config is null, what does this return? Null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Config cannot be null, it's not optional. In case that happen it will throw NullPointerException()

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, then why do we need the safe call operator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is optional is the return from currentVariants() that is a JSONArray?

@Arkariang
Copy link
Contributor Author

Made some comments with suggested method names for the tests you added.
All tests renamed! :) thanks for those suggestions @leighdouglas !

@Arkariang Arkariang merged commit 4741263 into master Nov 3, 2020
@Arkariang Arkariang deleted the imartin/NT-1646-Verification-Flow-Feature-Flag branch November 3, 2020 21:05
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.

None yet

2 participants