From f63a1a0ee311071b015b9fd5d41602930e3e0057 Mon Sep 17 00:00:00 2001 From: hokiepokedad2 <38219945+hokiepokedad2@users.noreply.github.com> Date: Mon, 24 Aug 2026 16:25:56 -0400 Subject: [PATCH 1/5] feat(poracleng): move the human, location and place proxy onto /api/v2 Nine operations now prefer PoracleNG's /api/v2/humans surface and keep their v1 path as a fallback, so a self-hoster on 5.1.0 sends byte-identical requests and loses nothing. There is no version floor. Three of them are more than a rename. The notification language stops being a direct write to humans.language and goes through POST .../language, which also reloads PoracleNG's in-memory state -- so a language change now takes effect on the next alert instead of at the next restart. Saving a place with a label you already have becomes a real 409 instead of a refusal buried inside a 200, and a label too long for the column is refused before it is sent rather than coming back as "database error". PUT /locations/{label} is new capability with no v1 equivalent: moving a place an alarm points at was previously impossible. IHumanService.UpdateAsync, IHumanRepository.UpdateAsync and CreateAsync are gone. Nothing in PoracleWeb.NET writes a humans row directly any more. IPoracleHumanProxy.CheckLocationAsync went with them -- it had no callers. --- .../ServiceCollectionExtensions.cs | 2 +- .../NotificationLanguageController.cs | 10 +- .../Repositories/IHumanRepository.cs | 2 - .../Services/IHumanService.cs | 5 +- .../Services/IPoracleHumanProxy.cs | 40 +- .../HumanRepository.cs | 24 +- .../HumanService.cs | 12 +- .../PoracleHumanProxy.cs | 253 ++++++++++- .../Controllers/LocationControllerTests.cs | 23 +- .../Services/HumanServiceTests.cs | 14 +- .../Services/PoracleHumanProxyRefusalTests.cs | 31 +- .../Services/PoracleHumanProxyTests.cs | 75 ++- .../Services/PoracleHumanProxyV2Tests.cs | 427 ++++++++++++++++++ 13 files changed, 793 insertions(+), 125 deletions(-) create mode 100644 Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyV2Tests.cs diff --git a/Applications/Pgan.PoracleWebNet.Api/Configuration/ServiceCollectionExtensions.cs b/Applications/Pgan.PoracleWebNet.Api/Configuration/ServiceCollectionExtensions.cs index 99be717a..4f761b6b 100644 --- a/Applications/Pgan.PoracleWebNet.Api/Configuration/ServiceCollectionExtensions.cs +++ b/Applications/Pgan.PoracleWebNet.Api/Configuration/ServiceCollectionExtensions.cs @@ -173,7 +173,7 @@ public static IServiceCollection AddPoracleServices(this IServiceCollection serv // Register HttpClient for PoracleNG summary schedule proxy (quest summary delivery) services.AddHttpClient(); - // Register HttpClient for PoracleNG's v2 mute store (quiet periods). The only /api/v2 caller. + // Register HttpClient for PoracleNG's v2 mute store (quiet periods). services.AddHttpClient(); // Register HttpClient for Discord notification service diff --git a/Applications/Pgan.PoracleWebNet.Api/Controllers/NotificationLanguageController.cs b/Applications/Pgan.PoracleWebNet.Api/Controllers/NotificationLanguageController.cs index 8786b205..d081fc9e 100644 --- a/Applications/Pgan.PoracleWebNet.Api/Controllers/NotificationLanguageController.cs +++ b/Applications/Pgan.PoracleWebNet.Api/Controllers/NotificationLanguageController.cs @@ -49,12 +49,16 @@ public async Task UpdateLanguage([FromBody] LanguageUpdateRequest return this.BadRequest(new { error = "Language must be 255 characters or fewer." }); } - human.Language = request.Language; - await this._humanService.UpdateAsync(human); + await this._humanService.SetLanguageAsync(this.UserId, request.Language); + + // Read back rather than echo. PoracleNG lowercases and trims what it stores -- "pt-BR" becomes + // "pt-br" -- and an endpoint that reported the request instead of the row would leave the SPA + // holding a value the server does not have. Verified on 5.2.1 against both API versions. + var stored = await this._humanService.GetByIdAsync(this.UserId); return this.Ok(new { - language = human.Language + language = stored?.Language ?? request.Language }); } diff --git a/Core/Pgan.PoracleWebNet.Core.Abstractions/Repositories/IHumanRepository.cs b/Core/Pgan.PoracleWebNet.Core.Abstractions/Repositories/IHumanRepository.cs index d7d61d0f..3f067da5 100644 --- a/Core/Pgan.PoracleWebNet.Core.Abstractions/Repositories/IHumanRepository.cs +++ b/Core/Pgan.PoracleWebNet.Core.Abstractions/Repositories/IHumanRepository.cs @@ -9,8 +9,6 @@ public interface IHumanRepository /// The webhook humans, id and name. Used to resolve a delegated webhook named by name. public Task> GetWebhooksAsync(); public Task GetByIdAsync(string id); - public Task CreateAsync(Human human); - public Task UpdateAsync(Human human); public Task> GetByIdsAsync(IEnumerable ids); public Task ExistsAsync(string id); public Task DeleteUserAsync(string userId); diff --git a/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IHumanService.cs b/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IHumanService.cs index 9655366e..83fdb5b8 100644 --- a/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IHumanService.cs +++ b/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IHumanService.cs @@ -10,7 +10,10 @@ public interface IHumanService public Task> GetWebhooksAsync(); public Task GetByIdAsync(string id); public Task CreateAsync(Human human); - public Task UpdateAsync(Human human); + + /// Sets the language PoracleNG writes this user's alerts in. + public Task SetLanguageAsync(string userId, string language); + public Task ExistsAsync(string id); public Task DeleteAllAlarmsByUserAsync(string userId); public Task DeleteUserAsync(string userId); diff --git a/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleHumanProxy.cs b/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleHumanProxy.cs index 2d71a9e6..fb3227df 100644 --- a/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleHumanProxy.cs +++ b/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleHumanProxy.cs @@ -11,7 +11,7 @@ public interface IPoracleHumanProxy { /// /// Fetches a single human record. - /// Maps to GET /api/humans/one/{userId} + /// GET /api/v2/humans/{userId}, or GET /api/humans/one/{userId} on a PoracleNG without v2. /// public Task GetHumanAsync(string userId); @@ -23,16 +23,31 @@ public interface IPoracleHumanProxy /// /// Enables alerts for a user. - /// Maps to POST /api/humans/{userId}/start + /// POST /api/v2/humans/{userId}/enable, or POST /api/humans/{userId}/start without v2. /// public Task StartAsync(string userId); /// /// Disables alerts for a user. - /// Maps to POST /api/humans/{userId}/stop + /// POST /api/v2/humans/{userId}/disable, or POST /api/humans/{userId}/stop without v2. /// public Task StopAsync(string userId); + /// + /// Sets the language PoracleNG writes this user's alerts in. + /// + /// + /// POST /api/v2/humans/{userId}/language, falling back to POST /api/humans/{userId}/language, which + /// exists on both supported releases. Replaces a direct write to humans.language: the handler + /// also reloads PoracleNG's in-memory state, which the direct write never did, so a language change + /// now takes effect on the next alert instead of at the next restart. + /// + /// Both handlers lowercase and trim what they store, so pt-BR comes back as pt-br. + /// Verified on 5.2.1 against v1 and v2 alike. + /// + /// + public Task SetLanguageAsync(string userId, string language); + /// /// Admin-disables or re-enables a user. /// Maps to POST /api/humans/{userId}/adminDisabled @@ -41,7 +56,7 @@ public interface IPoracleHumanProxy /// /// Sets user location. - /// Maps to POST /api/humans/{userId}/setLocation/{lat}/{lon} + /// POST /api/v2/humans/{userId}/location with a {lat,lon} body, or v1's coordinates-in-the-path form. /// public Task SetLocationAsync(string userId, double lat, double lon); @@ -89,12 +104,6 @@ public interface IPoracleHumanProxy /// public Task DeleteProfileAsync(string userId, int profileNo); - /// - /// Checks if a location is inside any geofence. - /// Maps to GET /api/humans/{userId}/checkLocation/{lat}/{lon} - /// - public Task CheckLocationAsync(string userId, double lat, double lon); - /// /// Copies all tracking rules from one profile to another. /// Maps to POST /api/profiles/{userId}/copy/{fromProfileNo}/{toProfileNo} @@ -115,6 +124,17 @@ public interface IPoracleHumanProxy /// Null on success, or PoracleNG's reason for refusing this label. public Task AddPlaceAsync(string userId, SavedPlace place); + /// + /// Moves a saved place, keeping its label so every alarm pointing at it follows. + /// + /// + /// PUT /api/v2/humans/{id}/locations/{label}. There is no v1 equivalent, which is why moving a place + /// an alarm referenced was impossible before: the delete answers 409 while anything still points at + /// it, so the only route was to repoint every alarm, delete, re-add and repoint back. + /// + /// False when this PoracleNG has no such route; the caller should say so rather than retry. + public Task UpdatePlaceAsync(string userId, string label, double latitude, double longitude); + /// /// Deletes a saved place. /// Maps to POST /api/humans/{id}/locations/{label}/delete diff --git a/Core/Pgan.PoracleWebNet.Core.Repositories/HumanRepository.cs b/Core/Pgan.PoracleWebNet.Core.Repositories/HumanRepository.cs index 684ad250..df113fe1 100644 --- a/Core/Pgan.PoracleWebNet.Core.Repositories/HumanRepository.cs +++ b/Core/Pgan.PoracleWebNet.Core.Repositories/HumanRepository.cs @@ -57,25 +57,11 @@ public async Task> GetByIdsAsync(IEnumerable ids) return results.Select(e => e.ToModel()); } - public async Task CreateAsync(Human human) - { - var entity = human.ToEntity(); - EnsureNotNullDefaults(entity); - this._context.Humans.Add(entity); - await this._context.SaveChangesAsync(); - return entity.ToModel(); - } - - public async Task UpdateAsync(Human human) - { - var entity = await this._context.Humans.FirstOrDefaultAsync(h => h.Id == human.Id) - ?? throw new InvalidOperationException($"Human with id {human.Id} not found."); - - human.ApplyTo(entity); - EnsureNotNullDefaults(entity); - await this._context.SaveChangesAsync(); - return entity.ToModel(); - } + // CreateAsync and UpdateAsync lived here and are gone. Create had no callers at all; the only + // caller of Update was the notification-language endpoint, which now goes through + // IPoracleHumanProxy.SetLanguageAsync. Nothing in PoracleWeb writes a humans row directly any more, + // which also means nothing can stamp a stale copy of the whole record over PoracleNG's -- the shape + // of #517. public async Task ExistsAsync(string id) => await this._context.Humans.AnyAsync(h => h.Id == id); diff --git a/Core/Pgan.PoracleWebNet.Core.Services/HumanService.cs b/Core/Pgan.PoracleWebNet.Core.Services/HumanService.cs index 699bcf67..bdfce317 100644 --- a/Core/Pgan.PoracleWebNet.Core.Services/HumanService.cs +++ b/Core/Pgan.PoracleWebNet.Core.Services/HumanService.cs @@ -7,9 +7,9 @@ namespace Pgan.PoracleWebNet.Core.Services; /// -/// Proxy-first service for human operations. Admin bulk operations (GetAll, DeleteUser, UpdateAsync) -/// remain direct DB via IHumanRepository because PoracleNG has no admin-list, admin-delete, or -/// generic update endpoints yet. See: docs/poracleng-enhancement-requests.md +/// Proxy-first service for human operations. Admin bulk operations (GetAll, GetWebhooks, DeleteUser) +/// remain direct DB via IHumanRepository because PoracleNG has no admin-list or admin-delete endpoint +/// on either API version. See: docs/poracleng-enhancement-requests.md /// public class HumanService( IHumanRepository repository, @@ -48,9 +48,9 @@ public async Task CreateAsync(Human human) return created ?? human; } - // TODO: Migrate once PoracleNG adds a generic human update endpoint. - // See: docs/poracleng-enhancement-requests.md - public async Task UpdateAsync(Human human) => await this._repository.UpdateAsync(human); + /// + public async Task SetLanguageAsync(string userId, string language) => + await this._humanProxy.SetLanguageAsync(userId, language); public async Task ExistsAsync(string id) { diff --git a/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs b/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs index 9ee24c95..79471ecb 100644 --- a/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs +++ b/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs @@ -1,20 +1,40 @@ using Pgan.PoracleWebNet.Core.Models; +using System.Globalization; using System.Net; using System.Text; using System.Text.Json; +using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; using Pgan.PoracleWebNet.Core.Abstractions.Services; namespace Pgan.PoracleWebNet.Core.Services; -public class PoracleHumanProxy(HttpClient httpClient, IConfiguration configuration) : IPoracleHumanProxy +public partial class PoracleHumanProxy( + HttpClient httpClient, + IConfiguration configuration, + IPoracleServerProfileService serverProfile, + IMemoryCache cache, + ILogger logger) : IPoracleHumanProxy { /// What to say when PoracleNG refused and explained nothing usable. private const string Unexplained = "Poracle rejected the request."; + /// The release that first carries /api/v2/humans. + private static readonly Version FirstWithV2 = new(5, 2, 0); + + /// + /// How long a route is remembered as missing. Matches the server profile's own cache, so an upgrade + /// is picked up on the same clock as every other version-gated thing. + /// + private static readonly TimeSpan V2AbsentFor = TimeSpan.FromMinutes(5); + private readonly HttpClient _httpClient = httpClient; private readonly string _apiAddress = configuration["Poracle:ApiAddress"] ?? string.Empty; private readonly string _apiSecret = configuration["Poracle:ApiSecret"] ?? string.Empty; + private readonly IPoracleServerProfileService _serverProfile = serverProfile; + private readonly IMemoryCache _cache = cache; + private readonly ILogger _logger = logger; /// /// URL-encodes a userId for safe path construction. Webhook IDs are full URLs @@ -22,6 +42,75 @@ public class PoracleHumanProxy(HttpClient httpClient, IConfiguration configurati /// private static string Encode(string userId) => Uri.EscapeDataString(userId); + /// A latitude or longitude in a form PoracleNG parses whatever the server's culture is. + /// + /// The v1 location paths interpolate the doubles straight into the URL. On a machine whose current + /// culture uses a comma for the decimal separator that produced /setLocation/51,5/-0,12, which + /// is four path segments rather than two. The v2 body does not have the problem -- JsonSerializer + /// is invariant -- but the v1 fallback is still there and still has to be right. + /// + private static string Coord(double value) => value.ToString(CultureInfo.InvariantCulture); + + /// + /// Whether PoracleNG is believed to carry /api/v2, from the version it reports. + /// + /// + /// A belief, not a fact: a fork can carry the routes while reporting an older number, or the reverse. + /// therefore also handles the route being absent at request time, so being + /// wrong here costs one extra round-trip rather than the operation. + /// + private async Task ServerCarriesV2Async() + { + var profile = await this._serverProfile.GetAsync(); + return profile.Reachable && profile.ParsedVersion is { } version && version >= FirstWithV2; + } + + /// + /// Sends one request to /api/v2, or answers null when this server has no such route so the + /// caller can use its v1 path for the same request instead of failing it. + /// + /// + /// The absence is remembered per route, not per surface. One shared flag would let a single missing + /// route drop every other call back to v1 -- and PUT /locations/{label} has no v1 path at all, + /// so for that one "fall back" means "vanish". + /// + private async Task<(HttpResponseMessage Response, string Payload)?> TryV2Async( + HttpMethod method, string route, string path, string? body = null) + { + var absentKey = $"poracle:v2-humans-absent:{route}"; + if (this._cache.TryGetValue(absentKey, out _)) + { + return null; + } + + if (!await this.ServerCarriesV2Async()) + { + return null; + } + + var reply = await this.SendReadAsync(method, path, body); + + // gin answers a route it does not have with the plaintext "404 page not found"; the v2 surface + // answers a missing human or place with problem+json at the same status. Verified against 5.1.0 + // and 5.2.1 -- the content type is the only thing separating them. + if (reply.Response.StatusCode == HttpStatusCode.NotFound + && !PoracleProblemDetails.IsProblemJson(reply.Payload)) + { + this._cache.Set(absentKey, true, V2AbsentFor); + LogV2RouteAbsent(this._logger, route); + return null; + } + + return reply; + } + + private async Task<(HttpResponseMessage Response, string Payload)> SendReadAsync( + HttpMethod method, string path, string? body = null) + { + var response = await this.SendAsync(method, path, body); + return (response, await response.Content.ReadAsStringAsync()); + } + /// /// Turns a refusal from PoracleNG into the answer it deserves, and lets everything else throw. /// @@ -72,6 +161,13 @@ private static async Task EnsureAcceptedAsync(HttpResponseMessage response) case HttpStatusCode.UnprocessableEntity: throw new PoracleRequestRefusedException(PoracleProblemDetails.Describe(payload, Unexplained)); + case HttpStatusCode.Conflict: + // v2 answers 409 where v1 buried the same refusal in a 200 body -- a duplicate saved-place + // label is the live case. Without this it fell through to EnsureSuccessStatusCode and the + // global handler turned "you already have one called that" into a 500. + throw new PoracleRequestRefusedException( + PoracleProblemDetails.Describe(payload, Unexplained), (int)HttpStatusCode.Conflict); + default: response.EnsureSuccessStatusCode(); return; @@ -92,13 +188,17 @@ payload is not null public async Task GetHumanAsync(string userId) { - var response = await this.SendAsync(HttpMethod.Get, $"/api/humans/one/{Encode(userId)}"); + // Both surfaces answer the same wrapper and the same columns -- verified field by field against a + // live 5.2.1 -- so nothing downstream can tell which one answered. + var (response, json) = + await this.TryV2Async(HttpMethod.Get, "get", $"/api/v2/humans/{Encode(userId)}") + ?? await this.SendReadAsync(HttpMethod.Get, $"/api/humans/one/{Encode(userId)}"); + if (!response.IsSuccessStatusCode) { return null; } - var json = await response.Content.ReadAsStringAsync(); using var doc = JsonDocument.Parse(json); // PoracleNG wraps the response: { "human": { ... }, "status": "ok" } @@ -118,13 +218,36 @@ public async Task CreateHumanAsync(JsonElement body) public async Task StartAsync(string userId) { - var response = await this.SendAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/start"); + var (response, _) = + await this.TryV2Async(HttpMethod.Post, "enable", $"/api/v2/humans/{Encode(userId)}/enable") + ?? await this.SendReadAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/start"); + await EnsureAcceptedAsync(response); } public async Task StopAsync(string userId) { - var response = await this.SendAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/stop"); + var (response, _) = + await this.TryV2Async(HttpMethod.Post, "disable", $"/api/v2/humans/{Encode(userId)}/disable") + ?? await this.SendReadAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/stop"); + + await EnsureAcceptedAsync(response); + } + + /// + public async Task SetLanguageAsync(string userId, string language) + { + var v2Body = JsonSerializer.Serialize(new + { + language + }); + + var (response, _) = + await this.TryV2Async( + HttpMethod.Post, "language", $"/api/v2/humans/{Encode(userId)}/language", v2Body) + ?? await this.SendReadAsync( + HttpMethod.Post, $"/api/humans/{Encode(userId)}/language", v2Body); + await EnsureAcceptedAsync(response); } @@ -137,13 +260,38 @@ public async Task AdminDisabledAsync(string userId, bool disabled) { state = disabled }); - var response = await this.SendAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/adminDisabled", body); + + // v2 renamed the key as well as the route: its adminDisableBody is `Disabled *bool`, so sending + // v1's `state` to it is a 422 rather than a no-op. + var v2Body = JsonSerializer.Serialize(new + { + disabled + }); + + var (response, _) = + await this.TryV2Async( + HttpMethod.Post, "admin-disable", $"/api/v2/humans/{Encode(userId)}/admin-disable", v2Body) + ?? await this.SendReadAsync( + HttpMethod.Post, $"/api/humans/{Encode(userId)}/adminDisabled", body); + await EnsureAcceptedAsync(response); } public async Task SetLocationAsync(string userId, double lat, double lon) { - var response = await this.SendAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/setLocation/{lat}/{lon}"); + var v2Body = JsonSerializer.Serialize(new + { + lat, + lon + }); + + var (response, _) = + await this.TryV2Async( + HttpMethod.Post, "location", $"/api/v2/humans/{Encode(userId)}/location", v2Body) + ?? await this.SendReadAsync( + HttpMethod.Post, + $"/api/humans/{Encode(userId)}/setLocation/{Coord(lat)}/{Coord(lon)}"); + await EnsureAcceptedAsync(response); } @@ -161,7 +309,17 @@ public async Task SetAreasAsync(string userId, string[] areas) public async Task SwitchProfileAsync(string userId, int profileNo) { - var response = await this.SendAsync(HttpMethod.Post, $"/api/humans/{Encode(userId)}/switchProfile/{profileNo}"); + var v2Body = JsonSerializer.Serialize(new + { + profile_no = profileNo + }); + + var (response, _) = + await this.TryV2Async( + HttpMethod.Post, "profile", $"/api/v2/humans/{Encode(userId)}/profile", v2Body) + ?? await this.SendReadAsync( + HttpMethod.Post, $"/api/humans/{Encode(userId)}/switchProfile/{profileNo}"); + await EnsureAcceptedAsync(response); } @@ -199,26 +357,15 @@ public async Task CopyProfileAsync(string userId, int fromProfileNo, int toProfi await EnsureAcceptedAsync(response); } - public async Task CheckLocationAsync(string userId, double lat, double lon) - { - var response = await this.SendAsync(HttpMethod.Get, $"/api/humans/{Encode(userId)}/checkLocation/{lat}/{lon}"); - if (!response.IsSuccessStatusCode) - { - return null; - } - - var json = await response.Content.ReadAsStringAsync(); - using var doc = JsonDocument.Parse(json); - return doc.RootElement.Clone(); - } - public async Task GetPlacesAsync(string userId) { - var response = await this.SendAsync(HttpMethod.Get, $"/api/humans/{Encode(userId)}/locations"); + var (response, json) = + await this.TryV2Async(HttpMethod.Get, "locations", $"/api/v2/humans/{Encode(userId)}/locations") + ?? await this.SendReadAsync(HttpMethod.Get, $"/api/humans/{Encode(userId)}/locations"); + await EnsureAcceptedAsync(response); - var json = await response.Content.ReadAsStringAsync(); using var doc = JsonDocument.Parse(json); // PoracleNG wraps this one as {"locations": {...}, "status": "ok"} -- reading the root as the @@ -258,6 +405,39 @@ public async Task GetPlacesAsync(string userId) public async Task AddPlaceAsync(string userId, SavedPlace place) { + // Ours, not PoracleNG's. v1 reported the overflow as "Data too long for column 'label'" inside a + // 200 and v2 answers 500 {"detail":"database error"} -- both verified live, and neither is + // something to show a person who typed a long name. humans_locations.label is varchar(64). + if (place.Label is { Length: > 64 }) + { + return "That name is too long. Use 64 characters or fewer."; + } + + // v2 takes lat/lon on the way in and still answers latitude/longitude on the way out. The + // asymmetry is upstream's, not a typo here. + var v2Body = JsonSerializer.Serialize(new + { + label = place.Label, + lat = place.Latitude, + lon = place.Longitude, + }); + + var v2 = await this.TryV2Async( + HttpMethod.Post, "locations-add", $"/api/v2/humans/{Encode(userId)}/locations", v2Body); + + if (v2 is { } reply) + { + // v2 turns the duplicate label into a real 409 instead of burying it in a 200. Returned as + // the same string v1 produced so the controller and the SPA see one behaviour. + if (reply.Response.StatusCode == HttpStatusCode.Conflict) + { + return PoracleProblemDetails.Describe(reply.Payload, Unexplained); + } + + await EnsureAcceptedAsync(reply.Response); + return null; + } + var body = JsonSerializer.Serialize(new { label = place.Label, @@ -292,6 +472,27 @@ public async Task GetPlacesAsync(string userId) return null; } + /// + public async Task UpdatePlaceAsync(string userId, string label, double latitude, double longitude) + { + var body = JsonSerializer.Serialize(new + { + lat = latitude, + lon = longitude, + }); + + var v2 = await this.TryV2Async( + HttpMethod.Put, "locations-update", $"/api/v2/humans/{Encode(userId)}/locations/{Encode(label)}", body); + + if (v2 is not { } reply) + { + return false; + } + + await EnsureAcceptedAsync(reply.Response); + return true; + } + public async Task DeletePlaceAsync(string userId, string label) { var response = await this.SendAsync( @@ -329,4 +530,10 @@ private async Task SendAsync(HttpMethod method, string path return await this._httpClient.SendAsync(request); } + + [LoggerMessage( + EventId = 6301, + Level = LogLevel.Debug, + Message = "PoracleNG has no /api/v2 route for {Route}; using the v1 path for it.")] + private static partial void LogV2RouteAbsent(ILogger logger, string route); } diff --git a/Tests/Pgan.PoracleWebNet.Tests/Controllers/LocationControllerTests.cs b/Tests/Pgan.PoracleWebNet.Tests/Controllers/LocationControllerTests.cs index 49698100..1620d79e 100644 --- a/Tests/Pgan.PoracleWebNet.Tests/Controllers/LocationControllerTests.cs +++ b/Tests/Pgan.PoracleWebNet.Tests/Controllers/LocationControllerTests.cs @@ -91,12 +91,31 @@ public async Task UpdateLanguageSetsLanguage() { var human = new Human { Id = "123456789", Language = "en" }; this._humanService.Setup(s => s.GetByIdAsync("123456789")).ReturnsAsync(human); - this._humanService.Setup(s => s.UpdateAsync(human)).ReturnsAsync(human); var result = await this.LanguageSut().UpdateLanguage(new NotificationLanguageController.LanguageUpdateRequest { Language = "de" }); Assert.IsType(result); - Assert.Equal("de", human.Language); + this._humanService.Verify(s => s.SetLanguageAsync("123456789", "de"), Times.Once); + } + + [Fact] + public async Task UpdateLanguageAnswersWhatWasStoredRatherThanWhatWasSent() + { + // PoracleNG lowercases and trims: "pt-BR" is stored as "pt-br", on v1 and v2 alike. Echoing the + // request would leave the SPA holding a value the server does not have, and its picker compares + // the two strings. + var human = new Human { Id = "123456789", Language = "en" }; + this._humanService.Setup(s => s.GetByIdAsync("123456789")).ReturnsAsync(human); + this._humanService + .Setup(s => s.SetLanguageAsync("123456789", "pt-BR")) + .Callback(() => human.Language = "pt-br") + .Returns(Task.CompletedTask); + + var result = await this.LanguageSut() + .UpdateLanguage(new NotificationLanguageController.LanguageUpdateRequest { Language = "pt-BR" }); + + var ok = Assert.IsType(result); + Assert.Equal("pt-br", ok.Value?.GetType().GetProperty("language")?.GetValue(ok.Value)); } [Fact] diff --git a/Tests/Pgan.PoracleWebNet.Tests/Services/HumanServiceTests.cs b/Tests/Pgan.PoracleWebNet.Tests/Services/HumanServiceTests.cs index f3581fce..4be276d4 100644 --- a/Tests/Pgan.PoracleWebNet.Tests/Services/HumanServiceTests.cs +++ b/Tests/Pgan.PoracleWebNet.Tests/Services/HumanServiceTests.cs @@ -106,14 +106,16 @@ public async Task CreateAsyncSendsEnabledAndAdminDisableAsBooleans( } [Fact] - public async Task UpdateAsyncDelegatesToRepository() + public async Task SetLanguageAsyncGoesThroughTheProxyAndNotTheDatabase() { - // UpdateAsync still uses direct DB for general updates - var human = new Human { Id = "u1", Name = "Updated" }; - this._repository.Setup(r => r.UpdateAsync(human)).ReturnsAsync(human); + // The repository has no write left to delegate to. Before this, changing the notification + // language read the whole human, mutated one field and wrote every column back -- which is how + // it managed to stamp last_checked as well (#517) -- and PoracleNG never learned about it until + // the next restart, because nothing reloaded its state. + await this._sut.SetLanguageAsync("u1", "de"); - await this._sut.UpdateAsync(human); - this._repository.Verify(r => r.UpdateAsync(human), Times.Once); + this._humanProxy.Verify(p => p.SetLanguageAsync("u1", "de"), Times.Once); + this._repository.VerifyNoOtherCalls(); } [Fact] diff --git a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyRefusalTests.cs b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyRefusalTests.cs index e53f97a9..2728717c 100644 --- a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyRefusalTests.cs +++ b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyRefusalTests.cs @@ -1,7 +1,10 @@ using System.Net; using System.Text; using System.Text.Json; +using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; +using Moq; using Pgan.PoracleWebNet.Core.Models; using Pgan.PoracleWebNet.Core.Services; @@ -32,7 +35,12 @@ private static PoracleHumanProxy CreateSut(MockHandler handler) => ["Poracle:ApiAddress"] = "http://localhost:3030", ["Poracle:ApiSecret"] = "test-secret", }) - .Build()); + .Build(), + // No version, so no /api/v2: every refusal here is v1's, which is what a 5.1.0 self-hoster + // still gets. + PoracleHumanProxyTests.ServerProfile(null), + new MemoryCache(new MemoryCacheOptions()), + Mock.Of>()); private static PoracleHumanProxy Refusing(HttpStatusCode status, string body) => CreateSut(new MockHandler(status, body)); @@ -301,12 +309,20 @@ public async Task AServerFaultIsStillAServerFault() } [Fact] - public async Task AConflictOutsideDeletePlaceIsStillAServerFault() + public async Task AConflictIsPassedOnAsAConflict() { - // Only the delete-place route gives 409 a meaning. Nothing else should start reading one. - var sut = Refusing(HttpStatusCode.Conflict, "{}"); - - await Assert.ThrowsAsync(() => sut.CreateHumanAsync(Body("{}"))); + // This asserted the opposite until /api/v2 arrived, on the premise that only delete-place gives + // 409 a meaning. v2 ended that: saving a place whose label you already have is a real 409 there, + // where v1 buried the same refusal inside a 200. A 409 is the caller being told no, so flattening + // it into HttpRequestException put "an unexpected error occurred" in front of the user and a + // fault in the log -- the exact shape of #539. It keeps its own status rather than becoming a 400. + var sut = Refusing(HttpStatusCode.Conflict, """{"title":"Conflict","status":409,"detail":"location label already exists"}"""); + + var refused = await Assert.ThrowsAsync( + () => sut.CreateHumanAsync(Body("{}"))); + + Assert.Equal(409, refused.StatusCode); + Assert.Contains("location label already exists", refused.Message, StringComparison.Ordinal); } [Fact] @@ -345,12 +361,11 @@ public async Task AddPlaceStillReportsTheRefusalHiddenInsideA200() [Fact] public async Task ReadsThatAnswerNullOnFailureStillAnswerNull() { - // GetHumanAsync and CheckLocationAsync are read paths whose callers branch on null. Turning their + // GetHumanAsync and GetAreasAsync are read paths whose callers branch on null. Turning their // failures into throws would break HumanService, ProfileService and TestAlertService at once. var sut = Refusing(HttpStatusCode.BadRequest, """{"message":"invalid latitude","status":"error"}"""); Assert.Null(await sut.GetHumanAsync("user1")); - Assert.Null(await sut.CheckLocationAsync("user1", 999, 999)); Assert.Null(await sut.GetAreasAsync("user1")); } diff --git a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyTests.cs b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyTests.cs index 9d1a9636..3b2d2c65 100644 --- a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyTests.cs +++ b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyTests.cs @@ -1,7 +1,12 @@ using System.Net; using System.Text; using System.Text.Json; +using Microsoft.Extensions.Caching.Memory; using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; +using Moq; +using Pgan.PoracleWebNet.Core.Abstractions.Services; +using Pgan.PoracleWebNet.Core.Models; using Pgan.PoracleWebNet.Core.Services; namespace Pgan.PoracleWebNet.Tests.Services; @@ -27,10 +32,31 @@ public class PoracleHumanProxyTests }) .Build(); - private static PoracleHumanProxy CreateSut(MockHttpMessageHandler handler, IConfiguration? config = null) + /// + /// A proxy pointed at a PoracleNG with no /api/v2, so every test here exercises the v1 path it has + /// always exercised. The v2 paths get their own file; what this one guards is that a self-hoster on + /// 5.1.0 still gets byte-identical requests. + /// + private static PoracleHumanProxy CreateSut( + MockHttpMessageHandler handler, IConfiguration? config = null, string? version = null) { var client = new HttpClient(handler); - return new PoracleHumanProxy(client, config ?? CreateConfig()); + return new PoracleHumanProxy( + client, config ?? CreateConfig(), ServerProfile(version), + new MemoryCache(new MemoryCacheOptions()), Mock.Of>()); + } + + /// A server profile reporting the given version, or an unreachable one when null. + internal static IPoracleServerProfileService ServerProfile(string? version) + { + var profile = new Mock(); + profile + .Setup(p => p.GetAsync(It.IsAny())) + .ReturnsAsync(version is null + ? PoracleServerProfile.Unknown(DateTimeOffset.UtcNow) + : new PoracleServerProfile { Version = version, Reachable = true, CheckedAt = DateTimeOffset.UtcNow }); + + return profile.Object; } // ────────────────────────────────────────────────────────────── @@ -109,7 +135,9 @@ public async Task CreateHumanAsyncSendsPostWithBody() [Fact] public async Task CreateHumanAsyncThrowsOnNon2xx() { - var handler = new MockHttpMessageHandler(HttpStatusCode.Conflict, "{}"); + // 500 rather than 409: a conflict is now read as a refusal and reported as one. See + // PoracleHumanProxyRefusalTests.AConflictIsPassedOnAsAConflict. + var handler = new MockHttpMessageHandler(HttpStatusCode.InternalServerError, "{}"); var sut = CreateSut(handler); var body = JsonDocument.Parse("{}").RootElement; @@ -456,47 +484,6 @@ public async Task DeleteProfileAsyncThrowsOnNon2xx() await Assert.ThrowsAsync(() => sut.DeleteProfileAsync("user1", 1)); } - // ────────────────────────────────────────────────────────────── - // CheckLocationAsync - // ────────────────────────────────────────────────────────────── - - [Fact] - public async Task CheckLocationAsyncReturnsJsonOn200() - { - var responseBody = /*lang=json,strict*/ """{"areas":["downtown"]}"""; - var handler = new MockHttpMessageHandler(HttpStatusCode.OK, responseBody); - var sut = CreateSut(handler); - - var result = await sut.CheckLocationAsync("user1", 40.7128, -74.006); - - Assert.NotNull(result); - Assert.True(result.Value.TryGetProperty("areas", out _)); - } - - [Fact] - public async Task CheckLocationAsyncReturnsNullOnNon2xx() - { - var handler = new MockHttpMessageHandler(HttpStatusCode.NotFound, "{}"); - var sut = CreateSut(handler); - - var result = await sut.CheckLocationAsync("user1", 0, 0); - - Assert.Null(result); - } - - [Fact] - public async Task CheckLocationAsyncCallsCorrectUrl() - { - var handler = new MockHttpMessageHandler(HttpStatusCode.OK, "{}"); - var sut = CreateSut(handler); - - await sut.CheckLocationAsync("user1", 51.5, -0.12); - - Assert.NotNull(handler.LastRequest); - Assert.Equal(HttpMethod.Get, handler.LastRequest.Method); - Assert.Contains("/api/humans/user1/checkLocation/51.5/-0.12", handler.LastRequest.RequestUri?.ToString()); - } - // ────────────────────────────────────────────────────────────── // Auth header // ────────────────────────────────────────────────────────────── diff --git a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyV2Tests.cs b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyV2Tests.cs new file mode 100644 index 00000000..fd6a5424 --- /dev/null +++ b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyV2Tests.cs @@ -0,0 +1,427 @@ +using System.Globalization; +using System.Net; +using System.Text; +using System.Text.Json; +using Microsoft.Extensions.Caching.Memory; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; +using Moq; +using Pgan.PoracleWebNet.Core.Models; +using Pgan.PoracleWebNet.Core.Services; + +namespace Pgan.PoracleWebNet.Tests.Services; + +/// +/// The human, location and place operations on PoracleNG's /api/v2 surface, and every way they +/// must fall back to v1 instead. +/// +/// +/// +/// Every response here was taken from a live 5.2.1 and, for the fallbacks, a live 5.1.0: the +/// {"human":{...}} wrapper that both versions share, the 409 on a duplicate place label, the 404 +/// problem+json for a place that does not exist, and gin's plaintext 404 page not found for a +/// route the build has never had. +/// +/// +/// Half of these assert that a 5.1.0 self-hoster is unaffected. That is the point of keeping v1: there +/// is no version floor, so the fallback has to be exercised as hard as the new path. +/// +/// +public class PoracleHumanProxyV2Tests +{ + private const string ApiAddress = "http://localhost:3030"; + + /// What gin answers for a route this build does not carry. Plaintext, not problem+json. + private const string RouteMissing = "404 page not found"; + + [Fact] + public async Task GetHumanReadsTheV2RouteOn521() + { + var handler = ScriptedHandler.Ok("""{"human":{"id":"user1","language":"en"}}"""); + var sut = CreateSut(handler, "5.2.1"); + + var human = await sut.GetHumanAsync("user1"); + + var request = Assert.Single(handler.Requests); + Assert.Equal($"{ApiAddress}/api/v2/humans/user1", request.Url); + Assert.Equal("user1", human!.Value.GetProperty("id").GetString()); + } + + [Fact] + public async Task GetHumanUsesV1OnAServerWithoutV2() + { + var handler = ScriptedHandler.Ok("""{"human":{"id":"user1"},"status":"ok"}"""); + var sut = CreateSut(handler, "5.1.0"); + + await sut.GetHumanAsync("user1"); + + Assert.Equal($"{ApiAddress}/api/humans/one/user1", Assert.Single(handler.Requests).Url); + } + + [Fact] + public async Task AVersionThatClaimsV2ButHasNoRouteFallsBackToV1() + { + // A fork can carry a version number without the routes. The plaintext 404 is the only signal, + // and answering it with a failure rather than a retry would take the operation away entirely. + var handler = new ScriptedHandler( + new Reply(HttpStatusCode.NotFound, RouteMissing, "text/plain"), + new Reply(HttpStatusCode.OK, """{"human":{"id":"user1"},"status":"ok"}""")); + var sut = CreateSut(handler, "5.2.1"); + + var human = await sut.GetHumanAsync("user1"); + + Assert.Equal(2, handler.Requests.Count); + Assert.Equal($"{ApiAddress}/api/v2/humans/user1", handler.Requests[0].Url); + Assert.Equal($"{ApiAddress}/api/humans/one/user1", handler.Requests[1].Url); + Assert.Equal("user1", human!.Value.GetProperty("id").GetString()); + } + + [Fact] + public async Task AMissingHumanIsNotMistakenForAMissingRoute() + { + // Both are 404. Only the content type separates them, and reading the problem+json one as a + // missing route would drop the whole surface to v1 for five minutes over a stale user id. + var handler = ScriptedHandler.Problem( + HttpStatusCode.NotFound, """{"title":"Not Found","status":404,"detail":"human not found"}"""); + var sut = CreateSut(handler, "5.2.1"); + + Assert.Null(await sut.GetHumanAsync("nobody")); + Assert.Single(handler.Requests); + } + + [Fact] + public async Task OneMissingRouteDoesNotDropTheOthersToV1() + { + // The absence is remembered per route. A single shared flag would let a 404 on one route send + // every other call back to v1 -- and PUT /locations/{label} has no v1 path at all, so for that + // one "fall back" means the feature disappears. + var cache = new MemoryCache(new MemoryCacheOptions()); + var handler = new ScriptedHandler( + new Reply(HttpStatusCode.NotFound, RouteMissing, "text/plain"), + new Reply(HttpStatusCode.OK, """{"status":"ok"}""")); + + await CreateSut(handler, "5.2.1", cache).StartAsync("user1"); + await CreateSut(handler, "5.2.1", cache).StopAsync("user1"); + + Assert.Equal($"{ApiAddress}/api/v2/humans/user1/enable", handler.Requests[0].Url); + Assert.Equal($"{ApiAddress}/api/humans/user1/start", handler.Requests[1].Url); + Assert.Equal($"{ApiAddress}/api/v2/humans/user1/disable", handler.Requests[2].Url); + } + + [Fact] + public async Task AMissingRouteIsNotProbedAgainWhileItIsRemembered() + { + var cache = new MemoryCache(new MemoryCacheOptions()); + var handler = new ScriptedHandler( + new Reply(HttpStatusCode.NotFound, RouteMissing, "text/plain"), + new Reply(HttpStatusCode.OK, """{"status":"ok"}""")); + + await CreateSut(handler, "5.2.1", cache).StartAsync("user1"); + await CreateSut(handler, "5.2.1", cache).StartAsync("user1"); + + Assert.Equal(3, handler.Requests.Count); + Assert.Equal($"{ApiAddress}/api/humans/user1/start", handler.Requests[2].Url); + } + + [Fact] + public async Task SetLanguageSendsTheSameBodyToWhicheverRouteAnswers() + { + var v2 = ScriptedHandler.Ok("""{"status":"ok"}"""); + await CreateSut(v2, "5.2.1").SetLanguageAsync("user1", "de"); + + var sent = Assert.Single(v2.Requests); + Assert.Equal(HttpMethod.Post, sent.Method); + Assert.Equal($"{ApiAddress}/api/v2/humans/user1/language", sent.Url); + Assert.Equal("de", JsonDocument.Parse(sent.Body!).RootElement.GetProperty("language").GetString()); + + var v1 = ScriptedHandler.Ok("""{"language":"de","status":"ok"}"""); + await CreateSut(v1, "5.1.0").SetLanguageAsync("user1", "de"); + + // Identical body on both, which is the whole reason this one needed no translation. + var legacy = Assert.Single(v1.Requests); + Assert.Equal($"{ApiAddress}/api/humans/user1/language", legacy.Url); + Assert.Equal("de", JsonDocument.Parse(legacy.Body!).RootElement.GetProperty("language").GetString()); + } + + [Fact] + public async Task ARefusedLanguageIsReportedAsARefusal() + { + // Where general.available_languages is configured, an unlisted code is refused. It has to reach + // the user as their input being wrong, not as the server having broken. See #539. + var handler = ScriptedHandler.Problem( + HttpStatusCode.UnprocessableEntity, + """{"title":"Unprocessable Entity","status":422,"detail":"language is required"}"""); + + var refused = await Assert.ThrowsAsync( + () => CreateSut(handler, "5.2.1").SetLanguageAsync("user1", "")); + + Assert.Contains("language is required", refused.Message, StringComparison.Ordinal); + } + + [Fact] + public async Task AdminDisableRenamesTheKeyAsWellAsTheRoute() + { + // v2's body is `disabled`; v1's is `state`. Sending v1's key to v2 is a 422, and sending v2's + // key to v1 is "state is required (true/false)" -- the defect the v1 body already exists to fix. + var v2 = ScriptedHandler.Ok("""{"status":"ok"}"""); + await CreateSut(v2, "5.2.1").AdminDisabledAsync("user1", true); + + var sent = Assert.Single(v2.Requests); + Assert.Equal($"{ApiAddress}/api/v2/humans/user1/admin-disable", sent.Url); + var v2Body = JsonDocument.Parse(sent.Body!).RootElement; + Assert.True(v2Body.GetProperty("disabled").GetBoolean()); + Assert.False(v2Body.TryGetProperty("state", out _)); + + var v1 = ScriptedHandler.Ok("""{"status":"ok"}"""); + await CreateSut(v1, "5.1.0").AdminDisabledAsync("user1", true); + + var legacy = Assert.Single(v1.Requests); + Assert.Equal($"{ApiAddress}/api/humans/user1/adminDisabled", legacy.Url); + Assert.True(JsonDocument.Parse(legacy.Body!).RootElement.GetProperty("state").GetBoolean()); + } + + [Fact] + public async Task SetLocationMovesTheCoordinatesIntoTheBody() + { + var handler = ScriptedHandler.Ok("""{"status":"ok"}"""); + await CreateSut(handler, "5.2.1").SetLocationAsync("user1", 51.5, -0.12); + + var sent = Assert.Single(handler.Requests); + Assert.Equal($"{ApiAddress}/api/v2/humans/user1/location", sent.Url); + var body = JsonDocument.Parse(sent.Body!).RootElement; + Assert.Equal(51.5, body.GetProperty("lat").GetDouble()); + Assert.Equal(-0.12, body.GetProperty("lon").GetDouble()); + } + + [Fact] + public async Task TheV1LocationPathIsInvariantWhateverTheServerCultureIs() + { + // The v1 path interpolates the doubles. Under a culture with a comma decimal separator that + // produced /setLocation/51,5/-0,12 -- four path segments where PoracleNG routes two. + var previous = CultureInfo.CurrentCulture; + try + { + CultureInfo.CurrentCulture = new CultureInfo("de-DE"); + + var handler = ScriptedHandler.Ok("""{"status":"ok"}"""); + await CreateSut(handler, "5.1.0").SetLocationAsync("user1", 51.5, -0.12); + + Assert.Equal( + $"{ApiAddress}/api/humans/user1/setLocation/51.5/-0.12", + Assert.Single(handler.Requests).Url); + } + finally + { + CultureInfo.CurrentCulture = previous; + } + } + + [Fact] + public async Task SwitchProfileMovesTheNumberIntoTheBody() + { + var handler = ScriptedHandler.Ok("""{"status":"ok"}"""); + await CreateSut(handler, "5.2.1").SwitchProfileAsync("user1", 3); + + var sent = Assert.Single(handler.Requests); + Assert.Equal($"{ApiAddress}/api/v2/humans/user1/profile", sent.Url); + Assert.Equal(3, JsonDocument.Parse(sent.Body!).RootElement.GetProperty("profile_no").GetInt32()); + } + + [Fact] + public async Task AddPlaceSendsLatLonWhereTheListAnswersLatitudeLongitude() + { + // The asymmetry is upstream's, not a typo: the request takes lat/lon, the read answers the + // long names. + var handler = ScriptedHandler.Ok("""{"status":"ok"}"""); + var sut = CreateSut(handler, "5.2.1"); + + var refusal = await sut.AddPlaceAsync( + "user1", new SavedPlace { Label = "home", Latitude = 1.5, Longitude = 2.5 }); + + Assert.Null(refusal); + var sent = Assert.Single(handler.Requests); + Assert.Equal(HttpMethod.Post, sent.Method); + Assert.Equal($"{ApiAddress}/api/v2/humans/user1/locations", sent.Url); + var body = JsonDocument.Parse(sent.Body!).RootElement; + Assert.Equal("home", body.GetProperty("label").GetString()); + Assert.Equal(1.5, body.GetProperty("lat").GetDouble()); + Assert.Equal(2.5, body.GetProperty("lon").GetDouble()); + Assert.False(body.TryGetProperty("latitude", out _)); + } + + [Fact] + public async Task ADuplicateLabelIsReportedNotThrown() + { + // v2 answers a real 409 where v1 buried the same refusal in a 200 body. Both have to reach the + // caller as the same string, because the controller turns it into the message shown by the field. + var handler = ScriptedHandler.Problem( + HttpStatusCode.Conflict, + """{"title":"Conflict","status":409,"detail":"location label already exists"}"""); + var sut = CreateSut(handler, "5.2.1"); + + var refusal = await sut.AddPlaceAsync("user1", new SavedPlace { Label = "home" }); + + Assert.Equal("location label already exists", refusal); + } + + [Fact] + public async Task ALabelTooLongForTheColumnIsRefusedBeforeItIsSent() + { + // humans_locations.label is varchar(64). v1 reported the overflow as "Data too long for column + // 'label'" inside a 200 and v2 answers 500 {"detail":"database error"} -- both verified live, + // and neither is a sentence to show someone who typed a long name. + var handler = ScriptedHandler.Ok("""{"status":"ok"}"""); + var sut = CreateSut(handler, "5.2.1"); + + var refusal = await sut.AddPlaceAsync("user1", new SavedPlace { Label = new string('x', 65) }); + + Assert.NotNull(refusal); + Assert.Empty(handler.Requests); + } + + [Fact] + public async Task ALabelOfExactlySixtyFourStillSaves() + { + var handler = ScriptedHandler.Ok("""{"status":"ok"}"""); + var sut = CreateSut(handler, "5.2.1"); + + Assert.Null(await sut.AddPlaceAsync("user1", new SavedPlace { Label = new string('x', 64) })); + Assert.Single(handler.Requests); + } + + [Fact] + public async Task AddPlaceStillReadsTheV1ResultsArrayOnAnOlderServer() + { + var handler = ScriptedHandler.Ok("""{"results":[{"error":"label already used"}],"status":"ok"}"""); + var sut = CreateSut(handler, "5.1.0"); + + Assert.Equal("label already used", await sut.AddPlaceAsync("user1", new SavedPlace { Label = "home" })); + Assert.Equal($"{ApiAddress}/api/humans/user1/locations/add", Assert.Single(handler.Requests).Url); + } + + [Fact] + public async Task UpdatePlacePutsTheNewCoordinates() + { + var handler = ScriptedHandler.Ok("""{"status":"ok"}"""); + var sut = CreateSut(handler, "5.2.1"); + + Assert.True(await sut.UpdatePlaceAsync("user1", "home", 9.5, 8.5)); + + var sent = Assert.Single(handler.Requests); + Assert.Equal(HttpMethod.Put, sent.Method); + Assert.Equal($"{ApiAddress}/api/v2/humans/user1/locations/home", sent.Url); + var body = JsonDocument.Parse(sent.Body!).RootElement; + Assert.Equal(9.5, body.GetProperty("lat").GetDouble()); + Assert.Equal(8.5, body.GetProperty("lon").GetDouble()); + } + + [Fact] + public async Task UpdatePlaceAnswersFalseRatherThanFailingOnAServerWithoutTheRoute() + { + // There is no v1 equivalent, so the honest answer is "this server cannot", not an exception the + // SPA would show as a failed save. + var handler = ScriptedHandler.Ok("""{"status":"ok"}"""); + var sut = CreateSut(handler, "5.1.0"); + + Assert.False(await sut.UpdatePlaceAsync("user1", "home", 9.5, 8.5)); + Assert.Empty(handler.Requests); + } + + [Fact] + public async Task UpdatingAPlaceThatIsNotThereIsARefusalNotAMissingRoute() + { + var handler = ScriptedHandler.Problem( + HttpStatusCode.NotFound, """{"title":"Not Found","status":404,"detail":"location not found"}"""); + var sut = CreateSut(handler, "5.2.1"); + + var refused = await Assert.ThrowsAsync( + () => sut.UpdatePlaceAsync("user1", "nope", 1, 2)); + + Assert.Equal(404, refused.StatusCode); + Assert.Contains("location not found", refused.Message, StringComparison.Ordinal); + } + + [Fact] + public async Task GetPlacesReadsTheSameWrapperFromBothSurfaces() + { + const string Body = + """{"locations":{"default":{"latitude":1,"longitude":2},"named":[{"label":"home","latitude":3,"longitude":4}]}}"""; + + foreach (var (version, url) in new[] + { + ("5.2.1", $"{ApiAddress}/api/v2/humans/user1/locations"), + ("5.1.0", $"{ApiAddress}/api/humans/user1/locations"), + }) + { + var handler = ScriptedHandler.Ok(Body); + var places = await CreateSut(handler, version).GetPlacesAsync("user1"); + + Assert.Equal(url, Assert.Single(handler.Requests).Url); + Assert.Equal("home", Assert.Single(places.Named).Label); + Assert.Equal(1, places.Default!.Latitude); + } + } + + [Fact] + public async Task AnUnreachableServerStaysOnV1() + { + // Unknown version means unknown routes. Guessing v2 costs an extra round-trip on every call + // while PoracleNG is down, and it is down often enough for that to matter. + var handler = ScriptedHandler.Ok("""{"human":{"id":"user1"},"status":"ok"}"""); + var sut = CreateSut(handler, version: null); + + await sut.GetHumanAsync("user1"); + + Assert.Equal($"{ApiAddress}/api/humans/one/user1", Assert.Single(handler.Requests).Url); + } + + private static PoracleHumanProxy CreateSut(ScriptedHandler handler, string? version, IMemoryCache? cache = null) + { + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Poracle:ApiAddress"] = ApiAddress, + ["Poracle:ApiSecret"] = "test-secret", + }) + .Build(); + + return new PoracleHumanProxy( + new HttpClient(handler), + config, + PoracleHumanProxyTests.ServerProfile(version), + cache ?? new MemoryCache(new MemoryCacheOptions()), + Mock.Of>()); + } + + private sealed record Reply(HttpStatusCode Status, string Body, string ContentType = "application/json"); + + private sealed record Sent(HttpMethod Method, string Url, string? Body); + + private sealed class ScriptedHandler(params Reply[] replies) : HttpMessageHandler + { + private int _next; + + public List Requests { get; } = []; + + public static ScriptedHandler Ok(string body) => new(new Reply(HttpStatusCode.OK, body)); + + public static ScriptedHandler Problem(HttpStatusCode status, string body) => + new(new Reply(status, body, "application/problem+json")); + + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + var body = request.Content is null + ? null + : await request.Content.ReadAsStringAsync(cancellationToken); + + this.Requests.Add(new Sent(request.Method, request.RequestUri!.ToString(), body)); + + var reply = replies[Math.Min(this._next++, replies.Length - 1)]; + return new HttpResponseMessage(reply.Status) + { + Content = new StringContent(reply.Body, Encoding.UTF8, reply.ContentType), + }; + } + } +} From d23e257fd319a598f21407657dad269227549dae Mon Sep 17 00:00:00 2001 From: hokiepokedad2 <38219945+hokiepokedad2@users.noreply.github.com> Date: Mon, 24 Aug 2026 16:31:31 -0400 Subject: [PATCH 2/5] fix(webhooks): stop a degraded admin-roles read reading as "administers nothing" GetAdminRolesAsync answered null for every non-2xx without throwing, so a 500 or a 503 from PoracleNG reached UserRoleResolver as a confident "no delegated webhooks" -- Resolved:true, cached for the full minute. That is the #656/#667 failure on the one source their fix did not cover, and it is the fifth way this path has broken. A 404 now means PoracleNG has no such human; anything else non-2xx throws, and the resolver reports the answer as unresolved. The read moved to IPoracleHumanProxy, where it belongs and where it picks up the /api/v2 route and the v1 fallback for free, and its id is now URL-encoded. Both isAdmin branches are deleted. Neither has ever fired: both API versions build the body from the same adminRolesResult, whose fields are channels, webhooks and users, and v2's schema is additionalProperties:false, so an isAdmin cannot appear even by accident. Admin status still comes from the configured ids and Poracle's own config. IPoracleApiProxy.GetAreasAsync goes with it -- zero callers, v1 path. --- .../Services/UserRoleResolver.cs | 47 +++++--------- .../Services/IPoracleApiProxy.cs | 2 - .../Services/IPoracleHumanProxy.cs | 20 ++++++ .../PoracleApiProxy.cs | 22 ------- .../PoracleHumanProxy.cs | 21 ++++++ .../Services/PoracleHumanProxyV2Tests.cs | 46 +++++++++++++ .../Services/UserRoleResolverTests.cs | 64 +++++++++++++++++-- 7 files changed, 162 insertions(+), 60 deletions(-) diff --git a/Applications/Pgan.PoracleWebNet.Api/Services/UserRoleResolver.cs b/Applications/Pgan.PoracleWebNet.Api/Services/UserRoleResolver.cs index ed32b6a8..20227a0f 100644 --- a/Applications/Pgan.PoracleWebNet.Api/Services/UserRoleResolver.cs +++ b/Applications/Pgan.PoracleWebNet.Api/Services/UserRoleResolver.cs @@ -46,6 +46,7 @@ public interface IUserRoleResolver /// public sealed partial class UserRoleResolver( IPoracleApiProxy poracleApiProxy, + IPoracleHumanProxy poracleHumanProxy, IWebhookDelegateService webhookDelegateService, IHumanService humanService, IOptions poracleSettings, @@ -57,6 +58,7 @@ public sealed partial class UserRoleResolver( private readonly IMemoryCache _cache = cache; private readonly ILogger _logger = logger; private readonly IPoracleApiProxy _poracleApiProxy = poracleApiProxy; + private readonly IPoracleHumanProxy _poracleHumanProxy = poracleHumanProxy; private readonly PoracleSettings _poracleSettings = poracleSettings.Value; private readonly IWebhookDelegateService _webhookDelegateService = webhookDelegateService; private readonly IHumanService _humanService = humanService; @@ -115,44 +117,34 @@ private async Task ResolveUncachedAsync(string userId) configReadable = false; } - // Call getAdministrationRoles once — resolves delegation including Discord guild roles + // Ask PoracleNG once for the delegated webhooks, Discord guild roles included. var managed = new HashSet(StringComparer.OrdinalIgnoreCase); - var isAdmin = false; try { - var rolesJson = await this._poracleApiProxy.GetAdminRolesAsync(userId); + var rolesJson = await this._poracleHumanProxy.GetAdminRolesAsync(userId); if (!string.IsNullOrEmpty(rolesJson)) { using var doc = JsonDocument.Parse(rolesJson); var root = doc.RootElement; - // Some versions return isAdmin at root; others wrap under admin.discord - if (root.TryGetProperty("isAdmin", out var isAdminProp) && isAdminProp.ValueKind == JsonValueKind.True) - { - isAdmin = true; - } - - // Parse admin.discord.webhooks — the authoritative delegate webhook list + // admin.discord.webhooks is the authoritative delegate webhook list. + // + // Two isAdmin branches used to sit here, one at the root and one under admin.discord. + // Neither has ever fired: both API versions build this body from the same + // adminRolesResult, whose only fields are channels, webhooks and users, and v2's schema + // is additionalProperties:false so an isAdmin could not appear even by accident. + // Admin status is resolved above, from the configured ids and Poracle's own config. if (root.TryGetProperty("admin", out var adminEl) && - adminEl.TryGetProperty("discord", out var discordEl)) + adminEl.TryGetProperty("discord", out var discordEl) && + discordEl.TryGetProperty("webhooks", out var webhooks) && + webhooks.ValueKind == JsonValueKind.Array) { - if (!isAdmin && - discordEl.TryGetProperty("isAdmin", out var discordAdmin) && - discordAdmin.ValueKind == JsonValueKind.True) + foreach (var wh in webhooks.EnumerateArray()) { - isAdmin = true; - } - - if (discordEl.TryGetProperty("webhooks", out var webhooks) && - webhooks.ValueKind == JsonValueKind.Array) - { - foreach (var wh in webhooks.EnumerateArray()) + if (wh.GetString() is { } id) { - if (wh.GetString() is { } id) - { - managed.Add(id); - } + managed.Add(id); } } } @@ -164,11 +156,6 @@ private async Task ResolveUncachedAsync(string userId) rolesReadable = false; } - if (isAdmin) - { - return new UserRoles(true, null); - } - // Also merge our own webhook delegate service layer try { diff --git a/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleApiProxy.cs b/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleApiProxy.cs index 90bea780..d54520f9 100644 --- a/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleApiProxy.cs +++ b/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleApiProxy.cs @@ -26,9 +26,7 @@ public interface IPoracleApiProxy /// to call. Verified: 5.1.0 has neither, 5.2.1 has both. /// Task GetShowcaseDisabledAsync(); - Task GetAreasAsync(string userId); Task GetTemplatesAsync(); - Task GetAdminRolesAsync(string userId); Task GetGruntsAsync(); /// diff --git a/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleHumanProxy.cs b/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleHumanProxy.cs index fb3227df..fb4579d8 100644 --- a/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleHumanProxy.cs +++ b/Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPoracleHumanProxy.cs @@ -60,6 +60,26 @@ public interface IPoracleHumanProxy /// public Task SetLocationAsync(string userId, double lat, double lon); + /// + /// The channels, webhooks and users this human may administer on Discord and Telegram. + /// + /// + /// GET /api/v2/humans/{userId}/admin-roles, or v1's getAdministrationRoles. Both compute the same + /// answer -- v2's handler calls the same delegated-administration logic -- and the only difference + /// on the wire is v1's extra "status":"ok". + /// + /// This sits on the delegated-webhook path, which has broken four separate times by one surface + /// disagreeing with another (#564, #601, #626, #786), so the distinction below is load-bearing: an + /// empty answer and an unknown answer must not look the same to the caller. + /// + /// + /// The roles JSON, or null when PoracleNG says it has no such human. + /// + /// Upstream is degraded. The caller must not read that as "this user administers nothing" -- doing + /// so denies a legitimate delegate for the whole cache TTL. + /// + public Task GetAdminRolesAsync(string userId); + /// /// Sets user area subscriptions. PoracleNG handles the dual-write to /// humans.area + profiles.area atomically. diff --git a/Core/Pgan.PoracleWebNet.Core.Services/PoracleApiProxy.cs b/Core/Pgan.PoracleWebNet.Core.Services/PoracleApiProxy.cs index 85d363a1..a8d9fbb4 100644 --- a/Core/Pgan.PoracleWebNet.Core.Services/PoracleApiProxy.cs +++ b/Core/Pgan.PoracleWebNet.Core.Services/PoracleApiProxy.cs @@ -252,14 +252,6 @@ public class PoracleApiProxy(HttpClient httpClient, IConfiguration configuration return null; } - public async Task GetAreasAsync(string userId) - { - var request = this.CreateRequest(HttpMethod.Get, $"{this._apiAddress}/api/humans/{userId}"); - var response = await this._httpClient.SendAsync(request); - response.EnsureSuccessStatusCode(); - return await response.Content.ReadAsStringAsync(); - } - public async Task GetTemplatesAsync() { var request = this.CreateRequest(HttpMethod.Get, $"{this._apiAddress}/api/config/templates"); @@ -268,20 +260,6 @@ public class PoracleApiProxy(HttpClient httpClient, IConfiguration configuration return await response.Content.ReadAsStringAsync(); } - public async Task GetAdminRolesAsync(string userId) - { - var request = this.CreateRequest(HttpMethod.Get, - $"{this._apiAddress}/api/humans/{userId}/getAdministrationRoles"); - var response = await this._httpClient.SendAsync(request); - - if (!response.IsSuccessStatusCode) - { - return null; - } - - return await response.Content.ReadAsStringAsync(); - } - /// /// Invasion grunt master data. The path is /api/masterdata/grunts — /api/config/grunts /// exists in neither supported backend, so this call could only ever 404 and throw. diff --git a/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs b/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs index 79471ecb..9e11f0dc 100644 --- a/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs +++ b/Core/Pgan.PoracleWebNet.Core.Services/PoracleHumanProxy.cs @@ -295,6 +295,27 @@ await this.TryV2Async( await EnsureAcceptedAsync(response); } + /// + public async Task GetAdminRolesAsync(string userId) + { + var (response, payload) = + await this.TryV2Async(HttpMethod.Get, "admin-roles", $"/api/v2/humans/{Encode(userId)}/admin-roles") + ?? await this.SendReadAsync( + HttpMethod.Get, $"/api/humans/{Encode(userId)}/getAdministrationRoles"); + + // A 404 is PoracleNG answering: this human has no roles because it has no such human. Anything + // else non-2xx is PoracleNG failing to answer, and returning null for it -- which is what this + // did for every status -- told UserRoleResolver "no delegated webhooks" confidently enough to + // cache for a minute. That is #656 and #667 on the one source their fix did not cover. + if (response.StatusCode == HttpStatusCode.NotFound) + { + return null; + } + + response.EnsureSuccessStatusCode(); + return payload; + } + public async Task SetAreasAsync(string userId, string[] areas) { var body = JsonSerializer.Serialize(areas); diff --git a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyV2Tests.cs b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyV2Tests.cs index fd6a5424..3751b6cf 100644 --- a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyV2Tests.cs +++ b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleHumanProxyV2Tests.cs @@ -362,6 +362,52 @@ public async Task GetPlacesReadsTheSameWrapperFromBothSurfaces() } } + [Fact] + public async Task AdminRolesReadsWhicheverRouteTheServerHas() + { + // The response is identical apart from v1's extra "status":"ok", because v2's handler calls the + // same delegated-administration logic. Verified live against both. + const string Body = """{"admin":{"discord":{"channels":[],"webhooks":["teamharmonyrares"],"users":false}}}"""; + + var v2 = ScriptedHandler.Ok(Body); + Assert.NotNull(await CreateSut(v2, "5.2.1").GetAdminRolesAsync("user1")); + Assert.Equal($"{ApiAddress}/api/v2/humans/user1/admin-roles", Assert.Single(v2.Requests).Url); + + var v1 = ScriptedHandler.Ok(Body); + Assert.NotNull(await CreateSut(v1, "5.1.0").GetAdminRolesAsync("user1")); + Assert.Equal( + $"{ApiAddress}/api/humans/user1/getAdministrationRoles", Assert.Single(v1.Requests).Url); + } + + [Fact] + public async Task AdminRolesTellsAMissingHumanApartFromAServerThatCouldNotAnswer() + { + // The whole point of this method's contract. Answering null for both -- which it did for every + // non-2xx -- told UserRoleResolver "administers nothing" confidently enough to cache, denying a + // legitimate delegate for the full minute after a blip. See #656 and #667. + var missing = ScriptedHandler.Problem( + HttpStatusCode.NotFound, """{"title":"Not Found","status":404,"detail":"human not found"}"""); + Assert.Null(await CreateSut(missing, "5.2.1").GetAdminRolesAsync("nobody")); + + var degraded = new ScriptedHandler(new Reply(HttpStatusCode.ServiceUnavailable, "{}")); + await Assert.ThrowsAsync( + () => CreateSut(degraded, "5.2.1").GetAdminRolesAsync("user1")); + } + + [Fact] + public async Task AWebhookIdIsEncodedIntoTheAdminRolesPath() + { + // A webhook human's id is a URL. The v1 call did not encode it, so its slashes became extra path + // segments and the request could only 404 -- which then read as "administers nothing". + var handler = ScriptedHandler.Ok("""{"admin":{"discord":{"channels":[],"webhooks":[],"users":false}}}"""); + + await CreateSut(handler, "5.2.1").GetAdminRolesAsync("https://discordapp.com/api/webhooks/1/tok"); + + Assert.Equal( + $"{ApiAddress}/api/v2/humans/https%3A%2F%2Fdiscordapp.com%2Fapi%2Fwebhooks%2F1%2Ftok/admin-roles", + Assert.Single(handler.Requests).Url); + } + [Fact] public async Task AnUnreachableServerStaysOnV1() { diff --git a/Tests/Pgan.PoracleWebNet.Tests/Services/UserRoleResolverTests.cs b/Tests/Pgan.PoracleWebNet.Tests/Services/UserRoleResolverTests.cs index d623f124..70d9f90e 100644 --- a/Tests/Pgan.PoracleWebNet.Tests/Services/UserRoleResolverTests.cs +++ b/Tests/Pgan.PoracleWebNet.Tests/Services/UserRoleResolverTests.cs @@ -18,6 +18,7 @@ namespace Pgan.PoracleWebNet.Tests.Services; public class UserRoleResolverTests { private readonly Mock _poracleApiProxy = new(); + private readonly Mock _poracleHumanProxy = new(); private readonly Mock _webhookDelegateService = new(); private readonly Mock _humanService = new(); @@ -25,6 +26,7 @@ public class UserRoleResolverTests private UserRoleResolver CreateSut(string adminIds = "") => new( this._poracleApiProxy.Object, + this._poracleHumanProxy.Object, this._webhookDelegateService.Object, this._humanService.Object, Options.Create(new PoracleSettings { AdminIds = adminIds }), @@ -35,7 +37,7 @@ public class UserRoleResolverTests public async Task AnUnreachablePoracleIsReportedAsUnresolvedRatherThanAsNotAnAdmin() { this._poracleApiProxy.Setup(p => p.GetConfigAsync()).ThrowsAsync(new HttpRequestException("down")); - this._poracleApiProxy.Setup(p => p.GetAdminRolesAsync(It.IsAny())).ThrowsAsync(new HttpRequestException("down")); + this._poracleHumanProxy.Setup(p => p.GetAdminRolesAsync(It.IsAny())).ThrowsAsync(new HttpRequestException("down")); this._webhookDelegateService.Setup(s => s.GetManagedWebhookIdsAsync(It.IsAny())).ReturnsAsync([]); var roles = await this.CreateSut().ResolveAsync("u1"); @@ -49,14 +51,64 @@ public async Task ADegradedAnswerIsNotCached() // Caching it would hold the user at the wrong privilege level for the full minute after a // momentary outage. this._poracleApiProxy.Setup(p => p.GetConfigAsync()).ThrowsAsync(new HttpRequestException("down")); - this._poracleApiProxy.Setup(p => p.GetAdminRolesAsync(It.IsAny())).ThrowsAsync(new HttpRequestException("down")); + this._poracleHumanProxy.Setup(p => p.GetAdminRolesAsync(It.IsAny())).ThrowsAsync(new HttpRequestException("down")); this._webhookDelegateService.Setup(s => s.GetManagedWebhookIdsAsync(It.IsAny())).ReturnsAsync([]); var sut = this.CreateSut(); await sut.ResolveAsync("u1"); await sut.ResolveAsync("u1"); - this._poracleApiProxy.Verify(p => p.GetAdminRolesAsync("u1"), Times.Exactly(2)); + this._poracleHumanProxy.Verify(p => p.GetAdminRolesAsync("u1"), Times.Exactly(2)); + } + + [Fact] + public async Task AnAdminRolesReadThatCouldNotBeMadeIsUnresolved() + { + // The hole the proxy change closes. GetAdminRolesAsync used to answer null for every non-2xx, + // so a 500 or a 503 arrived here as "administers nothing" with Resolved:true -- and got cached + // for the full minute, which is exactly what #656 and #667 were about on the other two sources. + this._poracleApiProxy.Setup(p => p.GetConfigAsync()).ReturnsAsync((PoracleConfig?)null!); + this._poracleHumanProxy + .Setup(p => p.GetAdminRolesAsync(It.IsAny())) + .ThrowsAsync(new HttpRequestException("503")); + this._webhookDelegateService.Setup(s => s.GetManagedWebhookIdsAsync(It.IsAny())).ReturnsAsync([]); + + var roles = await this.CreateSut().ResolveAsync("u1"); + + Assert.False(roles.Resolved); + } + + [Fact] + public async Task AHumanPoracleHasNeverHeardOfIsAResolvedNo() + { + // The legitimate case beside it: a 404 is PoracleNG answering, not failing, so the proxy turns + // it into null and this must stay a confident "no" rather than joining the degraded case. + this._poracleApiProxy.Setup(p => p.GetConfigAsync()).ReturnsAsync((PoracleConfig?)null!); + this._poracleHumanProxy.Setup(p => p.GetAdminRolesAsync(It.IsAny())).ReturnsAsync((string?)null); + this._webhookDelegateService.Setup(s => s.GetManagedWebhookIdsAsync(It.IsAny())).ReturnsAsync([]); + + var roles = await this.CreateSut().ResolveAsync("u1"); + + Assert.False(roles.IsAdmin); + Assert.True(roles.Resolved); + } + + [Fact] + public async Task AdminStatusIsNeverTakenFromTheRolesBody() + { + // Two isAdmin branches used to read one. Neither could ever fire -- both API versions build the + // body from the same adminRolesResult, whose fields are channels, webhooks and users, and v2's + // schema is additionalProperties:false. Left in, they were a promotion path from a field + // upstream does not send, which is a worse thing to carry than a missing feature. + this._poracleApiProxy.Setup(p => p.GetConfigAsync()).ReturnsAsync((PoracleConfig?)null!); + this._poracleHumanProxy + .Setup(p => p.GetAdminRolesAsync(It.IsAny())) + .ReturnsAsync("""{"isAdmin":true,"admin":{"discord":{"isAdmin":true,"webhooks":[],"users":false}}}"""); + this._webhookDelegateService.Setup(s => s.GetManagedWebhookIdsAsync(It.IsAny())).ReturnsAsync([]); + + var roles = await this.CreateSut().ResolveAsync("u1"); + + Assert.False(roles.IsAdmin); } [Fact] @@ -75,7 +127,7 @@ public async Task AGenuineNonAdminIsResolvedAndCached() // The legitimate-case-still-passes half: a clean "no" must still be a usable answer, and must // still be cached. this._poracleApiProxy.Setup(p => p.GetConfigAsync()).ReturnsAsync((PoracleConfig?)null!); - this._poracleApiProxy.Setup(p => p.GetAdminRolesAsync(It.IsAny())).ReturnsAsync("{}"); + this._poracleHumanProxy.Setup(p => p.GetAdminRolesAsync(It.IsAny())).ReturnsAsync("{}"); this._webhookDelegateService.Setup(s => s.GetManagedWebhookIdsAsync(It.IsAny())).ReturnsAsync([]); var sut = this.CreateSut(); @@ -84,7 +136,7 @@ public async Task AGenuineNonAdminIsResolvedAndCached() Assert.False(roles.IsAdmin); Assert.True(roles.Resolved); - this._poracleApiProxy.Verify(p => p.GetAdminRolesAsync("u1"), Times.Once); + this._poracleHumanProxy.Verify(p => p.GetAdminRolesAsync("u1"), Times.Once); } /// /// The defect: PoracleNG returns whatever key the operator wrote in [[discord.webhook_admins]], @@ -171,7 +223,7 @@ private void ArrangeDelegate(string[] grants) }); this._poracleApiProxy.Setup(p => p.GetConfigAsync()).ReturnsAsync((PoracleConfig?)null!); - this._poracleApiProxy.Setup(p => p.GetAdminRolesAsync(It.IsAny())).ReturnsAsync(payload); + this._poracleHumanProxy.Setup(p => p.GetAdminRolesAsync(It.IsAny())).ReturnsAsync(payload); this._webhookDelegateService.Setup(s => s.GetManagedWebhookIdsAsync(It.IsAny())).ReturnsAsync([]); this._humanService.Setup(h => h.GetWebhooksAsync()).ReturnsAsync( [ From df8a0a26dd2110eb6fc8e07e9cb229a3950c5f8f Mon Sep 17 00:00:00 2001 From: hokiepokedad2 <38219945+hokiepokedad2@users.noreply.github.com> Date: Mon, 24 Aug 2026 16:46:45 -0400 Subject: [PATCH 3/5] feat(places): a saved place can be moved, and the alert language stops losing case Moving a saved place was impossible in any way: there is no v1 update route, and the delete answers 409 while any alarm still references the label -- so moving the place you live meant repointing every alarm, deleting, re-adding and repointing back. PUT /v2/humans/{id}/locations/{label} does it in one call, with the label untouched so every alarm follows. It is the only operation in this migration with no older equivalent, so it is gated rather than degraded: IPlaceUpdateCapabilityService, same shape as MuteCapabilityService, 5.2.0, failing closed. On an older server the pencil is absent with no @else and no tooltip naming a version at someone who cannot act on it -- the delete-and-re-add flow is still right there. Also fixed, and independent of the migration: Poracle lowercases the language it stores, on both API versions and from the bot's own !language command, so humans.language reads back "pt-br" where the picker's list says "pt-BR". The comparison was exact, so it dropped the value and quietly fell back to the server default. It ignores case now and stores the list's own spelling back. --- .../ServiceCollectionExtensions.cs | 3 +- .../Controllers/LocationController.cs | 76 +++++++++++++++++- .../ClientApp/src/app/core/models/index.ts | 5 ++ .../services/alert-language.service.spec.ts | 27 +++++++ .../core/services/alert-language.service.ts | 11 ++- .../app/core/services/places.service.spec.ts | 58 ++++++++++++++ .../src/app/core/services/places.service.ts | 17 ++++ .../places-section.component.html | 5 ++ .../places-section.component.spec.ts | 38 +++++++++ .../places-section.component.ts | 32 ++++++++ .../ClientApp/src/assets/i18n/da.json | 3 + .../ClientApp/src/assets/i18n/de.json | 3 + .../ClientApp/src/assets/i18n/en.json | 3 + .../ClientApp/src/assets/i18n/es.json | 3 + .../ClientApp/src/assets/i18n/fr.json | 3 + .../ClientApp/src/assets/i18n/it.json | 3 + .../ClientApp/src/assets/i18n/nl.json | 3 + .../ClientApp/src/assets/i18n/pl.json | 3 + .../ClientApp/src/assets/i18n/pt-BR.json | 3 + .../ClientApp/src/assets/i18n/pt.json | 3 + .../ClientApp/src/assets/i18n/sv.json | 3 + CHANGELOG.md | 6 ++ CLAUDE.md | 10 +++ .../Services/IPlaceUpdateCapabilityService.cs | 10 +++ .../PlaceUpdateCapabilityService.cs | 41 ++++++++++ .../Controllers/LocationControllerTests.cs | 78 ++++++++++++++++++- .../PlaceUpdateCapabilityServiceTests.cs | 74 ++++++++++++++++++ docs/architecture/poracleng-compatibility.md | 12 ++- docs/architecture/poracleng-proxy.md | 30 ++++++- 29 files changed, 555 insertions(+), 11 deletions(-) create mode 100644 Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/places.service.spec.ts create mode 100644 Core/Pgan.PoracleWebNet.Core.Abstractions/Services/IPlaceUpdateCapabilityService.cs create mode 100644 Core/Pgan.PoracleWebNet.Core.Services/PlaceUpdateCapabilityService.cs create mode 100644 Tests/Pgan.PoracleWebNet.Tests/Services/PlaceUpdateCapabilityServiceTests.cs diff --git a/Applications/Pgan.PoracleWebNet.Api/Configuration/ServiceCollectionExtensions.cs b/Applications/Pgan.PoracleWebNet.Api/Configuration/ServiceCollectionExtensions.cs index 4f761b6b..528a736d 100644 --- a/Applications/Pgan.PoracleWebNet.Api/Configuration/ServiceCollectionExtensions.cs +++ b/Applications/Pgan.PoracleWebNet.Api/Configuration/ServiceCollectionExtensions.cs @@ -90,7 +90,8 @@ public static IServiceCollection AddPoracleServices(this IServiceCollection serv services.AddScoped(); services.AddScoped(); services.AddScoped(); - services.AddScoped(); + services.AddScoped(); + services.AddScoped(); services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/Applications/Pgan.PoracleWebNet.Api/Controllers/LocationController.cs b/Applications/Pgan.PoracleWebNet.Api/Controllers/LocationController.cs index b6913a9c..4ca4cb91 100644 --- a/Applications/Pgan.PoracleWebNet.Api/Controllers/LocationController.cs +++ b/Applications/Pgan.PoracleWebNet.Api/Controllers/LocationController.cs @@ -15,6 +15,7 @@ public class LocationController( IPoracleHumanProxy humanProxy, IPoracleApiProxy poracleApiProxy, IHttpClientFactory httpClientFactory, + IPlaceUpdateCapabilityService placeUpdateCapability, IScannerService? scannerService = null) : BaseApiController { private readonly IHumanService _humanService = humanService; @@ -22,6 +23,7 @@ public class LocationController( private readonly IPoracleHumanProxy _humanProxy = humanProxy; private readonly IPoracleApiProxy _poracleApiProxy = poracleApiProxy; private readonly IHttpClientFactory _httpClientFactory = httpClientFactory; + private readonly IPlaceUpdateCapabilityService _placeUpdateCapability = placeUpdateCapability; private readonly IScannerService? _scannerService = scannerService; [HttpGet] @@ -283,9 +285,79 @@ public double? Longitude /// /// The user's saved places, plus the profile pin every alarm falls back to. /// + /// + /// canEdit rides along rather than sitting on its own endpoint, matching + /// MuteController: the list is read on every visit to the Areas page, and a second call for + /// one boolean would double that for no gain. + /// [HttpGet("places")] - public async Task GetPlaces() => - this.Ok(await this._humanProxy.GetPlacesAsync(this.UserId)); + public async Task GetPlaces(CancellationToken cancellationToken) => + this.Ok(await this.PlacesWithCapabilityAsync(cancellationToken)); + + /// + /// The place list in the one shape every caller gets, capability included. The PUT answers it too: + /// a reply missing canEdit would clear the flag the SPA is holding and hide the control the user + /// just used. + /// + private async Task PlacesWithCapabilityAsync(CancellationToken cancellationToken) + { + var places = await this._humanProxy.GetPlacesAsync(this.UserId); + + return new + { + places.Default, + places.Named, + canEdit = await this._placeUpdateCapability.IsPlaceUpdateAvailableAsync(cancellationToken), + }; + } + + /// + /// Moves a saved place, keeping its label so every alarm pointing at it follows. + /// + /// + /// New on PoracleNG 5.2.0. Before it, a place an alarm referenced could not be moved at all: the + /// delete answers 409 while anything still points at it, so the only route was to repoint every + /// alarm, delete, re-add and repoint back. An older server answers 501 and the SPA hides the control. + /// + [HttpPut("places/{label}")] + public async Task UpdatePlace( + string label, [FromBody] PlaceMoveRequest request, CancellationToken cancellationToken = default) + { + if (request.Latitude is not { } latitude || request.Longitude is not { } longitude) + { + return this.BadRequest(new { error = "Latitude and longitude are required." }); + } + + if (latitude is < -90 or > 90 || longitude is < -180 or > 180) + { + return this.BadRequest(new { error = "Latitude must be -90 to 90 and longitude -180 to 180." }); + } + + var moved = await this._humanProxy.UpdatePlaceAsync(this.UserId, label, latitude, longitude); + + return moved + ? this.Ok(await this.PlacesWithCapabilityAsync(cancellationToken)) + : this.StatusCode(StatusCodes.Status501NotImplemented, new + { + error = "This Poracle server cannot move a saved place. Delete it and add it again.", + }); + } + + /// New coordinates for a saved place. The label is the path segment and does not change. + public class PlaceMoveRequest + { + [Range(-90, 90)] + public double? Latitude + { + get; set; + } + + [Range(-180, 180)] + public double? Longitude + { + get; set; + } + } /// /// Saves a place an alarm can be anchored to. diff --git a/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/models/index.ts b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/models/index.ts index 972c3e46..53f84b09 100644 --- a/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/models/index.ts +++ b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/models/index.ts @@ -717,6 +717,11 @@ export interface SavedPlace { /** Everywhere a user's alarms can be anchored: the profile pin, plus whatever they have named. */ export interface SavedPlaces { + /** + * Whether this Poracle server can move a place without deleting it first. Absent on an older + * PoracleWeb.NET API, and treated as false, because the edit is what would 404. + */ + canEdit?: boolean; /** The profile pin every alarm falls back to. Absent when the user has never set a location. */ default?: null | SavedPlace; named: SavedPlace[]; diff --git a/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/alert-language.service.spec.ts b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/alert-language.service.spec.ts index ec69280e..fd6cb1d3 100644 --- a/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/alert-language.service.spec.ts +++ b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/alert-language.service.spec.ts @@ -96,6 +96,33 @@ describe('AlertLanguageService', () => { expect(store['poracle-language']).toBe('it'); }); + it('should recognise a stored language whose case Poracle changed', () => { + // Poracle lowercases what it stores, on both API versions and from the bot's !language command, so + // humans.language reads back 'pt-br' where this list says 'pt-BR'. An exact match dropped it and + // the picker silently reverted to the server default. + locationService.getLanguage.mockReturnValue(of({ language: 'pt-br' })); + const { alert, i18n } = create(); + i18n.init(undefined, 'de'); + + alert.load(); + + expect(alert.selected()).toBe('pt-BR'); + expect(store['poracle-language']).toBe('pt-BR'); + }); + + it('should still ignore a language this UI does not ship', () => { + // The other half: Poracle carries translations we do not, and coercing one of them onto a UI + // language would put Japanese prose behind an English flag. + locationService.getLanguage.mockReturnValue(of({ language: 'ja' })); + const { alert, i18n } = create(); + i18n.init(undefined, 'de'); + + alert.load(); + + expect(alert.selected()).toBe('de'); + expect(store['poracle-language']).toBeUndefined(); + }); + it('should keep the server locale when humans.language is unset', () => { const { alert, i18n } = create(); i18n.init(undefined, 'de'); diff --git a/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/alert-language.service.ts b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/alert-language.service.ts index 4c3a7dc9..01429a6b 100644 --- a/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/alert-language.service.ts +++ b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/alert-language.service.ts @@ -65,9 +65,14 @@ export class AlertLanguageService { this.locationService.getLanguage().subscribe({ error: () => undefined, next: ({ language }) => { - if (language && this.languages.some(l => l.code === language)) { - this.chosen.set(language); - localStorage.setItem(STORAGE_KEY, language); + // Case-insensitively, and stored back in this list's casing. Poracle lowercases what it stores, + // on both API versions and from the bot's own !language command, so humans.language for a + // Brazilian Portuguese user reads back as 'pt-br' while the code here is 'pt-BR'. An exact + // comparison dropped it silently and the picker fell back to the server default. + const known = language ? this.languages.find(l => l.code.toLowerCase() === language.toLowerCase()) : undefined; + if (known) { + this.chosen.set(known.code); + localStorage.setItem(STORAGE_KEY, known.code); } }, }); diff --git a/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/places.service.spec.ts b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/places.service.spec.ts new file mode 100644 index 00000000..7d1b9aec --- /dev/null +++ b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/places.service.spec.ts @@ -0,0 +1,58 @@ +import { provideHttpClient } from '@angular/common/http'; +import { HttpTestingController, provideHttpClientTesting } from '@angular/common/http/testing'; +import { TestBed } from '@angular/core/testing'; + +import { ConfigService } from './config.service'; +import { PlacesService } from './places.service'; + +const API = 'http://test'; + +describe('PlacesService', () => { + let httpMock: HttpTestingController; + let service: PlacesService; + + beforeEach(() => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + providers: [PlacesService, { provide: ConfigService, useValue: { apiHost: API } }, provideHttpClient(), provideHttpClientTesting()], + }); + + service = TestBed.inject(PlacesService); + httpMock = TestBed.inject(HttpTestingController); + }); + + afterEach(() => httpMock.verify()); + + it('holds canEdit from the list response', () => { + service.load().subscribe(); + httpMock.expectOne(`${API}/api/location/places`).flush({ named: [], canEdit: true, default: null }); + + expect(service.canEdit()).toBe(true); + }); + + it('treats a response with no canEdit as not editable', () => { + // An older PoracleWeb.NET API, or one talking to a Poracle without the route. The edit is what + // would 404, so absent has to mean no. + service.load().subscribe(); + httpMock.expectOne(`${API}/api/location/places`).flush({ named: [], default: null }); + + expect(service.canEdit()).toBe(false); + }); + + it('puts the new point under the existing label', () => { + service.move('work', 9.5, 8.5).subscribe(); + + const request = httpMock.expectOne(`${API}/api/location/places/work`); + expect(request.request.method).toBe('PUT'); + expect(request.request.body).toEqual({ latitude: 9.5, longitude: 8.5 }); + request.flush({ named: [{ label: 'work', latitude: 9.5, longitude: 8.5 }], canEdit: true, default: null }); + + expect(service.named()[0].latitude).toBe(9.5); + }); + + it('encodes a label with a slash in it rather than growing a path segment', () => { + service.move('home/office', 1, 2).subscribe(); + + httpMock.expectOne(`${API}/api/location/places/home%2Foffice`).flush({ named: [], canEdit: true, default: null }); + }); +}); diff --git a/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/places.service.ts b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/places.service.ts index e024ff12..3287014f 100644 --- a/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/places.service.ts +++ b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/core/services/places.service.ts @@ -18,6 +18,13 @@ export class PlacesService { private readonly http = inject(HttpClient); private readonly places = signal(null); + /** + * Whether this Poracle server can move a place without deleting it first. Needs PoracleNG 5.2.0; + * before it, a place an alarm referenced could not be moved at all, because the delete answers 409 + * while anything still points at it. False until {@link load} has run. + */ + readonly canEdit = computed(() => this.places()?.canEdit === true); + /** Named places only. Empty until {@link load} has run. */ readonly named = computed(() => this.places()?.named ?? []); @@ -33,6 +40,16 @@ export class PlacesService { return this.http.get(`${this.config.apiHost}/api/location/places`).pipe(tap(places => this.places.set(places))); } + /** + * Moves a place, keeping its label so every alarm pointing at it follows. Answers 501 on a server + * without the endpoint, which is why {@link canEdit} gates the control that calls this. + */ + move(label: string, latitude: number, longitude: number): Observable { + return this.http + .put(`${this.config.apiHost}/api/location/places/${encodeURIComponent(label)}`, { latitude, longitude }) + .pipe(tap(updated => this.places.set(updated))); + } + /** * Deletes a place. Answers 409 with `referencingRules` when alarms still point at it — the caller * should name them rather than reporting a bare failure. diff --git a/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/shared/components/places-section/places-section.component.html b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/shared/components/places-section/places-section.component.html index e693a599..0d8ce5bb 100644 --- a/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/shared/components/places-section/places-section.component.html +++ b/Applications/Pgan.PoracleWebNet.App/ClientApp/src/app/shared/components/places-section/places-section.component.html @@ -46,6 +46,11 @@

+ @if (places.canEdit()) { + + }