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

Feature/chats business entity #110

Draft
wants to merge 71 commits into
base: development
Choose a base branch
from

Conversation

aleksr777
Copy link

Создание сущности бизнес-логики Chats

aleksr777 added 2 commits July 3, 2024 17:00
…ат', импортирует 'ChatModule' в 'app.module.ts', импортирует ChatService в 'app.controller.ts'
…рректировал файловую структуру, подключил сущность к app.module.
@aleksr777 aleksr777 linked an issue Jul 6, 2024 that may be closed by this pull request
5 tasks
Copy link
Collaborator

@kspshnik kspshnik left a comment

Choose a reason for hiding this comment

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

Необходимо использовать ChatsRepository и MessagesRepository из слоя datalake.
Посмотри архитектуру в Wiki репозитория :) Там чуть устарелое, но суть будет ясна.

И ещё, @aleksr777, ничего удалять не надо.

@@ -25,6 +25,9 @@ import { ContactsModule } from './core/contacts/contacts.module';
import { TasksModule } from './core/tasks/tasks.module';
import { PolicyModule } from './core/policy/policy.module';
import { SystemApiModule } from './api/system-api/system-api.module';
import { AppController } from './app.controller';
Copy link
Collaborator

Choose a reason for hiding this comment

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

А это - зачем?

@@ -54,11 +57,14 @@ import { SystemApiModule } from './api/system-api/system-api.module';
PolicyModule,
SystemApiModule,
],
controllers: [AppController],
Copy link
Collaborator

Choose a reason for hiding this comment

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

И это - зачем?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Никакого репозитория нам не нужно.

const foundChats = this.chats.filter((chat) =>
Object.entries(params).every(([key, value]) => chat[key] === value)
) as ConflictChat[];
return limit ? foundChats.slice(0, limit) : foundChats;
Copy link
Collaborator

Choose a reason for hiding this comment

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

А почему не воспользоваться возможностями Мангуста?
И ещё надо бы offset учесть :)

...messageData,
timestamp: new Date(),
} as Message;
this.messages.push(newMessage);
Copy link
Collaborator

Choose a reason for hiding this comment

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

А где сохранение в БД?

Copy link
Collaborator

@kspshnik kspshnik left a comment

Choose a reason for hiding this comment

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

Вообще другой подход: мы не держим тут в памяти массивы сообщений и чатов, работаем с ChatsRepository и MessagesRepository из src/datalake.

…ториям из datalake, создал необходимые методы в репозиториях, подключил сервисы, репозитории и сущность к chat.module.
Copy link
Collaborator

@kspshnik kspshnik left a comment

Choose a reason for hiding this comment

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

Чуть другая логика. Ты используешь репы из datalake, но сам слой сохранения данных вообще не трогаешь - надо создавать методы именно в entity, реализующие нужную логику.

Copy link
Collaborator

Choose a reason for hiding this comment

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

В src/datalake НИЧЕГО НЕ МЕНЯЕМ

…ат', импортирует 'ChatModule' в 'app.module.ts', импортирует ChatService в 'app.controller.ts'
…рректировал файловую структуру, подключил сущность к app.module.
…ториям из datalake, создал необходимые методы в репозиториях, подключил сервисы, репозитории и сущность к chat.module.
… из сущности, используя репозитории messages и chats из datalake.
Copy link
Collaborator

@kspshnik kspshnik left a comment

Choose a reason for hiding this comment

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

Вот это правильный подход! :)
Теперь осталось избавиться от жесткого зацепления, перейдя на интерфейсы, и сделать методы строго по задаче :)


@Injectable({ scope: Scope.TRANSIENT })
export class ChatEntity {
private metadata: any[];
private messages: any[][];
private metadata: Chat[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Здесь лучше использовать интерфейсы - так мы не привязаны к Мангусту.

private metadata: any[];
private messages: any[][];
private metadata: Chat[];
private messages: Message[][];
Copy link
Collaborator

Choose a reason for hiding this comment

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

А почему массив массивов? В отдельном чате только один набор сообщений :)

@@ -1,11 +1,14 @@
import { Injectable, Scope } from '@nestjs/common';
import { ChatsRepository } from '../../datalake/chats/chats.repository';
import { MessagesRepository } from '../../datalake/messages/messages.repository';
import { Chat } from '../../datalake/chats/schemas/chat.schema';
import { Message } from '../../datalake/messages/schemas/messages.schema';
import { Types } from 'mongoose';

@Injectable({ scope: Scope.TRANSIENT })
Copy link
Collaborator

Choose a reason for hiding this comment

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

А точно не REQUEST? Можешь обосновать выбор?
(Это не предложение поменять, тут надо подумать)

@@ -14,4 +17,48 @@ export class ChatEntity {
this.metadata = [];
this.messages = [];
}

async createChat(metadata: Partial<Chat>, messages: Partial<Message>[]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

При создании чата - может и не быть ни одного ещё сообщения. Если они есть - их надо сохранить в их репу :)

async findChatByParams(params: Record<string, any>) {
const chats = await this.chatsRepository.find(params);
this.metadata = chats;
return this;
Copy link
Collaborator

Choose a reason for hiding this comment

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

А если там уже есть сообщения? Их тоже надо найти :)

return this;
}

async findConflictingChats(params: Record<string, any>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Никакого any
  2. Этого нет в задаче :) Поиск конфликтного чата - это поиск двух чатов по типу (и указывающих друг на друга), что прекрасно делает предыдущий метод :)

Copy link
Collaborator

@kspshnik kspshnik left a comment

Choose a reason for hiding this comment

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

Мы затем и меняем scope у DI-контейнера, чтобы каждый экземпляр был про один чат (или кортеж чатов для конфликтных).
Наверное, не надо поддерживать возможность запихать много чатов - это ненужное усложнение.

this.messages = {}; // Сброс сообщений при новом поиске
for (const chat of chats) {
const chatMessages = await this.messagesRepository.find({ chatId: chat._id }) as MessageInterface[];
this.messages[chat._id.toString()] = chatMessages;
Copy link
Collaborator

Choose a reason for hiding this comment

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

В одном экземпляре - данные только одного чата

Copy link
Collaborator

@kspshnik kspshnik left a comment

Choose a reason for hiding this comment

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

В целом, направление правильное.

closeChat(): Promise<this>;
}

@Injectable({ scope: Scope.TRANSIENT })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Так у нас будет общий Entity на все чаты перетирать данные.
Здесь логичен scope: Scope.REQUEST

}

async createChat(
metadata: Partial<TaskChatInterface>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Тут можно определить конкретную dto, мы знаем, какие данные есть на старте при создании чатика :)


async createChat(
metadata: Partial<TaskChatInterface>,
messages: MessageInterface[]
Copy link
Collaborator

Choose a reason for hiding this comment

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

При создании чата при задаче (в момент создания задачи) - никаких сообщений точно ещё нет.

const chat = (await this.chatsRepository.create(metadata)) as TaskChatInterface;
this.metadata = chat;
this.chatId = chat._id;
if (messages.length > 0 && chat) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Это можно убрать - тут не будет сразу сообщений.
А на будущее - для массового создания лучше использовать bulkWrite()

return this;
}

async findConflictingChats(params: Partial<TaskChatInterface>): Promise<this> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Конфликтные чаты - другой тип, их надо убрать отсюда.

}

async addMessage(chatId: string, message: Partial<MessageInterface>): Promise<this> {
if (!this.chatId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ошибку чуть по-другому надо кидать, посмотри в других сервисах, как это делается.

}
const newMessage = (await this.messagesRepository.create({
...message,
chatId: new Types.ObjectId(chatId),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Здесь кмк можно просто строку передавать - Мангуст сам преобразует :)

);
this._messages = [volunteerMessages, recipientMessages] as MessagesType<T>;
break;
}
Copy link
Contributor

@IvannaBalanyuk IvannaBalanyuk Sep 6, 2024

Choose a reason for hiding this comment

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

Привет! Кажется тут потеряны сообщения админа 🙈
Ожидается, что для конфликтного чата метод вернёт данные типа ConflictChatContentTuple:

image

Вот этот интерфейс:

image

VolunteerChatContent и RecipientChatContent - это, по-хорошему, ВЕСЬ контент чата, т.е. и сообщения админа тоже.
Вопрос в том, как их вычленить.
@kspshnik мне чёт кажется, что при текущем интерфейсе сообщения мы не сможем разделить сообщения админа по нужным кучкам:

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. А зачем их вычленять-то?
  2. Поле author в MessageInterface нам зачем? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. А зачем их вычленять-то?
  2. Поле author в MessageInterface нам зачем? :)
  1. Чтобы часть положить в чат с волонтером, а часть - в чат с реципиентом;
  2. По значению в поле author мы получим все сообщения админа - и те, что волонтеру, и те, что реципиенту.

По chatId не выделить - судя по коду метода createChat(...) идентификатор создается на общий CONFLICT_CHAT. В методе addMessage(...), кстати, не вижу сохранения сообщений администратора для конфликтного чата.

В любом случае сейчас для конфликта метод loadMessages(...) возвращает только сообщения волонтера и реципиента. Так задумано?

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. А зачем их вычленять-то?
  2. Поле author в MessageInterface нам зачем? :)
  1. Чтобы часть положить в чат с волонтером, а часть - в чат с реципиентом;

Это должно делаться по chatId. В кортеже метаинформации - оба идентификатора, обоих чатов.

  1. По значению в поле author мы получим все сообщения админа - и те, что волонтеру, и те, что реципиенту.

Но они будут в разных элементах кортежа ConflictChatContentTuple ;o)

По chatId не выделить - судя по коду метода createChat(...) идентификатор создается на общий CONFLICT_CHAT. В методе addMessage(...), кстати, не вижу сохранения сообщений администратора для конфликтного чата.
В любом случае сейчас для конфликта метод loadMessages(...) возвращает только сообщения волонтера и реципиента. Так задумано?

С идентификаторами там чуть-чуть поменяется, вспомни начало прошлого QA.
А в массивах хранятся сообщения всего чата, например в массиве с чатом с волонтёром сообщения и волонтёра и администратора.

@kspshnik kspshnik added backend Задача для бэкенда PR Pull Request labels Oct 19, 2024
@kspshnik kspshnik self-assigned this Nov 21, 2024
@kspshnik kspshnik requested a review from INextYP November 21, 2024 15:05
@kspshnik kspshnik marked this pull request as draft November 21, 2024 15:05
…nto feature/chats-business-entity

# Conflicts:
#	src/entities/chats/chat.entity.ts
@kspshnik kspshnik dismissed their stale review December 12, 2024 19:42

Took assignment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Задача для бэкенда PR Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Создание сущности бизнес-логики Chats
3 participants