crmRouteBinder - Don't convert arrays to objects

[colemanw]

Overview

This fixes a subtle bug I encountered while writing the Api4 explorer.

Before

Arrays would be mangled because the function _.isObject([]) will return true.

After

Arrays and objects are both treated nicely.

[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).

[colemanw]

@totten I think you're the only one qualified to review this one. I'd like to get this merged before the 5.5 branch gets created so we can bundle in api4 sans-bugs.

[totten]

WRT to r-run, the main way I can think to evaluate this is to use the CiviCase UI and check the history management -- e.g. click-around with many different filters, then use the browser URL-bar and back/forward buttons to see how it behaves. On that front, it seems to be working.

However, in terms of the logic, I have a doubt. Let's restate the basic logic as pseudocode:

• To determine the value of a field....
  • Check if the field is supplied in the URL, If so, decode and use that.
  • Otherwise, use the default... If the default is an object, then clone it (angular.extend(...)).

Why bother with any cloning at all? Once we've setup the form, the user can interactively change the value of the field in the UI. Good for her. But if she makes a change, it should only apply to her view (and the active URL) -- it shouldn't propagate back to change the default value. You get that behavior automatically with primitive values (string/int), but for objects you have to clone.

I think the same logic would apply to arrays that applies to objects -- but maybe angular.extend doesn't work well for cloning arrays. Would something like the following address the subtle bug in api4 explorer?

OLD:

value = _.isObject(options.default) ? angular.extend({}, options.default) : options.default; 

NEW (untested pseudocode):

if (_.isArray(options.default)) {
  value = options.default.slice(0); // Array clone
} else if (_.isPlainObject(options.default)) {
  value = angular.extend({}, options.default); // Object clone
} else {
  value = options.default; // Primitive value
}

[colemanw]

Thanks for the review @totten - my goto function for this purpose is _.cloneDeep() which works on arrays as well as objects. So maybe we should just switch to that.
_.cloneDeep() also works on primitives; they just pass through unchanged.

[colemanw]

@totten I've pushed up that change. Much simpler!

[totten]

Actually, maybe https://code.angularjs.org/1.5.11/docs/api/ng/function/angular.copy would better...

(EDIT) Hadn't seen your comment/push when posting that. I've got no opinion on whether _.cloneDeep or angular.copy is better.

[colemanw]

From reading the docs, they both do exactly the same thing, so either way I think we're good :)

[totten]

• (r-explain) Soft Pass
• (r-test) Undecided : Pending results
• (r-code) Pass
• (r-doc) Pass
• (r-maint) Pass
• (r-run) Pass: Did various click-around testing in the CiviCase 5 UI, making heavy use of different filters and navigating back/forth.
• (r-user) Pass: No change to design of UX
• (r-tech) Soft Pass: This is basically a drop-in replacement that doesn't significantly change the data. There is a theoretical difference if (somehow) a default value included Angular metadata (default.$$foo), but this seems quite unlikely, and I couldn't spot any examples (based on quick grep of CiviCase).