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

Support for company fields in tracking code pageview event #5611

Merged

Conversation

kuzmany
Copy link
Member

@kuzmany kuzmany commented Jan 21, 2018

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

Description:

Added support for company fields in mt('send', 'pageview');
Inspirated by SubmissionModel

$companyFieldMatches = $getCompanyData($leadFieldMatches);

This PR also added more priority to companyname fields like take companyname from companyemail.

https://github.com/kuzmany/mautic/blob/1c1b1822aef2bec3f9ddebe0df8b7e0d7204569e/app/bundles/LeadBundle/Helper/IdentifyCompanyHelper.php#L76

Steps to reproduce the bug:

  1. Mark company fields as Publicly updatable
  2. Use
    mt('send', 'pageview', {companyemail: 'mycompany@mycompany.com', companywebsite: 'https://mautic.com/'});

More in docs https://mautic.org/docs/en/contacts/contact_monitoring.html (part Tracking of custom parameters)

  1. See fields wasn't update in tracking contact

Steps to test this PR:

  1. Repeat alls teps
  2. See company fields was updated in Mautic

@kuzmany kuzmany changed the title Support for company fields by tracking code pageview event Support for company fields in tracking code pageview event Jan 21, 2018
@npracht
Copy link
Member

npracht commented Jan 24, 2018

/label feature request

@mautibot mautibot added the feature A new feature for inclusion in minor or major releases label Jan 24, 2018
@npracht
Copy link
Member

npracht commented Jan 24, 2018

/label ready to test

@mautibot mautibot added the ready-to-test PR's that are ready to test label Jan 24, 2018
@dbhurley dbhurley added this to the 2.13.0 milestone Mar 4, 2018
@escopecz escopecz self-assigned this Mar 23, 2018
@escopecz escopecz added the needs-documentation PR's that need documentation before they can be merged label Mar 23, 2018
if ($leadAdded) {
$lead->addCompanyChangeLogEntry('form', 'Identify Company', 'Lead added to the company, '.$company['companyname'], $company['id']);
} else {
$this->companyModel->setFieldValues($companyEntity, $query);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I'm getting Type error: Argument 1 passed to Mautic\LeadBundle\Model\CompanyModel::setFieldValues() must be an instance of Mautic\LeadBundle\Entity\Company, null given on this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, fixed.

@escopecz escopecz 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 Mar 23, 2018
Copy link
Sponsor 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.

It works for me now. 👍

Well, the tracking code must contain a companyname value to create a company and assign it to the contact. So this requirement must be documented.

@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Mar 26, 2018
@escopecz escopecz 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 26, 2018
@Maxell92 Maxell92 self-assigned this Mar 26, 2018
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.

Works for me. Could be merged after documentation is added.

@mautibot mautibot added the code-review-needed PR's that require a code review before merging label Mar 26, 2018
@Maxell92 Maxell92 added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged labels Mar 26, 2018
@kuzmany
Copy link
Member Author

kuzmany commented Mar 26, 2018

@escopecz
As I read from source code https://github.com/kuzmany/mautic/blob/1c1b1822aef2bec3f9ddebe0df8b7e0d7204569e/app/bundles/LeadBundle/Helper/IdentifyCompanyHelper.php#L74

Mautic try find company name from
company
companyname
if not exist from domain name of email/company email
or if not exist domain name of companywebsite

This is right workflow?

@kuzmany
Copy link
Member Author

kuzmany commented Mar 26, 2018

@escopecz maybe we can remove email from findCompany https://github.com/kuzmany/mautic/blob/1c1b1822aef2bec3f9ddebe0df8b7e0d7204569e/app/bundles/LeadBundle/Helper/IdentifyCompanyHelper.php#L74

A lot of people use personal email like gmail, outlook, yahoo, seznam.cz and nobody from them work in that companies :)

@kuzmany
Copy link
Member Author

kuzmany commented Mar 26, 2018

@Maxell92 documentation added mautic/documentation#266

@escopecz
Copy link
Sponsor Member

Removing email domain from that code would be a business decision. Lets have it as is for now.

What I was noting is that if you use the sample code you provided:
mt('send', 'pageview', {companyemail: 'mycompany@mycompany.com', companywebsite: 'https://mautic.com/'});

It actually doesn't do anything. It won't create new company. The information will be lost. So some users could be searching for an answer why. The answer is that they must provide companyname in there too. Then the company will be created with the email and website they provided.

@kuzmany
Copy link
Member Author

kuzmany commented Mar 26, 2018

@escopecz your code create company with companyname mycompany
I am going to check again

@escopecz
Copy link
Sponsor Member

Nope, it did not create mycompany company because it check DNS MX record. But that's not the point. If you send in the page hit request companyaddress alone Mautic will ignore it. It must contain company or companyname to store the rest of the company* values. That I think should be documented.

@kuzmany
Copy link
Member Author

kuzmany commented Mar 26, 2018

@escopecz docs update mautic/documentation#266

@escopecz escopecz removed needs-documentation PR's that need documentation before they can be merged ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged labels Mar 26, 2018
@escopecz escopecz merged commit 61b4d36 into mautic:staging Mar 26, 2018
@kuzmany kuzmany deleted the support-for-company-fields-from-request branch April 7, 2018 17:57
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants