Feature/475 Fix issue with PDF not generated - #476
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughTwo handler fixes assign parsed/converted JSON to the shared ChangesUpload Data Variable Scoping
PDF Generation E2E Test
CI Workflow Update
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0fc07e9d-f6f8-453a-9c30-34ae548546d8
📒 Files selected for processing (4)
e2e-tests/tests/manual-upload.spec.tse2e-tests/utils/notification-helpers.tslibs/admin-pages/src/pages/manual-upload-summary/index.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.ts
| export async function getLatestArtefactByLocationAndListType(locationId: number, listTypeId: number) { | ||
| return await prisma.artefact.findFirst({ | ||
| where: { | ||
| locationId: locationId.toString(), | ||
| listTypeId | ||
| }, | ||
| orderBy: { lastReceivedDate: "desc" } | ||
| }); |
There was a problem hiding this comment.
Tighten this lookup to the current test run.
Filtering only by locationId and listTypeId is too broad for E2E assertions. A previous or concurrent upload for the same pair can be returned here, which makes the nightly PDF test flaky. Please add a discriminator from the current run, e.g. a createdAfter timestamp captured before submission.
| let jsonData: unknown; | ||
| if (!isFlatFile) { | ||
| try { | ||
| const jsonData = JSON.parse(uploadData.file.toString("utf-8")); | ||
| jsonData = JSON.parse(uploadData.file.toString("utf-8")); | ||
| await extractAndStoreArtefactSearch(artefactId, listTypeId, jsonData); | ||
| } catch (error) { |
There was a problem hiding this comment.
Do not swallow JSON parse failures here.
If JSON.parse() throws, jsonData stays undefined and processPublication() will skip PDF generation, but the request still carries on to the success page. Please fail the upload when the .json payload cannot be parsed, and only treat search extraction as non-fatal.
Suggested split between parse failure and search extraction failure
let jsonData: unknown;
if (!isFlatFile) {
try {
jsonData = JSON.parse(uploadData.file.toString("utf-8"));
- await extractAndStoreArtefactSearch(artefactId, listTypeId, jsonData);
- } catch (error) {
+ } catch {
+ throw new Error("Uploaded JSON could not be parsed");
+ }
+
+ try {
+ await extractAndStoreArtefactSearch(artefactId, listTypeId, jsonData);
+ } catch (error) {
console.error("[Manual Upload] Failed to extract artefact search data", {
artefactId,
error: error instanceof Error ? error.message : String(error)
});
}
🎭 Playwright E2E Test Results83 tests 50 ✅ 5m 4s ⏱️ Results for commit 57e0c94. ♻️ This comment has been updated with latest results. |
…eration-bug # Conflicts: # e2e-tests/tests/manual-upload.spec.ts # libs/admin-pages/src/pages/non-strategic-upload-summary/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
libs/admin-pages/src/pages/manual-upload-summary/index.ts (1)
132-140:⚠️ Potential issue | 🟠 MajorDo not continue when JSON parsing fails.
JSON.parse()andextractAndStoreArtefactSearch()are still in the sametry/catch. A parse failure leavesjsonDataundefined, and the upload can still proceed to success without generating a PDF.Suggested split: fail on parse, log-only on search extraction
let jsonData: unknown; if (!isFlatFile) { try { jsonData = JSON.parse(uploadData.file.toString("utf-8")); - await extractAndStoreArtefactSearch(artefactId, listTypeId, jsonData); - } catch (error) { + } catch { + throw new Error("Uploaded JSON could not be parsed"); + } + + try { + await extractAndStoreArtefactSearch(artefactId, listTypeId, jsonData); + } catch (error) { console.error("[Manual Upload] Failed to extract artefact search data", { artefactId, error: error instanceof Error ? error.message : String(error) }); }e2e-tests/utils/notification-helpers.ts (1)
144-152:⚠️ Potential issue | 🟠 MajorTighten this lookup to the current test run to avoid flaky E2E assertions.
Filtering only by
locationIdandlistTypeIdcan match an older or concurrent upload for the same pair.Suggested shape
-export async function getLatestArtefactByLocationAndListType(locationId: number, listTypeId: number) { +export async function getLatestArtefactByLocationAndListType( + locationId: number, + listTypeId: number, + createdAfter: Date +) { return await prisma.artefact.findFirst({ where: { locationId: locationId.toString(), - listTypeId + listTypeId, + lastReceivedDate: { gte: createdAfter } }, orderBy: { lastReceivedDate: "desc" } }); }
🧹 Nitpick comments (1)
e2e-tests/tests/admin/manual-upload.spec.ts (1)
505-541: Extend this nightly journey to include Welsh and accessibility checks inline.The new test validates PDF generation, but it does not include inline Welsh/a11y assertions required for E2E journey coverage.
As per coding guidelines:
e2e-tests/**/*.spec.ts: E2E tests in Playwright should minimise test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate tests. As per coding guidelines:e2e-tests/**/*.spec.ts: Use AxeBuilder with Playwright to test accessibility inline within journey tests, not as separate tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22e64a8d-bf7c-4f0f-a65f-13b525790d62
📒 Files selected for processing (4)
e2e-tests/tests/admin/manual-upload.spec.tse2e-tests/utils/notification-helpers.tslibs/admin-pages/src/pages/manual-upload-summary/index.tslibs/admin-pages/src/pages/non-strategic-upload-summary/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/admin-pages/src/pages/non-strategic-upload-summary/index.ts
|
This reverts commit fdf2f29.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e-tests/tests/admin/manual-upload.spec.ts (1)
504-537: Add inline accessibility and Welsh checks to keep this as a complete journey test.This new journey test currently skips the inline a11y/Welsh assertions used elsewhere in this spec. Please include them here too so this path remains compliant and self-contained.
Suggested patch
test("should generate a PDF after uploading a JSON publication `@nightly`", async ({ page }) => { @@ await page.getByRole("button", { name: "Confirm" }).click(); await page.waitForURL("/manual-upload-success", { timeout: 30000 }); + const accessibilityScanResults = await new AxeBuilder({ page }) + .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"]) + .analyze(); + expect(accessibilityScanResults.violations).toEqual([]); + + await page.goto("/manual-upload-success?lng=cy"); + await expect(page.getByRole("link", { name: "uwchlwytho ffeil arall" })).toBeVisible(); + const pdfPath = path.join(process.cwd(), "..", "storage", "temp", "uploads", `${artefact!.artefactId}.pdf`); expect(fs.existsSync(pdfPath)).toBe(true); expect(fs.statSync(pdfPath).size).toBeGreaterThan(0); });As per coding guidelines:
E2E tests in Playwright should minimize test count with one test per complete user journey, including validations, Welsh translations, and accessibility checks inline rather than in separate testsandUse AxeBuilder with Playwright to test accessibility inline within journey tests, not as separate tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7d1d3f6-588b-4b0d-9fd5-bf711857e2d8
📒 Files selected for processing (1)
e2e-tests/tests/admin/manual-upload.spec.ts
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e-tests/tests/admin/manual-upload.spec.ts (1)
491-527: ⚡ Quick winMissing inline accessibility scan — required by coding guidelines.
All journey tests should include an
AxeBuilderaccessibility check. The new@nightlytest has noAxeBuildercall.♻️ Suggested addition
await page.waitForURL("/manual-upload-success", { timeout: 30000 }); + const accessibilityScanResults = await new AxeBuilder({ page }) + .withTags(["wcag2a", "wcag2aa", "wcag21a", "wcag21aa", "wcag22aa"]) + .disableRules(["target-size", "link-name"]) + .analyze(); + expect(accessibilityScanResults.violations).toEqual([]); + const artefact = await getLatestArtefactByLocationAndListType(testLocationId, 8);As per coding guidelines: "Use AxeBuilder with Playwright to test accessibility inline within journey tests, not as separate tests."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35f4ab4b-89a3-469a-a549-ed465b46e078
📒 Files selected for processing (2)
e2e-tests/tests/admin/manual-upload.spec.tse2e-tests/utils/test-support-api.ts
| const artefact = await getLatestArtefactByLocationAndListType(testLocationId, 8); | ||
| expect(artefact).toBeDefined(); | ||
|
|
||
| const pdfPath = path.join(process.cwd(), "..", "storage", "temp", "uploads", `${artefact!.artefactId}.pdf`); | ||
| expect(fs.existsSync(pdfPath)).toBe(true); | ||
| expect(fs.statSync(pdfPath).size).toBeGreaterThan(0); |
There was a problem hiding this comment.
toBeDefined() passes for null — the artefact check will not fail cleanly.
getLatestArtefactByLocationAndListType returns null (not undefined) when no match is found. expect(null).toBeDefined() passes in Playwright/Jest because null !== undefined. The test will therefore proceed to line 524 where the runtime encounters null.artefactId and throws a TypeError rather than a clean assertion failure.
Use not.toBeNull() (or toBeTruthy()) instead.
🐛 Proposed fix
const artefact = await getLatestArtefactByLocationAndListType(testLocationId, 8);
- expect(artefact).toBeDefined();
+ expect(artefact).not.toBeNull();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const artefact = await getLatestArtefactByLocationAndListType(testLocationId, 8); | |
| expect(artefact).toBeDefined(); | |
| const pdfPath = path.join(process.cwd(), "..", "storage", "temp", "uploads", `${artefact!.artefactId}.pdf`); | |
| expect(fs.existsSync(pdfPath)).toBe(true); | |
| expect(fs.statSync(pdfPath).size).toBeGreaterThan(0); | |
| const artefact = await getLatestArtefactByLocationAndListType(testLocationId, 8); | |
| expect(artefact).not.toBeNull(); | |
| const pdfPath = path.join(process.cwd(), "..", "storage", "temp", "uploads", `${artefact!.artefactId}.pdf`); | |
| expect(fs.existsSync(pdfPath)).toBe(true); | |
| expect(fs.statSync(pdfPath).size).toBeGreaterThan(0); |
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
|
Preview Deployment Successful 🚀Your preview environment is ready:
The environment will be automatically cleaned up when this PR is closed. |
3 participants
Jira link
#475
Change description
Fix issue with PDF not generated
Checklist
Summary by CodeRabbit
Bug Fixes
Tests
Chores