Code review comment for lp:~adeuring/launchpad/bug-283941-show-patch-numbers-on-upstream-report

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

Hi Brad,

thanks for your review!

On 16.02.2010 17:34, Brad Crittenden wrote:
> Review: Approve code
> Hi Abel,
>
> Thanks for this branch. It looks good to me except for a couple of
> questions. Please do have a UI review too.

Sure, I wont forget the UI review ;)

>
> --Brad
>
>

[...]
>> === modified file 'lib/lp/bugs/doc/distribution-upstream-bug-report.txt'
>> --- lib/lp/bugs/doc/distribution-upstream-bug-report.txt 2009-10-16 20:33:56 +0000
>> +++ lib/lp/bugs/doc/distribution-upstream-bug-report.txt 2010-02-16 16:16:26 +0000
>> @@ -131,20 +131,56 @@
>> >>> task = getUtility(IBugTaskSet).createTask(bug, name12, product=pmount)
>> >>> Store.of(task).flush()
>> >>> print_report(ubuntu.getPackagesAndPublicUpstreamBugCounts(limit=3))
>> - pmount pmount 2 2 1 0
>> - linux-source-2.6.15 None 1 0 0 0
>> - mozilla-firefox firefox 1 1 1 1
>> -
>> -Linking that task to a bugwatch makes the watch counts update:
>> + pmount pmount 2 2 1 0 0
>> + linux-source-2.6.15 None 1 0 0 0 0
>> + mozilla-firefox firefox 1 1 1 1 0
>> +
>> +The last column counts those bugs with upstream tasks that have patches
>> +attached but which don't have an upstream bugwatch. If we add a ordinary
>> +attachment to our pmount bug, the value of the last column does not
>> +change...
>> +
>> + >>> attachment = factory.makeBugAttachment(bug)
>> + >>> print_report(ubuntu.getPackagesAndPublicUpstreamBugCounts(limit=3))
>> + pmount pmount 2 2 1 0 0
>> + linux-source-2.6.15 None 1 0 0 0 0
>> + mozilla-firefox firefox 1 1 1 1 0
>> +
>> +...but when we make this attachment a patch, the value of the column
>> +increases.
>> +
>> + >>> from lp.bugs.interfaces.bugattachment import BugAttachmentType
>> + >>> attachment.type = BugAttachmentType.PATCH
>> + >>> Store.of(attachment).flush()
>> + >>> print_report(ubuntu.getPackagesAndPublicUpstreamBugCounts(limit=3))
>> + pmount pmount 2 2 1 0 1
>> + linux-source-2.6.15 None 1 0 0 0 0
>> + mozilla-firefox firefox 1 1 1 1 0
>> +
>> +Note that we count only bugs with patches for products that do not
>> +use Malone officially.
>> +
>> + >>> pmount.official_malone = True
>> + >>> Store.of(pmount).flush()
>> + >>> print_report(ubuntu.getPackagesAndPublicUpstreamBugCounts(limit=3))
>> + pmount pmount 2 2 1 1 0
>> + linux-source-2.6.15 None 1 0 0 0 0
>> + mozilla-firefox firefox 1 1 1 1 0
>
> I find having a '0' to be misleading. Would not using a '-' or some
> other indicator to show the data are not relevant be more appropriate?

Thanks, this is a nice idea -- but I prefer to implement this special
case in the template. Firstly, this keeps the code of the method
getPackagesAndPublicUpstreamBugCounts() (or the SQL query) a bit
simpler, and secondly, we calculate and display the sum over all rows
for all columns in the browser class. Having something other than a
number would make that code also more convoluted.

>> === modified file 'lib/lp/bugs/templates/distribution-upstream-bug-report.pt'
>> --- lib/lp/bugs/templates/distribution-upstream-bug-report.pt 2009-09-04 16:28:13 +0000
>> +++ lib/lp/bugs/templates/distribution-upstream-bug-report.pt 2010-02-16 16:16:26 +0000
>> @@ -131,6 +131,13 @@
>> <img id="sortarrow-watched-delta"
>> tal:attributes="src links/watched_bugs_delta/arrow" />
>> </th>
>> + <th class="amount">
>> + <a id="sort-bugs-with-upstream-patches"
>> + tal:attributes="href links/bugs_with_upstream_patches/link"
>> + >Patches for upstream</a>
>
> I'm concerned about the heading length for this column. I hope in
> your UI review it will come up and see if you can shorten it as the
> table is getting quite wide now.

I am too a bit concerned. I think the page does not yet become too wide,
but having a shorter columkn title is always good. Only problem is that
I could not come up with a better name... Suggestions are welcome.

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

iD8DBQFLet1HekBPhm8NrtARAi5eAJ4+7K3ypz3d4lZ5/tf9Qi8G1OeA/wCgkW59
YLCysmSM38v6BYRfWnY3DRY=
=CJV1
-----END PGP SIGNATURE-----