dev/core#896 - fix notice warning on closing a case

[demeritcowboy]

Overview

See https://lab.civicrm.org/dev/core/issues/896. When you close a case, by making a change case status activity, it gives a notice warning. Actually for any case status change.

Before

Notice warning and test fails because of notice warning.

After

No warning and test passes.

Comments

It looks like there's a lot more going on in this PR than there actually is. The actual fix is a one line change that just checks if the array value being accessed exists or not by adding a !empty. The rest is all related to the test and doesn't introduce any functional changes.

The NFC in the existing test helper tests/phpunit/CRM_Case_BAO_CaseTest::createCase() is because it made little sense to me as a test - typically the client is not the same as the creator. But for backwards compatibility it will still function as before if your test calls it without the second parameter.

The change in CRM_Case_Form_Activity::postProcess() is to preserve backwards compatibility with any broken code (I didn't see any, and PR #2077 didn't see any either) while still allowing the postProcess parameter to be passed in by the unit test without getting overwritten. Previously, anything passed in was overwritten. Now, anybody who is passing something in outside of a unit test will still have it overwritten, so if that broken code was working before it will still work.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

I can't see why the CRM_Contact_Form_Task_EmailCommonTest test is failing. It passes when I run it locally, and the error doesn't seem to be related?
addRule("attachFile_1", "File size should be less than 3 MByte(s)" Rule 'maxfilesize' is not registered in HTML_QuickForm::addRule()

Trying again - Jenkins test this please

[demeritcowboy]

I'm confused. I can't reproduce the fail locally. Changing title to WIP until I figure this out.

[demeritcowboy]

Update: I've narrowed the test failure problem down to the new test. While the new test itself passes fine, deep inside somewhere it does something that affects maybe a static variable or something that carries over between tests, so when the other email test runs, quickform or whatever is in a messed up state, so it makes that other test fail. It doesn't seem to be something in $GLOBALS, which is why I think it might be a static, or maybe a require_once that isn't happening because it's already happened once. It's something like the quickform validation ruleset isn't getting re-initialized for the following test like it would on a new page request.

[demeritcowboy]

I got it to work by (and quoting star trek) "changing the conditions of the test". It runs in a separate process which is maybe not a good precedent but I'd weight it against the value of the test which directly checks a handful of things not covered elsewhere and indirectly checks a few more (e.g. if an error or notice was thrown during the closing of case role relationships during case closure).

Thoughts?

[pradpnayak]

Can be also written as

if (CRM_Utils_Array::value('updateLinkedCases', $params) === '1')) {

[demeritcowboy]

Hi @pradpnayak I've updated it although I've always avoided that function for a few reasons. Maybe I will make a gitlab ticket about it.

[demeritcowboy]

@pradpnayak Just for interest: https://lab.civicrm.org/dev/core/issues/937

[demeritcowboy]

I think the test failure is actually a timing issue with the way that particular test calls the api - the fcs hash is generated twice so if it happens during different values of time() it'll get different results. I will make a lab ticket (https://lab.civicrm.org/dev/core/issues/938).

[demeritcowboy]

Jenkins test this please

[eileenmcnaughton]

Well this is passing & I agree the changes to the actual code are minimal & save.

Generally I extract the postProcess function into a submit function & call that directly from the tests rather than playing with unit test params. However, you got this to pass & added coverage so that's great.