Web Client: Potential AngularJS UI Scoping Issues

Bug #1696238 reported by Jason Boyer on 2017-06-06
20
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Evergreen
High
Unassigned

Bug Description

(Excuse for some phrasing: This is the bug-i-fication of an email sent to open-ils-dev)

As I was working on https://bugs.launchpad.net/evergreen/+bug/1642035 I managed to get things working without too much trouble except for the SMS option. No matter what I did changes in its value weren't reflected until in a fit of flailing about I commented out the ng-if="org_settings['sms.enable']" directive from the span around it; *then* everything worked as expected. Several annoying Google searches later and I find myself here: https://github.com/angular/angular.js/wiki/Understanding-Scopes , which explains that there are multiple directives that introduce a new scope and while references to parent objects work as expected within this child scope, bare primitives (the JS equiv of a perl scalar, basically) on $scope are *copied* to the child scope and so changes to them don't make it back up the chain. So when you do something like this: <div ng-if="showme"><input type="checkbox" ng-model="enabled"/></div> the controller's interpretation of $scope.enabled will have nothing to do with the actual state of that checkbox because the child scope will have a copy of the parent's primitive, not a reference to it.

To avoid being surprised by this there is a semi-official best practice of always having a '.' in your ng-model directives (mentioned in the above link). ([] also works because a JS array is an instance of an Array object) So the above example would work fine if it were <div ng-if="showme"><input type="checkbox" ng-model="uiState.enabled"/></div> instead.

Which brings me to the actual point (only 3 paragraphs in...) If you execute ` grep -rE 'ng-model="[a-zA-Z_]+"' * ` at Open-ILS/src/templates/staff/, back come 72 instances where we use an unadorned primitive in an ng-model directive. *Most* of these are probably ok because they're in the same scope as everything else around them but they're only an enclosing ng-(if, switch, repeat, etc.) away from becoming totally disconnected from the outside world which will make regressions a real possibility and a real pain to track down. (For instance, I looked at https://bugs.launchpad.net/evergreen/+bug/1665465 shortly after 1642035 and it's the same thing, there's an ng-if that prevents the controller from ever seeing that you checked the box.)

Attached is the output of ` git grep -B3 -P 'ng-model="[a-zA-Z_]+"' ` run in an Evergreen source tree showing all of the places we're currently using an ng-model directive with a primitive value. Using -B gives a little bit of context, so I can see that the value of show_print_dialog checkbox in circ/patron/t_checkout.tt2 isn't connected to it's controller. That and the hold_sms_notify checkbox are the only ones that stand out from just 3 lines of context but each of these files (and any new ones that start showing up from that git grep) warrant scrutiny and if at all possible changing their ng-model directive to reference an object if at all possible. I realize some of the shared templates in src/templates/staff/share/ may not be so easily modified; this also means they have to be used with care, because if they are used within the following directives they'll likely be unusable: ng-if, ng-switch, ng-repeat, ng-include, and ng-view.

Helpful links to explain the whats and whys of the above:
Best Practices: https://github.com/angular/angular.js/wiki/Best-Practices
Understanding Scopes: https://github.com/angular/angular.js/wiki/Understanding-Scopes

Mike Rylander (mrylander) wrote :

I believe that replacing ng-if with ng-show will also correct any scope issues, as show doesn't create a new isolate scope. I ran into the ng-if issue myself last week.

Andrea Neiman (aneiman) on 2017-07-19
tags: added: webstaffclient
Andrea Neiman (aneiman) on 2017-09-07
Changed in evergreen:
milestone: none → 3.next
tags: added: angular
Jane Sandberg (sandbej) on 2019-04-14
summary: - Web Client: Potential Angular UI Scoping Issues
+ Web Client: Potential AngularJS UI Scoping Issues
tags: removed: angular
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers