[50954] Record progress in multiple units

[cbliard]

https://community.openproject.org/wp/50954

[aaron-contreras]

eslint is complaining about the chronic_duration.js file. This is taken from Gitlab's implementation and is meant to be kept inline with their chronic_duration gem. There's no npm package for chronic_duration.js as it's directly part of the gitlab source code. Would vendoring the file prevent eslint from trying to check it or is there a config to exclude this file from eslint. Probably has been done before in the core repo but unsure of how to handle it @oliverguenther

[aaron-contreras]

@cbliard 'eqeqeq' eslint issue addressed more globally in #15560

[cbliard]

I played a little with it. It responds really well. Good job!
3 things I wonder about, and probably @psatyal can help us on it:

• what do we do with text longer than the allowed size? Currently it's clipping. Do we extend the default width?
• what about the precision? When entering 1m, it can't represent it accurately with only 2 decimals. It results to "1m 12s". Not a big deal, but it's kinda surprising to enter "2h 12m" and end up with "2h 12m 0.0000000001s", or enter "2h 13m" and end up with "2h 13m 12.00000000001s". Should we switch from floating points holding nb of hours to integer holding nb of seconds?
• when omitting the "m", or before entering it, the value is interpreted as hours. It would be better if interpreted as minutes. This is especially visible when entering a value for the first time: with w -> rw, and typing each character one by one, it gives this:
  • work -> remaining work (when 100%)
  • 2 -> 2h
  • 2h -> 2h
  • 2h1 -> 3h (would have been expecting 2h 1m)
  • 2h15 -> 17h (would have been expecting 2h 15m)
  • 2h15m -> 2h 15m

[cbliard]

I can't approve nor request changes on this PR because I created it 😅

The multiple units display is really nice, and the way it's used in the popover is smooth and does not get in the way. That's nice!

I have some tiny remarks here and there.

I also have the feeling that the precision is annoying us. For instance:

• entering "2h 13m" leads to 7980 seconds, converted to 2.216666667 hours, rounded to 2.22 (2 decimals). This value 2.22 is the one being saved in database.
• When frontend is rendering it, it has 2 ways of rendering it:
  • the modal is rendering it through DurationConverter.output, which for 2.22 will render "2h 13m 12s"
  • the angular part is using a different route: the 2.22 is converted in the representer using datetime_formatter.format_duration_from_hours, so 2.22 is converted to iso duration "PT2H13M12S". This duration is then handled in the timezone service which converts it to hours with Number(moment.duration(durationString).asHours().toFixed(2)) which is 2.22, converted to seconds which gives 7992.000000000001, and then it is converted to chronic duration which gives "2h 13m 12.000000000001s"

So the rounding is always false, and sometimes more false than others. I think the second one can be fixed by avoiding converting iso duration to hours and then to second: by converting iso duration directly to seconds, we could avoid precision loss.

Another thing we could do is to avoid displaying seconds entirely by making the hours to seconds conversion remove them. Something like (hours * 3600 + 15).to_i / 60 * 60. For 2.22 hours, it would give 7980 which converts to "2h 13m". No seconds, no errors.

WDYT?

[cbliard]

I feel like this warning should belong to the next section. Currently it looks like it belongs to the wrong one.
Maybe @psatyal can confirm.

[aaron-contreras]

You're right. Played around with it and like it more this way:

[aaron-contreras]

Thoughts @psatyal ?

[aaron-contreras]

This is the commit adfa1bd

in case we agree this is where it should be

[cbliard]

Seen with Parimal during today's frontend meeting:

• the text has been slightly reworded
• the warning is moved on top of the section, so the warning can be read before doing the changes

He also asked if it's possible when clicking "Apply changes" to display the warning message about changed days only if there are actually changed days. I'll do another review to specifically ask for this so we can track it more easily.

[cbliard]

An additional request from Parimal while discussing this screen during today's frontend meeting

[cbliard]

When clicking "Apply changes", there is this "Change working days" information dialog that always opens itself to warn about changing working days.

Parimal would like it to be displayed only if the working days have been actually changed.

Would it be possible?

[cbliard]

I'll create a bug work package for that one.

[cbliard]

I can't approve because I created the PR, but I approve it 👍