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

Modify update to compare dates/timestamps better #1006

Merged
merged 3 commits into from
Feb 29, 2024

Conversation

jrchudy
Copy link
Member

@jrchudy jrchudy commented Feb 27, 2024

This PR addresses an issue in chaise where a timestamptz value can be submitted to ermrest for update but the value was unchanged. This can occur if the old/new value has fractional seconds and the other value doesn't. This can also occur when the timezone of the user is different from the timezone the data is returned in.

For the case of date and timestamp, it's almost unlikely that this precision matters but it's still worth converting these values to similar formats anyways to ensure a consistent comparison.

using isSame() from moment

For comparing dates and timestamps, instead of defining the format and outputting the moment object using .format(), I’m letting moment parse the value without telling it the format. To compare the values, using isSame() works how we expect when the values can be properly parsed to moment objects.

On thing to note, f the values are both null, moment will create an object with “invalid date” as the value for both. When you use isSame() for 2 invalid dates that both were created from null, they are considered different.

Related ermrestJS issue #463

…alues to the same representative value for date, timestamp, and timestamptz
@jrchudy jrchudy self-assigned this Feb 27, 2024
@jrchudy jrchudy requested a review from RFSH February 27, 2024 21:12
@jrchudy jrchudy added the bug label Feb 27, 2024
@jrchudy jrchudy linked an issue Feb 27, 2024 that may be closed by this pull request
Copy link
Member

@RFSH RFSH left a comment

Choose a reason for hiding this comment

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

Apart from the minor code style changes that I mentioned, I think you should add test cases for this.

js/reference.js Outdated Show resolved Hide resolved
js/reference.js Outdated Show resolved Hide resolved
@jrchudy jrchudy requested a review from RFSH February 28, 2024 21:24
@jrchudy jrchudy merged commit 7949f4f into master Feb 29, 2024
1 check passed
@jrchudy jrchudy deleted the update-compare-dates-better branch February 29, 2024 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reference.update doesn't check all values for changes properly
2 participants