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

fix(surveys): lt/gt operator error in survey creation #24283

Merged

Conversation

silentninja
Copy link
Contributor

@silentninja silentninja commented Aug 9, 2024

Problem

Fixes #22361

Changes

The query for checking the feature flag conditions failed when conditions employed numeric operators. This occurred because we check the property type before doing the comparison and the annotation needed for the property type check is missing. This PR addresses the issue by adding the missing property type annotations.

Does this work well for both Cloud and self-hosted?

Yes

How did you test this code?

I included test cases to verify the fix in this PR and also did a manual test.

@silentninja silentninja changed the title Fix/feature flag math property error Fix error faced when creating survey which use lt/gt operators for coma numeric properties Aug 9, 2024
@silentninja silentninja changed the title Fix error faced when creating survey which use lt/gt operators for coma numeric properties Fix error faced when creating survey which use lt/gt operators for comparing numeric properties Aug 9, 2024
@silentninja silentninja changed the title Fix error faced when creating survey which use lt/gt operators for comparing numeric properties Fix lt/gt operator error in survey creation Aug 9, 2024
@Twixes Twixes requested a review from a team August 9, 2024 22:40
@dmarticus dmarticus changed the title Fix lt/gt operator error in survey creation fix(surveys): lt/gt operator error in survey creation Aug 9, 2024
Copy link
Contributor

@dmarticus dmarticus left a comment

Choose a reason for hiding this comment

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

great work, thanks for this! Let me see about getting CI passing but it seems like it's some dumb permissions thing that's blocking it.

@dmarticus
Copy link
Contributor

@silentninja Not sure if you say this failing CI job, but it looks like one of the tests is failing still

FAILED ee/billing/test/test_billing_manager.py::TestBillingManager::test_update_billing_organization_users_with_multiple_members - AssertionError: assert equals failed
  [                                [                               
                                   +  {�                             
                                   +    'distinct_id': 'as0M8UVaE0t� 
                                   +0B8DvwIJ983byI4mmFEBRrlLuQU4B4R� 
                                   +h',�                             
                                   +    'email': '[email protected]',�        
                                   +    'role': 8,�                  
                                   +  },�                            
    {                                {                             
      'distinct_id': '2e5NgSciaOB      'distinct_id': '2e5NgSciaOB 
  qfZqGtTSCMsnGzLvomxcLFkJ9MeRvZn  qfZqGtTSCMsnGzLvomxcLFkJ9MeRvZn 
  ',                               ',                              
      'email': '[email protected]',             'email': '[email protected]',        
      'role': 15,                      'role': 15,                 
    },                               },                            
  -  {�                                                              
  -    'distinct_id': 'as0M8UVaE0t�                                  
  -0B8DvwIJ983byI4mmFEBRrlLuQU4B4R�                                  
  -h',�                                                              
  -    'email': '[email protected]',�                                         
  -    'role': 8,�                                                   
  -  },�                                                             
  ]                                ]
========== 1 failed, 656 passed, 5935 deselected in 440.42s (0:07:20) ==========

once you push that fix it should be good to go for us! I'm fixing the other CI failure on my end.

@silentninja
Copy link
Contributor Author

@dmarticus Thanks for the heads up. I ran the tests locally, and they're passing as expected. The CI environment might be experiencing some flakiness with this particular test. Can you try rerunning the test suite?

@dmarticus dmarticus merged commit c3cb02d into PostHog:master Aug 14, 2024
86 checks passed
@dmarticus
Copy link
Contributor

@silentninja it's good to go! thanks for your contribution!

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.

Survey targeting on lt/gt numerical properties not working
3 participants