feat: add context value descriptions (#1496)

* feat: add context value descriptions

* refactor: upcase SQL keywords

* refactor: allow blank descriptions

src/lib/db/context-field-store.ts

@@ -16,12 +16,12 @@ const COLUMNS = [
 ];
 const TABLE = 'context_fields';
 
-const mapRow: (IContextRow) => IContextField = (row) => ({
+const mapRow: (object) => IContextField = (row) => ({
     name: row.name,
     description: row.description,
     stickiness: row.stickiness,
     sortOrder: row.sort_order,
-    legalValues: row.legal_values ? row.legal_values.split(',') : undefined,
+    legalValues: row.legal_values || [],
     createdAt: row.created_at,
 });
 
@@ -52,7 +52,7 @@ class ContextFieldStore implements IContextFieldStore {
             description: data.description,
             stickiness: data.stickiness,
             sort_order: data.sortOrder, // eslint-disable-line
-            legal_values: data.legalValues ? data.legalValues.join(',') : undefined, // eslint-disable-line
+            legal_values: JSON.stringify(data.legalValues || []),
         };
     }
 

src/lib/routes/admin-api/context.test.ts

@@ -103,7 +103,7 @@ test('should create a context field with legal values', () => {
         .send({
             name: 'page',
             description: 'Bla bla',
-            legalValues: ['blue', 'red'],
+            legalValues: [{ value: 'blue' }, { value: 'red' }],
         })
         .set('Content-Type', 'application/json')
         .expect(201);
@@ -137,7 +137,7 @@ test('should not create a context field with duplicate legal values', () => {
         .send({
             name: 'page',
             description: 'Bla bla',
-            legalValues: ['blue', 'blue'],
+            legalValues: [{ value: 'blue' }, { value: 'blue' }],
         })
         .set('Content-Type', 'application/json')
         .expect(400);
@@ -151,7 +151,11 @@ test('should update a context field with new legal values', () => {
         .send({
             name: 'environment',
             description: 'Used target application envrionments',
-            legalValues: ['local', 'stage', 'production'],
+            legalValues: [
+                { value: 'local' },
+                { value: 'stage' },
+                { value: 'production' },
+            ],
         })
         .set('Content-Type', 'application/json')
         .expect(200);

src/lib/services/context-schema.ts

@@ -3,6 +3,11 @@ import { nameType } from '../routes/util';
 
 export const nameSchema = joi.object().keys({ name: nameType });
 
+const legalValueSchema = joi.object().keys({
+    value: joi.string().min(1).max(100).required(),
+    description: joi.string().allow('').allow(null).optional(),
+});
+
 export const contextSchema = joi
     .object()
     .keys({
@@ -11,9 +16,9 @@ export const contextSchema = joi
         legalValues: joi
             .array()
             .allow(null)
-            .unique()
+            .unique((a, b) => a.value === b.value)
             .optional()
-            .items(joi.string().max(100)),
+            .items(legalValueSchema),
         stickiness: joi.boolean().optional().default(false),
     })
     .options({ allowUnknown: false, stripUnknown: true });

src/lib/types/stores/context-field-store.ts

@@ -5,7 +5,12 @@ export interface IContextFieldDto {
     description: string;
     stickiness: boolean;
     sortOrder: number;
-    legalValues?: string[];
+    legalValues?: ILegalValue[];
+}
+
+export interface ILegalValue {
+    value: string;
+    description?: string;
 }
 
 export interface IContextField extends IContextFieldDto {

src/lib/util/validators/constraint-types.test.ts

@@ -1,4 +1,11 @@
 import { validateSemver, validateLegalValues } from './constraint-types';
+import { ILegalValue } from '../../types/stores/context-field-store';
+
+const legalValues: Readonly<ILegalValue[]> = [
+    { value: '100' },
+    { value: '200' },
+    { value: '300' },
+];
 
 test('semver validation should throw with bad format', () => {
     const badSemver = 'a.b.c';
@@ -54,7 +61,6 @@ test('semver validation should fail with leading v', () => {
 
 /* Legal values tests */
 test('should fail validation if value does not exist in single legal value', () => {
-    const legalValues = ['100', '200', '300'];
     const value = '500';
     expect.assertions(1);
 
@@ -68,7 +74,6 @@ test('should fail validation if value does not exist in single legal value', ()
 });
 
 test('should pass validation if value exists in single legal value', () => {
-    const legalValues = ['100', '200', '300'];
     const value = '100';
     expect.assertions(0);
 
@@ -82,7 +87,6 @@ test('should pass validation if value exists in single legal value', () => {
 });
 
 test('should fail validation if one of the values does not exist in multiple legal values', () => {
-    const legalValues = ['100', '200', '300'];
     const values = ['500', '100'];
     expect.assertions(1);
 
@@ -96,7 +100,6 @@ test('should fail validation if one of the values does not exist in multiple leg
 });
 
 test('should pass validation if all of the values exists in legal values', () => {
-    const legalValues = ['100', '200', '300'];
     const values = ['200', '100'];
     expect.assertions(0);
 

src/lib/util/validators/constraint-types.ts

@@ -6,6 +6,7 @@ import {
     constraintStringTypeSchema,
 } from '../../schema/constraint-value-types';
 import BadDataError from '../../error/bad-data-error';
+import { ILegalValue } from '../../types/stores/context-field-store';
 
 export const validateNumber = async (value: unknown): Promise<void> => {
     await constraintNumberTypeSchema.validateAsync(value);
@@ -31,18 +32,22 @@ export const validateDate = async (value: unknown): Promise<void> => {
 };
 
 export const validateLegalValues = (
-    legalValues: string[],
+    legalValues: Readonly<ILegalValue[]>,
     match: string[] | string,
 ): void => {
+    const legalStrings = legalValues.map((legalValue) => {
+        return legalValue.value;
+    });
+
     if (Array.isArray(match)) {
         // Compare arrays to arrays
-        const valid = match.every((value) => legalValues.includes(value));
+        const valid = match.every((value) => legalStrings.includes(value));
         if (!valid)
             throw new BadDataError(
                 `input values are not specified as a legal value on this context field`,
             );
     } else {
-        const valid = legalValues.includes(match);
+        const valid = legalStrings.includes(match);
         if (!valid)
             throw new BadDataError(
                 `${match} is not specified as a legal value on this context field`,

src/migrations/20220411103724-add-legal-value-description.js

@@ -0,0 +1,56 @@
+'use strict';
+
+exports.up = function (db, cb) {
+    db.runSql(
+        `
+        -- 1. Convert '1,2,3'::text to "1,2,3"::json
+        ALTER TABLE context_fields
+        ALTER COLUMN legal_values SET DATA TYPE json USING to_json(legal_values);
+
+        -- 2. Convert "1,2,3"::json to [{"value":"1"}, ...]::json.
+        WITH sub AS (
+            SELECT a.name, b.json AS legal_values
+            FROM context_fields AS a
+            LEFT JOIN (
+                SELECT name, json_agg(json_build_object('value', value)) AS json
+                FROM context_fields
+                CROSS JOIN unnest(string_to_array(legal_values #>> '{}', ',')) AS value
+                GROUP BY name
+            ) AS b ON a.name = b.name
+        )
+        UPDATE context_fields SET legal_values = sub.legal_values
+        FROM sub
+        WHERE context_fields.name = sub.name;
+        `,
+        cb,
+    );
+};
+
+exports.down = function (db, cb) {
+    db.runSql(
+        `
+        -- 1. Revert []::json to null.
+        UPDATE context_fields SET legal_values = null
+        WHERE legal_values::jsonb = '[]'::jsonb;
+
+        -- 2. Revert [{"value":"1"}, ...]::json to "1,2,3"::json.
+        WITH sub AS (
+            SELECT name, string_agg(json->>'value', ',') AS legal_values
+            FROM context_fields
+            CROSS JOIN json_array_elements(legal_values) AS json
+            GROUP BY name
+        )
+        UPDATE context_fields SET legal_values = to_json(sub.legal_values)
+        FROM sub
+        WHERE context_fields.name = sub.name;
+
+        -- 3. Revert "1,2,3"::json to '"1,2,3"'::text
+        ALTER TABLE context_fields
+        ALTER COLUMN legal_values SET DATA TYPE text;
+
+        -- 4. Revert '"1,2,3"'::text to '1,2,3'::text
+        UPDATE context_fields SET legal_values = legal_values::json #>> '{}';
+        `,
+        cb,
+    );
+};

src/test/e2e/api/admin/context.e2e.test.ts

@@ -50,29 +50,75 @@ test('should create context field', async () => {
 });
 
 test('should create context field with legalValues', async () => {
-    expect.assertions(0);
+    expect.assertions(3);
-    return app.request
+
+    const data = {
+        name: 'region',
+        description: 'A region',
+        legalValues: [
+            { value: 'north' },
+            { value: 'south', description: 'south-desc' },
+        ],
+    };
+
+    await app.request
         .post('/api/admin/context')
-        .send({
+        .send(data)
-            name: 'region',
-            description: 'A region',
-            legalValues: ['north', 'south'],
-        })
         .set('Content-Type', 'application/json')
         .expect(201);
+
+    const res = await app.request.get(`/api/admin/context/${data.name}`);
+    expect(res.body.name).toEqual(data.name);
+    expect(res.body.description).toEqual(data.description);
+    expect(res.body.legalValues).toEqual(data.legalValues);
 });
 
 test('should update context field with legalValues', async () => {
-    expect.assertions(0);
+    expect.assertions(3);
-    return app.request
+
+    const data = {
+        name: 'environment',
+        description: 'Updated description',
+        legalValues: [
+            { value: 'dev', description: 'dev-desc' },
+            { value: 'prod' },
+        ],
+    };
+
+    await app.request
         .put('/api/admin/context/environment')
-        .send({
+        .send(data)
-            name: 'environment',
-            description: 'Updated description',
-            legalValues: ['dev', 'prod'],
-        })
         .set('Content-Type', 'application/json')
         .expect(200);
+
+    const res = await app.request.get(`/api/admin/context/${data.name}`);
+    expect(res.body.name).toEqual(data.name);
+    expect(res.body.description).toEqual(data.description);
+    expect(res.body.legalValues).toEqual(data.legalValues);
+});
+
+test('should reject string legalValues', async () => {
+    await app.request
+        .put('/api/admin/context/environment')
+        .send({ name: 'environment', legalValues: ['a'] })
+        .set('Content-Type', 'application/json')
+        .expect(400);
+});
+
+test('should reject empty legalValues', async () => {
+    await app.request
+        .put('/api/admin/context/environment')
+        .send({ name: 'environment', legalValues: [{ description: 'b' }] })
+        .set('Content-Type', 'application/json')
+        .expect(400);
+});
+
+test('should reject legalValues without value', async () => {
+    await app.request
+        .put('/api/admin/context/environment')
+        .send({ name: 'environment', legalValues: [{ description: 'b' }] })
+        .set('Content-Type', 'application/json')
+        .expect(400);
 });
 
 test('should create context field with stickiness', async () => {