launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/lib/lp/registry/model/announcement.py b/lib/lp/registry/model/announcement.py
> index 45b5740..a2c9ac9 100644
> --- a/lib/lp/registry/model/announcement.py
> +++ b/lib/lp/registry/model/announcement.py
> @@ -151,44 +191,43 @@ class HasAnnouncements:
>  
>      def getAnnouncements(self, limit=5, published_only=True):
>          """See IHasAnnouncements."""
> +        from lp.registry.model.product import Product
>  
>          # Create the SQL query.
> -        query = '1=1 '
> +        using = [Announcement]
> +        query = []
>          # Filter for published news items if necessary.
>          if published_only:
> -            query += """ AND
> -                Announcement.date_announced <= timezone('UTC'::text, now()) AND
> -                Announcement.active IS TRUE
> -                """
> +            query += [
> +                Announcement.date_announced <= UTC_NOW,
> +                Announcement.active == True]
>          if IProduct.providedBy(self):
>              if self.projectgroup is None:
> -                query += """ AND
> -                    Announcement.product = %s""" % sqlvalues(self.id)
> +                query.append(Announcement.product == self.id)
>              else:
> -                query += """ AND
> -                    (Announcement.product = %s OR Announcement.project = %s)
> -                    """ % sqlvalues(self.id, self.projectgroup)
> +                query.append(Or(
> +                    Announcement.product == self.id,
> +                    Announcement.projectgroup == self.projectgroup))
>          elif IProjectGroup.providedBy(self):
> -            query += """ AND (
> -                Announcement.project = %s
> -                OR Announcement.product IN (
> -                    SELECT id FROM Product
> -                    WHERE Product.project = %s AND Product.active))
> -                    """ % sqlvalues(self.id, self.id)
> +            child_products = Select(
> +                Product.id,
> +                And(Product.projectgroup == self, Product.active == True))
> +            query.append(Or(
> +                Announcement.projectgroup == self,
> +                Announcement.product_id.is_in(child_products)))
>          elif IDistribution.providedBy(self):
> -            query += (' AND Announcement.distribution = %s'
> -                % sqlvalues(self.id))
> +            query.append(Announcement.distribution == self)
>          elif IAnnouncementSet.providedBy(self):
>              # Just filter out inactive projects, mostly to exclude spam.
> -            query += """ AND (
> -                Announcement.product IS NULL
> -                OR EXISTS (
> -                    SELECT 1 FROM Product WHERE
> -                    Product.id = Announcement.product AND Product.active))
> -                """
> +            using.append(
> +                LeftJoin(Product, Product.id == Announcement.product_id))
> +            query.append(Or(
> +                Announcement.product == None,
> +                Product.active == True))

I think this transformation is correct here, but make sure it has good tests.

>          else:
>              raise AssertionError('Unsupported announcement target')
> -        return Announcement.select(query, limit=limit)
> +        store = IStore(Announcement)
> +        return store.using(*using).find(Announcement, *query)[:limit]
>  
>  
>  class MakesAnnouncements(HasAnnouncements):


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/395106
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:stormify-announcement.