feat: Add Hours, Minutes, Seconds fields while taking input as idleTimeLimit #33708

thepiyush-303 · 2024-10-22T16:56:36Z

Proposed changes (including videos or screenshots)

Before

After

Issue(s)

#33566

dionisio-bot · 2024-10-22T16:56:39Z

Looks like this PR is not ready to merge, because of the following issues:

This PR is missing the 'stat: QA assured' label
This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

changeset-bot · 2024-10-22T16:56:40Z

🦋 Changeset detected

Latest commit: 4137e43

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

This PR includes changesets to release 35 packages

Name	Type
@rocket.chat/meteor	Patch
@rocket.chat/core-typings	Patch
@rocket.chat/rest-typings	Patch
@rocket.chat/uikit-playground	Patch
@rocket.chat/api-client	Patch
@rocket.chat/apps	Patch
@rocket.chat/core-services	Patch
@rocket.chat/cron	Patch
@rocket.chat/ddp-client	Patch
@rocket.chat/freeswitch	Patch
@rocket.chat/fuselage-ui-kit	Patch
@rocket.chat/gazzodown	Patch
@rocket.chat/livechat	Patch
@rocket.chat/model-typings	Patch
@rocket.chat/ui-contexts	Patch
@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/license	Patch
@rocket.chat/omnichannel-services	Patch
@rocket.chat/pdf-worker	Patch
@rocket.chat/presence	Patch
rocketchat-services	Patch
@rocket.chat/network-broker	Patch
@rocket.chat/models	Patch
@rocket.chat/ui-avatar	Patch
@rocket.chat/ui-client	Patch
@rocket.chat/ui-video-conf	Patch
@rocket.chat/ui-voip	Patch
@rocket.chat/web-ui-registration	Patch
@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

…iyush-303/Rocket.Chat into addingFeatureInUserPresence

thepiyush-303 · 2024-10-26T11:25:20Z

Hello @dougfabris ,
Can you please review my changes.

Gustrb · 2024-10-27T23:08:05Z

Hey @thepiyush-303 thanks for your contribution :)
There is no need to ping anyone, your PR will be reviewed as soon as possible.

But taking a closer look at it, it concerns me that we would need to have 3 redundant settings, I think that if we want to follow the approach of having 3 input fields, we could convert their respective values to seconds and just store that, wdyt?

thepiyush-303 · 2024-10-28T10:25:09Z

I understand your concern about having redundant settings. Converting the input values to seconds and storing that sounds like a more efficient approach.

However, I’ve encountered a couple of issues while implementing this. Firstly, when I change the field values and save them, they revert to the default values when I navigate back to edit them again.

Additionally, I'm running into an "invalid params" error when I attempt to save the values. This appears to be due to the extra fields not being recognized by the form.

Gustrb · 2024-10-28T10:48:15Z

I am not a frontend dev, but I don't think there is a need to store the value the same way the user put in, for example:

the user inserts 61 minutes, we convert it to 3660 seconds and whenever we load the page we display the value as 1 hour and 1 minute.

it does not sound like a complicated algorithm

The invalid fields error is probably because the backend is not expecting those new fields you are sending

.changeset/wild-sheep-hug.md

apps/meteor/client/startup/startup.ts

apps/meteor/server/methods/saveUserPreferences.ts

thepiyush-303 · 2024-10-28T11:36:22Z

I am not a frontend dev, but I don't think there is a need to store the value the same way the user put in, for example:

the user inserts 61 minutes, we convert it to 3660 seconds and whenever we load the page we display the value as 1 hour and 1 minute.

it does not sound like a complicated algorithm

The invalid fields error is probably because the backend is not expecting those new fields you are sending

Thanks for your input! The issue is that the user needs to provide input in hours, minutes, and seconds. Once they enter the values, if they want to see the changes in the fields, the backend would indeed need to support that. That’s why I chose the approach in the PR. If I'm misunderstanding something, please let me know!

Gustrb · 2024-10-28T11:41:44Z

That is what I mean, the backend should not support multiple settings because we should not have multiple settings.
I am pretty sure no one will approve that, the settings for hour, minute and second should be just comestic, internally we should convert to seconds, otherwise we will need to have a specifc way to fetch a specific user preference everytime, we should avoid that bacause it is error prone and confusing

