Subscription to New or Incomplete bugs is allowing just-filed bugs of all statuses through

[Gavin Panella]

I have the following structural subscription filter for
launchpad-project:

  Bug mail for Gavin Panella about Launchpad Suite is filtered; it
  will be sent only if it matches the following filter:

    “Triage” allows mail through when:
    the status is New or Incomplete

However, mail about bugs just filed with a status other than New or
Incomplete is also being sent.

Perhaps the list of recipients is being calculated before the extra
bug filing options (i.e. status, importance, assignee, etc) are
applied?

[Gary Poster (gary)]

Hi Gavin.

I just tried to dupe your report but could not.

I'm going to mark this incomplete. Could you review these options?

 - Since your report and now, perhaps we've fixed the problem incidentally. Do you still get the unexpected emails?

 - As you know, subscriptions are unioned to determine what notifications you receive. It may well be that you have additional subscriptions to Launchpad that you didn't expect. I think you can check this (even though you are not currently in the group to get all features) if you go to https://bugs.launchpad.net/launchpad/+subscriptions. When I go to that page, I see that I am subscribed via a Launchpad Bug Contacts subscription to *all* changes. I bet you are too. After the next db-devel rollout, you'll be able to opt-out of this team subscription, and then you might get the behavior you desire. Even now, you can also look at your emails to double-check this: "filters" (we don't call them filters in our UI actually but subscriptions) that have names and that trigger an email to be sent will be named in the headers ("X-Launchpad-Subscription: Triage") and in the email body ("Matching subscriptions: Triage"). You should be able to confirm that only the emails matching the filter have the "Triage" name.

 - Perhaps I did not duplicate your problem correctly. I include my replication steps below. Could you review them?

Thank you

Gary

On launchpad.dev:

1) For convenience, verify that you get the new features. Go to https://launchpad.dev/+feature-rules and make sure that these rules, or something like them, are in place. Note that <email address hidden>:test has the necessary privileges to do this, probably among other sampledata users.

malone.advanced-structural-subscriptions.enabled default 1 yes
malone.advanced-subscriptions.enabled default 1 yes

2) Set up a project that has the subscriptions set up as you describe. I set up the firefox project to work as you described, and verified via the +subscriptions page of the bug I was going to modify (https://bugs.launchpad.dev/firefox/+bug/1/+subscriptions) that everything was as I expected. Note that this bug matches the "Triage" filter initially.

3) I ran "cronscripts/send-bug-notifications.py -vv" to clear out pending notifications.

4) I added a comment on bug 1 that I expected to receive as a notification, as a control.

5) I changed the status of bug 1 to be Confirmed. It now no longer matched the filter.

6) I added another comment that I hoped to not receive, because it did not match the filter.

7) I waited 3-5 minutes for the notification code to be willing to send the mail (well, actually, first I ran send-bug-notifications.py, and when I didn't get any emails at all I remembered I had to wait).

8) I ran "cronscripts/send-bug-notifications.py -vv" again and examined the output. foo.bar would have received the first notification, but not the second. The new header and body text I described above about the filter name was there. It looked good.

[Gavin Panella (allenap)]

Thanks for looking into this!

On 25 April 2011 16:22, Gary Poster <email address hidden> wrote:
[...]
> - Since your report and now, perhaps we've fixed the problem
> incidentally. Do you still get the unexpected emails?

Yes. I've got an example later for you.

>
> - As you know, subscriptions are unioned to determine what
> notifications you receive. It may well be that you have additional
> subscriptions to Launchpad that you didn't expect. I think you can
> check this (even though you are not currently in the group to get all
> features) if you go to
> https://bugs.launchpad.net/launchpad/+subscriptions. When I go to that
> page, I see that I am subscribed via a Launchpad Bug Contacts
> subscription to *all* changes. I bet you are too.

That page and launchpad-project/+subscriptions are both empty... which
is a little puzzling because I do have a structural subscription with
a filter on launchpad-project:

  Bug mail for Gavin Panella about Launchpad Suite is filtered; it
  will be sent only if it matches the following filter:

    “Triage” allows mail through when:
    the status is New or Incomplete

