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

Create company from ip lookup switch #7482

Merged

Conversation

Projects
4 participants
@escopecz
Copy link
Member

commented Apr 29, 2019

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

Q A
Bug fix? N
New feature? Enhancement
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:

In PR #7083 I've added option to create and link a company from IP lookup info to Mautic. It may cause problems as if a contact has a company and nothing else then Mautic considers it as identified contact. That may not be the best option for all users. Therefore this PR adds a new option which will allow users to decide what behavior they want.

Steps to test this PR:

  1. Load up this PR
  2. This will be a bit hard to test as only some paid IP lookup services provide the company data and only some IP addresses have it. But if you can then test that IP lookup still works. The change is covered with tests.
  3. Check that the new option is in the configuration and you can turn it on and off.

@npracht npracht added this to the 2.16.0 milestone May 2, 2019

@kuzmany

kuzmany approved these changes May 3, 2019

Copy link
Contributor

left a comment

Works for me.
Tested both scenarios.
Code looks good 👍

@kuzmany

kuzmany approved these changes May 3, 2019

Copy link
Contributor

left a comment

Works for me.
Tested both scenarios.
Code looks good 👍

@kuzmany

kuzmany approved these changes May 3, 2019

Copy link
Contributor

left a comment

Works for me.
Tested both scenarios.
Code looks good 👍

@kuzmany

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Could we add this to 2.15.2?
For me it's bug fix partially.

@escopecz

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

We may as well. Because we started to add company from IP lookups recently and did not realize it will have this negative effect which can be considered a BC break.

@kuzmany

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Have mautic use it in production already?

@escopecz

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

Yes, every PR that is submitted by a Mautic Inc. engineer has been reviewed, tested and released to Mautic Cloud instances already.

@kuzmany

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@escopecz good to know that 👍

@kuzmany kuzmany modified the milestones: 2.16.0, 2.15.2 May 3, 2019

@kuzmany

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@escopecz can you look on Scrutinizer test or commit again for refresh test?

@escopecz

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

I'm not going to create new bogus commit just to make Scrutinizer workflow to finish. I asked @dbhurley to turn it off for this repository as it causes only confusion and we are not considering its findings anyway. Hopefully he will get to it eventually. Please ignore Scrutinizer.

@Woeler

Woeler approved these changes May 3, 2019

Copy link
Member

left a comment

Works great, and I agree on removing scrutinizer. It's mostly just annoying and the red cross always puts me off for some reason.

@npracht npracht added this to Ready to Test (first time) in Mautic 2 May 3, 2019

@npracht npracht moved this from Ready to Test (first time) to Ready to Commit (passed testing) in Mautic 2 May 3, 2019

@kuzmany kuzmany merged commit f7eff0d into mautic:staging May 5, 2019

1 of 2 checks passed

Scrutinizer Errored
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Mautic 2 automation moved this from Ready to Commit (passed testing) to Merged May 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.