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

Fix DueDate.StringDate conversion to and from DateTime. #37

Merged
merged 4 commits into from
Oct 26, 2023

Conversation

AhmedZaki99
Copy link
Contributor

Fixes #36

First, existing unit tests for the DueDate model should be modified to run on different time zones to ensure that date conversion works correctly no matter where it runs.
That's why on the first commit, a FakeLocalTimeZone helper class is added to mimic different time zones for tests when necessary (see this SO question for more details), which is used with the DueDate model tests to randomize the time zone in which tests are run.

Second, additional tests are added for the DueDate model to test the date conversion in the StringDate property getter and setter, which fail as expected.

Last, the StringDate to Date conversion process has been modified to use the DateTimeStyles.RoundtripKind with the DateTime.Parse method. Then when converting back, the round-trip format is used instead of the sortable-date format, with the exclusion of milliseconds.

Note: The DueDate floating date assignment unit test has been modified to use a date with DateTimeKind.Unspecified instead of DateTimeKind.Utc since floating dates are treated now as non-UTC dates.

The class is used to change the time zone of the tests upon initialization, and resets it back when disposed.
The added tests ensure consistency in StringDate property setter and getter.
Date is now converted using the round-trip format without milliseconds.
Date is also no longer being converted to universal time before formatting.
DateTimeKind is switched to Unspecified instead of Utc since the due date should be treated as floating.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@olsh
Copy link
Owner

olsh commented Oct 26, 2023

Thank you for your contribution! 👍🏻

@olsh olsh merged commit 465061e into olsh:master Oct 26, 2023
olsh added a commit that referenced this pull request Oct 26, 2023
olsh added a commit that referenced this pull request Oct 26, 2023
@olsh
Copy link
Owner

olsh commented Oct 26, 2023

@AhmedZaki99
Sorry, but I found an issue with the PR. This test fails with the changes.

[Trait(Constants.TraitName, Constants.IntegrationFreeTraitValue)]
public void CreateNewItem_DueDateIsLocal_DueDateNotChanged()
{
var client = TodoistClientFactory.Create(_outputHelper);
var item = new Item("New task") { DueDate = new DueDate(DateTime.Now.AddYears(1).Date) };
var taskId = client.Items.AddAsync(item).Result;
var itemInfo = client.Items.GetAsync(taskId).Result;
Assert.Equal(item.DueDate.Date, itemInfo.Item.DueDate.Date?.ToLocalTime());
client.Items.DeleteAsync(item.Id).Wait();
}

I should have run the test before merging the PR, my bad.
I've reverted the PR with the new PR.
#39

Please open a new PR with fixes.
Also, I've reopened the related issue #36

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.

Inconsistency in conversion between the DueDate.StringDate and DueDate.Date properties
2 participants