For EXP-3625: Hook up getCoenrollingFeatureIds to NimbusBuilder #5718

eliserichards · 2023-07-06T22:10:03Z

https://mozilla-hub.atlassian.net/browse/EXP-3625

Add getCoenrollingFeatureIds() to
- Kotlin:
  - ~~NimbusInterface.kt~~ We don't care about exposing this method here
  - NimbusBuilder.kt
- Swift:
  - ~~NimbusApi.swift~~ Same as above
  - NimbusBuilder.swift

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
- This PR follows the breaking change policy:
  - This PR has no breaking API changes, or
  - There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

eliserichards · 2023-07-06T22:58:47Z

I'm not quite sure if we need these methods on the NimbusInterface.kt/NimbusApi.swift. What do you think @jhugman?

jhugman · 2023-07-07T13:03:36Z

Swift tests are in https://github.com/mozilla/application-services/blob/main/megazords/ios-rust/MozillaTestServices/MozillaTestServicesTests/

There aren’t any specific tests for NimbusBuilderTest.swift — mainly because it is was just so much simpler than the kotlin equivalent (no android components customisation, the fetch/apply listener logic is done by NotificationCenter ). This is likely not sustainable long term. Please could you file a task to test that?

jhugman · 2023-07-07T13:13:53Z

I'm not quite sure if we need these methods on the NimbusInterface.kt/NimbusApi.swift. What do you think @jhugman?

I don't think so. There's no functional reason to have them exposed to the apps.

There are tests for the NimbusClient in Rust, so I'm less inclined to expose the getCoenrollingFeatureIds() in Kotlin and Swift for testing purposes.

jhugman

I think you should likely remove that test, then land this.

jhugman · 2023-07-07T11:55:31Z

components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/NimbusBuilder.kt

-    protected fun getCoenrollingFeatureIds(): List<String> =
-        // This will be changed to use the feature manifest in EXP-3265
-        listOf()
+    protected open fun getCoenrollingFeatureIds(): List<String> =


Micronit: I wondered about this. Should this be overridable by subclasses? I'm not sure whether we should or not.

Suggested change

protected open fun getCoenrollingFeatureIds(): List<String> =

protected fun getCoenrollingFeatureIds(): List<String> =

I personally am leaning towards no considering the FML writes the list out, that being said is there a case where it might be useful for testing (in-apps)?

I personally am leaning towards no considering the FML writes the list out

I'm not sure I understand?

I may have misunderstood what the FML does with the coenrolling feature ids but doesn't it write them out to the feature manifest kt/Swift file and that's the method being called here?

I think I'm leaning towards no, too. Why would we ever want to use this specific method to do anything other than pass along the list of IDs?

jhugman · 2023-07-07T11:56:46Z

components/nimbus/android/src/test/java/org/mozilla/experiments/nimbus/NimbusBuilderTest.kt

+            initialExperiments = bundledExperiments
+        }.build(appInfo)
+        val result = dummyNimbus.getCoenrollingFeatureIds()
+        assertEquals(result, listOf<String>())


What is this testing? There is no featureManifest here, so is this just testing that the we're defaulting to an empty list?

On second thoughts: this test doesn't compile: the NimbusBuilder has getCoenrollingFeatureIds() but not the NimbusInterface object.

I had originally put the getCoenrollingFeatureIds() in the NimbusInterfface, but I agree that we don't really want it there 👍 Removing this test.

eliserichards · 2023-07-07T18:58:34Z

New issue for building out the tests for swift NimbusBuilder: https://mozilla-hub.atlassian.net/browse/EXP-3665

… NimbusBuilder for kotlin

codecov-commenter · 2023-07-07T20:56:50Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (5b8d675) 89.86% compared to head (b56ca11) 89.86%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5718   +/-   ##
=======================================
  Coverage   89.86%   89.86%           
=======================================
  Files           1        1           
  Lines         148      148           
=======================================
  Hits          133      133           
  Misses         15       15

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

jhugman

Nice! There's one nit, but this is good work! Well done!

jhugman · 2023-07-10T16:14:53Z

components/nimbus/ios/Nimbus/NimbusBuilder.swift

