The Javascript methods used to manage adding a comment on the expandable row of the diff pages should be refactored into a proper YUI widget.

Bug #823777 reported by Raphaël Badin
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Launchpad itself
Fix Released
Low
Raphaël Badin

Bug Description

This will allow testing for these functions and may as well be a good occasion to refactor that code a bit:

"""
827 + var slide = function(direction) {
828 + // Slide out if direction is true, slide in if direction
829 + // is false, otherwise do the opposite of what's being
830 + // animated right now.
831 + if (slide_anim === null) {

Why the "do the opposite" option? Shouldn't the caller know what the desired end state is? In my experience, toggle functionality in code is almost always a bad idea. It comes with a potential for inconsistent states. I find it's usually better for the calling code to figure out what it wants, and then tell other code unequivocally to go do it. What you want is a policy decision, and how you get it is an implementation decision. Moving the policy decision into the implementation is asking for trouble; allowing the policy decision to be either in the caller or the callee adds an extra layer of confusion.
"""
Plus, of course, in this case:

838 + else {
839 + if (Y.Lang.isBoolean(direction)) {
840 + slide_anim.set("reverse", !direction);
841 + }
842 + else {
843 + slide_anim.set('reverse', !slide_anim.get('reverse'));
844 + }

This opens the door for pseudo-boolean values (e.g. because a function that answers a boolean question about the DOM may actually return a Node for "true" or null for "false") to trigger a toggle rather than a specific direction. In that situation, this code effectively flips a coin to pick an animation direction. As per Sod's Law, it will do the right thing in testing but the wrong thing in production. This may not be the right branch to ditch the toggle functionality, but I'd at least put up large red flags around it and think about how to kill it.

Related branches

Gavin Panella (allenap)
tags: added: javascript ui
Graham Binns (gmb)
Changed in launchpad:
importance: Undecided → Low
Revision history for this message
Launchpad QA Bot (lpqabot) wrote :
Changed in launchpad:
assignee: nobody → Raphaël Victor Badin (rvb)
tags: added: qa-needstesting
Changed in launchpad:
status: Triaged → Fix Committed
Raphaël Badin (rvb)
tags: added: qa-untestable
removed: qa-needstesting
William Grant (wgrant)
Changed in launchpad:
status: Fix Committed → Fix Released
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.