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

Date picker picks wrong date #2377

Open
3 tasks done
Tychodewaard opened this issue Oct 8, 2018 · 16 comments
Open
3 tasks done

Date picker picks wrong date #2377

Tychodewaard opened this issue Oct 8, 2018 · 16 comments

Comments

@Tychodewaard
Copy link
Contributor

Description of bug

If you assign a role to a user, you can pick a start date and end date to apply to this role.
The date is actually 1 day earlier than the day you pick in the date picker. This happens to both the start date and end date.

Steps to reproduce

  • go to a users
  • assign role
  • click on the start date icon (for instance, October 7th 2018)
  • check the field in the table and see that it says October 6th 2018

Expected result

I expect the date in the table to correspond with the date selected in the date picker.

Affected version

  • 9.2.2
  • 9.2.1
  • 9.1.1

Other versions not tested.

@Tychodewaard
Copy link
Contributor Author

Just checked DNN804 and in that version, all worked as expected.

@Tychodewaard
Copy link
Contributor Author

In 9.3RC1 this issue is still active.

Back in the Telerik days the way to solve this issue was to change new Date(1900, 1, 1) to new Date(1900, 0, 1) as months go from 0-11 (source: https://www.telerik.com/forums/raddatepicker---selecting-a-day-is-getting-the-previous-day)

But as it moved to the persona bar, I'm not sure where/how to verify this.

@valadas
Copy link
Contributor

valadas commented Feb 7, 2019

I cannot reproduce in the latest build, the dates match in the table, can you try RC2 and let us know.

@valadas
Copy link
Contributor

valadas commented Feb 7, 2019

Oh, sorry, the latest is RC1... Can anyone else reproduce, could this be time zone related or something. It works fine here...

@daguiler
Copy link
Contributor

daguiler commented Feb 7, 2019

I was able to reproduce. The database server's time zone needs to be ahead of GMT for this to happen.

@Tychodewaard
Copy link
Contributor Author

@valadas how can we move forward on this one? As a matter of fact most of Europe is ahead of GMT, so it's kind of a biggie :-)

@valadas
Copy link
Contributor

valadas commented Mar 27, 2019

Well, I guess someone needs to build a test environment using a database server in a timezone after gmt, then follow the code to see where the math is wrong and submit a PR... It may be in the platform api, or maybe in the persona bar UI, not sure yet. Right or wrong, the current way is to base time on the databse server so that we have a unique time reference if the site is on a load balancer or something that would make it be on multiple time zones. The database being on a single server kinda prevents variable timezones. Then I would assume it needs to convert from the database timezone to the site timezone by some math.

Now, I am not even sure what "database timezone" means exactly... Is it the timezone of windows settings on the machine that has the database or does the database have a timezone of itself, if so, should this be UTC to have a correct base time reference in the data. If you ask me, times should always be stored UTC, but then if your site is started in the wrong timezone and you change it, then all existing data will get offset, also, admins may be in different timezones too, so a time for one admin may be another time for another admin. And what is a site timezone for a site that is international? Then should be base the math on the user timezone and convert that to UTC (relative to the user currently editing) before saving that?

I guess it would be good to always store times as UTC and show them relative to the User timezone. But I also guess we need some kind of agreement on this and maybe publish some best practices so that everybody follows that decided pattern, then we can go fix those bugs according to that decision. ?

@Tychodewaard
Copy link
Contributor Author

@skamphuis ran into a simular situation. His fix:
// fix the date of birth
if (posted.DateOfBirth.HasValue && posted.DateOfBirth.Value.Kind == DateTimeKind.Utc)
{
posted.DateOfBirth = posted.DateOfBirth.Value.ToLocalTime();
}

When he is a little less busy, he'll take a look whether this would fix this issue as well.

@skamphuis
Copy link
Contributor

skamphuis commented Mar 28, 2019

Obiously, these lines of code solved the issue in a piece of custom software: it uses the current time on the server, which is definitely not a good solution in DNN. I agree with @valadas that the biggest problem in the world of DNN is to decide to which timezone we need the time to be set.
So while this could very well be a hint to solving the issue, the complete fix requires more than just a code change...

@Tychodewaard
Copy link
Contributor Author

Is there any way to move forward? Because in DNN 8 all was working fine. But I don't want to move to a previous version.

@valadas
Copy link
Contributor

valadas commented Jun 9, 2019

Well, so far nobody submitted a PR for this... I have a lot on my plate and no proper testing environment to reproduce the issue. If someone from Europe submits a PR soon, I will be glad to review, if not, I will get to it when possible.

@valadas
Copy link
Contributor

valadas commented Jul 28, 2019

I looks like if people may have workaround in place, fixing this has the potential be be a breaking change, so pushing this to Dnn 10 milestone for now.

@LeoZandvliet
Copy link

Is this affected by PR #3919 or is the solution related to it?
I have no knowledge of the code, I'm incapable to test the patch.

@sleupold
Copy link
Contributor

sleupold commented Sep 8, 2020

@LeoZandvliet
as far as I can see, #3919 does not affect role dates and deals with page visibility only.

@Tychodewaard
Copy link
Contributor Author

@LeoZandvliet @sleupold Just tested on 9.7.0 and the issue still persists

@rodrigoratan
Copy link
Contributor

Will try to check this out in the next DNN OpenSource Co-coding session

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment