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

[FFM-5103] changed prereq matches from value to identifer #100

Merged
merged 3 commits into from
Nov 7, 2022
Merged

Conversation

laura135
Copy link
Contributor

@laura135 laura135 commented Oct 27, 2022

What:
Updates prerequisite checks so that they compare by flag identifier and not value
Updates feature_test.go so that the boolean flag created for a prerequisite test is inline with UI Rules - variation identifiers should be true/false only.

Why:
Was sending back wrong variation when flag identifier and value were not identical for a prerequisite flag
Was sending back wrong variation when prereq bool flag had variation identifiers that weren't true/false

@swarmia
Copy link

swarmia bot commented Oct 27, 2022

… bool flags on the UI (identifiers for variations must me true/false only)
@laura135 laura135 changed the title changed prereq matches from value to identifer [FFM-5103] changed prereq matches from value to identifer Oct 28, 2022
@@ -546,13 +546,13 @@ func TestFeatureConfig_EvaluateWithPreReqFlags(t *testing.T) {
onBool := Variation{
Name: stringPtr("On"),
Value: "true",
Identifier: "on",
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this change essential for the test to pass?
If so it makes me think we still have an issue somewhere where we are comparing value to identifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@davejohnston davejohnston left a comment

Choose a reason for hiding this comment

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

In general the change looks good, but the update to the test makes me think we have another place in code where we are comparing values too identifiers.
Let me know what you think.

@laura135
Copy link
Contributor Author

laura135 commented Nov 3, 2022

Not so much an issue with comparing for this one - it's more on how we create boolean flags for the sdk versus the UI. The UI locks it down so that identifiers for variation can only be true or false but creating via api doesn't.

Before the change this test would have compared Values which were true/false. After the change this test compares with identifiers which were on/off before I change them) so I think it's running up against whatever rule we have that means we can only have the true/false id for variations when creating a flag on the UI

@laura135
Copy link
Contributor Author

laura135 commented Nov 3, 2022

But in looking I did see that we had another place where we compared values to identifiers for nested prereqs on line 568 so have updated that too

@laura135 laura135 merged commit c6f823f into main Nov 7, 2022
@erdirowlands erdirowlands deleted the FFM-5103 branch November 3, 2023 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants