Merge lp://staging/~gary/launchpad/accordionoverlay-server into lp://staging/launchpad

[r=gmb][no-qa] The server side components of the ~yellow/launchpad/accordionoverlay branch. Adds structural subscription functions, simplifies some related code, and adds json helpers for the upcoming client branch.

This branch contains the vast majority of the server side components of the integration branch that the ~yellow squad has been working on (lp:~yellow/launchpad/accordionoverlay). As such, much of this is work from Benji York, though I contributed to it significantly as well.

A client-side branch is forthcoming.

make lint is happy except with ./lib/canonical/launchpad/webapp/__init__.py, which has a bunch of re-exports.

As to be expected, this branch has a lot of different elements to it. I'll try to guide you through them all by focusing on one functionality addition/change at a time, and identifying the pertinent parts of the diff. If you see something in the diff that you don't understand, my intent is that you should be able to search through this doc and see what the story is.

= addBugSubscriptionFilter addition =

Work by Benji York; pre-imp with myself.

lib/lp/bugs/interfaces/structuralsubscription.py
lib/lp/bugs/model/structuralsubscription.py
lib/canonical/launchpad/interfaces/_schema_circular_imports.py

In the course of working on the client-side code to add a structural subscription filter (while hiding the existence of structural subscriptions as an architectural detail), Benji came upon the problem that creating a structural subscription would create a structural subscription if it existed, and silently return an existing structural subscription if it did not. In the first case, an initial filter was created, and that is the one he should edit with the details that the user gave for the new filter. In the second case, he should create another filter and set the user's chosen configuration on that one.

Two "wrongs"--hiding the fact that nothing was created on the one hand, and trying to hide an architectural component of the system from the end user on the other--did not make for a "right". His code didn't have the information it needed to be sure it was making the right decision, especially in the face of race conditions. Benji and I talked about changing lazr.restful to expose more of what we want, and other possibilities.

We settled on adding the ``addBugSubscriptionFilter`` method as a reasonable approach. If in fact we want to hide the structural subscription as an architectural detail, then let's provide support on the server for that. We can make it possible to create a filter, and a structural subscription will be added if necessary. This fits along nicely with the other design decision that deleting a subscription's last filter deletes the subscription: the filter really is the primary unit that should be considered. Therefore, while we wished that we could wave a magic wand and change class, method, table, and row names to make this aspect of the design clearer (i.e., that structural subscriptions are now essentially relegated to simple containers of filters), we felt this was a reasonable compromise.

= Cleanups for their own sake =

lib/canonical/launchpad/webapp/__init__.py cleaned up getUtility because it was not used.

lib/lp/bugs/interfaces/bug.py moved a comment because the linter couldn't handle it's previous location, and I was tired of it complaining and didn't care enough to file a bug.

lib/lp/bugs/interfaces/structuralsubscription.py now has a comment to indicate that the bug_filters references are to IBugSubscriptionFilters.

lib/lp/bugs/interfaces/structuralsubscription.py corrects the description of addSubscription and addBugSubscription, because we do now create an initial filter.

lib/lp/bugs/model/structuralsubscription.py StructuralSubscriptionTargetMixin.addSubscription does not stash the new new filter in a variable because we don't use it.

= Add functions to expose subscription information to our views for JavaScript =

Work by Benji York with additions from myself.

lib/lp/bugs/browser/structuralsubscription.py
lib/lp/bugs/browser/tests/test_expose.py

As discussed below, we have a couple of views that will have JS (in the upcoming branch) that needs to know a fair amount of information.

 * We need to know the values of the bugtask importances and statuses, so we can show them in the filter editing UI (``expose_enum_to_js``).
 * We need to know what subscriptions and filters the current user has (``expose_user_subscriptions_to_js``). This uses the functions described above, to get a user's subscriptions to a bug or a bug target.
 * When we allow selecting the recipient of a bug notification subscription, we need to know what teams the user administers, if any, because they are the ones that the user may subscribe (expose_user_administered_teams_to_js).
 * person_is_team_admin is a helper. Perhaps it should be preceded with an underline? However, this is a generically useful functionality, and I have another branch (for the "unsubscribe in anger" story) that wants it. Perhaps it should live somewhere in registry? Thoughts/ideas/opinions would be welcome. As an aside, this function is used in a loop, and might be used for the same team repeatedly; I thought about building a cache in. My other use case is even more likely to want to have the answers cached. ...I'm not sure what I want you to do with that aside. :-)
 * The tests have some stubby/mocky approaches, and some integration-y approaches. I felt there was a good reason to have the integration ones that I include, but I highlight it in case you have concerns. I only included tests of ``expose_user_subscriptions_to_js`` that looked at one subscription at a time. It would be reasonable to ask me to test multiple simultaneous subscriptions, if you desire. What I have seemed like minimally acceptable set.

 = Modify views to expose subscription information to JS appropriately =

Work by Benji York, with some tweaks from me.

lib/lp/bugs/browser/bugsubscription.py
lib/lp/bugs/browser/bugtarget.py
lib/lp/registry/browser/product.py
lib/lp/bugs/browser/tests/test_bugsubscription_views.py
lib/lp/bugs/browser/tests/test_bugtarget_subscription.py

Using the tools listed above, we have a couple of pages that are ready for some client side code in the next branch.

 * BugSubscriptionListView can show a user's structural subscriptions for a given bug. It will be part of the upcoming "unsubscribe in anger" story.
 * TargetSubscriptionView can show a user's structural subscriptions for a given bug target. It will be the main edit page for structural subscriptions.
 * In both views, I had added the structural_subscriptions property earlier because I thought it would be used, but it was not, so I deleted it. This caused the tests for these views to become even simpler.
 * The tests for these views are very simple. I thought it was kind of OK, because we show we *can* initialize, and elsewhere we test the tools that we actually use in the initialization. However, it might not be amiss to add a test to show that the functions have in fact been called. I'll be happy to do so if requested.

= Remove IBug.getStructuralSubscriptionsForPerson =

Work by myself.

lib/lp/bugs/configure.zcml
lib/lp/bugs/interfaces/bug.py
lib/lp/bugs/model/bug.py
lib/lp/bugs/model/tests/test_bug.py

The only code using getStructuralSubscriptionsForPerson was the structural_subscriptions attribute I removed from the views, as described above. The functionality is now provided in the ``get_structural_subscriptions_for_bug`` function described elsewhere in this document. I removed the method from the appropriate bits.

= Deleting the last structural subscription filter deletes the structural subscription =

Work by myself.

lib/lp/bugs/interfaces/bugsubscriptionfilter.py
lib/lp/bugs/model/bugsubscriptionfilter.py
lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py

Earlier in our work on this feature, we had decided to implement the policy that every structural subscription must have at least one filter by making a deletion of the last filter simply clear its settings out, rather than delete it. This was because we were still thinking of these as "filters"--if you delete the last filter but you don't delete the structural subscription, that must mean that you want the unadulterated subscription without filtering; if ever structural subscription must have a filter, then we can implement that by clearing out the filter to be a no-op.

However, when we actually started working on the UI to delete these things, we discovered that this was confusing. We are trying to treat the structural subscriptions as a hidden architectural artifact. Instead, in the UI, users see filters as their actual subscriptions. From that perspective, if you delete the last "subscription" (i.e., filter), that must mean that you want to delete the subscription itself. This policy made the behavior we wanted much easier to implement, so we changed it.

= Rename/tweak get_all_structural_subscriptions -> get_structural_subscriptions_for_bug =

Work by myself.

lib/lp/bugs/model/structuralsubscription.py
lib/lp/bugs/model/bug.py
lib/lp/bugs/tests/test_structuralsubscription.py

 * I had already made a number of changes to simplify the contract of some functions so that they too either a bug or a bugtask, rather than an arbitrary set of bugtasks. Simplifying that contract made for simpler, more efficient code. I continued down that path by making get_all_structural_subscriptions (for bugtasks) into get_structural_subscriptions_for_bug.

 * I proxied this function so that the output would br security proxied. This makes sense to me because the function is intended to be used by browser code.

 * The function did return only the directly subscribed subscriptions, but now it includes team subscriptions. The only clients of this function wanted that behavior, so that's what it does now, rather than adding a new one.

= Renamed get_all_structural_subscriptions_for_target -> get_structural_subscriptions_for_target =

Work by myself.

This naming seemed to better parallel the naming of get_structural_subscriptions_for_bug.

 * The current implementation of IStructuralSubscriptionTarget.userHasBugSubscriptions (on StructuralSubscriptionTargetMixin.userHasBugSubscriptions) looks like it could use this function to be more efficient. I noticed this while I was preparing this branch for review and was a bit loathe to add yet more weight to it. Feel free to request that I file a bug, or promise to make a follow up branch, or in fact make the change here, as you desire.

= Normalize statuses and filters on bug subscription filters =

Work by myself.

lib/lp/bugs/model/bugsubscriptionfilter.py
lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py

If you set a bugfilter to filter on all statuses, or all importances, that is equivalent to filtering on none of them--except that it uses more storage in the database and leads to more expensive queries. Therefore, I normalized setting all statuses and all importances to being equivalent to setting none of them.

= export userHasBugSubscriptions in API =

Work by Benji York.

lib/lp/bugs/interfaces/structuralsubscription.py

I think that this is no longer necessary for our own work. It's probably not horrible to do, though. That said, note my previous comments on the inefficient implementation of userHasBugSubscriptions.

Thank you!