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

[Ref] MembershipView page - Remove redundant financialacl check #23230

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Follow on from #22664 - removes the financial acl check that @colemanw questioned removing in the PR

Before

The same should work before & after .....

  1. Give webuser role 'access CiviMembership' - or merge this pr Give advisor user access to CiviMember & CiviEvent civicrm-buildkit#690 and rebuild the site...
  2. In incognito login as advisor/advisor
  3. Do a membership search & view a membership record
  4. As admin user enable financial acls
  5. try refreshing the view screen for advisor - should no longer load

After

No change in r-run, some code removed

Technical Details

#23228 needs to be merged through to master before merge (then I'll rebase it out) - but if testing / adding MOP before then you will need it :-)

Comments

@civibot
Copy link

civibot bot commented Apr 15, 2022

(Standard links)

@civibot civibot bot added the master label Apr 15, 2022
@eileenmcnaughton eileenmcnaughton changed the title Remove redundant financialacl check [Ref] MembershipView page - Remove redundant financialacl check Apr 17, 2022
@yashodha
Copy link
Contributor

@eileenmcnaughton PR #23228 has been merged. You might want to resolve conflicts in this one. Thanks!

@eileenmcnaughton
Copy link
Contributor Author

thanks @yashodha - done

@eileenmcnaughton
Copy link
Contributor Author

test this please

@BettyDolfing
Copy link

Test this please

@BettyDolfing
Copy link

Test this please

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

I just rebased this to make sure it's fresh - @yashodha or @monishdeb is one of you able to look at it?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw this is a PR I did in response to a comment from you - but it seems to have stalled (and hence I haven't been working on getting financialacls out of core for the last 6 monts) - but looking at it it seems pretty minimal & should probably be merged (the other option is to close it)

@seamuslee001
Copy link
Contributor

This seems ok to me

@seamuslee001 seamuslee001 merged commit e7fe693 into civicrm:master Jan 11, 2023
@seamuslee001 seamuslee001 deleted the mem branch January 11, 2023 22:33
@eileenmcnaughton
Copy link
Contributor Author

Thanks @seamuslee001

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants