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

feat: ability to upsert single legal values #9056

Merged
merged 4 commits into from
Jan 3, 2025
Merged

feat: ability to upsert single legal values #9056

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
54b1798
feat: ability to upsert single legal values
kwasniew Jan 3, 2025
a0bc011
feat: ability to upsert single legal values
kwasniew Jan 3, 2025
20dae18
feat: ability to upsert single legal values
kwasniew Jan 3, 2025
0e7aab6
feat: ability to upsert single legal values
kwasniew Jan 3, 2025
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
43 changes: 41 additions & 2 deletions src/lib/features/context/context-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ import {
} from '../../types';
import type { IPrivateProjectChecker } from '../private-project/privateProjectCheckerType';
import type EventService from '../events/event-service';
import { contextSchema } from '../../services/context-schema';
import { contextSchema, legalValueSchema } from '../../services/context-schema';
import { NameExistsError } from '../../error';
import { nameSchema } from '../../schema/feature-schema';
import type { LegalValueSchema } from '../../openapi';

class ContextService {
private eventService: EventService;
Expand Down Expand Up @@ -126,7 +127,6 @@ class ContextService {
);
const value = await contextSchema.validateAsync(updatedContextField);

// update
await this.contextFieldStore.update(value);

const { createdAt, sortOrder, ...previousContextField } = contextField;
Expand All @@ -140,6 +140,45 @@ class ContextService {
});
}

async updateContextFieldLegalValue(
contextFieldLegalValue: { name: string; legalValue: LegalValueSchema },
auditUser: IAuditUser,
): Promise<void> {
const contextField = await this.contextFieldStore.get(
contextFieldLegalValue.name,
);
const validatedLegalValue = await legalValueSchema.validateAsync(
contextFieldLegalValue.legalValue,
);
Comment on lines +150 to +152
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something, but why are we using the schema to validate here? Isn't that automatically taken care of at the endpoint level?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, also, legalValueSchema is not LegalValueSchema from ../../openapi. I thought it looked like Joi, but was a little confused by the import (and didn't look hard enough, apparently).

But everything that the joi schema does, we should be able to do with openapi directly, right? Set string min and max lengths and allow description to be null, undefined, or an arbitrary string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's consistent with our current approach in the context module where openapi validates simple string and joi actual 1-100 characters. I didn't want to introduce a new convention in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's fair. I suspect we'll just forget about it and never touch it, then. But that's fine too. It's a little added complexity, but not a big deal here.


const legalValues = contextField.legalValues
? [...contextField.legalValues]
: [];

const existingIndex = legalValues.findIndex(
(legalvalue) => legalvalue.value === validatedLegalValue.value,
);
Comment on lines +158 to +160
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, my. we store this in a list, huh? I imagine using a map would be better, but it's probably also not something we want to touch now, I guess 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

But then again, maybe order is important? 🤷🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this PR piggybacks on the current implementation


if (existingIndex !== -1) {
legalValues[existingIndex] = validatedLegalValue;
} else {
legalValues.push(validatedLegalValue);
}

const newContextField = { ...contextField, legalValues };

await this.contextFieldStore.update(newContextField);

await this.eventService.storeEvent({
type: CONTEXT_FIELD_UPDATED,
createdBy: auditUser.username,
createdByUserId: auditUser.id,
ip: auditUser.ip,
preData: contextField,
data: newContextField,
});
}

async deleteContextField(
name: string,
auditUser: IAuditUser,
Expand Down
53 changes: 53 additions & 0 deletions src/lib/features/context/context.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,59 @@ test('should update a context field with new legal values', () => {
.expect(200);
});

test('should add and update a single context field with new legal values', async () => {
expect.assertions(1);

// non existent context
await request
.put(`${base}/api/admin/context/doesntexist/legalValues`)
.send({
value: 'local',
description: 'Local environment',
})
.set('Content-Type', 'application/json')
.expect(404);

// invalid schema
await request
.put(`${base}/api/admin/context/environment/legalValues`)
.send({
valueInvalid: 'invalid schema',
description: 'Local environment',
})
.set('Content-Type', 'application/json')
.expect(400);

// add a new context field legal value
await request
.put(`${base}/api/admin/context/environment/legalValues`)
.send({
value: 'newvalue',
description: 'new description',
})
.set('Content-Type', 'application/json')
.expect(200);

// update existing context field legal value description
await request
.put(`${base}/api/admin/context/environment/legalValues`)
.send({
value: 'newvalue',
description: 'updated description',
})
.set('Content-Type', 'application/json')
.expect(200);

const { body } = await request.get(`${base}/api/admin/context/environment`);

expect(body).toMatchObject({
name: 'environment',
legalValues: [
{ value: 'newvalue', description: 'updated description' },
],
});
});

test('should not delete a unknown context field', () => {
expect.assertions(0);

Expand Down
34 changes: 34 additions & 0 deletions src/lib/features/context/context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
import type { UpdateContextFieldSchema } from '../../openapi/spec/update-context-field-schema';
import type { CreateContextFieldSchema } from '../../openapi/spec/create-context-field-schema';
import { extractUserIdFromUser } from '../../util';
import type { LegalValueSchema } from '../../openapi';

interface ContextParam {
contextField: string;
Expand Down Expand Up @@ -168,6 +169,25 @@ export class ContextController extends Controller {
],
});

this.route({
method: 'put',
path: '/:contextField/legalValues',
handler: this.updateContextFieldLegalValue,
permission: UPDATE_CONTEXT_FIELD,
middleware: [
openApiService.validPath({
tags: ['Context'],
summary: 'Add or update legal value for the context field',
description: `Endpoint that allows adding or updating a single custom context field legal value. If the legal value already exists, it will be updated with the new description`,
operationId: 'updateContextFieldLegalValue',
requestBody: createRequestSchema('legalValueSchema'),
responses: {
200: emptyResponse,
},
}),
],
});

this.route({
method: 'delete',
path: '/:contextField',
Expand Down Expand Up @@ -271,6 +291,20 @@ export class ContextController extends Controller {
res.status(200).end();
}

async updateContextFieldLegalValue(
req: IAuthRequest<ContextParam, void, LegalValueSchema>,
res: Response,
): Promise<void> {
const name = req.params.contextField;
const legalValue = req.body;

await this.contextService.updateContextFieldLegalValue(
{ name, legalValue },
req.audit,
);
res.status(200).end();
}

async deleteContextField(
req: IAuthRequest<ContextParam>,
res: Response,
Expand Down
2 changes: 1 addition & 1 deletion src/lib/services/context-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { nameType } from '../routes/util';

export const nameSchema = joi.object().keys({ name: nameType });

const legalValueSchema = joi.object().keys({
export const legalValueSchema = joi.object().keys({
value: joi.string().min(1).max(100).required(),
description: joi.string().allow('').allow(null).optional(),
});
Expand Down
Loading