Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughMessage recipient handling was refactored: MessagePrecacheDto's Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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
🤖 Fix all issues with AI agents
In `@src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php`:
- Around line 243-247: The call to
$this->configProvider->getValue(ConfigOption::ReportAddress) returns ?string but
is passed into SystemEmailBuilder::buildCampaignEmail which expects string,
risking a TypeError; update CampaignProcessorMessageHandler:: (the block
creating $email) to guard or normalize the value—either coalesce to a safe
default/email (e.g. null-coalesce) or validate and throw/log if missing before
calling SystemEmailBuilder::buildCampaignEmail, ensuring the $toEmail argument
is always a non-null string.
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 (3)
src/Domain/Subscription/Model/Subscriber.php (1)
112-120:⚠️ Potential issue | 🔴 CriticalFix the mock in SubscriberBlacklistManagerTest.php — it needs
disableOriginalConstructor().The constructor now requires
string $email, which breaks one test file:tests/Unit/Domain/Subscription/Service/Manager/SubscriberBlacklistManagerTest.phpat line 132. The mock builder there is missing->disableOriginalConstructor(), so it tries to invoke the real constructor without the required parameter.All other test mocks already have it in place. Just add the call to that one chain.
tests/Unit/Domain/Messaging/Service/ForwardContentServiceTest.php (1)
55-66:⚠️ Potential issue | 🟡 MinorFix argument order in
testThrowsWhenCacheMissing.The 3rd and 4th arguments to
MessageForwardDtoare swapped in this test. The constructor expects(emails, uid, fromName, fromEmail, note), but the first call has them as(emails, uid, 'from@example.com', 'From', note). The second test method shows the correct order:(emails, uid, 'From Name', 'from@example.com', note). While the exception fires before these values are accessed, the test data should still match the intended parameter types for clarity.tests/Unit/Domain/Subscription/Model/SubscriberTest.php (1)
71-74:⚠️ Potential issue | 🔴 CriticalTest will fail — constructor now sets the email.
The pipeline confirms this:
testGetEmailInitiallyReturnsEmptyStringasserts''butgetEmail()returns'test@example.com'because the constructor now accepts and stores the email. Either update the assertion to match the constructor argument, or rename/remove this test since "initially returns empty string" no longer applies.Proposed fix
- public function testGetEmailInitiallyReturnsEmptyString(): void + public function testGetEmailInitiallyReturnsConstructorValue(): void { - self::assertSame('', $this->subscriber->getEmail()); + self::assertSame('test@example.com', $this->subscriber->getEmail()); }
🤖 Fix all issues with AI agents
In `@src/Domain/Messaging/Service/Builder/EmailBuilder.php`:
- Around line 117-122: The test failing with AttachmentException is hitting the
new subscriber lookup in EmailBuilder (subscriberRepository->findOneByEmail) and
throwing SubscriberNotFoundException before attachment logic runs; update the
failing test to stub/mocking of subscriberRepository->findOneByEmail to return a
valid Subscriber instance (or otherwise seed a Subscriber) so attachment path
executes and AttachmentException can be asserted; locate usage of EmailBuilder
and its dependency injection in the test, mock the repository method to return a
Subscriber object instead of null, and rerun the attachment tests.
🧹 Nitpick comments (3)
src/Domain/Subscription/Service/Manager/SubscriberManager.php (1)
43-53: Minor inconsistency in persist strategy (pre-existing).
createSubscriberdelegates to$this->subscriberRepository->persist()whilecreateFromImportcalls$this->entityManager->persist()directly. Not introduced by this PR, but worth aligning when convenient.Also applies to: 106-121
tests/Integration/Domain/Subscription/Fixtures/SubscriberFixture.php (1)
31-38: Pre-existing:$headersfromfgetcsvisn't validated before use inarray_combine.If the CSV is empty or malformed,
fgetcsvreturnsfalse/null/[], andarray_combine($headers, $data)on line 38 would blow up. Not introduced by this PR, but worth a quick guard if you're in the neighborhood.Suggested guard
$headers = fgetcsv($handle); + if ($headers === false || $headers === [null]) { + fclose($handle); + throw new RuntimeException(sprintf('Fixture file "%s" has no valid header row.', $csvFile)); + }src/Domain/Messaging/Service/Builder/EmailBuilder.php (1)
119-121: Consider not embedding the email address in the exception message.Including the raw email in the exception string (
sprintf('Subscriber with email %s not found', $toEmail)) risks leaking PII into logs or error-reporting systems. A more privacy-friendly approach would be to use a generic message and pass context separately if needed for debugging.Suggested diff
- throw new SubscriberNotFoundException( - sprintf('Subscriber with email %s not found', $toEmail) - ); + throw new SubscriberNotFoundException('Subscriber not found for the given email');
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php (1)
271-284:⚠️ Potential issue | 🟡 MinorNotification addresses from
explodemay carry whitespace.
explode(',', ...)on"alice@example.com, bob@example.com"yields" bob@example.com"with a leading space. Passing that straight intotoEmailcould cause address-parsing issues downstream. A quicktrim()keeps things tidy.Suggested tweak
$notifications = explode(',', $loadedMessageData['notify_start']); foreach ($notifications as $notification) { + $notification = trim($notification); + if ($notification === '') { + continue; + } $data = new MessagePrecacheDto();tests/Unit/Domain/Messaging/Service/Builder/EmailBuilderTest.php (1)
171-175:⚠️ Potential issue | 🟠 MajorMock expectation mismatch —
__invokereceives 3 arguments, but test expects only 1.The implementation at
EmailBuilder.phplines 124-128 invokesmailContentBuilderwith three named arguments (messagePrecacheDto,receiver,campaignId), but the test mock at line 174 is set up with->with($dto), which PHPUnit enforces as exactly one argument. This will fail at runtime.Proposed fix
$this->mailConstructor ->expects($this->once()) ->method('__invoke') - ->with($dto) + ->with($dto, $this->isInstanceOf(Subscriber::class), 777) ->willReturn(['<p>HTML</p>', 'TEXT']);
🧹 Nitpick comments (1)
tests/Unit/Domain/Messaging/Service/Builder/EmailBuilderTest.php (1)
413-414: Nit: stray blank changed line.Line 414 is marked as changed but is just a blank line — no functional issue, just noise in the diff.
2 participants
Summary by CodeRabbit
Thanks for contributing to phpList!