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

Role export access in Leads, Forms, Reports #5995

Closed
wants to merge 17 commits into from

Conversation

kuzmany
Copy link
Member

@kuzmany kuzmany commented Apr 24, 2018

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

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

Description:

Added new permission for export in form results. This PR not require migrations, exports are default enable. This permission give ability disable export permission for Contacts, Forms, Reports.

image

Steps to test this PR:

  1. Create new user with role and enable all permission for Contacts, Forms, Reports with unchecked new Export access: Disable
  2. Login as new user and see If exports buttons exists and exports works properly in all sections: Contacts (batch export), Form (results exports) and Reports
  3. Open HTML exports of Form results and Reports.
  4. Edit in incognito user permission and disable export
  5. See If export button are hidden in Form results and Reports and Contacts lists in batch.
  6. See If opened HTML export from 3. return access denied after refresh

@mautibot mautibot added code-review-needed PR's that require a code review before merging Feature labels Apr 24, 2018
@kuzmany
Copy link
Member Author

kuzmany commented Apr 24, 2018

Label: Ready to test

@mautibot mautibot added ready-to-test PR's that are ready to test and removed ready-to-test PR's that are ready to test labels Apr 24, 2018
@kuzmany kuzmany changed the title Export permission in form results Export permission in Leads, Forms, Reports Apr 24, 2018
@kuzmany
Copy link
Member Author

kuzmany commented Apr 24, 2018

Label: WIP

@mautibot mautibot added the WIP PR's that are not ready for review and are currently in progress label Apr 24, 2018
@kuzmany kuzmany changed the title Export permission in Leads, Forms, Reports Role export access in Leads, Forms, Reports Apr 26, 2018
@mautibot mautibot added ready-to-test PR's that are ready to test and removed WIP PR's that are not ready for review and are currently in progress labels Apr 26, 2018
npracht
npracht previously approved these changes May 24, 2018
Copy link
Member

@npracht npracht left a comment

Choose a reason for hiding this comment

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

Working from a user perspective ! Bravo !

@npracht
Copy link
Member

npracht commented May 24, 2018

Label: Pending test confirmation

@npracht npracht mentioned this pull request Jun 8, 2018
@kuzmany kuzmany added this to the 2.15.0 milestone Sep 5, 2018
@npracht npracht added 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 Oct 26, 2018
@npracht
Copy link
Member

npracht commented Oct 26, 2018

@kuzmany please solve conflicts ;)

@Woeler Woeler added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Nov 26, 2018
@kuzmany kuzmany removed the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Nov 26, 2018
@Woeler Woeler modified the milestones: 2.15.0, 2.16.0 Dec 5, 2018
@kuzmany kuzmany mentioned this pull request Feb 13, 2019
cykonetic added a commit to TheDMSGroup/mautic-eb that referenced this pull request Feb 13, 2019
@cykonetic
Copy link
Contributor

👍

@npracht npracht added the forms Anything related to forms label Jul 9, 2021
@npracht npracht added this to the 4.1 milestone Aug 20, 2021
@RCheesley RCheesley changed the base branch from features to 4.x September 7, 2021 13:34
@dsp76
Copy link

dsp76 commented Nov 24, 2021

I tested this PR in Gitpod as described. Worked as expected. Only one comment - exporting a single contact is still possible.

@RCheesley
Copy link
Sponsor Member

PHPSTAN issues needing to be addressed here @escopecz - and still need to bump the coverage. Any chance @fedys or @dongilbert might be able to help with that?

@escopecz
Copy link
Sponsor Member

I think the new checkbox should say "Export access: enable" instead of "Export access: disable". I understand it is done because the "enable" option would require a migration or wait for Mautic 5 that allows for some small BC change, but I suggest to do it right instead of do it easy.

@kuzmany
Copy link
Member Author

kuzmany commented Nov 25, 2021

@escopecz It would be nice suggest it once If it's actual, not 3 years after. It's really getting frustrating to contribute to the community

@RCheesley RCheesley modified the milestones: 4.1, 4.2 Nov 29, 2021
@escopecz
Copy link
Sponsor Member

