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.

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

update(api): centralize updateOne error handling logic #579

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion api/src/analytics/services/bot-stats.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export class BotStatsService extends BaseService<BotStats> {
* @param name - The name or identifier of the statistics entry (e.g., a specific feature or component being tracked).
*/
@OnEvent('hook:stats:entry')
async handleStatEntry(type: BotStatsType, name: string) {
async handleStatEntry(type: BotStatsType, name: string): Promise<void> {
const day = new Date();
day.setMilliseconds(0);
day.setSeconds(0);
Expand Down
5 changes: 2 additions & 3 deletions api/src/chat/controllers/block.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import { PermissionService } from '@/user/services/permission.service';
import { RoleService } from '@/user/services/role.service';
import { UserService } from '@/user/services/user.service';
import { IGNORED_TEST_FIELDS } from '@/utils/test/constants';
import { getUpdateOneError } from '@/utils/test/errors/messages';
import {
blockFixtures,
installBlockFixtures,
Expand Down Expand Up @@ -327,9 +328,7 @@ describe('BlockController', () => {

await expect(
blockController.updateOne(blockToDelete.id, updateBlock),
).rejects.toThrow(
new NotFoundException(`Block with ID ${blockToDelete.id} not found`),
);
).rejects.toThrow(getUpdateOneError(Block.name, blockToDelete.id));
});
});
});
7 changes: 1 addition & 6 deletions api/src/chat/controllers/block.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -296,12 +296,7 @@ export class BlockController extends BaseController<
@Param('id') id: string,
@Body() blockUpdate: BlockUpdateDto,
): Promise<Block> {
const result = await this.blockService.updateOne(id, blockUpdate);
if (!result) {
this.logger.warn(`Unable to update Block by id ${id}`);
throw new NotFoundException(`Block with ID ${id} not found`);
}
return result;
return await this.blockService.updateOne(id, blockUpdate);
}

