Merge lp://staging/~gary/launchpad/accordionoverlay-server into lp://staging/launchpad
Status: | Merged |
---|---|
Approved by: | Gary Poster |
Approved revision: | no longer in the source branch. |
Merged at revision: | 12652 |
Proposed branch: | lp://staging/~gary/launchpad/accordionoverlay-server |
Merge into: | lp://staging/launchpad |
Diff against target: |
1154 lines (+492/-133) 20 files modified
lib/canonical/launchpad/interfaces/_schema_circular_imports.py (+3/-0) lib/canonical/launchpad/webapp/__init__.py (+0/-2) lib/lp/bugs/browser/bugsubscription.py (+12/-5) lib/lp/bugs/browser/bugtarget.py (+10/-7) lib/lp/bugs/browser/structuralsubscription.py (+92/-0) lib/lp/bugs/browser/tests/test_bugsubscription_views.py (+3/-5) lib/lp/bugs/browser/tests/test_bugtarget_subscription.py (+3/-5) lib/lp/bugs/browser/tests/test_expose.py (+197/-0) lib/lp/bugs/configure.zcml (+0/-1) lib/lp/bugs/interfaces/bug.py (+3/-9) lib/lp/bugs/interfaces/bugsubscriptionfilter.py (+4/-1) lib/lp/bugs/interfaces/structuralsubscription.py (+28/-5) lib/lp/bugs/model/bug.py (+2/-6) lib/lp/bugs/model/bugsubscriptionfilter.py (+24/-8) lib/lp/bugs/model/structuralsubscription.py (+41/-11) lib/lp/bugs/model/tests/test_bug.py (+0/-34) lib/lp/bugs/model/tests/test_bugsubscriptionfilter.py (+25/-14) lib/lp/bugs/tests/test_structuralsubscription.py (+33/-17) lib/lp/bugs/tests/test_structuralsubscriptiontarget.py (+2/-2) lib/lp/registry/browser/product.py (+10/-1) |
To merge this branch: | bzr merge lp://staging/~gary/launchpad/accordionoverlay-server |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Graham Binns (community) | code | Approve | |
Review via email:
|
Commit message
[r=gmb][no-qa] The server side components of the ~yellow/
Description of the change
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
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.
= addBugSubscript
Work by Benji York; pre-imp with myself.
lib/lp/
lib/lp/
lib/canonical/
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 ``addBugSubscri
= Cleanups for their own sake =
lib/canonical/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
= Add functions to expose subscription information to our views for JavaScript =
Work by Benji York with additions from myself.
lib/lp/
lib/lp/
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_
* We need to know what subscriptions and filters the current user has (``expose_
* 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_
* person_
* 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_
= Modify views to expose subscription information to JS appropriately =
Work by Benji York, with some tweaks from me.
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
Using the tools listed above, we have a couple of pages that are ready for some client side code in the next branch.
* BugSubscription
* TargetSubscript
* In both views, I had added the structural_
* 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.getStructu
Work by myself.
lib/lp/
lib/lp/
lib/lp/
lib/lp/
The only code using getStructuralSu
= Deleting the last structural subscription filter deletes the structural subscription =
Work by myself.
lib/lp/
lib/lp/
lib/lp/
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_
Work by myself.
lib/lp/
lib/lp/
lib/lp/
* 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_
* 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_
Work by myself.
This naming seemed to better parallel the naming of get_structural_
* The current implementation of IStructuralSubs
= Normalize statuses and filters on bug subscription filters =
Work by myself.
lib/lp/
lib/lp/
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 userHasBugSubsc
Work by Benji York.
lib/lp/
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 userHasBugSubsc
Thank you!
Hi Gary,
There are very few things wrong with this branch, and of the notes I've
made below one's a vague suggestion, one's a comment and only the last
two are things that I think need to be acted on before this lands. Great
work by both everyone involved.
[1]
Here:
56 + def initialize(self): iptionListView, self).initialize() subscriptions_ for_bug( user_administer ed_teams_ to_js(self. request, self.user) user_subscripti ons_to_ js( enum_to_ js(self. request, BugTaskImportance, 'importances') enum_to_ js(self. request, BugTaskStatus, 'statuses')
57 + super(BugSubscr
58 + subscriptions = get_structural_
59 + self.context.bug, self.user)
60 + expose_
61 + expose_
62 + self.user, subscriptions, self.request)
63 + expose_
64 + expose_
65 +
and here:
117 + def initialize(self): scriptionView, self).initialize() user_administer ed_teams_ to_js(self. request, self.user) subscriptions_ for_target( user_subscripti ons_to_ js( enum_to_ js(self. request, BugTaskImportance, 'importances') enum_to_ js(self. request, BugTaskStatus, 'statuses')
118 + super(TargetSub
119 + expose_
120 + subscriptions = get_structural_
121 + self.context, self.user)
122 + expose_
123 + self.user, subscriptions, self.request)
124 + expose_
125 + expose_
126 +
the code is almost identical (save for where the `subscriptions` list
comes from. Might it be worth wrapping this stuff up in a mixin,
something like:
class StructuralSubsc riptionJSMixin: subscription data in JS.
"""A mixin that exposes structural-
Descendants of this mixin must define a `subscriptions` property
that returns a list of the subscriptions to cache in the JS of the
page.
"""
def initialize(self):
super( StructuralSubsc riptionJSMixin, self).initialize()
expose_ user_administer ed_teams_ to_js(self. request, self.user)
expose_ user_subscripti ons_to_ js(
self. user, self.subscriptions, self.request)
expose_ enum_to_ js(self. request, BugTaskImportance, 'importances')
expose_ enum_to_ js(self. request, BugTaskStatus, 'statuses')
(Though that's only really worth the effort if we are going to do this
anywhere else, I suppose).
[2]
214 + record = info[target] = dict(
I really don't like this idiom; it took me about 30s to work out what
was actually going on (admittedly, my headcold probably isn't helping).
I'd prefer it if we had this as:
record = dict(...)
info[target] = record
[3]
249 + # It's a start.
A win for commentary, there.
[4]
435 + # [{'filters': [{'filter': <....BugSubscri ptionFilter object at ...>, is_team' : True, .../~team- name... ', team_admin' : True}], 127.0.0. 1/product- name... '}]
436 + # 'subscriber_
437 + # 'subscriber_link': u'.../api/
438 + # 'subscriber_title': u'Team Name...',
439 + # 'user_is_
440 + # 'target_title': u'title...',
441 + # 'target_url': u'http://
I couldn't tell whether this is deliberately left in or not. If not,
let's lose it, if so, please add some comment...