From 4d123ccadd1e8421e5023ada7671d5f589459e77 Mon Sep 17 00:00:00 2001 From: Soon-Mi Sugihara Date: Sun, 29 Oct 2023 22:11:54 -0400 Subject: [PATCH 01/10] feat(surveys) Add open-ended choices for multiple and single choice surveys --- cypress/e2e/surveys.cy.ts | 54 ++++++ frontend/src/scenes/surveys/EditSurvey.scss | 9 + .../src/scenes/surveys/SurveyAppearance.scss | 26 ++- .../src/scenes/surveys/SurveyAppearance.tsx | 90 ++++++++-- frontend/src/scenes/surveys/SurveyEdit.tsx | 166 ++++++++++++------ frontend/src/scenes/surveys/constants.tsx | 1 + .../src/scenes/surveys/surveyLogic.test.ts | 108 +++++++++++- frontend/src/scenes/surveys/surveyLogic.tsx | 10 +- 8 files changed, 386 insertions(+), 78 deletions(-) diff --git a/cypress/e2e/surveys.cy.ts b/cypress/e2e/surveys.cy.ts index 17805c09a8684..641f9b7a9a2b3 100644 --- a/cypress/e2e/surveys.cy.ts +++ b/cypress/e2e/surveys.cy.ts @@ -165,4 +165,58 @@ describe('Surveys', () => { cy.get('[data-attr=delete-survey]').click() cy.get('.Toastify__toast-body').contains('Survey deleted').should('be.visible') }) + + it('creates a new multiple choice survey with an open-ended choice', () => { + cy.get('h1').should('contain', 'Surveys') + cy.get('[data-attr=new-survey]').click() + cy.get('[data-attr=new-blank-survey]').click() + + // add a multiple choice question with an open-ended question + cy.get('[data-attr=survey-name]').focus().type(name).should('have.value', name) + cy.get('[data-attr="survey-question-type-0"]').click() + cy.contains('Multiple choice select').click() + cy.get('button').contains('Add open-ended choice').click() + + // check default open-ended choice form input and appearance after + // open-ended choice was added + cy.get('.LemonInput__input[value="Other"]') + cy.get('.choice-option').eq(3).contains('Other:') + cy.get('.choice-option').eq(3).find('input[type="text"]').should('have.value', '') + + // typing in open-ended question's appearance automatically checks the + // checkbox + cy.get('.choice-option').eq(3).find('input[type="checkbox"]').should('not.be.checked') + cy.get('.choice-option').eq(3).find('input[type="text"]').type('Outreach') + cy.get('.choice-option').eq(3).find('input[type="checkbox"]').should('be.checked') + + // clicking on open-ended question's appearance label unchecks or checks + // the checkbox + cy.get('.choice-option').eq(3).click() + cy.get('.choice-option').eq(3).find('input[type="checkbox"]').should('not.be.checked') + cy.get('.choice-option').eq(3).click() + cy.get('.choice-option').eq(3).find('input[type="checkbox"]').should('be.checked') + + // removing text in open-ended question's appearance automatically + // unchecks the checkbox + cy.get('.choice-option').eq(3).find('input[type="text"]').clear() + + // open-ended question label doesn't change even if appearance input + // changes + cy.get('.LemonInput__input[value="Other"]') + + // change open-ended choice after the label was added + cy.contains('Choices').parent().find('input[type="text"]').eq(3).clear() + cy.contains('Choices').parent().find('input[type="text"]').eq(3).type('First Choice') + cy.get('.choice-option').eq(3).contains('First Choice:') + + // attempt to create and save survey + cy.get('[data-attr=save-survey]').first().click() + + // after save there should be a launch button + cy.get('button[data-attr="launch-survey"]').should('have.text', 'Launch') + + cy.clickNavMenu('surveys') + cy.get('[data-attr=surveys-table]').should('contain', name) + cy.get(`[data-row-key="${name}"]`).contains(name).click() + }) }) diff --git a/frontend/src/scenes/surveys/EditSurvey.scss b/frontend/src/scenes/surveys/EditSurvey.scss index 2995d06667887..6348a88b03259 100644 --- a/frontend/src/scenes/surveys/EditSurvey.scss +++ b/frontend/src/scenes/surveys/EditSurvey.scss @@ -7,3 +7,12 @@ background: var(--border-light); } } + +.question-choice-open-ended-footer { + position: absolute; + bottom: -5px; + left: 6px; + font-size: 10px; + background-color: white; + padding: 0px 5px; +} diff --git a/frontend/src/scenes/surveys/SurveyAppearance.scss b/frontend/src/scenes/surveys/SurveyAppearance.scss index 32ee384fc1d83..d39b0e4941c77 100644 --- a/frontend/src/scenes/surveys/SurveyAppearance.scss +++ b/frontend/src/scenes/surveys/SurveyAppearance.scss @@ -226,12 +226,13 @@ opacity: 1 !important; } -.multiple-choice-options input[type='checkbox']:checked + label { +.multiple-choice-options input:checked + label { font-weight: bold; + border: 1.5px solid rgba(0, 0, 0); } -.multiple-choice-options input:checked + label { - border: 1.5px solid rgb(0 0 0); +.multiple-choice-options input:checked + label input { + font-weight: bold; } .multiple-choice-options label { @@ -243,6 +244,25 @@ background: white; } +.multiple-choice-options .choice-option-open label { + padding-right: 30px; +} + +.multiple-choice-options .choice-option-open input:disabled + label { + opacity: 0.6; +} + +.multiple-choice-options .choice-option-open label input { + position: relative; + opacity: 1; + outline: 1px solid rgba(0, 0, 0, 0.25); + border-radius: 4px; +} + +.multiple-choice-options .choice-option-open label input:focus { + outline-color: black; +} + .thank-you-message { position: relative; bottom: 0; diff --git a/frontend/src/scenes/surveys/SurveyAppearance.tsx b/frontend/src/scenes/surveys/SurveyAppearance.tsx index f4227ef131cc0..1d894a5e5df91 100644 --- a/frontend/src/scenes/surveys/SurveyAppearance.tsx +++ b/frontend/src/scenes/surveys/SurveyAppearance.tsx @@ -10,7 +10,7 @@ import { BasicSurveyQuestion, LinkSurveyQuestion, } from '~/types' -import { defaultSurveyAppearance } from './constants' +import { defaultSurveyAppearance, QUESTION_CHOICE_OPEN_ENDED_PREFIX } from './constants' import { cancel, check, @@ -526,6 +526,62 @@ export function SurveyRatingAppearance({ ) } +const OpenEndedChoice = ({ + label, + initialChecked, + inputType, +}: { + label: string + initialChecked: boolean + inputType: string + textColor: string +}): JSX.Element => { + const textRef = useRef(null) + const checkRef = useRef(null) + + return ( +
{ + if (checkRef.current && checkRef.current.checked) { + textRef.current?.focus() + } + }} + > + + + {check} +
+ ) +} + export function SurveyMultipleChoiceAppearance({ multipleChoiceQuestion, appearance, @@ -584,18 +640,28 @@ export function SurveyMultipleChoiceAppearance({ /> )}
- {(multipleChoiceQuestion.choices || []).map((choice, idx) => ( -
- + choice.startsWith(QUESTION_CHOICE_OPEN_ENDED_PREFIX) ? ( + - - {check} -
- ))} + ) : ( +
+ + + {check} +
+ ) + )}
diff --git a/frontend/src/scenes/surveys/SurveyEdit.tsx b/frontend/src/scenes/surveys/SurveyEdit.tsx index 2941e0a5576fd..375665167723b 100644 --- a/frontend/src/scenes/surveys/SurveyEdit.tsx +++ b/frontend/src/scenes/surveys/SurveyEdit.tsx @@ -39,6 +39,7 @@ import { defaultSurveyAppearance, SurveyQuestionLabel, SurveyUrlMatchTypeLabels, + QUESTION_CHOICE_OPEN_ENDED_PREFIX, } from './constants' import { FeatureFlagReleaseConditions } from 'scenes/feature-flags/FeatureFlagReleaseConditions' import React from 'react' @@ -462,66 +463,123 @@ export default function SurveyEdit(): JSX.Element { ( choice: string, index: number - ) => ( -
- { - const newChoices = - [...value] - newChoices[ - index - ] = val - onChange( - newChoices - ) - }} - /> + ) => { + const isOpenEnded = + choice.startsWith( + QUESTION_CHOICE_OPEN_ENDED_PREFIX + ) + return ( +
+ { + const newChoices = + [ + ...value, + ] + newChoices[ + index + ] = + (isOpenEnded + ? QUESTION_CHOICE_OPEN_ENDED_PREFIX + : '') + + val + onChange( + newChoices + ) + }} + /> + {isOpenEnded && ( + + open-ended + + )} + + } + size="small" + status="muted" + noPadding + onClick={() => { + const newChoices = + [ + ...value, + ] + newChoices.splice( + index, + 1 + ) + onChange( + newChoices + ) + }} + /> +
+ ) + } + )} +
+ {(value || []).length < 6 && ( + <> + } - size="small" - status="muted" - noPadding + type="secondary" + fullWidth={false} onClick={() => { - const newChoices = - [...value] - newChoices.splice( - index, - 1 - ) - onChange( - newChoices - ) + if (!value) { + onChange([ + '', + ]) + } else { + onChange([ + ...value, + '', + ]) + } }} - /> -
- ) - )} -
- {(value || []).length < 6 && ( - } - type="secondary" - fullWidth={false} - onClick={() => { - if (!value) { - onChange(['']) - } else { - onChange([ - ...value, - '', - ]) + > + Add choice + + } - }} - > - Add choice - + type="secondary" + fullWidth={false} + onClick={() => { + if (!value) { + onChange([ + QUESTION_CHOICE_OPEN_ENDED_PREFIX + + 'Other', + ]) + } else { + onChange([ + ...value, + QUESTION_CHOICE_OPEN_ENDED_PREFIX + + 'Other', + ]) + } + }} + > + Add open-ended + choice + + )}
diff --git a/frontend/src/scenes/surveys/constants.tsx b/frontend/src/scenes/surveys/constants.tsx index b63a8ee82a2a9..69d3349e10513 100644 --- a/frontend/src/scenes/surveys/constants.tsx +++ b/frontend/src/scenes/surveys/constants.tsx @@ -2,6 +2,7 @@ import { FeatureFlagFilters, Survey, SurveyQuestionType, SurveyType, SurveyUrlMa export const SURVEY_EVENT_NAME = 'survey sent' export const SURVEY_RESPONSE_PROPERTY = '$survey_response' +export const QUESTION_CHOICE_OPEN_ENDED_PREFIX = 'OPENlabel=' export const SurveyQuestionLabel = { [SurveyQuestionType.Open]: 'Freeform text', diff --git a/frontend/src/scenes/surveys/surveyLogic.test.ts b/frontend/src/scenes/surveys/surveyLogic.test.ts index 0eb577d4d94a7..f2d39471cd392 100644 --- a/frontend/src/scenes/surveys/surveyLogic.test.ts +++ b/frontend/src/scenes/surveys/surveyLogic.test.ts @@ -4,7 +4,7 @@ import { expectLogic } from 'kea-test-utils' import { useMocks } from '~/mocks/jest' import { Survey, SurveyQuestionType, SurveyType } from '~/types' -const SURVEY: Survey = { +const MULTIPLE_CHOICE_SURVEY: Survey = { id: '018b22a3-09b1-0000-2f5b-1bd8352ceec9', name: 'Multiple Choice survey', description: '', @@ -15,7 +15,7 @@ const SURVEY: Survey = { questions: [ { type: SurveyQuestionType.MultipleChoice, - choices: ['Tutorials', 'Customer case studies', 'Product announcements'], + choices: ['Tutorials', 'Customer case studies', 'Product announcements', 'OPENlabel=Other'], question: 'Which types of content would you like to see more of?', description: '', }, @@ -49,7 +49,52 @@ const SURVEY: Survey = { targeting_flag_filters: undefined, } -describe('survey logic', () => { +const SINGLE_CHOICE_SURVEY: Survey = { + id: '118b22a3-09b1-0000-2f5b-1bd8352ceec9', + name: 'Single Choice survey', + description: '', + type: SurveyType.Popover, + linked_flag: null, + linked_flag_id: null, + targeting_flag: null, + questions: [ + { + type: SurveyQuestionType.SingleChoice, + choices: ['Yes', 'No', 'OPENlabel=Maybe (explain)'], + question: 'Would you like us to continue this feature?', + description: '', + }, + ], + conditions: null, + appearance: { + position: 'right', + whiteLabel: false, + borderColor: '#c9c6c6', + placeholder: '', + backgroundColor: '#eeeded', + submitButtonText: 'Submit', + ratingButtonColor: 'white', + submitButtonColor: 'black', + thankYouMessageHeader: 'Thank you for your feedback!', + displayThankYouMessage: true, + ratingButtonActiveColor: 'black', + }, + created_at: '2023-10-12T06:46:32.113745Z', + created_by: { + id: 1, + uuid: '018aa8a6-10e8-0000-dba2-0e956f7bae38', + distinct_id: 'TGqg9Cn4jLkj9X87oXni9ZPBD6VbOxMtGV1GfJeB5LO', + first_name: 'test', + email: 'test@posthog.com', + is_email_verified: false, + }, + start_date: '2023-10-12T06:46:34.482000Z', + end_date: null, + archived: false, + targeting_flag_filters: undefined, +} + +describe('multiple choice survey logic', () => { let logic: ReturnType beforeEach(() => { @@ -69,6 +114,7 @@ describe('survey logic', () => { results: [ [336, '"Tutorials"'], [312, '"Customer case studies"'], + [20, '"Other choice"'], ], }, ], @@ -79,7 +125,7 @@ describe('survey logic', () => { describe('main', () => { it('post processes return values', async () => { await expectLogic(logic, () => { - logic.actions.loadSurveySuccess(SURVEY) + logic.actions.loadSurveySuccess(MULTIPLE_CHOICE_SURVEY) }).toDispatchActions(['loadSurveySuccess']) await expectLogic(logic, () => { @@ -89,8 +135,58 @@ describe('survey logic', () => { .toMatchValues({ surveyMultipleChoiceResults: { 0: { - labels: ['Tutorials', 'Customer case studies', 'Product announcements'], - data: [336, 312, 0], + labels: ['Tutorials', 'Customer case studies', 'Other choice', 'Product announcements'], + data: [336, 312, 20, 0], + }, + }, + }) + }) + }) +}) + +describe('single choice survey logic', () => { + let logic: ReturnType + + beforeEach(() => { + initKeaTests() + logic = surveyLogic({ id: 'new' }) + logic.mount() + + useMocks({ + get: { + '/api/projects/:team/surveys/': () => [200, { results: [] }], + '/api/projects/:team/surveys/responses_count': () => [200, {}], + }, + post: { + '/api/projects/:team/query/': () => [ + 200, + { + results: [ + ['"Yes"', 101], + ['"Only if I could customize my choices"', 1], + ], + }, + ], + }, + }) + }) + + describe('main', () => { + it('post processes return values', async () => { + await expectLogic(logic, () => { + logic.actions.loadSurveySuccess(SINGLE_CHOICE_SURVEY) + }).toDispatchActions(['loadSurveySuccess']) + + await expectLogic(logic, () => { + logic.actions.loadSurveySingleChoiceResults({ questionIndex: 1 }) + }) + .toFinishAllListeners() + .toMatchValues({ + surveySingleChoiceResults: { + 1: { + labels: ['"Yes"', '"Only if I could customize my choices"'], + data: [101, 1], + total: 102, }, }, }) diff --git a/frontend/src/scenes/surveys/surveyLogic.tsx b/frontend/src/scenes/surveys/surveyLogic.tsx index ecbe515974d7f..1af7a187761a8 100644 --- a/frontend/src/scenes/surveys/surveyLogic.tsx +++ b/frontend/src/scenes/surveys/surveyLogic.tsx @@ -22,7 +22,7 @@ import { dayjs } from 'lib/dayjs' import { eventUsageLogic } from 'lib/utils/eventUsageLogic' import { featureFlagLogic } from 'scenes/feature-flags/featureFlagLogic' import { featureFlagLogic as enabledFlagLogic } from 'lib/logic/featureFlagLogic' -import { defaultSurveyFieldValues, NEW_SURVEY, NewSurvey } from './constants' +import { defaultSurveyFieldValues, QUESTION_CHOICE_OPEN_ENDED_PREFIX, NEW_SURVEY, NewSurvey } from './constants' import { sanitizeHTML } from './utils' import { Scene } from 'scenes/sceneTypes' @@ -334,9 +334,13 @@ export const surveyLogic = kea([ return [r[0], r[1].slice(1, r[1].length - 1)] }) - // Zero-fill + // Zero-fill choices that are not open-ended question.choices.forEach((choice) => { - if (results?.length && !results.some((r) => r[1] === choice)) { + if ( + results?.length && + !choice.startsWith(QUESTION_CHOICE_OPEN_ENDED_PREFIX) && + !results.some((r) => r[1] === choice) + ) { results.push([0, choice]) } }) From 82f080f2745afcf3a7f0703450332e5fc4e4d284 Mon Sep 17 00:00:00 2001 From: Soon-Mi Sugihara Date: Sun, 29 Oct 2023 22:12:23 -0400 Subject: [PATCH 02/10] Validate question choices in backend --- posthog/api/survey.py | 4 ++++ posthog/api/test/test_survey.py | 21 +++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/posthog/api/survey.py b/posthog/api/survey.py index ef3e8c166dac8..70090e575cec3 100644 --- a/posthog/api/survey.py +++ b/posthog/api/survey.py @@ -135,6 +135,10 @@ def validate_questions(self, value): if description and nh3.is_html(description): cleaned_question["description"] = nh3_clean_with_allow_list(description) + choices = raw_question.get("choices") + if choices and not isinstance(choices, list): + raise serializers.ValidationError("Question choices must be a list of strings") + cleaned_questions.append(cleaned_question) return cleaned_questions diff --git a/posthog/api/test/test_survey.py b/posthog/api/test/test_survey.py index 75cd3d1c91e5b..e1b5d3163605b 100644 --- a/posthog/api/test/test_survey.py +++ b/posthog/api/test/test_survey.py @@ -1144,6 +1144,27 @@ def test_validate_malformed_questions_as_array_of_strings(self): assert response.status_code == status.HTTP_400_BAD_REQUEST, response_data assert response_data["detail"] == "Questions must be a list of objects" + def test_validate_malformed_question_choices_as_string(self): + response = self.client.post( + f"/api/projects/{self.team.id}/surveys/", + data={ + "name": "Notebooks beta release survey", + "description": "Get feedback on the new notebooks feature", + "type": "popover", + "questions": [ + { + "question": "this is my question", + "type": "multiple_choice", + "choices": "these are my question choices", + } + ], + }, + format="json", + ) + response_data = response.json() + assert response.status_code == status.HTTP_400_BAD_REQUEST, response_data + assert response_data["detail"] == "Question choices must be a list of strings" + class TestSurveysAPIList(BaseTest, QueryMatchingTest): def setUp(self): From 7f4082771de1083413c765663aef30fb8213b627 Mon Sep 17 00:00:00 2001 From: Soon-Mi Sugihara Date: Fri, 17 Nov 2023 16:16:30 -0500 Subject: [PATCH 03/10] Update open choice behavior and appearance to reflect posthog-js --- frontend/src/scenes/surveys/SurveyAppearance.scss | 12 +++++++++--- frontend/src/scenes/surveys/SurveyAppearance.tsx | 8 +++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/frontend/src/scenes/surveys/SurveyAppearance.scss b/frontend/src/scenes/surveys/SurveyAppearance.scss index d39b0e4941c77..6600e24a630ec 100644 --- a/frontend/src/scenes/surveys/SurveyAppearance.scss +++ b/frontend/src/scenes/surveys/SurveyAppearance.scss @@ -246,6 +246,10 @@ .multiple-choice-options .choice-option-open label { padding-right: 30px; + display: flex; + flex-wrap: wrap; + gap: 8px; + max-width: 100%; } .multiple-choice-options .choice-option-open input:disabled + label { @@ -255,12 +259,14 @@ .multiple-choice-options .choice-option-open label input { position: relative; opacity: 1; - outline: 1px solid rgba(0, 0, 0, 0.25); - border-radius: 4px; + border: 1.5px solid rgba(0, 0, 0, 0.25); + border-radius: 2px; + outline: 0; + flex-grow: 1; } .multiple-choice-options .choice-option-open label input:focus { - outline-color: black; + border-color: rgba(0, 0, 0); } .thank-you-message { diff --git a/frontend/src/scenes/surveys/SurveyAppearance.tsx b/frontend/src/scenes/surveys/SurveyAppearance.tsx index 1d894a5e5df91..d2793227e1acd 100644 --- a/frontend/src/scenes/surveys/SurveyAppearance.tsx +++ b/frontend/src/scenes/surveys/SurveyAppearance.tsx @@ -530,11 +530,13 @@ const OpenEndedChoice = ({ label, initialChecked, inputType, + key, }: { label: string initialChecked: boolean inputType: string textColor: string + key: number }): JSX.Element => { const textRef = useRef(null) const checkRef = useRef(null) @@ -543,23 +545,23 @@ const OpenEndedChoice = ({
{ - if (checkRef.current && checkRef.current.checked) { + if (checkRef.current?.checked || checkRef.current?.disabled) { textRef.current?.focus() } }} > -