launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/blueprints/configure.zcml'
> --- lib/lp/blueprints/configure.zcml	2014-11-23 21:37:40 +0000
> +++ lib/lp/blueprints/configure.zcml	2015-03-02 00:02:56 +0000
> @@ -87,6 +87,14 @@
>          interface="lp.blueprints.interfaces.specificationworkitem.ISpecificationWorkItem"/>
>    </class>
>  
> +  <!-- SpecificationWorkItemSet -->
> +
> +  <securedutility
> +      class="lp.blueprints.model.specificationworkitem.SpecificationWorkItemSet"
> +      provides="lp.blueprints.interfaces.specificationworkitem.ISpecificationWorkItemSet">
> +    <allow interface="lp.blueprints.interfaces.specificationworkitem.ISpecificationWorkItemSet"/>
> +  </securedutility>
> +
>    <!-- SpecificationDependency -->
>  
>    <class class="lp.blueprints.model.specificationdependency.SpecificationDependency">
> 
> === modified file 'lib/lp/blueprints/interfaces/specificationworkitem.py'
> --- lib/lp/blueprints/interfaces/specificationworkitem.py	2012-04-05 13:05:04 +0000
> +++ lib/lp/blueprints/interfaces/specificationworkitem.py	2015-03-02 00:02:56 +0000
> @@ -7,6 +7,7 @@
>  
>  __all__ = [
>      'ISpecificationWorkItem',
> +    'ISpecificationWorkItemSet',
>      ]
>  
>  
> @@ -80,3 +81,10 @@
>          description=_(
>              "True or False depending on whether or not there is more "
>              "work required on this work item."))
> +
> +
> +class ISpecificationWorkItemSet(Interface):
> +    """SpecificationWorkItem's public attributes and methods."""

I think not.

> +
> +    def unlinkMilestone(milestone):
> +        """Unlink the given milestone from all SpecificationWorkItems."""
> 
> === modified file 'lib/lp/blueprints/model/specificationworkitem.py'
> --- lib/lp/blueprints/model/specificationworkitem.py	2012-11-16 20:13:32 +0000
> +++ lib/lp/blueprints/model/specificationworkitem.py	2015-03-02 00:02:56 +0000
> @@ -12,11 +12,13 @@
>      Reference,
>      Unicode,
>      )
> +from storm.store import Store
>  from zope.interface import implements
>  
>  from lp.blueprints.enums import SpecificationWorkItemStatus
>  from lp.blueprints.interfaces.specificationworkitem import (
>      ISpecificationWorkItem,
> +    ISpecificationWorkItemSet,
>      )
>  from lp.registry.interfaces.person import validate_public_person
>  from lp.services.database.constants import DEFAULT
> @@ -65,3 +67,12 @@
>      def is_complete(self):
>          """See `ISpecificationWorkItem`."""
>          return self.status == SpecificationWorkItemStatus.DONE
> +
> +
> +class SpecificationWorkItemSet:
> +    implements(ISpecificationWorkItemSet)
> +
> +    def unlinkMilestone(self, milestone):

"""See `ISpecificationWorkItemSet`."""

> +        Store.of(milestone).find(
> +            SpecificationWorkItem, milestone_id=milestone.id).set(
> +                milestone_id=None)
> 
> === modified file 'lib/lp/blueprints/model/tests/test_specification.py'
> --- lib/lp/blueprints/model/tests/test_specification.py	2014-06-19 10:04:55 +0000
> +++ lib/lp/blueprints/model/tests/test_specification.py	2015-03-02 00:02:56 +0000
> @@ -24,6 +24,7 @@
>  from lp.app.validators import LaunchpadValidationError
>  from lp.blueprints.interfaces.specification import ISpecification
>  from lp.blueprints.interfaces.specificationworkitem import (
> +    ISpecificationWorkItemSet,
>      SpecificationWorkItemStatus,
>      )
>  from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
> @@ -659,6 +660,24 @@
>              title=wi.title, status=wi.status, assignee=wi.assignee,
>              milestone=wi.milestone, sequence=sequence)
>  
> +    def test_workitemspecificationset_can_unlink_milestones(self):
> +        milestone_a = self.factory.makeMilestone()
> +        milestone_b = self.factory.makeMilestone()
> +        work_item_1 = self.factory.makeSpecificationWorkItem(milestone=milestone_a)
> +        work_item_2 = self.factory.makeSpecificationWorkItem(milestone=milestone_a)
> +        work_item_3 = self.factory.makeSpecificationWorkItem(milestone=milestone_b)
> +
> +
> +        self.assertEqual(work_item_1.milestone, milestone_a)
> +        self.assertEqual(work_item_2.milestone, milestone_a)
> +        self.assertEqual(work_item_3.milestone, milestone_b)
> +
> +        getUtility(ISpecificationWorkItemSet).unlinkMilestone(milestone_a)
> +
> +        self.assertIs(work_item_1.milestone, None)
> +        self.assertIs(work_item_2.milestone, None)
> +        self.assertEqual(work_item_3.milestone, milestone_b)

Can you flip the assertion arguments?

"""
Help on method assertEqual in module testtools.testcase:

assertEqual(self, expected, observed, message='') unbound testtools.testcase.TestCase method
"""

> +
>  
>  class TestSpecificationInformationType(TestCaseWithFactory):
>  
> 
> === modified file 'lib/lp/registry/browser/__init__.py'
> --- lib/lp/registry/browser/__init__.py	2014-01-30 15:04:06 +0000
> +++ lib/lp/registry/browser/__init__.py	2015-03-02 00:02:56 +0000
> @@ -30,7 +30,7 @@
>      LaunchpadEditFormView,
>      )
>  from lp.app.interfaces.launchpad import ILaunchpadCelebrities
> -from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
> +from lp.blueprints.interfaces.specificationworkitem import ISpecificationWorkItemSet
>  from lp.bugs.interfaces.bugtask import IBugTaskSet
>  from lp.bugs.interfaces.bugtasksearch import BugTaskSearchParams
>  from lp.registry.interfaces.productseries import IProductSeries
> @@ -241,9 +241,7 @@
>              else:
>                  nb.milestone = None
>          removeSecurityProxy(milestone.all_specifications).set(milestoneID=None)
> -        Store.of(milestone).find(
> -            SpecificationWorkItem, milestone_id=milestone.id).set(
> -                milestone_id=None)
> +        getUtility(ISpecificationWorkItemSet).unlinkMilestone(milestone)
>          self._deleteRelease(milestone.product_release)
>          milestone.destroySelf()
>  
> 


-- 
https://code.launchpad.net/~thomir/launchpad/devel-fix-import-violations-specificationworkitem/+merge/251395
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.