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

org settings: Handle floating point durations better. #9256

Closed

Conversation

shubhamdhama
Copy link
Member

Fixes: #9253.

Tests: Added node tests.
(Casper may fail, I'll fix them once below points are addressed)

Needed feedback about:

  • Up to what precision we want to handle floating point value as there are various problems in converting minutes in decimal to seconds and vice-versa beyond a single floating point. Currently, I've limited it to one floating point.
  • I guess there should be a minimum value below which we don't need to set edit limit like don't allow value set below 10 sec and maybe we should enforce this in backend and frontend both, thoughts?
  • There is a small bug if the limit is set to <0.05 it get converted to 0.0 and automatically set to "Any time". This bug will get fixed once above point is addressed.
  • How we want to show pure integer time: 100 or 100.0

@timabbott @rishig FYI.

@zulipbot
Copy link
Member

Hello @zulip/server-settings members, this pull request was labeled with the "area: settings (admin/org)" label, so you may want to check it out!

@timabbott
Copy link
Sponsor Member

On your questions:

  • I think we want pure integer time to be displayed as 100, not 100.0; we should only show a decimal point if it's required.
  • I think it's reasonable to just round to multiples of 0.1 (aka 6 seconds); that should be "good enough" resolution and avoids the issue of unclean decimal numbers we'd get with trying to do multiples of 1 second.
  • I think if the limit is set low enough that we'd round down to 0 seconds rather than up to 6 seconds, it should convert to "Never", not "Any time".

The Casper failure looks like a bug?

@shubhamdhama
Copy link
Member Author

@timabbott I've updated the PR, let me know how is this approach!

@rishig
Copy link
Member

rishig commented Apr 28, 2018

I think these should be integer only. We don't enforce things on a 6 second accuracy anyway; e.g. we give something like a 10 second grace period.

@timabbott
Copy link
Sponsor Member

Oops, already merged this. You're probably right though; we should just change it to an integer field in the form (and fix the styling to make those not super broken looking).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

message editing and deletion settings: Handle floating point durations better
4 participants