Skip to content

Feature/pdf export#13

Open
ravindu2012 wants to merge 10 commits intomasterfrom
feature/pdf-export
Open

Feature/pdf export#13
ravindu2012 wants to merge 10 commits intomasterfrom
feature/pdf-export

Conversation

@ravindu2012
Copy link
Copy Markdown
Owner

Summary

Brief description of the changes.

Type of Change

  • Bug fix
  • New feature
  • Improvement/refactor
  • Documentation
  • Other

Related Issue

Fixes #(issue number)

Changes Made

  • Change 1
  • Change 2

Screenshots

If applicable, add screenshots of UI changes.

Checklist

  • I have tested my changes locally
  • My code follows Clean Architecture (dependencies flow inward)
  • ViewModels follow MVVM pattern
  • I have added tests for new business logic

- Cache ICommand properties to avoid allocating on every binding access
- Rename RelayCommand to ActionCommand to avoid conflict with CommunityToolkit.Mvvm
- Fix FocusSearch to match TextBox by binding path (FilterText/SearchText) instead of TabIndex
- Scope search to selected tab content instead of entire TabControl
- Add InputGestureText hints for F5, Ctrl+F, and Esc shortcuts in menus
- Add F1 keybinding and Help > Keyboard Shortcuts menu item
- Show themed dialog listing all shortcuts with their actions
- Covers global shortcuts (F1, F5, Ctrl+N/S/F, Esc) and form-level access keys (Alt+S/P/L/V/R/N/E/D)
implemented full keyboard navigation support
…mock data

- IPdfExportService: use Invoice parameter instead of object
- PdfExportService: static constructor for QuestPDF license, English error message
- IFileDialogService + WpfFileDialogService: extract SaveFileDialog from ViewModel
- InvoiceFormViewModel: remove mock data injection, use IFileDialogService, set Customer nav property
- Revert QBD.Modules.Customers.csproj to net8.0 (remove UseWPF dependency)
Copy link
Copy Markdown
Owner Author

@ravindu2012 ravindu2012 left a comment

Choose a reason for hiding this comment

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

Code Review: PR #13 — PDF Export + API Layer + GPLv3 Headers

This PR bundles 3 separate concerns: PDF export, the full REST API layer, and GPLv3 copyright headers across all source files. Consider splitting into separate PRs for easier review.

What's Good

  • Clean IPdfExportService / IFileDialogService abstractions following Clean Architecture
  • PdfExportService using QuestPDF with proper invoice layout (header, line items table, totals, pagination)
  • File dialog properly extracted from ViewModel via IFileDialogService — good for testability
  • API layer is comprehensive with consistent patterns (CRUD + post/void lifecycle)
  • ReferenceHandler.IgnoreCycles in JSON options prevents serialization loops

Issues

CRITICAL: QuickBooksDesktop.db is committed to the repository

src/Presentation/QBD.API/QuickBooksDesktop.db is a binary SQLite database checked into source control. This should be in .gitignore. It will cause merge conflicts and bloat the repo. Remove it and add *.db to .gitignore.

CRITICAL: API has no authentication or authorization

All 19 controllers are publicly accessible with no [Authorize] attribute, no authentication middleware, and no API key. Endpoints like DELETE /api/accounts/{id}, POST /api/bills/{id}/void, and POST /api/journalentries/{id}/post allow destructive financial operations without any auth. Even for development, add at minimum a TODO comment or [Authorize] placeholder.

HIGH: API controllers accept domain entities directly as [FromBody] parameters

For example:

public async Task<IActionResult> Create([FromBody] Account account)
public async Task<IActionResult> Create([FromBody] Bill bill)

This is an over-posting vulnerability — clients can set properties like IsSystemAccount, Balance, Status, or navigation properties that shouldn't be client-settable. Use DTOs/request models for create/update operations and map to domain entities.

HIGH: No input validation on API endpoints

No [Required], FluentValidation, or manual validation on any endpoint. For example, POST /api/invoices accepts any payload without checking that CustomerId exists, lines are present, or amounts are positive.

MEDIUM: ExportReportToPdfAsync is a stub

page.Content().PaddingVertical(1, Unit.Centimetre).Text("Report data export is currently in development...");

The reportData parameter is typed as object and never used. Either remove this method from the interface until implemented, or at minimum don't expose it as a public API contract.

MEDIUM: GPLv3 headers added to files that aren't changed by this PR

~130 of 159 files changed are only adding the 3-line copyright header. This makes the diff very noisy and should be a separate PR.

MEDIUM: Swagger is enabled unconditionally (including production)

app.UseSwagger();
app.UseSwaggerUI();

This should be wrapped in if (app.Environment.IsDevelopment()).

LOW: QBD.Modules.Customers.csproj changed from net8.0-windows to net8.0

- <TargetFramework>net8.0-windows</TargetFramework>
+ <TargetFramework>net8.0</TargetFramework>

This may break WPF-specific features in this module if it uses any Windows APIs. Verify this doesn't cause runtime issues.

LOW: Invoice PDF hardcodes "Arial" font family

page.DefaultTextStyle(x => x.FontSize(11).FontFamily("Arial"));

Arial may not be available on all systems. QuestPDF defaults are cross-platform safe — consider removing the explicit font or using a fallback chain.

LOW: Missing trailing newlines on new files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant