Support Remote Config managed value types - #1164
Conversation
Sorry, something went wrong.
Added support for Rollouts, Experiments, and Personalization value types in the Remote Config API. - Updated `ParameterValue` to include `RolloutValue`, `PersonalizationValue`, and `ExperimentValue` subclasses. - Added factory methods `rollout`, `personalization`, and `experiment` to `ParameterValue`. - Updated `TemplateResponse` to parse `rolloutValue`, `personalizationValue`, and `experimentValue` fields from the backend response. - Added unit tests for the new value types.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back! For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
Sorry, something went wrong.
ashish-kothari
left a comment
There was a problem hiding this comment.
Apart from the specific file comments, let's also add tests to -
- ParameterTest.java to validate the correctness of the new types
- FirebaseRemoteConfigTest.java to validate that the template is parsed correctly when it contains these managed values.
Sorry, something went wrong.
| * @param percent The percentage of the rollout. | ||
| * @return A {@link ParameterValue.RolloutValue} instance. | ||
| */ | ||
| public static RolloutValue rollout(String rolloutId, String value, double percent) { |
There was a problem hiding this comment.
Rename this to ofRollout(...).
Sorry, something went wrong.
There was a problem hiding this comment.
Done. Renamed rollout to ofRollout.
Sorry, something went wrong.
| * @param personalizationId The personalization ID. | ||
| * @return A {@link ParameterValue.PersonalizationValue} instance. | ||
| */ | ||
| public static PersonalizationValue personalization(String personalizationId) { |
There was a problem hiding this comment.
Rename this to ofPersonalization(...).
Sorry, something went wrong.
There was a problem hiding this comment.
Done. Renamed personalization to ofPersonalization.
Sorry, something went wrong.
| * @param variantValues The list of experiment variant values. | ||
| * @return A {@link ParameterValue.ExperimentValue} instance. | ||
| */ | ||
| public static ExperimentValue experiment(String experimentId, |
There was a problem hiding this comment.
Rename this to ofExperiment(...).
Sorry, something went wrong.
There was a problem hiding this comment.
Done. Renamed experiment to ofExperiment.
Sorry, something went wrong.
| @Key("experimentId") | ||
| private String experimentId; | ||
|
|
||
| @Key("experimentVariantValues") |
There was a problem hiding this comment.
The key should be variantValue instead.
Sorry, something went wrong.
There was a problem hiding this comment.
Done. Changed key to variantValue.
Sorry, something went wrong.
- Renamed factory methods in ParameterValue to ofRollout, ofPersonalization, and ofExperiment. - Updated JSON key for experiment variant values to variantValue in TemplateResponse.
| ParameterValue.ExperimentValue experimentValueThree = | ||
| ParameterValue.ofExperiment("experiment_2", ImmutableList.of( | ||
| ExperimentVariantValue.of("variant_1", "value_1") | ||
| )); |
There was a problem hiding this comment.
Add another experiment value with different variant values.
Sorry, something went wrong.
There was a problem hiding this comment.
Added a test case for experiment value with different variant values.
Sorry, something went wrong.
Added a new test case to `ParameterValueTest` to verify that `ExperimentValue` objects with the same ID but different variant values are not equal.
ashish-kothari
left a comment
There was a problem hiding this comment.
Let's also add tests to -
- ParameterTest.java to validate the correctness of the new types
- FirebaseRemoteConfigTest.java to validate that the template is parsed correctly when it contains these managed values.
Sorry, something went wrong.
…ConfigTest - Added `testEqualityWithManagedValues` to `ParameterTest` to verify equality of parameters with `RolloutValue`, `PersonalizationValue`, and `ExperimentValue`. - Added `testGetTemplateWithManagedValues` to `FirebaseRemoteConfigTest` to verify that `getTemplate()` correctly parses managed values.
| "p1", new Parameter().setDefaultValue( | ||
| ParameterValue.ofRollout("rollout_1", "value_1", 10.0)), | ||
| "p2", new Parameter().setDefaultValue( | ||
| ParameterValue.ofPersonalization("personalization_1")) |
There was a problem hiding this comment.
It's not possible to set managed values as default values, use the setConditionalValues() method instead.
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you for the clarification. I have updated the test to use setConditionalValues() for the managed values.
Sorry, something went wrong.
Updated `testGetTemplateWithManagedValues` in `FirebaseRemoteConfigTest` to set managed values (Rollout, Personalization) as conditional values instead of default values, as they are not supported as default values.
Updated the Firebase Admin SDK Remote Config API to support Rollouts, Experiments, and Personalization managed value types by introducing new POJOs in ParameterValue and updating TemplateResponse DTO mapping.
PR created automatically by Jules for task 15537558531219952152 started by @ashish-kothari