Skip to content
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

New campaign condition: Contacts campaign #4800

Merged
merged 7 commits into from Mar 30, 2018
Merged

New campaign condition: Contacts campaign #4800

merged 7 commits into from Mar 30, 2018

Conversation

kuzmany
Copy link
Member

@kuzmany kuzmany commented Sep 6, 2017 •
edited

Q A
Bug fix?
New feature? y
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs) #4137 #5246
BC breaks?
Deprecations?

Description:

PR added new campaign condition based on campaign membership and date added to campaign.

Use cases

  • My campaign running some time and I want to add some stuff just for contacts added after some day
  • My campaign running some time and I want to add some stuff just with contacts added years ago ...
  • I want to run campaign just for contacts they aren't in another campaigns

image

Steps to test this PR:

  1. Create campagin with new condition Contacts campaign
  2. Test membership and date added limit also

@kuzmany kuzmany mentioned this pull request Nov 6, 2017
Closed
@alanhartless alanhartless added the feature A new feature for inclusion in minor or major releases label Dec 13, 2017
@kuzmany
Copy link
Member Author

kuzmany commented Jan 29, 2018

Label: Ready to test

@mautibot mautibot added the ready-to-test PR's that are ready to test label Jan 29, 2018
@dbhurley dbhurley added this to the 2.13.0 milestone Mar 4, 2018
escopecz
escopecz previously requested changes Mar 28, 2018
View reviewed changes
app/bundles/CampaignBundle/Entity/LeadRepository.php Outdated
if (!empty($options['dataAddedLimit'])) {
$q->andWhere(
$q->expr()->{$options['expr']}('l.date_added', ':dateAdded')
)->setParameter('dateAdded', $options['dateAdded']);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Move the 2 lines above 4 spaces to the left.

app/bundles/LeadBundle/Config/config.php Outdated
@@ -596,6 +597,11 @@
'class' => 'Mautic\LeadBundle\Form\Type\CampaignEventLeadSegmentsType',
'alias' => 'campaignevent_lead_segments',
],
'mautic.form.type.campaignevent_lead_campaigns' => [
'class' => 'Mautic\LeadBundle\Form\Type\CampaignEventLeadCampaignsType',
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Use ::class instead of string

app/bundles/CampaignBundle/Entity/LeadRepository.php Outdated
$q->expr()->eq('l.lead_id', ':leadId'),
$q->expr()->in('l.campaign_id', $options['campaigns'])
)
);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Move the 5 lines above 4 spaces to the left.

Use $q->expr()->in('l.campaign_id', ':campaignIds') and $q->setParameter('campaignIds', $options['campaigns'], \Doctrine\DBAL\Connection::PARAM_INT_ARRAY) to make this secure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's review now.

@mautibot mautibot added the pending-test-confirmation PR's that require one test before they can be merged label Mar 28, 2018
@escopecz escopecz added pending-feedback PR's and issues that are awaiting feedback from the author ready-to-test PR's that are ready to test and removed pending-test-confirmation PR's that require one test before they can be merged ready-to-test PR's that are ready to test pending-feedback PR's and issues that are awaiting feedback from the author labels Mar 28, 2018
@Maxell92
Copy link
Contributor

Hello @kuzmany,

could you please fix CS fixer issues?

@escopecz escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label Mar 29, 2018
Maxell92
Maxell92 reviewed Mar 29, 2018
View reviewed changes
Copy link
Contributor

@Maxell92 Maxell92 left a comment

Choose a reason for hiding this comment

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

PR is working for me. CS fixer issues has to be resolved only. I will approve PR then.

@Maxell92 Maxell92 self-assigned this Mar 29, 2018
@mleffler
Copy link
Contributor

@Maxell92 Since kuzmany is on vacation, can we do the code formatting for him? Looks straightforward, and it's unfortunate timing we just realized this when he is out.

@Maxell92 Maxell92 force-pushed the date-added-to-campaign-condition branch from 07d7784 to b8fe681 Compare March 30, 2018 11:22
Maxell92
Maxell92 approved these changes Mar 30, 2018
View reviewed changes
Copy link
Contributor

@Maxell92 Maxell92 left a comment

Choose a reason for hiding this comment

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

Rebased and fixed CS fixer issue.

@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Mar 30, 2018
@Maxell92 Maxell92 added pending-test-confirmation PR's that require one test before they can be merged and removed code-review-needed PR's that require a code review before merging pending-feedback PR's and issues that are awaiting feedback from the author labels Mar 30, 2018
@mleffler mleffler assigned alanhartless and unassigned Maxell92 Mar 30, 2018
@mleffler mleffler merged commit 6f4f244 into mautic:staging Mar 30, 2018
@dbhurley dbhurley removed the ready-to-test PR's that are ready to test label Mar 30, 2018
@escopecz escopecz removed the pending-test-confirmation PR's that require one test before they can be merged label Mar 30, 2018
@kuzmany kuzmany deleted the date-added-to-campaign-condition branch April 7, 2018 17:53
@tony-brandner
Copy link

What happened with this merge? Does this mean it was released?

@escopecz
Copy link
Sponsor Member

@tony-brandner you can see the release in the right hand column as the milestone. It was merged to release 2.13.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Maxell92 Maxell92 Maxell92 approved these changes

@escopecz escopecz escopecz left review comments

Assignees

@alanhartless alanhartless

Labels
feature A new feature for inclusion in minor or major releases
Projects
None yet
Milestone
2.13.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@kuzmany @Maxell92 @mleffler @tony-brandner @escopecz @alanhartless @dbhurley @mautibot