Code review comment for lp:~intellectronica/launchpad/heat-decay-complete

Hi Tom,

This is a nice branch. I have a few comments below, and there are some
lint errors.

merge-conditional

-Edwin

== Pyflakes notices ==

lib/lp/bugs/scripts/bugheat.py
    13: 'getUtility' imported but unused
    14: 'implements' imported but unused
    16: 'ITunableLoop' imported but unused
    17: 'DBLoopTuner' imported but unused

lib/lp/bugs/scripts/tests/test_bugheat.py
    10: 'datetime' imported but unused

>=== modified file 'lib/lp/bugs/scripts/bugheat.py'
>--- lib/lp/bugs/scripts/bugheat.py 2010-01-26 20:31:13 +0000
>+++ lib/lp/bugs/scripts/bugheat.py 2010-02-26 21:30:36 +0000
>@@ -8,6 +8,7 @@
> 'BugHeatCalculator',
> ]
>
>+from datetime import datetime
>
> from zope.component import getUtility
> from zope.interface import implements
>@@ -15,6 +16,8 @@
> from canonical.launchpad.interfaces.looptuner import ITunableLoop
> from canonical.launchpad.utilities.looptuner import DBLoopTuner
>
>+from lp.bugs.interfaces.bugtask import RESOLVED_BUGTASK_STATUSES
>+
>
> class BugHeatConstants:
>
>@@ -63,8 +66,16 @@
> len(direct_subscribers) + len(subscribers_from_dupes))
> return subscriber_count * BugHeatConstants.SUBSCRIBER
>
>+ def _bugIsComplete(self):
>+ """Are all the tasks for this bug resolved?"""
>+ return all([(task.status in RESOLVED_BUGTASK_STATUSES)
>+ for task in self.bug.bugtasks])
>+
> def getBugHeat(self):
> """Return the total heat for the current bug."""
>+ if self._bugIsComplete():
>+ return 0
>+
> total_heat = sum([
> self._getHeatFromAffectedUsers(),
> self._getHeatFromDuplicates(),
>@@ -73,5 +84,15 @@
> self._getHeatFromSubscribers(),
> ])
>
>+ # Bugs decay over time. Every month the bug isn't touched its heat
>+ # decreeses by 5%.

s/decreeses/decreases/

The comment says 5%, but the test says 10%. Which is it?

>+ months = (
>+ datetime.utcnow() -
>+ self.bug.date_last_updated.replace(tzinfo=None)).days / 30
>+ for i in range(months):
>+ total_heat = total_heat * 0.95

This could be simplified as:
    total_heat *= 0.95 ** months

>+
>+ total_heat = int(total_heat)
>+
> return total_heat
>
>
>=== modified file 'lib/lp/bugs/scripts/tests/test_bugheat.py'
>--- lib/lp/bugs/scripts/tests/test_bugheat.py 2010-01-12 16:41:23 +0000
>+++ lib/lp/bugs/scripts/tests/test_bugheat.py 2010-02-26 21:30:36 +0000
>@@ -1,4 +1,3 @@
>-
> # Copyright 2010 Canonical Ltd. This software is licensed under the
> # GNU Affero General Public License version 3 (see the file LICENSE).
>
>@@ -8,12 +7,14 @@
>
> import unittest
>
>+from datetime import datetime, timedelta
>+
> from canonical.testing import LaunchpadZopelessLayer
>
>+from lp.bugs.interfaces.bugtask import BugTaskStatus
> from lp.bugs.scripts.bugheat import BugHeatCalculator, BugHeatConstants
> from lp.testing import TestCaseWithFactory
>
>-
> class TestBugHeatCalculator(TestCaseWithFactory):
> """Tests for the BugHeatCalculator class."""
>
>@@ -177,6 +178,35 @@
> "Expected bug heat did not match actual bug heat. "
> "Expected %s, got %s" % (expected_heat, actual_heat))
>
>+ def test_getBugHeat_complete_bugs(self):
>+ # Bug which are in a resolved status don't have heat at all.
>+ complete_bug = self.factory.makeBug()
>+ heat = BugHeatCalculator(complete_bug).getBugHeat()
>+ self.assertNotEqual(
>+ 0, heat,
>+ "Expected bug heat did not match actual bug heat. "
>+ "Expected a positive value, got 0")
>+ complete_bug.bugtasks[0].transitionToStatus(
>+ BugTaskStatus.INVALID, complete_bug.owner)
>+ heat = BugHeatCalculator(complete_bug).getBugHeat()
>+ self.assertEqual(
>+ 0, heat,
>+ "Expected bug heat did not match actual bug heat. "
>+ "Expected %s, got %s" % (0, heat))
>+
>+ def test_getBugHeat_decay(self):
>+ # Every month, a bug that wasn't touched has its heat reduced by 10%.

10% or 5% ?

>+ aging_bug = self.factory.makeBug()
>+ aging_bug.date_last_updated = (
>+ aging_bug.date_last_updated - timedelta(days=32))
>+ expected = int((
>+ BugHeatConstants.AFFECTED_USER + BugHeatConstants.SUBSCRIBER) * 0.9)
>+ heat = BugHeatCalculator(aging_bug).getBugHeat()

This test might not notice corner cases. Can you run getBugHeat()
before and after setting the date_last_updated and assert that
old_heat * 0.9 == new_heat

>+ self.assertEqual(
>+ expected, heat,
>+ "Expected bug heat did not match actual bug heat. "
>+ "Expected %s, got %s" % (expected, heat))
>+
>
> def test_suite():
> return unittest.TestLoader().loadTestsFromName(__name__)
>