Code review comment for lp:~adeuring/launchpad/bug-594247-unittests-for-searchtasks-4

Sorry for taking so long to review this. Reading unit tests can be
hard work :-/

A general comment is that the findBugtaskForOtherProduct and its
helper _findBugtaskForOtherProduct started to get a bit convoluted. I
don't have a suggestion for making it better though. I think I
wouldn't have separated it out into two methods; I would have just
overridden the method and called up, but that's a matter of
preference.

It's great to have some clear and accurate definitions of search
behaviour.

+1

[1]

+ # Return the bugtask for the product that not related to the
+ # main bug target.

s/that/that is/

[2]

+ def _findBugtaskForOtherProduct(self, bugtask, main_product):

To summarize this, just to check my understanding:

  Return the first bugtask of the given bugtask's bug that is (a)
  targeted to an IProduct and (b) not targeted to main_product.

[3]

+ # Search results can be limited to bugs with a bug target to which
+ # a given person has a structural subscription.

Oh my, I was not aware of this. This could get complicated with
filters. For now I'm going to ignore it :)

[4]

+ def changeStatusOfBugTaskForOtherProduct(self, bugtask, new_status):
+ # Change the status of another bugtask of the same bug to the
+ # given status.
...
+ bug = bugtask.bug
+ for other_task in bug.bugtasks:
+ other_target = other_task.target
+ if IProduct.providedBy(other_target):
+ with person_logged_in(other_target.owner):
+ other_task.transitionToStatus(
+ new_status, other_target.owner)

This will change the status of bugtask too, which might be fine but is
not what is implied by the method name and comment.

If you only wish to change the status of other tasks, the
related_tasks property could be useful:

    for other_task in bugtask.related_tasks:
        other_target = other_task.target
        if IProduct.providedBy(other_target):
            with person_logged_in(other_target.owner):
                other_task.transitionToStatus(
                    new_status, other_target.owner)

[5]

+ def makeCVE(self, sequence, description=None,
+ cvestate=CveStatus.CANDIDATE):

This probably ought to have a simple test or two. I only just noticed
that there are tests for the factory methods.