launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

У чет, 12. 05 2011. у 11:56 +0000, Stuart Bishop пише:
> So we have a UI problem, and the desire to restore subscriptions to the
> previous value when emails for a bug are reenabled.

It's not that simple.  "Muting" is orthogonal to any subscriptions you
might have and should affect a single bug.  In the bug report, Gary did
propose an alternative which is storing an actual muted subscription and
then restoring it when "unmuting", but the yellow team didn't like that
approach much (and it would still mean another new table to store muted
bug subscriptions; it was also only considered as the simpler of the
approaches for implementation, not as the preferred one).

> Is this feature genuinely interesting enough to add the new table,
> increasing the complexity of how to determine who gets what email?

Yes: it has been discussed and priority raised—we've basically inherited
the previous implementation from the Bugs team work in 2010, and didn't
want to change it unless explicitely asked to.  Since we are now
explicitely asked to, that's what we are doing.

Also, it was raised as a big problem with the "muting" concept from user
perspective. 

> The DB patch itself seems generally fine
> 
>  - We will want a who-did-it column to track who muted a team subscription if that is possible.

That should not be possible.  Perhaps I should assert that in the code.

>  - You are deleting a load of BugSubscription records.

Are we?  I was expecting only a few of them to have level set to NOTHING
(10) on production.  Am I mistaken?

> This means the
> code will need to cope with unmuting a bug with no existing
> BugSubscription, as well as restoring an existing BugSubscription.

That's transparently handled in the backend code (simply removing the
BugMute record restores the existing BugSubscription).  And yes, that's
what UI code will have to do anyway if we want this feature, so we are
aware of that.

> Would it not be better to set the affected BugSubscription notification
> level to something meaningful and avoid the extra code path and tests?

That's what the code does today (i.e. uses the "NOTHING" level).  Since
"muting" should work regardless of the way you are subscribed to a bug,
that's actually more complex and when it works, it does so "by
accident".  For instance, it doesn't work when you are a member of the
team subscribed to the bug.

It also represents exactly the same model as BugSubscriptionFilterMute,
which should make it easier to understand the code.

Also, this makes the logic explicit for muting, and actually *simpler*.
Thus, it's much easier to ensure and prove it is correct.  When
something is in practice orthogonal to the problem, it's best to
implement it like that as well.


-- 
https://code.launchpad.net/~danilo/launchpad/proper-bug-muting/+merge/60615
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/proper-bug-muting into lp:launchpad/db-devel.