Unhide financial acls, disable if not required. #29360

eileenmcnaughton · 2024-02-10T02:08:13Z

We should also

stop enabling on install - probably first! I can't recall where we do that...
@colemanw I started thinking about how we would do this -

Note my other PRs working this out of core are #28967 & #28960 -

Overview

Unhide financial acls, disable if not required.

Before

financialacls extension installed on all sites, disabled if setting not set, hidden

After

unhidden, no setting, uninstalled on sites that do not have the setting set

Technical Details

I think the main way in which core calls the extension currently is where you see isACLFinancialTypeStatus() - my most recent effort to address that is in #28967 & I guess we can't remove the setting as long as tose calls exist - hence this is still WIP - we could check if the extension is enabled - fugly but...

Comments

If this feature were to be built now the entire thing would be outside core, community supported, using hooks & I think that is ideally where it will end up. However, it was implemented a long time ago when there was less hook support

civibot · 2024-02-10T02:08:16Z

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

➡️ PR Demo site (can be rebuilt with the command test this please).
📖 Review standards
🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

eileenmcnaughton · 2024-03-15T05:06:09Z

@colemanw this is passing now!

colemanw · 2024-03-20T20:37:16Z

CRM/Upgrade/Incremental/php/FiveSeventyThree.php

+    if (!$setting) {
+      return TRUE;
+    }
+    $setting = unserialize($setting);
+    if ($setting) {


@eileenmcnaughton I'm confused about what this code is doing. Why the unserialize()? The setting appears to be a boolean, not serialized:

civicrm-core/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php

Line 24 in aac2d70

Civi::settings()->set('acl_financial_type', TRUE);

Oh, ok I guess you have to unserialize because you're pulling it directly from the database?
But now I'm still confused because the logic appears to be backward. You are uninstalling the extension if the setting is TRUE? Shouldn't it be the other way 'round?

@colemanw yep.... it should be...

@colemanw above is fixed

seamuslee001 · 2024-03-20T23:04:28Z

note just flagging at the moment if a user wants to disable civi_contribute extension / Contribute Component the financialacls block it

eileenmcnaughton · 2024-03-20T23:15:33Z

@seamuslee001 yeah - that is what this is supposed to help with - ie disable it for most users. There is another PR open to further move the logic over but struggling to get engagement on that one #28967 - possibly 'cheap & cheerful acls' should be in the core extension & only the JMA extension would have 'thorough but performance-harsh' acls but at the moment we have a bit of both in core

jitendrapurohit · 2024-04-01T06:37:05Z

ext/financialacls/CRM/Utils/Check/Component/FinancialTypeAcls.php

    $financialAclExtension = civicrm_api3('extension', 'get', ['key' => 'biz.jmaconsulting.financialaclreport', 'sequential' => 1]);
-    if ($ftAclSetting && (($financialAclExtension['count'] == 1 && $financialAclExtension['values'][0]['status'] != 'installed') || $financialAclExtension['count'] !== 1)) {
+    if (($financialAclExtension['count'] == 1 && $financialAclExtension['values'][0]['status'] !== 'installed') || $financialAclExtension['count'] !== 1) {


@eileenmcnaughton Do we need to add a check hook in this extension https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_check/?

so I had doubts about how valid this is now - it does depend on where we land with #28967 - ie do we

a) do expensive, super thorough thing in in core & make the extension obsolete or
b) do the thing that performs well & is probably enough for most cases in core & recommend people use the extension for the expensive thing - I haven't had any engagement from @JoeMurray or @seamuslee001 on that but I think the value of the check is grey enough that removing for now seems OK

now that @seamuslee001 has merged #28967 this check can go

eileenmcnaughton · 2024-05-24T00:50:11Z

This was held up because of lack of clarity around the check - but the merging of #28967 means the check / extension is no longer relevant so I have removed it. The previous review otherwise tested this so I think it should be mergeable too @seamuslee001 - I updated the upgrader version

seamuslee001 · 2024-05-24T02:25:12Z

CRM/Financial/BAO/FinancialType.php

@@ -396,10 +396,23 @@ public static function checkPermissionToEditFinancialType($financialTypeID) {
   *
   * @todo rename this function e.g isFinancialTypeACLsEnabled.
   *
+   * @deprecated since 5.75 will be removed around 5.90


Should this have a deprecatedWarning call as well?

We actually have core calls to it still - so the removal date is probably optimistic but we can always push it back - but I want people to still see it in their IDE

eileenmcnaughton · 2024-05-24T21:05:48Z

Just rebased this but it was passing - should be good to merge now I think

eileenmcnaughton · 2024-06-05T14:44:28Z

@colemanw can you give this MOP since all the issues are resolved now

civibot bot added the master label Feb 10, 2024

eileenmcnaughton force-pushed the fin_expose branch from 405dbf7 to 0c3594c Compare February 23, 2024 20:37

eileenmcnaughton changed the title ~~[wip] Unhide financial acls, disable if not required.~~ Unhide financial acls, disable if not required. Feb 23, 2024

eileenmcnaughton force-pushed the fin_expose branch 2 times, most recently from 2ccddf1 to 4b5563d Compare March 7, 2024 01:22

eileenmcnaughton force-pushed the fin_expose branch from 4b5563d to 0695ef7 Compare March 15, 2024 03:01

colemanw reviewed Mar 20, 2024

View reviewed changes

eileenmcnaughton force-pushed the fin_expose branch from 0695ef7 to b195ca8 Compare March 20, 2024 22:44

jitendrapurohit reviewed Apr 1, 2024

View reviewed changes

eileenmcnaughton force-pushed the fin_expose branch 3 times, most recently from 2e3f812 to 01e6b7d Compare May 24, 2024 00:48

seamuslee001 reviewed May 24, 2024

View reviewed changes

eileenmcnaughton force-pushed the fin_expose branch 3 times, most recently from c3b788f to e4dd6b3 Compare May 24, 2024 11:14

eileenmcnaughton force-pushed the fin_expose branch from e4dd6b3 to 2ef203c Compare May 24, 2024 21:06

Unhide financial acls, disable if not required.

8771548

eileenmcnaughton force-pushed the fin_expose branch from 2ef203c to 8771548 Compare June 5, 2024 13:56

colemanw added the merge on pass label Jun 5, 2024

eileenmcnaughton merged commit b3824b7 into civicrm:master Jun 6, 2024
1 check passed

eileenmcnaughton deleted the fin_expose branch June 6, 2024 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unhide financial acls, disable if not required. #29360

Unhide financial acls, disable if not required. #29360

eileenmcnaughton commented Feb 10, 2024 •

edited

Loading

civibot bot commented Feb 10, 2024

eileenmcnaughton commented Mar 15, 2024

colemanw Mar 20, 2024

colemanw Mar 20, 2024

eileenmcnaughton Mar 20, 2024

eileenmcnaughton Mar 26, 2024

seamuslee001 commented Mar 20, 2024

eileenmcnaughton commented Mar 20, 2024

jitendrapurohit Apr 1, 2024

eileenmcnaughton Apr 1, 2024

eileenmcnaughton May 24, 2024

eileenmcnaughton commented May 24, 2024

seamuslee001 May 24, 2024

eileenmcnaughton May 24, 2024 •

edited

Loading

eileenmcnaughton commented May 24, 2024

eileenmcnaughton commented Jun 5, 2024

Unhide financial acls, disable if not required. #29360

Unhide financial acls, disable if not required. #29360

Conversation

eileenmcnaughton commented Feb 10, 2024 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Feb 10, 2024

eileenmcnaughton commented Mar 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Mar 20, 2024

eileenmcnaughton commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented May 24, 2024

Choose a reason for hiding this comment

eileenmcnaughton May 24, 2024 • edited Loading

Choose a reason for hiding this comment

eileenmcnaughton commented May 24, 2024

eileenmcnaughton commented Jun 5, 2024

eileenmcnaughton commented Feb 10, 2024 •

edited

Loading

eileenmcnaughton May 24, 2024 •

edited

Loading