Merge lp:~brian-murray/launchpad/api-export-bug-linked-branches-take2 into lp:launchpad

[Brian Murray]

Exports IBug.linked_branches as an IBugBranch collection.

[Brian Murray (brian-murray)]

This is a redo of the branch in https://code.edge.launchpad.net/~brian-murray/launchpad/api-export-bug-linked-branches. That was failing ec2 testing and make build due to the schema for linked branches being wrong. I've resolved that in this branch.

This may also resolve bug 528569.

[Michael Nelson (michael.nelson)]

Hi Brian, thanks for getting in there and fixing this.

I was a bit confused at first, as the original MP says it was merged (https://code.edge.launchpad.net/~brian-murray/launchpad/api-export-bug-linked-branches/+merge/17821) but I assume it didn't land.

Now, with this branch, I have one main question and a small request. The question is: Why are we declaring and exporting the the linked_branches CollectionField attribute on IBug, when it is already exported on IHasLinkedBranches (which is inherited by IBug)? I'll ping a bugs person to see if they know.

And regarding the tests, if you look at around line 1793 of xx-bug.txt, you'll see the "And for every bug we can look at the CVEs linked to it..." part of the story which shows how the exported cves_collection_link is used. It would be great to add a similar example showing the linked_branches_collection_link being used (you may need to first link a branch, I couldn't see a bug with any branches linked when I tried).

Thanks!
-Michael

review: Needs Information (code)

[Abel Deuring (adeuring)]

bugs 4 and 5 of the test data have linked branches

[Abel Deuring (adeuring)]

Ah, one more detail:

19 + linked_branches = exported(
20 + CollectionField(
21 + title=_("Branches associated with this bug, usually "
22 + "branches on which this bug is being fixed."),
23 + value_type=Reference(schema=IBranch),
24 + readonly=True))

That should be "schema=IBugBranch", as the implementation lp.bugs.model.Bug.linked_branches returns BugBranch objects.

[Michael Nelson (michael.nelson)]

Hi Brian,

So from the conversation below, I now understand why the original MP says Merged, and why you're needing to re-define linked_branches on IBug (because the declaration on IHasLinkedBranches uses the schema IBranch, whereas Abel mentions below that IBugBranch should be the used as the schema, as it has a few extra fields.)

So, please switch the schema to IBugBranch (I just checked and your current test still passes), and then add the example showing how it can be used (as mentioned above for cves_collection_link in the same test) - this will show the extra fields provided by IBugBranch - and it should be good to land.

Thanks!

10:04 < noodles775> Hi adeuring, would you be able to have a look at the comment I just added to https://code.edge.launchpad.net/~brian-murray/launchpad/api-export-bug-linked-branches-take2/+merge/20592
10:04 < noodles775> adeuring: do you know if there's a reason why linked_branches is declared both in IBug and IHasLinkedBranches (which is inherited by IBug)?
10:05 < adeuring> noodles775: more or less. (1) class IBugBranch defines a few more properties, like revision_hint or bug_task
10:06 < adeuring> erm that was it, no (2)
10:07 < adeuring> noodles775: and the history of Brian's branch is quite complex. It was merged into a another branch, and that branch landed after Brian's changed had been reverted.
10:08 < noodles775> adeuring: sorry, I don't understand what that has to do with IBug.linked_branches and IHasLinkedBranches.linked_branches, when IBug inherits IHasLinkedBranches
10:08 < noodles775> adeuring: oh, ok.
10:08 < adeuring> noodles775: yes, that's quite convoluted ;)
10:09 < noodles775> adeuring: the declaration should only be required on IHasLinkedBranches right? (although the test fails if you remove the one on IBug)? Or what did I miss?
10:09 < adeuring> noodles775: and: Brian should use IBugBranch as the reference type
10:10 < adeuring> noodles775: sorry wrote the last line too early...
10:10 < adeuring> we need the new declaration at least now, for the export, because of schema=IBranch, which is wrong
10:11 < adeuring> it should be schema=IBugBranch, in the export declaration
10:11 < noodles775> Ah, so it's actually meant to override the IHasLinkedBranches declaration?
10:12 < noodles775> (as it uses the wrong schema). OK.
10:12 < adeuring> noodles775: well, I did not write the code ;) But I assume, yes
10:12 < noodles775> adeuring: ok, thanks for the help!
10:16 < adeuring> noodles775: an odd detail of overriding linked_branches is that there is not very much left from IHasLinkedBranches ;) But that's nothing we should nag Brain with ;)
10:17 < noodles775> adeuring: yes, afaics IHasLinkedBranches is used by other objects (blueprints).
10:17 < adeuring> right

