dev/core#1652 - Check for change case type activity on case activity form doesn't work

[demeritcowboy]

Overview

https://lab.civicrm.org/dev/core/issues/1652

This isn't the thing I was looking to fix but getting this out of the way will make it less distracting. I'm not sure what the scenario for this check is in real life but if all your case types are disabled then the intent seems to be that when you try to change a case type it will warn you when go to create a Change Case Type activity, but it's flawed in a couple ways. Given the rarity of the situation I've opted for the simple route of making the field required, so at least you don't get a fatal error on save.

To reproduce:

1. Create a case.
2. Disable all your case types.
3. Go back to manage case and create a Change Case Type activity.
4. Just click Save.
5. Before it would give a fatal error. Now it says field is required.

Technical Details

Detailed in lab ticket. Has name/label issues as well as a check against !something which seems to be wrong.

Since one of the deleted lines is an assignment of $session, I checked to see if it was used outside the deleted lines but it isn't.

Comments

Has test, which has nothing to do with this issue but it was ... hard ... so I opted for any kind of test. I'd argue the attached test is more useful, since it's for contact search which is used everywhere.

[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]

jenkins retest this please

[jaapjansma]

test this please

[jaapjansma]

• General standards
  • Explain (r-explain)
    • PASS : The goal/problem/solution have been adequately explained in the PR.
  • User impact (r-user)
  • Documentation (r-doc)
    • PASS: The changes do not require documentation.
  • Run it (r-run)
    • PASS: We tested it in dmaster and in test from this PR and followed the instructions from @demeritcowboy
• Developer standards
  • Technical impact (r-tech)
    • PASS: The change preserves compatibility with existing callers/code/downstream.
  • Code quality (r-code)
    • PASS: The functionality, purpose, and style of the code seems clear+sensible.
  • Maintainability (r-maint)
    • PASS: The change sufficiently improves test coverage.
  • Test results (r-test)
    • PASS: The test results are all-clear.

@eileenmcnaughton or @mattwire could one of you merge this PR?