btcmaps v1: address @rollforsats PR #211 testing review
Resolves the four findings from rollforsats's 2026-04-26 testing pass on the directory-submission path (PR #211 discussion thread starting at r3144167499). Three server-side fixes plus a clarification on #4. 1. Twitter link points to GitHub instead of X (BuildPrBody). - Bare `@handle` in markdown auto-resolves to `github.com/<handle>` when GitHub renders the directory PR. - Fix: render as an explicit `[@handle](https://x.com/<handle>)` link, normalising a leading `@` for the URL form. 2. merchants.json full-file diff on append (+142 / -131 in rollforsats/directory.btcpayserver.org#5). - Root cause: `JsonSerializer.Serialize` defaults to `JavaScriptEncoder.Default`, which is HTML-safe (escapes `<`, `>`, `&`, `'`, `"` plus all non-ASCII as `\uXXXX`). Every existing entry containing `'`, `&`, or non-ASCII (`'`, `u`, `n`, etc.) is rewritten in escape form even though no content changed. - Fix: pass `Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping`, the canonical option for writing JSON to a standalone file. Safe here because merchants.json is consumed by btcmaps tooling, not interpolated into HTML. After the change, an append diff is +N / -0 where N is the line count of the new entry. 3. Description required even when `submitToDirectory=false`. - Description is only consumed by BuildPrBody; tagOnOsm-only and unlistFromOsm-only requests have no consumer for it. - Fix: move the Description gate inside the existing `if (request.SubmitToDirectory)` branch, alongside Type / SubType / Country / OnionUrl which were already conditionally gated. 4. Directory PR fires even when `submitToDirectory=false` is reported by rollforsats. Server-side is already correctly gated at BtcMapsController.cs:39 (the directory branch only runs when the flag is true). Confirmed downstream of #3: as long as the server required Description unconditionally, the plugin had to send `submitToDirectory=true` to satisfy the gate. With #3 fixed the plugin can honour the merchant's actual selection. No server-side change. Test coverage: - New: Validate_RequiresDescription_OnDirectorySubmit covers the "Description required when SubmitToDirectory=true" path. - New: Validate_AllowsMissingDescription_WhenTagOnOsmOnly covers the conditional-gate behaviour for tagOnOsm flows. - New: Validate_AllowsMissingDescription_WhenUnlistOnly covers the unlistFromOsm flow. - Updated: Validate_RejectsOverlongDescription renamed to Validate_RejectsOverlongDescription_OnDirectorySubmit and now sets SubmitToDirectory=true to match the new conditional shape. Local verification: dotnet build clean (0/0 errors), dotnet test 50/50 passing on .NET 10 RC.2 (`10.0.100-rc.2.25502.107`). Co-Authored-By: rollforsats <59777267+rollforsats@users.noreply.github.com>
This commit is contained in:
r1ckstardev
parent
9f4039f6c1
commit
c43f64b960
@ -56,18 +56,71 @@ public class BtcMapsServiceTests
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Validate_RejectsOverlongDescription()
|
||||
public void Validate_RejectsOverlongDescription_OnDirectorySubmit()
|
||||
{
|
||||
var svc = MakeService();
|
||||
var req = new BtcMapsSubmitRequest
|
||||
{
|
||||
Name = "Shop",
|
||||
Url = "https://shop.example",
|
||||
Description = new string('x', 1001)
|
||||
Description = new string('x', 1001),
|
||||
Type = "merchants",
|
||||
SubmitToDirectory = true
|
||||
};
|
||||
Assert.Contains(svc.Validate(req), e => e.Path == nameof(BtcMapsSubmitRequest.Description));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Validate_RequiresDescription_OnDirectorySubmit()
|
||||
{
|
||||
// Description is the directory PR body content; required only when actually
|
||||
// submitting to the directory.
|
||||
var svc = MakeService();
|
||||
var req = new BtcMapsSubmitRequest
|
||||
{
|
||||
Name = "Shop",
|
||||
Url = "https://shop.example",
|
||||
Type = "merchants",
|
||||
SubmitToDirectory = true
|
||||
};
|
||||
Assert.Contains(svc.Validate(req), e => e.Path == nameof(BtcMapsSubmitRequest.Description));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Validate_AllowsMissingDescription_WhenTagOnOsmOnly()
|
||||
{
|
||||
// tagOnOsm-only requests do not consume Description (the OSM tag set is
|
||||
// name + amenity + currency:XBT + payment:lightning + website). Description
|
||||
// is exclusively a directory-PR field.
|
||||
var svc = MakeService();
|
||||
var req = new BtcMapsSubmitRequest
|
||||
{
|
||||
Name = "Shop",
|
||||
Url = "https://shop.example",
|
||||
OsmNodeId = 12345,
|
||||
OsmNodeType = "node",
|
||||
TagOnOsm = true
|
||||
};
|
||||
Assert.DoesNotContain(svc.Validate(req), e => e.Path == nameof(BtcMapsSubmitRequest.Description));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Validate_AllowsMissingDescription_WhenUnlistOnly()
|
||||
{
|
||||
// unlistFromOsm-only requests strip tags from an existing OSM element; no
|
||||
// Description path on the wire.
|
||||
var svc = MakeService();
|
||||
var req = new BtcMapsSubmitRequest
|
||||
{
|
||||
Name = "Shop",
|
||||
Url = "https://shop.example",
|
||||
UnlistFromOsm = true,
|
||||
OsmNodeId = 12345,
|
||||
OsmNodeType = "node"
|
||||
};
|
||||
Assert.Empty(svc.Validate(req));
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData("merchants", "books", true)]
|
||||
[InlineData("merchants", "not-a-subtype", false)]
|
||||
|
||||
@ -1,6 +1,7 @@
|
||||
using System.Globalization;
|
||||
using System.Net.Http.Headers;
|
||||
using System.Text;
|
||||
using System.Text.Encodings.Web;
|
||||
using System.Text.Json;
|
||||
using System.Xml.Linq;
|
||||
using Microsoft.Extensions.Configuration;
|
||||
@ -59,12 +60,14 @@ public sealed class BtcMapsService
|
||||
else if (!Uri.TryCreate(url, UriKind.Absolute, out var parsed) || parsed.Scheme != Uri.UriSchemeHttps)
|
||||
errors.Add(new ValidationError(nameof(request.Url), "Must be a valid https:// URL."));
|
||||
|
||||
var description = (request.Description ?? string.Empty).Trim();
|
||||
if (string.IsNullOrEmpty(description) || description.Length > 1000)
|
||||
errors.Add(new ValidationError(nameof(request.Description), "Required, 1-1000 characters."));
|
||||
|
||||
if (request.SubmitToDirectory)
|
||||
{
|
||||
// Description is only consumed by the directory PR body; not required for
|
||||
// tagOnOsm-only or unlistFromOsm-only requests.
|
||||
var description = (request.Description ?? string.Empty).Trim();
|
||||
if (string.IsNullOrEmpty(description) || description.Length > 1000)
|
||||
errors.Add(new ValidationError(nameof(request.Description), "Required, 1-1000 characters."));
|
||||
|
||||
var type = (request.Type ?? string.Empty).Trim();
|
||||
if (string.IsNullOrEmpty(type) || !ValidTypes.Contains(type))
|
||||
errors.Add(new ValidationError(nameof(request.Type),
|
||||
@ -222,7 +225,16 @@ public sealed class BtcMapsService
|
||||
.Select(e => e!.Value)
|
||||
.ToList();
|
||||
|
||||
var updatedJson = JsonSerializer.Serialize(updated, new JsonSerializerOptions { WriteIndented = true }) + "\n";
|
||||
// Use UnsafeRelaxedJsonEscaping so non-ASCII codepoints and HTML-only "unsafe"
|
||||
// chars (`&`, `'`, `<`, `>`) are written raw in the file, matching the upstream
|
||||
// merchants.json convention. The default JavaScriptEncoder is HTML-safe and
|
||||
// would re-encode every entry containing `'` or non-ASCII as `\uXXXX`, which
|
||||
// shows up as a noisy full-file diff on every append.
|
||||
var updatedJson = JsonSerializer.Serialize(updated, new JsonSerializerOptions
|
||||
{
|
||||
WriteIndented = true,
|
||||
Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping
|
||||
}) + "\n";
|
||||
|
||||
var branchRef = await GetJsonAsync(
|
||||
client,
|
||||
@ -532,7 +544,14 @@ public sealed class BtcMapsService
|
||||
sb.AppendLine($"- **URL:** {request.Url}");
|
||||
sb.AppendLine($"- **Type:** {request.Type}{(string.IsNullOrWhiteSpace(request.SubType) ? string.Empty : " / " + request.SubType)}");
|
||||
if (!string.IsNullOrWhiteSpace(request.Country)) sb.AppendLine($"- **Country:** {request.Country}");
|
||||
if (!string.IsNullOrWhiteSpace(request.Twitter)) sb.AppendLine($"- **Twitter:** {request.Twitter}");
|
||||
if (!string.IsNullOrWhiteSpace(request.Twitter))
|
||||
{
|
||||
// Render as an explicit https://x.com/<handle> link so GitHub markdown does
|
||||
// not auto-resolve a bare `@handle` to github.com/<handle>.
|
||||
var raw = request.Twitter.Trim();
|
||||
var handle = raw.StartsWith("@") ? raw[1..] : raw;
|
||||
sb.AppendLine($"- **Twitter:** [@{handle}](https://x.com/{handle})");
|
||||
}
|
||||
if (!string.IsNullOrWhiteSpace(request.Github)) sb.AppendLine($"- **GitHub:** {request.Github}");
|
||||
sb.AppendLine();
|
||||
sb.AppendLine("**Description:**");
|
||||
|
||||
Loading…
Reference in New Issue
Block a user