launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04357
lp:~allenap/launchpad/localpackagediffs-show-changed-by-bug-798865 into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/localpackagediffs-show-changed-by-bug-798865 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #798865 in Launchpad itself: "+localpackagediffs page should show who made the change rather than sponsor"
https://bugs.launchpad.net/launchpad/+bug/798865
For more details, see:
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-show-changed-by-bug-798865/+merge/69072
This changes the "Last changed by" column to "Last changed" because it shows a date too (it already did that before this branch). It also shows the in that column the person who made the package change instead of the person who uploaded it. In many cases these people are different. It does, however, show the uploader in smaller text.
Screenshot:
http://people.canonical.com/~gavin/ui/localpackagediffs-show-changed-by-bug-798865/last-changed.png
It also merges the tests in TestDistroSeriesLocalDifferences and TestDistroSeriesLocalDifferencesZopeless and calls the resulting class TestDistroSeriesLocalDifferences.
--
https://code.launchpad.net/~allenap/launchpad/localpackagediffs-show-changed-by-bug-798865/+merge/69072
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/localpackagediffs-show-changed-by-bug-798865 into lp:launchpad.
=== modified file 'lib/canonical/launchpad/icing/style-3-0.css'
--- lib/canonical/launchpad/icing/style-3-0.css 2011-07-04 13:24:48 +0000
+++ lib/canonical/launchpad/icing/style-3-0.css 2011-07-25 11:27:32 +0000
@@ -306,6 +306,9 @@
min-width: 1em;
outline: 1px #666;
}
+.nowrap {
+ white-space: nowrap;
+ }
/* =========================
Universal presentation
=== modified file 'lib/lp/registry/browser/tests/test_distroseries.py'
--- lib/lp/registry/browser/tests/test_distroseries.py 2011-07-21 09:27:45 +0000
+++ lib/lp/registry/browser/tests/test_distroseries.py 2011-07-25 11:27:32 +0000
@@ -91,6 +91,7 @@
login,
login_celebrity,
login_person,
+ normalize_whitespace,
person_logged_in,
StormStatementRecorder,
TestCaseWithFactory,
@@ -857,93 +858,6 @@
return (derived_series, parent_series)
-class TestDistroSeriesLocalDifferences(
- DistroSeriesDifferenceMixin, TestCaseWithFactory):
- """Test the distroseries +localpackagediffs page."""
-
- layer = DatabaseFunctionalLayer
-
- def setUp(self):
- super(TestDistroSeriesLocalDifferences,
- self).setUp('foo.bar@xxxxxxxxxxxxx')
- set_derived_series_ui_feature_flag(self)
- self.simple_user = self.factory.makePerson()
-
- def test_filter_form_if_differences(self):
- # Test that the page includes the filter form if differences
- # are present
- login_person(self.simple_user)
- derived_series, parent_series = self._createChildAndParent()
- self.factory.makeDistroSeriesDifference(
- derived_series=derived_series)
-
- view = create_initialized_view(
- derived_series, '+localpackagediffs', principal=self.simple_user)
-
- self.assertIsNot(
- None,
- find_tag_by_id(view(), 'distroseries-localdiff-search-filter'),
- "Form filter should be shown when there are differences.")
-
- def test_filter_noform_if_nodifferences(self):
- # Test that the page doesn't includes the filter form if no
- # differences are present
- login_person(self.simple_user)
- derived_series, parent_series = self._createChildAndParent()
-
- view = create_initialized_view(
- derived_series, '+localpackagediffs', principal=self.simple_user)
-
- self.assertIs(
- None,
- find_tag_by_id(view(), 'distroseries-localdiff-search-filter'),
- "Form filter should not be shown when there are no differences.")
-
- def test_parent_packagesets_localpackagediffs(self):
- # +localpackagediffs displays the packagesets.
- ds_diff = self.factory.makeDistroSeriesDifference()
- with celebrity_logged_in('admin'):
- ps = self.factory.makePackageset(
- packages=[ds_diff.source_package_name],
- distroseries=ds_diff.derived_series)
-
- with person_logged_in(self.simple_user):
- view = create_initialized_view(
- ds_diff.derived_series,
- '+localpackagediffs',
- principal=self.simple_user)
- html_content = view()
-
- packageset_text = re.compile('\s*' + ps.name)
- self._test_packagesets(
- html_content, packageset_text, 'packagesets',
- 'Packagesets')
-
- def test_parent_packagesets_localpackagediffs_sorts(self):
- # Multiple packagesets are sorted in a comma separated list.
- ds_diff = self.factory.makeDistroSeriesDifference()
- unsorted_names = [u"zzz", u"aaa"]
- with celebrity_logged_in('admin'):
- for name in unsorted_names:
- self.factory.makePackageset(
- name=name,
- packages=[ds_diff.source_package_name],
- distroseries=ds_diff.derived_series)
-
- with person_logged_in(self.simple_user):
- view = create_initialized_view(
- ds_diff.derived_series,
- '+localpackagediffs',
- principal=self.simple_user)
- html_content = view()
-
- packageset_text = re.compile(
- '\s*' + ', '.join(sorted(unsorted_names)))
- self._test_packagesets(
- html_content, packageset_text, 'packagesets',
- 'Packagesets')
-
-
class TestDistroSeriesLocalDiffPerformance(TestCaseWithFactory,
DistroSeriesDifferenceMixin):
"""Test the distroseries +localpackagediffs page's performance."""
@@ -1076,8 +990,8 @@
self._assertQueryCount(derived_series)
-class TestDistroSeriesLocalDifferencesZopeless(TestCaseWithFactory,
- DistroSeriesDifferenceMixin):
+class TestDistroSeriesLocalDifferences(TestCaseWithFactory,
+ DistroSeriesDifferenceMixin):
"""Test the distroseries +localpackagediffs view."""
layer = LaunchpadFunctionalLayer
@@ -1107,6 +1021,88 @@
principal=get_current_principal(),
current_request=True)
+ def test_filter_form_if_differences(self):
+ # Test that the page includes the filter form if differences
+ # are present
+ simple_user = self.factory.makePerson()
+ login_person(simple_user)
+ derived_series, parent_series = self._createChildAndParent()
+ self.factory.makeDistroSeriesDifference(
+ derived_series=derived_series)
+
+ set_derived_series_ui_feature_flag(self)
+ view = create_initialized_view(
+ derived_series, '+localpackagediffs', principal=simple_user)
+
+ self.assertIsNot(
+ None,
+ find_tag_by_id(view(), 'distroseries-localdiff-search-filter'),
+ "Form filter should be shown when there are differences.")
+
+ def test_filter_noform_if_nodifferences(self):
+ # Test that the page doesn't includes the filter form if no
+ # differences are present
+ simple_user = self.factory.makePerson()
+ login_person(simple_user)
+ derived_series, parent_series = self._createChildAndParent()
+
+ set_derived_series_ui_feature_flag(self)
+ view = create_initialized_view(
+ derived_series, '+localpackagediffs', principal=simple_user)
+
+ self.assertIs(
+ None,
+ find_tag_by_id(view(), 'distroseries-localdiff-search-filter'),
+ "Form filter should not be shown when there are no differences.")
+
+ def test_parent_packagesets_localpackagediffs(self):
+ # +localpackagediffs displays the packagesets.
+ ds_diff = self.factory.makeDistroSeriesDifference()
+ with celebrity_logged_in('admin'):
+ ps = self.factory.makePackageset(
+ packages=[ds_diff.source_package_name],
+ distroseries=ds_diff.derived_series)
+
+ set_derived_series_ui_feature_flag(self)
+ simple_user = self.factory.makePerson()
+ with person_logged_in(simple_user):
+ view = create_initialized_view(
+ ds_diff.derived_series,
+ '+localpackagediffs',
+ principal=simple_user)
+ html_content = view()
+
+ packageset_text = re.compile('\s*' + ps.name)
+ self._test_packagesets(
+ html_content, packageset_text, 'packagesets',
+ 'Packagesets')
+
+ def test_parent_packagesets_localpackagediffs_sorts(self):
+ # Multiple packagesets are sorted in a comma separated list.
+ ds_diff = self.factory.makeDistroSeriesDifference()
+ unsorted_names = [u"zzz", u"aaa"]
+ with celebrity_logged_in('admin'):
+ for name in unsorted_names:
+ self.factory.makePackageset(
+ name=name,
+ packages=[ds_diff.source_package_name],
+ distroseries=ds_diff.derived_series)
+
+ set_derived_series_ui_feature_flag(self)
+ simple_user = self.factory.makePerson()
+ with person_logged_in(simple_user):
+ view = create_initialized_view(
+ ds_diff.derived_series,
+ '+localpackagediffs',
+ principal=simple_user)
+ html_content = view()
+
+ packageset_text = re.compile(
+ '\s*' + ', '.join(sorted(unsorted_names)))
+ self._test_packagesets(
+ html_content, packageset_text, 'packagesets',
+ 'Packagesets')
+
def test_view_redirects_without_feature_flag(self):
# If the feature flag soyuz.derived_series_ui.enabled is not set the
# view simply redirects to the derived series.
@@ -1349,6 +1345,38 @@
self.assertEqual(versions['derived'], derived_span[0].string.strip())
self.assertEqual(versions['parent'], parent_span[0].string.strip())
+ def test_diff_row_shows_spr_creator(self):
+ # The SPR creator (i.e. who make the package change, rather than the
+ # uploader) is shown on each difference row.
+ set_derived_series_ui_feature_flag(self)
+ dsd = self.makePackageUpgrade()
+ view = self.makeView(dsd.derived_series)
+ root = html.fromstring(view())
+ [creator_cell] = root.cssselect(
+ "table.listing tbody td.last-changed")
+ self.assertEqual(
+ "a moment ago by %s" % (
+ dsd.source_package_release.creator.displayname,),
+ normalize_whitespace(creator_cell.text_content()))
+
+ def test_diff_row_shows_spr_creator_and_uploader_if_different(self):
+ # When the SPR creator and uploader are different both are named on
+ # each difference row.
+ set_derived_series_ui_feature_flag(self)
+ dsd = self.makePackageUpgrade()
+ uploader = self.factory.makePerson()
+ removeSecurityProxy(dsd.source_package_release).dscsigningkey = (
+ self.factory.makeGPGKey(uploader))
+ view = self.makeView(dsd.derived_series)
+ root = html.fromstring(view())
+ [creator_cell] = root.cssselect(
+ "table.listing tbody td.last-changed")
+ self.assertEqual(
+ "a moment ago by %s (uploaded by %s)" % (
+ dsd.source_package_release.creator.displayname,
+ dsd.source_package_release.dscsigningkey.owner.displayname),
+ normalize_whitespace(creator_cell.text_content()))
+
def test_getUpgrades_shows_updates_in_parent(self):
# The view's getUpgrades methods lists packages that can be
# trivially upgraded: changed in the parent, not changed in the
@@ -1477,12 +1505,6 @@
recorder2,
HasQueryCount(Equals(recorder1.count)))
-
-class TestDistroSeriesLocalDifferencesFunctional(TestCaseWithFactory,
- DistroSeriesDifferenceMixin):
-
- layer = LaunchpadFunctionalLayer
-
def makeDSDJob(self, dsd):
"""Create a `DistroSeriesDifferenceJob` to update `dsd`."""
job_source = getUtility(IDistroSeriesDifferenceJobSource)
@@ -2213,7 +2235,7 @@
DistroSeriesDifferenceMixin):
"""Test the distroseries +missingpackages page."""
- layer = DatabaseFunctionalLayer
+ layer = LaunchpadFunctionalLayer
def setUp(self):
super(DistroSeriesMissingPackagesPageTestCase,
@@ -2244,6 +2266,46 @@
html_content, packageset_text, 'parent-packagesets',
'Parent packagesets')
+ def test_diff_row_shows_spr_creator(self):
+ # The parent SPR creator (i.e. who make the package change, rather
+ # than the uploader) is shown on each difference row.
+ missing_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
+ dsd = self.factory.makeDistroSeriesDifference(
+ difference_type=missing_type)
+ with person_logged_in(self.simple_user):
+ view = create_initialized_view(
+ dsd.derived_series, '+missingpackages',
+ principal=self.simple_user)
+ root = html.fromstring(view())
+ [creator_cell] = root.cssselect(
+ "table.listing tbody td.last-changed")
+ self.assertEqual(
+ dsd.parent_source_package_release.creator.displayname,
+ creator_cell.find(".//a").text)
+
+ def test_diff_row_shows_spr_creator_and_uploader_if_different(self):
+ # When the SPR creator and uploader are different both are named on
+ # each difference row.
+ missing_type = DistroSeriesDifferenceType.MISSING_FROM_DERIVED_SERIES
+ dsd = self.factory.makeDistroSeriesDifference(
+ difference_type=missing_type)
+ uploader = self.factory.makePerson()
+ naked_spr = removeSecurityProxy(dsd.parent_source_package_release)
+ naked_spr.dscsigningkey = self.factory.makeGPGKey(uploader)
+ with person_logged_in(self.simple_user):
+ view = create_initialized_view(
+ dsd.derived_series, '+missingpackages',
+ principal=self.simple_user)
+ root = html.fromstring(view())
+ [creator_cell] = root.cssselect(
+ "table.listing tbody td.last-changed")
+ parent_spr = dsd.parent_source_package_release
+ self.assertEqual(
+ "a moment ago by %s (uploaded by %s)" % (
+ parent_spr.creator.displayname,
+ parent_spr.dscsigningkey.owner.displayname),
+ normalize_whitespace(creator_cell.text_content()))
+
class DistroSerieUniquePackageDiffsTestCase(TestCaseWithFactory,
DistroSeriesDifferenceMixin):
=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py 2011-07-08 09:06:24 +0000
+++ lib/lp/registry/model/distroseriesdifference.py 2011-07-25 11:27:32 +0000
@@ -481,11 +481,13 @@
gpgkeys = bulk.load_related(GPGKey, sprs, ("dscsigningkeyID",))
# Load DistroSeriesDifferenceComment owners,
- # SourcePackageRecipeBuild requesters and GPGKey owners.
+ # SourcePackageRecipeBuild requesters and GPGKey owners, and
+ # SourcePackageRelease creators.
person_ids = set().union(
(dsdc.message.ownerID for dsdc in latest_comments),
(sprb.requester_id for sprb in sprbs),
- (gpgkey.ownerID for gpgkey in gpgkeys))
+ (gpgkey.ownerID for gpgkey in gpgkeys),
+ (spr.creatorID for spr in sprs))
uploaders = getUtility(IPersonSet).getPrecachedPersonsFromIDs(
person_ids, need_validity=True)
list(uploaders)
=== modified file 'lib/lp/registry/templates/distroseries-localdifferences.pt'
--- lib/lp/registry/templates/distroseries-localdifferences.pt 2011-07-20 16:59:53 +0000
+++ lib/lp/registry/templates/distroseries-localdifferences.pt 2011-07-25 11:27:32 +0000
@@ -69,7 +69,7 @@
class="package-sets">
Package-sets
</th>
- <th>Last uploaded</th>
+ <th>Last changed</th>
<th>Latest comment</th>
</tr>
</thead>
@@ -145,26 +145,42 @@
<tal:replace replace="difference/@@/packagesets_names" />
</td>
- <td>
+ <td class="last-changed">
<tal:parent condition="not: view/show_derived_version">
<tal:published condition="parent_source_pub">
<span tal:attributes="title parent_source_pub/datepublished/fmt:datetime"
tal:content="parent_source_pub/datepublished/fmt:approximatedate">2005-09-16</span>
- <tal:signer condition="parent_source_pub/sourcepackagerelease/uploader">
- by <a tal:replace="structure parent_source_pub/sourcepackagerelease/uploader/fmt:link">Steph Smith</a>
- </tal:signer>
+ <tal:creator define="spr parent_source_pub/sourcepackagerelease">
+ <span class="nowrap">by <a tal:replace="structure spr/creator/fmt:link" /></span>
+ </tal:creator>
+ <tal:uploader
+ define="spr parent_source_pub/sourcepackagerelease"
+ condition="python: spr.uploader not in (None, spr.creator)">
+ <br />
+ <span class="discreet nowrap">
+ (uploaded by <a tal:replace="structure spr/uploader/fmt:link" />)
+ </span>
+ </tal:uploader>
</tal:published>
<tal:not-published condition="not:parent_source_pub">
<span tal:content="difference/parent_source_version" />
</tal:not-published>
</tal:parent>
<tal:derived condition="view/show_derived_version">
- <tal:published condition="parent_source_pub">
+ <tal:published condition="source_pub">
<span tal:attributes="title source_pub/datepublished/fmt:datetime"
tal:content="source_pub/datepublished/fmt:approximatedate">2005-09-16</span>
- <tal:signer condition="source_pub/sourcepackagerelease/uploader">
- by <a tal:replace="structure source_pub/sourcepackagerelease/uploader/fmt:link">Steph Smith</a>
- </tal:signer>
+ <tal:creator define="spr source_pub/sourcepackagerelease">
+ <span class="nowrap">by <a tal:replace="structure spr/creator/fmt:link" /></span>
+ </tal:creator>
+ <tal:uploader
+ define="spr source_pub/sourcepackagerelease"
+ condition="python: spr.uploader not in (None, spr.creator)">
+ <br />
+ <span class="discreet nowrap">
+ (uploaded by <a tal:replace="structure spr/uploader/fmt:link" />)
+ </span>
+ </tal:uploader>
</tal:published>
<tal:not-published condition="not:source_pub">
<span tal:content="difference/source_version" />
=== modified file 'lib/lp/soyuz/model/sourcepackagerelease.py'
--- lib/lp/soyuz/model/sourcepackagerelease.py 2011-06-16 11:31:24 +0000
+++ lib/lp/soyuz/model/sourcepackagerelease.py 2011-07-25 11:27:32 +0000
@@ -47,7 +47,6 @@
LibraryFileContent,
)
from canonical.launchpad.helpers import shortlist
-from lp.app.errors import NotFoundError
from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.archiveuploader.utils import determine_source_file_type
from lp.buildmaster.enums import BuildStatus
@@ -61,10 +60,7 @@
PackageDiffStatus,
PackagePublishingStatus,
)
-from lp.soyuz.interfaces.archive import (
- IArchiveSet,
- MAIN_ARCHIVE_PURPOSES,
- )
+from lp.soyuz.interfaces.archive import MAIN_ARCHIVE_PURPOSES
from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
from lp.soyuz.interfaces.packagediff import PackageDiffAlreadyRequested
from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
@@ -574,7 +570,7 @@
queue = getUtility(ITranslationImportQueue)
- only_templates=self.sourcepackage.has_sharing_translation_templates
+ only_templates = self.sourcepackage.has_sharing_translation_templates
queue.addOrUpdateEntriesFromTarball(
tarball, by_maintainer, importer,
sourcepackagename=self.sourcepackagename,
