yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

On May 19, 2011, at 6:35 AM, Danilo Šegan wrote:

> Hi Gary,
> 
> У сре, 18. 05 2011. у 17:48 -0400, Gary Poster пише:
> 
>> I'm cc'ing everyone else so they can see where we are.  Everyone else,
>> if you have some spare cycles to help out, check in with Danilo or me
>> to see if we can give you a branch to do.
>> 
>> I figured out what was going on with the mute UI, and did a spike to
>> get it to work experimentally.  It turns out there were a number of
>> changes that needed to be made.
>> 
>> The branch is lp:~gary/launchpad/bug-772763-remove-unmute-dialog
> 
> I looked at your branch a bit.  I find it weird that you modify unmute()
> call to return the previous direct subscription considering that one is
> already available in the page JS in my tests (eg. with fresh db-devel,
> try subscribing at a level different than default, muting that
> subscription, refreshing the page [to confirm the data is available from
> the get-go], then pop up the 'subscribe' dialog and notice how the
> 'unmute and update my subscription' shows the correct bug notification
> level).

The subscribe dialog is drawn by the server though, not JS, *after* you click the "subscribe" or "edit subscription" links.  So the information that the overlay has is not inherently available to the unmute code.  Or am I missing something?

> 
> IOW, server-side code already loads the sufficient information into the
> page.  Not saying it's pretty, but just saying it's there already. :)

I didn't see it available to the unmute JS, but I may have simply missed it.  Is it in the LP.cache and updated there?  I didn't see sufficient code for that to happen either, but that stuff seems to work in mysterious ways.  Let me know what you had in mind.

> 
>> We have a mostly working spike (I was clicking around on
>> ttps://bugs.launchpad.dev/firefox/+bug/1, fwiw).  I think we now need
>> to redo it with tests, dividing it up into smaller branches.
> 
> Ideally, we'd move all the mute stuff into a separate module (at least
> imho).  Eg. lp/bugs/javascript/bug_muting.js.

I'm not really interested in that myself.  It's, what, three functions now?  And in a module that we'll need to significantly refactor for the next bug.  I won't stop you, but I'm not excited to do it myself.

> 
> All of the Subscription class use for bug muting seems very much
> unnecessary imho, but I didn't dare change it previously.

So...you are glad that I ripped it out?  Or concerned?  Or both? :-)

> 
> Your branch also changed handle_advanced_subscription_layer which I
> didn't expect you to touch (i.e. since you wouldn't be popping up a
> dialog, you wouldn't need to change it).  I think that brings up one big
> issue with the bugtask_index_portlets.js as it is: one always wants to
> go and clean it up some more :)

heh.

The reason I touched the dialog is twofold.  First, I decided that having the "subscribe" link while you were muted but had a hidden description was going to be helpful for some people in some situations (notably, if you have forgotten that you muted, and are just looking for a "subscribe" link, this will tell you what's going on).  Second, keeping that functionality made the branch smaller, from a "get this bug done" perspective.

> 
>> In particular, there are some server changes that need to be made.  I
>> think they can be tested simply.  These could be one or more server
>> branches.  I wonder if the queries (in particular the also notified
>> code) need to be made more performant.  Then again, they might need to
>> be rethought more deeply for bug 772754.
> 
>> Then we need a separate JS branch.  I'll probably try to do a very
>> basic, minimal JS test, since we are probably going to trash so much of
>> this JS code for bug 772754.
>> 
>> If you get done with your other task and want to take some of this
>> tomorrow morning, feel free.  Maybe a first step would be to get my
>> branch running and see if you agree with some of the UI decisions I
>> made--or at least don't strongly disagree, since we are on a
>> timetable.  I'll jump in when I get going, though tomorrow is my call
>> day, so my availability will be somewhat limited.
> 
> I am trying to make sure that my in-progress branch and your in-progress
> branch make sense.  I'll probably merge them together because of all the
> refactorings you did.

If you mean a full merge, I'm a bit concerned by that.  As I said, I was hoping we could land smaller branches, and I regarded what I did as a spike.  Maybe you merge the two just to verify that we are on the right track, and then we use that to determine what next branches we actually need?

Gary