thepiyush-303 · 2024-10-28T11:51:51Z

Got it, thanks for the clarification! Should I leave this issue as is, or is there anything else you'd like me to address?

Gustrb · 2024-10-28T11:57:39Z

I mean, if you have the time I think it would be a cool addition to do it in the client

thepiyush-303 · 2024-10-28T12:05:27Z

I mean, if you have the time I think it would be a cool addition to do it in the client

but since you mentioned that having multiple settings might not be approved by other collaborators, I'm hesitant to continue with this approach. If it’s a significant enhancement, I’d be happy to explore it further if it aligns with the team's direction.

Gustrb · 2024-10-28T12:10:11Z

What I mean by that is, we have user preferences that we store in the database, I don't think those inputs should be mapped to actual values in the database, I think it would be interesting to convert their values to seconds and keep storing seconds.
The only change that I think would fit in this case is adding the inputs so we can have a better granularity in that field, I don't think that granularity should go all the way to the databse, it should be nothing more than an abstraction.

thepiyush-303 · 2024-11-12T19:03:08Z

@Gustrb
I have made the requested changes.

Gustrb · 2024-11-12T19:19:55Z

Hey @thepiyush-303 did you update the PR?
I couldn't see your changes :)

also, thanks for your dedication

.changeset/wild-sheep-hug.md

apps/meteor/client/startup/startup.ts

Gustrb · 2024-11-13T00:15:44Z

apps/meteor/client/views/account/preferences/PreferencesUserPresenceSection.tsx

