Feat/email forwarding #377
Open
+1,310
−224
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. 📝 WalkthroughWalkthroughAdds message-forwarding: new forwarding services, guards, stats, builders, DTOs, repositories, exceptions, admin notifications, placeholder/URL adjustments for forwarded context, configuration flags, DI/service wiring changes, and corresponding tests and translations. Changes
Sequence Diagram(s)sequenceDiagram
actor User as Forwarding Subscriber
participant MessageForwardService
participant ForwardingGuard
participant MessagePrecacheService
participant ForwardContentService
participant ForwardEmailBuilder
participant ForwardDeliveryService
participant AdminNotifier
participant DB as Database
User->>MessageForwardService: forward(MessageForwardDto)
MessageForwardService->>ForwardingGuard: assertCanForward(uid, campaign, cutoff)
ForwardingGuard->>DB: lookup subscriber / received message / count forwards
DB-->>ForwardingGuard: subscriber/message/counts
ForwardingGuard-->>MessageForwardService: allowed subscriber
loop per recipient
MessageForwardService->>ForwardingGuard: hasAlreadyBeenSent(email, campaign)?
ForwardingGuard->>DB: check forward records
DB-->>ForwardingGuard: exists?
alt not sent
MessageForwardService->>MessagePrecacheService: precache message
MessagePrecacheService-->>MessageForwardService: precached data
MessageForwardService->>ForwardContentService: getContents(campaign, subscriber, email, DTO)
ForwardContentService->>ForwardEmailBuilder: buildForwardEmail(...)
ForwardEmailBuilder-->>ForwardContentService: Email + OutputFormat
ForwardContentService-->>MessageForwardService: Email
MessageForwardService->>ForwardDeliveryService: send(Email)
ForwardDeliveryService->>DB: persist/send record
ForwardDeliveryService-->>MessageForwardService: ack
MessageForwardService->>AdminNotifier: notifyForwardSucceeded(...)
else already sent
MessageForwardService-->>MessageForwardService: skip
end
end
MessageForwardService->>DB: update forwarding subscriber friend count
DB-->>MessageForwardService: ack
MessageForwardService-->>User: complete
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@composer.json`:
- Around line 87-88: The composer.json lists "phpdocumentor/reflection-docblock"
at version ^5.2 but no runtime usage was found; either remove that dependency
from the "require" block or move it to "require-dev" if it is only used for
tooling (e.g., PHPStan or docs), and if you intend to keep it update the version
constraint to the current stable "6.0.1"; modify the composer.json entry
accordingly and run composer update to ensure lockfile consistency.
🧹 Nitpick comments (1)
src/Domain/Common/ExternalImageService.php (1)
150-166: Minor inconsistency:file_get_contentsfallback doesn't respect$verifySsl.The cURL path now respects the
verifySslconfig, but thefile_get_contentsfallback still uses PHP's default SSL behavior. If someone setsverify_ssl=falseexpecting to bypass verification, this fallback path would still verify (or vice versa depending on PHP config).Not a blocker, but worth considering for consistency if both paths should behave the same way.
🔧 Optional: Add SSL context to file_get_contents
private function downloadUsingFileGetContent(string $filename): string { $remoteURLContext = stream_context_create([ 'http' => [ 'method' => 'GET', 'timeout' => $this->externalImageTimeout, 'max_redirects' => '10', - ] + ], + 'ssl' => [ + 'verify_peer' => $this->verifySsl, + 'verify_peer_name' => $this->verifySsl, + ], ]); $cacheFileContent = file_get_contents($filename, false, $remoteURLContext);
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
TatevikGr
force-pushed
the
feat/email-forwarding
branch
from
215071b to
a736ce2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php`:
- Around line 32-33: The injected config key name is misleading: change the
Autowire attribute on the CampaignMailContentBuilder constructor parameter
forwardPersonalNote from '%phplist.forward_personal_note_size%' to a
boolean-named key like '%phplist.forward_personal_note_enabled%'; update any
config definitions, fixtures or tests that set forward_personal_note_size to use
forward_personal_note_enabled, and keep the parameter type (?bool
$forwardPersonalNote = false) and its usage in the builder (where it gates
inclusion of the forwarded note) unchanged so the name reflects boolean
semantics.
In `@src/Domain/Messaging/Service/ForwardingStatsService.php`:
- Around line 40-60: Both incrementFriendsCount and updateFriendsCount must
guard against null subscriber IDs to avoid cache collisions for transient
entities: in incrementFriendsCount and updateFriendsCount, after calling
Subscriber::getId() store it in $subscriberId and immediately return if
$subscriberId === null; this prevents comparing null to
$this->friendCountSubscriberId and stops reusing $this->friendCount for
different transient subscribers, preserving correct behavior of the
$this->friendCount cache and related logic.
src/Domain/Messaging/Service/Constructor/CampaignMailContentBuilder.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Domain/Configuration/Service/Placeholder/FooterValueResolver.php`:
- Around line 26-28: The early return when $ctx->forwardedBy() === null accesses
$ctx->messagePrecacheDto->textFooter / ->htmlFooter without a null check; update
the condition to also ensure $ctx->messagePrecacheDto is truthy (e.g., change
the if to check both $ctx->forwardedBy() === null && $ctx->messagePrecacheDto)
so the ternary that uses $ctx->messagePrecacheDto->textFooter / ->htmlFooter is
only executed when that DTO exists (look for this check pattern used later in
the file and apply the same guard in FooterValueResolver around the existing
isText() ternary).
In `@src/Domain/Identity/Service/AdminCopyEmailSender.php`:
- Around line 57-62: The mapping over $lists in AdminCopyEmailSender (inside the
$this->sendListAdminCopy branch) calls $list->getOwner()->getEmail() without
guarding for a null owner; change the logic to first filter or skip
SubscriberList instances where $list->getOwner() is null and then map only those
with a non-null owner to their emails, ensuring the resulting $emails is an
array (possibly empty) of strings; update the mapping around $lists and
references to getOwner()/getEmail() accordingly so no direct method call occurs
on a null owner.
🧹 Nitpick comments (3)
src/Domain/Messaging/Service/ForwardDeliveryService.php (1)
39-57: Consider using constants or an enum for status values.The magic strings
'sent'and'failed'are used here and likely referenced elsewhere. Extracting them to constants (or a PHP 8.1+ enum) would improve type safety and make refactoring easier down the road.💡 Example using class constants
class ForwardDeliveryService { + public const STATUS_SENT = 'sent'; + public const STATUS_FAILED = 'failed'; + public function __construct( // ... } public function markSent(Message $campaign, Subscriber $subscriber, string $friendEmail): void { $this->messageForwardManager->create( subscriber: $subscriber, campaign: $campaign, friendEmail: $friendEmail, - status: 'sent' + status: self::STATUS_SENT ); } public function markFailed(Message $campaign, Subscriber $subscriber, string $friendEmail): void { $this->messageForwardManager->create( subscriber: $subscriber, campaign: $campaign, friendEmail: $friendEmail, - status: 'failed' + status: self::STATUS_FAILED ); } }src/Domain/Messaging/Service/ForwardingGuard.php (1)
47-52: Consider extracting status string to a constant.The
'sent'status string appears here and likely in other places. A shared constant (or enum) would prevent typos and make the status values discoverable.Optional improvement
+ private const STATUS_SENT = 'sent'; + public function hasAlreadyBeenSent(string $friendEmail, Message $campaign): bool { $existing = $this->forwardRepository->findByEmailAndMessage($friendEmail, $campaign->getId()); - return $existing !== null && $existing->getStatus() === 'sent'; + return $existing !== null && $existing->getStatus() === self::STATUS_SENT; }src/Domain/Messaging/Service/ForwardContentService.php (1)
40-40: TODO noted - link handling for forwards.Worth tracking this as an issue so it doesn't get lost. Forwarded messages might need special treatment for click tracking or unsubscribe links.
Would you like me to open an issue to track the link handling investigation for forwarded messages?
src/Domain/Configuration/Service/Placeholder/FooterValueResolver.php
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Configuration
Internationalization
Thanks for contributing to phpList!