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.

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

Date condition on company field #7541

Merged
merged 3 commits into from
Jun 28, 2020

Conversation

escopecz
Copy link
Sponsor Member

@escopecz escopecz commented May 17, 2019

Please be sure you are submitting this against the staging branch.

Q A
Bug fix? Y
New feature? N
Automated tests included? Y
Related user documentation PR URL /
Related developer documentation PR URL /
Issues addressed (#s or URLs) /
BC breaks? N
Deprecations? N

Description:

Campaign conditions with date fields and date operators did not work on company fields. This PR fixes that.

Steps to reproduce the bug:

  1. Create a custom field (Data type = Date) on the company object
  2. Add a date (5 days ago) in that field to the record for a contact's Primary Company
  3. Create a campaign with a contact linked to that company in the Contact Source segment
  4. Add a condition for Contact field value
    --Execute immediately
    --Select the company date field
    --Operator = date; Value = custom, # = 5, day(s)
  5. Add an action on the yes path to trigger immediately
  6. Publish the campaign

The contact will go to the false (red) route of the campaign.

Steps to test this PR:

  1. Load up this PR
  2. Clone the campaign so you could run it again.
  3. The contact will go to the true (green) campaign path.

@escopecz escopecz added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test labels May 17, 2019
@npracht npracht added this to the 2.16.0 milestone May 22, 2019
@RCheesley
Copy link
Sponsor Member

I'm a bit confused by the test instructions on this, particularly this part:

--Operator = date; Value = custom, # = 5, day(s)

Why would setting the condition of the date to equal 5 days (my understanding of what this is doing) succeed if I selected any random date @escopecz ?

@escopecz
Copy link
Sponsor Member Author

@RCheesley are you asking why 5 days and not 4 nor 6? I don't think it matters. It's just an example.

@npracht npracht modified the milestone: 2.16.0 Jan 23, 2020
@RCheesley RCheesley added this to the 2.16.1 milestone Mar 9, 2020
@dennisameling dennisameling added the T1 Low difficulty to fix (issue) or test (PR) label Mar 18, 2020
@RCheesley
Copy link
Sponsor Member

@escopecz coming back to this one as hoping to get it merged in 2.16.1. I still don't understand what I am looking for in order to show it is the bug you mention, and what to look for to show it is being fixed.

I pick a date, say 1st January 2019 for the field company_date on Company A, for example.

Create a campaign, use the USA segment as a source which includes a contact from company A.

The rest of the description I don't understand - why would it go down the red route as a bug or the green route as a fixed bug based on the company_date value equalling 5 days?

1st January 2019 doesn't equal five days, so that would result in the 'no' route - which you'd suggested was incorrect and should be the green route.

All puzzlement here, please enlighten me!

@RCheesley RCheesley added the pending-feedback PR's and issues that are awaiting feedback from the author label Mar 18, 2020
@escopecz
Copy link
Sponsor Member Author

escopecz commented Mar 19, 2020

@RCheesley I updated the steps if it makes sense now. Basically it should work the same way for contacts as for companies.

The goal is to get true (green route) from the condition if the date value is NOW - 5 days (5 days ago in plain English) and false (red route) for other date values. Does it make sense?

@escopecz escopecz removed the pending-feedback PR's and issues that are awaiting feedback from the author label Mar 19, 2020
@RCheesley
Copy link
Sponsor Member

Yes, that makes complete sense now! Thanks for the explanation!

@RCheesley
Copy link
Sponsor Member

I thought I saw another PR doing the same thing though. Let me take a look!

@RCheesley
Copy link
Sponsor Member

@escopecz is this the same issue: #7788 - I wasn't sure if your code was actually specific to company fields.

@escopecz
Copy link
Sponsor Member Author

#7788 is for segments. Different feature.

@dennisameling dennisameling removed this from the 2.16.1 milestone Mar 19, 2020
@npracht npracht added Triage M2/M3 and removed ready-to-test PR's that are ready to test labels Apr 4, 2020
@npracht
Copy link
Member

npracht commented Apr 4, 2020

Hi there!
It has been decided to not create any extra Mautic 2 versions (except for major security purpose) knowing that Mautic 3 is coming very soon.

We now want to integrate your contribution in the Mautic 3 roadmap as 3.0.1 candidate.

How to do?

  1. Check if the bug still present in Mautic 3
  2. If Yes: rebase your PR against the 3.x branch
  3. If No: please comment on your PR: “This PR only applies to Mautic 2.x”

Please report results by commenting on your PR to make us administration easier.

In case your bugfix only apply to Mautic 2, we'll consider adding it in an extra Mautic 2 version.


You can more information on how to do all of that on this blog post "Getting you PR ready for Mautic 3".

@npracht
Copy link
Member

npracht commented May 20, 2020

Hello @escopecz could you please test mautic 3.0.0-beta2 and tell me if the issue is still existing?

  • If no, i'll tag this issue at M2 only relevant
  • If yes, could you please rebase your branch against 3.x branch ?

Waiting for your feedback, i would very love to merge it in 3.0.1 if still relevant. Thanks !

@npracht npracht changed the base branch from staging to 3.x June 10, 2020 07:24
@npracht npracht added ready-to-test PR's that are ready to test and removed Triage M2/M3 labels Jun 10, 2020
@npracht npracht added this to the 3.0.1 milestone Jun 10, 2020
@RCheesley RCheesley changed the base branch from 3.x to staging June 17, 2020 11:35
@RCheesley
Copy link
Sponsor Member

Unable to rebase to trigger the code coverage report, @escopecz could you take a look please? Thank you!

@escopecz escopecz force-pushed the date-condition-on-company-field branch from d18fd3d to 0210dd5 Compare June 25, 2020 07:29
@escopecz
Copy link
Sponsor Member Author

@RCheesley rebased. Thanks for the ping.
@npracht it is still relevant for M3.

@mautic mautic deleted a comment from TravisBuddy Jun 25, 2020
@codecov
Copy link

codecov bot commented Jun 25, 2020

Codecov Report

Merging #7541 into staging will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             staging    #7541      +/-   ##
=============================================
+ Coverage      35.24%   35.28%   +0.03%     
- Complexity     27884    27964      +80     
=============================================
  Files           1731     1731              
  Lines          96712    96898     +186     
=============================================
+ Hits           34088    34190     +102     
- Misses         62624    62708      +84     
Impacted Files Coverage Δ Complexity Δ
.../bundles/LeadBundle/Entity/LeadFieldRepository.php 36.80% <100.00%> (+10.88%) 37.00 <0.00> (ø)
...undles/LeadBundle/EventListener/LeadSubscriber.php 30.96% <0.00%> (+0.62%) 147.00% <0.00%> (+64.00%)
app/bundles/FormBundle/Entity/FormRepository.php 53.90% <0.00%> (+2.94%) 34.00% <0.00%> (+8.00%)
app/bundles/CoreBundle/Helper/FilePathResolver.php 88.88% <0.00%> (+3.17%) 20.00% <0.00%> (+7.00%)
...bundles/LeadBundle/Entity/LeadDeviceRepository.php 13.15% <0.00%> (+7.44%) 22.00% <0.00%> (+1.00%)

@RCheesley
Copy link
Sponsor Member

@escopecz seems like Scrutinizer encountered an error on the last run :/

@escopecz
Copy link
Sponsor Member Author

I don't think that's a blocker. I cannot run the Scrutinizer task again. I've been lobbying to disable Scrutinizer as no one is looking at the results and suggestions anyway. But no one has access to that tool.

@RCheesley
Copy link
Sponsor Member

Close & reopen for Scrutinizer

@RCheesley RCheesley closed this Jun 26, 2020
@RCheesley RCheesley reopened this Jun 26, 2020
@kuzmany kuzmany self-assigned this Jun 27, 2020
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.

Works for me 👍
In description should be set to -5 days in campaign condition

@RCheesley RCheesley added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels Jun 27, 2020
Copy link
Sponsor Member

@RCheesley RCheesley left a comment

Choose a reason for hiding this comment

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

Confirmed the bug and the resolution with this PR applied! 🚀

@RCheesley RCheesley added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels Jun 27, 2020
@dennisameling dennisameling merged commit a348f90 into mautic:staging Jun 28, 2020
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 ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged T1 Low difficulty to fix (issue) or test (PR)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants