dev/core#1451 Incorrect dropdown action choices on case type listing screen

[demeritcowboy]

Overview

There's some incorrect boolean comparisons being made against strings and undefined that prevent the dropdown list from appearing or showing appropriate choices. For example you should be able to delete the stock case types from the UI, or when you disable a case type the dropdown action to enable should be there but it still says disable.

Before

After

Choices as intended.
Note also that unless you've done something in code or in the database directly to set is_reserved=1, there will almost always be a dropdown with at least something in it. I'm not sure what is_reserved means here though because you can still edit the type, but that's maybe a separate question. The stock types are not reserved.

Technical Details

The below javascript code will output true for both. And similarly if (!x) would output false for both.

var x = "0";
if (x) {console.log('true'); } else {console.log('false');}
x = "1";
if (x) {console.log('true'); } else {console.log('false');}

So for example where it used to check !caseType.is_reserved to determine whether to show Delete, it still wouldn't show it even if is_reserved="0". Note also that is_reserved might not even be present, which is why I've used !=1 which covers both situations.

The addition of the classes in list.html is to support the test.

Comments

I guess maybe nobody's questioned it because you might assume the screen doesn't let you delete the stock case types because there's something special about them. But there isn't. And not showing "Enable" is only a minor annoyance because you can just edit the type and check the box, or if you decide to click disable again anyway it actually enables it.

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

Added test. As noted at https://lab.civicrm.org/dev/core/issues/1451#note_29902 without the patch 112 out of a possible 625 assertions fail without the patch.

[jaapjansma]

Betty and I are reviewing PR's and we came across this PR. We have added a comment to the issue in gitlab as we don't know how to test this functionally.

[jaapjansma]

Betty and I reviewed this patch @demeritcowboy provided a reproduce instruction in the issue.
Below our report:

• General standards
  • Explain (r-explain)
    • PASS : The goal/problem/solution have been adequately explained in the PR and in the issue.
  • User impact (r-user)
    • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
  • Documentation (r-doc)
    • PASS: The changes do not require documentation.
  • Run it (r-run)
    • PASS: We followed the instructions given by @demeritcowboy to test this and the patch fixes the issue.
• 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, or the change is trivial enough that it does not require tests.
    • COMMENTS: Well done in providing a test
  • Test results (r-test)
    • PASS: The test results are all-clear.

@eileenmcnaughton or @mattwire are you able to merge this?

[demeritcowboy]

Thanks!