fix: denormalize module data when a show page is recieved, unit test#862
Merged
fix: denormalize module data when a show page is recieved, unit test#862
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:
📝 WalkthroughWalkthroughCentralized page-module normalization/denormalization into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/reducers/sponsors/show-pages-list-reducer.js (1)
152-165: Extract the DOCUMENT denormalization into a shared helper.This branch now mirrors
src/reducers/sponsors_inventory/page-template-reducer.js:60-85, so the next document-shape change will have to land in two reducers. A small shared helper for the file/type rewrite would keep the receive flows from drifting.♻️ Refactor sketch
- if (module.kind === PAGES_MODULE_KINDS.DOCUMENT) { - if (module.file) { - tmpModule.file = [ - { - ...module.file, - file_path: module.file.storage_key, - public_url: module.file.file_url - } - ]; - tmpModule.type = PAGE_MODULES_DOWNLOAD.FILE; - } else { - tmpModule.type = PAGE_MODULES_DOWNLOAD.URL; - } - } + Object.assign(tmpModule, denormalizeDocumentModule(module));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reducers/sponsors/show-pages-list-reducer.js` around lines 152 - 165, Extract the DOCUMENT denormalization logic into a shared helper to avoid duplication: move the block that checks module.kind === PAGES_MODULE_KINDS.DOCUMENT and rewrites module.file -> tmpModule.file (including file_path: module.file.storage_key and public_url: module.file.file_url) and sets tmpModule.type to PAGE_MODULES_DOWNLOAD.FILE or PAGE_MODULES_DOWNLOAD.URL into a new utility function (e.g., normalizeDocumentModule(module) or denormalizeDocumentFile(module, tmpModule)); replace the inlined logic in show-pages-list-reducer (and mirror change in sponsors_inventory/page-template-reducer) to call that helper so future document-shape changes are implemented once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/reducers/sponsors/show-pages-list-reducer.js`:
- Around line 152-165: Extract the DOCUMENT denormalization logic into a shared
helper to avoid duplication: move the block that checks module.kind ===
PAGES_MODULE_KINDS.DOCUMENT and rewrites module.file -> tmpModule.file
(including file_path: module.file.storage_key and public_url:
module.file.file_url) and sets tmpModule.type to PAGE_MODULES_DOWNLOAD.FILE or
PAGE_MODULES_DOWNLOAD.URL into a new utility function (e.g.,
normalizeDocumentModule(module) or denormalizeDocumentFile(module, tmpModule));
replace the inlined logic in show-pages-list-reducer (and mirror change in
sponsors_inventory/page-template-reducer) to call that helper so future
document-shape changes are implemented once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6728e2ce-4a59-4cb2-8326-61268074ce99
📒 Files selected for processing (2)
src/reducers/sponsors/__tests__/show-pages-list-reducer.test.jssrc/reducers/sponsors/show-pages-list-reducer.js
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/utils/page-template.js (1)
14-21: Consider validatingepochToMomentTimeZonereturn value.Based on usage patterns elsewhere in the codebase (e.g.,
src/utils/methods.js:315-325),epochToMomentTimeZonemay return invalid values when conversion fails. The existing pattern usesmoment.isMoment()to validate before proceeding. Here, no such validation occurs, which could result in an invalid moment object being assigned toupload_deadline.🤖 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 14 - 21, The assignment to upload_deadline should validate epochToMomentTimeZone's return like other usages: when timeZone is provided call epochToMomentTimeZone(module.upload_deadline, timeZone) and check with moment.isMoment() (or fall back to moment(module.upload_deadline * MILLISECONDS_IN_SECOND) or omit the field) so that an invalid moment is never assigned; update the ternary/conditional creating upload_deadline to perform this validation and fallback logic referencing epochToMomentTimeZone, moment.isMoment, timeZone, module.upload_deadline and MILLISECONDS_IN_SECOND.src/actions/show-pages-actions.js (1)
129-142: Consider passing summit timezone for consistency.
normalizePageTemplateModulesis called without thetimeZoneparameter, causing it to usemoment.utc().unix()for deadline conversion. Whilemoment.unix()returns the same epoch regardless of timezone display, passing the summit timezone would maintain consistency with howdenormalizePageModulesis called in the reducer (withstate.summitTZ).♻️ Optional fix for consistency
-const normalizeShowPage = (entity) => { +const normalizeShowPage = (entity, timeZone) => { const normalizedEntity = { ...entity }; normalizedEntity.apply_to_all_types = false; if (entity.sponsorship_types?.includes("all")) { normalizedEntity.apply_to_all_types = true; delete normalizedEntity.sponsorship_types; } - normalizedEntity.modules = normalizePageTemplateModules(entity.modules); + normalizedEntity.modules = normalizePageTemplateModules(entity.modules, timeZone); return normalizedEntity; };Then update the call site in
saveShowPage:- const normalizedShowPage = normalizeShowPage(entity); + const summitTZ = currentSummit.time_zone?.name; + const normalizedShowPage = normalizeShowPage(entity, summitTZ);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/show-pages-actions.js` around lines 129 - 142, The normalizeShowPage function calls normalizePageTemplateModules without a timezone, causing inconsistent deadline handling; update normalizeShowPage to accept and forward the summit timezone into normalizePageTemplateModules (e.g., add a timeZone parameter and call normalizePageTemplateModules(entity.modules, timeZone)); also ensure saveShowPage (where normalizeShowPage is invoked) passes state.summitTZ through to normalizeShowPage so the same summitTZ used by denormalizePageModules in the reducer is used here as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/page-template.js`:
- Around line 10-11: denormalizePageModules and normalizePageTemplateModules
call .map() on the modules parameter without guarding against null/undefined;
update both functions (denormalizePageModules and normalizePageTemplateModules)
to defensively handle falsy modules by treating modules as an empty array (e.g.,
defaulting modules to [] before mapping or returning [] when modules is falsy)
so the .map() call never throws; preserve existing parameters (like timeZone)
and behavior for non-empty arrays.
---
Nitpick comments:
In `@src/actions/show-pages-actions.js`:
- Around line 129-142: The normalizeShowPage function calls
normalizePageTemplateModules without a timezone, causing inconsistent deadline
handling; update normalizeShowPage to accept and forward the summit timezone
into normalizePageTemplateModules (e.g., add a timeZone parameter and call
normalizePageTemplateModules(entity.modules, timeZone)); also ensure
saveShowPage (where normalizeShowPage is invoked) passes state.summitTZ through
to normalizeShowPage so the same summitTZ used by denormalizePageModules in the
reducer is used here as well.
In `@src/utils/page-template.js`:
- Around line 14-21: The assignment to upload_deadline should validate
epochToMomentTimeZone's return like other usages: when timeZone is provided call
epochToMomentTimeZone(module.upload_deadline, timeZone) and check with
moment.isMoment() (or fall back to moment(module.upload_deadline *
MILLISECONDS_IN_SECOND) or omit the field) so that an invalid moment is never
assigned; update the ternary/conditional creating upload_deadline to perform
this validation and fallback logic referencing epochToMomentTimeZone,
moment.isMoment, timeZone, module.upload_deadline and MILLISECONDS_IN_SECOND.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24657e69-6cf7-45e4-96c3-9d2379950506
📒 Files selected for processing (6)
src/actions/page-template-actions.jssrc/actions/show-pages-actions.jssrc/actions/sponsor-pages-actions.jssrc/reducers/sponsors/show-pages-list-reducer.jssrc/reducers/sponsors_inventory/page-template-reducer.jssrc/utils/page-template.js
✅ Files skipped from review due to trivial changes (1)
- src/actions/sponsor-pages-actions.js
smarcet
requested changes
Apr 22, 2026
smarcet
left a comment
smarcet
left a comment
There was a problem hiding this comment.
Review Summary
The refactoring to centralize page module normalization/denormalization into src/utils/page-template.js is well-structured and the new reducer tests are comprehensive. However, there is a behavioral regression in sponsorship_types handling and some gaps in test coverage for the normalization path.
|
|
||
| return normalizedModule; | ||
| }); | ||
| normalizedEntity.modules = normalizePageTemplateModules(entity.modules); |
There was a problem hiding this comment.
[MEDIUM] Normalization scope expanded — verify API compatibility
The previous inline code here only handled:
- MEDIA
upload_deadline→ unix timestamp - MEDIA
file_type_id→ unwrap.value - DOCUMENT
file→ extractfile[0]
The centralized normalizePageTemplateModules now also:
- Deletes
file_type_idandmax_file_sizeforMEDIA/INPUTmodules - Deletes
external_urlforDOCUMENT/FILEmodules - Deletes
fileandfile_idforDOCUMENT/URLmodules
These additional field deletions were previously only applied in page-template-actions. If the show-pages API endpoint has different expectations (e.g., ignores extra fields vs. requires them), this could cause unintended data loss on save.
The same applies to sponsor-pages-actions.js which had the same inline logic.
Suggested fix: Verify that the show-pages and sponsor-pages API endpoints accept the same normalized payload as the page-templates endpoint.
smarcet
requested changes
Apr 22, 2026
…ests Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
tomrndom
force-pushed
the
fix/show-pages-denormalize-reducer
branch
from
cd0c5e7 to
59ab1d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/utils/page-template.js (1)
40-75:⚠️ Potential issue | 🟠 Major
normalizePageTemplateModulesstill needs direct unit coverage.This function still carries branch-heavy payload shaping (MEDIA INPUT/FILE, DOCUMENT FILE/URL, timezone conversion), but no direct utility-level tests are shown here. A regression here can silently corrupt API payloads.
🤖 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 40 - 75, Add direct unit tests for normalizePageTemplateModules that cover all branches: MEDIA with and without upload_deadline and with timeZone/non-timeZone conversions (verify unix timestamps), MEDIA file_type_id normalization and branch where type === PAGE_MODULES_MEDIA_TYPES.INPUT (ensure file_type_id and max_file_size are removed), DOCUMENT with type === PAGE_MODULES_DOWNLOAD.FILE (ensure file is set to first element and external_url removed) and DOCUMENT else branch (ensure file and file_id are removed), and that _tempId is always deleted; reference the normalizePageTemplateModules function and the constants PAGES_MODULE_KINDS, PAGE_MODULES_MEDIA_TYPES, PAGE_MODULES_DOWNLOAD in your tests and assert exact output shapes for representative payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/actions/sponsor-pages-actions.js`:
- Around line 25-33: The code is referencing undefined symbols
normalizeSelectAllField, PAGES_MODULE_KINDS and moment in the managed-page save
flow; replace that logic to call the already-imported
normalizePageTemplateModules instead: feed the page/module data into
normalizePageTemplateModules, use its normalized modules in the payload instead
of relying on normalizeSelectAllField/PAGES_MODULE_KINDS, and replace
moment-based timestamp formatting with plain JS Date (e.g., new
Date().toISOString()) or the existing app date helper so no new imports are
required; ensure you update the save payload construction to use the normalized
modules returned by normalizePageTemplateModules and remove any references to
the undefined symbols.
---
Duplicate comments:
In `@src/utils/page-template.js`:
- Around line 40-75: Add direct unit tests for normalizePageTemplateModules that
cover all branches: MEDIA with and without upload_deadline and with
timeZone/non-timeZone conversions (verify unix timestamps), MEDIA file_type_id
normalization and branch where type === PAGE_MODULES_MEDIA_TYPES.INPUT (ensure
file_type_id and max_file_size are removed), DOCUMENT with type ===
PAGE_MODULES_DOWNLOAD.FILE (ensure file is set to first element and external_url
removed) and DOCUMENT else branch (ensure file and file_id are removed), and
that _tempId is always deleted; reference the normalizePageTemplateModules
function and the constants PAGES_MODULE_KINDS, PAGE_MODULES_MEDIA_TYPES,
PAGE_MODULES_DOWNLOAD in your tests and assert exact output shapes for
representative payloads.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9582a9b9-69a2-4fe3-bde0-1b9202db2865
📒 Files selected for processing (7)
src/actions/page-template-actions.jssrc/actions/show-pages-actions.jssrc/actions/sponsor-pages-actions.jssrc/reducers/sponsors/__tests__/show-pages-list-reducer.test.jssrc/reducers/sponsors/show-pages-list-reducer.jssrc/reducers/sponsors_inventory/page-template-reducer.jssrc/utils/page-template.js
✅ Files skipped from review due to trivial changes (1)
- src/reducers/sponsors/tests/show-pages-list-reducer.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/reducers/sponsors_inventory/page-template-reducer.js
- src/reducers/sponsors/show-pages-list-reducer.js
- src/actions/show-pages-actions.js
…rebase Signed-off-by: Tomás Castillo <tcastilloboireau@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/reducers/sponsors/__tests__/show-pages-list-reducer.test.js (1)
370-390: Add an assertion thatexternal_urlis preserved for DOCUMENT URL modules.This keeps the URL-path behavior fully guarded (not just
typeandfileshape).Small assertion addition
const mod = result.currentShowPage.modules[0]; expect(mod.type).toBe(PAGE_MODULES_DOWNLOAD.URL); expect(mod.file).toBeUndefined(); + expect(mod.external_url).toBe("https://example.com/doc");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/reducers/sponsors/__tests__/show-pages-list-reducer.test.js` around lines 370 - 390, The test for showPagesListReducer handling a DOCUMENT module should also assert the external_url is preserved: in the RECEIVE_SHOW_PAGE case where modules include { kind: PAGES_MODULE_KINDS.DOCUMENT, external_url: "https://example.com/doc" }, add an expectation that the resulting mod (from result.currentShowPage.modules[0]) has mod.external_url === "https://example.com/doc" in addition to the existing checks for mod.type === PAGE_MODULES_DOWNLOAD.URL and mod.file being undefined.src/utils/__tests__/page-template.test.js (1)
40-65: Strengthen UTC vs timezone branch assertions to prevent false positives.Both tests currently assert the same mocked timestamp, so they can pass even if the wrong branch is executed. Add call assertions for the conversion path in each test.
Proposed test hardening
describe("normalizePageTemplateModules", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + it("should convert upload_deadline to unix using moment.utc when no timeZone provided", () => { + const moment = jest.requireMock("moment-timezone"); const module = { kind: PAGES_MODULE_KINDS.MEDIA, type: PAGE_MODULES_MEDIA_TYPES.FILE, upload_deadline: "2024-01-15T00:00:00Z", file_type_id: 3, max_file_size: 1024 }; const [result] = normalizePageTemplateModules([module]); expect(result.upload_deadline).toBe(1700000000); + expect(moment.utc).toHaveBeenCalledWith("2024-01-15T00:00:00Z"); + expect(moment.tz).not.toHaveBeenCalled(); }); it("should convert upload_deadline to unix using moment.tz when timeZone is provided", () => { + const moment = jest.requireMock("moment-timezone"); const module = { kind: PAGES_MODULE_KINDS.MEDIA, type: PAGE_MODULES_MEDIA_TYPES.FILE, upload_deadline: "2024-01-15T00:00:00Z", file_type_id: 3, max_file_size: 1024 }; const [result] = normalizePageTemplateModules( [module], "America/New_York" ); expect(result.upload_deadline).toBe(1700000000); + expect(moment.tz).toHaveBeenCalledWith( + "2024-01-15T00:00:00Z", + "America/New_York" + ); + expect(moment.utc).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/__tests__/page-template.test.js` around lines 40 - 65, The two tests in page-template.test.js are weak because they both assert the same unix value and don't verify which conversion path was used; update the tests around normalizePageTemplateModules so the UTC branch spies on moment.utc (or mocks moment and asserts moment.utc was called) and the timezone branch spies on moment.tz (or mocks moment and asserts moment.tz was called with "America/New_York"), and keep the original expect(result.upload_deadline) assertions but also add the call assertions to ensure the correct conversion function (moment.utc vs moment.tz) was invoked for each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/reducers/sponsors/__tests__/show-pages-list-reducer.test.js`:
- Around line 370-390: The test for showPagesListReducer handling a DOCUMENT
module should also assert the external_url is preserved: in the
RECEIVE_SHOW_PAGE case where modules include { kind:
PAGES_MODULE_KINDS.DOCUMENT, external_url: "https://example.com/doc" }, add an
expectation that the resulting mod (from result.currentShowPage.modules[0]) has
mod.external_url === "https://example.com/doc" in addition to the existing
checks for mod.type === PAGE_MODULES_DOWNLOAD.URL and mod.file being undefined.
In `@src/utils/__tests__/page-template.test.js`:
- Around line 40-65: The two tests in page-template.test.js are weak because
they both assert the same unix value and don't verify which conversion path was
used; update the tests around normalizePageTemplateModules so the UTC branch
spies on moment.utc (or mocks moment and asserts moment.utc was called) and the
timezone branch spies on moment.tz (or mocks moment and asserts moment.tz was
called with "America/New_York"), and keep the original
expect(result.upload_deadline) assertions but also add the call assertions to
ensure the correct conversion function (moment.utc vs moment.tz) was invoked for
each test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1f1d992-fe72-4b1a-a04c-2aa2c1701fd3
📒 Files selected for processing (3)
src/actions/sponsor-pages-actions.jssrc/reducers/sponsors/__tests__/show-pages-list-reducer.test.jssrc/utils/__tests__/page-template.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/actions/sponsor-pages-actions.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref:
Signed-off-by: Tomás Castillo tcastilloboireau@gmail.com
Summary by CodeRabbit
Tests
Bug Fixes
Refactor