Skip to content

feat: Added getFeatureVariableJson and getAllFeatureVariables #194

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
May 13, 2020
Merged

feat: Added getFeatureVariableJson and getAllFeatureVariables #194

merged 15 commits into from
May 13, 2020

Conversation

fayyazarshad
Copy link
Contributor

@fayyazarshad fayyazarshad commented May 5, 2020 •
edited
Loading

Summary

Implemented getFeatureVariableJson and getAllFeatureVariables

Test plan

1- Tested with FSC
2- Added Unit tests

@fayyazarshad fayyazarshad added the WIP Work in progress label May 5, 2020
@fayyazarshad fayyazarshad self-assigned this May 5, 2020
@coveralls
Copy link

coveralls commented May 5, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.1%) to 98.128% when pulling a5bab1d on fayyaz/feature-json-getFeatureVariableJson into 5ab0d64 on master.

zashraf1985
zashraf1985 reviewed May 5, 2020
View reviewed changes
zashraf1985
zashraf1985 reviewed May 6, 2020
View reviewed changes
@fayyazarshad fayyazarshad changed the title feat: Added a new getFeatureVariableJson Api feat: Added getFeatureVariableJson and getAllFeatureVariables Api's May 7, 2020
@fayyazarshad fayyazarshad force-pushed the fayyaz/feature-json-getFeatureVariableJson branch from e02932e to 6c609d3 Compare May 7, 2020 13:03
@fayyazarshad fayyazarshad changed the title feat: Added getFeatureVariableJson and getAllFeatureVariables Api's feat: Added getFeatureVariableJson Api May 7, 2020
@fayyazarshad fayyazarshad changed the title feat: Added getFeatureVariableJson Api feat: Added getFeatureVariableJson and getAllFeatureVariables May 10, 2020
@fayyazarshad fayyazarshad removed the WIP Work in progress label May 11, 2020
@fayyazarshad fayyazarshad marked this pull request as ready for review May 11, 2020 05:53
@fayyazarshad fayyazarshad requested a review from a team as a code owner May 11, 2020 05:53
@fayyazarshad fayyazarshad removed their assignment May 11, 2020
zashraf1985
zashraf1985 approved these changes May 11, 2020
View reviewed changes
Copy link
Contributor

@zashraf1985 zashraf1985 left a comment

Choose a reason for hiding this comment

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

LGTM - Great Work

oakbani
oakbani approved these changes May 11, 2020
View reviewed changes
Copy link
Contributor

@oakbani oakbani left a comment

Choose a reason for hiding this comment

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

Looks good. Just minor feedback.

oakbani
oakbani reviewed May 11, 2020
View reviewed changes
pawels-optimizely
pawels-optimizely reviewed May 11, 2020
View reviewed changes
Copy link

@pawels-optimizely pawels-optimizely left a comment

Choose a reason for hiding this comment

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

for getAllFeatureVariables I did not see the tests that would get all the variables including json and primitive types, looks like it tests only double_single_variable_feature

pawels-optimizely
pawels-optimizely approved these changes May 12, 2020
View reviewed changes
Copy link

@pawels-optimizely pawels-optimizely left a comment

Choose a reason for hiding this comment

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

looks good, nit: json_single_variable_feature should be more like multiple_variables_feautre

pawels-optimizely
pawels-optimizely reviewed May 12, 2020
View reviewed changes
aliabbasrizvi
aliabbasrizvi reviewed May 12, 2020
View reviewed changes
aliabbasrizvi
aliabbasrizvi reviewed May 13, 2020
View reviewed changes
tests/TestData.php Outdated
},
{
"id": "122252",
"key": "json_invalid_variable",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this invalid? Why not call this json_type_variable or something?

aliabbasrizvi
aliabbasrizvi approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

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

2 more changes recommended. Looks good after that.

@pawels-optimizely pawels-optimizely merged commit 76ef881 into master May 13, 2020
@pawels-optimizely pawels-optimizely deleted the fayyaz/feature-json-getFeatureVariableJson branch May 13, 2020 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
4 more reviewers

@oakbani oakbani oakbani approved these changes

@zashraf1985 zashraf1985 zashraf1985 approved these changes

@pawels-optimizely pawels-optimizely pawels-optimizely approved these changes

@aliabbasrizvi aliabbasrizvi aliabbasrizvi approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@fayyazarshad @coveralls @aliabbasrizvi @oakbani @zashraf1985 @pawels-optimizely @Arshadf