Reopen "Fix DueDate.StringDate conversion to and from DateTime."

[AhmedZaki99]

Fixes #36
All previous details of this request are discussed in #37

While I was checking this failing test, I noticed that the Roundtrip date conversion depends on information about timezones given in the DueDate.Date object itself, however, Todoist API always treats the DueDate.Date object as a UTC date and depends on the DueDate.Timezone to store timezone info.

I will try to fix this issue and update the pull request with a new commit shortly.

[AhmedZaki99]

@olsh
I am not sure what is the proper way to solve this, since all timezone data is given in a separate parameter (DueDate.Timezone), what should happen when the user provides a local date in the DueDate constructor?
I know that we can use the old approach (converting the given date to universal), but wouldn't that cause some confusion to the users?

For example, when a user's code tries to create a floating due date with a local Date object, the time will be adjusted with the offset of whatever timezone the server is running in, resulting in inconsistent behavior if multiple server instances are running in different locations.

So, should we prevent Local date objects in the constructor (throw an ArgumentException), and assume that Unspecified dates are UTC?
Or should we ignore any Date.Kind given in the constructor and assume any given date as UTC?

[olsh]

I'll take a look at the problem deeper.

BTW, here is the related issue
#1

[AhmedZaki99]

@olsh
After some thinking, I figured out a way to handle different kinds of given dates.

First of all, for floating due dates, we ignore any timezone info given in the DateTime.Kind property and only use the time data given to us:

private const string FloatingEventDateFormat = "yyyy-MM-ddTHH:mm:ss";

...

if (string.IsNullOrEmpty(Timezone))
{
    return Date.Value.ToString(FloatingEventDateFormat);
}

That is because floating dates only represent a time in the user's timezone, no matter where he is:

Due date always represent an event in current user's timezone.

So, the code makes sure that whatever date is given should be the same date we find in the task without conversion.

Second, for fixed due dates, we should provide time in UTC since all timezone info is stored in DueDate.Timezone property:

Due date is stored in UTC.

That's why any dates given with the kind Unspecified are assumed UTC, but only dates given with the kind Local are converted to UTC based on the timezone of the machine running the code:

private const string FixedEventDateFormat = "yyyy-MM-ddTHH:mm:ssZ";

...

if (Date.Value.Kind == DateTimeKind.Local)
{
    return Date.Value.ToUniversalTime().ToString(FixedEventDateFormat);
}

return Date.Value.ToString(FixedEventDateFormat); // Unspecified dates are assuemd UTC.

Conversion of local DateTime objects is helpful in cases where the code is being used by a local client application (not a hosted server), where the user would expect to find the task date matching with the local date provided if the fixed timezone is, in fact, the one he lives in.

Commits with these changes have been added, only one simple extra change is made to the failing test:

Assert.Equal(item.DueDate.Date, itemInfo.Item.DueDate.Date);

Since floating due dates are now converted to Unspecified date time, the method ToLocalTime is no longer used because it assumes the given date as UTC, but the conversion of floating dates is not necessary because it doesn't represent any timezone.

I hope this approach solves the problem.

[olsh]

@AhmedZaki99 thank you for the contribution. These changes totally make sense to me.
I'd also suggest removing the public constructors and creating three template methods for each date type, something like:

DueDate.CreateFullDay(...)
DueDate.CreateFloating(...)
DueDate.CreateFixedTimeZone(...)

With only necessary parameters. I think the API is more explicit and easy to understand.

Also, it's worth adding comments to the methods from the Todoist docs. Because the due date concept is pretty hard to understand.

[AhmedZaki99]

@olsh Great suggestion!

Should I create another pull request for it or should I just add the commits here?

[olsh]

Let's add the changes to this PR.

[AhmedZaki99]

Three commits are added:

1. DueDate public constructors have been replaced by four static methods to create new instances (detailed documentation comments are provided):

DueDate FromText(string text, [Language language = null]);
DueDate CreateFullDay(DateTime date);
DueDate CreateFloating(DateTime dateTime);
DueDate CreateFixedTimeZone(DateTime dateTime, string timezone);

2. Static methods are now handling different DateTime kinds, so the DueDate.StringDate property getter has been reverted to just do Roundtrip date conversion:

private const string DefaultEventDateFormat = "yyyy-MM-ddTHH:mm:ssK";

...

if (IsFullDay)
{
    return Date.Value.ToString(FullDayEventDateFormat);
}

return Date.Value.ToString(DefaultEventDateFormat);

3. Tests have been updated with the new static methods replacing the public constructors, for example:

var item = new Item("demo task") { DueDate = DueDate.FromText("22 Dec 2021", Language.English) };

[AhmedZaki99]

P.S. Don't forget to run all tests, I am not sure what might break after all these changes.

[olsh]

Great job! Let's do the last changes and merge the PR.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

[olsh]

Thank you! 👍🏻