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

Make sure TimeZone doesn't store to database just by setting the prop #3037

Merged
merged 2 commits into from
Dec 6, 2019
Merged

Make sure TimeZone doesn't store to database just by setting the prop #3037

merged 2 commits into from
Dec 6, 2019

Conversation

donker
Copy link
Contributor

@donker donker commented Sep 28, 2019

Currently when setting the TimeZone property on the PortalSettings object it will persist to the database and clears the cache. This is obviously not a good idea. The front end already sets the TimeZone when an admin changes the value in the site settings personabar module.

@donker donker added this to the 9.4.2 milestone Sep 28, 2019
@donker
Copy link
Contributor Author

donker commented Sep 29, 2019

This fails the LoadPortalSettings_Sets_TimeZone_Property_To_Local_TimeZone test. I have no idea how to adjust this test so it will pass. We'll need to discuss.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

I am ok with this

}
}

public TimeZoneInfo TimeZone { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we continue defaulting this to TimeZoneInfo.Local?

Suggested change
public TimeZoneInfo TimeZone { get; set; }
public TimeZoneInfo TimeZone { get; set; } = TuneZoneInfo.Local;

Copy link
Contributor

Choose a reason for hiding this comment

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

Watch out, you have TuneZone instead of TimeZone in your suggested change here...

@valadas
Copy link
Contributor

valadas commented Oct 3, 2019

After thinking about this again, is this just a code cleanup effort or is it causing a bug? If this is just cleanup, I think I would prefer targetting this for 10 release just to make sure we don't break something along the way. Everything that touches times scares me :)

@valadas valadas modified the milestones: 9.4.2, 10.0.0 Oct 15, 2019
@donker donker modified the milestones: 10.0.0, 9.5.0 Nov 17, 2019
@donker
Copy link
Contributor Author

donker commented Nov 17, 2019

After thinking about this again, is this just a code cleanup effort or is it causing a bug? If this is just cleanup, I think I would prefer targetting this for 10 release just to make sure we don't break something along the way. Everything that touches times scares me :)

From my point of view it's code cleanup

@valadas valadas changed the base branch from release/9.4.x to develop December 3, 2019 23:21
@valadas
Copy link
Contributor

valadas commented Dec 3, 2019

@donker can you look at the conflict and the failing test...

Copy link
Contributor

@bdukes bdukes left a comment

Choose a reason for hiding this comment

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

I looked into the failing test and my suggested change (without the typo) was what we needed to get that test to pass again. This should be green now and ready to merge.

@bdukes bdukes merged commit 315eb2e into dnnsoftware:develop Dec 6, 2019
@donker donker deleted the timezoneps branch January 15, 2020 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants