Hi Markus, I've gone through each revision in turn since the last review; I couldn't figure out how to get a cumulative diff without pulling in all the merges from devel. There are still a few things left to do to get this ready for landing, but they're not huge, and I've done some of them (a few more unit tests). It's really close though. Gavin. Revision 10114: > === modified file 'lib/lp/bugs/doc/bugtask-search.txt' > --- lib/lp/bugs/doc/bugtask-search.txt 2010-02-02 03:23:24 +0000 > +++ lib/lp/bugs/doc/bugtask-search.txt 2010-02-04 15:33:37 +0000 > @@ -65,7 +65,7 @@ > == Person bugs == > > To get all related tasks to a person call searchTasks() on the person > -pbject: > +object: > > >>> from canonical.launchpad.interfaces import IPersonSet > >>> user = getUtility(IPersonSet).getByName('name16') > Revision 10115: > === modified file 'lib/lp/registry/model/person.py' > --- lib/lp/registry/model/person.py 2010-02-03 14:18:27 +0000 > +++ lib/lp/registry/model/person.py 2010-02-04 16:34:53 +0000 > @@ -31,7 +31,6 @@ > import random > import re > import weakref > -import copy > > from zope.lifecycleevent import ObjectCreatedEvent > from zope.interface import alsoProvides, implementer, implements > @@ -838,16 +837,20 @@ > > def searchTasks(self, search_params, *args, **kwargs): > """See `IHasBugs`.""" > - if search_params is None and not args: > + if search_params is None and len(args) == 0: > # this method is called via webapi directly > - args = list() > + args = [] > for key in ('assignee', 'bug_subscriber', 'owner', 'bug_commenter'): > - if kwargs.get(key, None) is None: > - arguments = copy.copy(kwargs) > + # all these parameter default to None > + > + if kwargs.get(key) is None: > + > + arguments = kwargs.copy() > arguments[key] = self > if key == 'owner': > - # Specify both owner and bug_reporter to try to prevent the same > - # bug (but different tasks) being displayed. > + # Specify both owner and bug_reporter to try to > + # prevent the same bug (but different tasks) > + # being displayed. > # see `PersonRelatedBugTaskSearchListingView.searchUnbatched` > arguments['bug_reporter'] = self > args.append(BugTaskSearchParams.fromSearchForm(**arguments)) > Revision 10118: > === modified file 'lib/lp/registry/interfaces/person.py' > --- lib/lp/registry/interfaces/person.py 2010-02-10 07:36:25 +0000 > +++ lib/lp/registry/interfaces/person.py 2010-02-10 10:51:00 +0000 > @@ -25,6 +25,7 @@ > 'ITeamContactAddressForm', > 'ITeamCreation', > 'ITeamReassignment', > + 'IllegalRelatedBugTasksParams', > 'ImmutableVisibilityError', > 'InvalidName', > 'JoinNotAllowed', > @@ -2057,6 +2058,12 @@ > webservice_error(409) > > > +class IllegalRelatedBugTasksParams(Exception): > + """Exception raised when trying to overwirte all relevant parameter s/wirte/write/ s/parameter/parameters/ > + in a search for related bug tasks""" > + webservice_error(400) #Bad request. > + > + This is a nice addition, and adding this one exception is not in itself a big change, but its use is going to need a test in the API and a view or page test for the web UI. This branch is quite large already, it's generally better to land small focused branches, and we can do without this change for now. I suggest you move this out of this branch and keep it for later. That way we can concentrate on getting this branch landed. UPDATE: After talking with intellectronica, we've agreed that it's okay to leave this in. But, please can you add a quick test to demonstrate the error via the API, then manually confirm it's broken via the web UI and file a bug for it? > class NoSuchPerson(NameLookupFailed): > """Raised when we try to look up an IPerson that doesn't exist.""" > > Revision 10119: > === modified file 'lib/lp/registry/model/person.py' > --- lib/lp/registry/model/person.py 2010-02-10 07:36:25 +0000 > +++ lib/lp/registry/model/person.py 2010-02-10 10:52:50 +0000 > @@ -107,7 +107,7 @@ > IPerson, IPersonSet, ITeam, ImmutableVisibilityError, InvalidName, > JoinNotAllowed, NameAlreadyTaken, PersonCreationRationale, > PersonVisibility, PersonalStanding, TeamMembershipRenewalPolicy, > - TeamSubscriptionPolicy) > + TeamSubscriptionPolicy, IllegalRelatedBugTasksParams) > from canonical.launchpad.interfaces.personnotification import ( > IPersonNotificationSet) > from lp.registry.interfaces.pillar import IPillarNameSet > @@ -209,6 +209,47 @@ > raise ImmutableVisibilityError(warning) > > return value > + > +def getRelatedBugTasksParams(user, user_context, **kwargs): Heh, I'm going to be more of a PITA now :) First, module-level functions should follow PEP8 naming conventions, so get_related_bugtasks_params(). Maybe chuck a search in there for good measure: get_related_bugtasks_search_params(). Second, I suggest moving this into lp.bugs.model.bugtask, and importing it from there. Third, I got confused by the name user_context. I think it would be clearer as context_user, or just context (with an `assert IPerson.providedBy(context)` in the function body perhaps). > + """Returns a list of `BugTaskSearchParams` which can be used to > + search for all tasks related to a user given by `user_context`. > + > + Which tasks are related to a user? > + * the user has to be either assignee or owner of this tasks > + OR > + * the user has to be subscriber or commenter to the underlying bug > + OR > + * the user is reporter of the underlying bug, but this condition > + is automatically fulfilled by the first one as each new bug > + always get one task owned by the bug reporter Great docstring :) > + """ > + relevant_fields = ('assignee', 'bug_subscriber', 'owner', 'bug_commenter') > + search_params = [] > + for key in relevant_fields: > + # all these parameter default to None > + user_param = kwargs.get(key) > + if user_param is None or user_param == user_context: > + # we are only creating a `BugTaskSearchParams` object if > + # the field is None or equal to the context > + arguments = kwargs.copy() > + arguments[key] = user_context > + if key == 'owner': > + # Specify both owner and bug_reporter to try to > + # prevent the same bug (but different tasks) > + # being displayed. > + # see `PersonRelatedBugTaskSearchListingView.searchUnbatched` > + arguments['bug_reporter'] = user_context > + search_params.append( > + BugTaskSearchParams.fromSearchForm(user, **arguments)) > + if len(search_params) == 0: > + # unable to search for related tasks to user_context because user > + # modified the query in an invalid way by overwriting all user > + # related parameters > + raise IllegalRelatedBugTasksParams( > + ('Cannot search for related tasks to \'%s\', at least one ' > + 'of these parameter has to be empty: %s' > + %(user_context.name, ", ".join(relevant_fields)))) > + return search_params > > > class Person( > @@ -840,26 +881,18 @@ > """See `IHasBugs`.""" > if search_params is None and len(args) == 0: > # this method is called via webapi directly > - args = [] > - for key in ('assignee', 'bug_subscriber', 'owner', 'bug_commenter'): > - # all these parameter default to None > - > - if kwargs.get(key) is None: > - > - arguments = kwargs.copy() > - arguments[key] = self > - if key == 'owner': > - # Specify both owner and bug_reporter to try to > - # prevent the same bug (but different tasks) > - # being displayed. > - # see `PersonRelatedBugTaskSearchListingView.searchUnbatched` > - arguments['bug_reporter'] = self > - args.append(BugTaskSearchParams.fromSearchForm(**arguments)) > - if args: > - search_params = args.pop(0) > - # all keyword arguments are considered in the > - # search parameters, use the implementation in BugTaskSet > - return getUtility(IBugTaskSet).search(search_params, *args) > + # calling this method on a Person object directly via the > + # webservice API means searching for user related tasks > + user = kwargs.pop('user') > + try: > + search_params = getRelatedBugTasksParams(user, self, **kwargs) > + except IllegalRelatedBugTasksParams, e: > + # dirty hack, marking an exception with a HTTP error > + # only works if the exception is raised in the exported > + # method, see docstring of > + # `lazr.restful.declarations.webservice_error()` > + raise e > + return getUtility(IBugTaskSet).search(*search_params) > if len(kwargs) > 0: > # if keyword arguments are supplied, use the deault > # implementation in HasBugsBase. > Revision 10120: > === modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt' > --- lib/lp/bugs/stories/webservice/xx-bug.txt 2010-02-04 21:18:35 +0000 > +++ lib/lp/bugs/stories/webservice/xx-bug.txt 2010-02-10 12:14:41 +0000 > @@ -1643,6 +1643,26 @@ > >>> for bugtask in bugtasks: > ... print bugtask['self_link'] > http://api.launchpad.dev/beta/ubuntu/+source/linux-source-2.6.15/+bug/10 > + > + > +User related bug tasks > +~~~~~~~~~~~~~~~~~~~~~~ > + > +Calling searchTasks() on a Person object returns a collection of tasks > +related to this person. > + > + >>> json = webservice.named_get( > + ... '/~name12', 'searchTasks' > + ... ).jsonBody() > + >>> related = json['entries'] > + >>> len(related) > 0 > + True > + >>> name12 = webservice.get("/~name12").jsonBody() > + >>> json = webservice.named_get( > + ... '/~name12', 'searchTasks', assignee=name12['self_link'] > + ... ).jsonBody() > + >>> len(json['entries']) < len(related) > + True I think this still needs the pprint_entry() and pprint_collection() stuff, but thanks for moving it. > > > Affected users > > === modified file 'lib/lp/registry/stories/person/xx-person-bugs.txt' > --- lib/lp/registry/stories/person/xx-person-bugs.txt 2010-02-02 11:08:27 +0000 > +++ lib/lp/registry/stories/person/xx-person-bugs.txt 2010-02-10 12:14:41 +0000 > @@ -164,22 +164,3 @@ > >>> anon_browser.open('http://launchpad.dev/~name12/+commentedbugs') > >>> print anon_browser.getLink('List assigned bugs').url > http://bugs.launchpad.dev/~name12/+assignedbugs > - > - > -== It is possible to get related tasks via the webservice API == > - > -Calling searchTasks() on a Person object returns a collection of tasks > -related to this person. > - > - >>> json = webservice.named_get( > - ... '/~name12', 'searchTasks' > - ... ).jsonBody() > - >>> related = json['entries'] > - >>> len(related) > 0 > - True > - >>> name12 = webservice.get("/~name12").jsonBody() > - >>> json = webservice.named_get( > - ... '/~name12', 'searchTasks', assignee=name12['self_link'] > - ... ).jsonBody() > - >>> len(json['entries']) < len(related) > - True > Revision 10121: > === modified file 'lib/lp/registry/tests/test_person.py' > --- lib/lp/registry/tests/test_person.py 2009-12-08 17:53:37 +0000 > +++ lib/lp/registry/tests/test_person.py 2010-02-10 12:49:08 +0000 > @@ -24,11 +24,12 @@ > from lp.registry.interfaces.mailinglist import IMailingListSet > from lp.registry.interfaces.person import ( > IPersonSet, ImmutableVisibilityError, NameAlreadyTaken, > - PersonCreationRationale, PersonVisibility) > + PersonCreationRationale, PersonVisibility, > + IllegalRelatedBugTasksParams) > from canonical.launchpad.database import Bug, BugTask, BugSubscription > from lp.registry.model.structuralsubscription import ( > StructuralSubscription) > -from lp.registry.model.person import Person > +from lp.registry.model.person import Person, getRelatedBugTasksParams > from lp.answers.model.answercontact import AnswerContact > from lp.blueprints.model.specification import Specification > from lp.testing import TestCaseWithFactory > @@ -493,6 +494,29 @@ > InvalidName, self.person_set.createPersonAndEmail, > '