Skip to content
This repository was archived by the owner on Jul 6, 2022. It is now read-only.

fix: use created_at for unsynced session history entries#305

Merged
arielsvg merged 4 commits intomasterfrom
fix/note-history-entry
May 26, 2021
Merged

fix: use created_at for unsynced session history entries#305
arielsvg merged 4 commits intomasterfrom
fix/note-history-entry

Conversation

@arielsvg
Copy link
Copy Markdown
Contributor

@arielsvg arielsvg commented May 25, 2021

@arielsvg arielsvg force-pushed the fix/note-history-entry branch from 8f0fe47 to a42296f Compare May 25, 2021 13:21
@arielsvg arielsvg force-pushed the fix/note-history-entry branch from 029de60 to 35bb43d Compare May 25, 2021 14:06
@arielsvg arielsvg marked this pull request as ready for review May 25, 2021 14:22
@arielsvg arielsvg requested review from atmoio and karolsojko May 25, 2021 14:22
protected readonly hasPreviousEntry: boolean;

constructor(payload: SurePayload, previousEntry?: HistoryEntry) {
const updated_at = payload.serverUpdatedAt ?? new Date();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, here we are creating new Date(), which is the present date, but in the constructor of pure_payload we are defaulting to new Date(0), which is 1970. Is this intended? So seems like here after this change if updated_at is nil then entry will default to 1970 rather than present.

Copy link
Copy Markdown
Contributor Author

@arielsvg arielsvg May 25, 2021

Choose a reason for hiding this comment

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

There's a bug with this line in that payload.serverUpdatedAt is always truthy, because the pure_payload constructor always creates a date object. So updated_at can never be new Date().

I don't know what the original intent was, but I think a history entry making up dates is more confusing than helpful. updated_at being equal to the time of construction does not make sense with how the rest of the code treats that kind of field

More broadly speaking I'm not sure setting a payload's updated_at to 1/1/1970 by default is a very accurate way to describe an unsynced payload, but I may be missing something

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the intent is that if creating a new entry, the present time is likely the right time for its creation. But this applies only to the offline use case. I suppose as long as you've tested this functionality in the web app while not signed into an account, and it shows the right date, then all is good.

@atmoio atmoio self-requested a review May 25, 2021 15:35
@arielsvg arielsvg force-pushed the fix/note-history-entry branch from 9c18d19 to db6ddb2 Compare May 26, 2021 09:43
@arielsvg arielsvg merged commit fef99a8 into master May 26, 2021
@arielsvg arielsvg deleted the fix/note-history-entry branch May 26, 2021 10:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants