Trello (patch) trim checklist items #462
Conversation
WalkthroughThis pull request updates the Trello integration by modifying the application version and changelog in the bundle file, updating error messages in the CreateCard component to reflect a raised checklist item limit from 5 to 10, and adding new test suites. The tests validate the expected behavior of the AddChecklistToCard and CreateCard functionalities, covering scenarios such as whitespace handling in checklist items, error handling for exceeded limits, and ensuring proper API interactions. Changes
Suggested reviewers
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
test/trello/checklist/AddChecklistToCard.test.js (1)
1-65:⚠️ Potential issueFix the failing test assertion on line 58.
The test is well structured but there's a pipeline failure at line 58 indicating a missing expected rejection. The test sends checklist items with a leading newline which creates 11 items when split, correctly triggering the error condition. However, the assertion might be failing due to how the error is being checked.
The issue likely relates to how
assert.rejectsis being used. Try modifying the assertion to explicitly check for the error:await assert.rejects( async () => { await action.receive(context); }, - context.CancelError('Maximum 10 checklist items are allowed') + error => { + return error instanceof context.CancelError && + error.message === 'Maximum 10 checklist items are allowed'; + } );🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 58-58: AssertionError [ERR_ASSERTION]: Missing expected rejection (Error).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/appmixer/trello/bundle.json(2 hunks)src/appmixer/trello/list/CreateCard/CreateCard.js(1 hunks)test/trello/checklist/AddChecklistToCard.test.js(1 hunks)test/trello/list/createCard.test.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*/bundle.json`: Every pull request should include changes to the related `bundle.json` file. This file contains version information, so all changes must also include a version ...
**/*/bundle.json: Every pull request should include changes to the relatedbundle.jsonfile. This file contains version information, so all changes must also include a version update.
src/appmixer/trello/bundle.json
🧬 Code Definitions (3)
test/trello/checklist/AddChecklistToCard.test.js (1)
test/trello/list/createCard.test.js (5)
assert(1-1)sinon(2-2)testUtils(3-3)action(4-4)context(8-8)
src/appmixer/trello/list/CreateCard/CreateCard.js (3)
test/trello/checklist/AddChecklistToCard.test.js (1)
context(8-8)test/trello/list/createCard.test.js (1)
context(8-8)src/appmixer/trello/checklist/AddChecklistToCard/AddChecklistToCard.js (2)
context(9-9)context(13-20)
test/trello/list/createCard.test.js (1)
test/trello/checklist/AddChecklistToCard.test.js (5)
assert(1-1)sinon(2-2)testUtils(3-3)action(4-4)context(8-8)
🪛 GitHub Actions: Node.js CI
test/trello/checklist/AddChecklistToCard.test.js
[error] 58-58: AssertionError [ERR_ASSERTION]: Missing expected rejection (Error).
test/trello/list/createCard.test.js
[error] 76-76: AssertionError [ERR_ASSERTION]: Missing expected rejection (Error).
[error] 94-94: AssertionError [ERR_ASSERTION]: Missing expected rejection (Error).
🔇 Additional comments (7)
src/appmixer/trello/bundle.json (2)
3-3: Version incremented properly.The version update from 2.1.4 to 2.1.5 follows the semantic versioning practice for a patch update, which is appropriate for the type of change being made.
35-36: Changelog entry is concise and descriptive.The changelog entry clearly describes the issue that was fixed in this version, which helps users understand what changed.
src/appmixer/trello/list/CreateCard/CreateCard.js (1)
76-76: Limit increased from 5 to 10 checklist items.The error message has been updated to reflect an increased limit of checklist items from 5 to 10. This change is consistent with the actual implementation in line 75 that checks for
items.length > 10.test/trello/checklist/AddChecklistToCard.test.js (2)
29-45: Test properly checks for leading newline handling.This test correctly verifies that whitespace trimming works properly by including a leading newline in the checklist items. When split by newlines, this would produce 11 items (first empty, plus 10 named items), exceeding the limit of 10.
47-61: Verify API request count for checklist items.This test correctly verifies that exactly 11 API calls are made when processing 10 checklist items (1 call to create the checklist + 10 calls to add each item).
🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 58-58: AssertionError [ERR_ASSERTION]: Missing expected rejection (Error).
test/trello/list/createCard.test.js (2)
102-118: Comprehensive test for trimming whitespace in checklist items.This test properly validates that leading and trailing whitespace in checklist items is trimmed when processing. It uses a variety of whitespace characters including newlines and spaces to ensure robust handling.
66-82:Details
✅ Verification successful
Validate behavior with leading newlines.
This test is checking for the case where a leading newline in the checklist items would cause the total number of items to exceed the limit. Double-check if the input
\na\nb\nc\nd\ne\nf\ng\nh\ni\njactually produces 11 items when split by\n.
🏁 Script executed:
#!/bin/bash # Verify item count when splitting with leading newline node -e " const items = '\na\nb\nc\nd\ne\nf\ng\nh\ni\nj'.split('\n'); console.log('Number of items:', items.length); console.log('Items:', items); "Length of output: 265
Confirmed: Leading Newline Produces 11 Items
The verification confirms that splitting the input string
\na\nb\nc\nd\ne\nf\ng\nh\ni\njby\nindeed results in 11 items (with the first element being an empty string due to the leading newline). This validates the test case’s expectation that having a leading newline pushes the total count to 11, thereby triggering the error message "Maximum 10 checklist items are allowed."No changes are required in the test implementation here.
🧰 Tools
🪛 GitHub Actions: Node.js CI
[error] 76-76: AssertionError [ERR_ASSERTION]: Missing expected rejection (Error).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/trello/list/createCard.test.js (1)
40-45:⚠️ Potential issueFix the failing test assertions.
The current use of
assert.rejectsis problematic and causing the tests to fail. The pipeline shows errors at lines 77 and 94.Modify all assertions to explicitly check for the error type and message:
await assert.rejects( async () => { await action.receive(context); }, - context.CancelError('Maximum 10 checklist items are allowed') + error => { + return error instanceof context.CancelError && + error.message === 'Maximum 10 checklist items are allowed'; + } );Make this same change for all
assert.rejectscalls in the file (lines 40-45, 51-56, 59-64, 77-82, and 92-97).Also applies to: 51-56, 59-64, 77-82, 92-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/appmixer/trello/checklist/AddChecklistToCard/AddChecklistToCard.js(1 hunks)test/trello/list/createCard.test.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
src/appmixer/trello/checklist/AddChecklistToCard/AddChecklistToCard.js (1)
src/appmixer/trello/list/CreateCard/CreateCard.js (2)
items(73-73)checklistItems(35-35)
test/trello/list/createCard.test.js (1)
test/trello/checklist/AddChecklistToCard.test.js (5)
assert(1-1)sinon(2-2)testUtils(3-3)action(4-4)context(8-8)
🔇 Additional comments (3)
src/appmixer/trello/checklist/AddChecklistToCard/AddChecklistToCard.js (1)
24-24: Good improvement to checklist item handling.Adding
trim()before splitting ensures that leading and trailing whitespace is properly removed from the input string, preventing empty items from being counted toward the limit. This aligns with the individual item trimming that was already happening on line 37.test/trello/list/createCard.test.js (2)
100-116: Well-implemented test for trimming functionality.This test effectively validates that the trimming functionality correctly handles checklist items with leading and trailing whitespace. It confirms that exactly 10 items are created when the limit is met, even when the input contains extra whitespace.
108-108: Good test coverage for whitespace handling.The test input correctly simulates a user entering checklist items with leading and trailing whitespace and newlines, which is precisely the scenario that the trim functionality in AddChecklistToCard is designed to handle.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/trello/list/createCard.test.js (1)
77-82:⚠️ Potential issueFix the failing test assertion as highlighted in pipeline errors.
This assertion is failing in the CI pipeline. The correct way to use
assert.rejectsis to provide a function that checks both error type and message.await assert.rejects( async () => { await action.receive(context); }, - context.CancelError('Maximum 10 checklist items are allowed') + error => { + return error instanceof context.CancelError && + error.message === 'Maximum 10 checklist items are allowed'; + } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/trello/list/createCard.test.js(1 hunks)
🔇 Additional comments (1)
test/trello/list/createCard.test.js (1)
85-101: Good test for checklist item trimming functionality.This test effectively validates that checklist items with leading and trailing whitespace are properly handled. The test checks that with 10 valid items (after trimming), exactly 12 API requests are made (1 for card creation, 1 for checklist creation, and 10 for the items).
| await assert.rejects( | ||
| async () => { | ||
| await action.receive(context); | ||
| }, | ||
| context.CancelError('Card name is required') | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the assertion usage for missing card name test.
Update the assertion to check both error type and message for consistency.
await assert.rejects(
async () => {
await action.receive(context);
},
- context.CancelError('Card name is required')
+ error => {
+ return error instanceof context.CancelError &&
+ error.message === 'Card name is required';
+ }
);📝 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.
| await assert.rejects( | |
| async () => { | |
| await action.receive(context); | |
| }, | |
| context.CancelError('Card name is required') | |
| ); | |
| await assert.rejects( | |
| async () => { | |
| await action.receive(context); | |
| }, | |
| error => { | |
| return error instanceof context.CancelError && | |
| error.message === 'Card name is required'; | |
| } | |
| ); |
| await assert.rejects( | ||
| async () => { | ||
| await action.receive(context); | ||
| }, | ||
| context.CancelError('Card name is required') | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the assertion usage for empty card name test.
Similar to the previous issue, update the assertion to properly check both error type and message.
await assert.rejects(
async () => {
await action.receive(context);
},
- context.CancelError('Card name is required')
+ error => {
+ return error instanceof context.CancelError &&
+ error.message === 'Card name is required';
+ }
);📝 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.
| await assert.rejects( | |
| async () => { | |
| await action.receive(context); | |
| }, | |
| context.CancelError('Card name is required') | |
| ); | |
| await assert.rejects( | |
| async () => { | |
| await action.receive(context); | |
| }, | |
| error => { | |
| return error instanceof context.CancelError && | |
| error.message === 'Card name is required'; | |
| } | |
| ); |
| await assert.rejects( | ||
| async () => { | ||
| await action.receive(context); | ||
| }, | ||
| context.CancelError('Checklist name is required to add checklist items') | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the assertion usage for better error validation.
The current usage of assert.rejects with a direct error object doesn't properly validate both the error type and message. This can lead to false positives in tests.
await assert.rejects(
async () => {
await action.receive(context);
},
- context.CancelError('Checklist name is required to add checklist items')
+ error => {
+ return error instanceof context.CancelError &&
+ error.message === 'Checklist name is required to add checklist items';
+ }
);📝 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.
| await assert.rejects( | |
| async () => { | |
| await action.receive(context); | |
| }, | |
| context.CancelError('Checklist name is required to add checklist items') | |
| ); | |
| await assert.rejects( | |
| async () => { | |
| await action.receive(context); | |
| }, | |
| error => { | |
| return error instanceof context.CancelError && | |
| error.message === 'Checklist name is required to add checklist items'; | |
| } | |
| ); |
2 participants
Before
https://github.com/clientIO/appmixer-connectors/actions/runs/14308045256/job/40096243646
After
https://github.com/clientIO/appmixer-connectors/actions/runs/14308058070/job/40096283683
Summary by CodeRabbit
Summary by CodeRabbit