Code review comment for lp:~abentley/launchpad/jobstatus

Hi Aaron,

I have a few questions/suggestions below, but it looks good overall.

 review needsfixing

On Thu, 2009-12-17 at 21:46 +0000, Aaron Bentley wrote:
> = Summary =
> Fix bug #336936 "Show something on the merge proposal page while diff
> is being
> generated".
>
> == Proposed fix ==
> Add a notice similar to the one about branch scanning
>
> == Pre-implementation notes ==
> None.
>
> == Implementation details ==
>
> == Tests ==
> bin/test -t test_branchmergeproposals -t xx-branch-merge-proposals
>

> === modified file 'lib/canonical/launchpad/icing/style.css'
> --- lib/canonical/launchpad/icing/style.css 2009-12-08 10:57:44 +0000
> +++ lib/canonical/launchpad/icing/style.css 2009-12-17 21:46:16 +0000
> @@ -1104,7 +1104,7 @@
> height: 2.5em;
> }
>
> -#branch-pending-writes {
> +#branch-pending-writes, #diff-pending-update {

Is this not a good opportunity to rename this class to 'pending-update'
or 'update-in-progress' instead of creating an alias to it?

> background: #FFF59C;
> width: 40em;
> margin: 1em auto;
> @@ -1117,7 +1117,7 @@
> -webkit-border-radius: 5px;
> }
>
> -#branch-pending-writes h3 {
> +#branch-pending-writes h3, #diff-pending-update h3 {
> color: black;
> font-weight: bold;
> text-align: center;
>

> === modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
> --- lib/lp/code/interfaces/branchmergeproposal.py 2009-10-23 00:48:47 +0000
> +++ lib/lp/code/interfaces/branchmergeproposal.py 2009-12-17 21:46:16 +0000
> @@ -155,6 +155,8 @@
> IStaticDiff, title=_('The diff to be used for reviews.'),
> readonly=True)
>
> + next_preview_diff_job = Attribute('foo')
> +

!?

> preview_diff = exported(
> Reference(
> IPreviewDiff,
> @@ -511,6 +513,10 @@
> class IBranchMergeProposalJob(Interface):
> """A Job related to a Branch Merge Proposal."""
>
> + id = Int(
> + title=_('DB ID'), required=True, readonly=True,
> + description=_("Database id for this BranchJob."))

Do you really need to expose this in the public interface or is it just
for your test?

> +
> branch_merge_proposal = Object(
> title=_('The BranchMergeProposal this job is about'),
> schema=IBranchMergeProposal, required=True)
>
> === modified file 'lib/lp/code/model/branchmergeproposal.py'
> --- lib/lp/code/model/branchmergeproposal.py 2009-12-01 01:09:38 +0000
> +++ lib/lp/code/model/branchmergeproposal.py 2009-12-17 21:46:16 +0000
[...]
>
>
> def is_valid_transition(proposal, from_state, next_state, user=None):
> @@ -132,6 +134,26 @@
> review_diff = ForeignKey(
> foreignKey='StaticDiff', notNull=False, default=None)
>
> + @property
> + def next_preview_diff_job(self):
> + from lp.code.model.branchmergeproposaljob import (
> + BranchMergeProposalJob, MergeProposalCreatedJob,
> + UpdatePreviewDiffJob)

Circular dependencies? If so, care to leave a comment here?

> + job_classes = [MergeProposalCreatedJob, UpdatePreviewDiffJob]
> + type_classes = dict(
> + (job_class.class_job_type, job_class)
> + for job_class in job_classes)
> + job = Store.of(self).find(
> + BranchMergeProposalJob,
> + BranchMergeProposalJob.job_type.is_in(type_classes),

I think it'd be better to explicitly pass type_classes.keys() above, to
not leave your reader wondering why a dict is being passed to is_in().

> + BranchMergeProposalJob.job == Job.id,
> + Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING])
> + ).order_by(Job.scheduled_start, Job.date_created).first()
> + if job is not None:
> + return type_classes[job.job_type](job)
> + else:
> + return None
> +
> preview_diff_id = Int(name='merge_diff')
> preview_diff = Reference(preview_diff_id, 'PreviewDiff.id')
>
>
> === modified file 'lib/lp/code/model/branchmergeproposaljob.py'
> --- lib/lp/code/model/branchmergeproposaljob.py 2009-11-03 21:05:14 +0000
> +++ lib/lp/code/model/branchmergeproposaljob.py 2009-12-17 21:46:16 +0000
> @@ -27,6 +27,7 @@
> from storm.store import Store
> from zope.component import getUtility
> from zope.interface import classProvides, implements
> +from zope.security.proxy import removeSecurityProxy
>
> from canonical.database.enumcol import EnumCol
> from canonical.launchpad.database.message import MessageJob, MessageJobAction
> @@ -151,7 +152,9 @@
> self.context = job
>
> def __eq__(self, job):
> - return (self.__class__ is job.__class__ and self.job == job.job)
> + return (
> + self.__class__ is removeSecurityProxy(job.__class__)
> + and self.job == job.job)

Why did you change this?

>
> def __ne__(self, job):
> return not (self == job)
>
> === modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
> --- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-01 01:09:38 +0000
> +++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-17 21:46:16 +0000
> @@ -13,6 +13,7 @@
>
> from pytz import UTC
> from sqlobject import SQLObjectNotFound
> +from storm.locals import Store
> from zope.component import getUtility
> import transaction
> from zope.security.proxy import removeSecurityProxy
> @@ -1828,5 +1829,26 @@
> self.checkExampleMerge(bmp.preview_diff.text)
>
>
> +class TestNextPreviewDiffJob(TestCaseWithFactory):
> +
> + layer = DatabaseFunctionalLayer
> +
> + def test_returns_bmp_job(self):
> + bmp = self.factory.makeBranchMergeProposal()
> + job = bmp.next_preview_diff_job
> + self.assertEqual(bmp, job.branch_merge_proposal)
> + self.assertIs(
> + MergeProposalCreatedJob, removeSecurityProxy(job).__class__)
> + job.start()
> + self.assertEqual(job.id, bmp.next_preview_diff_job.id)

Why do you compare them by ID here?

> + job.fail()
> + self.assertIs(None, bmp.next_preview_diff_job)
> + updatejob = UpdatePreviewDiffJob.create(bmp)
> + Store.of(updatejob.context).flush()
> + self.assertEqual(updatejob, bmp.next_preview_diff_job)
> + updatejob2 = UpdatePreviewDiffJob.create(bmp)
> + self.assertEqual(updatejob, bmp.next_preview_diff_job)
> +
> +
> def test_suite():
> return TestLoader().loadTestsFromName(__name__)

--
Guilherme Salgado <email address hidden>