@@ -231,8 +231,7 @@ public class NimbusBuilder {
    // swiftlint:enable function_body_length

    func getCoenrollingFeatureIds() -> [String] {
-        // This will be changed to use the feature manifest in EXP-3265
-        []
+        featureManifest != nil ? featureManifest!.getCoenrollingFeatureIds() : []


Nit: ?? is the same as the elvis operator.

Suggested change

featureManifest != nil ? featureManifest!.getCoenrollingFeatureIds() : []

featureManifest?.getCoenrollingFeatureIds() ?? []

Ahhh perfect!

jhugman · 2023-07-10T16:33:48Z

components/nimbus/android/src/main/java/org/mozilla/experiments/nimbus/NimbusBuilder.kt

@@ -187,8 +187,7 @@ abstract class AbstractNimbusBuilder<T : NimbusInterface>(val context: Context)
     * use this to pass into the [NimbusInterface] instance.
     */
    protected fun getCoenrollingFeatureIds(): List<String> =
-        // This will be changed to use the feature manifest in EXP-3265
-        listOf()
+        featureManifest?.getCoenrollingFeatureIds() ?: listOf()


…sBuilder for swift

eliserichards added the nimbus Nimbus, the experimentation platform. label Jul 6, 2023

eliserichards force-pushed the EXP-3625_connect-coenroll-to-builder branch from 6aafe9a to 74d995a Compare July 6, 2023 22:57

eliserichards requested review from jhugman and jeddai July 6, 2023 22:57

eliserichards changed the title ~~For EXP-3625: Hook up getCoenrollingFeatureIds to NimbusInterface and NimbusBuilder~~ For EXP-3625: Hook up getCoenrollingFeatureIds to NimbusBuilder Jul 6, 2023

eliserichards marked this pull request as ready for review July 6, 2023 23:05

jhugman reviewed Jul 7, 2023

View reviewed changes

eliserichards force-pushed the EXP-3625_connect-coenroll-to-builder branch from ceda838 to 7e27321 Compare July 7, 2023 20:25

eliserichards requested a review from jhugman July 7, 2023 20:26

eliserichards force-pushed the EXP-3625_connect-coenroll-to-builder branch from 7e27321 to 4f119a2 Compare July 7, 2023 20:50

For EXP-3625: Hook up getCoenrollingFeatureIds to NimbusInterface and…

741ccbd

… NimbusBuilder for kotlin

eliserichards force-pushed the EXP-3625_connect-coenroll-to-builder branch from 4f119a2 to b56ca11 Compare July 7, 2023 20:50

eliserichards force-pushed the EXP-3625_connect-coenroll-to-builder branch from b56ca11 to a1da763 Compare July 7, 2023 21:30

jhugman approved these changes Jul 10, 2023

View reviewed changes

eliserichards added 3 commits July 10, 2023 11:36

For EXP-3625: Hook up getCoenrollingFeatureIds to NimbusApi and Nimbu…

8e6a308

…sBuilder for swift

For EXP-3625: Changelog

955aaa0

For EXP-3625: Take getCoenrollingFeatureIds out of the interfaces

c2353d0

eliserichards force-pushed the EXP-3625_connect-coenroll-to-builder branch from a1da763 to c2353d0 Compare July 10, 2023 16:37

eliserichards added this pull request to the merge queue Jul 10, 2023

Merged via the queue into mozilla:main with commit 5247bf0 Jul 10, 2023
16 checks passed

eliserichards deleted the EXP-3625_connect-coenroll-to-builder branch July 10, 2023 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For EXP-3625: Hook up getCoenrollingFeatureIds to NimbusBuilder #5718

For EXP-3625: Hook up getCoenrollingFeatureIds to NimbusBuilder #5718

eliserichards commented Jul 6, 2023 •

edited

eliserichards commented Jul 6, 2023

jhugman commented Jul 7, 2023

jhugman commented Jul 7, 2023

jhugman left a comment

jhugman Jul 7, 2023

jeddai Jul 7, 2023 •

edited

jhugman Jul 7, 2023

jeddai Jul 7, 2023

eliserichards Jul 7, 2023

jhugman Jul 7, 2023

jhugman Jul 7, 2023

eliserichards Jul 7, 2023

eliserichards commented Jul 7, 2023

codecov-commenter commented Jul 7, 2023

jhugman left a comment

jhugman Jul 10, 2023

eliserichards Jul 10, 2023

jhugman Jul 10, 2023

	protected open fun getCoenrollingFeatureIds(): List<String> =
	protected fun getCoenrollingFeatureIds(): List<String> =

	featureManifest != nil ? featureManifest!.getCoenrollingFeatureIds() : []
	featureManifest?.getCoenrollingFeatureIds() ?? []

For EXP-3625: Hook up getCoenrollingFeatureIds to NimbusBuilder #5718

For EXP-3625: Hook up getCoenrollingFeatureIds to NimbusBuilder #5718

Conversation

eliserichards commented Jul 6, 2023 • edited

Pull Request checklist

eliserichards commented Jul 6, 2023

jhugman commented Jul 7, 2023

jhugman commented Jul 7, 2023

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeddai Jul 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliserichards commented Jul 7, 2023

codecov-commenter commented Jul 7, 2023

Codecov Report

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eliserichards commented Jul 6, 2023 •

edited

jeddai Jul 7, 2023 •

edited