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

Tracking lead manipulator #5379

Merged
merged 24 commits into from Apr 5, 2018
Merged

Tracking lead manipulator #5379

merged 24 commits into from Apr 5, 2018

Conversation

Hadamcik
Copy link
Contributor

@Hadamcik Hadamcik commented Nov 27, 2017

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

Description:

Log source of lead creation and identification

Steps to test this PR:

  1. Run migrations php app/console doctrine:migrations:migrate
  2. Create a new lead through the following sources (clear your browsers cache each time): tracking pixel on 3rd party page and via a Mautic landing page. Check the contacts created and note the created by source entry in the history should be the URL for the tracking pixel and the name of the landing page for the other.
  3. Now submit a Mautic form embedded in a 3rd party page then also a Mautic form standalone (/form/ID). Note the identified by source entry should be the name of the form in the timeline.
  4. Do a CRM sync and note the created by and identified source should be the CRM name
  5. Do an import and then also manually create a contact and the created by/identified by source should be the name of the logged in user.

@Hadamcik Hadamcik changed the title WIP - Tracking lead source 2 Tracking lead manipulator Nov 28, 2017
@Hadamcik Hadamcik added the ready-to-test PR's that are ready to test label Nov 28, 2017
@alanhartless alanhartless added this to the 2.13.0 milestone Dec 13, 2017
* @param string|null $objectName
* @param int|null $objectId
*/
public function __construct($bundleName = null, $objectName = null, $objectId = null)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

What's the reason why the first 2 params are nullable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because in database, they can be nullable. I thought maybe there is use case when it's possible

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Good point. I was thinking we should write the code as strict as possible.

@@ -199,6 +200,9 @@ class Lead extends FormEntity implements CustomFieldEntityInterface
*/
private $color;

/** @var LeadManipulator */
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Can this annotation use the new lines as all of the annotations above and bellow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, it's just so long..

$lead->setManipulator(new LeadManipulator(
'lead',
'lead'
));
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Maybe I'm not understanding the task right, but how can you tell if the source of the lead is import over manual creation over UI? This code is called 4 times on 4 different places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already long time, so I did not fully recalled what is bundle name and what is object name. Where do you see problem? It's manipulated from lead bundle, lead model so it's lead-lead if I'm right.

I'm not sure if it says something about UI

@escopecz escopecz added feature A new feature for inclusion in minor or major releases needs-documentation PR's that need documentation before they can be merged labels Feb 8, 2018
@dbhurley dbhurley modified the milestone: 2.13.0 Mar 4, 2018
@javjim javjim removed the ready-to-test PR's that are ready to test label Mar 27, 2018
@escopecz escopecz added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Mar 27, 2018
@escopecz escopecz removed the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Mar 28, 2018
@Maxell92 Maxell92 self-assigned this Mar 29, 2018
@Maxell92
Copy link
Contributor

Maxell92 commented Mar 29, 2018

Seems that this PR is not working for me. The only difference after applying the PR is that log has different order. Could you provide us screenshot how log should look like?

Staging:

screen shot 2018-03-29 at 12 21 15

After applying PR:

screen shot 2018-03-29 at 12 21 26

Also - there are no tests for new functionality.

@Maxell92 Maxell92 added pending-test-confirmation PR's that require one test before they can be merged needs-automated-tests PR's that need automated tests before they can be merged pending-feedback PR's and issues that are awaiting feedback from the author and removed pending-test-confirmation PR's that require one test before they can be merged labels Mar 29, 2018
@alanhartless
Copy link
Contributor

alanhartless commented Mar 30, 2018

I did a little work to this to address the follow issues:

  1. It generated an entry every time a lead was saved, so I limited it to when a contact is new or when a contact is identified
  2. Tracking pixel broke because Page is not associated with those
  3. A forms generate.js and the mtc.js would create entires because they leveraged getCurrentLead so the tracking pixel wouldn’t register.
  4. Needed a way to display in the timeline.

@alanhartless alanhartless removed the pending-feedback PR's and issues that are awaiting feedback from the author label Apr 2, 2018
@mleffler mleffler assigned Hadamcik and unassigned Maxell92 Apr 2, 2018
@javjim javjim 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-feedback PR's and issues that are awaiting feedback from the author labels Apr 3, 2018
@javjim
Copy link

javjim commented Apr 3, 2018

Wasnt able to test with landing pages.

CRM, Forms and embedded forms, Creating contacts and importing work properly

Maybe I am missing something in testing with landing pages...

@javjim javjim added pending-feedback PR's and issues that are awaiting feedback from the author and removed ready-to-test PR's that are ready to test labels Apr 3, 2018
@javjim
Copy link

javjim commented Apr 3, 2018

Was able to test it and its working properly

@javjim javjim added pending-test-confirmation PR's that require one test before they can be merged and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Apr 3, 2018
@alanhartless alanhartless removed the needs-documentation PR's that need documentation before they can be merged label Apr 4, 2018
@alanhartless
Copy link
Contributor

@javjim-mautic So landing pages work or no?

I removed the needs documentation label because this is just a minor UI change as far as the frontend goes. We don't have existing documentation for the timeline to append to.

@Hadamcik Hadamcik removed needs-automated-tests PR's that need automated tests before they can be merged pending-test-confirmation PR's that require one test before they can be merged labels Apr 5, 2018
@mleffler mleffler merged commit 182a711 into mautic:staging Apr 5, 2018
@npracht
Copy link
Member

npracht commented Apr 11, 2018

@mleffler @dreiser could we have a documentation for that ?

@escopecz escopecz added the needs-documentation PR's that need documentation before they can be merged label Apr 11, 2018
@alanhartless
Copy link
Contributor

@npracht What documentation are you looking for? All this does is display the source of the contact's identification and creation in the lead's History tab. There's not much to document.

@escopecz
Copy link
Sponsor Member

escopecz commented Apr 12, 2018

I added the label because some valuable info can be documented about this:

  • User docs:
    • What sources does Mautic recognize by default?
    • Is the source just info in the contact detail page or can a use create segment filters based on this? Or a campaign condition? Point trigger?
    • Will it appear for old contacts too or only for the new contacts after the upgrade to 2.13.0?
  • Dev docs:
    • How a plugin can add a new source type?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature for inclusion in minor or major releases needs-documentation PR's that need documentation before they can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants