Feat: track message view

[TatevikGr]

Summary by CodeRabbit

• New Features

  • Added 1x1 pixel message-open tracking endpoint to record when messages are viewed.

• Tests

  • Added integration tests validating pixel responses, headers, behavior with missing/unknown parameters, and included test fixtures linking messages and subscribers.

• Chores

  • Adjusted CI/config finishing touches related to docstring handling.

Thanks for contributing to phpList!

[coderabbitai]

Warning

Rate limit exceeded

@TatevikGr has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 2 minutes and 45 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)

Validation error: Unrecognized key(s) in object: 'checks'

⚙️ Configuration instructions

• Please see the configuration documentation for more information.
• You can also validate your configuration using the online YAML validator.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

📝 Walkthrough

Walkthrough

This PR adds a new tracking pixel endpoint handled by a MessageOpenTrackController at /t/open.gif. The controller accepts query parameters u (subscriber UID) and m (message ID); when present it collects request metadata (user agent, referer, IP), calls UserMessageService->trackUserMessageView, flushes the EntityManager, and logs exceptions. Regardless of outcome or missing params, it always returns a 1x1 GIF with headers preventing caching. Integration tests and a test fixture for UserMessage were added; a small config change to .coderabbit.yaml was included.

Sequence Diagram

sequenceDiagram
    participant Client as Browser/Client
    participant Controller as MessageOpenTrackController
    participant Service as UserMessageService
    participant EM as EntityManager
    participant DB as Database
    participant Logger as Logger

    Client->>Controller: GET /t/open.gif?u={uid}&m={id}
    activate Controller
    Controller->>Controller: extract metadata (UA, Referer, IP)

    alt uid and messageId present
        Controller->>Service: trackUserMessageView(uid, messageId, metadata)
        activate Service
        Service->>DB: update user_message record
        Service->>EM: persist changes
        deactivate Service
        Controller->>EM: flush()
    else missing params
        Controller->>Logger: note missing parameters
    end

    Controller->>Logger: log exceptions if any
    Controller->>Client: 1x1 GIF (Content-Type: image/gif, Cache-Control: no-store)
    deactivate Controller

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Feat: track message view' directly reflects the main functionality added: a new controller that tracks when messages are opened via a GET endpoint.
Merge Conflict Detection	✅ Passed	✅ No merge conflicts detected when merging into dev

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/track-message-view

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@src/Statistics/Controller/MessageOpenTrackController.php`:
- Around line 75-88: The catch block logs potentially sensitive PII (subscriber
UID, client IP and user-agent) — modify the error handling in
MessageOpenTrackController so you sanitize or omit PII before calling
$this->logger->error: for example, transform $uid and any IP fields inside
$metadata (and HTTP_USER_AGENT/HTTP_REFERER) to a hashed or truncated form or
remove them entirely, then pass the sanitized array into the existing
logger->error call (keeping message_id and non-PII context); ensure this change
is applied around the trackUserMessageView()/entityManager->flush() catch block
and any other logging that uses $metadata or $uid.

In `@tests/Integration/Statistics/Controller/MessageOpenTrackControllerTest.php`:
- Around line 67-74: The comment in testOpenGifMissingParametersReturns200Anyway
is contradictory: it states "should yield 400 Bad Request" while the test
asserts 200; update the comment to reflect the actual expected behavior or
adjust the controller. Specifically, either change the test comment to explain
why a 200 is intentional (e.g., open.gif endpoint should tolerate missing
MapQueryParameter and return a tracking pixel) or, if 400 is correct, modify the
controller handling of MapQueryParameter to return a 400 when required
non-nullable args are missing; locate the test method
testOpenGifMissingParametersReturns200Anyway and the MapQueryParameter usage in
the controller (or validation middleware) to apply the correct fix.

🧹 Nitpick comments (4)

src/Statistics/Controller/MessageOpenTrackController.php (3)

21-31: Consider not extending BaseController for this unauthenticated endpoint.

This tracking pixel endpoint is public (no auth required), yet it inherits BaseController which pulls in Authentication and RequestValidator as constructor dependencies. That's unnecessary coupling for a pixel endpoint. Extending AbstractController directly would be cleaner and make the intent clearer.

---

76-77: flush() in the controller is a layered-architecture smell.

The EntityManager::flush() call here suggests UserMessageService::trackUserMessageView() doesn't flush internally. Typically the service (or a transactional boundary) should own persistence. Having the controller call flush() means any future caller of the service must remember to flush too.

---

65-67: Falsy check on $messageId would reject messageId=0.

!$messageId evaluates to true for both null and 0. While message ID 0 is unlikely, a strict null check ($uid === null || $messageId === null) would be more precise and convey intent better.

Suggested fix

-        if (!$uid || !$messageId) {
+        if ($uid === null || $messageId === null) {

tests/Integration/Statistics/Controller/MessageOpenTrackControllerTest.php (1)

57-65: Consider asserting that the error was logged for unknown UID.

This test verifies the pixel is still returned (good), but doesn't verify that the failed tracking attempt was logged. If logging is important for observability, asserting on the logger output would strengthen this test.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/Statistics/Controller/MessageOpenTrackController.php`:
- Around line 60-67: The current guard in trackUserMessageView uses a falsy
check that treats messageId=0 as missing; change the condition to test for null
explicitly. Update the if in trackUserMessageView to use strict null checks
(e.g. $uid === null || $messageId === null) before calling
returnPixelResponse(), so a messageId of 0 is preserved while truly absent
parameters still trigger the pixel response.

🧹 Nitpick comments (1)

src/Statistics/Controller/MessageOpenTrackController.php (1)

20-31: Consider extending AbstractController directly—this endpoint doesn't need BaseController's auth/validator.

MessageOpenTrackController is correctly left unauthenticated (email clients can't send API keys), but BaseController exists specifically to provide those capabilities. Extending it here just means passing two unused dependencies up the constructor chain. Switching to AbstractController directly would simplify the code and make the intent (public, stateless tracking endpoint) more explicit.

♻️ Proposed refactor

-use PhpList\RestBundle\Common\Controller\BaseController;
-use PhpList\Core\Security\Authentication;
-use PhpList\RestBundle\Common\Validator\RequestValidator;
+use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;

 #[Route('/t', name: 'tracks_')]
-class MessageOpenTrackController extends BaseController
+class MessageOpenTrackController extends AbstractController
 {
     public function __construct(
-        Authentication $authentication,
-        RequestValidator $validator,
         private readonly UserMessageService $userMessageService,
         private readonly EntityManagerInterface $entityManager,
         private readonly LoggerInterface $logger,
     ) {
-        parent::__construct($authentication, $validator);
     }