launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/default-private-bug-damnit into lp:launchpad.

Requested reviews:
  William Grant (wgrant)
Related bugs:
  Bug #1034089 in Launchpad itself: "Bugs reported via the API are always public"
  https://bugs.launchpad.net/launchpad/+bug/1034089

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/default-private-bug-damnit/+merge/118644

A recent attempt to refactor and normalise the bug creation rules causes
all bugs created of the API to be public if no private or security bools
are passed. The bug creation rules do not respect the default private
bug creation rules of the project.

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * Locate the existing test that shows bugs created via the API honour
      the default private bugs rules
      * Damn it. Write one (again!)
    * BugSet.createBug() is getting InformationType.PUBLIC even when
      InformationType or the old private/security bools are not passed.
    * lp.systemhomes.MaloneApplication.createBug() adapts the private and
      security bools to InformationType. The method sets the default values
      for private/security to False/False; manufacturing user submitted
      data for BugSet.createBug().
      * Only adapt private/security if the user submits the information.
        BugSet.createBug() knows how to handle information_type == None
        and will apply the projects default rules.


QA

    * Run this function against qastaging:
        def test_default_private():
            lp = Launchpad.login_with(
                'testing', service_root='https://api.qastaging.launchpad.net',
                version='devel')
            project = lp.projects['sutton']
            lp.bugs.createBug(
                target=project,
                title='testing bug creation via API',
                description="testing that the bug is private by default.")
    * Verify the bug is private.
    * Run this function against qastaging:
        def test_force_public():
            lp = Launchpad.login_with(
                'testing', service_root='https://api.qastaging.launchpad.net',
                version='devel')
            project = lp.projects['sutton']
            lp.bugs.createBug(
                target=project,
                title='testing bug creation via API',
                description="testing that the bug is force to public."
                public=True)
    * Verify the bug is public.


LINT

    lib/lp/systemhomes.py
    lib/lp/bugs/browser/tests/test_bugs.py
    lib/lp/bugs/interfaces/malone.py
    lib/lp/bugs/tests/test_bugs_webservice.py


TEST

    ./bin/test -vvc -t TestMaloneView lp.bugs.browser.tests.test_bugs
    ./bin/test -vvc -t BugSetTestCase lp.bugs.tests.test_bugs_webservice


IMPLEMENTATION

Updated MaloneApplication.createBug() to only adapt the private and security
kwargs if they are provided. Let BugSet.createBug() the bug using the
project's rules for default bug creation.
    lib/lp/systemhomes.py
    lib/lp/bugs/browser/tests/test_bugs.py

Update the exported createBug() to permit default None so that Lp does not
tamper with the data the user submits. Added an integration test to ensure
This rule does not break again.
    lib/lp/bugs/interfaces/malone.py
    lib/lp/bugs/tests/test_bugs_webservice.py
-- 
https://code.launchpad.net/~sinzui/launchpad/default-private-bug-damnit/+merge/118644
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/bugs/browser/tests/test_bugs.py'
--- lib/lp/bugs/browser/tests/test_bugs.py	2012-08-06 02:54:11 +0000
+++ lib/lp/bugs/browser/tests/test_bugs.py	2012-08-07 22:38:21 +0000
@@ -13,6 +13,7 @@
 from lp.bugs.interfaces.malone import IMaloneApplication
 from lp.bugs.publisher import BugsLayer
 from lp.registry.enums import InformationType
