Add duration property to the Item model to support the new duration feature.

[AhmedZaki99]

Fixes #34

Changes include:

• Adding a Duration model to be mapped against the task duration JSON object.
• Adding a DurationUnit model that extends StringEnum and supports two values: minute & day.
• Adding a Duration property of type Duration in the Item model.

[olsh]

Could you please write at least one integration tests with duration?

[olsh]

I'd suggest to change ifs to switch pattern in this way static analyzers will find an issue if we add a new DurationUnit in the future.

[AhmedZaki99]

You have a good point, but the problem is that we are dealing with static values that won't work with switch patterns.
I've looked it up and found a way to make the code more readable, following this SO answer, I did the following:

switch (Unit)
{
    case var _ when Unit == DurationUnit.Minute:
        return TimeSpan.FromMinutes(Amount);
    case var _ when Unit == DurationUnit.Day:
        return TimeSpan.FromDays(Amount);
    default:
        throw new NotImplementedException();
}

Unfortunately, this still doesn't guide static analyzers to help us add support for new values.

I thought of adding an enum that can be mapped to each of the DurationUnit values, but again, analyzers will be useful only if new values are added both to the class and the enum.

Maybe there's something that I'm missing, what do you think?

[olsh]

Thank you for the contribution! 👍🏻

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[AhmedZaki99]

Could you please write at least one integration tests with duration?

You were right about this, the test I wrote was failing at first, but then I found that the Duration property was being ignored when set to null, so I added an exception for it in the JsonProperty attribute:

[JsonProperty("duration", NullValueHandling = NullValueHandling.Include)]
public Duration Duration { get; set; }

In addition to the integration test, I wrote a couple of unit tests for the Duration object, which have guided me later to move the validation checks on the constructor parameters to be executed within property setters instead, e.g.:

public int Amount
{
    get => _amount;
    set => _amount = value > 0
        ? value
        : throw new ArgumentOutOfRangeException(nameof(Amount), "Duration amount must be greater than zero.");
}

All tests are passing now, but on my machine, the integration test I added was failing due to inconsistency in conversion between DueDate.StringDate and DueDate.Date properties.
When the task contains a floating due date, it is treated like a local time when being converted from DateTime to string in the DueDate.StringDate property getter, hence, its string value changes from what it was given by the API.

In my case (time zone is GMT +2), the value "2021-12-22T09:15:00" fetched from the API was converted to a DateTime of kind DateTimeKind.Unspecified, and then converted back to a "2021-12-22T07:15:00" when sent back to the API for update.
That results in an unwanted update on the task without even changing any field.

I think I might've faced a similar issue while I was working on my project and it's clear now why it's happening. I will wait until we get done with this pull request and then open an issue ticket for it.