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.

Sign up for GitHub

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): Guard for nil iteration_count when updating survey iterations #23917

Merged
merged 10 commits into from
Jul 23, 2024
+68 −4
Merged

fix(Surveys): Guard for nil iteration_count when updating survey iterations #23917

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a6a6036
guard for nil iteration_count when updating survey iterations
Phanatic Jul 23, 2024
775e309
Merge branch 'master' into fix-none-comparision
Phanatic Jul 23, 2024
cb0aaa7
Merge branch 'master' into fix-none-comparision
Phanatic Jul 23, 2024
860d552
fix test
Phanatic Jul 23, 2024
bad0bb3
Merge branch 'master' into fix-none-comparision
Phanatic Jul 23, 2024
14d2f5a
Actually reset current_iteration after recurrence is disabled
Phanatic Jul 23, 2024
8b049c5
Merge branch 'master' into fix-none-comparision
dmarticus Jul 23, 2024
c5a24e5
more tests
Phanatic Jul 23, 2024
0b4a287
Merge branch 'master' into fix-none-comparision
dmarticus Jul 23, 2024
e7a0c87
fix statement unreachable error
Phanatic Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion posthog/api/survey.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,11 @@ def update(self, instance: Survey, validated_data):
instance.targeting_flag.save()

iteration_count = validated_data.get("iteration_count")
if instance.current_iteration is not None and instance.current_iteration > iteration_count > 0:
if (
instance.current_iteration is not None
and iteration_count is not None
and instance.current_iteration > iteration_count > 0
Comment on lines +383 to +385
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 14d2f5a#diff-c2c811931c76d29d59ee8c932d3ef20d484ecffe1fa1ca049f6abae7edf437a2R179-R184

):
raise serializers.ValidationError(
f"Cannot change survey recurrence to {iteration_count}, should be at least {instance.current_iteration}"
)
Expand Down
59 changes: 57 additions & 2 deletions posthog/api/test/test_survey.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,7 +958,6 @@ def test_can_list_surveys(self):
response_data = list.json()
assert list.status_code == status.HTTP_200_OK, response_data
survey = Survey.objects.get(team_id=self.team.id)

assert response_data == {
"count": 1,
"next": None,
Expand Down Expand Up @@ -1019,7 +1018,7 @@ def test_can_list_surveys(self):
"responses_limit": None,
"iteration_count": None,
"iteration_frequency_days": None,
"iteration_start_dates": None,
"iteration_start_dates": [],
"current_iteration": None,
"current_iteration_start_date": None,
}
Expand Down Expand Up @@ -2233,6 +2232,27 @@ def _create_recurring_survey(self) -> Survey:
survey = Survey.objects.get(id=response_data["id"])
return survey

def _create_non_recurring_survey(self) -> Survey:
random_id = generate("1234567890abcdef", 10)
response = self.client.post(
f"/api/projects/{self.team.id}/surveys/",
data={
"name": f"Recurring NPS Survey {random_id}",
"description": "Get feedback on the new notebooks feature",
"type": "popover",
"questions": [
{
"type": "open",
"question": "What's a survey?",
}
],
},
)

response_data = response.json()
survey = Survey.objects.get(id=response_data["id"])
return survey

def test_can_create_recurring_survey(self):
survey = self._create_recurring_survey()
response = self.client.patch(
Expand Down Expand Up @@ -2346,6 +2366,41 @@ 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_can_handle_non_nil_current_iteration(self):
survey = self._create_non_recurring_survey()
survey.current_iteration = 2
survey.save()
response = self.client.patch(
f"/api/projects/{self.team.id}/surveys/{survey.id}/",
data={
"start_date": datetime.now() - timedelta(days=1),
},
)
assert response.status_code == status.HTTP_200_OK

def test_guards_for_nil_iteration_count(self):
survey = self._create_recurring_survey()
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

survey.current_iteration = 2
survey.save()
response = self.client.patch(
f"/api/projects/{self.team.id}/surveys/{survey.id}/",
data={
"start_date": datetime.now() - timedelta(days=1),
},
)
assert response.status_code == status.HTTP_200_OK
survey.refresh_from_db()
self.assertIsNone(survey.current_iteration)
response = self.client.patch(
f"/api/projects/{self.team.id}/surveys/{survey.id}/",
data={
"start_date": datetime.now() - timedelta(days=1),
"iteration_count": 3,
"iteration_frequency_days": 30,
},
)
assert response.status_code == status.HTTP_200_OK

def test_can_turn_off_recurring_schedule(self):
survey = self._create_recurring_survey()
response = self.client.patch(
Expand Down
7 changes: 6 additions & 1 deletion posthog/models/feedback/survey.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,12 @@ def update_survey_iterations(sender, instance, *args, **kwargs):
iteration_count = 0 if instance.iteration_count is None else instance.iteration_count
iteration_frequency_dates = 0 if instance.iteration_frequency_days is None else instance.iteration_frequency_days

if instance.iteration_count == 0 or instance.iteration_frequency_days == 0:
if (
instance.iteration_count is None
or instance.iteration_frequency_days is None
or instance.iteration_count == 0
or instance.iteration_frequency_days == 0
):
instance.iteration_start_dates = []
instance.current_iteration = None
instance.current_iteration_start_date = None
Expand Down
Loading