+from lp.registry.interfaces.product import License
 from lp.services.webapp.publisher import canonical_url
 from lp.testing import (
     celebrity_logged_in,
@@ -153,3 +154,43 @@
         # The getBugData method works as expected if related bug is specified.
         related_bug = self.factory.makeBug()
         self._assert_getBugData(related_bug)
+
+    def test_createBug_default_private_bugs_true(self):
+        # createBug() does not adapt the default kwargs when they are none.
+        project = self.factory.makeProduct(
+            licenses=[License.OTHER_PROPRIETARY])
+        with person_logged_in(project.owner):
+            project.setPrivateBugs(True, project.owner)
+            bug = self.application.createBug(
+                project.owner, 'title', 'description', project)
+            self.assertEqual(InformationType.USERDATA, bug.information_type)
+
+    def test_createBug_public_bug_private_bugs_true(self):
+        # createBug() adapts a kwarg to InformationType if one is is not None.
+        project = self.factory.makeProduct(
+            licenses=[License.OTHER_PROPRIETARY])
+        with person_logged_in(project.owner):
+            project.setPrivateBugs(True, project.owner)
+            bug = self.application.createBug(
+                project.owner, 'title', 'description', project, private=False)
+            self.assertEqual(InformationType.PUBLIC, bug.information_type)
+
+    def test_createBug_default_private_bugs_false(self):
+        # createBug() does not adapt the default kwargs when they are none.
+        project = self.factory.makeProduct(
+            licenses=[License.OTHER_PROPRIETARY])
+        with person_logged_in(project.owner):
+            project.setPrivateBugs(False, project.owner)
+            bug = self.application.createBug(
+                project.owner, 'title', 'description', project)
+            self.assertEqual(InformationType.PUBLIC, bug.information_type)
+
+    def test_createBug_private_bug_private_bugs_false(self):
+        # createBug() adapts a kwarg to InformationType if one is is not None.
+        project = self.factory.makeProduct(
+            licenses=[License.OTHER_PROPRIETARY])
+        with person_logged_in(project.owner):
+            project.setPrivateBugs(False, project.owner)
+            bug = self.application.createBug(
+                project.owner, 'title', 'description', project, private=True)
+            self.assertEqual(InformationType.USERDATA, bug.information_type)

=== modified file 'lib/lp/bugs/interfaces/malone.py'
--- lib/lp/bugs/interfaces/malone.py	2012-08-06 02:54:11 +0000
+++ lib/lp/bugs/interfaces/malone.py	2012-08-07 22:38:21 +0000
@@ -74,11 +74,13 @@
         target=Reference(
             schema=IBugTarget, required=True,
             title=u"The project, distribution or source package that has "
-                   "this bug."))
+                   "this bug."),
+        security_related=copy_field(IBug['security_related'], default=None),
+        private=copy_field(IBug['private'], default=None))
     @export_factory_operation(
-        IBug, ['title', 'description', 'tags', 'security_related', 'private'])
-    def createBug(owner, title, description, target, security_related=False,
-                  private=False, tags=None):
+        IBug, ['title', 'description', 'tags'])
+    def createBug(owner, title, description, target, security_related=None,
+                  private=None, tags=None):
         """Create a bug (with an appropriate bugtask) and return it.
 
         :param target: The Product, Distribution or DistributionSourcePackage

=== modified file 'lib/lp/bugs/tests/test_bugs_webservice.py'
--- lib/lp/bugs/tests/test_bugs_webservice.py	2012-07-07 15:26:29 +0000
+++ lib/lp/bugs/tests/test_bugs_webservice.py	2012-08-07 22:38:21 +0000
@@ -24,6 +24,7 @@
 from lp.bugs.browser.bugtask import get_comments_for_bugtask
 from lp.bugs.interfaces.bug import IBug
 from lp.registry.enums import InformationType
+from lp.registry.interfaces.product import License
 from lp.services.webapp import snapshot
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.testing import (
@@ -32,6 +33,7 @@
     login,
     login_person,
     logout,
+    person_logged_in,
     TestCaseWithFactory,
     )
 from lp.testing._webservice import QueryCollector
@@ -363,3 +365,37 @@
         lp_bug = launchpad.load(api_url(bug))
         self.assertRaises(
             BadRequest, lp_bug.addTask, target=api_url(product))
+
+
+class BugSetTestCase(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_default_private_bugs_true(self):
+        # Verify the path through user submission, to MaloneApplication to
+        # BugSet, and back to the user creates a private bug according
+        # to the project's bugs are private by default rule.
+        project = self.factory.makeProduct(
+            licenses=[License.OTHER_PROPRIETARY])
+        with person_logged_in(project.owner):
+            project.setPrivateBugs(True, project.owner)
+        webservice = launchpadlib_for('test', 'salgado')
+        bugs_collection = webservice.load('/bugs')
+        bug = bugs_collection.createBug(
+            target=api_url(project), title='title', description='desc')
+        self.assertEqual('Private', bug.information_type)
+
+    def test_explicit_private_private_bugs_true(self):
+        # Verify the path through user submission, to MaloneApplication to
+        # BugSet, and back to the user creates a private bug beause the
+        # user commands it.
+        project = self.factory.makeProduct(
+            licenses=[License.OTHER_PROPRIETARY])
+        with person_logged_in(project.owner):
+            project.setPrivateBugs(True, project.owner)
+        webservice = launchpadlib_for('test', 'salgado')
+        bugs_collection = webservice.load('/bugs')
+        bug = bugs_collection.createBug(
+            target=api_url(project), title='title', description='desc',
+            private=True)
+        self.assertEqual('Private', bug.information_type)

=== modified file 'lib/lp/systemhomes.py'
--- lib/lp/systemhomes.py	2012-08-07 08:18:39 +0000
+++ lib/lp/systemhomes.py	2012-08-07 22:38:21 +0000
@@ -155,10 +155,19 @@
         return data
 
     def createBug(self, owner, title, description, target,
-                  security_related=False, private=False, tags=None):
+                  security_related=None, private=None, tags=None):
         """See `IMaloneApplication`."""
-        information_type = convert_to_information_type(
-            private, security_related)
+
+#        if security_related is False and private is False:
+#            # XXX sinzui 2012-08-07: Liar! Fuck lazr.restful and its evil
+#            # cache of default values
+#            information_type = None
+        if security_related is None and private is None:
+            # Nothing to adapt, let BugSet.createBug() choose the default.
+            information_type = None
+        else:
+            information_type = convert_to_information_type(
+                private, security_related)
         params = CreateBugParams(
             title=title, comment=description, owner=owner,
             information_type=information_type, tags=tags)