I have an example for you, an email I received earlier today:

  Subject: [Bug 771335] [NEW] Title of add subscription widget ...

  If the bug target name is long the title of the subscription
  add/edit form can wrap. Se attached screen shot for example.

  ** Affects: launchpad
      Importance: High
          Status: Triaged

  --
  You received this bug notification because you are subscribed to
  Launchpad Suite.
  Matching subscriptions: Triage
  https://bugs.launchpad.net/bugs/771335

Note the "Matching subscriptions" line and the status. The activity
log for that bug also shows that the status was set at the time the
bug was filed (i.e. in +filebug), not after.

[...]
> On launchpad.dev:
>
> 1) For convenience, verify that you get the new features. Go to
> https://launchpad.dev/+feature-rules and make sure that these rules, or
> something like them, are in place. Note that <email address hidden>:test
> has the necessary privileges to do this, probably among other sampledata
> users.
>
> malone.advanced-structural-subscriptions.enabled default 1 yes
> malone.advanced-subscriptions.enabled default 1 yes
>
> 2) Set up a project that has the subscriptions set up as you describe.
> I set up the firefox project to work as you described, and verified via
> the +subscriptions page of the bug I was going to modify
> (https://bugs.launchpad.dev/firefox/+bug/1/+subscriptions) that
> everything was as I expected. Note that this bug matches the "Triage"
> filter initially.
>
> 3) I ran "cronscripts/send-bug-notifications.py -vv" to clear out
> pending notifications.
>
> 4) I added a comment on bug 1 that I expected to receive as a
> notification, as a control.
>
> 5) I changed the status of bug 1 to be Confirmed. It now no longer
> matched the filter.
>
> 6) I added another comment that I hoped to not receive, because it did
> not match the filter.

The problem manifests with bugs that have their status set at
bug-filing time, in +filebug, rather than with existing bugs. If you
repeat these steps but report...

[Gary Poster (gary)]

Ah-ha! Thanks, Gavin, that should help us get to the bottom of this.

As an aside from the main report, I'm somewhat concerned that you don't see your subscriptions on +subscriptions, but hopefully that's because you are not in the necessary group. If you have a moment tomorrow to test, I have temporarily added you to ~yellow so you have the necessary flags. Could you try https://bugs.launchpad.net/launchpad/+subscriptions and https://bugs.launchpad.net/launchpad-project/+subscriptions and see if you actually see some subscriptions then? If not, we may have another bug. I'd be pretty surprised if you didn't get the Launchpad Bug Contacts at the very least, since I don't see why our accounts would be different in that regard.

[Graham Binns (gmb)]

