Skip to content

Tasks#74

Open
xStenx wants to merge 3 commits into
ProCharity:developfrom
xStenx:tasks
Open

Tasks#74
xStenx wants to merge 3 commits into
ProCharity:developfrom
xStenx:tasks

Conversation

@xStenx

@xStenx xStenx commented Dec 20, 2022

Copy link
Copy Markdown

Создал репозиторий для Task. Добавил метод update в репозиторий User.

По поводу репозиториев, нашел такое решение:
https://gist.github.com/uris77/4711015/

Метод create и метод update в репозиториях предлагаю объединить в метод persist. Для этого надо переопределить эти методы в AbstractRepository.

Добавил недостающее поле archive в TaskSchema (app/webhooks/tasks).

Comment thread app/webhooks/tasks.py Outdated
location = fields.String(required=True)
link = fields.String(required=True)
description = fields.String()
archive = fields.Boolean()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

А зачем ты добавил в модель новое поле?

@xStenx xStenx Dec 21, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Для сохранения целостности сущности, так как это поле есть в модели Task. Убрать? На добавление записи это влиять не должно.

@xStenx

xStenx commented Dec 27, 2022

Copy link
Copy Markdown
Author

Создал репозиторий и сервис для работы с моделью Task. В сервис добавил два метода работы с БД. (взять все tasks, взять только активные tasks(archive=False))
Начал рефакторинг кода в tasks.py. Обернул в условия часть кода отвечающую за разархивацию task, добавление новой task, обновление существующей task.

Дальше хочу заменить некоторые операции с id для большего удобства чтения кода.

Comment thread core/services/task_service.py Outdated
Comment on lines +12 to +16
tasks = Task.query.options(load_only('archive')).all()
return tasks

def get_active_tasks(self):
tasks = Task.query.filter_by(archive=False).all()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Работает с базой данных именно репозиторий, и эти все методы переноси в него. А от сервиса вообще избавляемся

Comment thread app/webhooks/tasks.py Outdated
Comment on lines +80 to +81
tasks_db = task_db.get_archive_tasks()
task_active_db = task_db.get_active_tasks()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Но вообще, эти две функции возвращают одинаковый результат...
И давай через сессию пробовать получать значения.

И посмотри старый комент, я там описал как получить только те записи которые необходимы, а не всё из базы, у нас там уже несколько тысяч записей, а ээто довольно накладно, особенно, когда записей станет в 10 раз больше, а для этого нужно ещё один год. Доставай только то, что нужно.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

А не, не одно и то же, но всё равно, попробуй сделать запрос про который я писал раньше и всё станет проще.

@xStenx

xStenx commented Dec 28, 2022

Copy link
Copy Markdown
Author

Запросы к бд перенес в репозиторий, сервис удалил. Немного изменил код в tasks.py. Работал только с активными задачами в бд, как ты и писал в комментарии notions. Пока не придумал как сделать лучше hash функцию. Удалил метод разархивации(не нашел ему применения, единственное логи пришлось оставить. Хотелось бы это улучшить).
Строк стало немного больше, но выглядят они пожалуй понятнее. (во всяком случае для меня) )

@kr0t kr0t left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Есть конфликт, в tasks.py добавлена валидация модели для тасков.

return self.session.get(Task, task_id)

def get(self, task_id: int) -> Task:
task = self.get_or_none(task_id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Метод может вернуть None, но ты его никак не отлавливаешь.

Comment thread core/repositories/task_repository.py Outdated
return task_list

def update(self, task: Task) -> Task:
self.session.add(Task)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Тут должен быть экземпляр класса task, а не Task.

Comment thread app/webhooks/tasks.py
logger.info(f"Tasks: Archived task ids: {task_ids}")
return task_ids

def __unarchive_tasks(self, unarchive_records, task_to_send, tasks_dict):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Не очень хорошая идея удалить этот метод.
Может быть не часто, но он используется. Лог с тестового сервера:
image

Comment thread core/repositories/user_repository.py Outdated

def update(self, user: User) -> None:
pass
def update(self, task: User) -> User:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Должен быть user вместо task

Comment thread core/repositories/user_repository.py Outdated
def update(self, task: User) -> User:
self.session.add(User)
self.session.commit()
self.session.refresh(task)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

И тут

xStenx pushed a commit to xStenx/ProCharity_bot that referenced this pull request Jan 7, 2023
@xStenx

xStenx commented Jan 7, 2023

Copy link
Copy Markdown
Author

Вернул метод __unarchive_tasks(переписал немного его логику). Поправил репозитории.

@xStenx

xStenx commented Jan 14, 2023

Copy link
Copy Markdown
Author

Разрешил конфликт в tasks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants