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

bug Delete Chat Replies on Delete #1064

Merged
merged 5 commits into from
Jun 21, 2023
Merged

bug Delete Chat Replies on Delete #1064

merged 5 commits into from
Jun 21, 2023

Conversation

MatthiasReumann
Copy link
Collaborator

Motivation and Context

Currently if a message with replies is deleted only the "base" message is deleted, i.e. replies are kept in the database. In other words deleted_at is only not-null for base messages. As a consequence, each time we fetch messages deleted "ghost" replies are transmitted also.

This PR fixes this issue.

Description

Add DeleteReplies to ChatDao and call in delete handler.

Steps for Testing

  • 1 Account
  • 1 Livestream with the chat activated
  1. Log in
  2. Navigate to a Livestream
  3. Write a message and reply to that message
  4. Delete the base message
  5. Use DataGrip (or similar) to check that both the base message and the replies are deleted

@MatthiasReumann MatthiasReumann added the bug Something isn't working label Jun 13, 2023
@github-actions
Copy link

Your Testserver will be ready at https://1064.test.live.mm.rbg.tum.de in a few minutes.

Logins
Kurs1 Kurs2 Kurs3 Kurs4
public public loggedin enrolled
prof1 prof1 prof2 prof1
prof2
student1
student2
student3
student1
student2
student2
student3
student1
student2

Copy link
Collaborator

@mono424 mono424 left a comment

Choose a reason for hiding this comment

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

Checked locally with DataGrip, looks great! 🚀

Copy link
Member

@joschahenningsen joschahenningsen left a comment

Choose a reason for hiding this comment

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

Looks good! Do we ever need to delete a chat without deleting it's replies? If not we can also merge these methods into one.

@MatthiasReumann
Copy link
Collaborator Author

Looks good! Do we ever need to delete a chat without deleting it's replies? If not we can also merge these methods into one.

Probably not. Good idea! 👍🏻

@MatthiasReumann
Copy link
Collaborator Author

MatthiasReumann commented Jun 14, 2023

Please ignore the go test / test error. We need to update the workflow to go1.20 (#1065) 😬

dao/chat.go Outdated
Comment on lines 167 to 169
return errors.Join(
DB.Model(&model.Chat{}).Delete(&model.Chat{}, id).Error,
DB.Delete(&model.Chat{}, "reply_to = ?", id).Error)
Copy link
Member

@joschahenningsen joschahenningsen Jun 14, 2023

Choose a reason for hiding this comment

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

This seems a little cleaner

Suggested change
return errors.Join(
DB.Model(&model.Chat{}).Delete(&model.Chat{}, id).Error,
DB.Delete(&model.Chat{}, "reply_to = ?", id).Error)
return DB.Where("id = ? OR reply_to = ?", id, id).Delete(&model.Chat{}).Error

@joschahenningsen joschahenningsen merged commit 59e215b into dev Jun 21, 2023
@joschahenningsen joschahenningsen deleted the bug/delete-replies branch June 21, 2023 09:39
SebiWrn pushed a commit that referenced this pull request May 7, 2024
* Add DeleteReplies

* Fix tests

* Merge functions

* Update dao function
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants