Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 69 additions & 0 deletions src/actions/__tests__/page-template-actions.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import { describe, it, expect } from "@jest/globals";
import {
PAGE_MODULES_DOWNLOAD,
PAGES_MODULE_KINDS
} from "../../utils/constants";
import { normalizeEntity } from "../page-template-actions";

const buildModule = (kind, type, file) => ({
kind,
type,
file
});

describe("Page Template Actions", () => {
describe("normalizeEntity", () => {
it("should include new file in payload", () => {
const entity = {
modules: [
buildModule(
PAGES_MODULE_KINDS.DOCUMENT,
PAGE_MODULES_DOWNLOAD.FILE,
[{ name: "newfile.pdf" }] // new file, no id
)
]
};
const result = normalizeEntity(entity);
expect(result.modules[0].file).toBeDefined();
expect(result.modules[0].file.name).toBe("newfile.pdf");
});

it("should omit existing file from payload", () => {
const entity = {
modules: [
buildModule(PAGES_MODULE_KINDS.DOCUMENT, PAGE_MODULES_DOWNLOAD.FILE, [
{ id: 123, name: "existing.pdf" }
])
]
};
const result = normalizeEntity(entity);
expect(result.modules[0].file).toBeUndefined();
});

it("should omit file if not FILE type", () => {
const entity = {
modules: [
buildModule(PAGES_MODULE_KINDS.DOCUMENT, "NOT_FILE", [
{ name: "other.pdf" }
])
]
};
const result = normalizeEntity(entity);
expect(result.modules[0].file).toBeUndefined();
});

it("should handle file as null", () => {
const entity = {
modules: [
buildModule(
PAGES_MODULE_KINDS.DOCUMENT,
PAGE_MODULES_DOWNLOAD.FILE,
null
)
]
};
const result = normalizeEntity(entity);
expect(result.modules[0].file).toBeUndefined();
});
});
});
2 changes: 1 addition & 1 deletion src/actions/page-template-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ export const resetPageTemplateForm = () => (dispatch) => {
dispatch(createAction(RESET_PAGE_TEMPLATE_FORM)({}));
};

const normalizeEntity = (entity) => {
export const normalizeEntity = (entity) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

do we need to export this method ? who is using it ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

it's only for testing purposes

const normalizedEntity = { ...entity };
normalizedEntity.modules = normalizePageTemplateModules(entity.modules);
return normalizedEntity;
Expand Down
8 changes: 4 additions & 4 deletions src/utils/__tests__/page-template.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ describe("normalizePageTemplateModules", () => {
});

describe("DOCUMENT kind — FILE type", () => {
it("should extract the first element from the file array and delete external_url", () => {
it("should remove file and external_url for existing files", () => {
const fileObj = {
id: 10,
storage_key: "key/file.pdf",
Expand All @@ -134,18 +134,18 @@ describe("normalizePageTemplateModules", () => {
external_url: "https://example.com"
};
const [result] = normalizePageTemplateModules([module]);
expect(result.file).toStrictEqual(fileObj);
expect(result.file).toBeUndefined();
expect(result.external_url).toBeUndefined();
});

it("should set file to null when the file array is empty", () => {
it("should remove file when the file array is empty", () => {
const module = {
kind: PAGES_MODULE_KINDS.DOCUMENT,
type: PAGE_MODULES_DOWNLOAD.FILE,
file: []
};
const [result] = normalizePageTemplateModules([module]);
expect(result.file).toBeNull();
expect(result.file).toBeUndefined();
});
});

Expand Down
11 changes: 10 additions & 1 deletion src/utils/page-template.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,15 @@ export const normalizePageTemplateModules = (modules = [], timeZone = null) =>

if (module.kind === PAGES_MODULE_KINDS.DOCUMENT) {
if (module.type === PAGE_MODULES_DOWNLOAD.FILE) {
normalizedModule.file = module.file?.[0] || null;
// Only new files (without id or file_id) are sent in the payload; existing files are omitted to prevent overwriting.
const file = Array.isArray(module.file) ? module.file[0] : null;
const isNewFile =
file && typeof file === "object" && !file.id && !file.file_id;
Comment on lines +66 to +67

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Use explicit null checks for file identity.

!file.id && !file.file_id treats falsy values (e.g. 0) as missing, which can misclassify an existing file as new. Prefer null/undefined checks.

Suggested patch
-        const isNewFile =
-          file && typeof file === "object" && !file.id && !file.file_id;
+        const isNewFile =
+          file &&
+          typeof file === "object" &&
+          file.id == null &&
+          file.file_id == null;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/utils/page-template.js` around lines 66 - 67, The isNewFile determination
currently treats falsy id values as missing; change the check in the isNewFile
logic to explicitly test for null/undefined on file.id and file.file_id (i.e.,
use explicit null/undefined checks rather than truthiness) so numeric IDs like 0
are not misclassified; update the expression that sets isNewFile (referencing
isNewFile, file.id, and file.file_id) to only consider the file new when both id
and file_id are strictly null or undefined.

if (isNewFile) {
normalizedModule.file = file;
} else {
delete normalizedModule.file;
}
delete normalizedModule.external_url;
} else {
delete normalizedModule.file;
Expand All @@ -73,3 +81,4 @@ export const normalizePageTemplateModules = (modules = [], timeZone = null) =>

return normalizedModule;
});

Loading