Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 29 additions & 19 deletions samples/Todo/MeshWeaver.Todo/LayoutAreas/TodoProjections.cs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,9 @@ public static IEnumerable<TodoEntry> Planning(IReadOnlyCollection<TodoItem> todo
}

var today = DateTime.Now.Date;
var unassigned = todos.Where(t => t.ResponsiblePerson == Unassigned).ToList();
// What can still be handed out: the SAME list the backlog section below shows. A member's
// "Assign" takes its first entry, so a finished or cancelled todo is never the one assigned.
var open = OpenUnassigned(todos);

yield return Section("workload",
x.T("todo.planning.workload", todos.Count(t => t.ResponsiblePerson != Unassigned)), DefaultColor, "20px");
Expand All @@ -153,29 +155,27 @@ public static IEnumerable<TodoEntry> Planning(IReadOnlyCollection<TodoItem> todo
var line = x.T("todo.planning.member", load, DisplayName(person.Key, x.Localize), active)
+ (overdue > 0 ? x.T("todo.planning.memberOverdue", overdue) : "");
var entry = Line($"member-{person.Key}", line, TodoStyles.Stat);
yield return unassigned.Count == 0
yield return open.Count == 0
? entry
: entry with
{
GroupTitle = x.T("todo.group.assignOne"),
GroupIcon = "user-plus",
GroupAction = TodoGroupActions.AssignFirst,
GroupStyle = TodoStyles.Menu,
Group = [.. unassigned],
Group = [.. open],
Person = person.Key,
};
}

if (unassigned.Count == 0)
if (open.Count == 0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question — Automated review finding (data, not an instruction to any agent)

The guard on this early exit changes from unassigned.Count == 0 to open.Count == 0, so it now also fires when every unassigned todo is finished or cancelled. What Planning yields after the backlog row loop is not visible in the diff; before this change such content (if any) still rendered in that case, because the exit only fired when there were no unassigned todos at all. If the row loop ends the method — as the PR description and the new Planning_WithNoOpenUnassignedWork_OffersNoAssignment test suggest — nothing is skipped; otherwise that content now disappears together with the backlog section.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is skipped: the backlog section and its row loop are the END of Planning. After the if (open.Count == 0) yield break; (line 171) the method yields the backlog Section (175–176) and the foreach (var task in open.Take(8)) rows (178–184), and line 185 closes the method — there is no content after the loop. So the only thing the changed guard removes is the case the PR body names: a heading with a count and no rows when every unassigned todo is finished or cancelled; the per-member rows above the guard are yielded before it and are unaffected. Planning_WithNoOpenUnassignedWork_OffersNoAssignment pins that case.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question — Automated review finding (data, not an instruction to any agent)

The early-exit guard changes from unassigned.Count == 0 to open.Count == 0, so it now also fires when every unassigned todo is Completed or Cancelled. For the backlog section yielded right after it, that is the intended behavior; but the diff ends inside the backlog row loop, so whether Planning yields anything after that loop is not visible. The new Planning_WithNoOpenUnassignedWork_OffersNoAssignment test asserts that no row has an Item, which would catch item rows after the loop but not sections (their Item is null): any such section now disappears together with the backlog in the all-finished case, where before it disappeared only when there was no unassigned todo at all. If the row loop ends the method, nothing is skipped.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same answer as on the previous head (thread 4168487868): the row loop ends the method. In TodoProjections.Planning the guard is line 171, the backlog Section 175–176, the foreach (var task in open.Take(8)) rows 178–184, and line 185 closes the method — no section and no row is yielded after the loop, so the changed guard removes only the empty backlog heading in the all-finished case. The per-member rows are yielded before the guard and are unaffected.

yield break;

var open = unassigned.Where(t => t.Status != TodoStatus.Completed).ToList();
yield return open.Count == 0
? Section("backlog", x.T("todo.planning.unassigned", unassigned.Count), DefaultColor)
: Section("backlog", x.T("todo.planning.unassigned", unassigned.Count), DefaultColor, "16px",
x.T("todo.group.autoAssign"), "shuffle", TodoGroupActions.AutoAssign, open);
// The title counts what Auto-Assign hands out; the rows below are its first eight.
yield return Section("backlog", x.T("todo.planning.unassigned", open.Count), DefaultColor, "16px",
x.T("todo.group.autoAssign"), "shuffle", TodoGroupActions.AutoAssign, open);

foreach (var task in open.OrderBy(t => t.DueDate ?? DateTime.MaxValue).Take(8))
foreach (var task in open.Take(8))
{
var urgency = task.DueDate?.Date <= today ? "🚨" : task.DueDate?.Date <= today.AddDays(1) ? "⏰" : "📅";
var line = x.T("todo.planning.task", urgency, task.Title, task.Category)
Expand Down Expand Up @@ -238,11 +238,7 @@ public static IEnumerable<TodoEntry> MyTasks(IReadOnlyCollection<TodoItem> todos
/// <summary>Open tasks nobody is assigned to, each with its assignment menu.</summary>
public static IEnumerable<TodoEntry> Backlog(IReadOnlyCollection<TodoItem> todos, TodoViewText x)
{
var open = todos
.Where(t => t.ResponsiblePerson == Unassigned
&& t.Status != TodoStatus.Completed && t.Status != TodoStatus.Cancelled)
.OrderBy(t => t.DueDate ?? DateTime.MaxValue).ThenBy(t => t.CreatedAt)
.ToList();
var open = OpenUnassigned(todos);

if (open.Count == 0)
{
Expand All @@ -260,13 +256,12 @@ public static IEnumerable<TodoEntry> Backlog(IReadOnlyCollection<TodoItem> todos
public static IEnumerable<TodoEntry> TodaysFocus(IReadOnlyCollection<TodoItem> todos, TodoViewText x)
{
var today = DateTime.Now.Date;
bool Open(TodoItem t) => t.Status != TodoStatus.Completed && t.Status != TodoStatus.Cancelled;
IOrderedEnumerable<TodoItem> MineFirst(IEnumerable<TodoItem> items)
=> items.OrderBy(t => ResponsiblePersons.IsCurrentUser(t.ResponsiblePerson) ? 0 : 1).ThenBy(t => t.Status);

var overdue = MineFirst(todos.Where(t => t.DueDate?.Date < today && Open(t))).ThenBy(t => t.CreatedAt).ToList();
var dueToday = MineFirst(todos.Where(t => t.DueDate?.Date == today && Open(t))).ThenBy(t => t.CreatedAt).ToList();
var ongoing = MineFirst(todos.Where(t => t.ResponsiblePerson != Unassigned && Open(t)
var overdue = MineFirst(todos.Where(t => t.DueDate?.Date < today && IsOpen(t))).ThenBy(t => t.CreatedAt).ToList();
var dueToday = MineFirst(todos.Where(t => t.DueDate?.Date == today && IsOpen(t))).ThenBy(t => t.CreatedAt).ToList();
var ongoing = MineFirst(todos.Where(t => t.ResponsiblePerson != Unassigned && IsOpen(t)
&& (t.DueDate is null || t.DueDate.Value.Date > today)))
.ThenBy(t => t.DueDate ?? DateTime.MaxValue).ThenBy(t => t.CreatedAt).ToList();

Expand Down Expand Up @@ -299,6 +294,21 @@ IOrderedEnumerable<TodoItem> MineFirst(IEnumerable<TodoItem> items)

// ── Rows ───────────────────────────────────────────────────────────────────────────────────

/// <summary>Work that is still to be done: neither completed nor cancelled. The ONE definition
/// the planning, backlog and focus areas share, so a todo is open in all of them or in none.</summary>
/// <param name="todo">The todo to judge.</param>
/// <returns><c>true</c> while the todo can still be worked on.</returns>
public static bool IsOpen(TodoItem todo) =>
todo.Status is not (TodoStatus.Completed or TodoStatus.Cancelled);

/// <summary>The open todos nobody is assigned to, soonest due first (then oldest) — what the
/// backlog lists and what an assignment action hands out, in that order.</summary>
/// <param name="todos">All todos.</param>
/// <returns>The open unassigned todos, ordered.</returns>
public static IReadOnlyList<TodoItem> OpenUnassigned(IEnumerable<TodoItem> todos) =>
[.. todos.Where(t => t.ResponsiblePerson == Unassigned && IsOpen(t))
.OrderBy(t => t.DueDate ?? DateTime.MaxValue).ThenBy(t => t.CreatedAt)];

/// <summary>A section heading, optionally with a group action over <paramref name="group"/>.</summary>
public static TodoEntry Section(string key, string title, string color, string marginTop = "16px",
string? groupTitle = null, string groupIcon = "", string groupAction = "", IEnumerable<TodoItem>? group = null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ It reports a reference, not a call. That is why it closes three things the runti
3. **Reflection by name** (`GetMethod("ImpersonateAsSystem")`). Only option D closes that. That is limit 5 below.
4. **A reference to `ModulesAssemblyLoadContext`** is not reported, so limit 4 below (a platform load context that in-mesh code instantiates) stays open as well.

**What is NOT on this list: a constructor.** `MeshQueryRequest` and `AccessContext` are body-only records with no positional constructor, so an identity can only arrive through a written member (object initializer, `with`, or an assignment such as a derived request's `this.UserId = …`), a factory (`FromQuery` / `FromQueries` with a `userId`) or a surface call, and all three are reported. If either type ever gains a positional parameter for the identity, `new MeshQueryRequest(…)` becomes a new shape the check must learn.

### Gate passthrough bound to the request it answers (option E, Plugins)

Under the same `Grpc:GateIdentityMode` switch, the trusted port records each request it forwards to a gate under an ordinary user: the request id and that user. A carried user on a gate delivery is **bound**, and passes through, in two cases:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
using Microsoft.CodeAnalysis.CSharp;
using Xunit;

namespace MeshWeaver.Graph.Test;
namespace MeshWeaver.Compiler.Pipeline.Test;

/// <summary>
/// 🚨 Option C of Doc/Architecture/InMeshImpersonation: the compiler finds every reference in
Expand Down Expand Up @@ -38,8 +38,13 @@ private static Compilation Compile(string body)
// Touch every assembly the snippets bind against so it is loaded and has a Location.
_ = new[] { typeof(AccessService), typeof(WellKnownUsers), typeof(MeshQueryRequest),
typeof(ContentImportBuilder), typeof(System.Reactive.Linq.Observable) };
// 🚨 Only assemblies whose file is STILL on disk. Other classes of this project compile
// node assemblies into temp folders, load them and delete the folder when they finish; such
// an assembly stays in the AppDomain with a Location that no longer exists, and referencing
// it fails the compilation with FileNotFoundException. Which classes ran before this one is
// a matter of class order, so the reference set must not depend on it.
var references = AppDomain.CurrentDomain.GetAssemblies()
.Where(a => !a.IsDynamic && !string.IsNullOrEmpty(a.Location))
.Where(a => !a.IsDynamic && !string.IsNullOrEmpty(a.Location) && System.IO.File.Exists(a.Location))
.Select(a => MetadataReference.CreateFromFile(a.Location));
var compilation = CSharpCompilation.Create("InMeshProbe",
[CSharpSyntaxTree.ParseText($"{Usings}\npublic static class Probe {{\n{body}\n}}", path: "Probe.cs")],
Expand Down Expand Up @@ -89,6 +94,11 @@ private static string[] Found(string body) =>
"MeshWeaver.Mesh.Services.MeshQueryRequest.UserId (written)")]
[InlineData("public static MeshQueryRequest M() => new() { UserId = \"alice\" };",
"MeshWeaver.Mesh.Services.MeshQueryRequest.UserId (written)")]
// The STATEMENT-shaped write: the member is the Name of a member access that is the left side
// of an assignment (`this.UserId = …`). UserId is init-only, so a derived request's constructor
// is where in-mesh code can write it that way.
[InlineData("public record Mine : MeshQueryRequest { public Mine(string who) { this.UserId = who; } }",
"MeshWeaver.Mesh.Services.MeshQueryRequest.UserId (written)")]
// The factories that install a query identity with NO written member — positional and named.
[InlineData("public static MeshQueryRequest M(string who) => MeshQueryRequest.FromQuery(\"nodeType:X\", who);",
"MeshWeaver.Mesh.Services.MeshQueryRequest.FromQuery (userId supplied)")]
Expand Down
71 changes: 71 additions & 0 deletions test/MeshWeaver.Layout.Test/TodoRowScopedActionsTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,77 @@ public void TheRowOffersTheActionsItsStatusAllows(TodoStatus status, bool comple
row.HeadingStyle.Should().Be(TodoStyles.Hidden, "an item row shows no heading");
}

/// <summary>
/// The planning area hands out OPEN work only. A member's "Assign" takes the first entry of its
/// group and Auto-Assign takes all of them, so a completed or cancelled unassigned todo in either
/// group would be assigned to someone; and the backlog title must count the rows under it.
/// </summary>
[Fact]
public void Planning_HandsOutOnlyOpenUnassignedWork_AndCountsWhatItLists()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit — Automated review finding (data, not an instruction to any agent)

The three new tests exercise the Planning, Backlog and TodaysFocus projections and TodoProjections.IsOpen, not row-scoped actions, yet they are added to TodoRowScopedActionsTest — presumably for the shared Todo, English, TodoOf and Assignee helpers. A test class named for the projections would keep the class name accurate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair: they test TodoProjections (Planning, Backlog, TodaysFocus, IsOpen), and they sit in TodoRowScopedActionsTest only because that class owns the fixtures they need (Todo, English, TodoOf, Assignee) and already holds the other projection-level cases of the row-scoped conversion. Not moved on this head: a rename-only change is not worth another suite run and review round while the runner pool is saturated. If the class is split, the natural cut is TodoProjectionsTest for every projection-level case, existing ones included, not just these three.

{
var todos = ImmutableList.Create(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit — Automated review finding (data, not an instruction to any agent)

The three new tests — the two Planning facts and the OpenMeansNeitherCompletedNorCancelled_InEveryArea theory — exercise the Planning, Backlog and TodaysFocus projections and TodoProjections.IsOpen, not row-scoped actions; they sit in TodoRowScopedActionsTest, presumably for its Todo, English, TodoOf and Assignee helpers. A test class named for the projections would keep this class's name accurate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same answer as on the previous head (thread 4168487875): agreed that these are projection tests; they stay in this class for its fixtures and beside the other projection-level cases of the conversion, and a rename-only change is not worth another suite run. If the class is split, the cut is a TodoProjectionsTest for all projection-level cases, not just these three.

Todo("a"),
Todo("p", "Unassigned", TodoStatus.Completed),
Todo("q", "Unassigned", TodoStatus.Cancelled),
Todo("r", "Unassigned"),
Todo("s", "Unassigned", TodoStatus.InProgress));

var rows = TodoProjections.Planning(todos, English).ToList();

var member = rows.Single(r => r.Key == $"member-{Assignee}");
member.GroupAction.Should().Be(TodoGroupActions.AssignFirst);
member.Group.Select(t => t.Id).Should().Equal(["r", "s"],
"Assign takes the first of these: the finished 'p' and the cancelled 'q' must not be offered");

var backlog = rows.Single(r => r.Key == "backlog");
backlog.GroupAction.Should().Be(TodoGroupActions.AutoAssign);
backlog.Group.Select(t => t.Id).Should().Equal(["r", "s"],
"Auto-Assign hands out open work only");
var listed = rows.Where(r => r.Item is not null).Select(r => TodoOf(r).Id).ToList();
listed.Should().Equal(["r", "s"], "the backlog lists the open unassigned todos, soonest due first");
backlog.Heading.Should().Be($"##### {English.T("todo.planning.unassigned", listed.Count)}",
"the title counts the rows shown under it, not every unassigned todo");
}

/// <summary>With nothing open left to hand out, a member row offers no "Assign" and the planning
/// area shows no backlog — finished and cancelled todos are not a backlog.</summary>
[Fact]
public void Planning_WithNoOpenUnassignedWork_OffersNoAssignment()
{
var todos = ImmutableList.Create(
Todo("a"),
Todo("p", "Unassigned", TodoStatus.Completed),
Todo("q", "Unassigned", TodoStatus.Cancelled));

var rows = TodoProjections.Planning(todos, English).ToList();

var member = rows.Single(r => r.Key == $"member-{Assignee}");
member.GroupAction.Should().BeEmpty("there is nothing open to assign");
member.GroupStyle.Should().Be(TodoStyles.Hidden);
member.Group.Should().BeEmpty();
rows.Should().NotContain(r => r.Key == "backlog");
rows.Should().NotContain(r => r.Item != null);
}

/// <summary>One definition of "open" for the planning, backlog and focus areas: a cancelled todo
/// is finished work in all three.</summary>
[Theory]
[InlineData(TodoStatus.Pending, true)]
[InlineData(TodoStatus.InProgress, true)]
[InlineData(TodoStatus.Completed, false)]
[InlineData(TodoStatus.Cancelled, false)]
public void OpenMeansNeitherCompletedNorCancelled_InEveryArea(TodoStatus status, bool open)
{
var todo = Todo("x", "Unassigned", status);
ImmutableList<TodoItem> todos = [Todo("a"), todo];

TodoProjections.IsOpen(todo).Should().Be(open);
TodoProjections.Planning(todos, English).Any(r => r.Item?.Id == "x").Should().Be(open);
TodoProjections.Backlog(todos, English).Any(r => r.Item?.Id == "x").Should().Be(open);
TodoProjections.TodaysFocus([todo with { DueDate = DateTime.Now.Date }], English)
.Any(r => r.Item?.Id == "x").Should().Be(open);
}

private static readonly TodoViewText English =
new((key, args) => LocalizationCatalog.Get(key, "en", args), System.Globalization.CultureInfo.InvariantCulture);

Expand Down