Fixes issue-3299 #3480
Fixes issue-3299 #3480
Conversation
WalkthroughThis pull request addresses a bug in the Action Items feature where updating an action item was causing the assignee to be unassigned. The changes involve modifying the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/screens/OrganizationActionItems/ItemModal.tsx (1)
353-353: LGTM! Consider adding a comment explaining the importance of this line.The addition of
isCompleted: actionItem?.isCompletedis crucial as it preserves the action item's completion status during updates, preventing unintended state changes. This change aligns with the PR's objective of maintaining state consistency during updates.Add a comment above this line:
+ // Preserve the completion status to prevent unintended state changes during updates isCompleted: actionItem?.isCompleted,src/screens/OrganizationActionItems/ItemModal.spec.tsx (3)
519-547: Consider adding assertions to explicitly verify the assignee remains unchanged.While the test implicitly verifies the assignee preservation through the successful update, adding explicit assertions would make the test's intent clearer.
// Verify successful update await waitFor(() => { expect(itemProps[2].actionItemsRefetch).toHaveBeenCalled(); expect(itemProps[2].hide).toHaveBeenCalled(); expect(toast.success).toHaveBeenCalledWith(t.successfulUpdation); + // Verify assignee remains unchanged + const memberSelect = screen.getByTestId('memberSelect'); + expect(within(memberSelect).getByRole('combobox')).toHaveValue('Harve Lance'); });
Line range hint
519-1183: Consider adding test cases for assignee-related edge cases.While the test coverage is comprehensive, consider adding these scenarios to further strengthen the test suite:
- Verify that other fields are preserved when changing the assignee
- Test switching between assignee types (User/EventVolunteer/EventVolunteerGroup) during an update
Example test structure:
it('should preserve other fields when changing assignee', async () => { renderItemModal(link1, itemProps[2]); // Update category and notes first const categorySelect = await screen.findByTestId('categorySelect'); fireEvent.mouseDown(within(categorySelect).getByRole('combobox')); fireEvent.click(await screen.findByText('Category 2')); fireEvent.change(screen.getByLabelText(t.postCompletionNotes), { target: { value: 'Test Notes' }, }); // Change assignee const memberSelect = await screen.findByTestId('memberSelect'); fireEvent.mouseDown(within(memberSelect).getByRole('combobox')); fireEvent.click(await screen.findByText('Bruce Graza')); // Submit and verify all fields const submitButton = screen.getByTestId('submitBtn'); fireEvent.click(submitButton); await waitFor(() => { expect(toast.success).toHaveBeenCalledWith(t.successfulUpdation); expect(within(categorySelect).getByRole('combobox')).toHaveValue('Category 2'); expect(screen.getByLabelText(t.postCompletionNotes)).toHaveValue('Test Notes'); }); });
Line range hint
519-1183: Consider organizing related tests into describe blocks for better readability.The test file could benefit from grouping related tests into describe blocks. This would improve navigation and maintenance of the test suite.
Example structure:
describe('ItemModal Creation', () => { it('Create Action Item (for Member)', ...); it('Create Action Item (for Volunteer)', ...); it('Create Action Item (for Group)', ...); it('should fail to Create Action Item', ...); }); describe('ItemModal Updates', () => { it('Update Action Item (completed)', ...); it('should preserve the assignee when updating other fields', ...); it('Update Action Item (Volunteer)', ...); // ... other update tests }); describe('Input Validation', () => { it('validates allotted hours maximum values', ...); it('handles empty and null form values correctly', ...); // ... other validation tests }); describe('Edge Cases', () => { it('handles timezone edge cases', ...); it('handles potentially malicious input patterns correctly', ...); // ... other edge case tests });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/screens/OrganizationActionItems/ItemModal.spec.tsx(1 hunks)src/screens/OrganizationActionItems/ItemModal.tsx(1 hunks)src/screens/OrganizationActionItems/OrganizationActionItem.mocks.ts(5 hunks)
🧰 Additional context used
📓 Learnings (2)
src/screens/OrganizationActionItems/ItemModal.tsx (1)
Learnt from: Chaitanya1672
PR: PalisadoesFoundation/talawa-admin#2049
File: src/screens/OrganizationActionItems/ActionItemUpdateModal.tsx:112-138
Timestamp: 2024-11-12T10:40:58.654Z
Learning: The `istanbul ignore next` comments in the `ActionItemUpdateModal.tsx` file were added as part of a commit that introduced tests for the `ActionItemUpdateModal` component. Removing these comments and writing tests to cover the previously ignored lines is recommended to ensure code integrity and improve test coverage.
src/screens/OrganizationActionItems/ItemModal.spec.tsx (1)
Learnt from: Chaitanya1672
PR: PalisadoesFoundation/talawa-admin#2049
File: src/screens/OrganizationActionItems/ActionItemUpdateModal.tsx:112-138
Timestamp: 2024-11-12T10:40:58.654Z
Learning: The `istanbul ignore next` comments in the `ActionItemUpdateModal.tsx` file were added as part of a commit that introduced tests for the `ActionItemUpdateModal` component. Removing these comments and writing tests to cover the previously ignored lines is recommended to ensure code integrity and improve test coverage.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
src/screens/OrganizationActionItems/OrganizationActionItem.mocks.ts (1)
218-218: LGTM! Test mocks updated to support the new behavior.The addition of the
isCompletedfield in various mutation requests ensures comprehensive testing of both completed and non-completed states, validating that the completion status is correctly preserved during updates.Also applies to: 259-259, 297-297, 317-317, 474-474
src/screens/OrganizationActionItems/ItemModal.spec.tsx (1)
519-547: LGTM! The new test case effectively verifies the fix for issue #3299.The test case thoroughly validates that updating other fields (category, allotted hours, post-completion notes) preserves the assignee information, which directly addresses the reported issue.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3480 +/- ##
=====================================================
+ Coverage 1.90% 88.80% +86.89%
=====================================================
Files 316 338 +22
Lines 8249 8626 +377
Branches 1880 1922 +42
=====================================================
+ Hits 157 7660 +7503
+ Misses 8083 636 -7447
- Partials 9 330 +321 ☔ View full report in Codecov by Sentry. |
palisadoes
merged commit bc2b0a7
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
This PR fixes the issue where updating an action item causes the assignee to be unassigned, and "Undefined Undefined" appears instead of the assignee's name.
Fixes #3299
Snapshots/Videos:
20250125174111.mp4
If relevant, did you update the documentation?
No
Summary
Problem: When updating an action item’s description, the assignee’s name is replaced by "Undefined Undefined", and the user is unassigned.
Cause: The issue was caused by an incorrect GraphQL query or mutation handling, leading to the loss of user assignment details.
Solution: The fix involves ensuring that the assignee details are preserved during the update, and the correct user is retained after any modification.
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Tests