feat: Allow different time units for retention policy #32425

gabriellsh · 2024-05-13T14:29:00Z

Proposed changes (including videos or screenshots)

Issue(s)

CORE-302
CORE-303

Steps to test or reproduce

Further comments

dionisio-bot · 2024-05-13T14:29:04Z

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

changeset-bot · 2024-05-13T14:29:05Z

🦋 Changeset detected

Latest commit: 02b5ddf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 34 packages

Name	Type
@rocket.chat/meteor	Minor
@rocket.chat/core-typings	Minor
@rocket.chat/i18n	Minor
@rocket.chat/apps	Patch
@rocket.chat/core-services	Patch
@rocket.chat/cron	Patch
@rocket.chat/fuselage-ui-kit	Major
@rocket.chat/gazzodown	Major
@rocket.chat/livechat	Patch
@rocket.chat/model-typings	Patch
@rocket.chat/rest-typings	Minor
@rocket.chat/ui-contexts	Major
@rocket.chat/account-service	Patch
@rocket.chat/authorization-service	Patch
@rocket.chat/ddp-streamer	Patch
@rocket.chat/omnichannel-transcript	Patch
@rocket.chat/presence-service	Patch
@rocket.chat/queue-worker	Patch
@rocket.chat/stream-hub-service	Patch
@rocket.chat/api-client	Patch
@rocket.chat/license	Patch
@rocket.chat/omnichannel-services	Patch
@rocket.chat/pdf-worker	Patch
@rocket.chat/presence	Patch
rocketchat-services	Patch
@rocket.chat/mock-providers	Patch
@rocket.chat/web-ui-registration	Major
@rocket.chat/uikit-playground	Patch
@rocket.chat/models	Patch
@rocket.chat/ddp-client	Patch
@rocket.chat/ui-avatar	Major
@rocket.chat/ui-client	Major
@rocket.chat/ui-video-conf	Major
@rocket.chat/instance-status	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2024-05-13T14:45:11Z

Codecov Report

Attention: Patch coverage is 95.34884% with 4 lines in your changes missing coverage. Please review.

Project coverage is 56.59%. Comparing base (c0828b4) to head (02b5ddf).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #32425      +/-   ##
===========================================
+ Coverage    56.54%   56.59%   +0.04%     
===========================================
  Files         2484     2483       -1     
  Lines        54746    54790      +44     
  Branches     11305    11321      +16     
===========================================
+ Hits         30958    31007      +49     
- Misses       21139    21142       +3     
+ Partials      2649     2641       -8

