Skip to content

Refactor onCampaignTriggerActionUpdateLead to use batches #14495

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
Feb 5, 2025
Merged

Refactor onCampaignTriggerActionUpdateLead to use batches #14495

merged 4 commits into from
Feb 5, 2025

Conversation

rohitpavaskar
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 campaign, in the campaign there you add the action "Update contact"
  3. If you are on local, run bin/console mautic:campaigns:rebuild, bin/console mautic:campaigns:trigger
  4. The related contacts should be updated

@codecov Codecov
Copy link

codecov bot commented Jan 22, 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.03%. Comparing base (29b01fe) to head (10bef23).
Report is 64 commits behind head on 6.x.

Files with missing lines Patch % Lines
...es/LeadBundle/EventListener/CampaignSubscriber.php 81.81% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##                6.x   #14495   +/-   ##
=========================================
  Coverage     64.03%   64.03%           
- Complexity    34653    34667   +14     
=========================================
  Files          2287     2287           
  Lines        103717   103763   +46     
=========================================
+ Hits          66418    66449   +31     
- Misses        37299    37314   +15
Files with missing lines Coverage Δ
...es/LeadBundle/EventListener/CampaignSubscriber.php 73.82% <81.81%> (-0.49%) ⬇️

... and 18 files with indirect coverage changes

@escopecz
Copy link
Member

@rohitpavaskar I would normally recommend a PHPSTAN ignore comment but since this is in multiple places I think it should be ignored in https://github.com/mautic/mautic/blob/6.x/phpstan.neon#L29. It's still good as new subscribers won't be using the deprecated event

@rohitpavaskar rohitpavaskar requested review from a team and rahuld-dev and removed request for a team January 24, 2025 07:32
@rohitpavaskar rohitpavaskar added the unforking Used for PRs in the Acquia's unforking initiative label Jan 24, 2025
@escopecz escopecz added ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging refactoring The change does not change behavior but improves the code labels Jan 31, 2025
rahuld-dev
rahuld-dev approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@rahuld-dev rahuld-dev left a comment

Choose a reason for hiding this comment

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

Thanks @rohitp19
Code changes looks good to me, also i tested it on local env it is working as expected.

@rahuld-dev rahuld-dev 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 Feb 4, 2025
@escopecz escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label Feb 4, 2025
escopecz
escopecz approved these changes Feb 5, 2025
View reviewed changes
Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

Thanks Rohit!

  • New tests were added and the old ones were simplified with reducing duplicated code
  • The phpstan baseline was regenerated as on event was marked as deprecated.

All good 👍

@escopecz escopecz removed ready-to-test PR's that are ready to test pending-feedback PR's and issues that are awaiting feedback from the author labels Feb 5, 2025
@escopecz escopecz added this to the 6.0.0-beta milestone Feb 5, 2025
@escopecz escopecz merged commit 720c995 into mautic:6.x Feb 5, 2025
16 of 17 checks passed
@RCheesley RCheesley modified the milestones: 6.0.0-beta, 6.0.0-beta2 Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@escopecz escopecz escopecz approved these changes

+2 more reviewers

@rohitp19 rohitp19 rohitp19 left review comments

@rahuld-dev rahuld-dev rahuld-dev approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
code-review-passed PRs which have passed code review refactoring The change does not change behavior but improves the code unforking Used for PRs in the Acquia's unforking initiative
Projects
Open Source Fridays
Status: 🥳 Done
Milestone
6.0.0-beta2
Development

Successfully merging this pull request may close these issues.
5 participants
@rohitpavaskar @escopecz @rohitp19 @rahuld-dev @RCheesley