Skip to content
This repository was archived by the owner on Jun 18, 2024. It is now read-only.

TimeWindow.writeToXmlUnscopedDatesOnly should have UTC set on formatter#396

Merged
serious6 merged 2 commits intoOfficeDev:masterfrom
cfraser:bug/TimeWindowUnscopedDatesOnlyUTC
Aug 21, 2015
Merged

TimeWindow.writeToXmlUnscopedDatesOnly should have UTC set on formatter#396
serious6 merged 2 commits intoOfficeDev:masterfrom
cfraser:bug/TimeWindowUnscopedDatesOnlyUTC

Conversation

@cfraser
Copy link
Copy Markdown

@cfraser cfraser commented Aug 7, 2015

This pull request resolves an issue where the TimeWindow can be incorrectly shifted because it is serialized to "yyyy-MM-dd'T'00:00:00" in the timezone of the server and deserialized in UTC. This occurs for two reasons:

Additionally, treating a date with no timezone as UTC also conforms to Table 2 in the Handling Timezones in EWS section of Time zones and EWS in Exchange.

* set the timezone to UTC on the formatter in ```writeToXmlUnscopedDatesOnly```
@azurecla
Copy link
Copy Markdown

azurecla commented Aug 7, 2015

Hi @cfraser, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.azure.com.

TTYL, AZPRBOT;

* removed private method that effectively duplicated ```TimeWindow.loadFromXml()```
@cfraser cfraser force-pushed the bug/TimeWindowUnscopedDatesOnlyUTC branch from 0b15e7a to 9beddf2 Compare August 7, 2015 17:46
@cfraser
Copy link
Copy Markdown
Author

cfraser commented Aug 7, 2015

I'm investigating if I need legal sign-off for the CLA.

@serious6
Copy link
Copy Markdown
Member

In my opinion the whole TimeWindow class should be replaced with joda#period. Any comments?

@cfraser
Copy link
Copy Markdown
Author

cfraser commented Aug 10, 2015

Hrm, I'm not sure that TimeWindow and joda#period overlap neatly.

I think we could back the TimeWindow with a joda period that functions on 24 hour blocks. I agree that the use of epochs is misleading, so it would be nice to provide an API that self documents the full day constraint that TimeWindow enforces. Of course that's a breaking change, so it's something we could put up for consideration in 3.x.x.

We also need to consider that TimeWindow has some specific code for serializing the class to xml so we likely can't completely replace it with joda.

@serious6
Copy link
Copy Markdown
Member

this PR should be referenced against the 2.0.x branch as this will be a minor change.

@cfraser
Copy link
Copy Markdown
Author

cfraser commented Aug 12, 2015

Talked to legal, now I'm waiting on my senior director to sign then we should be good.

@cfraser
Copy link
Copy Markdown
Author

cfraser commented Aug 12, 2015

Should I open another PR to the 2.0.x branch once this merges into master?

@serious6
Copy link
Copy Markdown
Member

can you also add some explanation on why we need this change in detail?

@cfraser
Copy link
Copy Markdown
Author

cfraser commented Aug 14, 2015

I'll add it to the PR description shortly.

@cfraser
Copy link
Copy Markdown
Author

cfraser commented Aug 15, 2015

@serious6 I've updated the PR description to explain my reasoning for the pull request. Please let me know if you need any further details, or have any concerns.

@serious6
Copy link
Copy Markdown
Member

@cfraser cla still needs to be signed.

@cfraser
Copy link
Copy Markdown
Author

cfraser commented Aug 19, 2015

@serious6 I've signed it and I'm waiting on my senior manager to sign. I just sent him a follow up email to try and move things along.

@azurecla
Copy link
Copy Markdown

@cfraser, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, AZPRBOT;

serious6 added a commit that referenced this pull request Aug 21, 2015
Fix: #241 TimeWindow.writeToXmlUnscopedDatesOnly should have UTC set on formatter
@serious6 serious6 merged commit 7e612cb into OfficeDev:master Aug 21, 2015
@serious6
Copy link
Copy Markdown
Member

@cfraser thanks for your contribution.

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.

Availability Service timewindow shifted by timezone

3 participants