Skip to content
This repository was archived by the owner on Nov 22, 2025. It is now read-only.

Comments

feat: 2025-11-21 improvements#6

Merged
Pertempto merged 6 commits intomainfrom
2025-11-21-improvements
Nov 22, 2025
Merged

feat: 2025-11-21 improvements#6
Pertempto merged 6 commits intomainfrom
2025-11-21-improvements

Conversation

@Pertempto
Copy link
Contributor

No description provided.

@github-actions
Copy link

backend/internal/server/invoices.go — rows.Scan uses concrete types (string, float64) which will panic or error if DB returns NULLs or different types. Please use sql.NullString / sql.NullFloat64 (or validate/COALESCE in SQL) and map them to the response struct, e.g. handle NULL Sales Rep or Tran Subtotal values. Also confirm the actual DB column types for "Sales Rep" and "Tran Subtotal" and adjust scanning types accordingly.

@github-actions
Copy link

backend/internal/database/database.go — The connection strings now include encrypt=%s. For SQL Server older versions, allowed values are disable, true, false. Ensure DB_ENCRYPT env var is validated/limited to these values. Suggest adding validation in config.Load() to return an error if an invalid value is supplied (to avoid runtime connection errors).

@github-actions
Copy link

Changes Requested

  • Validate DB_ENCRYPT in config.Load() to ensure it is one of "disable", "true", or "false" (fail fast with a clear error).
  • Make DB row scanning robust to NULLs/types in backend/internal/server/invoices.go and backend/internal/server/employees.go (use sql.NullString, sql.NullFloat64 or COALESCE in SQL).
  • Confirm Employees filter change from DateReleased IS NULL to inactive = 0 is intentional and consistent with Aptora schema.

Summary of Changes

  • Added DB_ENCRYPT environment variable to /.env.example and integrated it into the configuration and DB connection string.
  • Updated .gitignore to ignore .env.* files but keep .env.example tracked.
  • Propagated DBEncrypt setting from config through main into the database manager and included encrypt=%s in SQL Server connection strings.
  • Split handleEmployees and handleInvoices out of server.go into separate handler files and refactored invoice handling to use the aptCDV_VW_APT_InvSalCredEstList view with new column names and pagination/limit checks.
  • Updated frontend InvoicesPage.tsx to use number/subtotal/employee query parameter names matching backend changes.

Overall Feedback

  • Negative: There are a few risky assumptions that may cause runtime errors or incorrect data: unvalidated DB_ENCRYPT values, and direct scanning into concrete Go types from DB columns that may be NULL or have different types. The Employees SQL filter was changed in a way that could alter results if the inactive column doesn't exist or behaves differently.

  • Positive: The refactor cleanly extracts handlers into separate files, adds safeguards (invoice count limit), and wires TLS/ENCRYPT handling through configuration consistently. Frontend updates align with backend field name changes. Nice work simplifying and organizing handlers! 🎉

If you want, I can provide patch suggestions for the sql.Null* scan changes and DB_ENCRYPT validation.

@Pertempto Pertempto merged commit df6633e into main Nov 22, 2025
2 of 3 checks passed
@Pertempto Pertempto deleted the 2025-11-21-improvements branch November 22, 2025 14:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant