feat: Allow different time units for retention policy #32425
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 3bab15c The changes in this PR will be included in the next version bump. This PR includes changesets to release 34 packages
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #32425 +/- ##
===========================================
- Coverage 56.35% 55.65% -0.70%
===========================================
Files 2434 2406 -28
Lines 53708 53324 -384
Branches 11057 10995 -62
===========================================
- Hits 30267 29678 -589
- Misses 20803 21032 +229
+ Partials 2638 2614 -24
Flags with carried forward coverage won't be shown. Click here to find out more. |
| } | ||
|
|
||
| export const timeUnitToMs = (unit: TIMEUNIT, timespan: number) => { | ||
| if (unit === TIMEUNIT.days) { |
There was a problem hiding this comment.
Think a switch would look better here
| }; | ||
|
|
||
| export const msToTimeUnit = (unit: TIMEUNIT, timespan: number) => { | ||
| if (unit === TIMEUNIT.days) { |
apps/meteor/client/views/admin/settings/inputs/TimespanSettingInput.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@RocketChat/architecture are we okay about adding this migration here for a minor release?
Will be fixed
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
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
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) ...
| export const getHighestTimeUnit = (value: number): TIMEUNIT => { | ||
| const minutes = msToTimeUnit(TIMEUNIT.minutes, value); | ||
| if (minutes % 60 !== 0) { | ||
| return TIMEUNIT.minutes; | ||
| } | ||
|
|
||
| const hours = msToTimeUnit(TIMEUNIT.hours, value); | ||
| if (hours % 24 !== 0) { | ||
| return TIMEUNIT.hours; | ||
| } | ||
|
|
||
| return TIMEUNIT.days; | ||
| }; |
There was a problem hiding this comment.
is this correct? the name of the function isn't very clear to me, but my guess is it's trying to see the longest the number goes.
I see two issues here,
first is what if in L15, minutes is literally 61?
Second, this assumes two types, ints and floats, and assumes all conversions up to % are in ints. That is not true in js.
msToTimeUnit can return with a decimal part while integer part is > 0 (or multiplier for the respective unit).
imagine passing a huge number (milliseconds) that is divisible by 1000 (for second), 60 (for minute), 60 (hour), plus 1. 3600001.
When converted to minutes using your function, you get 60.<something>. Pass through the first branch and you get a false positive. 60.001 (e.g.) % 60 != 0, yes, but that doesn't mean the highest unit for that number is minutes, it is actually hour.
There was a problem hiding this comment.
There's 2 caveats.
First, this function is meant to get the highest possible TIMEUNIT, meaning if the number isn't divisible by 60, it cannot be transformed into hours, nor can it be transformed into days, since converting it into higher time units would return a non whole value (e.g if minutes are not divisible by 60, it'll never be a whole hour nor will it ever be a whole day. This extends to the other timeunits aswell).
There's no issue if the value is not a whole number, it just means the time will be displayed as minutes. The input does not accept decimal numbers, so there shouldn't ever be a time where the value of the setting cannot be described as a whole number.
You can however save the setting through the API to whatever number of MS you want. If the value turns out to be some value with decimal cases, it'll just be converted to minutes and rounded back to a whole number again. And this will be just on the setting input, the actual value of the setting will still be the same unless you change something in the inputs.
There was a problem hiding this comment.
TL;DR; it works as intended for the setting input. This is not used elsewhere and although all the points made are true, those were taken into account for providing the desired behaviour.
There was a problem hiding this comment.
First, this function is meant to get the highest possible TIMEUNIT, meaning if the number isn't divisible by 60, it cannot be transformed into hours, nor can it be transformed into days, since converting it into higher time units would return a non whole value (e.g if minutes are not divisible by 60, it'll never be a whole hour nor will it ever be a whole day. This extends to the other timeunits aswell).
There's no issue if the value is not a whole number, it just means the time will be displayed as minutes. The input does not accept decimal numbers, so there shouldn't ever be a time where the value of the setting cannot be described as a whole number.
But, if we have 61 minutes, the UI should say what's closest. Otherwise you'll get to minutes (59) .. hour (60) .. minute (61) .. minute (62) ... minute(119) .. 2 hours (120) .. UI will incorrectly switch to minutes if it's not a whole hour, which is true for the largest span of time. Same goes for days. technically you will never stay long in hours since anything not divisible by 60 (first condition) even though > 60 will always result in unit being minutes. Like 24 hours and 1 minute will result in minute as the unit not 1 day. 25th hour becomes hour, not day.
There was a problem hiding this comment.
better way i think is to start with the largest unit check, and simply doing if (value >= 24) .. return day; getHours; if (value >= 60) return hour; getMinutes ; if (value >= 60) return minute;
There was a problem hiding this comment.
The input does not accept not-whole values, so you can’t put in 1,5 hours or something similar. If you want to be more granular you should go to the previous time unit. This is more of a UX issue than implementation issue, if you still think this shouldn't be as it is, the design team should be included in the discussion.
IMO the way it is provides enough granularity without increasing the complexity of the UI too much. What you pointed out about 60 minutes being treated as an hour, and 61 minutes being treated as minutes is actually how I intended this to be. We do not save the selected time unit, just the time in milliseconds, so the purpose of the function is to get the highest time unit that is a whole number.
If you check this line of the unit tests you'll see that I tested the specific case you're pointing out.
There was a problem hiding this comment.
Btw this is the describer of the test
it('should return minutes if milliseconds cannot be evenly divided into hours or days'
| await Settings.find({ _id: { $in: Array.from(maxAgeSettingMap.keys()) } }, { projection: { _id: 1, value: 1 } }).forEach( | ||
| ({ _id, value }) => { | ||
| if (!maxAgeSettingMap.has(_id)) { | ||
| throw new Error(`moveRetentionSetting - Setting ${_id} equivalent does not exist`); | ||
| } | ||
|
|
||
| void Settings.update( | ||
| { | ||
| _id: maxAgeSettingMap.get(_id), | ||
| }, | ||
| { | ||
| $set: { | ||
| value: convertDaysToMs(Number(value)), | ||
| }, | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
| await Settings.find({ _id: { $in: Array.from(maxAgeSettingMap.keys()) } }, { projection: { _id: 1, value: 1 } }).forEach( | |
| ({ _id, value }) => { | |
| if (!maxAgeSettingMap.has(_id)) { | |
| throw new Error(`moveRetentionSetting - Setting ${_id} equivalent does not exist`); | |
| } | |
| void Settings.update( | |
| { | |
| _id: maxAgeSettingMap.get(_id), | |
| }, | |
| { | |
| $set: { | |
| value: convertDaysToMs(Number(value)), | |
| }, | |
| }, | |
| ); | |
| }, | |
| ); | |
| }; | |
| await Settings.find({ _id: { $in: Array.from(maxAgeSettingMap.keys()) } }, { projection: { _id: 1, value: 1 } }).map( | |
| async ({ _id, value }) => { | |
| if (!maxAgeSettingMap.has(_id)) { | |
| throw new Error(`moveRetentionSetting - Setting ${_id} equivalent does not exist`); | |
| } | |
| await Settings.update( | |
| { | |
| _id: maxAgeSettingMap.get(_id), | |
| }, | |
| { | |
| $set: { | |
| value: convertDaysToMs(Number(value)), | |
| }, | |
| }, | |
| ); | |
| }, | |
| ); | |
| }; | |
Proposed changes (including videos or screenshots)
Issue(s)
CORE-302
CORE-303
Steps to test or reproduce
Further comments