fix(Surveys): Guard for nil iteration_count when updating survey iterations #23917
Conversation
Phanatic
changed the title
| instance.current_iteration is not None | ||
| and iteration_count is not None | ||
| and instance.current_iteration > iteration_count > 0 |
There was a problem hiding this comment.
Okay, so this was the fix I was going to go with, but the reason I didn't just fix this myself is because this doesn't tell me why these illegal states are possible? Like, if we have a situation where we have a current_iteration but no iteration_count, isn't that something we should make impossible or force-correct? can you tell me more about how this state could be reached?
There was a problem hiding this comment.
This is an invalid state, where the survey previously had recurrence set on it and went to current_iteration of 2.
Then recurrence was disabled and customer tried to set it to be recurring again. Let me write a test for the full scenario.
There was a problem hiding this comment.
| @@ -2346,6 +2346,18 @@ def test_cannot_reduce_iterations_lt_current_iteration(self): | |||
| assert response.status_code == status.HTTP_400_BAD_REQUEST | |||
| assert response.json()["detail"] == "Cannot change survey recurrence to 1, should be at least 2" | |||
|
|
|||
| def test_guards_for_nil_iteration_count(self): | |||
| survey = self._create_recurring_survey() | |||
There was a problem hiding this comment.
this creates a survey with an iteration count, though, right? Shouldn't our test handle the case where we don't have an iteration count and we confirm that the API doesn't fail, since that was the case that failed in production?
…ations (#23917) We weren't guarding for a nil iteration_count when the current_iteration was > 0.
…ations (PostHog#23917) We weren't guarding for a nil iteration_count when the current_iteration was > 0.
Problem
fixes https://posthog.sentry.io/issues/5512240918/events/956d5e7ba790487ebe731103ba98f934/?project=1899813&statsPeriod=14d
Changes
We weren't guarding for a nil
iteration_countwhen thecurrent_iterationwas> 0.👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Automated tests