Merge pull request #116 from Tietokilta/fix/answer-semicolons

Stop splitting strings by magic semicolons

packages/ilmomasiina-backend/src/app.ts

@@ -22,6 +22,11 @@ export default async function initApp(): Promise<FastifyInstance> {
   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)

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());

packages/ilmomasiina-backend/src/models/answer.ts

@@ -12,7 +12,7 @@ export interface AnswerCreationAttributes extends Optional<AnswerAttributes, 'id
 
 export class Answer extends Model<AnswerAttributes, AnswerCreationAttributes> 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,

packages/ilmomasiina-backend/src/models/migrations/0000-initial.ts

@@ -1,12 +1,13 @@
-import { DataTypes, Sequelize } from 'sequelize';
-import { RunnableMigration } from 'umzug';
+import { DataTypes } from 'sequelize';
+
+import { defineMigration } from './util';
 
 // Constant from ../randomId
 const RANDOM_ID_LENGTH = 12;
 
-const migration: RunnableMigration<Sequelize> = {
+export default defineMigration({
   name: '0000-initial',
-  async up({ context: sequelize }) {
+  async up({ context: { sequelize, transaction } }) {
     const query = sequelize.getQueryInterface();
     await query.createTable(
       'event',
@@ -97,6 +98,7 @@ const migration: RunnableMigration<Sequelize> = {
           type: DataTypes.DATE,
         },
       },
+      { transaction },
     );
     await query.createTable(
       'quota',
@@ -138,6 +140,7 @@ const migration: RunnableMigration<Sequelize> = {
           type: DataTypes.DATE,
         },
       },
+      { transaction },
     );
     await query.createTable(
       'signup',
@@ -190,6 +193,7 @@ const migration: RunnableMigration<Sequelize> = {
           type: DataTypes.DATE,
         },
       },
+      { transaction },
     );
     await query.createTable(
       'question',
@@ -246,6 +250,7 @@ const migration: RunnableMigration<Sequelize> = {
           type: DataTypes.DATE,
         },
       },
+      { transaction },
     );
     await query.createTable(
       'answer',
@@ -291,6 +296,7 @@ const migration: RunnableMigration<Sequelize> = {
           type: DataTypes.DATE,
         },
       },
+      { transaction },
     );
     await query.createTable(
       'user',
@@ -318,8 +324,7 @@ const migration: RunnableMigration<Sequelize> = {
           allowNull: false,
         },
       },
+      { transaction },
     );
   },
-};
-
-export default migration;
+});

packages/ilmomasiina-backend/src/models/migrations/0001-add-audit-logs.ts

@@ -1,12 +1,13 @@
-import { DataTypes, Sequelize } from 'sequelize';
-import { RunnableMigration } from 'umzug';
+import { DataTypes } from 'sequelize';
+
+import { defineMigration } from './util';
 
 // Constant from ../randomId
 const RANDOM_ID_LENGTH = 12;
 
-const migration: RunnableMigration<Sequelize> = {
+export default defineMigration({
   name: '0001-add-audit-logs',
-  async up({ context: sequelize }) {
+  async up({ context: { sequelize, transaction } }) {
     const query = sequelize.getQueryInterface();
     await query.createTable(
       'auditlog',
@@ -57,8 +58,7 @@ const migration: RunnableMigration<Sequelize> = {
           allowNull: false,
         },
       },
+      { transaction },
     );
   },
-};
-
-export default migration;
+});

packages/ilmomasiina-backend/src/models/migrations/0002-add-event-endDate.ts

@@ -1,18 +1,18 @@
-import { DataTypes, Sequelize } from 'sequelize';
-import { RunnableMigration } from 'umzug';
+import { DataTypes } from 'sequelize';
 
