Merge lp:~abentley/launchpad/jobstatus into lp:launchpad

= 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

== Demo and Q/A ==
Create a branch merge proposal. It should show a notice saying the diff is
being updated. After a few minutes, the diff should appear and the message
should go away. Now push new changes to the source branch. The message should
reappear.

= Launchpad lint =

There is lint here, but not of it seems very rational.

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/icing/style.css
  lib/lp/code/configure.zcml
  lib/lp/code/browser/branchmergeproposal.py
  lib/lp/code/interfaces/branchmergeproposal.py
  lib/lp/code/model/branchmergeproposal.py
  lib/lp/code/model/branchmergeproposaljob.py
  lib/lp/code/model/tests/test_branchmergeproposals.py
  lib/lp/code/stories/branches/xx-branch-merge-proposals.txt
  lib/lp/code/templates/branchmergeproposal-index.pt

== Pylint notices ==

lib/lp/code/interfaces/branchmergeproposal.py
    27: [F0401] Unable to import 'lazr.lifecycle.event' (No module named lifecycle)
    42: [F0401] Unable to import 'lazr.restful.fields' (No module named restful)
    43: [F0401] Unable to import 'lazr.restful.declarations' (No module named restful)
    309: [C0322, IBranchMergeProposal.setStatus] Operator not preceded by a space
    title=_("The new status of the merge proposal."),
    ^
    vocabulary=BranchMergeProposalStatus),
    revision_id=Text(
    description=_("An optional parameter for specifying the "
    "revision of the branch for the status change."),
    required=False))
    @export_write_operation()
    def setStatus(status, user, revision_id):
    459: [C0322, IBranchMergeProposal.createComment] Operator not preceded by a space
    parent=Reference(schema=Interface))
    ^
    @call_with(owner=REQUEST_USER)

    @export_factory_operation(Interface, [])
    def createComment(owner, subject, content=None, vote=None,
    review_type=None, parent=None):
    490: [C0322, IBranchMergeProposal.updatePreviewDiff] Operator not preceded by a space
    target_revision_id=TextLine(), prerequisite_revision_id=TextLine(),
    ^
    conflicts=Text())
    @export_write_operation()
    def updatePreviewDiff(diff_content, source_revision_id,
    target_revision_id, prerequisite_revision_id=None,
    conflicts=None):

lib/lp/code/model/branchmergeproposaljob.py
    20: [F0401] Unable to import 'lazr.delegates' (No module named delegates)
    21: [F0401] Unable to import 'lazr.enum' (No module named enum)

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

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