CRM-21106, removed code related to Financial type acl from report

[pradpnayak]

---

• CRM-21106:
  https://issues.civicrm.org/jira/browse/CRM-21106

Overview

This PR holds changes of removing additional where clause for contribution related reports to respect Financial type ACL which is now moved into Financial ACL report.

RELEASE NOTES

People using financial type ACLs have had notification through system alerts for a couple of months to install the extension to ensure these are applied to reports. With this change the ACLs are REMOVED from core in favour of those reports and anyone using those ACLs without having installed the extension will no longer have ACLs applied

[Stoob]

I have tested this on EventParticipant.php report with Financial ACL extension enabled and it works.

[eileenmcnaughton]

I think this is good & I'm grateful for @Stoob's testing. It seems to me that there could be circumstances in which people are relying on this code to implement an ACL and that behaviour will change? In which case what should we do about notifying them? Does this count as a breaking change? If so does this become the point at which we announce 5.0 & switch to semver

(Note that I am actually in favour of switching to 5.0 on a trivial change like this. We have a general desire to switch to a semver based numbering system - which would involve much more frequent increases of the major version with only minor (but breaking) changes like this & dropping legacy php version support. One thing that has been holding us back from doing that is the mental block we, in the civi community, have about the version increment. (FYI @totten discussion))

[Stoob]

If I understand the issue correctly it will "break" only if they don't have the ACL report extension.

So if someone has Financial ACL enabled (it's a component setting in the Administer menu) is it possible to warn them the ACL report extension is now required to get reports to work upon UPGRADE?

Is that even possible (not sure)

[seamuslee001]

@Stoob i would guess you could set a message in the PreUpgrade step

[seamuslee001]

@Stoob are you able to link me to the extension?

[Stoob]

Before you do my idea please check with Joe and Pradeep to make sure it will work. Extension: https://github.com/JMAConsulting/biz.jmaconsulting.financialaclreport

[eileenmcnaughton]

I feel like we have had a lot of push back lately about minor changes that have changed functionality (I can think of an api change which no-one picked up a few months back) or which the people commenting have formed an opinion that that they would change functionality. In this case we have a change which does change functionality and we also have a technical fix in the offing to make it easier to deal with minor changes (basically incrementing the major version more often) so I think it is worth considering switching to that now. The fact that I would be very surprised if anyone at all were negatively impacted by this change makes it a good candidate for starting to bed in a new approach to changes.

[JoeMurray]

@totten and @colemanw I don't really want to comment on @eileenmcnaughton 's suggestion about using this to justify switching to proper semver, or at least taking this as the occasion for making the change. I'll let others move that discussion and decision forward in other forums like the partner and dev and marketing channels in chat and email. A PR comment area is not the right place for the discussion.

With regard to this particular PR, I have a different question of more general relevance. When moving functionality from core to an extension, should there be a general pattern of asking during upgrade whether to download and install and enable the new extension? If so, this seems like a good project for core team to lead, at least on specs and design, @totten. @monishdeb might be able to assist in implementation.

I'm not sure we should hold up merging this PR until that infrastructure is in place. I'd be happy in this case with a PreUpdate message warning about removal of functionality from core and availability of it in extension, and then a PostUpdate message as well telling people to do additional post-upgrade action of installing the extension.

[eileenmcnaughton]

I think it would be OK to let this through if we signpost it. I agree that it's quite a hurdle & I suspect this has low usage by fairly skilled people & it is quite new functionality.

My current thinking about switching to semver is to do it when we release the last php 5.3 release & drop 2 releases with only one line of difference (to make it really really clear they are the same)

[seamuslee001]

Just noting that at present i believe (@eileenmcnaughton correct me if i'm wrong) i believe that we have a pre upgrade message and a status check to alert people to the need to download and install the extension, which is in of its self pretty good signposting. A blog post would also be useful

[eileenmcnaughton]

yep pretty sure we merged that & it will go out a month before this change does

[JoeMurray]

So can we merge this to master now, since it won't come out till December, @eileenmcnaughton ?

[eileenmcnaughton]

OK - I've edited the description to ensure it gets picked up in the release notes. Yay for moving it out of core. I do wish the extension had just been called financial type acls (without the word report) as it makes sense to move more parts of those acls into it.

[eileenmcnaughton]

@JoeMurray did you see @seamuslee001 suggestion re blog post? Also might be worth checking our docs