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.

Sign up for GitHub

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

Jump to bottom

Desktop: Fixes #10182: Detailed note list doesn't follow preferred date and time formats #10204

Merged
merged 4 commits into from
Apr 4, 2024
Merged

Desktop: Fixes #10182: Detailed note list doesn't follow preferred date and time formats #10204

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f55ccb5
#10182 - Use the current date and time format
cagnusmarlsen Mar 25, 2024
cb9c40d
used the correct function
cagnusmarlsen Mar 25, 2024
2d908d5
removed unnecessary argument
cagnusmarlsen Mar 26, 2024
be1b1e5
removed test
cagnusmarlsen Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 29 additions & 0 deletions packages/lib/services/noteList/renderViewProps.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Logger from '@joplin/utils/Logger';
import { RenderNoteView } from '../plugins/api/noteListType';
import renderViewProps from './renderViewProps';
import time from '../../time';

describe('renderViewProps', () => {

Expand Down Expand Up @@ -57,4 +58,32 @@ describe('renderViewProps', () => {
});
});

it('should show correct datetime', async () => {
const timeValue = time.unixMs();
const view: RenderNoteView = {
note: {
user_updated_time: timeValue,
user_created_time: timeValue,
tags: 123,
},
};

Logger.globalLogger.enabled = false;

time.setDateFormat('YYYY-MM-DD');
time.setTimeFormat('HH:mm');

await renderViewProps(view, [], {
noteTitleHtml: '',
});
Logger.globalLogger.enabled = true;

expect(view).toEqual({
note: {
user_updated_time: time.formatMsToLocal(timeValue),
user_created_time: time.formatMsToLocal(timeValue),
tags: 123,
},
});
});
Copy link
Owner

@laurent22 laurent22 Mar 28, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so please remove the test then since it does nothing

});
10 changes: 5 additions & 5 deletions packages/lib/services/noteList/renderViewProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ export interface RenderViewPropsOptions {

const renderViewProp = (name: ListRendererDependency, value: any, options: RenderViewPropsOptions) => {
const renderers: Partial<Record<ListRendererDependency, ()=> string>> = {
'note.user_updated_time': () => time.unixMsToLocalDateTime(value),
'note.user_created_time': () => time.unixMsToLocalDateTime(value),
'note.updated_time': () => time.unixMsToLocalDateTime(value),
'note.created_time': () => time.unixMsToLocalDateTime(value),
'note.todo_completed': () => value ? time.unixMsToLocalDateTime(value) : '',
'note.user_updated_time': () => time.formatMsToLocal(value),
'note.user_created_time': () => time.formatMsToLocal(value),
'note.updated_time': () => time.formatMsToLocal(value),
'note.created_time': () => time.formatMsToLocal(value),
'note.todo_completed': () => value ? time.formatMsToLocal(value) : '',
'note.tags': () => value ? value.map((t: TagEntity) => t.title).join(', ') : '',
'note.title': () => options.noteTitleHtml,
};
Expand Down