/**
Expand Down
7 changes: 1 addition & 6 deletions api/src/chat/controllers/category.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,7 @@ export class CategoryController extends BaseController<Category> {
@Param('id') id: string,
@Body() categoryUpdate: CategoryUpdateDto,
): Promise<Category> {
const result = await this.categoryService.updateOne(id, categoryUpdate);
if (!result) {
this.logger.warn(`Unable to update Category by id ${id}`);
throw new NotFoundException(`Category with ID ${id} not found`);
}
return result;
return await this.categoryService.updateOne(id, categoryUpdate);
}

/**
Expand Down
5 changes: 2 additions & 3 deletions api/src/chat/controllers/context-var.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { MongooseModule } from '@nestjs/mongoose';
import { Test } from '@nestjs/testing';

import { LoggerService } from '@/logger/logger.service';
import { getUpdateOneError } from '@/utils/test/errors/messages';
import {
contextVarFixtures,
installContextVarFixtures,
Expand Down Expand Up @@ -211,9 +212,7 @@ describe('ContextVarController', () => {
contextVarUpdatedDto,
),
).rejects.toThrow(
new NotFoundException(
`ContextVar with ID ${contextVarToDelete.id} not found`,
),
getUpdateOneError(ContextVar.name, contextVarToDelete.id),
);
});
});
Expand Down
7 changes: 1 addition & 6 deletions api/src/chat/controllers/context-var.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,7 @@ export class ContextVarController extends BaseController<ContextVar> {
@Param('id') id: string,
@Body() contextVarUpdate: ContextVarUpdateDto,
): Promise<ContextVar> {
const result = await this.contextVarService.updateOne(id, contextVarUpdate);
if (!result) {
this.logger.warn(`Unable to update ContextVar by id ${id}`);
throw new NotFoundException(`ContextVar with ID ${id} not found`);
}
return result;
return await this.contextVarService.updateOne(id, contextVarUpdate);
}

/**
Expand Down
7 changes: 3 additions & 4 deletions api/src/chat/controllers/label.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { UserModel } from '@/user/schemas/user.schema';
import { RoleService } from '@/user/services/role.service';
import { UserService } from '@/user/services/user.service';
import { IGNORED_TEST_FIELDS } from '@/utils/test/constants';
import { getUpdateOneError } from '@/utils/test/errors/messages';
import { labelFixtures } from '@/utils/test/fixtures/label';
import { installSubscriberFixtures } from '@/utils/test/fixtures/subscriber';
import { getPageQuery } from '@/utils/test/pagination';
Expand Down Expand Up @@ -223,12 +224,10 @@ describe('LabelController', () => {
);
});

it('should throw a NotFoundException when attempting to update a non existing label by id', async () => {
it('should throw a NotFoundException when attempting to update a non existing label by id', async () => {
await expect(
labelController.updateOne(labelToDelete.id, labelUpdateDto),
).rejects.toThrow(
new NotFoundException(`Label with ID ${labelToDelete.id} not found`),
);
).rejects.toThrow(getUpdateOneError(Label.name, labelToDelete.id));
});
});
});
7 changes: 1 addition & 6 deletions api/src/chat/controllers/label.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,7 @@ export class LabelController extends BaseController<
@Param('id') id: string,
@Body() labelUpdate: LabelUpdateDto,
) {
const result = await this.labelService.updateOne(id, labelUpdate);
if (!result) {
this.logger.warn(`Unable to update Label by id ${id}`);
throw new NotFoundException(`Label with ID ${id} not found`);
}
return result;
return await this.labelService.updateOne(id, labelUpdate);
}

@CsrfCheck(true)
Expand Down
7 changes: 1 addition & 6 deletions api/src/chat/controllers/subscriber.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,11 +178,6 @@ export class SubscriberController extends BaseController<
@Param('id') id: string,
@Body() subscriberUpdate: SubscriberUpdateDto,
) {
const result = await this.subscriberService.updateOne(id, subscriberUpdate);
if (!result) {
this.logger.warn(`Unable to update Subscriber by id ${id}`);
throw new NotFoundException(`Subscriber with ID ${id} not found`);
}
return result;
return await this.subscriberService.updateOne(id, subscriberUpdate);
}
}
44 changes: 23 additions & 21 deletions api/src/chat/repositories/block.repository.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ describe('BlockRepository', () => {
let hasNextBlocks: Block;
let validIds: string[];
let validCategory: string;
let blockValidIds: string[];

beforeAll(async () => {
const module = await Test.createTestingModule({
Expand All @@ -58,6 +59,7 @@ describe('BlockRepository', () => {
hasNextBlocks = (await blockRepository.findOne({
name: 'hasNextBlocks',
}))!;
blockValidIds = (await blockRepository.findAll()).map(({ id }) => id);
});

afterEach(jest.clearAllMocks);
Expand Down Expand Up @@ -169,22 +171,22 @@ describe('BlockRepository', () => {
describe('prepareBlocksInCategoryUpdateScope', () => {
it('should update blocks within the scope based on category and ids', async () => {
jest.spyOn(blockRepository, 'findOne').mockResolvedValue({
id: validIds[0],
id: blockValidIds[0],
category: 'oldCategory',
nextBlocks: [validIds[1]],
attachedBlock: validIds[1],
nextBlocks: [blockValidIds[1]],
attachedBlock: blockValidIds[1],
} as Block);

const mockUpdateOne = jest.spyOn(blockRepository, 'updateOne');

await blockRepository.prepareBlocksInCategoryUpdateScope(
validCategory,
validIds,
blockValidIds,
);

expect(mockUpdateOne).toHaveBeenCalledWith(validIds[0], {
nextBlocks: [validIds[1]],
attachedBlock: validIds[1],
expect(mockUpdateOne).toHaveBeenCalledWith(blockValidIds[0], {
nextBlocks: [blockValidIds[1]],
attachedBlock: blockValidIds[1],
});
});

Expand All @@ -211,41 +213,41 @@ describe('BlockRepository', () => {
it('should update blocks outside the scope by removing references from attachedBlock', async () => {
const otherBlocks = [
{
id: '64abc1234def567890fedcab',
attachedBlock: validIds[0],
nextBlocks: [validIds[0]],
id: blockValidIds[1],
attachedBlock: blockValidIds[0],
nextBlocks: [blockValidIds[0]],
},
] as Block[];

const mockUpdateOne = jest.spyOn(blockRepository, 'updateOne');

await blockRepository.prepareBlocksOutOfCategoryUpdateScope(
otherBlocks,
validIds,
blockValidIds,
);

expect(mockUpdateOne).toHaveBeenCalledWith('64abc1234def567890fedcab', {
expect(mockUpdateOne).toHaveBeenCalledWith(blockValidIds[1], {
attachedBlock: null,
});
});

it('should update blocks outside the scope by removing references from nextBlocks', async () => {
const otherBlocks = [
{
id: '64abc1234def567890fedcab',
id: blockValidIds[1],
attachedBlock: null,
nextBlocks: [validIds[0], validIds[1]],
nextBlocks: [blockValidIds[0], blockValidIds[1]],
},
] as unknown as Block[];

const mockUpdateOne = jest.spyOn(blockRepository, 'updateOne');

await blockRepository.prepareBlocksOutOfCategoryUpdateScope(otherBlocks, [
validIds[0],
blockValidIds[0],
]);

expect(mockUpdateOne).toHaveBeenCalledWith('64abc1234def567890fedcab', {
nextBlocks: [validIds[1]],
expect(mockUpdateOne).toHaveBeenCalledWith(blockValidIds[1], {
nextBlocks: [blockValidIds[1]],
});
});
});
Expand All @@ -254,7 +256,7 @@ describe('BlockRepository', () => {
it('should update blocks in and out of the scope', async () => {
const mockFind = jest.spyOn(blockRepository, 'find').mockResolvedValue([
{
id: '64abc1234def567890fedcab',
id: blockValidIds[1],
attachedBlock: validIds[0],
nextBlocks: [validIds[0]],
},
Expand All @@ -278,17 +280,17 @@ describe('BlockRepository', () => {
expect(mockFind).toHaveBeenCalled();
expect(prepareBlocksInCategoryUpdateScope).toHaveBeenCalledWith(
validCategory,
['64abc1234def567890fedcab'],
[blockValidIds[1]],
);
expect(prepareBlocksOutOfCategoryUpdateScope).toHaveBeenCalledWith(
[
{
id: '64abc1234def567890fedcab',
id: blockValidIds[1],
attachedBlock: validIds[0],
nextBlocks: [validIds[0]],
},
],
['64abc1234def567890fedcab'],
[blockValidIds[1]],
);
});

Expand Down
2 changes: 1 addition & 1 deletion api/src/chat/repositories/conversation.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export class ConversationRepository extends BaseRepository<
*
* @returns A promise resolving to the result of the update operation.
*/
async end(convo: Conversation | ConversationFull) {
async end(convo: Conversation | ConversationFull): Promise<Conversation> {
return await this.updateOne(convo.id, { active: false });
}
}
8 changes: 3 additions & 5 deletions api/src/chat/repositories/subscriber.repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ export class SubscriberRepository extends BaseRepository<
async updateOneByForeignIdQuery(
id: string,
updates: SubscriberUpdateDto,
): Promise<Subscriber | null> {
): Promise<Subscriber> {
return await this.updateOne({ foreign_id: id }, updates);
}

