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

[bugfix]: switch to UTC for release dates #743

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

iiPythonx
Copy link
Contributor

This commit switches DayJS to use UTC rather then local time when processing absolute dates, this fixes a bug where albums would show as releasing a day before they really did.

Some quick tests I did:

  • Album release (and original) dates are correct.
  • createdAt and dateAdded fields are still correct.

If you see anything wrong let me know, but I've tested every file that uses this function and they all seem to work correctly.

Copy link

vercel bot commented Sep 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
feishin ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 13, 2024 1:35am

Copy link
Collaborator

@kgarner7 kgarner7 left a comment

Choose a reason for hiding this comment

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

While this is fine for release date, it is not necessarily fine for things like createdAt/updatedAt (date added), which from Navidrome at least include timezone. It would be more correct to conditionally call local() for certain locations

@iiPythonx
Copy link
Contributor Author

iiPythonx commented Sep 13, 2024

In that case, would you prefer the default being left as local and then adding a UTC version?
Edit: did that instead.

@kgarner7 kgarner7 merged commit 8d8826a into jeffvli:development Sep 13, 2024
3 checks passed
@iiPythonx iiPythonx deleted the fix-date branch September 13, 2024 01:36
@Optekah
Copy link

Optekah commented Sep 16, 2024

Thank you for fixing this! I have entries tagged 1983-09 showing up as Aug 31, 1983

When this is implemented, will that 1983-09 release now display as Sept, 1983 or Sept 1, 1983 on an album page?

@kgarner7
Copy link
Collaborator

Thank you for fixing this! I have entries tagged 1983-09 showing up as Aug 31, 1983

When this is implemented, will that 1983-09 release now display as Sept, 1983 or Sept 1, 1983 on an album page?

You can test this live on https://feishin.vercel.app, but in my testing it would be Sep 1, 1983. Basically, take the string, treat it as UTC+0 and then display it that way (without time offsets).

@Optekah
Copy link

Optekah commented Sep 16, 2024

You can test this live on https://feishin.vercel.app

Just tried it and there's indeed a bug present where it incorrectly states it was released Sept 1, 1983 instead of just Sept, 1983.

Basically, take the string, treat it as UTC+0 and then display it that way (without time offsets).

As someone who has no dev experience, I have no idea what any of this means :P Is it a way for users to fix the above problem?

@iiPythonx
Copy link
Contributor Author

Just tried it and there's indeed a bug present where it incorrectly states it was released Sept 1, 1983 instead of just Sept, 1983.

I don't recall there ever being an option for it to say Sept, 1983 rather then including the day. That could be added after-the-fact, but nothing you would be able to do (inside Feishin) without writing some more changes.

@Optekah
Copy link

Optekah commented Sep 16, 2024

Checking my Navidrome GUI, it correctly says Sept, 1983.

Just took a look at a different release that is tagged with only YYYY instead of one that is tagged YYYY-MM like my above example, and the way it's displayed is particularly egregious in its incorrectness. It displays as Jan 1, 2000 even though the band didn't even exist for at least another six or seven months after that— they met and formed sometime around June 2000.

I don't recall there ever being an option for it to say Sept, 1983 rather then including the day.

Would it be appropriate if I submit a separate issue for this to be changed / implemented?

I suppose it comes down to the concept that displaying no information is better than displaying wrong information; one I would assume most of us would agree with :P

@kgarner7
Copy link
Collaborator

730683f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants