CRM-16526 [needs-review] Financial ACL API tests #7818

Edzelopez · 2016-02-16T11:35:06Z

CRM-16526: ACLs for Financial Types

eileenmcnaughton · 2016-02-16T17:59:19Z

api/v3/LineItem.php

@@ -44,8 +44,28 @@
 *   api result array
 */
 function civicrm_api3_line_item_create($params) {
-  $params = CRM_Contribute_BAO_Contribution::checkTaxAmount($params, TRUE);
-  return _civicrm_api3_basic_create(_civicrm_api3_get_BAO(__FUNCTION__), $params);
+  if (CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus()) {


Can't this go in the BAO? the api should just be a wrapper for the BAO

eileenmcnaughton · 2016-02-19T03:42:44Z

Great to see the test!

What is worrying me about all this extra logic going into the api layer is that apart from being against our general principle of trying to put all logic in the BAO layer you are also creating future-you or someone like you work because api v4 will drop all of the api layer create logic and you will either have to move it to the BAO at that point or negotiate some exception on api v4.

Of course moving it to the BAO is unit tested but it would require at least some UI click around checks too to check nothing wierd is happening.

Edzelopez · 2016-02-19T04:09:57Z

I agree @eileenmcnaughton, but I checked and the lineitem get API does not rely on the BAO. It just does a basic DAO get (). So unless we change that, we can't really modify the way the line items are being retrieved. Of course if there's anot her way this can be done please let me know :)

eileenmcnaughton · 2016-02-19T04:14:55Z

I was looking more at create than get (which does have a BAO function to add things to).

The issue with get leveraging ACLs has recently been looked at by @colemanw and he can hopefully advise as to whether that approach is mature enough to add here.

I'm also kind of aware that you used an ACL approach that is more drupal-like than CiviCRM-like but I didn't dig into that side of it - I suspect the difference won't have much impact thought

Edzelopez · 2016-02-19T11:56:24Z

Thanks for the suggestion @eileenmcnaughton
I've added the check to perform at BAO level for create rather than API, although I have set financial_type_id to be a required field for creating/editing a lineitem.

eileenmcnaughton · 2016-02-19T12:25:54Z

CRM/Price/BAO/LineItem.php

@@ -67,6 +69,12 @@ public static function create(&$params) {
    if ($id) {
      unset($params['entity_id'], $params['entity_table']);
    }
+    if (CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus()) {
+      CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($types, $op);
+      if (!in_array($params['financial_type_id'], array_keys($types))) {


You need to only do this if check_permissions = TRUE

Edzelopez · 2016-02-22T13:10:44Z

Jenkins, retest this please

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

… for line item ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

…n if even one is non-permissioned ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

…sioned line items for contribution ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

…PI for create ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

… added only if ACL is enabled ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

… for lineitems ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

Edzelopez · 2016-02-26T05:36:14Z

Jenkins, retest this please

…tead of logic in API ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

pradpnayak · 2016-03-03T11:21:38Z

Hi @eileenmcnaughton ,

We are having an issue with getting Jenkins to pass.
When running all tests for api_v3_FinancialTypeACLTest via PHPUnit on our local machines, we get a successful assertion after the tests have finished completion. Unfortunately, this is not the same when we try to merge the PR to master and let Jenkins test the API test there. Such can be seen from here.

We were wondering if you have any idea what we are doing wrong here and would very much appreciate ideas as to how to make sure that there is consistency of our tests between our local machine and Jenkins.

eileenmcnaughton · 2016-03-03T11:31:29Z

See if it works when running api_v3_AllTests - probably some other test is leaving something behind that is messing with your test so it fails when they all run but not with only some. If it does fail there is a clever way to run selectively but I just delete around half the test classes locally & re-run until I've homing in on the clashing class

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

Edzelopez · 2016-03-09T02:46:29Z

@eileenmcnaughton you were right, there was a caching issue with one of the static variables that was failing when run with the other tests. I've fixed it now.

JoeMurray · 2016-03-16T20:12:19Z

@eileenmcnaughton would you be able to merge this for us? It's been waiting for over a week.

CRM-16526 [needs-review] Financial ACL API tests

civicrm-builder added the master label Feb 16, 2016

eileenmcnaughton reviewed Feb 16, 2016
View reviewed changes

Edzelopez changed the title ~~CRM-16526 [DO NOT MERGE - PR submitted to facilitate testing] Financial ACL API tests~~ CRM-16526 [needs-review] Financial ACL API tests Feb 18, 2016

eileenmcnaughton reviewed Feb 19, 2016
View reviewed changes

pradpnayak force-pushed the CRM-16526-1 branch from 992b95f to 3423f5a Compare February 22, 2016 13:45

Edzelopez added 18 commits February 22, 2016 23:41

CRM-16526 Added financial ACL api test

147f096

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Bug fix for permission for API

8715ff9

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Added permission for lineitem API

1923106

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Temporary fix to prevent rollback when no permission…

313fc1e

… for line item ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Added lineitem contribution financial api test

dee383f

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 More work done on ft acl test

d8bf43f

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 More work done on ft acl test

be23f12

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Added exception for creating line item / contributio…

b8758b4

…n if even one is non-permissioned ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Added test to check creation / failure of non-permis…

8b92461

…sioned line items for contribution ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Added restriction for retrieving lineitems

2344a42

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Added restriction for lineitem based on financial api

00a0c61

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Added restrictions for deleting line items

8cb8ae9

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Added check for delete

a3fa034

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Fixed jenkins style warnings

9716cd7

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Fixed more jenkins style warnings

3007d34

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Added permission check in Lineitem BAO rather than A…

513e587

…PI for create ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Bug fix for exception

f8acc36

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Modified get lineitem permissioning

10dafdd

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

Edzelopez and others added 6 commits February 22, 2016 23:41

CRM-16526 CIVI-3 Bug fix for default permission

2808d9c

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Removed mandatory lineitem check from create api and…

cfba316

… added only if ACL is enabled ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Added check only when check permissions is specified…

93b2190

… for lineitems ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Bug fixes

b8f2d49

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Removed unintentional debug

5cac986

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

--CRM-16526, code improvement

50d8cef

pradpnayak force-pushed the CRM-16526-1 branch from 3454dc3 to 50d8cef Compare February 22, 2016 18:12

CRM-16526 CIVI-3 Added BAO function to filter the financial types ins…

ac5a0d1

…tead of logic in API ---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

Edzelopez added 4 commits March 4, 2016 13:18

CRM-16526 CIVI-3 Enabled setup

e2054d8

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Fixed Jenkins test failure

f4a331f

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Fixed Jenkins test failure

87e14d2

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

CRM-16526 CIVI-3 Fixed Jenkins test failure

a01a892

---------------------------------------- * CRM-16526: ACLs for Financial Types https://issues.civicrm.org/jira/browse/CRM-16526

eileenmcnaughton added a commit that referenced this pull request Mar 16, 2016

Merge pull request #7818 from JMAConsulting/CRM-16526-1

c067566

CRM-16526 [needs-review] Financial ACL API tests

eileenmcnaughton merged commit c067566 into civicrm:master Mar 16, 2016

pradpnayak deleted the CRM-16526-1 branch April 27, 2016 08:51

MegaphoneJon mentioned this pull request Nov 21, 2017

Document financial permissions civicrm/civicrm-user-guide#241

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-16526 [needs-review] Financial ACL API tests #7818

CRM-16526 [needs-review] Financial ACL API tests #7818

Edzelopez commented Feb 16, 2016

eileenmcnaughton Feb 16, 2016

eileenmcnaughton commented Feb 19, 2016

Edzelopez commented Feb 19, 2016

eileenmcnaughton commented Feb 19, 2016

Edzelopez commented Feb 19, 2016

eileenmcnaughton Feb 19, 2016

Edzelopez commented Feb 22, 2016

Edzelopez commented Feb 26, 2016

pradpnayak commented Mar 3, 2016

eileenmcnaughton commented Mar 3, 2016

Edzelopez commented Mar 9, 2016

JoeMurray commented Mar 16, 2016

CRM-16526 [needs-review] Financial ACL API tests #7818

CRM-16526 [needs-review] Financial ACL API tests #7818

Conversation

Edzelopez commented Feb 16, 2016

eileenmcnaughton Feb 16, 2016

Choose a reason for hiding this comment

eileenmcnaughton commented Feb 19, 2016

Edzelopez commented Feb 19, 2016

eileenmcnaughton commented Feb 19, 2016

Edzelopez commented Feb 19, 2016

eileenmcnaughton Feb 19, 2016

Choose a reason for hiding this comment

Edzelopez commented Feb 22, 2016

Edzelopez commented Feb 26, 2016

pradpnayak commented Mar 3, 2016

eileenmcnaughton commented Mar 3, 2016

Edzelopez commented Mar 9, 2016

JoeMurray commented Mar 16, 2016