Desktop: Fixes #10182: Detailed note list doesn't follow preferred date and time formats #10204
Conversation
packages/lib/time.ts
Outdated
| @@ -89,8 +89,8 @@ class Time { | |||
| ); | |||
| } | |||
|
|
|||
| public unixMsToLocalDateTime(ms: number): string { | |||
| return moment.unix(ms / 1000).format('DD/MM/YYYY HH:mm'); | |||
| public unixMsToLocalDateTime(ms: number, format?: string): string { | |||
There was a problem hiding this comment.
So as you can see in this file, there's already a formatMsToLocal() method that does the same thing and that is already setup to use the preferred date/time format. Can this be used instead?
There was a problem hiding this comment.
Ah, that was an oversight on my part. I basically modified unixMsToLocalDateTime to do what formatMsToLocal already does. I have made the changes to use formatMsToLocal instead.
| 'note.user_updated_time': () => time.formatMsToLocal(value, options.dateTimeFormat), | ||
| 'note.user_created_time': () => time.formatMsToLocal(value, options.dateTimeFormat), | ||
| 'note.updated_time': () => time.formatMsToLocal(value, options.dateTimeFormat), | ||
| 'note.created_time': () => time.formatMsToLocal(value, options.dateTimeFormat), | ||
| 'note.todo_completed': () => value ? time.formatMsToLocal(value, options.dateTimeFormat) : '', |
There was a problem hiding this comment.
If you look at formatMsToLocal you see that it's taking a default argument, right? And what is the value of the default argument. Isn't it already the user's preferred date/time format?
There was a problem hiding this comment.
Yeah that makes sense. I have made the change and also made some changes to the test.
| expect(view).toEqual({ | ||
| note: { | ||
| user_updated_time: time.formatMsToLocal(timeValue), | ||
| user_created_time: time.formatMsToLocal(timeValue), | ||
| tags: 123, | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I feel this is not testing much because timeValue is used to generate both the input and expected value, which is generally not a good idea in tests. For example here if formatMsToLocal is broken and returns "undefined", your test will pass even though it's obviously wrong.
The way around this is to use static values in tests. Set timeValue to a known hard-coded timestamp in milliseconds. From that check that you got the expected (hard-coded) formatted date as output.
There was a problem hiding this comment.
The way around this is to use static values in tests. Set timeValue to a known hard-coded timestamp in milliseconds. From that check that you got the expected (hard-coded) formatted date as output.
That makes sense, but I think the problem here is that because formatMsToLocal converts the timestamp to the user's local time, even if I hard code the expected value, the test would fail in a different timezone. I wonder if there could be another approach to test formatMsToLocal here, perhaps mocking moment?
There was a problem hiding this comment.
Ok so please remove the test then since it does nothing
Summary
Made changes so that the format of date and time shown in the detailed note list matches the format selected under General > Date format and Time format.
Added a test to check this behavior.
Fixes #10182
Testing
This has been tested successfully on Windows 11 and Linux.