Skip to content

fix: Enhance validation in controllers and filters to handle missing … - #1201

Merged
gibahjoe merged 3 commits into
mainfrom
2584-sentry-template-render-error
Jun 4, 2026
Merged

fix: Enhance validation in controllers and filters to handle missing …#1201
gibahjoe merged 3 commits into
mainfrom
2584-sentry-template-render-error

Conversation

@gibahjoe

@gibahjoe gibahjoe commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes a submit journey crash where datasetSlugToReadableName could be called with a missing dataset, causing Cannot read properties of undefined (reading 'charAt').

The submit journey now redirects users back to /check/url when required session data is missing, and the shared page controller avoids formatting a dataset name unless a dataset exists. The dataset name filter also has a defensive fallback for missing slugs.

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Run the focused unit tests:

npx vitest run test/unit/PageController.test.js test/unit/lpaDetailsController.test.js test/unit/checkAnswersController.test.js test/unit/makeDatasetSlugToReadableNameFilter.test.js

Expected result: all tests pass.

Manual QA:

  • Start from a valid check URL result and continue into the submit journey.
  • Confirm /submit/lpa-details, /submit/dataset-details, and /submit/check-answers still render correctly when session data is valid.
  • Try visiting /submit/dataset-details or /submit/check-answers without a valid submit session.
  • Confirm the user is redirected to /check/url instead of seeing a server error.

Before

The check answers page could throw:

TypeError: Cannot read properties of undefined (reading 'charAt')

when values['dataset'] was missing.

After

Missing required submit journey data redirects the user to /check/url, and the dataset name formatter is not called with a missing dataset.

Added/updated tests?

  • Yes
  • No, and this is why:
  • I need help with writing tests

QA sign off

  • Code has been checked and approved
  • Design has been checked and approved
  • Product and business logic has been checked and proved

[optional] Are there any post-deployment tasks we need to perform?

None.

[optional] Are there any dependencies on other PRs or Work?

None.

Summary by CodeRabbit

  • Bug Fixes

    • Validation now requires both request identifier and dataset; missing values redirect to the check URL.
    • Safer handling when dataset information is absent, avoiding unnecessary lookups and errors.
    • Dataset-derived display names are only computed when dataset data exists, with graceful fallback on failure.
  • Tests

    • Expanded unit tests covering missing/invalid dataset scenarios and related redirect behaviour.

@gibahjoe gibahjoe linked an issue Jun 3, 2026 that may be closed by this pull request
@coderabbitai

coderabbitai Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8b36b75b-04f1-4538-9581-bc36b74407c7

📥 Commits

Reviewing files that changed from the base of the PR and between 79385ad and 3d71e48.

📒 Files selected for processing (2)
  • src/controllers/lpaDetailsController.js
  • test/unit/lpaDetailsController.test.js
💤 Files with no reviewable changes (1)
  • test/unit/lpaDetailsController.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/controllers/lpaDetailsController.js

Walkthrough

Controllers and filters now consistently validate that dataset exists in the session before proceeding. LpaDetailsController tightens params validation and uses a local requestId for lastPage. PageController and the dataset-slug filter avoid downstream lookups when dataset or slug is missing.

Changes

Dataset presence validation and session resilience

Layer / File(s) Summary
Dataset validation in request-handling controllers
src/controllers/CheckAnswersController.js, src/controllers/datasetDetailsController.js
CheckAnswersController and DatasetDetailsController now expand their redirect guards to require both requestId and dataset to be present in the session before proceeding; if either is missing, they redirect to /check/url.
LpaDetailsController session fallback and enhanced validation
src/controllers/lpaDetailsController.js
LpaDetailsController derives params with optional chaining, requires type === 'check_url' and a truthy dataset, and interpolates the local requestId variable into req.form.options.lastPage instead of reading req.session.checkRequestId.
Downstream dataset handling in filters and views
src/controllers/pageController.js, src/filters/makeDatasetSlugToReadableNameFilter.js
PageController only derives req.form.options.datasetName when a session dataset exists; the makeDatasetSlugToReadableNameFilter guard returns falsy slugs unchanged and logs a debug message instead of performing a mapping lookup.
Test coverage for dataset validation and fallback logic
test/unit/checkAnswersController.test.js, test/unit/lpaDetailsController.test.js, test/unit/PageController.test.js, test/unit/makeDatasetSlugToReadableNameFilter.test.js
Unit tests added to verify redirect behaviour when dataset is missing, LpaDetailsController params/dataset guard and lastPage assignment, PageController missing-dataset handling, and safe handling of falsy slug inputs in the filter.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

