Skip to content

Conversation

biozshock
Copy link
Contributor

@biozshock biozshock commented May 28, 2025

Q A
Bug fix? (use the a.b branch) ✔️
New feature/enhancement? (use the a.x branch)
Deprecations?
BC breaks? (use the c.x branch)
Automated tests included? ✔️

Description

PR fixes #14976

The PR also fixes a couple of issues (covered by test):

  1. Allow ReportModel to be tested: check if the session was started, and only then grab values from it. Prevents the exception Failed to start the session because headers have already been sent by "/var/www/html/vendor/phpunit/phpunit/src/Util/Printer.php" at line 104.
  2. Do not remove attachments for Report straight away, because, in case of the message being send asynchronously, the file must be present at the time of actually sending the message.

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Please refer to the issue Scheduled Report Emails fail if the system is set to "Queue" #14976 description.

Copy link

codecov bot commented May 29, 2025

Codecov Report

Attention: Patch coverage is 91.42857% with 3 lines in your changes missing coverage. Please review.

Project coverage is 63.75%. Comparing base (9b82e89) to head (a421e8a).
Report is 31 commits behind head on 5.2.

Files with missing lines Patch % Lines
...e/MessageHandler/RemoveReportAttachmentHandler.php 81.25% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.2   #15052      +/-   ##
============================================
+ Coverage     63.72%   63.75%   +0.02%     
- Complexity    34687    34698      +11     
============================================
  Files          2274     2275       +1     
  Lines        103776   103804      +28     
============================================
+ Hits          66131    66176      +45     
+ Misses        37645    37628      -17     
Files with missing lines Coverage Δ
app/bundles/ReportBundle/Form/Type/ReportType.php 98.05% <100.00%> (+0.16%) ⬆️
app/bundles/ReportBundle/Model/ReportModel.php 86.25% <100.00%> (+0.81%) ⬆️
...dles/ReportBundle/Scheduler/Model/SendSchedule.php 100.00% <ø> (ø)
...e/MessageHandler/RemoveReportAttachmentHandler.php 81.25% <81.25%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@biozshock biozshock force-pushed the fix-14976 branch 12 times, most recently from 63cab7b to de05275 Compare June 12, 2025 20:12
@npracht npracht linked an issue Jun 13, 2025 that may be closed by this pull request
1 task
@npracht
Copy link
Member

npracht commented Jun 13, 2025

@biozshock could you please review CI ? Tests are failing. We'll close the next version in a few days. Thanks.

@npracht npracht added T2 Medium difficulty to fix (issue) or test (PR) code-review-needed PR's that require a code review before merging reports Anything related to reports pending-feedback PR's and issues that are awaiting feedback from the author labels Jun 13, 2025
@biozshock biozshock force-pushed the fix-14976 branch 2 times, most recently from e9d4a83 to f86e4e0 Compare June 13, 2025 15:07
@biozshock
Copy link
Contributor Author

@npracht That's a known issue with M5 tests ran between 21:00 and 2:00 UTC. Just need to restart the build.

Copy link
Contributor

@oltmanns-leuchtfeuer oltmanns-leuchtfeuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and fixed the issue - LGTM! :)

@RCheesley RCheesley removed the pending-feedback PR's and issues that are awaiting feedback from the author label Jun 23, 2025
@RCheesley RCheesley added the user-testing-passed PRs which have been successfully tested by the required number of people. label Jun 23, 2025
Copy link

@RCheesley RCheesley requested a review from a team June 23, 2025 13:21
Copy link
Contributor

@JonasLudwig1998 JonasLudwig1998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. The code looks fine from my point of view. Can be merged

@oltmanns-leuchtfeuer oltmanns-leuchtfeuer added code-review-passed PRs which have passed code review and removed code-review-needed PR's that require a code review before merging labels Jun 30, 2025
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me
I am just bit aware of do that operation also for each email with attachment. With mass volume sending of email it's not necessary. Would consider add check if it's report attachment.

@biozshock WDYT?

@kuzmany kuzmany added this to the 5.2.7 milestone Jun 30, 2025
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve it , it fixed what we need.
My concern is a note, break nothing

@kuzmany kuzmany merged commit a50f3dd into mautic:5.2 Jun 30, 2025
19 checks passed
@kuzmany kuzmany changed the title Report saved with proper options. Fix report scheduled emails not being sent with proper options Jun 30, 2025
@kuzmany kuzmany added the bug Issues or PR's relating to bugs label Jun 30, 2025
kuzmany added a commit to kuzmany/mautic that referenced this pull request Jun 30, 2025
The test was failing because it was checking for messages after they were consumed and removed by messenger:consume. The fix checks for the queued message before consumption, which properly validates that the delayed transport functionality is working.

- Remove flawed logic that checked for messages after messenger:consume
- Add proper validation by checking for queued messages before consumption
- This correctly tests the delayed transport feature from PR mautic#15052
- Files existence is validated at the appropriate time in the test flow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs code-review-passed PRs which have passed code review reports Anything related to reports T2 Medium difficulty to fix (issue) or test (PR) user-testing-passed PRs which have been successfully tested by the required number of people.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scheduled Report Emails fail if the system is set to "Queue"
6 participants