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.

Sign up for GitHub

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

Jump to bottom

[FIX] making the cleanRoomHistory method delete the files and attachments when deleting the messages #11046

Closed
wants to merge 6 commits into from
Closed

[FIX] making the cleanRoomHistory method delete the files and attachments when deleting the messages #11046

wants to merge 6 commits into from

Conversation

MarcosSpessatto
Copy link
Member

Closes #10870

@MarcosSpessatto MarcosSpessatto added this to the 0.66.0 milestone Jun 7, 2018
@MarcosSpessatto MarcosSpessatto self-assigned this Jun 7, 2018
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11046 June 7, 2018 21:06 Inactive
ggazzo
ggazzo reviewed Jun 11, 2018
View reviewed changes
packages/rocketchat-lib/server/methods/cleanRoomHistory.js Outdated
RocketChat.models.Uploads.remove({
rid: roomId,
uploadedAt: {
$gte: oldest,
Copy link
Member

@ggazzo ggazzo Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@MarcosSpessatto what do you think about use

const gt = inclusive ? '$gte' : 'gt'
uploadedAt: {
					[gt] :.....

ggazzo
ggazzo previously approved these changes Jun 11, 2018
View reviewed changes
Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

the PR is good to me, just tell me what do you think about the suggestion

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-11046 June 11, 2018 22:05 Inactive
@MarcosSpessatto
Copy link
Member Author

@ggazzo I think it's a better approach, it reduces the amount of code.

@vynmera
Copy link
Contributor

vynmera commented Jun 14, 2018

How will this impact the performance of this method? On a large team with many messages, clearing a lot of them takes very very long, so long it causes a gateway timeout...

@rodrigok
Copy link
Member

@MarcosSpessatto this does not remove the attachments, just the records that register the attachments location/url and metadata, wouldn't be better to delete the delete the files too?

vynmera
vynmera previously approved these changes Jun 22, 2018
View reviewed changes
@vynmera
Copy link
Contributor

vynmera commented Jun 25, 2018

I've implemented further changes to this method in #11236. However, merging this may be useful for the time being until that PR is ready to merge.

As for deleting the files fully, we can use FileUpload.getStore('Uploads').deleteById(message.file._id);

@MarcosSpessatto
Copy link
Member Author

@rodrigok This great work by @vynmera closes this issue. 💯

@ggazzo ggazzo modified the milestones: 0.66.0, 0.67.0 Jul 4, 2018
@vynmera vynmera mentioned this pull request Jul 15, 2018
Merged
42 tasks
@theorenck theorenck modified the milestones: 0.67.0, 0.68.0 Jul 19, 2018
ggazzo pushed a commit that referenced this pull request Jul 20, 2018
@vynmera @ggazzo
[NEW][BREAK] Message retention policy and pruning (#11236)
829c5d1 
Closes #6749
Closes #8321
Closes #9374
Closes #2700
Closes #2639
Closes #2355 
Closes #1861
Closes #8757
Closes #7228
Closes #10870
Closes #6193 
Closes #11299
Closes #11468
Closes #9317
Closes #11300 (will incorporate a fix to this PR's issue)
Closes #11046 (will incorporate a fix to this PR's issue)
Contributes to #5944 
Contributes to #11475
_...and possibly more!_

This PR makes deleting messages (automatically and manually) a lot easier on Rocket.Chat.

- [X] Implement a bulk message deletion notification, to quickly push large message deletions to users without reload
  - [X] Use it in `rooms.cleanHistory`
  - [X] Use it in user deletions
- [X] Completely remove cleanChannelHistory as required by v0.67
  - [X] Remove server method `cleanChannelHistory`
  - [X] Remove REST API `channels.cleanHistory`
- [x] Implement a sidebar option to clean history
  - [x] Basic implementation
  - [x] Allow excluding pinned messages
  - [x] Allow attachment-only mode
  - [x] Allow specifying user(s) to narrow down to
    - [x] Also update REST API
    - [x] Also update docs
  - [x] Break the deletion into multiple different requests, so the client can keep track of progress
  - [x] Clear animation / progress bar for deleting
- [x] Retention policy
  - [X] Global, set by admin
    - [X] Global timer that runs every second and deletes messages over the set limit
      - [X] Can change its timer's resolution to prevent insane CPU overhead
    - [X] Admin can decide what room types to target (channels, groups and/or DMs)
    - [X] Allow excluding pinned messages
    - [X] Allow attachment-only mode
  - [x] Per-channel, set by those with a new permission
    - [x] Disabled when master switch off
    - [x] Set in channel info
    - [x] Can override global policy with a switch that requires `edit-privileged-setting`
    - [x] Allow excluding pinned messages
    - [x] Allow attachment-only mode
    - [x] Uses same global timer for cleanup
  - [X] Message at start of channel history / in channel info if there is a retention policy set
  - [x] Message in channel info if there is a retention policy set on that channel specifically
- [X] Make cleaning history also delete files (completely!)
  - [X] Manual purging
  - [X] Automatic purging
- [x] Make other deletions also delete files
  - [x] User deletion
    - [X] Own messages
    - [x] DMs with them's partner messages
  - [x] Room deletion
- [x] Cleanup
- [x] Finish related [docs](https://github.com/RocketChat/docs/pull/815)
- [x] Link to the docs in the settings

Please suggest any cool changes/additions! Any support is greatly appreciated.

**Breaking change:** This PR removes REST API endpoint `channels.cleanHistory` and Meteor callable `cleanChannelHistory` as per the protocol specified for them.

![bzzzzzzzz](https://user-images.githubusercontent.com/39674991/41799087-56d1dea0-7670-11e8-94c0-bc534b1f832d.png)
@rodrigok rodrigok deleted the fix-clean-room-history branch February 15, 2019 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vynmera vynmera vynmera left review comments

@ggazzo ggazzo ggazzo left review comments

Assignees

@MarcosSpessatto MarcosSpessatto

Labels
feat: api feat: rest type: bug
Projects
None yet
Milestone
0.68.0
Development

Successfully merging this pull request may close these issues.
6 participants
@MarcosSpessatto @vynmera @rodrigok @ggazzo @engelgabriel @theorenck