8000 Fix import DNC for new contacts by kuzmany · Pull Request #9732 · mautic/mautic · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix import DNC for new contacts #9732

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 23, 2021
Merged

Fix import DNC for new contacts #9732

merged 6 commits into from
Apr 23, 2021

Conversation

kuzmany
Copy link
Member
@kuzmany kuzmany commented Feb 26, 2021
Q A
Branch? "features" for all features, enhancements and bug fixes (until 3.3.0 is released)
Bug fix? yes/no
New feature? yes/no
Deprecations? yes/no
BC breaks? yes/no
Automated tests included? yes/no
Related user documentation PR URL mautic/mautic-documentation#...
Related developer documentation PR URL mautic/developer-documentation#...
Issue(s) addressed Fixes #...

Description:

This PR fixed DNC for new imported contacts.
Inspiration from @dennisameling #9186 (comment)

Steps to test this PR:

  1. Load up this PR
  2. Try import this example dnc-import-lead-test.zip
  3. Map email and dnc
  4. Sure both contacts not exists already (john@smith.com, john2@smith.com)
  5. Without this PR DNC is not set for on of the contact
  6. After this PR DNC is set properly

@kuzmany kuzmany added bug Issues or PR's relating to bugs ready-to-test PR's that are ready to test labels Feb 26, 2021
@kuzmany kuzmany added this to the 3.3.2 milestone Feb 26, 2021
@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Feb 26, 2021
@codecov
Copy link
codecov bot commented Feb 26, 2021 •
edited
Loading

Codecov Report

Merging #9732 (c7c0b29) into features (1825a89) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##             features    #9732      +/-   ##
==============================================
+ Coverage       39.37%   39.40%   +0.03%     
- Complexity      34335    34336       +1     
==============================================
  Files            2038     2038              
  Lines          108521   108527       +6     
==============================================
+ Hits            42726    42763      +37     
+ Misses          65795    65764      -31
Impacted Files Coverage Δ Complexity Δ
...eadBundle/EventListener/DoNotContactSubscriber.php 80.00% <100.00%> (+53.68%) 5.00 <0.00> (+1.00)
.../bundles/LeadBundle/Event/DoNotContactAddEvent.php 100.00% <0.00%> (+100.00%) 8.00% <0.00%> (ø%)

@RCheesley RCheesley added contacts 8000 Anything related to contacts import-export Anything related to importing and exporting T1 Low difficulty to fix (issue) or test (PR) labels Feb 26, 2021
RCheesley
RCheesley previously approved these changes Mar 13, 2021
View reviewed changes
Copy link
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 initial bug that the contact is not assigned DNC on import.

PR results in the correct assignment of the DNC flag for Email.

screenshot-mautic32 ddev site-2021 03 13-19_19_57
screenshot-mautic32 ddev site-2021 03 13-19_21_31

Do we also need to test this with other channels (eg SMS) or will the import only allow you to set DNC for email?

@RCheesley
Copy link
Member

Also @kuzmany this one needs to improve test coverage - sorry! 😅

@RCheesley RCheesley added 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 and removed ready-to-test PR's that are ready to test labels Mar 13, 2021
@kuzmany kuzmany removed the needs-automated-tests PR's that need automated tests before they can be merged label Mar 14, 2021
@kuzmany
Copy link
Member Author
kuzmany commented Mar 14, 2021

@RCheesley unit test added

@RCheesley RCheesley modified the milestones: 3.3.2, 3.3.3 Mar 15, 2021
@ts-navghane ts-navghane 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 Apr 9, 2021
@RCheesley
Copy link
Member

@kuzmany this has the 3.3.3 milestone but is based on the Features branch. Shall we switch up the branch, or wait for 4.0 beta? Either is fine with me but the former may require a rebase - not sure!

@RCheesley RCheesley changed the base branch from features to 3.3 April 13, 2021 13:40
@RCheesley RCheesley changed the base branch from 3.3 to features April 13, 2021 13:42
@RCheesley
Copy link
Member

Bumping to 4.0 beta as it will require rebasing with conflicts to fix if we try to merge this into 3.3 as it stands! Will be RTC as soon as the gitsplit is set up on the features branch.

@RCheesley RCheesley modified the milestones: 3.3.3, 4.0-beta Apr 13, 2021
@RCheesley RCheesley merged commit fca8522 into mautic:features Apr 23, 2021
@npracht npracht mentioned this pull request Jun 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RCheesley RCheesley RCheesley left review comments

@dennisameling dennisameling Awaiting requested review from dennisameling

Assignees
No one assigned
Labels
bug Issues or PR's relating to bugs cla-signed The PR contributors have signed the contributors agreement contacts Anything related to contacts import-export Anything related to importing and exporting 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
Milestone
4.0.0-beta
Development

Successfully merging this pull request may close these issues.
4 participants
@kuzmany @RCheesley @ts-navghane @dennisameling
0