@@ -27,14 +30,19 @@ const PreferencesUserPresenceSection = () => {
 					</FieldRow>
 				</Field>
 				<Field>
-					<FieldLabel htmlFor={idleTimeLimit}>{t('Idle_Time_Limit')}</FieldLabel>
+					<FieldLabel htmlFor={idleTimeLimitH}>{t('Idle_Time_Limit')}</FieldLabel>


don't you have access to a OnChange callback or smth?
so you can update idleTimeLimit without needing to have 3 separate settings

apps/meteor/client/views/account/preferences/useAccountPreferencesValues.ts

apps/meteor/server/methods/saveUserPreferences.ts

thepiyush-303 · 2024-11-14T18:38:37Z

As per your suggestion, I'm working on the second approach to resolve this issue, but I'm currently stuck on a particular challenge.

When I modify the seconds, minutes, and hours fields, I'm able to convert and store these values as a total in seconds. If you'd like more details, I'm happy to raise a new PR for further clarification.

…iyush-303/Rocket.Chat into addingFeatureInUserPresence

thepiyush-303 · 2024-12-15T18:35:46Z

Hello @Gustrb ,
I have made the changes you suggested. Specifically:

I updated my approach to storing values in three different settings, aligning with your earlier suggestion.
I have tested the changes, and everything is working fine.

Additionally, I have added a new file. Could you please advise on the appropriate location for this file?

Finally, could you suggest any changes needed to ensure that my code adheres to Rocket's coding standards?

addHoursMinSecFields

34d5fb9

thepiyush-303 requested review from a team as code owners October 22, 2024 16:56

Merge branch 'develop' into addingFeatureInUserPresence

57161f7

thepiyush-303 mentioned this pull request Oct 22, 2024

UX: Idle Time Limit misses unit #33566

Open

thepiyush-303 and others added 3 commits October 26, 2024 16:29

chore:changeset

49c34f7

Merge branch 'develop' into addingFeatureInUserPresence

d4dc02c

Merge branch 'addingFeatureInUserPresence' of https://github.com/thep…

ffbee29

…iyush-303/Rocket.Chat into addingFeatureInUserPresence

Gustrb requested changes Oct 28, 2024

View reviewed changes

.changeset/wild-sheep-hug.md Outdated Show resolved Hide resolved

apps/meteor/client/startup/startup.ts Outdated Show resolved Hide resolved

apps/meteor/server/methods/saveUserPreferences.ts Outdated Show resolved Hide resolved

fix Some files

50ab19e

thepiyush-303 requested a review from Gustrb November 11, 2024 13:40

Gustrb changed the title ~~feat: Add Hours ,Minutes,Seconds fields while taking input as idleTimeLimit~~ feat: Add Hours, Minutes, Seconds fields while taking input as idleTimeLimit Nov 12, 2024

Gustrb reviewed Nov 13, 2024

View reviewed changes

.changeset/wild-sheep-hug.md Outdated Show resolved Hide resolved

Gustrb reviewed Nov 13, 2024

View reviewed changes

apps/meteor/client/startup/startup.ts Outdated Show resolved Hide resolved

Gustrb reviewed Nov 13, 2024

View reviewed changes

apps/meteor/client/views/account/preferences/useAccountPreferencesValues.ts Outdated Show resolved Hide resolved

Gustrb reviewed Nov 13, 2024

View reviewed changes

apps/meteor/server/methods/saveUserPreferences.ts Outdated Show resolved Hide resolved

thepiyush-303 added 2 commits December 15, 2024 19:24

draft

357db55

Merge branch 'addingFeatureInUserPresence' of https://github.com/thep…

1081ce2

…iyush-303/Rocket.Chat into addingFeatureInUserPresence

thepiyush-303 closed this Dec 15, 2024

newApproach

c1d385e

thepiyush-303 reopened this Dec 15, 2024

thepiyush-303 added 2 commits December 15, 2024 20:00

newdraft

1366917

better approach

2d7506e

thepiyush-303 added 2 commits December 16, 2024 00:16

lint

dbbe733

lint

7de98a7

thepiyush-303 requested a review from Gustrb December 15, 2024 19:00

thepiyush-303 and others added 6 commits December 16, 2024 00:45

lint

aeeb204

lint

0677a65

lint

4545ae7

merge conflicts

2003296

conflicts

526cdc0

Merge branch 'develop' into addingFeatureInUserPresence

4137e43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Hours, Minutes, Seconds fields while taking input as idleTimeLimit #33708

feat: Add Hours, Minutes, Seconds fields while taking input as idleTimeLimit #33708

thepiyush-303 commented Oct 22, 2024

dionisio-bot bot commented Oct 22, 2024

changeset-bot bot commented Oct 22, 2024 •

edited

Loading

thepiyush-303 commented Oct 26, 2024

Gustrb commented Oct 27, 2024 •

edited

Loading

thepiyush-303 commented Oct 28, 2024

Gustrb commented Oct 28, 2024 •

edited

Loading

thepiyush-303 commented Oct 28, 2024

Gustrb commented Oct 28, 2024

thepiyush-303 commented Oct 28, 2024

Gustrb commented Oct 28, 2024

thepiyush-303 commented Oct 28, 2024

Gustrb commented Oct 28, 2024

thepiyush-303 commented Nov 12, 2024

Gustrb commented Nov 12, 2024

Gustrb Nov 13, 2024

thepiyush-303 commented Nov 14, 2024 •

edited

Loading

thepiyush-303 commented Dec 15, 2024

feat: Add Hours, Minutes, Seconds fields while taking input as idleTimeLimit #33708

Are you sure you want to change the base?

feat: Add Hours, Minutes, Seconds fields while taking input as idleTimeLimit #33708

Conversation

thepiyush-303 commented Oct 22, 2024

Proposed changes (including videos or screenshots)

Issue(s)

dionisio-bot bot commented Oct 22, 2024

changeset-bot bot commented Oct 22, 2024 • edited Loading

🦋 Changeset detected

thepiyush-303 commented Oct 26, 2024

Gustrb commented Oct 27, 2024 • edited Loading

thepiyush-303 commented Oct 28, 2024

Gustrb commented Oct 28, 2024 • edited Loading

thepiyush-303 commented Oct 28, 2024

Gustrb commented Oct 28, 2024

thepiyush-303 commented Oct 28, 2024

Gustrb commented Oct 28, 2024

thepiyush-303 commented Oct 28, 2024

Gustrb commented Oct 28, 2024

thepiyush-303 commented Nov 12, 2024

Gustrb commented Nov 12, 2024

Gustrb Nov 13, 2024

Choose a reason for hiding this comment

thepiyush-303 commented Nov 14, 2024 • edited Loading

thepiyush-303 commented Dec 15, 2024

changeset-bot bot commented Oct 22, 2024 •

edited

Loading

Gustrb commented Oct 27, 2024 •

edited

Loading

Gustrb commented Oct 28, 2024 •

edited

Loading

thepiyush-303 commented Nov 14, 2024 •

edited

Loading