Web Client: Potential AngularJS UI Scoping Issues

Bug #1696238 reported by Jason Boyer
20
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Evergreen
Confirmed
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

Revision history for this message
Jason Boyer (jboyer) wrote :
Revision history for this message
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)
tags: added: webstaffclient
Andrea Neiman (aneiman)
Changed in evergreen:
milestone: none → 3.next
tags: added: angular
summary: - Web Client: Potential Angular UI Scoping Issues
+ Web Client: Potential AngularJS UI Scoping Issues
tags: removed: angular
Changed in evergreen:
status: New → Confirmed
tags: added: angular usability
removed: webstaffclient
Gina Monti (gmonti90)
tags: added: angularjs
removed: angular
Revision history for this message
Jason Boyer (jboyer) wrote :

Seeing this in my email reminded me 1, that it exists; and 2, that it's not a concern in the Angular (/eg2/) portions of the client. So in time this bug will fade from a rarely seen reminder to nothing but "No longer affects x.y.z" + Invalid as the AngularJS client is phased out. (Or perhaps before if anyone is interested in making such a change now.)

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.