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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Guilherme Salgado wrote:

>> === 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?

Okay, I can do that.

>> 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')
>> +
>
> !?

Circular dependencies. I'll make the attribute somewhat better.

>> 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?

I think it makes a lot of sense to expose in the public interface, and I
have another branch in which it is necessary to expose it in the public
interface:

https://code.edge.launchpad.net/~abentley/launchpad/ampoulejob/+merge/15982

In this case, once I change the test to use an equality, I don't need it.

>
>> +
>> 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?

Yes.

> If so, care to leave a comment here?

Alright.

>
>> + 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().

Okay.

>> === 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?

So that it would work when comparing a securityproxied object with an
unproxied object.

>
>>
>> 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?

Hysterical raisins.

Updates made, diff attached.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAksr448ACgkQ0F+nu1YWqI3B9ACffOi6cTu5+nE6XT0Bq6IosAGI
4FcAn03I7oSjPlKQagLDL/sOzu9+yAWE
=Cf/V
-----END PGP SIGNATURE-----