Expand All @@ -162,9 +162,7 @@ export class SubscriberRepository extends BaseRepository<
*
* @returns The updated subscriber entity.
*/
async handBackByForeignIdQuery(
foreignId: string,
): Promise<Subscriber | null> {
async handBackByForeignIdQuery(foreignId: string): Promise<Subscriber> {
return await this.updateOne(
{
foreign_id: foreignId,
Expand All @@ -187,7 +185,7 @@ export class SubscriberRepository extends BaseRepository<
async handOverByForeignIdQuery(
foreignId: string,
userId: string,
): Promise<Subscriber | null> {
): Promise<Subscriber> {
return await this.updateOne(
{
foreign_id: foreignId,
Expand Down
5 changes: 0 additions & 5 deletions api/src/chat/services/conversation.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,11 +184,6 @@ export class ConversationService extends BaseService<
const updatedConversation = await this.updateOne(convo.id, {
context: convo.context,
});
if (!updatedConversation) {
throw new Error(
'Conversation Model : No conversation has been updated',
);
}

//TODO: add check if nothing changed don't update
const criteria =
Expand Down
3 changes: 2 additions & 1 deletion api/src/cms/controllers/content-type.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { AttachmentService } from '@/attachment/services/attachment.service';
import { BlockService } from '@/chat/services/block.service';
import { LoggerService } from '@/logger/logger.service';
import { NOT_FOUND_ID } from '@/utils/constants/mock';
import { getUpdateOneError } from '@/utils/test/errors/messages';
import { installContentFixtures } from '@/utils/test/fixtures/content';
import { contentTypeFixtures } from '@/utils/test/fixtures/contenttype';
import { getPageQuery } from '@/utils/test/pagination';
Expand Down Expand Up @@ -174,7 +175,7 @@ describe('ContentTypeController', () => {
jest.spyOn(contentTypeService, 'updateOne');
await expect(
contentTypeController.updateOne(updatedContent, NOT_FOUND_ID),
).rejects.toThrow(NotFoundException);
).rejects.toThrow(getUpdateOneError(ContentType.name, NOT_FOUND_ID));
expect(contentTypeService.updateOne).toHaveBeenCalledWith(
NOT_FOUND_ID,
updatedContent,
Expand Down
13 changes: 1 addition & 12 deletions api/src/cms/controllers/content-type.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,17 +148,6 @@ export class ContentTypeController extends BaseController<ContentType> {
@Body() contentTypeDto: ContentTypeUpdateDto,
@Param('id') id: string,
) {
const updatedContentType = await this.contentTypeService.updateOne(
id,
contentTypeDto,
);

if (!updatedContentType) {
this.logger.warn(
`Failed to update content type with id ${id}. Content type not found.`,
);
throw new NotFoundException(`Content type with id ${id} not found`);
}
return updatedContentType;
return await this.contentTypeService.updateOne(id, contentTypeDto);
}
}
3 changes: 2 additions & 1 deletion api/src/cms/controllers/content.controller.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { LoggerService } from '@/logger/logger.service';
import { NOT_FOUND_ID } from '@/utils/constants/mock';
import { PageQueryDto } from '@/utils/pagination/pagination-query.dto';
import { IGNORED_TEST_FIELDS } from '@/utils/test/constants';
import { getUpdateOneError } from '@/utils/test/errors/messages';
import {
contentFixtures,
installContentFixtures,
Expand Down Expand Up @@ -194,7 +195,7 @@ describe('ContentController', () => {
it('should throw NotFoundException if the content is not found', async () => {
await expect(
contentController.updateOne(updatedContent, NOT_FOUND_ID),
).rejects.toThrow(NotFoundException);
).rejects.toThrow(getUpdateOneError(Content.name, NOT_FOUND_ID));
});
});

Expand Down
9 changes: 1 addition & 8 deletions api/src/cms/controllers/content.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -300,13 +300,6 @@ export class ContentController extends BaseController<
@Body() contentDto: ContentUpdateDto,
@Param('id') id: string,
): Promise<Content> {
const updatedContent = await this.contentService.updateOne(id, contentDto);
if (!updatedContent) {
this.logger.warn(
`Failed to update content with id ${id}. Content not found.`,
);
throw new NotFoundException(`Content of id ${id} not found`);
}
return updatedContent;
return await this.contentService.updateOne(id, contentDto);
}
}
5 changes: 4 additions & 1 deletion api/src/cms/controllers/menu.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,10 @@ export class MenuController extends BaseController<
*/
@CsrfCheck(true)
@Patch(':id')
async updateOne(@Body() body: MenuCreateDto, @Param('id') id: string) {
async updateOne(
@Body() body: MenuCreateDto,
@Param('id') id: string,
): Promise<Menu> {
if (!id) return await this.create(body);
return await this.menuService.updateOne(id, body);
}
Expand Down
Loading
Loading