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

soft deletes #22

Closed
wants to merge 7 commits into from
Closed

soft deletes #22

wants to merge 7 commits into from

Conversation

Zimovchik
Copy link
Member

Изменения

Детали реализации

Check-List

  • Вы проверили свой код перед отправкой запроса?
  • Вы написали тесты к реализованным функциям?
  • Вы не забыли применить форматирование black и isort для Back-End или Prettier для Front-End?

@Zimovchik Zimovchik linked an issue Oct 23, 2024 that may be closed by this pull request
Copy link

github-actions bot commented Oct 23, 2024

Code Coverage

Coverage Report
FileStmtsMissCoverMissing
rating_api
   __main__.py440%1–7
   exceptions.py23387%35–36, 44
rating_api/models
   base.py55591%24–27, 76
rating_api/routes
   comment.py662858%28, 34–37, 85–106, 123–126, 140
   exc_handlers.py17288%26, 33
   lecturer.py885439%31–32, 53–74, 104–143, 157, 165, 183, 187–192
rating_api/schemas
   base.py12467%6–9
TOTAL40810075% 

Summary

Tests Skipped Failures Errors Time
8 0 💤 5 ❌ 0 🔥 2.863s ⏱️

@Zimovchik Zimovchik self-assigned this Oct 23, 2024
Copy link
Member

@Temmmmmo Temmmmmo left a comment

Choose a reason for hiding this comment

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

Еще нужно также софт делить учесть в релейшенах
Посмотри, как это реализовано в аутхе, там хорошо прописано

rating_api/routes/comment.py Outdated Show resolved Hide resolved
@@ -188,7 +188,7 @@ async def delete_lecturer(
for lecturer_user_comment in lecturer_user_comments:
LecturerUserComment.delete(lecturer_user_comment.id, session=db.session)

Lecturer.delete(session=db.session, id=id)
Lecturer.update(session=db.session, id=id, is_deleted=True)
Copy link
Member

Choose a reason for hiding this comment

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

Тоже не требуется

rating_api/routes/lecturer.py Outdated Show resolved Hide resolved
@Temmmmmo
Copy link
Member

Идея софтделитов в том, что можно, не меняя запросов в базу, ставить нужный флажок вместо реального удаления

Опять же, данную логику можно прочитать в базовом декларативном классе

Там эта логика уже прописана в методах

@Temmmmmo
Copy link
Member

Да, и надо nullable=False проставить, а также учесть потом это в миграции, чтобы база не упала

@Temmmmmo
Copy link
Member

Хоть у нас в базе нет сейчас записей, но все равно

op.add_column('lecturer', sa.Column('is_deleted', sa.Boolean(), nullable=False, server_default=sa.false()))
op.add_column(
'lecturer_user_comment', sa.Column('is_deleted', sa.Boolean(), nullable=False, server_default=sa.false())
)
Copy link
Member

Choose a reason for hiding this comment

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

nullable False
надо тогда докинуть update на старые строки в базе, чтоб не упало

lecturer_id: Mapped[int] = mapped_column(
Integer,
ForeignKey("lecturer.id"),
primary_join="and_(Comment.lecturer_id==Lecturer.id, not_(Lecturer.is_deleted))",
Copy link
Member

Choose a reason for hiding this comment

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

не туда вписал)
lecturer_id это не отношение
надо джоин вписать в lecturer который ниже

@Temmmmmo
Copy link
Member

Temmmmmo commented Nov 3, 2024

еще надо добавить тесты)

@Zimovchik
Copy link
Member Author

Еще не доделано

Copy link

github-actions bot commented Nov 9, 2024

💩 Code linting failed, use black and isort to fix it.

@Zimovchik Zimovchik mentioned this pull request Nov 10, 2024
3 tasks
@Zimovchik Zimovchik closed this Nov 10, 2024
@Zimovchik Zimovchik deleted the soft-deletes branch November 10, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Добавить soft-deletes в рейтинг
2 participants