launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03607
[Merge] lp:~sinzui/launchpad/project-review-1 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/project-review-1 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #782206 in Launchpad itself: "+review-licenses formatting is broken"
https://bugs.launchpad.net/launchpad/+bug/782206
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/project-review-1/+merge/60941
Fix +review-licenses formatting.
Launchpad bug: https://bugs.launchpad.net/bugs/782206
Pre-implementation: no one
1. Projects with dots (.) in their name are enabled to ajax review. This is
because the project name is used to make the control id. The dots must be
replaced with dashes.
2. The whiteboard markup is visible; the template should use structure.
3. Long whiteboards and license_info break the layout showing information that
is rarely needed.
--------------------------------------------------------------------
RULES
* Use Formatters.css_id and fmt:css-id to ensure the ids are valid.
* Use structure when rendering the whiteboard
* Use CSS overflow: auto; to add scrollbars to long content instead of
displaying all the test in the row.
QA
* Visit https://qastaging.launchpad.net/projects/+review-licenses
* Administer a project to put dots in the name
* Reload +review-licenses
* Verify the review/approve widgets work.
* Verify that whiteboards do not show <p>s
* Verify long whiteboard and licenses_info entries scrolled.
LINT
lib/lp/registry/browser/product.py
lib/lp/registry/browser/tests/test_product.py
lib/lp/registry/templates/product-listing-for-review.pt
lib/lp/registry/templates/products-review-licenses.pt
TEST
./bin/test -vv -t ProductSetReviewLicensesViewTestCase -t TestProductView
IMPLEMENTATION
Escaped product.name to make valid CSS Ids.
lib/lp/registry/browser/product.py
lib/lp/registry/browser/tests/test_product.py
lib/lp/registry/templates/product-listing-for-review.pt
Use structure to render the formatted content.
lib/lp/registry/templates/product-listing-for-review.pt
Added rule to add scrollbars to long content.
lib/lp/registry/templates/products-review-licenses.pt
--
https://code.launchpad.net/~sinzui/launchpad/project-review-1/+merge/60941
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/project-review-1 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py 2011-05-12 18:25:06 +0000
+++ lib/lp/registry/browser/product.py 2011-05-13 15:41:29 +0000
@@ -133,6 +133,7 @@
BooleanChoiceWidget,
TextLineEditorWidget,
)
+from lp.app.browser.stringformatter import FormattersAPI
from lp.app.browser.tales import MenuAPI
from lp.app.enums import ServiceUsage
from lp.app.errors import NotFoundError
@@ -1145,7 +1146,8 @@
def active_widget(self):
return BooleanChoiceWidget(
self.context, IProduct['active'],
- content_box_id='%s-edit-active' % self.context.name,
+ content_box_id='%s-edit-active' % FormattersAPI(
+ self.context.name).css_id(),
edit_view='+review-license',
tag='span',
false_text='Deactivted',
@@ -1156,7 +1158,8 @@
def project_reviewed_widget(self):
return BooleanChoiceWidget(
self.context, IProduct['project_reviewed'],
- content_box_id='%s-edit-project-reviewed' % self.context.name,
+ content_box_id='%s-edit-project-reviewed' % FormattersAPI(
+ self.context.name).css_id(),
edit_view='+review-license',
tag='span',
false_text='Unreviewed',
@@ -1172,7 +1175,8 @@
return 'License required'
return BooleanChoiceWidget(
self.context, IProduct['license_approved'],
- content_box_id='%s-edit-license-approved' % self.context.name,
+ content_box_id='%s-edit-license-approved' % FormattersAPI(
+ self.context.name).css_id(),
edit_view='+review-license',
tag='span',
false_text='Unapproved',
=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py 2011-05-12 18:25:06 +0000
+++ lib/lp/registry/browser/tests/test_product.py 2011-05-13 15:41:29 +0000
@@ -286,6 +286,21 @@
text = view.license_approved_widget
self.assertEqual('License required', text)
+ def test_widget_id_for_name_dots(self):
+ # Dots are replaced with dashes to make a valid CSS Id.
+ login_celebrity('registry_experts')
+ self.product.name = 'fnord.dom'
+ view = create_initialized_view(self.product, '+index')
+ self.assertEqual(
+ 'fnord-dom-edit-active',
+ view.active_widget.content_box_id)
+ self.assertEqual(
+ 'fnord-dom-edit-project-reviewed',
+ view.project_reviewed_widget.content_box_id)
+ self.assertEqual(
+ 'fnord-dom-edit-license-approved',
+ view.license_approved_widget.content_box_id)
+
class ProductSetReviewLicensesViewTestCase(TestCaseWithFactory):
"""Tests the ProductSetReviewLicensesView."""
@@ -331,7 +346,7 @@
content = find_tag_by_id(view.render(), 'project-fnord')
self.assertTrue(content.find(id='fnord-maintainer') is not None)
self.assertTrue(content.find(id='fnord-registrant') is not None)
- self.assertTrue(content.find(id='fnord-desciption') is not None)
+ self.assertTrue(content.find(id='fnord-description') is not None)
self.assertTrue(content.find(id='fnord-packages') is not None)
self.assertTrue(content.find(id='fnord-releases') is not None)
self.assertTrue(content.find(id='fnord-usage') is not None)
=== modified file 'lib/lp/registry/templates/product-listing-for-review.pt'
--- lib/lp/registry/templates/product-listing-for-review.pt 2011-05-12 18:53:28 +0000
+++ lib/lp/registry/templates/product-listing-for-review.pt 2011-05-13 15:41:29 +0000
@@ -4,7 +4,7 @@
xmlns:i18n="http://xml.zope.org/namespaces/i18n";
omit-tag="">
- <tr tal:attributes="id string:project-${context/name}">
+ <tr tal:attributes="id string:project-${context/name/fmt:css-id}">
<td>
<p>
<a tal:replace="structure context/fmt:link" />
@@ -17,40 +17,40 @@
<dl class="horizontal">
<dt>Maintained by</dt>
- <dd tal:attributes="id string:${context/name}-maintainer">
+ <dd tal:attributes="id string:${context/name/fmt:css-id}-maintainer">
<a tal:replace="structure context/owner/fmt:link" />
</dd>
<dt>Registered by</dt>
- <dd tal:attributes="id string:${context/name}-registrant">
+ <dd tal:attributes="id string:${context/name/fmt:css-id}-registrant">
<a tal:replace="structure context/registrant/fmt:link" />
</dd>
</dl>
<p
- title="Beware of these words: test, prueba, personal, sandbox, repo, package, archive"
+ title="Beware of these words: test, prueba, personal, sandbox, repo, package, archive"
tal:content="context/summary" />
<div
title="Beware of these words: test, prueba, personal, sandbox, repo, package, archive"
tal:condition="context/description"
- tal:attributes="id string:${context/name}-desciption"
+ tal:attributes="id string:${context/name/fmt:css-id}-description"
tal:content="structure context/description/fmt:text-to-html" />
<dl class="horizontal"
title="Projects with packages and releases cannot be deactivated.">
<dt>Has packages</dt>
<dd
- tal:attributes="id string:${context/name}-packages"
+ tal:attributes="id string:${context/name/fmt:css-id}-packages"
tal:content="structure context/ubuntu_packages/image:boolean" />
<dt>Has releases</dt>
<dd
- tal:attributes="id string:${context/name}-releases"
+ tal:attributes="id string:${context/name/fmt:css-id}-releases"
tal:content="structure view/latest_release_with_download_files/image:boolean" />
</dl>
<dl class="horizontal"
title="Projects over 6 months old should be using an application."
- tal:attributes="id string:${context/name}-usage">
+ tal:attributes="id string:${context/name/fmt:css-id}-usage">
<dt>Code usage</dt>
<dd tal:content="context/codehosting_usage/title" />
<dt>Bug usage</dt>
@@ -61,7 +61,7 @@
</td>
<td>
- <dl tal:attributes="id string:${context/name}-statuses">
+ <dl tal:attributes="id string:${context/name/fmt:css-id}-statuses">
<dt>Project enabled</dt>
<dd tal:content="structure view/active_widget" />
<dt>Project reviewed</dt>
@@ -72,7 +72,7 @@
</dl>
<dl class="horizontal"
- tal:attributes="id string:${context/name}-licenses">
+ tal:attributes="id string:${context/name/fmt:css-id}-licenses">
<dt>Licenses</dt>
<dd>
<ul class="horizontal">
@@ -86,7 +86,7 @@
<dl class="horizontal"
tal:condition="view/is_proprietary"
- tal:attributes="id string:${context/name}-commercial-subscription">
+ tal:attributes="id string:${context/name/fmt:css-id}-commercial-subscription">
<dt>Commercial subscription expiration</dt>
<dd
tal:condition="context/commercial_subscription"
@@ -97,14 +97,18 @@
<dl class="horizontal"
tal:condition="view/show_license_info"
- tal:attributes="id string:${context/name}-license-info">
+ tal:attributes="id string:${context/name/fmt:css-id}-license-info">
<dt>Licenses info</dt>
- <dd tal:content="structure context/license_info/fmt:text-to-html" />
+ <dd class="scrolled"
+ tal:content="structure context/license_info/fmt:text-to-html" />
</dl>
- <div
- tal:attributes="id string:${context/name}-whiteboard"
- tal:content="context/reviewer_whiteboard/fmt:text-to-html" />
+ <dl
+ tal:attributes="id string:${context/name/fmt:css-id}-whiteboard">
+ <dt>Whiteboard</dt>
+ <dd class="scrolled"
+ tal:content="structure context/reviewer_whiteboard/fmt:text-to-html" />
+ </dl>
</td>
</tr>
</tal:root>
=== modified file 'lib/lp/registry/templates/products-review-licenses.pt'
--- lib/lp/registry/templates/products-review-licenses.pt 2011-05-10 22:16:37 +0000
+++ lib/lp/registry/templates/products-review-licenses.pt 2011-05-13 15:41:29 +0000
@@ -26,6 +26,10 @@
dl + p {
margin-top: 1em;
}
+ .scrolled {
+ max-height: 6em;
+ overflow: auto;
+ }
</style>
</metal:block>
@@ -39,7 +43,7 @@
<tal:navigation replace="structure batch/@@+navigation-links-upper" />
<table id="product-listing" class="listing">
<thead>
- <th style="max-width: 45em">Project information</th>
+ <th style="width: 45em">Project information</th>
<th>Status and Licenses</th>
</thead>
<tbody>
