Skip to content

Improve Marketing message event log handling #14583

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Mar 13, 2025
Merged

Improve Marketing message event log handling #14583

merged 4 commits into from
Mar 13, 2025

Conversation

nileshlohar
Copy link
Contributor

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? 🔴
Related user documentation PR URL mautic/user-documentation#...
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #...

Description

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Create a Marketing Messages with Multiple Channels like SMS and EMail.
  3. Send the above Marketing Message via Campaign and it should work as expected.

@escopecz escopecz added bug Issues or PR's relating to bugs unforking Used for PRs in the Acquia's unforking initiative marketing-messages labels Feb 11, 2025
@escopecz escopecz requested review from a team, rohitpavaskar and aarohiprasad and removed request for a team February 11, 2025 13:39
@codecov Codecov
Copy link

codecov bot commented Feb 11, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 64.68%. Comparing base (d84cbc9) to head (9b75f44).
Report is 5 commits behind head on 6.x.

Files with missing lines Patch % Lines
app/bundles/CampaignBundle/Event/PendingEvent.php 75.00% 1 Missing ⚠️
...ChannelBundle/EventListener/CampaignSubscriber.php 85.71% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                6.x   #14583      +/-   ##
============================================
- Coverage     64.69%   64.68%   -0.01%     
- Complexity    34697    34704       +7     
============================================
  Files          2274     2274              
  Lines        103648   103658      +10     
============================================
+ Hits          67050    67055       +5     
- Misses        36598    36603       +5
Files with missing lines Coverage Δ
app/bundles/CampaignBundle/Event/PendingEvent.php 79.01% <75.00%> (-4.11%) ⬇️
...ChannelBundle/EventListener/CampaignSubscriber.php 96.05% <85.71%> (-1.10%) ⬇️
🚀 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.

rohitpavaskar
rohitpavaskar approved these changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@rohitpavaskar rohitpavaskar left a comment

Choose a reason for hiding this comment

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

Thanks @nileshlohar!

@rohitpavaskar rohitpavaskar removed their assignment Feb 20, 2025
aarohiprasad
aarohiprasad approved these changes Mar 12, 2025
View reviewed changes
Copy link
Contributor

@aarohiprasad aarohiprasad left a comment

Choose a reason for hiding this comment

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

This works as expected, thanks @nileshlohar
image

@aarohiprasad aarohiprasad added the ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged label Mar 12, 2025
@escopecz escopecz added this to the 6.0.0-RC milestone Mar 12, 2025
@RCheesley
Copy link
Member

@escopecz it's not clear to me if code review has been done here?

@RCheesley RCheesley added the user-testing-passed PRs which have been successfully tested by the required number of people. label Mar 12, 2025
@escopecz
Copy link
Member

@rohitpavaskar please update your comment with what was done from your side, whether it was a code review or a test or both.

@escopecz escopecz assigned rohitpavaskar and unassigned escopecz Mar 13, 2025
@rohitp19
Copy link
Contributor

I have reviewed the PR. It looks good to me.

@escopecz escopecz added the code-review-passed PRs which have passed code review label Mar 13, 2025
@escopecz escopecz merged commit 900e177 into mautic:6.x Mar 13, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@rohitpavaskar rohitpavaskar rohitpavaskar approved these changes

@aarohiprasad aarohiprasad aarohiprasad approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@rohitpavaskar rohitpavaskar

Labels
bug Issues or PR's relating to bugs code-review-passed PRs which have passed code review marketing-messages ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged unforking Used for PRs in the Acquia's unforking initiative user-testing-passed PRs which have been successfully tested by the required number of people.
Projects
None yet
Milestone
6.0.0-rc
Development

Successfully merging this pull request may close these issues.
6 participants
@nileshlohar @RCheesley @escopecz @rohitp19 @rohitpavaskar @aarohiprasad