Skip to content

Conversation

@esrakartalOpt
Copy link
Contributor

@esrakartalOpt esrakartalOpt commented Nov 13, 2025

Summary

  • Update impression event handling
  • Notification for global holdout
  • Created new test cases for the impression event and notification
  • Moved all holdout test cases to new file

Test plan

PR checks

Issues

Copy link
Contributor

@Mat001 Mat001 left a comment

Choose a reason for hiding this comment

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

What Changed in PR #469:

-- decision_service.py - CHANGED
holdouts = project_config.get_holdouts_for_flag(feature.id) # ← Using ID now

-- project_config.py - CHANGED
def get_holdouts_for_flag(self, flag_id: str) -> list[HoldoutDict]:
return self.flag_holdouts_map.get(flag_id, []) # ← Looking up by ID

What DIDN'T Change:

-- project_config.py init - NOT CHANGED (still in codebase)
if applicable_holdouts:
self.flag_holdouts_map[feature.key] = applicable_holdouts # ← Still using KEY!

The Problem:

-- Map is built with KEYS:
flag_holdouts_map = {
"checkout_flag": [holdout1, holdout2], # ← Key = "checkout_flag"
"payment_flow": [holdout3] # ← Key = "payment_flow"
}

-- Code is looking up by IDs:
holdouts = flag_holdouts_map.get("123456", []) # ← ID = "123456"
-- Result: [] (empty!) because "123456" != "checkout_flag"

Impact: Holdouts will NEVER be found. All lookups return empty list. Holdout decisions
will never happen.

@Mat001
Copy link
Contributor

Mat001 commented Nov 14, 2025

Also consistency issue:

  • JavaScript SDK uses flag KEY consistently
  • Python PR changes to flag ID
  • This breaks cross-SDK consistency

@Mat001 Mat001 self-requested a review November 19, 2025 02:44
Copy link
Contributor

@Mat001 Mat001 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 now!

@esrakartalOpt esrakartalOpt merged commit 53421a6 into master Nov 19, 2025
20 checks passed
@esrakartalOpt esrakartalOpt deleted the esra/FSSDK-11572_update_impression_event branch November 19, 2025 14:04
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.

3 participants