Suggested labels

bug

Suggested reviewers

  • rosado
  • DilwoarH
  • p3dr0migue1

Poem

A rabbit hops through guards so neat,
Checks each session for the dataset seat,
When slugs are missing, it stays calm and true,
Names left alone, no lookup to do,
Hooray — the form proceeds anew. 🐰

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The changes address the bug fix mentioned in the PR objectives (handling missing dataset to prevent TypeError), but the linked issue #258 is about page content and copy improvements, not validation fixes. Verify that the correct issue is linked. The PR appears to fix a bug (missing dataset validation) which may be a separate issue from #258's content/copy objectives.
Title check ❓ Inconclusive The PR title is incomplete and truncated, making it unclear what specific validation enhancements are being addressed. Complete the PR title to clearly specify what is being validated (e.g., 'fix: Enhance validation in controllers and filters to handle missing dataset values').
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Out of Scope Changes check ✅ Passed All changes are focused on adding validation to handle missing dataset values across controllers and filters, with corresponding unit tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 2584-sentry-template-render-error

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 66.54% 7222 / 10852
🔵 Statements 66.54% 7222 / 10852
🔵 Functions 63.93% 296 / 463
🔵 Branches 77.59% 1001 / 1290
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
src/controllers/CheckAnswersController.js 87.5% 70.37% 100% 87.5% 17-19, 23-29, 153-159, 166, 183-188
src/controllers/datasetDetailsController.js 0% 0% 0% 0% 1-28
src/controllers/lpaDetailsController.js 95.23% 90% 100% 95.23% 24-25
src/controllers/pageController.js 87.87% 86.36% 80% 87.87% 20-21, 46-50, 75-79
src/filters/makeDatasetSlugToReadableNameFilter.js 100% 100% 100% 100%
Generated in workflow #1489 for commit 3d71e48 by the Vitest Coverage Report Action

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/controllers/lpaDetailsController.js`:
- Line 8: The code in lpaDetailsController sets requestId from
req.session?.checkRequestId but falls back only to
req.sessionModel.get('request_id'), while other controllers store/read the
camelCase key; update the fallback logic in lpaDetailsController so requestId is
resolved by checking req.session?.checkRequestId first, then
req.sessionModel.get('requestId') and finally req.sessionModel.get('request_id')
to cover both stored formats; locate the assignment to requestId in
lpaDetailsController and adjust the fallback order accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8fcdaccc-3937-41f4-8f4c-8b80ebc8a8da

📥 Commits

Reviewing files that changed from the base of the PR and between 1f339c3 and 79385ad.

📒 Files selected for processing (9)
  • src/controllers/CheckAnswersController.js
  • src/controllers/datasetDetailsController.js
  • src/controllers/lpaDetailsController.js
  • src/controllers/pageController.js
  • src/filters/makeDatasetSlugToReadableNameFilter.js
  • test/unit/PageController.test.js
  • test/unit/checkAnswersController.test.js
  • test/unit/lpaDetailsController.test.js
  • test/unit/makeDatasetSlugToReadableNameFilter.test.js

Comment thread src/controllers/lpaDetailsController.js Outdated
@gibahjoe
gibahjoe merged commit c1d0b66 into main Jun 4, 2026
5 checks passed
@gibahjoe
gibahjoe deleted the 2584-sentry-template-render-error branch June 4, 2026 13:06
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.

Sentry Template render error Spike: Options/proposal for the content of the page

2 participants