From c2cbec2ec82011088b7bb183f86f107e218f1ae4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jernej=20Kavka=20=28JK=29=20=5BSSW=20=C2=B7=20Microsoft=20?= =?UTF-8?q?MVP=5D?= Date: Sun, 13 Sep 2026 01:10:30 +1000 Subject: [PATCH] fix(leave): return the created entry from leave create POST /api/leave/ answers with an empty body, so `tp leave create --json` and the MCP `CreateLeave` tool both reported a bare `{"success":true}` and callers had to run `leave list` to find the id. `LeaveCreateService.ApplyAsync` now re-reads the entry through `LeaveLookup.FindCreatedAsync` (UPCOMING then PAST for the requesting employee, matched on start date, end date, leave type and note) and returns `{"success":true,"leaveId":"","leave":{...}}`. A missing or ambiguous match still reports success, with `leave: null` and a warning whose recovery is `tp leave list` - a second create would duplicate the request. A failed read-back is never allowed to turn a successful write into an error. Dry-run is untouched: same validation, same proposed request, no POST. MCP adopts the same document. Under the wire-shape policy this is a permitted exception: the only committed `create_leave` goldens were the dry-run preview and the API-error payload, so no client-visible create result is being replaced - `Goldens/Mcp/Tools/CreateLeave.created.json` is a new snapshot, generated with UPDATE_MCP_GOLDENS=1 and reviewed. The new parity row runs both surfaces and asserts exactly one POST followed by a GET and no PUT; it stays `ExpectParity = false` only because the CLI omits nulls that MCP writes, the same declared difference as the timesheet write rows. It is appended to the table because parity goldens are named by row index. Review follow-ups: the read-back now swallows every failure except the caller's own cancellation (a `TimeProConnectionException` previously escaped and made a completed write look failed, inviting a duplicate); matching also requires the all-day flag and, for a partial day, the slot to the minute, so two requests on one day cannot be confused; and the recovery names `tp leave list --filter ALL`, which is correct for past dates too. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 15 + .../Features/Leave/CreateCommand.cs | 13 +- .../Features/Leave/LeaveCreateService.cs | 39 ++- .../Features/Leave/LeaveLookup.cs | 110 +++++++ .../Features/Mcp/Tools/LeaveMcpTools.cs | 4 +- .../Mcp/Parity/CreateLeave.15.cli.json | 29 ++ .../Mcp/Parity/CreateLeave.15.mcp.json | 34 ++ .../Mcp/Tools/CreateLeave.created.json | 34 ++ .../Mcp/LeaveCreateReadbackTests.cs | 112 +++++++ .../Mcp/McpCliParityTable.cs | 28 ++ .../Mcp/McpToolCatalog.cs | 12 + .../Mcp/McpToolHost.cs | 2 +- .../Features/Leave/CreateCommandTests.cs | 92 ++++++ .../Features/Leave/LeaveCreateServiceTests.cs | 300 ++++++++++++++++++ .../Features/Mcp/LeaveMcpToolsTests.cs | 2 +- 15 files changed, 816 insertions(+), 10 deletions(-) create mode 100644 tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Parity/CreateLeave.15.cli.json create mode 100644 tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Parity/CreateLeave.15.mcp.json create mode 100644 tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Tools/CreateLeave.created.json create mode 100644 tests/SSW.TimePro.Cli.Integration/Mcp/LeaveCreateReadbackTests.cs create mode 100644 tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveCreateServiceTests.cs diff --git a/AGENTS.md b/AGENTS.md index e298fce..179f440 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -213,6 +213,21 @@ CLI and MCP leave create/update surfaces support dry-run. Dry-run performs the s validation and payload preparation, returns the proposed request, and must not call `CreateLeaveAsync` or `UpdateLeaveAsync`. +Create answers with an empty body, so `LeaveCreateService.ApplyAsync` re-reads the entry through +`LeaveLookup.FindCreatedAsync` (UPCOMING then PAST, matched on leave type, note, all-day flag and +dates — plus the slot to the minute for a partial day, since two partial-day requests on one day +differ only by their times) and both surfaces return +`{"success":true,"leaveId":"","leave":{...}}`. A missing or ambiguous match still reports +success, with `leave: null` and a `warning` whose recovery is `tp leave list --filter ALL` — never a +second create, which would duplicate the request. The read-back swallows every failure except the +caller's own cancellation: a completed write is reported as a success, because surfacing it as an +error is what invites a duplicate submission. + +Date comparison is deliberately asymmetric. All-day entries are compared on the calendar date only +(the server normalises their times and may echo the day in its own offset); partial-day entries +prefer the offset-free `startDateWithoutOffset`/`endDateWithoutOffset` values and otherwise compare +instants shifted into the offset the request was built in. + The cancel endpoint (`PUT /api/leave/{id}/cancel`) requires `LeaveId` (Guid) and `CancellationReason` in the request body. It returns as soon as the server accepts the request: the entry reads `PendingCancellation` and only becomes `Cancelled` minutes later, so `tp leave cancel` never claims the request is diff --git a/src/SSW.TimePro.Cli/Features/Leave/CreateCommand.cs b/src/SSW.TimePro.Cli/Features/Leave/CreateCommand.cs index e4fd89d..59071d8 100644 --- a/src/SSW.TimePro.Cli/Features/Leave/CreateCommand.cs +++ b/src/SSW.TimePro.Cli/Features/Leave/CreateCommand.cs @@ -117,12 +117,19 @@ protected override async Task ExecuteAsync(CommandContext context, Settings return 0; } - await _createService.ApplyAsync(plan, cancellationToken); + var result = await _createService.ApplyAsync(plan, cancellationToken); if (settings.Json) - OutputHelper.WriteJson(new { success = true }); + { + OutputHelper.WriteJson(result); + } else - OutputHelper.WriteSuccess("Leave request created"); + { + OutputHelper.WriteSuccess( + $"Leave request created{(result.LeaveId is not null ? $" (ID: {result.LeaveId})" : "")}"); + if (result.Warning is not null) + OutputHelper.WriteWarning(result.Warning); + } return 0; } diff --git a/src/SSW.TimePro.Cli/Features/Leave/LeaveCreateService.cs b/src/SSW.TimePro.Cli/Features/Leave/LeaveCreateService.cs index 9c63bf5..ede3559 100644 --- a/src/SSW.TimePro.Cli/Features/Leave/LeaveCreateService.cs +++ b/src/SSW.TimePro.Cli/Features/Leave/LeaveCreateService.cs @@ -17,6 +17,19 @@ public sealed record LeaveCreateOptions( public sealed record LeaveCreatePlan(CreateLeaveRequest Request, string TypeLabel); +/// +/// Outcome of a leave create, shared by the CLI --json path and the MCP tool. +/// is the entry re-read after the write, because POST /api/leave/ answers +/// with an empty body. +/// +public sealed record LeaveCreateResult +{ + public bool Success { get; init; } = true; + public string? LeaveId { get; init; } + public LeaveEntry? Leave { get; init; } + public string? Warning { get; init; } +} + public sealed class LeaveCreateValidationException(string message) : Exception(message); /// @@ -25,8 +38,13 @@ public sealed class LeaveCreateValidationException(string message) : Exception(m public sealed class LeaveCreateService { private readonly ITimeProApiClient _api; + private readonly LeaveLookup _lookup; - public LeaveCreateService(ITimeProApiClient api) => _api = api; + public LeaveCreateService(ITimeProApiClient api, LeaveLookup lookup) + { + _api = api; + _lookup = lookup; + } public async Task PrepareAsync( string employeeId, @@ -128,8 +146,23 @@ public async Task PrepareAsync( return new LeaveCreatePlan(request, options.Type); } - public Task ApplyAsync(LeaveCreatePlan plan, CancellationToken ct = default) => - _api.CreateLeaveAsync(plan.Request, ct); + public async Task ApplyAsync(LeaveCreatePlan plan, CancellationToken ct = default) + { + await _api.CreateLeaveAsync(plan.Request, ct); + + var matches = await _lookup.FindCreatedAsync(plan.Request, ct); + if (matches.Count == 1) + return new LeaveCreateResult { LeaveId = matches[0].Id, Leave = matches[0] }; + + // The request was submitted, so the recovery is always to look it up — creating again + // would duplicate it. + var count = matches.Count == 0 ? "no entry matches" : $"{matches.Count} entries match"; + return new LeaveCreateResult + { + Warning = $"Leave request created, but the new entry could not be identified ({count}). " + + "Find it with: tp leave list --filter ALL. Do not create it again." + }; + } private async Task ResolveLeaveTypeAsync(string typeInput, CancellationToken ct) { diff --git a/src/SSW.TimePro.Cli/Features/Leave/LeaveLookup.cs b/src/SSW.TimePro.Cli/Features/Leave/LeaveLookup.cs index b0e9991..3db7c52 100644 --- a/src/SSW.TimePro.Cli/Features/Leave/LeaveLookup.cs +++ b/src/SSW.TimePro.Cli/Features/Leave/LeaveLookup.cs @@ -1,3 +1,4 @@ +using System.Globalization; using SSW.TimePro.Cli.Infrastructure.ApiClient; using SSW.TimePro.Cli.Shared.Models; @@ -36,4 +37,113 @@ public sealed class LeaveLookup return null; } + + /// + /// Locates a freshly created leave request, which the create endpoint answers with an empty + /// body. Returns every entry matching the submitted dates, type and note so an ambiguous + /// result stays ambiguous to the caller. + /// + public async Task> FindCreatedAsync( + CreateLeaveRequest request, + CancellationToken ct = default) + { + try + { + foreach (var filter in new[] { LeaveListService.Upcoming, LeaveListService.Past }) + { + var matches = await MatchesAsync(filter, request, ct); + if (matches.Count > 0) + return matches; + } + } + catch (Exception ex) when (ex is not OperationCanceledException || !ct.IsCancellationRequested) + { + // A completed write is reported as success, full stop: any read-back failure that + // surfaced as an error would invite a duplicate submission. + } + + return []; + } + + private async Task> MatchesAsync( + string filter, + CreateLeaveRequest request, + CancellationToken ct) + { + var matches = new List(); + var pageNumber = 1; + + while (true) + { + var response = await _api.GetLeaveAsync(filter, pageNumber, PageSize, request.RequestedEmpId, ct); + var page = response?.Leaves; + if (page is not null) + matches.AddRange(page.Items.Where(entry => Matches(entry, request))); + + if (page is null || pageNumber >= page.TotalPages) + return matches; + + pageNumber++; + } + } + + private static bool Matches(LeaveEntry entry, CreateLeaveRequest request) => + entry.AllDay == request.AllDay + && entry.LeaveType?.Id == request.LeaveTypeId + && string.Equals(entry.Note?.Trim() ?? "", request.Note?.Trim() ?? "", StringComparison.Ordinal) + && (request.AllDay + ? SameDay(entry.StartDateLocal ?? entry.StartDate, request.StartDate) + && SameDay(entry.EndDateLocal ?? entry.EndDate, request.EndDate) + // Two partial-day requests on one day differ only by their slot, so comparing the + // dates alone would return the wrong entry's id. + : SameSlot(entry.StartDateLocal, entry.StartDate, request.StartDate) + && SameSlot(entry.EndDateLocal, entry.EndDate, request.EndDate)); + + /// + /// All-day requests are compared on the calendar date only, never as instants: the server + /// normalises their times and may render the day in its own offset. + /// + private static bool SameDay(string? left, string? right) => + DatePart(left) is { } day && day == DatePart(right); + + private static string? DatePart(string? value) => + value is null ? null : value.Split('T')[0]; + + /// + /// One end of a partial-day range, to the minute. The offset-free value is preferred; + /// otherwise the two are compared as instants, because the server may echo the same moment in + /// its own zone. + /// + private static bool SameSlot(string? entryLocal, string? entryOffset, string requestValue) + { + if (ParseOffset(requestValue) is not { } requested) + return false; + + if (entryLocal is not null) + return ToMinute(entryLocal) is { } local && local == Minute(requested); + + return ParseOffset(entryOffset) is { } entry + && Minute(entry.ToOffset(requested.Offset)) == Minute(requested); + } + + private static string? ToMinute(string value) => + DatePart(value) is { } day && TimePart(value) is { } time ? $"{day}T{time}" : null; + + private static DateTimeOffset? ParseOffset(string? value) => + DateTimeOffset.TryParse( + value, CultureInfo.InvariantCulture, DateTimeStyles.RoundtripKind, out var parsed) + ? parsed + : null; + + private static string Minute(DateTimeOffset value) => + value.ToString("yyyy-MM-ddTHH:mm", CultureInfo.InvariantCulture); + + private static string? TimePart(string? value) + { + if (value?.Split('T') is not [_, var time]) + return null; + + var parts = time.Split(':'); + return parts.Length >= 2 ? $"{parts[0]}:{parts[1]}" : null; + } } diff --git a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs index 25cf426..437eb9f 100644 --- a/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs +++ b/src/SSW.TimePro.Cli/Features/Mcp/Tools/LeaveMcpTools.cs @@ -133,8 +133,8 @@ public async Task CreateLeave( if (dryRun) return JsonSerializer.Serialize(new { dryRun = true, request = plan.Request }, JsonOpts); - await _leaveCreateService.ApplyAsync(plan, ct); - return JsonSerializer.Serialize(new { success = true }, JsonOpts); + var result = await _leaveCreateService.ApplyAsync(plan, ct); + return JsonSerializer.Serialize(result, JsonOpts); } catch (LeaveCreateValidationException ex) { diff --git a/tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Parity/CreateLeave.15.cli.json b/tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Parity/CreateLeave.15.cli.json new file mode 100644 index 0000000..f9a524e --- /dev/null +++ b/tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Parity/CreateLeave.15.cli.json @@ -0,0 +1,29 @@ +{ + "leave": { + "allDay": true, + "approvedBy": "bob@northwind.example", + "createdAt": "2026-03-01T09:00:00\u002B10:00", + "daysAway": 1, + "endDate": "2026-04-01T23:59:00\u002B10:00", + "id": "0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", + "leaveType": { + "id": 1, + "isActive": true, + "name": "Annual Leave" + }, + "length": 1, + "note": "Family day", + "optionalEmp": [ + "bob@northwind.example" + ], + "requestedEmpId": "BOB", + "startDate": "2026-04-01T00:00:00\u002B10:00", + "status": 1, + "statusName": "Pending", + "updatedAt": "2026-03-02T09:00:00\u002B10:00", + "userEndTime": "18:00", + "userStartTime": "09:00" + }, + "leaveId": "0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", + "success": true +} diff --git a/tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Parity/CreateLeave.15.mcp.json b/tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Parity/CreateLeave.15.mcp.json new file mode 100644 index 0000000..c651214 --- /dev/null +++ b/tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Parity/CreateLeave.15.mcp.json @@ -0,0 +1,34 @@ +{ + "leave": { + "allDay": true, + "approvedBy": "bob@northwind.example", + "cancellationReason": null, + "createdAt": "2026-03-01T09:00:00\u002B10:00", + "daysAway": 1, + "endDate": "2026-04-01T23:59:00\u002B10:00", + "endDateWithoutOffset": null, + "id": "0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", + "leaveType": { + "id": 1, + "isActive": true, + "name": "Annual Leave" + }, + "length": 1, + "note": "Family day", + "optionalEmp": [ + "bob@northwind.example" + ], + "requestedEmpId": "BOB", + "startDate": "2026-04-01T00:00:00\u002B10:00", + "startDateWithoutOffset": null, + "status": 1, + "statusName": "Pending", + "timeLessOverride": null, + "updatedAt": "2026-03-02T09:00:00\u002B10:00", + "userEndTime": "18:00", + "userStartTime": "09:00" + }, + "leaveId": "0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", + "success": true, + "warning": null +} diff --git a/tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Tools/CreateLeave.created.json b/tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Tools/CreateLeave.created.json new file mode 100644 index 0000000..c651214 --- /dev/null +++ b/tests/SSW.TimePro.Cli.Integration/Goldens/Mcp/Tools/CreateLeave.created.json @@ -0,0 +1,34 @@ +{ + "leave": { + "allDay": true, + "approvedBy": "bob@northwind.example", + "cancellationReason": null, + "createdAt": "2026-03-01T09:00:00\u002B10:00", + "daysAway": 1, + "endDate": "2026-04-01T23:59:00\u002B10:00", + "endDateWithoutOffset": null, + "id": "0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", + "leaveType": { + "id": 1, + "isActive": true, + "name": "Annual Leave" + }, + "length": 1, + "note": "Family day", + "optionalEmp": [ + "bob@northwind.example" + ], + "requestedEmpId": "BOB", + "startDate": "2026-04-01T00:00:00\u002B10:00", + "startDateWithoutOffset": null, + "status": 1, + "statusName": "Pending", + "timeLessOverride": null, + "updatedAt": "2026-03-02T09:00:00\u002B10:00", + "userEndTime": "18:00", + "userStartTime": "09:00" + }, + "leaveId": "0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", + "success": true, + "warning": null +} diff --git a/tests/SSW.TimePro.Cli.Integration/Mcp/LeaveCreateReadbackTests.cs b/tests/SSW.TimePro.Cli.Integration/Mcp/LeaveCreateReadbackTests.cs new file mode 100644 index 0000000..824522c --- /dev/null +++ b/tests/SSW.TimePro.Cli.Integration/Mcp/LeaveCreateReadbackTests.cs @@ -0,0 +1,112 @@ +using System.Text.Json.Nodes; +using FluentAssertions; +using Xunit; + +namespace SSW.TimePro.Cli.Integration.Mcp; + +/// +/// The create endpoint answers with an empty body, so both surfaces submit once and then read the +/// entry back. A second submission would duplicate the leave request. +/// +[Collection(CliConsoleCollection.Name)] +public class LeaveCreateReadbackTests : TestBase +{ + private static readonly string[] CliArgs = + [ + "leave", "create", "--start", "2026-04-01", "--end", "2026-04-01", + "--type", "Annual Leave", "--note", "Family day", + "--timezone", McpToolCatalog.TimeZone, "--yes", "--json" + ]; + + [Fact] + public async Task Cli_SubmitsOnceThenReadsTheEntryBack() + { + var ct = TestContext.Current.CancellationToken; + NorthwindApi.StubAll(WireMock); + + var result = await CliRunner.RunAsync(CliArgs, ApiClient, McpToolCatalog.Config(WireMock.Url!), ct); + + result.ExitCode.Should().Be(0); + AssertCreatedEntry(result.Stdout); + AssertOneCreateThenAReadBack(); + } + + [Fact] + public async Task Mcp_SubmitsOnceThenReadsTheEntryBack() + { + var ct = TestContext.Current.CancellationToken; + NorthwindApi.StubAll(WireMock); + + var json = await new McpToolHost(ApiClient, McpToolCatalog.Config(WireMock.Url!)) + .Leave.CreateLeave( + start: "2026-04-01", end: "2026-04-01", type: "Annual Leave", note: "Family day", + timeZoneId: McpToolCatalog.TimeZone, ct: ct); + + AssertCreatedEntry(json); + AssertOneCreateThenAReadBack(); + } + + [Fact] + public async Task WhenNoEntryMatches_NeitherSurfaceSubmitsAgain() + { + var ct = TestContext.Current.CancellationToken; + NorthwindApi.StubAll(WireMock); + NorthwindApi.RawJson( + WireMock, "/api/leave/", "GET", + """{"leaves":{"pageNumber":1,"pageSize":100,"totalItems":0,"totalPages":0,"items":[]},"cancelledCount":0}""", + NorthwindApi.OverridePriority); + + var cli = await CliRunner.RunAsync(CliArgs, ApiClient, McpToolCatalog.Config(WireMock.Url!), ct); + cli.ExitCode.Should().Be(0); + Creates().Should().Be(1); + + WireMock.Reset(); + NorthwindApi.StubAll(WireMock); + NorthwindApi.RawJson( + WireMock, "/api/leave/", "GET", + """{"leaves":{"pageNumber":1,"pageSize":100,"totalItems":0,"totalPages":0,"items":[]},"cancelledCount":0}""", + NorthwindApi.OverridePriority); + + var mcp = await new McpToolHost(ApiClient, McpToolCatalog.Config(WireMock.Url!)) + .Leave.CreateLeave( + start: "2026-04-01", end: "2026-04-01", type: "Annual Leave", note: "Family day", + timeZoneId: McpToolCatalog.TimeZone, ct: ct); + Creates().Should().Be(1); + + foreach (var (surface, json) in new[] { ("leave create --json", cli.Stdout), ("CreateLeave", mcp) }) + { + var root = JsonNode.Parse(json)!.AsObject(); + ((bool?)root["success"]).Should().BeTrue(surface); + root["leave"].Should().BeNull(surface); + ((string?)root["warning"]).Should().Contain("could not be identified", surface) + .And.Contain("Do not create it again"); + } + } + + private static void AssertCreatedEntry(string json) + { + var root = JsonNode.Parse(json)!.AsObject(); + + ((bool?)root["success"]).Should().BeTrue(); + ((string?)root["leaveId"]).Should().Be(NorthwindApi.LeaveId); + ((string?)root["leave"]!["id"]).Should().Be(NorthwindApi.LeaveId); + ((string?)root["leave"]!["note"]).Should().Be("Family day"); + } + + private void AssertOneCreateThenAReadBack() + { + var leaveCalls = WireMock.LogEntries + .Select(e => $"{e.RequestMessage!.Method.ToUpperInvariant()} {e.RequestMessage.AbsolutePath}") + .Where(call => call.EndsWith("/api/leave/", StringComparison.Ordinal)) + .ToList(); + + leaveCalls.Should().StartWith(["POST /api/leave/", "GET /api/leave/"]); + leaveCalls.Should().NotContain("PUT /api/leave/"); + leaveCalls.Count(call => call == "POST /api/leave/").Should().Be(1); + } + + private int Creates() => + WireMock.LogEntries.Count(e => + e.RequestMessage!.AbsolutePath == "/api/leave/" + && string.Equals(e.RequestMessage.Method, "POST", StringComparison.OrdinalIgnoreCase)); +} diff --git a/tests/SSW.TimePro.Cli.Integration/Mcp/McpCliParityTable.cs b/tests/SSW.TimePro.Cli.Integration/Mcp/McpCliParityTable.cs index 80020db..d1a585f 100644 --- a/tests/SSW.TimePro.Cli.Integration/Mcp/McpCliParityTable.cs +++ b/tests/SSW.TimePro.Cli.Integration/Mcp/McpCliParityTable.cs @@ -265,6 +265,34 @@ public static class McpCliParityTable Note = "Separate projections over one API call; the CLI derives the same range from --date." }, + // Appended rather than filed with the other leave rows: the Parity goldens are named by + // row index, so inserting above renames every snapshot below. + new("CreateLeave", "leave create") + { + CliArgs = ["leave", "create", "--start", "2026-04-01", "--end", "2026-04-01", + "--type", "Annual Leave", "--note", "Family day", + "--approved-by", NorthwindApi.EmpEmail, "--cc", NorthwindApi.EmpEmail, + "--timezone", McpToolCatalog.TimeZone, "--yes", "--json"], + InvokeTool = (h, ct) => h.Leave.CreateLeave( + start: "2026-04-01", end: "2026-04-01", type: "Annual Leave", note: "Family day", + approvedBy: NorthwindApi.EmpEmail, cc: NorthwindApi.EmpEmail, + timeZoneId: McpToolCatalog.TimeZone, ct: ct), + ExpectParity = false, + PermittedDifferences = + [ + "$.warning", + "$.leave.cancellationReason", "$.leave.endDateWithoutOffset", + "$.leave.startDateWithoutOffset", "$.leave.timeLessOverride" + ], + Note = "LeaveCreateService now owns the read-back; the CLI omits the nulls MCP writes.", + ExpectedRequests = + [ + new("POST", "/api/leave/") { BodyContains = ["\"Note\":\"Family day\""] }, + new("GET", "/api/leave/") + ], + ForbiddenRequests = [new("PUT", "/api/leave/")] + }, + // ───────── Not unified yet: declared so a later slice flips the flag ───────── new("GetTimesheets", "ts get") { Note = "MCP reshapes the row; CLI returns the API shape." }, diff --git a/tests/SSW.TimePro.Cli.Integration/Mcp/McpToolCatalog.cs b/tests/SSW.TimePro.Cli.Integration/Mcp/McpToolCatalog.cs index 56b32d4..d13afa1 100644 --- a/tests/SSW.TimePro.Cli.Integration/Mcp/McpToolCatalog.cs +++ b/tests/SSW.TimePro.Cli.Integration/Mcp/McpToolCatalog.cs @@ -159,6 +159,18 @@ private static IReadOnlyList BuildPopulated() => HasApiErrorCase = true }, + // No PrimaryRoute: the generated empty/apiError variants would collide with the dryRun + // case's goldens, which already cover a failing create. + new("CreateLeave", "created", (h, ct) => h.Leave.CreateLeave( + start: "2026-04-01", + end: "2026-04-01", + type: "Annual Leave", + note: "Family day", + approvedBy: NorthwindApi.EmpEmail, + cc: NorthwindApi.EmpEmail, + timeZoneId: TimeZone, + ct: ct)), + new("UpdateLeave", "dryRun", (h, ct) => h.Leave.UpdateLeave( id: NorthwindApi.LeaveId, note: "Family day (updated)", diff --git a/tests/SSW.TimePro.Cli.Integration/Mcp/McpToolHost.cs b/tests/SSW.TimePro.Cli.Integration/Mcp/McpToolHost.cs index 8ca1f71..c14c0b8 100644 --- a/tests/SSW.TimePro.Cli.Integration/Mcp/McpToolHost.cs +++ b/tests/SSW.TimePro.Cli.Integration/Mcp/McpToolHost.cs @@ -22,7 +22,7 @@ public McpToolHost(ITimeProApiClient api, IConfigService config) Leave = new LeaveMcpTools( api, config, - new LeaveCreateService(api), + new LeaveCreateService(api, new LeaveLookup(api)), new LeaveUpdateService(api, new LeaveLookup(api)), new LeaveListService(api)); Accounting = new AccountingMcpTools(api, config, new LeaveBalanceImportService(api)); diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Leave/CreateCommandTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Leave/CreateCommandTests.cs index d58f590..dccc5a7 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Leave/CreateCommandTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Leave/CreateCommandTests.cs @@ -1,3 +1,4 @@ +using System.Text.Json; using FluentAssertions; using Microsoft.Extensions.DependencyInjection; using NSubstitute; @@ -307,6 +308,96 @@ public async Task Create_WhenHalfDayEndTimeIsNotOnTheHourOrHalfHour_ReturnsError api.ShouldNotHaveReceived(nameof(ITimeProApiClient.CreateLeaveAsync)); } + [Fact] + public async Task Create_WithJson_ReturnsTheCreatedEntry() + { + var api = Substitute.For(); + api.GetEmployeeSettingsAsync(Arg.Any()) + .Returns(new EmployeeSettings { TimezoneId = "UTC" }); + api.GetLeaveAsync("UPCOMING", Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new LeaveListResponse + { + Leaves = new PaginatedList + { + PageNumber = 1, + PageSize = 100, + TotalItems = 1, + TotalPages = 1, + Items = + [ + new LeaveEntry + { + Id = "0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", + StartDate = "2026-03-30T00:00:00+00:00", + EndDate = "2026-03-30T23:59:00+00:00", + Note = "Annual leave", + AllDay = true, + LeaveStatus = 1, + LeaveType = new LeaveTypeInfo { Id = 1, Name = "Annual Leave", IsActive = true } + } + ] + } + }); + + var (exitCode, stdout) = await RunAsync(api, [ + "create", + "--start", "2026-03-30", + "--end", "2026-03-30", + "--type", "1", + "--note", "Annual leave", + "--json" + ]); + + exitCode.Should().Be(0); + using var doc = JsonDocument.Parse(stdout); + doc.RootElement.GetProperty("success").GetBoolean().Should().BeTrue(); + doc.RootElement.GetProperty("leaveId").GetString().Should().Be("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa"); + doc.RootElement.GetProperty("leave").GetProperty("statusName").GetString().Should().Be("Pending"); + } + + [Fact] + public async Task Create_WithDryRun_WritesNothingAndReturnsTheProposedRequest() + { + var api = Substitute.For(); + api.GetEmployeeSettingsAsync(Arg.Any()) + .Returns(new EmployeeSettings { TimezoneId = "UTC" }); + + var (exitCode, stdout) = await RunAsync(api, [ + "create", + "--start", "2026-03-30", + "--end", "2026-03-30", + "--type", "1", + "--note", "Annual leave", + "--dry-run", + "--json" + ]); + + exitCode.Should().Be(0); + using var doc = JsonDocument.Parse(stdout); + doc.RootElement.GetProperty("dryRun").GetBoolean().Should().BeTrue(); + doc.RootElement.GetProperty("request").GetProperty("startDate").GetString() + .Should().Be("2026-03-30T00:00:00.0000000+00:00"); + api.ShouldNotHaveReceived(nameof(ITimeProApiClient.CreateLeaveAsync)); + api.ShouldNotHaveReceived(nameof(ITimeProApiClient.GetLeaveAsync)); + } + + private static async Task<(int ExitCode, string Stdout)> RunAsync(ITimeProApiClient api, string[] args) + { + var app = CreateApp(api); + var original = Console.Out; + var writer = new StringWriter(); + try + { + Console.SetOut(writer); + var exitCode = await app.RunAsync(args, TestContext.Current.CancellationToken); + return (exitCode, writer.ToString().Trim()); + } + finally + { + Console.SetOut(original); + } + } + private static CommandApp CreateApp(ITimeProApiClient api) { var tenantProvider = Substitute.For(); @@ -321,6 +412,7 @@ private static CommandApp CreateApp(ITimeProApiClient api) var services = new ServiceCollection(); services.AddSingleton(api); services.AddSingleton(tenantProvider); + services.AddSingleton(); services.AddSingleton(); var app = new CommandApp(new TypeRegistrar(services)); diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveCreateServiceTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveCreateServiceTests.cs new file mode 100644 index 0000000..1f808da --- /dev/null +++ b/tests/SSW.TimePro.Cli.Tests/Features/Leave/LeaveCreateServiceTests.cs @@ -0,0 +1,300 @@ +using FluentAssertions; +using NSubstitute; +using SSW.TimePro.Cli.Features.Leave; +using SSW.TimePro.Cli.Infrastructure.ApiClient; +using SSW.TimePro.Cli.Shared.Models; +using Xunit; + +namespace SSW.TimePro.Cli.Tests.Features.Leave; + +public class LeaveCreateServiceTests +{ + private const string EmpId = "BOB"; + + [Fact] + public async Task Apply_WhenExactlyOneEntryMatches_ReturnsTheCreatedEntry() + { + var api = Substitute.For(); + StubList(api, LeaveListService.Upcoming, Entry("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa")); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service), + TestContext.Current.CancellationToken); + + result.Success.Should().BeTrue(); + result.LeaveId.Should().Be("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa"); + result.Leave!.Note.Should().Be("Family day"); + result.Warning.Should().BeNull(); + await api.Received(1).CreateLeaveAsync(Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task Apply_WhenOnlyThePastFilterMatches_ReturnsTheCreatedEntry() + { + var api = Substitute.For(); + StubList(api, LeaveListService.Upcoming); + StubList(api, LeaveListService.Past, Entry("1a3c8f8b-2222-4444-8888-bbbbbbbbbbbb")); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service), + TestContext.Current.CancellationToken); + + result.LeaveId.Should().Be("1a3c8f8b-2222-4444-8888-bbbbbbbbbbbb"); + } + + [Fact] + public async Task Apply_WhenNothingMatches_WarnsWithoutCreatingAgain() + { + var api = Substitute.For(); + StubList(api, LeaveListService.Upcoming, Entry("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", note: "Other leave")); + StubList(api, LeaveListService.Past); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service), + TestContext.Current.CancellationToken); + + result.Success.Should().BeTrue(); + result.LeaveId.Should().BeNull(); + result.Leave.Should().BeNull(); + result.Warning.Should().Contain("no entry matches").And.Contain("tp leave list"); + await api.Received(1).CreateLeaveAsync(Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task Apply_WhenSeveralEntriesMatch_WarnsWithoutGuessing() + { + var api = Substitute.For(); + StubList( + api, + LeaveListService.Upcoming, + Entry("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa"), + Entry("1a3c8f8b-2222-4444-8888-bbbbbbbbbbbb")); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service), + TestContext.Current.CancellationToken); + + result.Success.Should().BeTrue(); + result.Leave.Should().BeNull(); + result.Warning.Should().Contain("2 entries match"); + await api.Received(1).CreateLeaveAsync(Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task Apply_WhenReadBackFails_StillReportsTheCreateAsSucceeded() + { + var api = Substitute.For(); + api.GetLeaveAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns>(_ => throw new ApiException(500, "boom")); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service), + TestContext.Current.CancellationToken); + + result.Success.Should().BeTrue(); + result.Warning.Should().NotBeNull(); + } + + [Fact] + public async Task Apply_WhenTheReadBackCannotConnect_StillReportsTheCreateAsSucceeded() + { + var api = Substitute.For(); + api.GetLeaveAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns>(_ => throw new TimeProConnectionException( + "Connection refused", "tenant.json", "test", "https://timepro.example")); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service), + TestContext.Current.CancellationToken); + + result.Success.Should().BeTrue(); + result.Leave.Should().BeNull(); + result.Warning.Should().Contain("Do not create it again"); + await api.Received(1).CreateLeaveAsync(Arg.Any(), Arg.Any()); + } + + [Fact] + public async Task Apply_WhenTheReadBackThrowsAnythingElse_StillReportsTheCreateAsSucceeded() + { + var api = Substitute.For(); + api.GetLeaveAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns>(_ => throw new InvalidOperationException("unexpected")); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service), + TestContext.Current.CancellationToken); + + result.Success.Should().BeTrue(); + result.Warning.Should().Contain("Do not create it again"); + } + + [Fact] + public async Task Apply_PrefersTheOffsetFreeDates_WhenTheOffsetDatesFallOnAnotherDay() + { + var api = Substitute.For(); + var entry = Entry("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa"); + entry.StartDateLocal = "2026-04-01T00:00:00"; + entry.EndDateLocal = "2026-04-01T23:59:00"; + entry.StartDate = "2026-03-31T13:00:00+11:00"; + entry.EndDate = "2026-04-02T12:59:00+11:00"; + StubList(api, LeaveListService.Upcoming, entry); + StubList(api, LeaveListService.Past); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service), + TestContext.Current.CancellationToken); + + result.LeaveId.Should().Be("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa"); + } + + [Fact] + public async Task Apply_WhenTwoPartialDayRequestsShareTheDay_ResolvesTheSubmittedSlot() + { + var api = Substitute.For(); + StubList( + api, + LeaveListService.Upcoming, + PartialDayEntry("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", "09:00", "13:00"), + PartialDayEntry("1a3c8f8b-2222-4444-8888-bbbbbbbbbbbb", "13:00", "17:00")); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service, halfDay: true, startTime: "13:00", endTime: "17:00"), + TestContext.Current.CancellationToken); + + result.LeaveId.Should().Be("1a3c8f8b-2222-4444-8888-bbbbbbbbbbbb"); + } + + [Fact] + public async Task Apply_WhenTwoPartialDayRequestsShareTheSlot_WarnsWithoutGuessing() + { + var api = Substitute.For(); + StubList( + api, + LeaveListService.Upcoming, + PartialDayEntry("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", "13:00", "17:00"), + PartialDayEntry("1a3c8f8b-2222-4444-8888-bbbbbbbbbbbb", "13:00", "17:00")); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service, halfDay: true, startTime: "13:00", endTime: "17:00"), + TestContext.Current.CancellationToken); + + result.Leave.Should().BeNull(); + result.Warning.Should().Contain("2 entries match"); + } + + [Fact] + public async Task Apply_WhenOnlyAPartialDayEntryExists_DoesNotClaimItForAnAllDayRequest() + { + var api = Substitute.For(); + StubList(api, LeaveListService.Upcoming, PartialDayEntry("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", "09:00", "13:00")); + StubList(api, LeaveListService.Past); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service), + TestContext.Current.CancellationToken); + + result.Leave.Should().BeNull(); + result.Warning.Should().Contain("no entry matches"); + } + + [Fact] + public async Task Apply_WhenTheServerEchoesAnotherOffset_StillMatchesTheSlot() + { + var api = Substitute.For(); + var entry = PartialDayEntry("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa", "13:00", "17:00"); + entry.StartDateLocal = null; + entry.EndDateLocal = null; + entry.StartDate = "2026-04-01T23:00:00+10:00"; + entry.EndDate = "2026-04-02T03:00:00+10:00"; + StubList(api, LeaveListService.Upcoming, entry); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + var result = await service.ApplyAsync( + await PrepareAsync(service, halfDay: true, startTime: "13:00", endTime: "17:00"), + TestContext.Current.CancellationToken); + + result.LeaveId.Should().Be("0f2b7e7a-1111-4444-8888-aaaaaaaaaaaa"); + } + + [Fact] + public async Task Prepare_DoesNotWrite() + { + var api = Substitute.For(); + var service = new LeaveCreateService(api, new LeaveLookup(api)); + + await PrepareAsync(service); + + await api.DidNotReceive().CreateLeaveAsync(Arg.Any(), Arg.Any()); + await api.DidNotReceive().GetLeaveAsync( + Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + + private static Task PrepareAsync( + LeaveCreateService service, + bool halfDay = false, + string? startTime = null, + string? endTime = null) => + service.PrepareAsync( + EmpId, + new LeaveCreateOptions( + Start: "2026-04-01", + End: "2026-04-01", + Type: "1", + Note: "Family day", + HalfDay: halfDay, + StartTime: startTime, + EndTime: endTime, + TimeZoneId: "UTC"), + TestContext.Current.CancellationToken); + + private static LeaveEntry PartialDayEntry(string id, string startTime, string endTime) + { + var entry = Entry(id); + entry.AllDay = false; + entry.StartDateLocal = $"2026-04-01T{startTime}:00"; + entry.EndDateLocal = $"2026-04-01T{endTime}:00"; + entry.StartDate = $"2026-04-01T{startTime}:00+00:00"; + entry.EndDate = $"2026-04-01T{endTime}:00+00:00"; + return entry; + } + + private static LeaveEntry Entry(string id, string note = "Family day") => new() + { + Id = id, + StartDate = "2026-04-01T00:00:00+00:00", + EndDate = "2026-04-01T23:59:00+00:00", + Note = note, + RequestedEmpId = EmpId, + AllDay = true, + LeaveStatus = 1, + LeaveType = new LeaveTypeInfo { Id = 1, Name = "Annual Leave", IsActive = true } + }; + + private static void StubList(ITimeProApiClient api, string filter, params LeaveEntry[] items) => + api.GetLeaveAsync(filter, Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()) + .Returns(new LeaveListResponse + { + Leaves = new PaginatedList + { + PageNumber = 1, + PageSize = 100, + TotalItems = items.Length, + TotalPages = 1, + Items = [.. items] + } + }); +} diff --git a/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs index 431c76f..616546b 100644 --- a/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs +++ b/tests/SSW.TimePro.Cli.Tests/Features/Mcp/LeaveMcpToolsTests.cs @@ -335,7 +335,7 @@ public async Task GetLeaveBalanceStatus_WhenNothingImported_ReportsNotImported() private static LeaveMcpTools CreateTools(ITimeProApiClient api, IConfigService config) => new(api, config, - new LeaveCreateService(api), + new LeaveCreateService(api, new LeaveLookup(api)), new LeaveUpdateService(api, new LeaveLookup(api)), new LeaveListService(api));