Skip to content

Follow contact permissions when importing contacts #14554

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 15 commits into from
Mar 11, 2025
Merged

Follow contact permissions when importing contacts #14554

merged 15 commits into from
Mar 11, 2025

Conversation

fedys
Copy link
Member

@fedys fedys commented Feb 4, 2025

Q A
Bug fix? (use the a.b branch) 🟢
New feature/enhancement? (use the a.x branch) 🔴
Deprecations? 🔴
BC breaks? (use the c.x branch) 🔴
Automated tests included? 🟢
Related user documentation PR URL
Related developer documentation PR URL
Issue(s) addressed

Description

This PR adds permission checks during contact and company imports.

📋 Steps to test this PR:

  1. Create a role that does not have permissions for editing contacts but has permissions to create contacts.
  2. Login as a user that has the above role assigned to them.
  3. Create a new contact import that contains rows for both inserts and updates.
  4. Wait till the import is imported.
  5. Any CSV rows containing updates should be ignored.
  6. There should be error messages like User '<username>' has insufficient permissions for all the rows containing updates.
  7. Validate that permissions are checked against a user who last updated a given import.
  8. Validate the above steps for company imports at /s/companies/import/new. The company import follows the same Contact Permissions. There are no separate Company Permissions.

@fedys fedys added contacts Anything related to contacts import-export Anything related to importing and exporting unforking Used for PRs in the Acquia's unforking initiative labels Feb 4, 2025
@fedys fedys self-assigned this Feb 4, 2025
@fedys fedys requested a review from escopecz February 4, 2025 11:57
@fedys fedys mentioned this pull request Feb 4, 2025
Closed
@codecov Codecov
Copy link

codecov bot commented Feb 4, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 83.87097% with 5 lines in your changes missing coverage. Please review.

Project coverage is 64.67%. Comparing base (aecad42) to head (b19e282).
Report is 45 commits behind head on 6.x.

Files with missing lines Patch % Lines
app/bundles/LeadBundle/Model/CompanyModel.php 68.75% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                6.x   #14554      +/-   ##
============================================
- Coverage     64.70%   64.67%   -0.04%     
- Complexity    34687    34695       +8     
============================================
  Files          2274     2274              
  Lines        103626   103647      +21     
============================================
- Hits          67053    67032      -21     
- Misses        36573    36615      +42
Files with missing lines Coverage Δ
app/bundles/CoreBundle/Entity/FormEntity.php 94.00% <ø> (+4.00%) ⬆️
app/bundles/LeadBundle/Command/ImportCommand.php 86.53% <100.00%> (+0.82%) ⬆️
...undles/LeadBundle/Controller/CompanyController.php 51.37% <100.00%> (ø)
app/bundles/LeadBundle/Entity/Lead.php 90.47% <ø> (-0.02%) ⬇️
app/bundles/LeadBundle/Model/ImportModel.php 82.62% <100.00%> (-6.57%) ⬇️
app/bundles/LeadBundle/Model/LeadModel.php 60.66% <100.00%> (+0.52%) ⬆️
...pp/bundles/UserBundle/Security/UserTokenSetter.php 100.00% <100.00%> (ø)
app/bundles/LeadBundle/Model/CompanyModel.php 72.35% <68.75%> (+1.88%) ⬆️

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

escopecz
escopecz previously approved these changes Feb 4, 2025
View reviewed changes
Copy link
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.

I see no issues in the code here 👍

@escopecz escopecz added pending-test-confirmation PR's that require one test before they can be merged code-review-passed PRs which have passed code review labels Feb 4, 2025
shinde-rahul
shinde-rahul reviewed Feb 5, 2025
View reviewed changes
Copy link
Contributor

@shinde-rahul shinde-rahul left a comment

Choose a reason for hiding this comment

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

The patch is missing some coverage. Please add it.

@fedys fedys removed their assignment Feb 7, 2025
@fedys
Merge branch '6.x' into follow-contact-permissions-when-importing-con…
cd9f483 
…tacts-2

# Conflicts:
#	app/bundles/LeadBundle/Tests/Model/LeadModelTest.php
@fedys
Copy link
Member Author

fedys commented Mar 10, 2025

@rahuld-dev the coverage is not reported correctly by Codecov. I checked the coverage in my environment and can see the lines I changed within this PR are fully covered. See the screenshots below.

Screenshot 2025-03-10 at 13 20 30
Screenshot 2025-03-10 at 13 00 39

@fedys fedys requested review from shinde-rahul and escopecz March 10, 2025 12:21
@rahuld-dev
Copy link
Contributor

@rahuld-dev the coverage is not reported correctly by Codecov. I checked the coverage in my environment and can see the lines I changed within this PR are fully covered. See the screenshots below.

Screenshot 2025-03-10 at 13 20 30 Screenshot 2025-03-10 at 13 00 39

@shinde-rahul

shinde-rahul
shinde-rahul approved these changes Mar 11, 2025
View reviewed changes
Copy link
Contributor

@shinde-rahul shinde-rahul left a comment

Choose a reason for hiding this comment

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

Code changes LGTM!!!

escopecz
escopecz approved these changes Mar 11, 2025
View reviewed changes
Copy link
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.

When I was following the testing steps I found 2 unrelated bugs from previously merged PRs so I fixed those directly here.

Then I was able to get successful results following the steps.

Company import:
Screenshot 2025-03-11 at 13 33 26

Contact import:
Screenshot 2025-03-11 at 13 01 13

@escopecz escopecz added user-testing-passed PRs which have been successfully tested by the required number of people. and removed pending-test-confirmation PR's that require one test before they can be merged labels Mar 11, 2025
@escopecz escopecz added this to the 6.0.0-RC milestone Mar 11, 2025
@escopecz escopecz merged commit 41f112b into mautic:6.x Mar 11, 2025
16 of 17 checks passed
@github-project-automation github-project-automation bot moved this from ⏳︎ Needs 1 more test to 🥳 Done in Open Source Fridays Mar 11, 2025
@RCheesley RCheesley added the bug Issues or PR's relating to bugs label Mar 17, 2025
@mautibot
Copy link
Contributor

mautibot commented May 1, 2025

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/issue-with-logging-in-to-mautic-6-0-after-upgrade-from-5-2-5/35610/3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@escopecz escopecz escopecz approved these changes

+1 more reviewer

@shinde-rahul shinde-rahul shinde-rahul approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@fedys fedys

Labels
bug Issues or PR's relating to bugs code-review-passed PRs which have passed code review contacts Anything related to contacts import-export Anything related to importing and exporting unforking Used for PRs in the Acquia's unforking initiative user-testing-passed PRs which have been successfully tested by the required number of people.
Projects
Open Source Fridays
Status: 🥳 Done
Milestone
6.0.0-rc
Development

Successfully merging this pull request may close these issues.
6 participants
@fedys @rahuld-dev @mautibot @shinde-rahul @escopecz @RCheesley