Flag	Coverage Δ
unit	`72.29% <98.64%> (+0.18%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

apps/meteor/client/views/admin/settings/inputs/TimespanSettingInput.tsx

apps/meteor/server/startup/migrations/v305.ts

apps/meteor/client/views/admin/settings/inputs/TimespanSettingInput.tsx

MarcosSpessatto

Doing some local tests, looks like there's something weird going on with the feature.

1 - The callout seems wrong

2- Executing the branch locally, the values for the new settings look weird.

3 - It's not possible to use the same granularity if the user wants to override the global retention policy, we can use only days (old behavior)

4 - It seems the messages are not being pruned after the specified time (1 minute), I'm not sure if it's respecting what the callout says... Or if it's just not working.

apps/meteor/server/startup/migrations/v305.ts

gabriellsh · 2024-05-20T00:29:25Z

@MarcosSpessatto

1 - The callout seems wrong

Will be fixed

2 - Executing the branch locally, the values for the new settings look weird.

Seems like an issue with the migration. I'm suspecting the packageValue is changing before the migration runs, so I'll have to do something different there

3 - 3 - It's not possible to use the same granularity if the user wants to override the global retention policy, we can use only days (old behavior)

There will be a fix to the input value but this form specifically will stay as it is for now. It was not part of the scope as far as I'm aware

4 - It seems the messages are not being pruned after the specified time (1 minute), I'm not sure if it's respecting what the callout says... Or if it's just not working.

This depends on the cron running, which is managed through settings above the ones I changed. Both callout and warning will be changing to reflect the cron time.

…retention * 'develop' of github.com:RocketChat/Rocket.Chat: (36 commits) refactor: IntegrationHistory out of DB Watcher (#32502) fix: Message update being broadcasted without updated values (#32472) test: make api teams test fully independent (#31756) test: Fix test name (#32490) fix: streams being called with no logged user (#32489) feat: Un-encrypted messages not allowed in E2EE rooms (#32040) feat(UiKit): Users select (#31455) fix: Re-login same browser tab issues (#32479) chore: move all webclient code out of the COSS folders (#32273) chore(deps): bump thehanimo/pr-title-checker from 1.3.7 to 1.4.1 (#30619) fix: Don't show join default channels option for edit user form (#31750) fix: CAS user merge not working (#32444) fix: Overriding Retention Policy not working (#32454) fix: `rooms.export` endpoint generates an empty export when given an invalid date (#32364) fix: "Allow Password Change for OAuth Users" setting is not honored in the "Forgot Password" flow (#32398) fix: Bypass trash when removing OTR system messages and read receipts (#32269) fix: Monitors dissapearing from Unit upon edit (#32393) fix: Link image preview not opening in gallery (#32391) feat: Allow visitors & integrations to access downloaded files after a room has closed (#32439) regression: Users tab misaligned (#32451) ...

apps/meteor/server/startup/migrations/v305.ts

apps/meteor/app/retention-policy/server/cronPruneMessages.ts

apps/meteor/client/lib/convertTimeUnit.spec.ts

apps/meteor/client/views/room/hooks/useRetentionPolicy.ts

apps/meteor/server/startup/migrations/xrun.ts

apps/meteor/client/lib/convertTimeUnit.ts

…retention * 'develop' of github.com:RocketChat/Rocket.Chat: fix: Not possible to edit room without proper permission with retention policy enabled (#32547) feat: Apps-Engine Deno Runtime update (#31821) feat: E2EE room setup header (#32446) fix: E2EE thread main message reactivity (#32381) chore: Add telemetry to CI so we can get a better understanding of resource usage (#32113) fix: Long katex strings breaking overflow in x axis (#32609) fix: Force highlighted code language registration (#32507) fix: sidebar last message E2EE (#32431) chore: remove message column on moderation console (#32432) fix: Accepted Media Types settings validation (#32478)

done

dougfabris

This is a really good and solid work.
Really well coded and you added great tests to cover your work.

My only concern here is related to the consistency of the feature. In my opinion we should have keep the same time unit possibilities when overriding retention policy in the rooms. But it seems a product decision to follow like this.

Because of that, I'm approving it on behalf of frontend team!

gabriellsh added 3 commits May 13, 2024 11:15

Settings and translations

7ee66fb

Timespan input

1c7467c

cron

8d55014

gabriellsh added 2 commits May 13, 2024 11:30

add cs

183357e

Merge branch 'develop' into imp/retention

62e38fc

add unit tests

d7ae3bd

gabriellsh marked this pull request as ready for review May 15, 2024 17:18

gabriellsh requested review from a team as code owners May 15, 2024 17:18

scuciatto added this to the 6.9 milestone May 16, 2024

KevLehman requested changes May 16, 2024

View reviewed changes

clean migration

ee32132

Gustrb reviewed May 16, 2024

View reviewed changes

apps/meteor/client/views/admin/settings/inputs/TimespanSettingInput.tsx Outdated Show resolved Hide resolved

gabriellsh added 2 commits May 16, 2024 18:57

Review and QA

430e960

review and QA 2

6d13c8f

MarcosSpessatto requested changes May 17, 2024

View reviewed changes

apps/meteor/server/startup/migrations/v305.ts Outdated Show resolved Hide resolved

apps/meteor/server/startup/migrations/v305.ts Outdated Show resolved Hide resolved

gabriellsh added 2 commits May 19, 2024 22:32

Fix other places that use the setting

d24ba1b

fix migration check

ed03337

scuciatto modified the milestones: 6.9, 6.10 May 21, 2024

dionisio-bot bot added the stat: conflict label May 21, 2024

scuciatto modified the milestones: 6.10, 7.0 May 21, 2024

dionisio-bot bot removed the stat: conflict label May 28, 2024

scuciatto added this to the 6.10 milestone Jun 10, 2024

KevLehman previously requested changes Jun 10, 2024

View reviewed changes

gabriellsh and others added 5 commits June 11, 2024 10:47

remove the balaca

48e4c0c

update translation

b7039a5

Review

3b01f2d

fix lint

c5f280e

Merge branch 'develop' into imp/retention

457e539

MarcosSpessatto requested a review from KevLehman June 17, 2024 11:40

MarcosSpessatto reviewed Jun 17, 2024

View reviewed changes

apps/meteor/client/lib/convertTimeUnit.ts Show resolved Hide resolved

MarcosSpessatto previously approved these changes Jun 18, 2024

View reviewed changes

jessicaschelly added the stat: QA assured Means it has been tested and approved by a company insider label Jun 19, 2024

dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jun 19, 2024

Merge branch 'develop' into imp/retention

bf4db11

Merge branch 'develop' into imp/retention

97b16db

dougfabris previously approved these changes Jun 20, 2024

View reviewed changes

ggazzo added 2 commits June 20, 2024 16:28

Merge branch 'develop' into imp/retention

533f8bf

Merge branch 'develop' into imp/retention

02b5ddf

ggazzo dismissed stale reviews from dougfabris and MarcosSpessatto via 02b5ddf June 21, 2024 02:06

ggazzo approved these changes Jun 21, 2024

View reviewed changes

ggazzo merged commit dfa49bd into develop Jun 21, 2024
36 of 39 checks passed

ggazzo deleted the imp/retention branch June 21, 2024 02:36

This was referenced Jun 22, 2024

Release 6.10.0 #32655

Closed

Release 6.10.0 #32656

Merged

krzysztof-osiecki mentioned this pull request Aug 26, 2024

RocketChat 6.11.1 prunes messages to the previous month #33146

Closed

gabriellsh mentioned this pull request Sep 9, 2024

fix: Retention Policy cached settings not updated during upgrade procedure #33237

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow different time units for retention policy #32425

feat: Allow different time units for retention policy #32425

gabriellsh commented May 13, 2024 •

edited

Loading

dionisio-bot bot commented May 13, 2024 •

edited

Loading

changeset-bot bot commented May 13, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

MarcosSpessatto left a comment

gabriellsh commented May 20, 2024 •

edited

Loading

dougfabris left a comment •

edited

Loading

feat: Allow different time units for retention policy #32425

feat: Allow different time units for retention policy #32425

Conversation

gabriellsh commented May 13, 2024 • edited Loading

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

dionisio-bot bot commented May 13, 2024 • edited Loading

changeset-bot bot commented May 13, 2024 • edited Loading

🦋 Changeset detected

codecov bot commented May 13, 2024 • edited Loading

Codecov Report

MarcosSpessatto left a comment

Choose a reason for hiding this comment

gabriellsh commented May 20, 2024 • edited Loading

dougfabris left a comment • edited Loading

Choose a reason for hiding this comment

gabriellsh commented May 13, 2024 •

edited

Loading

dionisio-bot bot commented May 13, 2024 •

edited

Loading

changeset-bot bot commented May 13, 2024 •

edited

Loading

codecov bot commented May 13, 2024 •

edited

Loading

gabriellsh commented May 20, 2024 •

edited

Loading

dougfabris left a comment •

edited

Loading