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

Move acl check for contributionView to the extension #22684

Merged
merged 1 commit into from
Feb 10, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Feb 2, 2022

Overview

Move acl check for contributionView to the extension

Before

Code that belongs in the extension is in the core form

After

moved to the extension

Technical Details

Comments

@monishdeb are you able to review this?

@civibot
Copy link

civibot bot commented Feb 2, 2022

(Standard links)

@civibot civibot bot added the master label Feb 2, 2022
@mattwire
Copy link
Contributor

mattwire commented Feb 2, 2022

I think this will probably make #22588 go stale any chance of a review on that one :-)

@eileenmcnaughton
Copy link
Contributor Author

@mattwire I took a look & you are right it would have conflicted. This PR moved 2 parts

  1. status bounce on no-access
  2. availability of links

I've removed the second part from the PR - as that bit would have
a) conflicted and
b) been able to be solved better by hooks in the context of your PR.

However, I think your PR is removing the financialacl hackery without re-instating the functionality via a hook (presumably the links hook) - so $canEdit is still being assigned but no longer being heeded -

@colemanw
Copy link
Member

Merging as the cleanup is an improvement and part of a larger effort we don't want stalled.

@colemanw colemanw merged commit 0ee4cba into civicrm:master Feb 10, 2022
@eileenmcnaughton eileenmcnaughton deleted the contacl branch February 10, 2022 01:16
@eileenmcnaughton
Copy link
Contributor Author

thanks @colemanw - I did remove the conflict potential from it to make this one mergeable

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.

3 participants