launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> === modified file 'lib/lp/answers/browser/question.py'
> --- lib/lp/answers/browser/question.py	2018-08-09 15:08:14 +0000
> +++ lib/lp/answers/browser/question.py	2018-11-18 18:00:36 +0000
> @@ -364,27 +361,23 @@
>              # No post, nothing to do
>              return
>  
> -        question_unmodified = Snapshot(
> -            self.context, providing=providedBy(self.context))
>          modified_fields = set()
> -
> -        response = self.request.response
> -        # Establish if a subscription form was posted.
> -        newsub = self.request.form.get('subscribe', None)
> -        if newsub is not None:
> -            if newsub == 'Subscribe':
> -                self.context.subscribe(self.user)
> -                response.addNotification(
> -                    _("You have subscribed to this question."))
> -                modified_fields.add('subscribers')
> -            elif newsub == 'Unsubscribe':
> -                self.context.unsubscribe(self.user, self.user)
> -                response.addNotification(
> -                    _("You have unsubscribed from this question."))
> -                modified_fields.add('subscribers')
> -            response.redirect(canonical_url(self.context))
> -        notify(ObjectModifiedEvent(
> -            self.context, question_unmodified, list(modified_fields)))
> +        with notify_modified(self.context):
> +            response = self.request.response
> +            # Establish if a subscription form was posted.
> +            newsub = self.request.form.get('subscribe', None)
> +            if newsub is not None:
> +                if newsub == 'Subscribe':
> +                    self.context.subscribe(self.user)
> +                    response.addNotification(
> +                        _("You have subscribed to this question."))
> +                    modified_fields.add('subscribers')
> +                elif newsub == 'Unsubscribe':
> +                    self.context.unsubscribe(self.user, self.user)
> +                    response.addNotification(
> +                        _("You have unsubscribed from this question."))
> +                    modified_fields.add('subscribers')
> +                response.redirect(canonical_url(self.context))

Thanks for catching that.  I had indeed intended to pass this to notify_modified, but forgot.

I think this may actually be unused; the test suite doesn't catch the missing fields in the event, and none of the functions subscribed to this particular kind of event appear to care about the case where just the subscribers change.  But to keep this MP simple, I've just added a test that explicitly checks the emitted event and an XXX comment about the possible cleanup.

>  
>      @property
>      def page_title(self):


-- 
https://code.launchpad.net/~cjwatson/launchpad/snapshot-modifying-helper-use-answers/+merge/358962
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.