-const migration: RunnableMigration<Sequelize> = {
+import { defineMigration } from './util';
+
+export default defineMigration({
   name: '0002-add-event-endDate',
-  async up({ context: sequelize }) {
+  async up({ context: { sequelize, transaction } }) {
     const query = sequelize.getQueryInterface();
     await query.addColumn(
       'event',
       'endDate',
       {
         type: DataTypes.DATE,
       },
+      { transaction },
     );
   },
-};
-
-export default migration;
+});

packages/ilmomasiina-backend/src/models/migrations/0003-add-signup-language.ts

@@ -1,9 +1,10 @@
-import { DataTypes, Sequelize } from 'sequelize';
-import { RunnableMigration } from 'umzug';
+import { DataTypes } from 'sequelize';
 
-const migration: RunnableMigration<Sequelize> = {
+import { defineMigration } from './util';
+
+export default defineMigration({
   name: '0003-add-signup-language',
-  async up({ context: sequelize }) {
+  async up({ context: { sequelize, transaction } }) {
     const query = sequelize.getQueryInterface();
     await query.addColumn(
       'signup',
@@ -12,8 +13,7 @@ const migration: RunnableMigration<Sequelize> = {
         type: DataTypes.STRING(8),
         allowNull: true,
       },
+      { transaction },
     );
   },
-};
-
-export default migration;
+});

packages/ilmomasiina-backend/src/models/migrations/0004-answers-to-json.ts

@@ -0,0 +1,63 @@
+import { QueryTypes } from 'sequelize';
+
+import { defineMigration } from './util';
+
+type RawQuestion = {
+  id: string;
+  type: 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();
+    // Handle different quoting for Postgres & MySQL
+    const q = ([name]: TemplateStringsArray) => query.quoteIdentifiers(name);
+    // Convert question options to JSON
+    const questions = await sequelize.query<RawQuestion>(
+      `SELECT ${q`id`}, ${q`type`}, ${q`options`} FROM ${q`question`}`,
+      { type: QueryTypes.SELECT, transaction },
+    );
+    for (const row of questions) {
+      let optionsJson = null;
+      if (row.type === 'checkbox' || row.type === 'select') {
+        optionsJson = row.options ? JSON.stringify(row.options.split(';')) : JSON.stringify(['']);
+      }
+      await query.bulkUpdate(
+        'question',
+        { options: optionsJson },
+        { id: row.id },
+        { transaction },
+      );
+    }
+    // Convert answers to JSON
+    const answers = await sequelize.query<RawAnswer>(
+      `SELECT ${q`answer.id`}, ${q`answer.answer`}, ${q`question.type`} `
+      + `FROM ${q`answer`} `
+      + `LEFT JOIN ${q`question`} ON ${q`answer.questionId`} = ${q`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 },
+      );
+    }
+  },
+});

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<Sequelize>[] = [
   _0000_initial,
   _0001_add_audit_logs,
   _0002_add_event_endDate,
   _0003_add_signup_language,
+  _0004_answers_to_json,
 ];
 
 export default migrations;

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<MigrationContext>): RunnableMigration<Sequelize> {
+  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 },
+      }))
+    )),
+  };
+}

packages/ilmomasiina-backend/src/models/question.ts

@@ -19,7 +19,7 @@ export class Question extends Model<QuestionAttributes, QuestionCreationAttribut
   public order!: number;
   public question!: string;
   public type!: QuestionType;
-  public options!: string | null;
+  public options!: string[] | null;
   public required!: boolean;
   public public!: boolean;
 
@@ -74,6 +74,15 @@ export default function setupQuestionModel(sequelize: Sequelize) {
     options: {
       type: DataTypes.STRING,
       allowNull: true,
+      // TODO: Once we upgrade to Sequelize v7, try migrating this to custom datatypes again.
+      get(): string[] {
+        const json = this.getDataValue('options');
+        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('options', json as unknown as string[]);
+      },
     },
     required: {
       type: DataTypes.BOOLEAN,

packages/ilmomasiina-backend/src/routes/admin/events/createEvent.ts

@@ -22,7 +22,7 @@ export default async function createEvent(
         questions: request.body.questions ? request.body.questions.map((q, order) => ({
           ...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 })) : [],

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 });
         }

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 }),