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

Fullcontact bundle api upgrade #8783

Open
wants to merge 32 commits into
base: 4.x
Choose a base branch
from

Conversation

mohit-rocks
Copy link
Contributor

@mohit-rocks mohit-rocks commented May 12, 2020

closes #8213
resolves #8079

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

Description:

Steps to reproduce the bug:

Steps to test this PR:

  1. Load up this PR

List deprecations along with the new alternative:

List backwards compatibility breaks:

@TravisBuddy
Copy link

Travis tests have failed

Hey @mohit-rocks,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 8b992790-940c-11ea-95ad-2b40da7e2a04

@npracht npracht added plugin Anything related to plugins needs-documentation PR's that need documentation before they can be merged T2 Medium difficulty to fix (issue) or test (PR) labels May 12, 2020
@npracht npracht added this to the 3.0.1 milestone May 12, 2020
@TravisBuddy
Copy link

Travis tests have failed

Hey @mohit-rocks,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 0d3006e0-9ab9-11ea-ae6f-3d1c6927e18c

@TravisBuddy
Copy link

Travis tests have failed

Hey @mohit-rocks,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 02273450-9b89-11ea-acf2-2540bfc6cd37

@TravisBuddy
Copy link

Travis tests have failed

Hey @mohit-rocks,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 74540ea0-9dc7-11ea-80ab-25118b76b5d1

@mohit-rocks mohit-rocks marked this pull request as draft May 24, 2020 16:25
@TravisBuddy
Copy link

Travis tests have failed

Hey @mohit-rocks,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: c034c4b0-9f82-11ea-a34f-dd9ab62511aa

Added validation to check if key is set in value check.
Remove redundant code in field mapping yaml.
@TravisBuddy
Copy link

Travis tests have failed

Hey @mohit-rocks,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 332d1a90-a03b-11ea-aa49-9d3926c79f60

@TravisBuddy
Copy link

Travis tests have failed

Hey @mohit-rocks,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 7f31a2b0-a03d-11ea-aa49-9d3926c79f60

@kuzmany
Copy link
Member

kuzmany commented Jun 2, 2020

I did test.

1.I see this error: The following Mautic fields are required and must be mapped: Email
Cannot match fields in contact tab

image

  1. Need support just sync to Mautic direction

image

@TravisBuddy
Copy link

Travis tests have failed

Hey @mohit-rocks,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 8099f140-abee-11ea-a7dd-93c9ffbfb007

@mohit-rocks mohit-rocks marked this pull request as ready for review June 11, 2020 16:18
@mohit-rocks mohit-rocks changed the title [WIP] Fullcontact bundle api upgrade Fullcontact bundle api upgrade Jun 11, 2020
@TravisBuddy
Copy link

Travis tests have failed

Hey @mohit-rocks,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 75ba7f10-ac02-11ea-a7dd-93c9ffbfb007

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.

I comment some parts to improve it
Also could we rework company fetch not by website, but by company name?
https://dashboard.fullcontact.com/api-ref#company-enrichment

displayName: "Name"
name: name
type: text
-
Copy link
Member

Choose a reason for hiding this comment

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

Should add required: true to company name

displayName: "Email"
name: emails
type: text
-
Copy link
Member

Choose a reason for hiding this comment

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

Should add required: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is just to provide the mapping of source data field and mapping field.
Do we need to parse this attribute and add it on the configuration modal?

Copy link
Member

Choose a reason for hiding this comment

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

You have this attribute in Field.php DTO.
My suggestion make email/company name required and in the first place.

https://github.com/mautic/mautic/pull/8783/files#diff-b095ba0cad61c1b1c2ad02108d685e1cR22

Copy link
Member

Choose a reason for hiding this comment

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

Still small improvement.
Maybe email and company email as required.

@npracht npracht modified the milestones: 3.2.3, 3.2.5, 3.3.1 Jan 15, 2021
@npracht npracht changed the base branch from staging to features January 19, 2021 16:20
@RCheesley RCheesley modified the milestones: 3.3.1, 3.3.2 Feb 23, 2021
@RCheesley RCheesley modified the milestones: 3.3.2, 3.3.3 Mar 15, 2021
@RCheesley RCheesley modified the milestones: 3.3.3, 4.0-beta Apr 16, 2021
@npracht npracht modified the milestones: 4.0-beta, 4.0-rc May 14, 2021
@RCheesley RCheesley removed this from the 4.0-rc milestone May 22, 2021
@stale
Copy link

stale bot commented Aug 20, 2021

This issue or PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you would like to keep it open please let us know by replying and confirming that this is still relevant to the latest version of Mautic and we will try to get to it as soon as we can. Thank you for your contributions.

@stale stale bot added the stale Issues which have not received an update within 90 days label Aug 20, 2021
@stale
Copy link

stale bot commented Sep 3, 2021

This issue or PR has been automatically closed because it has not had recent activity. In the case of issues, if it persists in the latest version of Mautic, please create a new issue and link back to this one for reference. With PRs if you wish to pick up the PR and update it so that it can be considered for a future release, please comment and we will re-open it. Thank you for your contributions.

@stale stale bot closed this Sep 3, 2021
@kuzmany kuzmany reopened this Sep 3, 2021
@stale stale bot removed the stale Issues which have not received an update within 90 days label Sep 3, 2021
@RCheesley RCheesley added the enhancement Any improvement to an existing feature or functionality label Sep 7, 2021
@RCheesley RCheesley changed the base branch from features to 4.x September 7, 2021 11:09
kuzmany
kuzmany previously approved these changes Oct 12, 2021
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.

We use it in production.
Works 👍

@kuzmany kuzmany added the pending-test-confirmation PR's that require one test before they can be merged label Oct 12, 2021
@kuzmany
Copy link
Member

kuzmany commented Oct 12, 2021

@mohit-rocks can you resolve CS fixer?

@RCheesley RCheesley added the needs-rebase PR's that need to be rebased label Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The PR contributors have signed the contributors agreement enhancement Any improvement to an existing feature or functionality needs-automated-tests PR's that need automated tests before they can be merged needs-documentation PR's that need documentation before they can be merged needs-rebase PR's that need to be rebased pending-test-confirmation PR's that require one test before they can be merged plugin Anything related to plugins T2 Medium difficulty to fix (issue) or test (PR)
Projects
None yet
8 participants