review: Needs Fixing (code)

[Brian Murray (brian-murray)]

I've made the recommended changes and pushed them in revision 10429.

[Michael Nelson (michael.nelson)]

Great stuff, thanks Brian.

review: Approve (code)

=== modified file 'lib/lp/bugs/interfaces/bug.py'
--- lib/lp/bugs/interfaces/bug.py	2010-02-27 10:19:18 +0000
+++ lib/lp/bugs/interfaces/bug.py	2010-03-04 18:54:33 +0000
@@ -36,6 +36,7 @@
 from lp.bugs.interfaces.bugtask import (
     BugTaskImportance, BugTaskStatus, IBugTask)
 from lp.bugs.interfaces.bugwatch import IBugWatch
+from lp.bugs.interfaces.bugbranch import IBugBranch
 from lp.bugs.interfaces.cve import ICve
 from canonical.launchpad.interfaces.launchpad import  IPrivacy, NotFoundError
 from canonical.launchpad.interfaces.message import IMessage
@@ -250,9 +251,12 @@
             readonly=True))
     questions = Attribute("List of questions related to this bug.")
     specifications = Attribute("List of related specifications.")
-    linked_branches = Attribute(
-        "Branches associated with this bug, usually "
-        "branches on which this bug is being fixed.")
+    linked_branches = exported(
+        CollectionField(
+            title=_("Branches associated with this bug, usually "
+            "branches on which this bug is being fixed."),
+            value_type=Reference(schema=IBugBranch),
+            readonly=True))
     tags = exported(
         List(title=_("Tags"), description=_("Separated by whitespace."),
              value_type=Tag(), required=False))

=== modified file 'lib/lp/bugs/stories/webservice/xx-bug.txt'
--- lib/lp/bugs/stories/webservice/xx-bug.txt	2010-02-12 09:54:48 +0000
+++ lib/lp/bugs/stories/webservice/xx-bug.txt	2010-03-04 18:54:33 +0000
@@ -32,6 +32,7 @@
   duplicates_collection_link: u'http://.../bugs/11/duplicates'
   heat: 0
   id: 11
+  linked_branches_collection_link: u'http://.../bugs/11/linked_branches'
   messages_collection_link: u'http://.../bugs/11/messages'
   name: None
   number_of_duplicates: 0
@@ -1957,3 +1958,35 @@
     >>> for submission in linked_submissions['entries']:
     ...     print submission['submission_key']
     private-submission
+
+Bug branches
+------------
+
+For every bug we can look at the branches linked to it.
+
+    >>> bug_four = webservice.get("/bugs/4").jsonBody()
+    >>> bug_four_branches_url = bug_four['linked_branches_collection_link']
+    >>> bug_four_branches = webservice.get(bug_four_branches_url).jsonBody()
+    >>> pprint_collection(bug_four_branches)
+    resource_type_link: u'http://.../#bug_branch-page-resource'
+    start: 0
+    total_size: 2
+    ---
+    branch_link: u'http://.../~mark/firefox/release-0.9.2'
+    bug_link: u'http://.../bugs/4'
+    resource_type_link: u'http://.../#bug_branch'
+    self_link: u'http://.../~mark/firefox/release-0.9.2/+bug/4'
+    ---
+    branch_link: u'http://.../~name12/firefox/main'
+    bug_link: u'http://.../bugs/4'
+    resource_type_link: u'http://.../beta/#bug_branch'
+    self_link: u'http://.../~name12/firefox/main/+bug/4'
+    ---
+
+For every branch we can also look at the bugs linked to it.
+
+    >>> branch_entry = bug_four_branches['entries'][0]
+    >>> bug_link = webservice.get(
+    ...     branch_entry['bug_link']).jsonBody()
+    >>> print bug_link['self_link']
+    http://.../bugs/4