Whilst trying to reproduce this (I haven't yet and am trying a different tack at the moment), a thought occurred to me. Maybe this bug isn't with the fact that we send emails, maybe it's due to the filter creation overlay not being explicit enough. I've attached a screenshot showing what I think is an implication that you're always going to get told about new bugs.

What I think it should read is something like this:

  (*) are added or changed in any way
    [x] Send mail about comments
    [x] Only tell me about changes to bugs that match this filter:

I think what Gavin's seeing is actually not buggy; it's just that we're not expressing the expected behaviour clearly enough when the user is creating the filter.

Of course, this doesn't mean we shouldn't just change the expected behaviour.

[Gavin Panella (allenap)]

I created my subscription filter in the long dark days before the
overlay. Psh, kids of today, they don't know how easy they have it!

I think the problem lies in FileBugViewBase.submit_bug_action:

        self.added_bug = bug = context.createBug(params)

        # Apply any extra options given by a bug supervisor.
        bugtask = self.added_bug.default_bugtask
        if 'assignee' in data:
            bugtask.transitionToAssignee(data['assignee'])
        if 'status' in data:
            bugtask.transitionToStatus(data['status'], self.user)
        if 'importance' in data:
            bugtask.transitionToImportance(data['importance'], self.user)

The call to context.createBug(...) fires an ObjectCreatedEvent, which
is probably where notifications to subscribers are sent... *before*
the status is updated.

Now, status can be set on the CreateBugParams object that is passed
into createBug(), but not importance for example, so a general fix is
not possible down that road without adding more to CreateBugParams.

A general fix might involve delaying the notification of the
ObjectCreatedEvent within createBug(). createBugWithoutTarget() is an
example of how a similar problem has been solved in the past.

[Graham Binns (gmb)]

On 28 April 2011 09:49, Gavin Panella <email address hidden> wrote:
> I think the problem lies in FileBugViewBase.submit_bug_action:
>
>         self.added_bug = bug = context.createBug(params)
>
>         # Apply any extra options given by a bug supervisor.
>         bugtask = self.added_bug.default_bugtask
>         if 'assignee' in data:
>             bugtask.transitionToAssignee(data['assignee'])
>         if 'status' in data:
>             bugtask.transitionToStatus(data['status'], self.user)
>         if 'importance' in data:
>             bugtask.transitionToImportance(data['importance'], self.user)

This is exactly the same conclusion as I've come to this morning.

> The call to context.createBug(...) fires an ObjectCreatedEvent, which
> is probably where notifications to subscribers are sent... *before*
> the status is updated.

It could be this or it could be the transitionToStatus() call that's
doing the notification. Remember that until the BugTask's status is
set to TRIAGED it actually meets your criteria for the "Triage"
filter. I'm not wholly sure if that means that we'll send you an email
about it when it gets changed from NEW to TRIAGED, but it kind of
makes sense to do that in every situation but this one (because the
bug was in the set of "I generally want to know about this" and you
may need to still know about it after it drops out of that general
set).

> Now, status can be set on the CreateBugParams object that is passed
> into createBug(), but not importance for example, so a general fix is
> not possible down that road without adding more to CreateBugParams.
>
> A general fix might involve delaying the notification of the
> ObjectCreatedEvent within createBug(). createBugWithoutTarget() is an
> example of how a similar problem has been solved in the past.

Right. I'm broadly more in favour of fixing CreateBugParams to accept
more options than futzing around with when notify() happens and which
method's responsible for it. In fact I think that change might be
(relatively) simple, so I'm going to give it a shot now and see what
breaks (and also see if it fixes the problem).

Now, if the notification is actually the one being sent out by
transitionToStatus() then no amount of changing CreateBugParams is
going to help. In that case I don't think there's a simple solution to
this problem (at least not one that I can tackle in what remains of
today) so I'll settle for just changing the wording on the overlay for
the time being to make things a bit clearer.

[Gavin Panella (allenap)]

On 28 April 2011 12:17, Graham Binns <email address hidden> wrote:
> On 28 April 2011 09:49, Gavin Panella <email address hidden> wrote:
[...]
>> The call to context.createBug(...) fires an ObjectCreatedEvent, which
>> is probably where notifications to subscribers are sent... *before*
>> the status is updated.
>
> It could be this or it could be the transitionToStatus() call that's
> doing the notification.

I'm reasonably sure that transitionToStatus() does not notify().

Instead I think a view takes care of firing an ObjectModifiedEvent
(once all changes have been applied), and the web service machinery
does the same for API calls over the wire.

[...]
>> Now, status can be set on the CreateBugParams object that is passed
>> into createBug(), but not importance for example, so a general fix is
>> not possible down that road without adding more to CreateBugParams.
>>
>> A general fix might involve delaying the notification of the
>> ObjectCreatedEvent within createBug(). createBugWithoutTarget() is an
>> example of how a similar problem has been solved in the past.
>
> Right. I'm broadly more in favour of fixing CreateBugParams to accept
> more options than futzing around with when notify() happens and which
> method's responsible for it.

I think it might be a bad idea to bypass transitionToStatus() because
it does a *lot* more than set the bug's status. It's probably true of
all the transitionTo*() methods.

The very end of BugSet.createBug() is:

        # Tell everyone.
        notify(event)

        # Calculate the bug's initial heat.
        bug.updateHeat()

        return bug

The call to updateHeat() could probably come before notify():

        # Calculate the bug's initial heat.
        bug.updateHeat()

        # Tell everyone.
        notify(event)

        return bug

Then it's only a short hop to having two functions:

    def createBug(self, bug_params):
        bug, event = self.createBugAndEvent(bug_params)
        notify(event)
        return bug

    def createBugAndEvent(self, bug_params):
        ...
        return bug, event

FileBugViewBase.submit_bug_action can call createBugAndEvent(), muck
around with the bug's state, then notify().

[Graham Binns (gmb)]

On 28 April 2011 16:12, Gavin Panella <email address hidden> wrote:
>> It could be this or it could be the transitionToStatus() call that's
>> doing the notification.
>
> I'm reasonably sure that transitionToStatus() does not notify().
>
> Instead I think a view takes care of firing an ObjectModifiedEvent
> (once all changes have been applied), and the web service machinery
> does the same for API calls over the wire.

Yeah, transitionToStatus() isn't the source.

>>> Now, status can be set on the CreateBugParams object that is passed
>>> into createBug(), but not importance for example, so a general fix is
>>> not possible down that road without adding more to CreateBugParams.
>>>
>>> A general fix might involve delaying the notification of the
>>> ObjectCreatedEvent within createBug(). createBugWithoutTarget() is an
>>> example of how a similar problem has been solved in the past.
>>
>> Right. I'm broadly more in favour of fixing CreateBugParams to accept
>> more options than futzing around with when notify() happens and which
>> method's responsible for it.
>
> I think it might be a bad idea to bypass transitionToStatus() because
> it does a *lot* more than set the bug's status. It's probably true of
> all the transitionTo*() methods.
>

I thought the same, so I've tried using transitionTo*() in createBug()
and it seems not to have broken anything. I think its existence in the
view is due purely to hysterical raisins and our old "don't do
security checks in the model" rule, which went out when the API
arrived.

[Gavin Panella (allenap)]

On 28 April 2011 19:33, Graham Binns <email address hidden> wrote:
[...]
> I thought the same, so I've tried using transitionTo*() in createBug()
> and it seems not to have broken anything.

D'oh, of course; I always look for the hard way to do things :-/

[Graham Binns (gmb)]

On 28 April 2011 20:14, Gavin Panella <email address hidden> wrote:
> On 28 April 2011 19:33, Graham Binns <email address hidden> wrote:
> [...]
>> I thought the same, so I've tried using transitionTo*() in createBug()
>> and it seems not to have broken anything.
>
> D'oh, of course; I always look for the hard way to do things :-/

Well, a thought did just pop up, which is that we need to check that
the user can actually change the various properties of a task *before*
we create that task, otherwise we end up in the interesting position
of having a Bug with no BugTasks, which is going to cause OOPSes.

Not sure how best to solve that one, other than moving
canTransitionTo*() off of IBugTask... except there are some cases
where it will actually need a BugTask to operate.

Ech, this could be knotty.

[Gavin Panella (allenap)]

> Well, a thought did just pop up, which is that we need to check that
> the user can actually change the various properties of a task *before*
> we create that task, otherwise we end up in the interesting position
> of having a Bug with no BugTasks, which is going to cause OOPSes.

An exception is probably (almost certainly) raised and the transaction
aborted.

[Graham Binns (gmb)]

On Thursday, 28 April 2011, Gavin Panella <email address hidden> wrote:
>> Well, a thought did just pop up, which is that we need to check that
>> the user can actually change the various properties of a task *before*
>> we create that task, otherwise we end up in the interesting position
>> of having a Bug with no BugTasks, which is going to cause OOPSes.
>
> An exception is probably (almost certainly) raised and the transaction
> aborted.

Yeah, Gary pointed out this surprisingly-hard-to-spot-at-night fact, too :)

--
Graham Binns | PGP Key: EC66FA7D
http://launchpad.net/~gmb

[Brad Crittenden (bac)]

After review the attached branch was submitted to ec2 and had the failures shown in the attachment. I don't have time to follow up so finishing and landing this bug will have to fall back to Graham or someone else.

[Launchpad QA Bot (lpqabot)]

Fixed in stable r12967 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/12967>.

[Robert Collins (lifeless)]

Deployable: unflagged workflow succeeds.