Skip to content

Feat/df 702 - dont log for summary page with missing title#276

Merged
jbarnsley10 merged 4 commits intomainfrom
feat/DF-702-no-log
Dec 5, 2025
Merged

Feat/df 702 - dont log for summary page with missing title#276
jbarnsley10 merged 4 commits intomainfrom
feat/DF-702-no-log

Conversation

@jbarnsley10
Copy link
Copy Markdown
Contributor

@jbarnsley10 jbarnsley10 commented Dec 4, 2025

Proposed change

Don't log unnecessarily for Summary pages that have an empty title, since it gets defaulted anyway

Jira ticket: DF-702

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Misc. (documentation, build updates, etc)

Checklist

  • You have executed this code locally and it performs as expected.
  • You have added tests to verify your code works.
  • You have added code comments and JSDoc, where appropriate.
  • There is no commented-out code.
  • You have added developer docs in README.md and docs/* (where appropriate, e.g. new features).
  • The tests are passing (npm run test).
  • The linting checks are passing (npm run lint).
  • The code has been formatted (npm run format).

Comment thread src/server/plugins/engine/helpers.ts Outdated
Comment on lines 418 to 424
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.

Is this something we even need to log? A component must have a title anyway, right?

Copy link
Copy Markdown
Contributor Author

@jbarnsley10 jbarnsley10 Dec 4, 2025

Choose a reason for hiding this comment

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

The Summary pages can have a page title of empty so that they use the default 'Check your answers before sending your form'. Other pages can have an empty title (where it defaults to use the first question on the page).
So it does happen.
The question is ... are we bothered enough to log it?

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.

Sry I approved this PR but missed this convo

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So @alexluckett are you happy for me to remove the logging altogether (lines 418 - 424)?

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.

Sry I approved this PR but missed this convo

np, I always approve even if someone else isn't happy and leave it up to the other reviewer to block the PR.

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 think we can remove it, yeah @jbarnsley10 . Feel free. Debug if we're concerned as that won't show in prod, but it can be removed altogether.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Dec 5, 2025

@jbarnsley10 jbarnsley10 merged commit 8ee8d79 into main Dec 5, 2025
23 checks passed
@jbarnsley10 jbarnsley10 deleted the feat/DF-702-no-log branch December 5, 2025 09:50
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.

3 participants