@kuzmany I know and I also suffer from the lack of testers. I commented with the same a year ago but that is still a couple years late. I have a plan how to improve this in the future and spend more time here. 🤞

@kuzmany kuzmany removed this from the 4.2 milestone Feb 15, 2022
@medsun88
Copy link

medsun88 commented Feb 28, 2022

#7645 #6180 #7246

OMG, Since 2018 we are discussing this feature ( to restrict user from export)
Still not sure when It will be introduced .

I have more than, 25000 Contact in Mautic, and any of my user can easily can export them.
Such a big loophole, and so much discussion.

Guys, this is a Very Import Feature. When it will come ?

@kuzmany @escopecz @RCheesley
Do you have some suggestion to fix this.

@kuzmany
Copy link
Member Author

kuzmany commented Feb 28, 2022

@medsun88 as you see this is PR and you can use it.

It come to stable version once when it will be approve by team/community/volunteers, finish unit tests, resolve CI checks etc etc.

@RCheesley
Copy link
Sponsor Member

@kuzmany needs some PHPSTAN attention here & the additional test coverage - would be really nice if we can aim to get this into Mautic 5 if it's BC as @escopecz suggests. I am inclined to agree that checking something should be in the affirmative, so if this means it needs to be held to 5.x that is what we should do.

Really appreciate that this is a really old PR and a pain to have to come back to after so long!

@mautibot
Copy link

mautibot commented Mar 3, 2022

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

https://forum.mautic.org/t/how-to-restrict-users-to-export-contact-from-mautic/22845/8

@kuzmany
Copy link
Member Author

kuzmany commented Mar 3, 2022

@RCheesley we build PR without BC. The suggestions from @escopecz will make BC.
In that case this change need a lot of hours of works, because we already use it in production and the changes make difficult to us. At the moment we are able support just this version of PR. Will see

@medsun88
Copy link

medsun88 commented Mar 3, 2022

Hi @kuzmany @RCheesley

getting error while clearing cache

In RegisterListenersPass.php line 111:

Class "Mautic\LeadBundle\EventListener\SegmentFiltersSubscriber" used for service "mautic.lead.subscriber.
segment.filter" cannot be found.

Check screenshot
https://prnt.sc/CJP17PZobVnt

@RCheesley RCheesley added the needs-rebase PR's that need to be rebased label Aug 4, 2023
@xcellenceit
Copy link

Reviewers, pls look into this for release. Its been long for such a critical feature.

@escopecz
Copy link
Sponsor Member

escopecz commented Nov 7, 2023

@xcellenceit it's not waiting for reviewers but for the rebase and addressing the reported issue

@adiux
Copy link
Contributor

adiux commented Nov 7, 2023

I understand everybody wants this features as soon as possible (me included) as it is critical. However, from a UX standpoint, I strongly agree with @escopecz. We should not change the logic of the checkboxes:

"Export access: enable" instead of "Export access: disable".

It can be a BC Break with v5 now. So we don't need to do a migration. I assume the admin role will get the permission anyway.

@medsun88
Copy link

medsun88 commented Nov 8, 2023

@kuzmany, @escopecz

I understand everybody wants this features as soon as possible (me included) as it is critical. However, from a UX standpoint, I strongly agree with @escopecz. We should not change the logic of the checkboxes:

"Export access: enable" instead of "Export access: disable".

It can be a BC Break with v5 now. So we don't need to do a migration. I assume the admin role will get the permission anyway.

@kuzmany, @escopecz
Any update, When it can be released
it has been pending for a long time.

@kuzmany
Copy link
Member Author

kuzmany commented Nov 10, 2023

Close favor to M5 version #12884

@kuzmany kuzmany closed this Nov 10, 2023
@kuzmany
Copy link
Member Author

kuzmany commented Nov 10, 2023

All guys please test updated version for M5 #12884 (unit tests added later)

@medsun88
Copy link

I am using Mautic v4.4.12
In which version will it be introduced? It long-awaited feature.

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 code-review-needed PR's that require a code review before merging feature A new feature for inclusion in minor or major releases forms Anything related to forms needs-automated-tests PR's that need automated tests before they can be merged needs-rebase PR's that need to be rebased pending-feedback PR's and issues that are awaiting feedback from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.