[#284]: Updated EmailTrait. #362
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update focuses on extensive improvements to email-related Behat test steps and supporting code. Step definitions in documentation and implementation are standardized for clarity and consistency, adopting explicit "should" phrasing and more descriptive parameter names. New steps are introduced for enabling/disabling the test email system, asserting email content presence or absence, and handling email attachments and links with both exact and partial subject matching. The corresponding feature tests and documentation are updated to reflect these changes, and the mail handler is enhanced to support dynamic subjects for non-default email keys. A new test utility is added to send emails with attachments. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester as Behat Test
participant EmailTrait as EmailTrait
participant MailSystem as Mail Handler
Tester->>EmailTrait: Enable test email system
EmailTrait->>MailSystem: Set test email system enabled
Tester->>EmailTrait: Send email to address with subject/body/attachment
EmailTrait->>MailSystem: Send email with parameters
MailSystem-->>EmailTrait: Email is stored for test
Tester->>EmailTrait: Assert email should be sent to address
EmailTrait->>MailSystem: Retrieve sent emails
EmailTrait-->>Tester: Assertion result
Tester->>EmailTrait: Assert email field (body/header) should (not) contain/be value
EmailTrait->>MailSystem: Retrieve sent email
EmailTrait-->>Tester: Assertion result
Tester->>EmailTrait: Follow link number in email with (partial/exact) subject
EmailTrait->>MailSystem: Retrieve email by subject
EmailTrait->>Tester: Visit link in email body
Tester->>EmailTrait: Assert file attachment in email with (partial/exact) subject
EmailTrait->>MailSystem: Retrieve email by subject
EmailTrait-->>Tester: Assertion result
Suggested labels
🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.x #362 +/- ##
==========================================
+ Coverage 79.46% 80.48% +1.01%
==========================================
Files 30 30
Lines 1495 1532 +37
==========================================
+ Hits 1188 1233 +45
+ Misses 307 299 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 50994 @AlexSkrypnyk I've completed a full review of PR #362 which updates the EmailTrait component. Overall AssessmentThis is a well-structured PR that significantly improves the EmailTrait by modernizing its API, enhancing functionality, and improving test coverage. The changes align with what appears to be a broader effort to standardize the codebase as part of a v2 to v3 migration. Strengths
Areas for Improvement
ConclusionThis PR represents a significant improvement to the EmailTrait component, making it more robust, user-friendly, and maintainable. The consistent naming conventions and enhanced functionality will make it easier for developers to write and maintain Behat tests related to email functionality. I recommend approving this PR after addressing the test coverage issues highlighted by Codecov. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (5)
src/EmailTrait.php (5)
120-131:⚠️ Potential issueAvoid calling instance methods statically – switch to
$this->
emailAssertEmailShouldBeSentTo()invokes the non-static methodemailGetCollectedEmails()viaself::, which triggers a “non-static method called statically” deprecation notice in PHP 8 and will become a fatal error in PHP 9.- foreach (self::emailGetCollectedEmails() as $record) { + foreach ($this->emailGetCollectedEmails() as $record) {The same static invocation pattern appears in other assertion helpers (
emailAssertNoEmailsShouldBeSent,emailFind, etc.). Please revise them all to use$this->…for future-proof compatibility.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-129: src/EmailTrait.php#L129
Added line #L129 was not covered by tests
468-472:⚠️ Potential issueStatic call to non-static method inside
emailEnableTestEmailSystem()
self::emailClearTestEmailSystemQueue(TRUE);is another static call to a non-static method.
Replace with an instance call to prevent the same deprecation/fatal-error path:- self::emailClearTestEmailSystemQueue(TRUE); + $this->emailClearTestEmailSystemQueue(TRUE);
543-550:⚠️ Potential issuePotential undefined-variable and invalid
array_mapsyntax$query = Database::getConnection()->query(...); if ($query instanceof StatementInterface) { $emails = array_map(unserialize(...), $query->fetchAllKeyed()); } $emails = empty($emails['system.test_mail_collector']) ? [] : $emails['system.test_mail_collector'];
$emailsis only initialised inside theifblock—if the query fails, the later dereference raisesUndefined variable $emails.array_map(unserialize(...), …)is invalid PHP; you meant to pass the string'unserialize'.Suggested fix:
- if ($query instanceof StatementInterface) { - $emails = array_map(unserialize(...), $query->fetchAllKeyed()); - } - $emails = empty($emails['system.test_mail_collector']) ? [] : $emails['system.test_mail_collector']; + $emails = []; + if ($query instanceof StatementInterface) { + $emails = array_map('unserialize', $query->fetchAllKeyed()); + } + $emails = $emails['system.test_mail_collector'] ?? [];Without this, the trait will throw a fatal error the first time it runs.
587-593: 🛠️ Refactor suggestionPrefer
(string)$stringoverstrval($string)forPyStringNode
strval($string)on aPyStringNodereturns the class name, not its textual content.
Using the cast operator forces the node’s__toString()implementation and returns the real string:- $string = strval($string); + $string = (string) $string;This affects
emailFind()andemailAssertEmailFieldShouldNotContain()and will cause all content-matching assertions to fail silently.
289-303: 🧹 Nitpick (assertive)Guard against missing headers to avoid PHP notices
Inside
emailAssertEmailFieldShouldNotContain()you directly access$record[$field]; when$fieldissubjectorbodythat’s fine, but forto/fromthe key may be absent, leading toUndefined indexnotices.Add a null-coalescing fallback:
- $field_string = $exact ? $record[$field] : - trim((string) preg_replace('/\s+/', ' ', (string) $record[$field])); + $value = $record[$field] ?? ''; + $field_string = $exact ? $value : + trim((string) preg_replace('/\s+/', ' ', (string) $value));🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 300-300: src/EmailTrait.php#L300
Added line #L300 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
MIGRATION.md(2 hunks)src/EmailTrait.php(6 hunks)steps.md(1 hunks)tests/behat/bootstrap/FeatureContextTrait.php(1 hunks)tests/behat/features/email.feature(6 hunks)tests/behat/fixtures/d10/web/modules/custom/mysite_core/mysite_core.module(1 hunks)
🧰 Additional context used
🪛 LanguageTool
steps.md
[grammar] ~321-~321: Make sure that the adjective ‘exact’ is correct. Possibly, it should be an adverb (typically ~ly) that modifies ‘specified’. Possibly, it should be the first word in a compound adjective (hyphenated adjective). Possibly, it is correct.
Context: ... the email message header should be the exact specified content ```gherkin @then the email hea...
(ADVERB_OR_HYPHENATED_ADJECTIVE)
🪛 GitHub Check: codecov/patch
src/EmailTrait.php
[warning] 129-129: src/EmailTrait.php#L129
Added line #L129 was not covered by tests
[warning] 139-139: src/EmailTrait.php#L139
Added line #L139 was not covered by tests
[warning] 152-152: src/EmailTrait.php#L152
Added line #L152 was not covered by tests
[warning] 158-158: src/EmailTrait.php#L158
Added line #L158 was not covered by tests
[warning] 165-165: src/EmailTrait.php#L165
Added line #L165 was not covered by tests
[warning] 189-189: src/EmailTrait.php#L189
Added line #L189 was not covered by tests
[warning] 270-270: src/EmailTrait.php#L270
Added line #L270 was not covered by tests
[warning] 300-300: src/EmailTrait.php#L300
Added line #L300 was not covered by tests
[warning] 341-341: src/EmailTrait.php#L341
Added line #L341 was not covered by tests
[warning] 345-345: src/EmailTrait.php#L345
Added line #L345 was not covered by tests
[warning] 371-371: src/EmailTrait.php#L371
Added line #L371 was not covered by tests
[warning] 378-379: src/EmailTrait.php#L378-L379
Added lines #L378 - L379 were not covered by tests
[warning] 382-382: src/EmailTrait.php#L382
Added line #L382 was not covered by tests
[warning] 388-388: src/EmailTrait.php#L388
Added line #L388 was not covered by tests
[warning] 392-392: src/EmailTrait.php#L392
Added line #L392 was not covered by tests
[warning] 439-439: src/EmailTrait.php#L439
Added line #L439 was not covered by tests
[warning] 450-450: src/EmailTrait.php#L450
Added line #L450 was not covered by tests
🔇 Additional comments (23)
tests/behat/fixtures/d10/web/modules/custom/mysite_core/mysite_core.module (1)
26-30: Good enhancement for flexible email subject handling!Adding a default case to handle non-default email keys with dynamic subjects is a clean solution that increases flexibility. This change aligns well with the new email attachment functionality and supports more diverse testing scenarios.
tests/behat/bootstrap/FeatureContextTrait.php (1)
174-194: Well-implemented email attachment test method!The new
testSendEmailWithAttachmentmethod is a valuable addition that complements the existing email testing functionality. The implementation correctly:
- Uses descriptive parameter names
- Properly structures email params including attachments
- Uses a dedicated mail key for attachment testing
- Creates test attachment content based on the filename
This method aligns with the improvements to the mail handler and the new attachment assertion steps in EmailTrait.
tests/behat/features/email.feature (18)
12-14: Improved step definition with clearer, more consistent phrasingThe updated step phrase uses "should be sent" instead of "is sent" which is more consistent with BDD assertion patterns.
27-28: Improved step definition consistencyThe updated phrasing aligns with BDD best practices by using "should" for assertions.
32-43: Excellent clarity in positive and negative field assertionsThe revised assertions use clearer, more consistent wording with "should contain" vs "should not contain" and "should be" vs "should not be" for exact matches, making test intentions more explicit.
44-51: Well-structured content assertionsThese assertions verify both exact and approximate text matching scenarios, which provides more comprehensive testing coverage.
62-70: Improved email assertion stepsThe updated steps use more consistent phrasing and include header assertion steps that match the rest of the updated steps.
74-91: Good addition: Exact header matching scenarioThis new scenario verifies that email headers can be matched exactly, not just with partial content matching. This covers an important testing edge case.
92-119: Well-designed positive content assertion scenariosThese new scenarios provide comprehensive test coverage for different kinds of email content assertions:
- Exact content matching
- Content containing a substring
- Content not containing a substring
This allows for more precise testing of email content.
120-160: Important negative assertion scenariosThese new test scenarios cover negative assertions, verifying that emails with specific content are not sent to certain addresses. This completes the test matrix for email assertions.
170-179: Updated assertions with consistent phrasingThe revised assertions follow the new naming pattern consistently.
183-204: Good test for email system enabling and queue clearingThis scenario now explicitly enables the test email system first, which is more explicit and clear than relying on the @email tag alone.
214-217: Clearer link following stepThe updated step for following links in emails now explicitly includes the subject parameter in the step definition, making it easier to understand.
231-240: Good addition: Subject substring matching for link followingThis new scenario tests following links in emails by matching partial subjects, which is a useful practical case for testing emails with dynamic subjects.
243-254: Improved "no emails" assertionThe updated step uses "should be sent" rather than "were sent" for consistency with other assertions.
255-274: Good explicit test email system enable/verification testThis new scenario explicitly tests enabling the test email system and verifies it works correctly, which is important for validating the test infrastructure itself.
275-286: Important test for disabling the email systemThis scenario confirms that the test email system can be disabled explicitly, which complements the enabling test and provides complete coverage of the system's control functionality.
287-299: Good attachment verification testThis scenario tests the new attachment functionality, verifying that attachments can be correctly associated with emails by subject.
300-307: Useful partial subject matching for attachmentsThis scenario verifies that attachments can be identified in emails with subjects containing a substring, which is useful for dynamic subject scenarios.
315-321: Updated error message textThe error message text has been updated to match the new wording pattern, maintaining consistency throughout.
MIGRATION.md (2)
44-64: Well-documented EmailTrait migration mappingsThe new EmailTrait section in the migration document provides clear and comprehensive mappings from v2 to v3 step definitions. The documentation:
- Follows the established format of the document
- Clearly shows both renamed steps and new steps
- Reflects the standardization to "should" language
- Includes all the new steps for email verification, such as:
- Enabling/disabling the test email system
- Email content assertions with exact/substring matching
- Email links with partial subject matching
- Email attachment assertions
This documentation will be valuable for users migrating from v2 to v3.
99-99: Minor formatting fix in the JsTrait sectionThe formatting fix in the JsTrait section simplifies the step definition format from a regex pattern to a named parameter style, which is more consistent with the other step definitions.
src/EmailTrait.php (1)
456-467: Unit-test gap flagged by CodecovSeveral new control-flow branches (e.g.,
emailEnableTestEmailSystem()) lack coverage, notably the interaction with the mailsystem module. Consider adding a behavioural test that:
- Enables the test mail system via the new step,
- Sends a dummy email,
- Verifies the collector receives exactly one record,
- Disables the system and asserts the configuration is restored.
This will lock in the new functionality and raise patch coverage.
steps.md
Outdated
| #### Assert that the email message header should be the exact specified content | ||
|
|
||
| ```gherkin | ||
| @Then an email header :header contains exact: | ||
| @Then the email header :header should be: | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider smoothing the wording for clarity
The phrase “the email message header should be the exact specified content” is a bit awkward.
A more idiomatic alternative would be:
@Then the email header :header should exactly be:(or “… should be exactly:”)
Minor, but polishing wording here keeps the step catalogue consistently readable.
🧰 Tools
🪛 LanguageTool
[grammar] ~321-~321: Make sure that the adjective ‘exact’ is correct. Possibly, it should be an adverb (typically ~ly) that modifies ‘specified’. Possibly, it should be the first word in a compound adjective (hyphenated adjective). Possibly, it is correct.
Context: ... the email message header should be the exact specified content ```gherkin @then the email hea...
(ADVERB_OR_HYPHENATED_ADJECTIVE)
|
@skipper-vp |
src/EmailTrait.php
Outdated
| * @When I disable the test email system | ||
| */ | ||
| public function emailDisableTestEmailSystemStep(): void { | ||
| $this->emailDisableTestEmailSystem(); |
There was a problem hiding this comment.
can we put the step on emailDisableTestEmailSystem() directly?
|
@AlexSkrypnyk All items addressed, please have a look. |
[#284]: Updated EmailTrait.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes