From c973c49f88a728ff2543c4cc90ca3a8430b53a87 Mon Sep 17 00:00:00 2001 From: Petrus Asikainen Date: Fri, 29 Dec 2023 21:37:57 +0200 Subject: [PATCH 01/74] Remove .gitattributes that breaks binary files like PNG --- .gitattributes | 2 -- 1 file changed, 2 deletions(-) delete mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes deleted file mode 100644 index 846c8d27..00000000 --- a/.gitattributes +++ /dev/null @@ -1,2 +0,0 @@ -# Force the following filetypes to have unix eols, so Windows does not break them -*.* text eol=lf From f8ef08dafa5f01a787e0b0a9f79ec6fcca902603 Mon Sep 17 00:00:00 2001 From: Petrus Asikainen Date: Mon, 25 Sep 2023 01:01:15 +0300 Subject: [PATCH 02/74] Stop splitting by semicolons (#92) --- packages/ilmomasiina-backend/src/app.ts | 5 ++ .../src/mail/signupConfirmation.ts | 2 +- .../ilmomasiina-backend/src/models/answer.ts | 11 +++- .../models/migrations/0004-answers-to-json.ts | 57 +++++++++++++++++++ .../src/models/migrations/index.ts | 2 + .../src/models/migrations/util.ts | 25 ++++++++ .../src/models/question.ts | 11 +++- .../src/routes/admin/events/createEvent.ts | 2 +- .../src/routes/admin/events/updateEvent.ts | 8 +-- .../src/routes/events/getEventDetails.ts | 17 +----- .../src/routes/signups/getSignupForEdit.ts | 6 +- .../src/routes/signups/updateSignup.ts | 41 ++++++------- .../EditSignup/components/QuestionFields.tsx | 28 ++++----- .../SingleEvent/components/SignupListRow.tsx | 4 +- .../src/utils/signupUtils.ts | 11 +++- .../routes/Editor/components/SignupsTab.tsx | 8 ++- .../ilmomasiina-models/src/models/answer.ts | 2 +- .../ilmomasiina-models/src/models/question.ts | 2 +- .../src/schema/signup/attributes.ts | 10 +++- 19 files changed, 176 insertions(+), 76 deletions(-) create mode 100644 packages/ilmomasiina-backend/src/models/migrations/0004-answers-to-json.ts create mode 100644 packages/ilmomasiina-backend/src/models/migrations/util.ts diff --git a/packages/ilmomasiina-backend/src/app.ts b/packages/ilmomasiina-backend/src/app.ts index 0afbeead..74ba0e46 100644 --- a/packages/ilmomasiina-backend/src/app.ts +++ b/packages/ilmomasiina-backend/src/app.ts @@ -22,6 +22,11 @@ export default async function initApp(): Promise { const server = fastify({ trustProxy: config.isAzure || config.trustProxy, // Get IPs from X-Forwarded-For logger: true, // Enable logger + ajv: { + customOptions: { + coerceTypes: false, // Disable type coercion - we don't need it, and it breaks stuff like anyOf + }, + }, }); // Register fastify-sensible (https://github.com/fastify/fastify-sensible) diff --git a/packages/ilmomasiina-backend/src/mail/signupConfirmation.ts b/packages/ilmomasiina-backend/src/mail/signupConfirmation.ts index 729a47dc..e6b94a40 100644 --- a/packages/ilmomasiina-backend/src/mail/signupConfirmation.ts +++ b/packages/ilmomasiina-backend/src/mail/signupConfirmation.ts @@ -28,7 +28,7 @@ export default async function sendSignupConfirmationMail(signup: Signup) { .filter(([, answer]) => answer) .map(([question, answer]) => ({ label: question.question, - answer: answer!.answer, + answer: Array.isArray(answer!.answer) ? answer!.answer.join(', ') : answer!.answer, })); const edited = answers.some((answer) => answer.createdAt.getTime() !== answer.updatedAt.getTime()); diff --git a/packages/ilmomasiina-backend/src/models/answer.ts b/packages/ilmomasiina-backend/src/models/answer.ts index 2e773389..5ca2cd59 100644 --- a/packages/ilmomasiina-backend/src/models/answer.ts +++ b/packages/ilmomasiina-backend/src/models/answer.ts @@ -12,7 +12,7 @@ export interface AnswerCreationAttributes extends Optional implements AnswerAttributes { public id!: string; - public answer!: string; + public answer!: string | string[]; public questionId!: Question['id']; public question?: Question; @@ -48,6 +48,15 @@ export default function setupAnswerModel(sequelize: Sequelize) { answer: { type: DataTypes.STRING, allowNull: false, + // TODO: Once we upgrade to Sequelize v7, try migrating this to custom datatypes again. + get(): string | string[] { + const json = this.getDataValue('answer'); + return json === null ? null : JSON.parse(json as unknown as string); + }, + set(val: string[] | null) { + const json = val === null ? null : JSON.stringify(val); + this.setDataValue('answer', json as unknown as (string | string[])); + }, }, }, { sequelize, diff --git a/packages/ilmomasiina-backend/src/models/migrations/0004-answers-to-json.ts b/packages/ilmomasiina-backend/src/models/migrations/0004-answers-to-json.ts new file mode 100644 index 00000000..8142a93d --- /dev/null +++ b/packages/ilmomasiina-backend/src/models/migrations/0004-answers-to-json.ts @@ -0,0 +1,57 @@ +import { QueryTypes } from 'sequelize'; + +import { defineMigration } from './util'; + +type RawQuestion = { + id: string; + options: string; +}; + +type RawAnswer = { + id: string; + answer: string; + type: string; +}; + +/* eslint-disable no-await-in-loop */ + +export default defineMigration({ + name: '0004-answers-to-json', + async up({ context: { sequelize, transaction } }) { + const query = sequelize.getQueryInterface(); + // Convert question options to JSON + const questions = await sequelize.query( + 'SELECT `id`, `options` FROM `question`', + { type: QueryTypes.SELECT, transaction }, + ); + for (const row of questions) { + const optionsJson = row.options ? JSON.stringify(row.options.split(';')) : null; + await query.bulkUpdate( + 'question', + { options: optionsJson }, + { id: row.id }, + { transaction }, + ); + } + // Convert answers to JSON + const answers = await sequelize.query( + 'SELECT `answer`.`id`, `answer`.`answer`, `question`.`type` ' + + 'FROM `answer` ' + + 'LEFT JOIN `question` ON `answer`.`questionId` = `question`.`id`', + { type: QueryTypes.SELECT, transaction }, + ); + for (const row of answers) { + // Non-checkbox question -> "entire answer" + // Non-empty answer to checkbox question -> ["ans1", "ans2"] + // Empty answer to checkbox question -> [] + const answer = row.type === 'checkbox' ? row.answer.split(';').filter(Boolean) : row.answer; + const answerJson = JSON.stringify(answer); + await query.bulkUpdate( + 'answer', + { answer: answerJson }, + { id: row.id }, + { transaction }, + ); + } + }, +}); diff --git a/packages/ilmomasiina-backend/src/models/migrations/index.ts b/packages/ilmomasiina-backend/src/models/migrations/index.ts index 805fb206..5260e94a 100644 --- a/packages/ilmomasiina-backend/src/models/migrations/index.ts +++ b/packages/ilmomasiina-backend/src/models/migrations/index.ts @@ -5,12 +5,14 @@ import _0000_initial from './0000-initial'; import _0001_add_audit_logs from './0001-add-audit-logs'; import _0002_add_event_endDate from './0002-add-event-endDate'; import _0003_add_signup_language from './0003-add-signup-language'; +import _0004_answers_to_json from './0004-answers-to-json'; const migrations: RunnableMigration[] = [ _0000_initial, _0001_add_audit_logs, _0002_add_event_endDate, _0003_add_signup_language, + _0004_answers_to_json, ]; export default migrations; diff --git a/packages/ilmomasiina-backend/src/models/migrations/util.ts b/packages/ilmomasiina-backend/src/models/migrations/util.ts new file mode 100644 index 00000000..539e3477 --- /dev/null +++ b/packages/ilmomasiina-backend/src/models/migrations/util.ts @@ -0,0 +1,25 @@ +import { Sequelize, Transaction } from 'sequelize'; +import { RunnableMigration } from 'umzug'; + +export type MigrationContext = { + sequelize: Sequelize; + transaction: Transaction; +}; + +export function defineMigration(migration: RunnableMigration): RunnableMigration { + return { + ...migration, + up: ({ context: sequelize, ...params }) => ( + sequelize.transaction((transaction) => migration.up({ + ...params, + context: { sequelize, transaction }, + })) + ), + down: migration.down && (({ context: sequelize, ...params }) => ( + sequelize.transaction((transaction) => migration.down!({ + ...params, + context: { sequelize, transaction }, + })) + )), + }; +} diff --git a/packages/ilmomasiina-backend/src/models/question.ts b/packages/ilmomasiina-backend/src/models/question.ts index 5154dc02..92560efe 100644 --- a/packages/ilmomasiina-backend/src/models/question.ts +++ b/packages/ilmomasiina-backend/src/models/question.ts @@ -19,7 +19,7 @@ export class Question extends Model ({ ...q, order, - options: (q.options && q.options.length) ? q.options.join(';') : null, + options: q.options?.length ? q : null, })) : [], // add order to quotas quotas: request.body.quotas ? request.body.quotas.map((q, order) => ({ ...q, order })) : [], diff --git a/packages/ilmomasiina-backend/src/routes/admin/events/updateEvent.ts b/packages/ilmomasiina-backend/src/routes/admin/events/updateEvent.ts index 8b56f317..af68b5ab 100644 --- a/packages/ilmomasiina-backend/src/routes/admin/events/updateEvent.ts +++ b/packages/ilmomasiina-backend/src/routes/admin/events/updateEvent.ts @@ -106,16 +106,10 @@ export default async function updateEvent( }; // Update if an id was provided if (question.existing) { - await question.existing.update({ - ...questionAttribs, - // TODO: Splitting by semicolon might cause problems - requires better solution - options: questionAttribs.options ? questionAttribs.options.join(';') : null, - }, { transaction }); + await question.existing.update(questionAttribs, { transaction }); } else { await Question.create({ ...questionAttribs, - // TODO: Splitting by semicolon might cause problems - requires better solution - options: questionAttribs.options ? questionAttribs.options.join(';') : null, eventId: event.id, }, { transaction }); } diff --git a/packages/ilmomasiina-backend/src/routes/events/getEventDetails.ts b/packages/ilmomasiina-backend/src/routes/events/getEventDetails.ts index 6b32427d..acf84b7a 100644 --- a/packages/ilmomasiina-backend/src/routes/events/getEventDetails.ts +++ b/packages/ilmomasiina-backend/src/routes/events/getEventDetails.ts @@ -68,13 +68,6 @@ export async function eventDetailsForUser( throw new NotFound('No event found with id'); } - const questions = event.questions!.map((question) => ({ - ...question.get({ plain: true }), - // Split answer options into array - // TODO: Splitting by semicolon might cause problems - requires better solution - options: question.options ? question.options.split(';') : null, - })); - // Only return answers to public questions const publicQuestions = new Set( event.questions! @@ -97,8 +90,7 @@ export async function eventDetailsForUser( return { ...stringifyDates(event.get({ plain: true })), - questions, - + questions: event.questions!.map((question) => question.get({ plain: true })), quotas: event.quotas!.map((quota) => ({ ...quota.get({ plain: true }), signups: event.signupsPublic // Hide all signups from non-admins if answers are not public @@ -175,12 +167,7 @@ export async function eventDetailsForAdmin( // Admins get a simple result with many columns return stringifyDates({ ...event.get({ plain: true }), - questions: event.questions!.map((question) => ({ - ...question.get({ plain: true }), - // Split answer options into array - // TODO: Splitting by semicolon might cause problems - requires better solution - options: question.options ? question.options.split(';') : null, - })), + questions: event.questions!.map((question) => question.get({ plain: true })), updatedAt: event.updatedAt, quotas: event.quotas!.map((quota) => ({ ...quota.get({ plain: true }), diff --git a/packages/ilmomasiina-backend/src/routes/signups/getSignupForEdit.ts b/packages/ilmomasiina-backend/src/routes/signups/getSignupForEdit.ts index a1ea5fb1..ec1de32f 100644 --- a/packages/ilmomasiina-backend/src/routes/signups/getSignupForEdit.ts +++ b/packages/ilmomasiina-backend/src/routes/signups/getSignupForEdit.ts @@ -54,11 +54,7 @@ export default async function getSignupForEdit( }, event: { ...stringifyDates(event.get({ plain: true })), - questions: event.questions!.map((question) => ({ - ...question.get({ plain: true }), - // Split answer options into array - options: question.options ? question.options.split(';') : null, - })), + questions: event.questions!.map((question) => question.get({ plain: true })), }, }; diff --git a/packages/ilmomasiina-backend/src/routes/signups/updateSignup.ts b/packages/ilmomasiina-backend/src/routes/signups/updateSignup.ts index bf5c3651..7f2e9a0e 100644 --- a/packages/ilmomasiina-backend/src/routes/signups/updateSignup.ts +++ b/packages/ilmomasiina-backend/src/routes/signups/updateSignup.ts @@ -73,16 +73,33 @@ export default async function updateSignup( // Check that all questions are answered with a valid answer const newAnswers = questions.map((question) => { - const answer = request.body.answers + const emptyAnswer = question.type === 'checkbox' ? [] : ''; + let answer = request.body.answers ?.find((a) => a.questionId === question.id) - ?.answer - || ''; + ?.answer; - if (!answer) { + if (!answer || !answer.length) { + // Normalize empty answers if (question.required) { throw new BadRequest(`Missing answer for question ${question.question}`); } + answer = emptyAnswer; + } else if (question.type === 'checkbox') { + // Ensure checkbox answers are arrays + if (!Array.isArray(answer)) { + throw new BadRequest(`Invalid answer to question ${question.question}`); + } + // Check that all checkbox answers are valid + answer.forEach((option) => { + if (!question.options!.includes(option)) { + throw new BadRequest(`Invalid answer to question ${question.question}`); + } + }); } else { + // Don't allow arrays for non-checkbox questions + if (typeof answer !== 'string') { + throw new BadRequest(`Invalid answer to question ${question.question}`); + } switch (question.type) { case 'text': case 'textarea': @@ -95,25 +112,11 @@ export default async function updateSignup( break; case 'select': { // Check that the select answer is valid - const options = question.options!.split(';'); - - if (!options.includes(answer)) { + if (!question.options!.includes(answer)) { throw new BadRequest(`Invalid answer to question ${question.question}`); } break; } - case 'checkbox': { - // Check that all checkbox answers are valid - const options = question.options!.split(';'); - const answers = answer.split(';'); - - answers.forEach((option) => { - if (!options.includes(option)) { - throw new BadRequest(`Invalid answer to question ${question.question}`); - } - }); - break; - } default: throw new Error('Invalid question type'); } diff --git a/packages/ilmomasiina-components/src/routes/EditSignup/components/QuestionFields.tsx b/packages/ilmomasiina-components/src/routes/EditSignup/components/QuestionFields.tsx index b8174155..ff95caf7 100644 --- a/packages/ilmomasiina-components/src/routes/EditSignup/components/QuestionFields.tsx +++ b/packages/ilmomasiina-components/src/routes/EditSignup/components/QuestionFields.tsx @@ -1,7 +1,6 @@ import React, { ReactNode } from 'react'; import { useField } from 'formik'; -import filter from 'lodash/filter'; import find from 'lodash/find'; import reject from 'lodash/reject'; import without from 'lodash/without'; @@ -11,6 +10,7 @@ import { useTranslation } from 'react-i18next'; import type { Question, SignupUpdateBody } from '@tietokilta/ilmomasiina-models'; import { QuestionType } from '@tietokilta/ilmomasiina-models'; import FieldRow from '../../../components/FieldRow'; +import { stringifyAnswer } from '../../../utils/signupUtils'; type Props = { name: string; @@ -25,9 +25,11 @@ const QuestionFields = ({ name, questions, disabled }: Props) => { return ( <> {questions.map((question) => { - const currentAnswer = find(value, { questionId: question.id })?.answer || ''; + const answerValue = find(value, { questionId: question.id })?.answer || ''; + const currentAnswerString = stringifyAnswer(answerValue); + const currentAnswerArray = Array.isArray(answerValue) ? answerValue : []; - function updateAnswer(answer: string) { + function updateAnswer(answer: string | string[]) { setValue(reject(value, { questionId: question.id }).concat({ questionId: question.id, answer, @@ -35,9 +37,8 @@ const QuestionFields = ({ name, questions, disabled }: Props) => { } function toggleChecked(option: string, checked: boolean) { - const currentAnswers = filter(currentAnswer.split(';')); - const newAnswers = checked ? [...currentAnswers, option] : without(currentAnswers, option); - updateAnswer(newAnswers.join(';')); + const newAnswers = checked ? [...currentAnswerArray, option] : without(currentAnswerArray, option); + updateAnswer(newAnswers); } const help = question.public ? t('editSignup.publicQuestion') : null; @@ -51,7 +52,7 @@ const QuestionFields = ({ name, questions, disabled }: Props) => { type="text" required={question.required} readOnly={disabled} - value={currentAnswer} + value={currentAnswerString} onChange={(e) => updateAnswer(e.target.value)} /> ); @@ -62,13 +63,12 @@ const QuestionFields = ({ name, questions, disabled }: Props) => { type="number" required={question.required} readOnly={disabled} - value={currentAnswer} + value={currentAnswerString} onChange={(e) => updateAnswer(e.target.value)} /> ); break; case QuestionType.CHECKBOX: { - const currentAnswers = currentAnswer.split(';'); input = question.options?.map((option, optIndex) => ( { id={`question-${question.id}-option-${optIndex}`} value={option} label={option} - required={question.required && !currentAnswers.some((answer) => answer !== option)} + required={question.required && !currentAnswerArray.some((answer) => answer !== option)} disabled={disabled} - checked={currentAnswers.includes(option)} + checked={currentAnswerArray.includes(option)} onChange={(e) => toggleChecked(option, e.target.checked)} /> )); @@ -94,7 +94,7 @@ const QuestionFields = ({ name, questions, disabled }: Props) => { cols={40} required={question.required} readOnly={disabled} - value={currentAnswer} + value={currentAnswerString} onChange={(e) => updateAnswer(e.target.value)} /> ); @@ -106,7 +106,7 @@ const QuestionFields = ({ name, questions, disabled }: Props) => { as="select" required={question.required} disabled={disabled} - value={currentAnswer} + value={currentAnswerString} onChange={(e) => updateAnswer(e.target.value)} >