launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01068
[Merge] lp:~michael.nelson/launchpad/635005-difference-details-2 into lp:launchpad
Michael Nelson has proposed merging lp:~michael.nelson/launchpad/635005-difference-details-2 into lp:launchpad.
Requested reviews:
Launchpad UI Reviewers (launchpad-ui-reviewers): ui
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#635005 DistroSeriesDifference detailed dropdown is missing
https://bugs.launchpad.net/bugs/635005
Overview
========
This branch adds comment rendering to the extra-details template for DistroSeriesDifferences and hooks them up to the DistroSeriesDifference list like this:
https://devpad.canonical.com/~michaeln/tmp/distroseriesdifference-extra.ogv
Details
=======
This branch adds and tests the rendering of comments on the distroseriesdifference-listing-extra.pt template, and then adds the non-js link to this page from the distroseries-localdifferences.pt template.
It then adds JS to enhance this by loading the snippet and adding it inline as an expanded table row.
Styling
=======
I need to check with Curtis as I don't think he'll want the additional css in the universal section (or with those classnames), but I wasn't sure what else to do (other than just styling the comments similar to merge proposals / bugs... actually, in retrospect I should probably just do that).
To test
=======
bin/test -vv -m test_distroseriesdifference_views -m test_series_views
Note: I've started a windmill test, but it's doing this: http://pastebin.ubuntu.com/494647/ Any ideas? (oh, actually this seems to happen for the other tests too since merging a new db-devel - I'll keep looking into why this is happening).
To demo
=======
Run http://pastebin.ubuntu.com/494656/ in bin/iharness and then open
https://launchpad.dev/ubuntu/hoary/+localpackagediffs
--
https://code.launchpad.net/~michael.nelson/launchpad/635005-difference-details-2/+merge/35640
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~michael.nelson/launchpad/635005-difference-details-2 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
--- lib/canonical/launchpad/icing/style-3-0.css.in 2010-08-27 19:20:53 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css.in 2010-09-16 09:58:53 +0000
@@ -1804,6 +1804,19 @@
color: #b8b8ff;
}
+/* =========================
+ * Universal presentation
+ * Comments
+ */
+div.comment {
+ margin-bottom: 2em;
+ }
+div.comment .logo {
+ float: left;
+ }
+div.comment .comment-detail {
+ margin-left: 70px;
+ }
/* =========================
Universal presentation
=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt 2010-08-20 13:33:51 +0000
+++ lib/lp/app/templates/base-layout-macros.pt 2010-09-16 09:58:53 +0000
@@ -183,6 +183,8 @@
<script type="text/javascript"
tal:attributes="src string:${lp_js}/bugs/bugtracker_overlay.js"></script>
<script type="text/javascript"
+ tal:attributes="src string:${lp_js}/registry/distroseriesdifferences_details.js"></script>
+ <script type="text/javascript"
tal:attributes="src string:${lp_js}/registry/milestoneoverlay.js"></script>
<script type="text/javascript"
tal:attributes="src string:${lp_js}/registry/milestonetable.js"></script>
=== modified file 'lib/lp/registry/browser/configure.zcml'
--- lib/lp/registry/browser/configure.zcml 2010-09-13 10:04:20 +0000
+++ lib/lp/registry/browser/configure.zcml 2010-09-16 09:58:53 +0000
@@ -151,7 +151,7 @@
permission="zope.Public"/>
<browser:url
for="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifference"
- path_expression="string:+difference/${difference/source_package_name}"
+ path_expression="string:+difference/${source_package_name/name}"
rootsite="mainsite"
attribute_to_parent="derived_series"/>
<browser:page
@@ -160,6 +160,9 @@
class="lp.registry.browser.distroseriesdifference.DistroSeriesDifferenceView"
template="../templates/distroseriesdifference-listing-extra.pt"
permission="zope.Public"/>
+ <browser:defaultView
+ for="lp.registry.interfaces.distroseriesdifference.IDistroSeriesDifference"
+ name="+listing-distroseries-extra"/>
<browser:menus
classes="
DistroSeriesFacets
=== modified file 'lib/lp/registry/browser/tests/test_distroseriesdifference_views.py'
--- lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2010-09-14 13:37:17 +0000
+++ lib/lp/registry/browser/tests/test_distroseriesdifference_views.py 2010-09-16 09:58:53 +0000
@@ -170,3 +170,23 @@
view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
self.assertEqual(1, self.number_of_request_diff_texts(view()))
+
+ def test_comments_rendered(self):
+ # If there are comments on the difference, they are rendered.
+ ds_diff = self.factory.makeDistroSeriesDifference()
+ owner = ds_diff.derived_series.owner
+ with person_logged_in(owner):
+ ds_diff.addComment(owner, "I'm working on this.")
+ ds_diff.addComment(owner, "Here's a \n\ntwo-line comment "
+ "which should be in two paras.")
+
+ view = create_initialized_view(ds_diff, '+listing-distroseries-extra')
+ soup = BeautifulSoup(view())
+
+ self.assertEqual(
+ 1, len(soup.findAll('p', text="I'm working on this.")))
+ self.assertEqual(
+ 1, len(soup.findAll('p', text="Here's a")))
+ self.assertEqual(
+ 1, len(soup.findAll('p',
+ text="two-line comment which should be in two paras.")))
=== modified file 'lib/lp/registry/browser/tests/test_series_views.py'
--- lib/lp/registry/browser/tests/test_series_views.py 2010-09-08 07:53:06 +0000
+++ lib/lp/registry/browser/tests/test_series_views.py 2010-09-16 09:58:53 +0000
@@ -181,6 +181,25 @@
self.assertIn("Latest comment", unicode(rows[0]))
self.assertNotIn("Earlier comment", unicode(rows[0]))
+ def test_diff_row_links_to_extra_details(self):
+ # The source package name links to the difference details.
+ derived_series = self.makeDerivedSeries(
+ parent_name='lucid', derived_name='derilucid')
+ difference = self.factory.makeDistroSeriesDifference(
+ derived_series=derived_series)
+
+ self.setDerivedSeriesUIFeatureFlag()
+ view = create_initialized_view(
+ derived_series, '+localpackagediffs')
+ soup = BeautifulSoup(view())
+ diff_table = soup.find('table', {'class': 'listing'})
+ row = diff_table.tbody.findAll('tr')[0]
+
+ href = canonical_url(difference).replace('http://launchpad.dev', '')
+ links = row.findAll('a', href=href)
+ self.assertEqual(1, len(links))
+ self.assertEqual(difference.source_package_name.name, links[0].string)
+
class TestMilestoneBatchNavigatorAttribute(TestCaseWithFactory):
"""Test the series.milestone_batch_navigator attribute."""
=== added file 'lib/lp/registry/javascript/distroseriesdifferences_details.js'
--- lib/lp/registry/javascript/distroseriesdifferences_details.js 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/javascript/distroseriesdifferences_details.js 2010-09-16 09:58:53 +0000
@@ -0,0 +1,86 @@
+/* Copyright 2010 Canonical Ltd. This software is licensed under the
+ * GNU Affero General Public License version 3 (see the file LICENSE).
+ *
+ * Enhancements for the distroseries differences page.
+ *
+ * @module registry
+ * @submodule distroseriesdifferences_details
+ * @requires io-base, lp.soyuz.base
+ */
+YUI.add('lp.registry.distroseriesdifferences_details', function(Y) {
+
+var namespace = Y.namespace('lp.registry.distroseriesdifferences_details');
+
+/*
+ * Setup the expandable rows for each difference.
+ *
+ * @method setup_expandable_rows
+ */
+namespace.setup_expandable_rows = function() {
+ var start_update = function(uri, container) {
+
+ var in_progress_message = Y.lp.soyuz.base.makeInProgressNode(
+ 'Fetching difference details ...')
+ container.insert(in_progress_message, 'replace');
+
+ var config = {
+ on: {
+ 'success': function(transaction_id, response, args) {
+ args.container.set(
+ 'innerHTML', response.responseText);
+ // Change to insert(,'replace)
+ },
+ 'failure': function(transaction_id, response, args){
+ var retry_handler = function(e) {
+ e.preventDefault();
+ start_update(args.uri, args.container);
+ };
+ var failure_message = Y.lp.soyuz.base.makeFailureNode(
+ 'Failed to fetch difference details.',
+ retry_handler);
+ args.container.insert(failure_message, 'replace');
+
+ var anim = Y.lazr.anim.red_flash({
+ node: args.container
+ });
+ anim.run();
+ }
+ },
+ arguments: {
+ 'container': container,
+ 'uri': uri
+ }
+ };
+ Y.io(uri, config);
+
+ };
+
+ var expander_handler = function(e) {
+ e.preventDefault();
+ var toggle = e.currentTarget;
+ var row = toggle.ancestor('tr');
+ toggle.toggleClass('treeCollapsed').toggleClass('treeExpanded');
+
+ // Only insert if there isn't already a container row there.
+ next_row = row.next();
+ if (next_row == null || !next_row.hasClass('diff-extra')) {
+ details_row = Y.Node.create(
+ '<table><tr class="diff-extra unseen"><td colspan="5"></td></tr></table>').one('tr');
+ row.insert(details_row, 'after');
+ var uri = toggle.get('href');
+ start_update(uri, details_row.one('td'));
+ } else {
+ details_row = next_row
+ }
+
+ details_row.toggleClass('unseen');
+
+ };
+ Y.all('table.listing a.toggle-extra').each(function(toggle){
+ toggle.addClass('treeCollapsed').addClass('sprite');
+ toggle.on("click", expander_handler);
+ })
+
+};
+
+}, "0.1", {"requires": ["io-base", "lp.soyuz.base"]});
=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt 2010-09-08 09:18:11 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt 2010-09-16 09:58:53 +0000
@@ -45,8 +45,8 @@
<tr tal:define="parent_source_pub difference/parent_source_pub;
source_pub difference/source_pub;
signer source_pub/sourcepackagerelease/uploader/fmt:link|nothing;">
- <td><span
- tal:replace="parent_source_pub/source_package_name">Foo</span>
+ <td><a tal:attributes="href difference/fmt:url" class="toggle-extra"
+ tal:content="parent_source_pub/source_package_name">Foo</a>
</td>
<td><a tal:attributes="href parent_source_pub/sourcepackagerelease/fmt:url">
<tal:replace
@@ -79,6 +79,13 @@
</tbody>
</table>
</div>
+<script type="text/javascript">
+LPS.use('lp.registry.distroseriesdifferences_details', function(Y) {
+ diff_module = Y.lp.registry.distroseriesdifferences_details
+
+ Y.on('domready', diff_module.setup_expandable_rows);
+});
+</script>
</div>
=== modified file 'lib/lp/registry/templates/distroseriesdifference-listing-extra.pt'
--- lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2010-09-14 13:37:17 +0000
+++ lib/lp/registry/templates/distroseriesdifference-listing-extra.pt 2010-09-16 09:58:53 +0000
@@ -29,6 +29,17 @@
</dd>
</tal:parent-diff-option>
</dl>
+
<h2>Comments:</h2>
+ <div class="comment" tal:repeat="comment context/getComments">
+ <div class="logo">
+ <span tal:replace="structure comment/message/owner/image:logo"/>
+ </div>
+ <div class="comment-detail">
+ <span tal:replace="structure comment/comment/fmt:text-to-html">My comment</span>
+ <span class="greyed-out greylink"><span tal:replace="comment/message/datecreated/fmt:approximatedate">2005-09-16</span>
+ by <a tal:replace="structure comment/message/owner/fmt:link">joesmith</a></span>
+ </div>
+ </div>
</tal:root>
=== added file 'lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py'
--- lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/windmill/tests/test_distroseriesdifference_expander.py 2010-09-16 09:58:53 +0000
@@ -0,0 +1,36 @@
+# Copyright 2010 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from canonical.launchpad.webapp.publisher import canonical_url
+from canonical.launchpad.windmill.testing import constants
+from lp.registry.windmill.testing import RegistryWindmillLayer
+from lp.services.features.model import FeatureFlag, getFeatureStore
+from lp.testing import WindmillTestCase
+
+
+class TestDistroSeriesDifferenceExtraJS(WindmillTestCase):
+ """Each listed source package can be expanded for extra information."""
+
+ layer = RegistryWindmillLayer
+
+ def setUp(self):
+ """Enable the feature and populate with data."""
+ super(TestDistroSeriesDifferenceExtraJS, self).setUp()
+ # First just ensure that the feature is enabled.
+ getFeatureStore().add(FeatureFlag(
+ scope=u'default', flag=u'soyuz.derived-series-ui.enabled',
+ value=u'on', priority=1))
+
+ # Setup the difference record.
+ self.diff = self.factory.makeDistroSeriesDifference(
+ source_package_name_str="foo", versions=dict(
+ derived='1.15-2ubuntu1derilucid2', parent='1.17-1'))
+
+ self.package_diffs_url = canonical_url(
+ self.diff.derived_series, '+localpackagediffs')
+
+ def test_diff_extra_details_available(self):
+ """A successful request for the extra info updates the display."""
+
+ self.client.open(self.package_diffs_url)
+ self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
