Conversation
- Introduced CreateDashboardInput and UpdateDashboardInput classes for dashboard creation and updates. - Added UpdateWidgetInput class to facilitate widget updates. - Implemented DashboardMutations for creating, updating, and deleting dashboards. - Created DashboardWidgetMutations for adding, updating, and removing widgets from dashboards. - Developed DashboardType and DashboardWidgetType object types for GraphQL responses. - Implemented DashboardQueries to retrieve dashboards and their associated widgets. - Updated BankTrackerDbContext to include DbSet for Dashboards and DashboardWidgets. - Added migrations to create Dashboards and DashboardWidgets tables with appropriate relationships. - Enhanced BankImportTemplate and ImportResultType to use array initialization for dictionaries. - Created Dashboard and DashboardWidget models with necessary properties and relationships.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
frontend/src/app/widgets/widget-net-graph/widget-net-graph.component.spec.ts
Fixed
Show fixed
Hide fixed
frontend/src/app/widgets/widget-net-graph/widget-net-graph.component.spec.ts
Fixed
Show fixed
Hide fixed
…, function or class Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: PhantomDave <34485699+PhantomDave@users.noreply.github.com>
| public void GetAccountIdFromContext_WithNullHttpContext_ThrowsGraphQLException() | ||
| { | ||
| // Arrange | ||
| _mockHttpContextAccessor.Setup(a => a.HttpContext).Returns((HttpContext?)null); |
Check warning
Code scanning / CodeQL
Useless upcast Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the problem, simply remove the redundant explicit cast (HttpContext?) from the line where null is being returned or passed. In this precise case, within file PhantomDave.BankTracking.UnitTests/HttpContextExtensionsTests.cs on line 65, the call:
_mockHttpContextAccessor.Setup(a => a.HttpContext).Returns((HttpContext?)null);should be changed to:
_mockHttpContextAccessor.Setup(a => a.HttpContext).Returns((HttpContext)null);or — even better — since the method expects a nullable reference, it is perfectly correct and idiomatic to simply write:
_mockHttpContextAccessor.Setup(a => a.HttpContext).Returns(null);no cast needed. No other code changes, imports, or definitions are needed.
| @@ -62,7 +62,7 @@ | ||
| public void GetAccountIdFromContext_WithNullHttpContext_ThrowsGraphQLException() | ||
| { | ||
| // Arrange | ||
| _mockHttpContextAccessor.Setup(a => a.HttpContext).Returns((HttpContext?)null); | ||
| _mockHttpContextAccessor.Setup(a => a.HttpContext).Returns(null); | ||
|
|
||
| // Act & Assert | ||
| var exception = Assert.Throws<GraphQLException>(() => |
| public void GetAccountIdFromContext_WithNullUser_ThrowsGraphQLException() | ||
| { | ||
| // Arrange | ||
| _mockHttpContext.Setup(c => c.User).Returns((ClaimsPrincipal)null!); |
Check warning
Code scanning / CodeQL
Useless upcast Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To fix the issue, simply remove the explicit upcast (ClaimsPrincipal)null! to just null! (or even null if suppression of nullable reference warnings isn't needed) in the .Returns call on line 78. This change does not alter runtime behavior and the intent remains clear—configuring the mock to return null for a property of type ClaimsPrincipal. Only the unnecessary explicit cast is removed; no other code changes are required.
| @@ -75,7 +75,7 @@ | ||
| public void GetAccountIdFromContext_WithNullUser_ThrowsGraphQLException() | ||
| { | ||
| // Arrange | ||
| _mockHttpContext.Setup(c => c.User).Returns((ClaimsPrincipal)null!); | ||
| _mockHttpContext.Setup(c => c.User).Returns(null!); | ||
| _mockHttpContextAccessor.Setup(a => a.HttpContext).Returns(_mockHttpContext.Object); | ||
|
|
||
| // Act & Assert |
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive dashboard functionality to the BankTracker application, including CRUD operations for dashboards and widgets on both backend and frontend, with full unit test coverage.
Purpose: Enable users to create customizable dashboards with draggable/resizable widgets to visualize financial data (current balance, net income graphs).
Key Changes:
- Backend: Added Dashboard and DashboardWidget entities with GraphQL queries/mutations, service layer, EF Core migrations, and 19 comprehensive unit tests
- Frontend: Refactored widget components to use signals and shared service state, integrated angular-gridster2 for drag-and-drop dashboard layout, added NetGraph widget for yearly trends
- Architecture: Widgets now fetch their own data independently rather than receiving it via inputs
Reviewed changes
Copilot reviewed 45 out of 48 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| PhantomDave.BankTracking.Library/Models/Dashboard.cs | New Dashboard entity with AccountId and widget collection |
| PhantomDave.BankTracking.Library/Models/DashboardWidget.cs | New DashboardWidget entity with position/size and WidgetType enum |
| PhantomDave.BankTracking.Data/Context/BankTrackerDbContext.cs | EF Core configuration for Dashboard and DashboardWidget entities |
| PhantomDave.BankTracking.Data/Migrations/* | Database migrations for dashboard tables |
| PhantomDave.BankTracking.Data/UnitOfWork/IUnitOfWork.cs | Added Dashboard and DashboardWidget repositories |
| PhantomDave.BankTracking.Api/Services/DashboardService.cs | Service layer with validation, normalization, and CRUD logic for dashboards/widgets |
| PhantomDave.BankTracking.Api/Types/Queries/DashboardQueries.cs | GraphQL queries with authorization for fetching dashboards |
| PhantomDave.BankTracking.Api/Types/Mutations/Dashboard*.cs | GraphQL mutations for dashboard and widget CRUD (missing [Authorize] attributes) |
| PhantomDave.BankTracking.Api/Types/ObjectTypes/Dashboard*.cs | GraphQL types for dashboard responses |
| PhantomDave.BankTracking.Api/Types/Inputs/*.cs | Input types for dashboard/widget mutations |
| PhantomDave.BankTracking.UnitTests/Services/DashboardServiceTests.cs | 19 comprehensive unit tests covering validation, normalization, and edge cases |
| frontend/src/app/widgets/widget-remaining/* | Refactored to inject FinanceRecordService and fetch own data |
| frontend/src/app/widgets/widget-net-graph/* | New widget displaying yearly net balance trend chart |
| frontend/src/app/widgets/widget-wrapper/* | Added edit mode support with drag handle |
| frontend/src/app/components/dashboard-component/* | Dashboard with gridster integration and edit mode toggle |
| frontend/src/app/models/dashboards/gridster-item.ts | Widget interface extending GridsterItem with WidgetType enum |
| TESTING.md | Documentation of 19 new DashboardService unit tests |
Files not reviewed (2)
- PhantomDave.BankTracking.Data/Migrations/20251124092813_Added Dashboard and Widgets.Designer.cs: Language not supported
- PhantomDave.BankTracking.Data/Migrations/20251124094829_AddAccountIdToDashboard.Designer.cs: Language not supported
| var dashboard = new Dashboard | ||
| { | ||
| AccountId = accountId, | ||
| Name = input.Name?.Trim() ?? string.Empty | ||
| }; |
There was a problem hiding this comment.
Missing name truncation: The CreateDashboard mutation doesn't enforce the 100-character name length limit that's defined in the database configuration (ConfigureDashboard in BankTrackerDbContext.cs line 115) and enforced in DashboardService.NormalizeName. Consider truncating the name to 100 characters before saving to prevent potential database errors.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
check also the security warning from codeQL
| { | ||
| dashboard.Name = input.Name.Trim(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing UpdateAsync call: The mutation modifies the dashboard entity but only calls SaveChangesAsync() without calling unitOfWork.Dashboards.UpdateAsync(dashboard). While EF Core change tracking may handle this, it's inconsistent with the pattern used in DashboardService.UpdateDashboardAsync (line 86) and other mutations. Consider calling UpdateAsync for consistency and explicit intent.
| await unitOfWork.Dashboards.UpdateAsync(dashboard); |
| public async Task<DashboardType> UpdateDashboard( | ||
| [Service] IUnitOfWork unitOfWork, | ||
| [Service] IHttpContextAccessor httpContextAccessor, | ||
| UpdateDashboardInput input) | ||
| { | ||
| var accountId = httpContextAccessor.GetAccountIdFromContext(); | ||
|
|
||
| var dashboard = await unitOfWork.Dashboards.GetByIdAsync(input.Id); | ||
| if (dashboard == null || dashboard.AccountId != accountId) | ||
| { | ||
| throw new GraphQLException( | ||
| ErrorBuilder.New() | ||
| .SetMessage("Dashboard not found.") | ||
| .SetCode("NOT_FOUND") | ||
| .Build()); | ||
| } | ||
|
|
||
| if (input.Name != null) | ||
| { | ||
| dashboard.Name = input.Name.Trim(); | ||
| } | ||
|
|
||
| await unitOfWork.SaveChangesAsync(); | ||
|
|
||
| return DashboardType.FromDashboard(dashboard); | ||
| } |
There was a problem hiding this comment.
Missing [Authorize] attribute: This mutation allows updating dashboards without authentication. Add the [Authorize] attribute to ensure only authenticated users can update dashboards, consistent with the query patterns in DashboardQueries.cs.
| var dashboard = new Dashboard | ||
| { | ||
| AccountId = accountId, | ||
| Name = input.Name?.Trim() ?? string.Empty |
There was a problem hiding this comment.
Missing input validation: The CreateDashboard mutation allows empty dashboard names (after trimming whitespace becomes an empty string). According to DashboardService.CreateDashboardAsync, this should return null, but here it proceeds to create the dashboard. Consider validating that the trimmed name is not empty and throw a BAD_USER_INPUT error if it is, consistent with other mutations.
| var dashboard = new Dashboard | |
| { | |
| AccountId = accountId, | |
| Name = input.Name?.Trim() ?? string.Empty | |
| var trimmedName = input.Name?.Trim() ?? string.Empty; | |
| if (string.IsNullOrEmpty(trimmedName)) | |
| { | |
| throw new GraphQLException( | |
| ErrorBuilder.New() | |
| .SetMessage("Dashboard name cannot be empty.") | |
| .SetCode("BAD_USER_INPUT") | |
| .Build()); | |
| } | |
| var dashboard = new Dashboard | |
| { | |
| AccountId = accountId, | |
| Name = trimmedName |
|
|
||
| if (input.Name != null) | ||
| { | ||
| dashboard.Name = input.Name.Trim(); |
There was a problem hiding this comment.
Missing name truncation: The UpdateDashboard mutation doesn't truncate the name to 100 characters like DashboardService.NormalizeName does. Consider truncating the name before saving to maintain consistency with the service layer and prevent potential database errors.
| dashboard.Name = input.Name.Trim(); | |
| dashboard.Name = input.Name.Trim().Length > 100 | |
| ? input.Name.Trim().Substring(0, 100) | |
| : input.Name.Trim(); |
| public async Task<bool> DeleteDashboard( | ||
| [Service] IUnitOfWork unitOfWork, | ||
| [Service] IHttpContextAccessor httpContextAccessor, | ||
| int id) | ||
| { | ||
| var accountId = httpContextAccessor.GetAccountIdFromContext(); | ||
|
|
||
| var dashboard = await unitOfWork.Dashboards.GetByIdAsync(id); | ||
| if (dashboard == null || dashboard.AccountId != accountId) | ||
| { | ||
| throw new GraphQLException( | ||
| ErrorBuilder.New() | ||
| .SetMessage("Dashboard not found.") | ||
| .SetCode("NOT_FOUND") | ||
| .Build()); | ||
| } | ||
|
|
||
| await unitOfWork.Dashboards.DeleteAsync(id); | ||
| await unitOfWork.SaveChangesAsync(); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Missing [Authorize] attribute: This mutation allows deleting dashboards without authentication. Add the [Authorize] attribute to ensure only authenticated users can delete dashboards, consistent with the query patterns in DashboardQueries.cs.
| public async Task<DashboardWidgetType> AddWidget( | ||
| [Service] IUnitOfWork unitOfWork, | ||
| [Service] IHttpContextAccessor httpContextAccessor, | ||
| AddWidgetInput input) | ||
| { | ||
| var accountId = httpContextAccessor.GetAccountIdFromContext(); | ||
|
|
||
| var dashboard = await unitOfWork.Dashboards.GetByIdAsync(input.DashboardId); | ||
| if (dashboard is null || dashboard.AccountId != accountId) | ||
| { | ||
| throw new GraphQLException( | ||
| ErrorBuilder.New() | ||
| .SetMessage("Dashboard not found.") | ||
| .SetCode("NOT_FOUND") | ||
| .Build()); | ||
| } | ||
|
|
||
| if (input.Rows <= 0 || input.Cols <= 0) | ||
| { | ||
| throw new GraphQLException( | ||
| ErrorBuilder.New() | ||
| .SetMessage("Widget rows and cols must be greater than 0.") | ||
| .SetCode("BAD_USER_INPUT") | ||
| .Build()); | ||
| } | ||
|
|
||
| var widget = new DashboardWidget | ||
| { | ||
| DashboardId = input.DashboardId, | ||
| Type = input.Type, | ||
| X = Math.Max(0, input.X), | ||
| Y = Math.Max(0, input.Y), | ||
| Rows = input.Rows, | ||
| Cols = input.Cols | ||
| }; | ||
|
|
||
| await unitOfWork.DashboardWidgets.AddAsync(widget); | ||
| await unitOfWork.SaveChangesAsync(); | ||
|
|
||
| return DashboardWidgetType.FromDashboardWidget(widget); | ||
| } |
There was a problem hiding this comment.
Missing [Authorize] attribute: This mutation allows adding widgets without authentication. Add the [Authorize] attribute to ensure only authenticated users can add widgets, consistent with the query patterns in DashboardQueries.cs.
| public async Task<DashboardWidgetType> UpdateWidget( | ||
| [Service] IUnitOfWork unitOfWork, | ||
| [Service] IHttpContextAccessor httpContextAccessor, | ||
| UpdateWidgetInput input) | ||
| { | ||
| var accountId = httpContextAccessor.GetAccountIdFromContext(); | ||
|
|
||
| var widget = await unitOfWork.DashboardWidgets.GetByIdAsync(input.Id); | ||
| if (widget is null) | ||
| { | ||
| throw new GraphQLException( | ||
| ErrorBuilder.New() | ||
| .SetMessage("Widget not found.") | ||
| .SetCode("NOT_FOUND") | ||
| .Build()); | ||
| } | ||
|
|
||
| var dashboard = await unitOfWork.Dashboards.GetByIdAsync(widget.DashboardId); | ||
| if (dashboard is null || dashboard.AccountId != accountId) | ||
| { | ||
| throw new GraphQLException( | ||
| ErrorBuilder.New() | ||
| .SetMessage("Widget not found.") | ||
| .SetCode("NOT_FOUND") | ||
| .Build()); | ||
| } | ||
|
|
||
| if (input.Type.HasValue) | ||
| { | ||
| widget.Type = input.Type.Value; | ||
| } | ||
|
|
||
| if (input.X.HasValue) | ||
| { | ||
| widget.X = Math.Max(0, input.X.Value); | ||
| } | ||
|
|
||
| if (input.Y.HasValue) | ||
| { | ||
| widget.Y = Math.Max(0, input.Y.Value); | ||
| } | ||
|
|
||
| if (input.Rows.HasValue) | ||
| { | ||
| if (input.Rows.Value <= 0) | ||
| { | ||
| throw new GraphQLException( | ||
| ErrorBuilder.New() | ||
| .SetMessage("Widget rows must be greater than 0.") | ||
| .SetCode("BAD_USER_INPUT") | ||
| .Build()); | ||
| } | ||
|
|
||
| widget.Rows = input.Rows.Value; | ||
| } | ||
|
|
||
| if (input.Cols.HasValue) | ||
| { | ||
| if (input.Cols.Value <= 0) | ||
| { | ||
| throw new GraphQLException( | ||
| ErrorBuilder.New() | ||
| .SetMessage("Widget cols must be greater than 0.") | ||
| .SetCode("BAD_USER_INPUT") | ||
| .Build()); | ||
| } | ||
|
|
||
| widget.Cols = input.Cols.Value; | ||
| } | ||
|
|
||
| await unitOfWork.DashboardWidgets.UpdateAsync(widget); | ||
| await unitOfWork.SaveChangesAsync(); | ||
|
|
||
| return DashboardWidgetType.FromDashboardWidget(widget); | ||
| } |
There was a problem hiding this comment.
Missing [Authorize] attribute: This mutation allows updating widgets without authentication. Add the [Authorize] attribute to ensure only authenticated users can update widgets, consistent with the query patterns in DashboardQueries.cs.
| public async Task<bool> RemoveWidget( | ||
| [Service] IUnitOfWork unitOfWork, | ||
| [Service] IHttpContextAccessor httpContextAccessor, | ||
| int id) | ||
| { | ||
| var accountId = httpContextAccessor.GetAccountIdFromContext(); | ||
|
|
||
| var widget = await unitOfWork.DashboardWidgets.GetByIdAsync(id); | ||
| if (widget is null) | ||
| { | ||
| throw new GraphQLException( | ||
| ErrorBuilder.New() | ||
| .SetMessage("Widget not found.") | ||
| .SetCode("NOT_FOUND") | ||
| .Build()); | ||
| } | ||
|
|
||
| var dashboard = await unitOfWork.Dashboards.GetByIdAsync(widget.DashboardId); | ||
| if (dashboard is null || dashboard.AccountId != accountId) | ||
| { | ||
| throw new GraphQLException( | ||
| ErrorBuilder.New() | ||
| .SetMessage("Widget not found.") | ||
| .SetCode("NOT_FOUND") | ||
| .Build()); | ||
| } | ||
|
|
||
| await unitOfWork.DashboardWidgets.DeleteAsync(id); | ||
| await unitOfWork.SaveChangesAsync(); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Missing [Authorize] attribute: This mutation allows removing widgets without authentication. Add the [Authorize] attribute to ensure only authenticated users can remove widgets, consistent with the query patterns in DashboardQueries.cs.
| @@ -0,0 +1,27 @@ | |||
| /* tslint:disable:no-unused-variable */ | |||
| import { async, ComponentFixture, TestBed } from '@angular/core/testing'; | |||
| import { DebugElement } from '@angular/core'; | |||
There was a problem hiding this comment.
Unused import DebugElement.
|
@PhantomDave I've opened a new pull request, #92, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: PhantomDave <34485699+PhantomDave@users.noreply.github.com>
|
@copilot open a new pull request to apply changes based on the comments in this thread Also implement everything under the codeQL warning |
|
@PhantomDave I've opened a new pull request, #93, to work on those changes. Once the pull request is ready, I'll request review from you. |
… APIs (#93) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: PhantomDave <34485699+PhantomDave@users.noreply.github.com>
No description provided.