feat(refactor): improve email search commands #40
Conversation
- Refactored the `mailpitHasEmailsBySubject` and `mailpitHasEmailsByTo` commands to include an optional timeout and interval for automatic retries. - Updated the `mailpitNotHasEmailsBySubject` and `mailpitNotHasEmailsByTo` commands to also support the optional timeout and interval parameters. - Added a new private method `waitForCondition` to handle the retry logic for the email search commands. Closes #39
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
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
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: 4
🧹 Outside diff range and nitpick comments (7)
cypress.d.ts (5)
31-43: LGTM! Consider adding example usage in the documentation.The changes to
mailpitHasEmailsBySubjectalign well with the PR objectives. The newoptionsparameter for configurable timeout and interval is a great addition. The updated description and return type specification improve clarity and type safety.Consider adding a brief example of how to use the new
optionsparameter in the method documentation. This could help users quickly understand how to utilize the new functionality./** * @example * cy.mailpitHasEmailsBySubject('Welcome', 0, 10, { timeout: 10000, interval: 1000 }); */
46-58: LGTM! Consider adding example usage in the documentation.The changes to
mailpitNotHasEmailsBySubjectare consistent with those made tomailpitHasEmailsBySubjectand align well with the PR objectives. The newoptionsparameter and updated description improve the method's functionality and clarity.As suggested for
mailpitHasEmailsBySubject, consider adding a brief example of how to use the newoptionsparameter in the method documentation:/** * @example * cy.mailpitNotHasEmailsBySubject('Spam', 0, 10, { timeout: 5000, interval: 500 }); */
69-81: LGTM! Consider adding example usage in the documentation.The changes to
mailpitHasEmailsByToare consistent with the previous methods and align well with the PR objectives. The newoptionsparameter and updated description enhance the method's functionality and clarity.As suggested for the previous methods, consider adding a brief example of how to use the new
optionsparameter in the method documentation:/** * @example * cy.mailpitHasEmailsByTo('[email protected]', 0, 10, { timeout: 8000, interval: 800 }); */
84-96: LGTM! Consider adding example usage in the documentation.The changes to
mailpitNotHasEmailsByToare consistent with the previous methods and align well with the PR objectives. The newoptionsparameter and updated description improve the method's functionality and clarity.As suggested for the previous methods, consider adding a brief example of how to use the new
optionsparameter in the method documentation:/** * @example * cy.mailpitNotHasEmailsByTo('[email protected]', 0, 10, { timeout: 6000, interval: 600 }); */
129-132: LGTM! Consider correcting the deprecated method name.The deprecation notice is clear and helpful. However, there's a typo in the deprecated method name.
Consider correcting the spelling of the deprecated method name:
- mailpitGetMailSpamAssainSummary(message?: Message): Chainable<SpamAssassin>; + mailpitGetMailSpamAssassinSummary(message?: Message): Chainable<SpamAssassin>;This will ensure consistency with the correctly spelled replacement method
mailpitGetMailSpamAssassinSummary.README.md (2)
171-177: LGTM! Consider enhancing the example usage.The addition of
timeoutandintervalparameters aligns well with the PR objectives. This change provides users with the flexibility to customize the retry behavior.To better illustrate the new functionality, consider updating the example to show how to use the new parameters:
cy.mailpitHasEmailsBySubject('My Test', 0, 50, { timeout: 15000, interval: 2000 }).should('be.true');
Line range hint
1-203: Overall, good improvements with minor adjustments needed.The changes to the README.md file effectively document the new functionality for automatic waiting and retries in email checking commands. This aligns well with the PR objectives and the linked issue #39.
Please address the following points:
- Correct the descriptions for
mailpitNotHasEmailsBySubjectandmailpitNotHasEmailsByTo.- Update the examples for
mailpitHasEmailsBySubject,mailpitNotHasEmailsBySubject, andmailpitNotHasEmailsByToto demonstrate the usage of the newtimeoutandintervalparameters.These changes will ensure that the documentation accurately reflects the new functionality and provides clear guidance for users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- README.md (1 hunks)
- cypress.d.ts (4 hunks)
- src/MailpitCommands.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
cypress.d.ts (2)
169-169: LGTM! Improved parameter flexibility.The update to the
messagesparameter type and description formailpitSetStatusAsReadis a good improvement. It now allows for bothMessageandMessageSummaryobjects, which increases flexibility and aligns with modern TypeScript practices.
175-175: LGTM! Consistent improvement in parameter flexibility.The update to the
messagesparameter type and description formailpitSetStatusAsUnReadis consistent with the changes made tomailpitSetStatusAsRead. This improvement in flexibility, allowing bothMessageandMessageSummaryobjects, is a good practice and maintains consistency across related methods.README.md (1)
180-186: LGTM! Clear documentation and good example usage.The updates to
mailpitHasEmailsByToare consistent with the changes made tomailpitHasEmailsBySubject. The example usage effectively demonstrates how to use the newtimeoutandintervalparameters.src/MailpitCommands.ts (4)
139-149: Enhancement ofmailpitHasEmailsBySubjectwith retry logic.The addition of the
optionsparameter fortimeoutandinterval, along with the use ofwaitForCondition, enhances the reliability of this method.
152-162: Update tomailpitNotHasEmailsBySubjectusingwaitForCondition.Including the
optionsparameter and implementing retry logic improves consistency and usability.
169-179: Improved functionality inmailpitHasEmailsByTowith added retry capabilities.This change aligns the method with the new retry logic, providing better handling for asynchronous email receipt.
182-192: Consistent updates inmailpitNotHasEmailsByTomethod.The method now appropriately supports
timeoutandintervaloptions, improving user control over the retry behavior.
|
It would be a nice improvment to have a check by Search. |
|
|
|
@TobiasHH I have added two extra commands please check. |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (11)
cypress/e2e/mailpit.cy.ts (1)
169-177: LGTM! Consider enhancing test coverage.The new test case effectively demonstrates the search functionality using both
mailpitHasEmailsBySearchandmailpitNotHasEmailsBySearchcommands, aligning well with the PR objectives.To further improve the test coverage:
- Consider adding assertions to verify the email content after the search.
- Add a test case to demonstrate the usage of optional
timeoutandintervalparameters for these new commands.Would you like assistance in implementing these suggestions?
README.md (2)
171-178: Approve addition with minor suggestion.The new
mailpitHasEmailsBySearchcommand is a valuable addition that aligns with the PR objectives. The description and functionality are clear and accurate.Consider enhancing the example to demonstrate the usage of optional parameters:
cy.mailpitHasEmailsBySearch('subject:My Test', 0, 50, { timeout: 15000, interval: 2000 }).should('be.true');
190-196: Approve update with minor suggestion.The update to
mailpitHasEmailsBySubjectcommand aligns with the PR objectives. The description and functionality are clear and accurate.Consider enhancing the example to demonstrate the usage of optional parameters:
cy.mailpitHasEmailsBySubject('My Test', 0, 50, { timeout: 15000, interval: 2000 }).should('be.true');src/MailpitCommands.ts (3)
141-151: LGTM: New mailpitHasEmailsBySearch method implemented correctlyThe new method is well-implemented, using the
waitForConditionto provide polling behavior. The condition check formessages_count > 0is correct for a "has" method.Consider adding a JSDoc comment to describe the method's purpose and parameters, especially the
optionsobject, to improve code documentation.
167-178: LGTM: mailpitHasEmailsBySubject updated with polling behaviorThe method has been successfully updated to use the new
waitForConditionfunction, implementing the desired polling behavior. The condition checkresult.messages_count > 0is correct for a "has" method.Consider updating the JSDoc comment (if it exists) to reflect the new
optionsparameter and the polling behavior.
180-220: LGTM: Consistent updates to email checking methodsThe methods
mailpitNotHasEmailsBySubject,mailpitHasEmailsByTo, andmailpitNotHasEmailsByTohave been consistently updated to include theoptionsparameter and use thewaitForConditionfunction. The condition checks are correct for each method:
mailpitNotHasEmailsBySubject:result.messages_count === 0mailpitHasEmailsByTo:result.messages_count > 0mailpitNotHasEmailsByTo:result.messages_count === 0These changes successfully implement the desired polling behavior across all email checking methods.
Consider updating the JSDoc comments (if they exist) for these methods to reflect the new
optionsparameter and the polling behavior.cypress.d.ts (5)
31-43: Ensure Method Description Reflects Functionality AccuratelyThe method
mailpitHasEmailsBySearchis designed to check if Mailpit has any emails matching the search query. To improve clarity, consider rephrasing the description in line 31.Suggested change:
- * Check if mailpit has any email with the search query + * Check if Mailpit has any emails matching the search query.
60-73: Improve Consistency in Method DocumentationFor
mailpitHasEmailsBySubject, the description could be clearer and consistent with other method descriptions. Consider updating it to specify that it checks for emails with a specific subject.Suggested change:
- * Check if mails have emails using the subject. + * Check if Mailpit has any emails with the specified subject.
99-111: Enhance Clarity inmailpitHasEmailsByToDescriptionThe method
mailpitHasEmailsByTochecks for emails sent to a specific recipient. To improve understanding, refine the description accordingly.Suggested change:
- * Check if mails have emails sent to a specific email address. + * Check if Mailpit has any emails sent to the specified email address.
153-153: Ensure Consistent Capitalization in Method DescriptionsIn the description for
mailpitGetMailSpamAssassinSummary, verify that "SpamAssassin" is consistently capitalized according to official naming.
199-199: Clarify Parameter Description formailpitSetStatusAsReadThe parameter description mentions "Array of Message or MessageSummary objects," but the method accepts a single object or an array. Updating the description adds clarity.
Suggested change:
- * @param messages Array of Message or MessageSummary objects to mark as read + * @param messages Optional. A Message or MessageSummary object, or an array of them, to mark as read.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- README.md (1 hunks)
- cypress.d.ts (4 hunks)
- cypress/e2e/mailpit.cy.ts (1 hunks)
- src/MailpitCommands.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (5)
README.md (2)
199-205: Approve update.The update to
mailpitHasEmailsByTocommand aligns with the PR objectives. The description is clear and accurate, and the example usage effectively demonstrates the use of optional parameters.
171-219: Overall improvements to email search commands are valuable.The additions and updates to the email search commands in the README significantly enhance the functionality of the
cypress-mailpitpackage. These changes align well with the PR objectives and provide users with more flexible and robust email querying capabilities.Key improvements:
- Addition of
mailpitHasEmailsBySearchandmailpitNotHasEmailsBySearchcommands.- Updates to existing commands to include optional timeout and interval parameters.
- Consistent structure and descriptions for all commands.
While the overall changes are positive, please address the minor issues in descriptions and examples as noted in the previous comments to ensure complete accuracy and clarity for users.
src/MailpitCommands.ts (2)
22-23: LGTM: New commands added to parentCypressCommandsThe addition of "mailpitHasEmailsBySearch" and "mailpitNotHasEmailsBySearch" to the parentCypressCommands array is consistent with the new methods implemented in the class. The naming convention follows the existing pattern, maintaining consistency.
Line range hint
1-386: Overall assessment: Good implementation with minor improvements neededThe changes to
MailpitCommands.tssuccessfully implement new email search commands and update existing ones with polling behavior. The additions are consistent and well-structured. However, there are a few areas for improvement:
- Add JSDoc comments to new and modified methods for better documentation.
- Fix the condition in
mailpitNotHasEmailsBySearchmethod.- Refactor the
waitForConditionmethod to avoid potential issues with Cypress command queueing.After addressing these points, the implementation will be robust and maintainable.
cypress.d.ts (1)
135-135:⚠️ Potential issueFix Typo in Method Name
The method name
mailpitGetMailHTMlBodycontains a typo in "HTMl". It should be capitalized as "HTML" to reflect proper naming conventions.Apply this diff to correct the method name:
- mailpitGetMailHTMlBody(message?: Message): Chainable<string>; + mailpitGetMailHTMLBody(message?: Message): Chainable<string>;Likely invalid or redundant comment.
|
I will have a look at the new changes today evening (german time zone). |
|
I have also a question for another method. As I was running over the mailpit API documentation I found that a delete over search is also possible. https://mailpit.axllent.org/docs/api-v1/view.html#delete-/api/v1/search It would be nice to have this functionality also in this framework. "mailpitDeleteBySearch" In my testing I have a shared mailpit instance where I cannot delete all messages before a testrun. Deleting mails by search will enable me to design my tests much more nicely. With this new function I can delete mails by using subject, to, from, etc. . Should I open another feature request for this or could this be implemented with this branch? |
|
Running over the API I also saw a paramter in the search command. Timezone identifier used specifically for before: & after: searches (eg: "Pacific/Auckland"). tz: string Is this parameter relevant for the framework? |
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
I guess I can add timezone change in the same MR but I will add |
* feat(refactor): add retry ability on email assertion commands - Refactored the `mailpitHasEmailsBySubject` and `mailpitHasEmailsByTo` commands to include an optional timeout and interval for automatic retries. - Updated the `mailpitNotHasEmailsBySubject` and `mailpitNotHasEmailsByTo` commands to also support the optional timeout and interval parameters. -Added the `mailpitHasEmailsBySearch` and `mailpitNotHasEmailsBySearch` commands to also support the optional timeout and interval parameters. - Added a new private method `waitForCondition` to handle the retry logic for the email search commands. Closes #39
pushpak1300
deleted the
feat/add_timeout_and_retry_to_selected_commands
branch
mailpitHasEmailsBySubjectandmailpitHasEmailsByTocommands to include an optional timeout and interval for automatic retries.mailpitNotHasEmailsBySubjectandmailpitNotHasEmailsByTocommands to also support the optional timeout and interval parameters.waitForConditionto handle the retry logic for the email search commands.Closes #39
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation