fix: fix bulk inline edit mode for event activities#818
fix: fix bulk inline edit mode for event activities#818priscila-moneo wants to merge 1 commit intomasterfrom
Conversation
priscila-moneo
force-pushed
the
fix/fix-bulk-inline-edit-mode-event-activities
branch
from
a27bc30 to
9520b71
Compare
📝 WalkthroughWalkthroughThis PR introduces comprehensive unit tests for event normalization and table component behaviors, updates event action logic to handle speakers and sponsors more robustly with type checking and filtering, passes full row context to table render callbacks, and refactors media_uploads rendering in the event list page while removing the afterUpdate hook integration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/events/summit-event-list-page.js (1)
1950-1963:⚠️ Potential issue | 🟡 MinorAdd an inline comment explaining the intentional omission of
afterUpdatein bulk edit mode.The removal is intentional—a test explicitly verifies that
afterUpdateis not passed in bulk mode. However, add a comment in the code (near theEditableTablecomponent) explaining thatafterUpdateis intentionally omitted to prevent bulk edits from triggering individualsaveEventMaterialcalls. This will clarify the design decision for future maintainers reviewing the code directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/events/summit-event-list-page.js` around lines 1950 - 1963, Add an inline comment next to the EditableTable JSX explaining that the absence of the afterUpdate prop is intentional in bulk edit mode: mention that bulkUpdateEvents is used instead and afterUpdate is omitted to avoid triggering per-row saveEventMaterial calls (this omission is explicitly asserted in tests); locate the comment near the EditableTable component props (EditableTable, bulkUpdateEvents, afterUpdate, saveEventMaterial) so future maintainers understand the design decision.src/actions/event-actions.js (1)
510-540:⚠️ Potential issue | 🟡 MinorSpeakers field in bulk edit requires clarification on intentional exclusion.
normalizeBulkEventsintentionally excludesspeakersfrom the bulk update payload (per the test "does not include speakers in bulk payload"). However, for event types withuse_speakers: true, theSpeakerInputcomponent is rendered and editable in the bulk edit table (line 79 insummit-event-list-page.js). This creates a UX inconsistency: users can modify speakers in the UI for these event types, but the changes won't persist to the backend.For event types where
use_speakers: false, the speakers column is already hidden, so this is not an issue. But for event types that support speakers, consider either:
- Disabling/hiding the speakers column in bulk edit mode, or
- Adding speaker support to the bulk update payload if the backend supports it
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/event-actions.js` around lines 510 - 540, normalizeBulkEvents currently strips speakers from the bulk update payload causing a UX mismatch when SpeakerInput is editable in bulk edit; fix by either (A) adding speakers support to the bulk payload in normalizeBulkEvents: include a speakers property (e.g., map e.speakers to an array of ids or objects the backend expects, using existing helpers like getIdValue where appropriate) and ensure null/undefined cleanup logic still applies, or (B) prevent editing by hiding/disabling the SpeakerInput in the bulk edit UI (update summit-event-list-page.js to conditionally render/disable SpeakerInput for bulk mode or for types with use_speakers true but not editable in bulk); pick one approach and update tests that assert "does not include speakers in bulk payload" to match the chosen behavior.
🧹 Nitpick comments (1)
src/pages/events/__tests__/summit-event-list-page.test.js (1)
186-196: Fragile DOM traversal to access button click handler.The nested
props.childrenaccess pattern (lines 188-191) is brittle and will break if the render function's JSX structure changes. Consider extracting the render function and testing it in isolation, or using a more robust query approach.// Alternative: Extract render function for direct testing const mediaUploadsRender = mediaUploadsColumn.render; // Then test the function directly with mock data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/events/__tests__/summit-event-list-page.test.js` around lines 186 - 196, The test is using brittle DOM traversal to reach the click handler (navigating nested props.children of mediaUploadsColumn.render output and calling firstButton.props.onClick), so refactor the test to call mediaUploadsColumn.render directly with a mock row (e.g., mediaUploadItem) and inspect the returned element or query for the button more robustly; specifically, extract mediaUploadsColumn.render into a variable (mediaUploadsRender) and invoke it with the test data, then either find the button by its component type/props within the returned React element or use a DOM querying utility (React Testing Library) to locate the button and fire the click, rather than assuming a specific children nesting structure.
🤖 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/pages/events/__tests__/edit-event-material-page.test.js`:
- Around line 13-16: The test's mocked EventMaterialForm currently submits only
{ id: 77 }, but saveEventMaterial derives the slug from entity.class_name (see
saveEventMaterial in src/actions/event-material-actions.js), so update the mock
submit payloads/regular-submit fixture to include a class_name (e.g., { id: 77,
class_name: "EventMaterial" }) so the test exercises the same routing logic as
runtime; make the same change for the other mocked submit used later in the file
to ensure both mocks include class_name.
---
Outside diff comments:
In `@src/actions/event-actions.js`:
- Around line 510-540: normalizeBulkEvents currently strips speakers from the
bulk update payload causing a UX mismatch when SpeakerInput is editable in bulk
edit; fix by either (A) adding speakers support to the bulk payload in
normalizeBulkEvents: include a speakers property (e.g., map e.speakers to an
array of ids or objects the backend expects, using existing helpers like
getIdValue where appropriate) and ensure null/undefined cleanup logic still
applies, or (B) prevent editing by hiding/disabling the SpeakerInput in the bulk
edit UI (update summit-event-list-page.js to conditionally render/disable
SpeakerInput for bulk mode or for types with use_speakers true but not editable
in bulk); pick one approach and update tests that assert "does not include
speakers in bulk payload" to match the chosen behavior.
In `@src/pages/events/summit-event-list-page.js`:
- Around line 1950-1963: Add an inline comment next to the EditableTable JSX
explaining that the absence of the afterUpdate prop is intentional in bulk edit
mode: mention that bulkUpdateEvents is used instead and afterUpdate is omitted
to avoid triggering per-row saveEventMaterial calls (this omission is explicitly
asserted in tests); locate the comment near the EditableTable component props
(EditableTable, bulkUpdateEvents, afterUpdate, saveEventMaterial) so future
maintainers understand the design decision.
---
Nitpick comments:
In `@src/pages/events/__tests__/summit-event-list-page.test.js`:
- Around line 186-196: The test is using brittle DOM traversal to reach the
click handler (navigating nested props.children of mediaUploadsColumn.render
output and calling firstButton.props.onClick), so refactor the test to call
mediaUploadsColumn.render directly with a mock row (e.g., mediaUploadItem) and
inspect the returned element or query for the button more robustly;
specifically, extract mediaUploadsColumn.render into a variable
(mediaUploadsRender) and invoke it with the test data, then either find the
button by its component type/props within the returned React element or use a
DOM querying utility (React Testing Library) to locate the button and fire the
click, rather than assuming a specific children nesting structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 297e8a7c-ba70-48b1-98a3-e427c8cb894b
📒 Files selected for processing (7)
src/actions/__tests__/event-actions-bulk.test.jssrc/actions/event-actions.jssrc/components/tables/editable-table/EditableTableRow.jssrc/components/tables/editable-table/__tests__/EditableTable.test.jssrc/pages/events/__tests__/edit-event-material-page.test.jssrc/pages/events/__tests__/summit-event-list-page.test.jssrc/pages/events/summit-event-list-page.js
| jest.mock("../../../components/forms/event-material-form", () => (props) => ( | ||
| <div> | ||
| <button type="button" onClick={() => props.onSubmit({ id: 77 })}> | ||
| submit-material |
There was a problem hiding this comment.
Include class_name in the regular-submit fixture.
Line 15 submits only { id: 77 }, but saveEventMaterial derives its slug from entity.class_name in src/actions/event-material-actions.js:71-86. That makes this test pass even if the page drops class_name and would hit the wrong "media-uploads" route at runtime.
Suggested change
jest.mock("../../../components/forms/event-material-form", () => (props) => (
<div>
- <button type="button" onClick={() => props.onSubmit({ id: 77 })}>
+ <button
+ type="button"
+ onClick={() =>
+ props.onSubmit({ id: 77, class_name: "PresentationSlide" })
+ }
+ >
submit-material
</button>
@@
expect(EventMaterialActions.saveEventMaterial).toHaveBeenCalledTimes(1);
expect(EventMaterialActions.saveEventMaterial).toHaveBeenCalledWith({
- id: 77
+ id: 77,
+ class_name: "PresentationSlide"
});
});Also applies to: 90-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/events/__tests__/edit-event-material-page.test.js` around lines 13
- 16, The test's mocked EventMaterialForm currently submits only { id: 77 }, but
saveEventMaterial derives the slug from entity.class_name (see saveEventMaterial
in src/actions/event-material-actions.js), so update the mock submit
payloads/regular-submit fixture to include a class_name (e.g., { id: 77,
class_name: "EventMaterial" }) so the test exercises the same routing logic as
runtime; make the same change for the other mocked submit used later in the file
to ensure both mocks include class_name.
1 participant
ref: https://app.clickup.com/t/86b8rq0hj
afterUpdatemedia side effects).Summary by CodeRabbit
Bug Fixes
Tests