Daily support report #22
Conversation
…ules and bower_components
… able to run the tests in SupportManager.Web.Tests
…oundTimestampToNearestMinute
…oNearestMinute.cs
…andlerTests.GetWeekSummaries.cs
There was a problem hiding this comment.
Pull request overview
This PR adds a new Daily Report page to the SupportManager application, providing per-day breakdowns of support coverage alongside the existing Monthly report. The implementation follows the established MediatR/CQRS pattern used in the Monthly report, with comprehensive unit tests for the calculation logic.
Key Changes:
- New Daily Report page with day-by-day support participation breakdown including time slots, summaries, and user contributions
- Unit test suite covering rounding functions, week slot creation, summary aggregation, and participation tracking
- Test project configuration with xunit and Shouldly frameworks
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds Bower package manager dependency (deprecated) |
| package-lock.json | Lockfile for Bower dependency |
| SupportManager.sln | Adds new test project with x64/x86 build configurations |
| SupportManager.Web/appsettings.Development.json | Removes developer-specific configuration file (moved to .gitignore) |
| SupportManager.Web/Areas/Teams/Pages/Shared/Components/Menu/Default.cshtml | Adds navigation link to Daily Report page |
| SupportManager.Web/Areas/Teams/Pages/Report/Daily.cshtml.cs | Implements PageModel with MediatR handler for daily report logic |
| SupportManager.Web/Areas/Teams/Pages/Report/Daily.cshtml | Razor view rendering daily support data in tabular format |
| SupportManager.Web.Tests/SupportManager.Web.Tests.csproj | Test project configuration with invalid package versions |
| SupportManager.Web.Tests/HandlerTests.*.cs | Unit tests for Handler methods (7 test files covering various calculation functions) |
| SupportManager.Telegram/Properties/launchSettings.json | Adds development launch profile configuration |
| .gitignore | Excludes node_modules, bower_components, and development settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| internal static List<Result.Summary> GetDaySummaries(Result.Day day) | ||
| { | ||
| var summaries = new List<Result.Summary>(); | ||
|
|
||
| var groupedDay = day.Slots.GroupBy(s => s.GroupingKey); | ||
|
|
||
| foreach (var group in groupedDay) | ||
| { | ||
| var participations = new Dictionary<string, (TimeSpan duration, DateTimeOffset? firstStart)>(); | ||
|
|
||
| foreach (var p in group.SelectMany(g => g.Participations)) | ||
| { | ||
| if (string.IsNullOrEmpty(p.UserName)) continue; | ||
|
|
||
| if (participations.TryGetValue(p.UserName, out var info)) | ||
| { | ||
| var first = info.firstStart; | ||
| if (p.FirstStart.HasValue && | ||
| (!first.HasValue || p.FirstStart.Value < first.Value)) | ||
| { | ||
| first = p.FirstStart; | ||
| } | ||
|
|
||
| participations[p.UserName] = (info.duration + p.Duration, first); | ||
| } | ||
| else | ||
| { | ||
| participations[p.UserName] = (p.Duration, p.FirstStart); | ||
| } | ||
| } | ||
|
|
||
| summaries.Add(new Result.Summary | ||
| { | ||
| Duration = TimeSpan.FromSeconds( | ||
| group.Sum(g => (g.EndTime - g.StartTime).TotalSeconds)), | ||
| GroupingKey = group.Key, | ||
| Participations = participations | ||
| .Select(x => new Result.Participation | ||
| { | ||
| UserName = x.Key, | ||
| Duration = x.Value.duration, | ||
| FirstStart = x.Value.firstStart | ||
| }) | ||
| .OrderByDescending(p => p.Duration) | ||
| .ToList() | ||
| }); | ||
| } | ||
|
|
||
| return summaries; | ||
| } |
There was a problem hiding this comment.
This method contains duplicated logic with GetWeekSummaries (lines 389-435). Both methods perform the same aggregation of participations by user and grouping key. Consider extracting this common logic into a shared helper method that accepts a collection of slots to reduce code duplication and improve maintainability.
|
|
||
| List<Result.Week> weeks = GetWeeks(weekSlots, resultStart, resultEnd, forwardingStates, lastRealState); | ||
|
|
||
| // Week-samenvattingen |
There was a problem hiding this comment.
The comment is in Dutch. For consistency and maintainability in an English codebase, comments should be in English.
| // Week-samenvattingen | |
| // Week summaries |
| // Week-samenvattingen | ||
| foreach (var week in weeks) week.Summaries.AddRange(GetWeekSummaries(week)); | ||
|
|
||
| // Dag-samenvattingen (per dag per categorie) |
There was a problem hiding this comment.
The comment is in Dutch. For consistency and maintainability in an English codebase, comments should be in English.
| // Dag-samenvattingen (per dag per categorie) | |
| // Day summaries (per day per category) |
| } | ||
|
|
||
| <h2>Daily report</h2> | ||
| <h3>@Model.Data.Date.ToString("MMMM yyyy", Thread.CurrentThread.CurrentUICulture)</h3> |
There was a problem hiding this comment.
Using 'Thread.CurrentThread.CurrentUICulture' for date formatting is a legacy pattern in ASP.NET Core. The preferred approach is to use the built-in localization features via '@CultureInfo.CurrentCulture' or configure culture-specific formatting through the ASP.NET Core localization middleware.
| var lastRealState = forwardingStates | ||
| .Where(s => s.When >= resultStart) | ||
| .OrderByDescending(s => s.When) | ||
| .FirstOrDefault(); | ||
|
|
||
| if (lastRealState != null && lastRealState.When < resultEnd) | ||
| { | ||
| resultEnd = lastRealState.When.DateTime; | ||
| } | ||
|
|
||
| List<Result.Week> weeks = GetWeeks(weekSlots, resultStart, resultEnd, forwardingStates, lastRealState); |
There was a problem hiding this comment.
The variable 'lastRealState' can be null when there are no forwarding states with When >= resultStart (line 137). However, it's passed to GetWeeks on line 144 without a null check. This could lead to a NullReferenceException when GetWeeks attempts to use lastRealState (e.g., at line 334 where lastRealState.When is accessed).
| @page | ||
| @model SupportManager.Web.Areas.Teams.Pages.Report.DailyModel | ||
|
|
||
| @using System.Threading |
There was a problem hiding this comment.
The inline 'using' directive for a namespace at the page level is unusual. System.Threading is already available in .NET 8 through implicit usings. If this is needed, consider adding it to the global usings configuration in the project file instead of including it on each page.
| @using System.Threading |
| else | ||
| { | ||
| var summaries = day.Summaries | ||
| .OrderBy(s => order[s.GroupingKey]) |
There was a problem hiding this comment.
If a summary has a GroupingKey that is not in the 'order' dictionary, this line will throw a KeyNotFoundException. The hardcoded dictionary only contains 'Kantooruren', 'Doordeweeks', and 'Weekend'. Consider using TryGetValue or providing a default ordering value to handle unexpected GroupingKey values gracefully.
| .OrderBy(s => order[s.GroupingKey]) | |
| .OrderBy(s => { | |
| if (order.TryGetValue(s.GroupingKey, out var value)) | |
| return value; | |
| return int.MaxValue; | |
| }) |
| foreach (var p in group.SelectMany(g => g.Participations)) | ||
| { | ||
| if (string.IsNullOrEmpty(p.UserName)) continue; | ||
|
|
||
| if (participations.TryGetValue(p.UserName, out var info)) | ||
| { | ||
| var first = info.firstStart; | ||
| if (p.FirstStart.HasValue && | ||
| (!first.HasValue || p.FirstStart.Value < first.Value)) | ||
| { | ||
| first = p.FirstStart; | ||
| } | ||
|
|
||
| participations[p.UserName] = (info.duration + p.Duration, first); | ||
| } | ||
| else | ||
| { | ||
| participations[p.UserName] = (p.Duration, p.FirstStart); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var p in group.SelectMany(g => g.Participations)) | ||
| { | ||
| if (string.IsNullOrEmpty(p.UserName)) continue; | ||
|
|
||
| if (participations.TryGetValue(p.UserName, out var info)) | ||
| { | ||
| var first = info.firstStart; | ||
| if (p.FirstStart.HasValue && | ||
| (!first.HasValue || p.FirstStart.Value < first.Value)) | ||
| { | ||
| first = p.FirstStart; | ||
| } | ||
|
|
||
| participations[p.UserName] = (info.duration + p.Duration, first); | ||
| } | ||
| else | ||
| { | ||
| participations[p.UserName] = (p.Duration, p.FirstStart); | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| if (t.Second >= 30) | ||
| { | ||
| // round UP | ||
| return new DateTimeOffset(t.Year, t.Month, t.Day, t.Hour, t.Minute, 0, t.Offset) | ||
| .AddMinutes(1); | ||
| } | ||
| else | ||
| { | ||
| // round DOWN | ||
| return new DateTimeOffset(t.Year, t.Month, t.Day, t.Hour, t.Minute, 0, t.Offset); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement return - consider using '?' to express intent better.
| if (t.Second >= 30) | |
| { | |
| // round UP | |
| return new DateTimeOffset(t.Year, t.Month, t.Day, t.Hour, t.Minute, 0, t.Offset) | |
| .AddMinutes(1); | |
| } | |
| else | |
| { | |
| // round DOWN | |
| return new DateTimeOffset(t.Year, t.Month, t.Day, t.Hour, t.Minute, 0, t.Offset); | |
| } | |
| // round UP if seconds >= 30, else round DOWN | |
| return t.Second >= 30 | |
| ? new DateTimeOffset(t.Year, t.Month, t.Day, t.Hour, t.Minute, 0, t.Offset).AddMinutes(1) | |
| : new DateTimeOffset(t.Year, t.Month, t.Day, t.Hour, t.Minute, 0, t.Offset); |
1 participant
SupportManager – Daily report PageModel
This PR contains:
No changes were made to existing Monthly report behavior.
Project is functionally complete and ready for review.