Skip to content

fix(menu): remove the dead Reports menu item by its link alone (#2136) #2139

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
renemadsen merged 1 commit into from
Oct 7, 2026
Merged

fix(menu): remove the dead Reports menu item by its link alone (#2136) #2139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
96 changes: 95 additions & 1 deletion eFormAPI/Plugins/ItemsPlanning.Pn/ItemsPlanning.Pn.Test/DeadReportsMenuCleanupTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ namespace ItemsPlanning.Pn.Test;

/// <summary>
/// Plugin start removes the dead "Reports" menu entry (#2136) from the host's Angular database: the item,
/// its template and their child rows go; a second start changes nothing; other menu entries are untouched.
/// its template and their child rows go, whatever E2E id they carry; a second start changes nothing; other
/// menu entries, including ones whose link merely starts like the dead one, are untouched.
/// Runs <see cref="EformItemsPlanningPlugin.Configure"/> against a real MariaDB, the way the host calls it.
/// </summary>
[TestFixture]
Expand Down Expand Up @@ -169,6 +170,99 @@ public async Task PluginStart_RemovesTheDeadReportsEntry_AndASecondStartIsANoOp(
"a second start changes nothing");
}

[Test]
public async Task PluginStart_RemovesTheDeadLink_WhateverItsE2EIdAndWithoutATemplate()
{
int advancedItemId, trailingSlashItemId, lookalikeItemId, renamedTemplateId;
await using (var db = NewDbContext())
{
var securityGroup = new SecurityGroup { Name = "Example group" };
db.SecurityGroups.Add(securityGroup);
await db.SaveChangesAsync();

// The shape a menu save leaves behind: the dead link under the "advanced" E2E id, no template.
var advanced = new MenuItem
{
Name = "Reports",
E2EId = "advanced",
Link = DeadReportsMenuCleaner.Link,
Type = MenuItemTypeEnum.Link,
Translations = { new MenuItemTranslation { Name = "Reports", LocaleName = "en-US", Language = "English" } },
MenuItemSecurityGroups = { new MenuItemSecurityGroup { SecurityGroup = securityGroup } }
};
var trailingSlash = new MenuItem
{
Name = "Reports",
E2EId = "example-reports",
Link = DeadReportsMenuCleaner.Link + "/",
Type = MenuItemTypeEnum.Link
};
// A user's own entry whose link only starts like the dead one: it stays.
var lookalike = new MenuItem
{
Name = "Example reports archive",
E2EId = "advanced",
Link = DeadReportsMenuCleaner.Link + "-archive",
Type = MenuItemTypeEnum.Link,
Translations =
{
new MenuItemTranslation { Name = "Example reports archive", LocaleName = "en-US", Language = "English" }
},
MenuItemSecurityGroups = { new MenuItemSecurityGroup { SecurityGroup = securityGroup } }
};
// A template on the dead link under another E2E id goes too.
var renamedTemplate = new MenuTemplate
{
Name = "Reports",
E2EId = "example-reports-template",
DefaultLink = DeadReportsMenuCleaner.Link,
Comment on lines +214 to +218

@renemadsen renemadsen Oct 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Considered, not acted on: this is a coverage suggestion, not a correctness defect. The permission-removal and detachment queries use the same link-only predicate as the item/template queries this test does pin down, and both relationships are already exercised end-to-end (with permission and detached item) by PluginStart_RemovesTheDeadReportsEntry_AndASecondStartIsANoOp.

Translations =
{
new MenuTemplateTranslation { Name = "Reports", LocaleName = "en-US", Language = "English" }
}
};
db.MenuItems.AddRange(advanced, trailingSlash, lookalike);
db.MenuTemplates.Add(renamedTemplate);
await db.SaveChangesAsync();

advancedItemId = advanced.Id;
trailingSlashItemId = trailingSlash.Id;
lookalikeItemId = lookalike.Id;
renamedTemplateId = renamedTemplate.Id;
}

var before = await SnapshotAsync();

new EformItemsPlanningPlugin().Configure(_appBuilder);

var after = await SnapshotAsync();
Assert.That(after, Is.EqualTo(before with
{
MenuItemIds = before.MenuItemIds.Where(x => x != advancedItemId && x != trailingSlashItemId).ToList(),
MenuItemTranslations = before.MenuItemTranslations - 1,
MenuItemSecurityGroups = before.MenuItemSecurityGroups - 1,
MenuTemplates = before.MenuTemplates - 1,
MenuTemplateTranslations = before.MenuTemplateTranslations - 1
}).Using<Snapshot>(SameSnapshot));

await using (var db = NewDbContext())
{
Assert.That(await db.MenuTemplates.AnyAsync(x => x.Id == renamedTemplateId), Is.False);
var kept = await db.MenuItems.SingleAsync(x => x.Id == lookalikeItemId);
Assert.That(kept.Link, Is.EqualTo(DeadReportsMenuCleaner.Link + "-archive"));
Assert.That(kept.E2EId, Is.EqualTo("advanced"));
Assert.That(await db.MenuItemTranslations.CountAsync(x => x.MenuItemId == lookalikeItemId),
Is.EqualTo(1));
Assert.That(await db.MenuItemSecurityGroups.CountAsync(x => x.MenuItemId == lookalikeItemId),
Is.EqualTo(1));
}

new EformItemsPlanningPlugin().Configure(_appBuilder);

Assert.That(await SnapshotAsync(), Is.EqualTo(after).Using<Snapshot>(SameSnapshot),
"a second start changes nothing");
}

[Test]
public void PluginStart_WithoutTheAngularDatabase_DoesNotThrow()
{
Expand Down
22 changes: 13 additions & 9 deletions ...lugins/ItemsPlanning.Pn/ItemsPlanning.Pn/Infrastructure/Helpers/DeadReportsMenuCleaner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,21 +32,24 @@ namespace ItemsPlanning.Pn.Infrastructure.Helpers;
/// The "reports" page left the Angular module in 2023, but the plugin kept registering a "Reports" menu
/// entry for it. Clicking it matches no route and the frontend's catch-all silently lands on "My eForms"
/// (#2136). The host only ever adds plugin menu entries, so the plugin removes its own dead one on start.
/// Only rows that carry both the dead link and its E2E id are touched; a second run finds nothing.
/// Rows are matched on the dead link alone (with or without a trailing slash): a menu save can re-create
/// the item with another E2E id and no template, and no live page answers that link anyway. A second run
/// finds nothing.
/// </summary>
public static class DeadReportsMenuCleaner
{
public const string Link = "/plugins/items-planning-pn/reports";
public const string E2EId = "items-planning-pn-reports";
private const string LinkWithTrailingSlash = Link + "/";

/// <returns>The number of menu items plus menu templates removed.</returns>
public static int Remove(BaseDbContext dbContext, ILogger logger)
{
var menuItems = dbContext.MenuItems
.Where(x => x.Link == Link && x.E2EId == E2EId)
.Where(x => x.Link == Link || x.Link == LinkWithTrailingSlash)
.ToList();
var menuTemplates = dbContext.MenuTemplates
.Where(x => x.DefaultLink == Link && x.E2EId == E2EId)
.Where(x => x.DefaultLink == Link || x.DefaultLink == LinkWithTrailingSlash)
.ToList();

if (menuItems.Count == 0 && menuTemplates.Count == 0)
Expand All @@ -57,24 +60,25 @@ public static int Remove(BaseDbContext dbContext, ILogger logger)
// Child rows are matched through their parent's link rather than an id list, so the queries need no
// IN (...) translation.
dbContext.MenuItemTranslations.RemoveRange(dbContext.MenuItemTranslations
.Where(x => x.MenuItem.Link == Link && x.MenuItem.E2EId == E2EId));
.Where(x => x.MenuItem.Link == Link || x.MenuItem.Link == LinkWithTrailingSlash));
dbContext.MenuItemSecurityGroups.RemoveRange(dbContext.MenuItemSecurityGroups
.Where(x => x.MenuItem.Link == Link && x.MenuItem.E2EId == E2EId));
.Where(x => x.MenuItem.Link == Link || x.MenuItem.Link == LinkWithTrailingSlash));
dbContext.MenuItems.RemoveRange(menuItems);

// A menu item that still points at a removed template would block the delete; detach it the way
// the host does when it removes a plugin's templates.
foreach (var remaining in dbContext.MenuItems
.Where(x => x.MenuTemplate.DefaultLink == Link && x.MenuTemplate.E2EId == E2EId)
.Where(x => x.Link != Link || x.E2EId != E2EId))
.Where(x => x.MenuTemplate.DefaultLink == Link
|| x.MenuTemplate.DefaultLink == LinkWithTrailingSlash)
.Where(x => !(x.Link == Link || x.Link == LinkWithTrailingSlash)))
{
remaining.MenuTemplateId = null;
}

dbContext.MenuTemplateTranslations.RemoveRange(dbContext.MenuTemplateTranslations
.Where(x => x.MenuTemplate.DefaultLink == Link && x.MenuTemplate.E2EId == E2EId));
.Where(x => x.MenuTemplate.DefaultLink == Link || x.MenuTemplate.DefaultLink == LinkWithTrailingSlash));
dbContext.MenuTemplatePermissions.RemoveRange(dbContext.MenuTemplatePermissions
.Where(x => x.MenuTemplate.DefaultLink == Link && x.MenuTemplate.E2EId == E2EId));
.Where(x => x.MenuTemplate.DefaultLink == Link || x.MenuTemplate.DefaultLink == LinkWithTrailingSlash));
dbContext.MenuTemplates.RemoveRange(menuTemplates);

dbContext.SaveChanges();
Expand Down
Loading