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 validate conditions #28711

Merged
merged 4 commits into from
Feb 14, 2025
Merged

fix: surveys validate conditions #28711

merged 4 commits into from
Feb 14, 2025

Conversation

marandaneto
Copy link
Member

@marandaneto marandaneto commented Feb 14, 2025

Problem

AttributeError
'str' object has no attribute 'get'

https://posthog.sentry.io/issues/6294642235/?alert_rule_id=15777369&alert_type=issue&notification_uuid=1392b11e-fcc6-4661-9fb7-ca780439ee5e&project=1899813

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

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

How did you test this code?

@@ -755,7 +759,7 @@ def summarize_responses(self, request: request.Request, **kwargs):

survey = self.get_object()

cache_key = f'summarize_survey_responses_{self.team.pk}_{self.kwargs["pk"]}'
cache_key = f"summarize_survey_responses_{self.team.pk}_{self.kwargs['pk']}"
Copy link
Member Author

Choose a reason for hiding this comment

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

commit linter did that so I guess its safe

@marandaneto marandaneto requested a review from a team February 14, 2025 09:11
@marandaneto marandaneto marked this pull request as ready for review February 14, 2025 09:11
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR adds validation for survey conditions to prevent runtime errors, specifically addressing a Sentry-reported AttributeError where string values were being incorrectly passed instead of dictionaries.

  • Added validation in posthog/api/survey.py to ensure survey conditions are provided as dictionary objects
  • Fixed potential AttributeError when string values were passed instead of dictionaries for survey conditions
  • Updated survey response summarization cache key formatting for better consistency

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@ioannisj
Copy link
Contributor

I think we probably need a similar check here?

actions = conditions.get("actions")

@marandaneto
Copy link
Member Author

I think we probably need a similar check here?

actions = conditions.get("actions")

I don't think so since those are called manually.
validate_conditions is called by django, so If I guess it right, the validate_conditions already has been executed and thrown if something is off, and the calls to _associate_actions should be ok by this time.

@marandaneto
Copy link
Member Author

@pauldambra adding you master of djangos so you double check my comment here.

@pauldambra
Copy link
Member

@marandaneto @ioannisj looks like the conditions definition is conditions = models.JSONField(blank=True, null=True) so None is a valid value for conditions, no?

and in the validator

    def validate_conditions(self, value):
        if value is None:
            return value

so i think can definitely be None

Comment on lines 211 to 212
if value is None:
return value
Copy link
Member

Choose a reason for hiding this comment

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

so this makes the return type dict | None

posthog/api/survey.py Outdated Show resolved Hide resolved
Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

surgical 👨‍⚕️

Copy link
Contributor

@ioannisj ioannisj left a comment

Choose a reason for hiding this comment

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

Based on the discussion I think this should be 👍

@marandaneto marandaneto enabled auto-merge (squash) February 14, 2025 10:03
@marandaneto marandaneto merged commit 87fad1e into master Feb 14, 2025
96 checks passed
@marandaneto marandaneto deleted the fix/validate-conditions branch February 14, 2025 12:54
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