Skip to content

ILLDEV-466 Use template record for PDF pullslip - #731

Open
JanisSaldabols wants to merge 8 commits into
mainfrom
ILLDEV-466
Open

ILLDEV-466 Use template record for PDF pullslip#731
JanisSaldabols wants to merge 8 commits into
mainfrom
ILLDEV-466

Conversation

@JanisSaldabols

Copy link
Copy Markdown
Collaborator

No description provided.

Copilot AI balanced review requested due to automatic review settings August 25, 2026 14:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Moves PDF pull-slip rendering from an embedded file to tenant-configurable template records.

Changes:

  • Adds a default PDF template and state-model label.
  • Resolves and renders tenant templates for PDFs and HTML emails.
  • Relocates rendering/data helpers into the email package with tests.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
misc/state-models.yaml Adds the PDF template default and label.
broker/test/pullslip/api/api_handler_test.go Seeds the template in integration tests.
broker/scheduler/service/email_sender.go Uses shared batch-email helpers.
broker/pullslip/service/pull_slip_template.html Removes the embedded template.
broker/pullslip/service/pdf.go Loads templates from tenant records.
broker/pullslip/service/pdf_test.go Tests record-based PDF templates.
broker/pullslip/api/api_handler_test.go Extends the repository mock.
broker/patron_request/service/statemodel_test.go Tests template defaults and labels.
broker/patron_request/service/action.go Renders HTML notification templates.
broker/patron_request/service/action_test.go Updates email helper tests.
broker/patron_request/service/action_mapping.go Resolves state models from requests.
broker/patron_request/api/api-handler_test.go Updates expected template defaults.
broker/oapi/open-api.yaml Adds the state-model template-label property.
broker/email/email.go Adds shared template data and rendering helpers.
broker/email/email_test.go Tests shared rendering and data extraction.
Suppressed comments (1)

broker/pullslip/service/pull_slip_template.html:1

  • Deleting this file leaves PULLSLIP_TEMPLATE=pullslip/service/pull_slip_template.html as a prerequisite of both the binary and archive targets (broker/Makefile:25,103,106). Because the prerequisite no longer exists and has no generation rule, those make targets fail with “No rule to make target” before Go compilation. Remove the stale Makefile variable/prerequisites as part of this deletion.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread broker/pullslip/service/pdf.go
Comment thread broker/oapi/open-api.yaml Outdated
@jakub-id

Copy link
Copy Markdown
Contributor

Since this PR introduces placeholder rendering for notification templates, it should support both HTML and plain-text templates consistently. Please use the same data model and placeholder syntax for both, with html/template for HTML and text/template for plain text.

The OpenAPI documentation should also be updated to show the actual syntax, such as {{.Title}} and {{.PatronName}}, and list the supported placeholders. The existing default notification templates do not use placeholders, so compatibility with their current syntax is not a concern.

@jakub-id

Copy link
Copy Markdown
Contributor

This PR should not break pull-slip generation for tenants without a persisted template. When no owner-specific template exists, please fall back to the matching built-in state-model default; persisted templates should remain overrides. This also avoids requiring migrations or provisioning for every existing and future tenant.

@jakub-id jakub-id left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

Suppressed comments (5)

broker/pullslip/service/pdf.go:118

  • Add a test where the repository returns pgx.ErrNoRows and assert that the built-in template is rendered. The current mock returns the built-in template directly, so this new fallback branch—the availability path when an owner has no template record—is not exercised.
	if err != nil {
		if errors.Is(err, pgx.ErrNoRows) {
			return getDefaultTemplate(*stateModel.PullslipPdfTemplateLabel)
		}

broker/email/email.go:217

  • This exported type needs a GoDoc-compatible comment beginning with BatchEmailData, as required for exported structs.
// Only string fields are allowed
type BatchEmailData struct {

broker/email/email.go:295

  • Add a GoDoc-compatible comment for this newly exported function.
func RenderHtmlTemplate(data any, templateBody string) (string, error) {

broker/email/email.go:384

  • Add a GoDoc-compatible comment for this newly exported function.
func GetBatchEmailData(fullCount int64, actualCount int, batchQuery string) BatchEmailData {

broker/email/email.go:392

  • Add a GoDoc-compatible comment for this newly exported function.
func RenderTextTemplate(data any, template string) string {

Comment thread broker/scheduler/service/email_sender.go Outdated
Comment thread broker/email/email.go
Comment thread broker/email/email.go
Comment thread broker/patron_request/service/action_mapping.go
Comment thread broker/oapi/open-api.yaml
@jakub-id

Copy link
Copy Markdown
Contributor

This changes the existing batch placeholders from {{batchQuery}}, {{actualCount}}, and {{fullCount}} to the new Go-template syntax. Persisted batch templates will not be migrated, so please normalize these legacy placeholders to their new equivalents before rendering and add a compatibility test.

@jakub-id

Copy link
Copy Markdown
Contributor

RenderTextTemplate only performs exact string replacements, while HTML uses Go templates. Please use text/template for text bodies and subjects so both formats support the same placeholder syntax and template behavior, including whitespace variants and proper errors for invalid templates.

Comment thread broker/email/email.go Outdated
Comment thread broker/email/email.go
-- Replace placeholders in template.subject and template.body
-- Old: {{batchQuery}}, {{actualCount}}, {{fullCount}}
-- New: {{.BatchQuery}}, {{.ActualCount}}, {{.FullCount}}
UPDATE template

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Was there a reason to migrate the stored templates instead of normalizing the three legacy batch placeholders before rendering? Runtime normalization would preserve compatibility with older clients, rolling deployments, and rollbacks.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Was thinking that one migration is better solution then normalization every time we need to use template

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I tend to agree, just check if this migration is reversible.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There is down script which is reverting migration

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants