launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04449
[Merge] lp:~rvb/launchpad/api-add-comment-bug-798522 into lp:launchpad
Raphaël Victor Badin has proposed merging lp:~rvb/launchpad/api-add-comment-bug-798522 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #798522 in Launchpad itself: "https://launchpad.net/ubuntu/oneiric/+localpackagediffs should require a comment when blacklisting a package"
https://bugs.launchpad.net/launchpad/+bug/798522
For more details, see:
https://code.launchpad.net/~rvb/launchpad/api-add-comment-bug-798522/+merge/70035
This is the back-end side of the fix for bug 798522. The goal is to better track blackling/unblacklisting actions on DSDs. This branch modifies the methods DSD.blacklist and DSD unblacklist so that: a) they automatically add a comment to the DSD when they are used to change the 'Ignored' status b) they accept an additional string parameter 'comment' that allows the user to document why he changed the 'Ignored' status.
(The next branch will feature the UI for this)
= Tests =
./bin/test -vvc test_distroseriesdifference test_unblacklist_creates_comment
./bin/test -vvc test_distroseriesdifference test_blacklist_creates_comment
= QA =
This is only the back-end side of the change but it can be QAed using .blacklist and .unblacklist via the API and making sure that a proper comment has been added on the corresponding DSD.
--
https://code.launchpad.net/~rvb/launchpad/api-add-comment-bug-798522/+merge/70035
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/launchpad/api-add-comment-bug-798522 into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/distroseriesdifference.py'
--- lib/lp/registry/interfaces/distroseriesdifference.py 2011-07-29 15:47:45 +0000
+++ lib/lp/registry/interfaces/distroseriesdifference.py 2011-08-01 16:01:27 +0000
@@ -244,21 +244,36 @@
class IDistroSeriesDifferenceAdmin(Interface):
"""Difference attributes requiring launchpad.Admin."""
+ @call_with(commenter=REQUEST_USER)
@operation_parameters(
- all=Bool(title=_("All"), required=False))
+ all=Bool(title=_("All"), required=False),
+ comment=TextLine(title=_('Comment text'), required=False),
+ )
@export_write_operation()
- def blacklist(all=False):
- """Blacklist this version or all versions of this source package.
+ def blacklist(commenter, all=False, comment=None):
+ """Blacklist this version or all versions of this source package and
+ adds a comment on this difference.
- :param all: indicates whether all versions of this package should
+ :param commenter: The requestor `IPerson`.
+ :param comment: The comment string.
+ :param all: Indicates whether all versions of this package should
be blacklisted or just the current (default).
+ :return: The created `DistroSeriesDifferenceComment` object.
"""
+ @call_with(commenter=REQUEST_USER)
+ @operation_parameters(
+ comment=TextLine(title=_('Comment text'), required=False))
@export_write_operation()
- def unblacklist():
- """Removes this difference from the blacklist.
+ def unblacklist(commenter, comment=None):
+ """Removes this difference from the blacklist and adds a comment on
+ this difference.
The status will be updated based on the versions.
+
+ :param commenter: The requestor `IPerson`.
+ :param comment: The comment string.
+ :return: The created `DistroSeriesDifferenceComment` object.
"""
@@ -290,6 +305,7 @@
:return: A new `DistroSeriesDifference` object.
"""
+<<<<<<< TREE
def getForDistroSeries(
distro_series,
difference_type=None,
@@ -299,6 +315,12 @@
parent_series=None,
packagesets=None,
changed_by=None):
+=======
+ def getForDistroSeries(distro_series, difference_type=None,
+ name_filter=None, status=None,
+ child_version_higher=False, parent_series=None,
+ packagesets=None):
+>>>>>>> MERGE-SOURCE
"""Return differences for the derived distro series sorted by
package name.
=== modified file 'lib/lp/registry/model/distroseriesdifference.py'
--- lib/lp/registry/model/distroseriesdifference.py 2011-07-28 18:37:47 +0000
+++ lib/lp/registry/model/distroseriesdifference.py 2011-08-01 16:01:27 +0000
@@ -365,6 +365,16 @@
SourcePackageName, dsds, ("source_package_name_id",))
+def get_comment_with_status_change(status, new_status, comment):
+ # Create a new comment string with the description of the status
+ # change and the given comment string.
+ new_comment = "Ignored: %s => %s" % (
+ status.title, new_status.title)
+ if comment:
+ new_comment = "%s\n\n%s" % (comment, new_comment)
+ return new_comment
+
+
class DistroSeriesDifference(StormBase):
"""See `DistroSeriesDifference`."""
implements(IDistroSeriesDifference)
@@ -885,17 +895,29 @@
DSDComment.distro_series_difference == self)
return comments.order_by(Desc(DSDComment.id))
- def blacklist(self, all=False):
+ def _getCommentWithStatusChange(self, new_status, comment=None):
+ return get_comment_with_status_change(
+ self.status, new_status, comment)
+
+ def blacklist(self, commenter, all=False, comment=None):
"""See `IDistroSeriesDifference`."""
if all:
- self.status = DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS
+ new_status = DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS
else:
- self.status = DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT
+ new_status = DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT
+ new_comment = self._getCommentWithStatusChange(new_status, comment)
+ dsd_comment = self.addComment(commenter, new_comment)
+ self.status = new_status
+ return dsd_comment
- def unblacklist(self):
+ def unblacklist(self, commenter, comment=None):
"""See `IDistroSeriesDifference`."""
- self.status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
+ new_status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
+ new_comment = self._getCommentWithStatusChange(new_status, comment)
+ self.status = new_status
+ dsd_comment = self.addComment(commenter, new_comment)
self.update(manual=True)
+ return dsd_comment
def requestPackageDiffs(self, requestor):
"""See `IDistroSeriesDifference`."""
=== modified file 'lib/lp/registry/tests/test_distroseriesdifference.py'
--- lib/lp/registry/tests/test_distroseriesdifference.py 2011-07-28 18:37:47 +0000
+++ lib/lp/registry/tests/test_distroseriesdifference.py 2011-08-01 16:01:27 +0000
@@ -32,6 +32,7 @@
)
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.model.distroseriesdifference import (
+ get_comment_with_status_change,
most_recent_comments,
most_recent_publications,
)
@@ -464,7 +465,7 @@
ds_diff.derived_series.main_archive)
with person_logged_in(admin):
- ds_diff.blacklist()
+ ds_diff.blacklist(admin)
self.assertEqual(
DistroSeriesDifferenceStatus.BLACKLISTED_CURRENT,
@@ -477,7 +478,7 @@
ds_diff.derived_series.main_archive)
with person_logged_in(admin):
- ds_diff.blacklist(all=True)
+ ds_diff.blacklist(admin, all=True)
self.assertEqual(
DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS,
@@ -491,7 +492,7 @@
ds_diff.derived_series.main_archive)
with person_logged_in(admin):
- ds_diff.unblacklist()
+ ds_diff.unblacklist(admin)
self.assertEqual(
DistroSeriesDifferenceStatus.NEEDS_ATTENTION,
@@ -514,12 +515,65 @@
admin = self.factory.makeArchiveAdmin(
ds_diff.derived_series.main_archive)
with person_logged_in(admin):
- ds_diff.unblacklist()
+ ds_diff.unblacklist(admin)
self.assertEqual(
DistroSeriesDifferenceStatus.RESOLVED,
ds_diff.status)
+ def test_get_comment_with_status_change(self):
+ # Test the new comment string created to describe a status
+ # change.
+ old_status = DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS
+ new_status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
+ new_comment = get_comment_with_status_change(
+ old_status, new_status, 'simple comment')
+
+ self.assertEqual(
+ 'simple comment\n\nIgnored: %s => %s' % (
+ old_status.title, new_status.title),
+ new_comment)
+
+ def assertDSDComment(self, ds_diff, dsd_comment, comment_string):
+ self.assertEqual(
+ dsd_comment,
+ ds_diff.latest_comment)
+ self.assertEqual(
+ comment_string,
+ ds_diff.latest_comment.message.text_contents)
+
+ def test_unblacklist_creates_comment(self):
+ old_status = DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS
+ ds_diff = self.factory.makeDistroSeriesDifference(
+ status=old_status,
+ source_package_name_str="foo")
+ admin = self.factory.makeArchiveAdmin(
+ ds_diff.derived_series.main_archive)
+ with person_logged_in(admin):
+ dsd_comment = ds_diff.unblacklist(
+ admin, "Ok now")
+ new_status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
+ expected_comment = 'Ok now\n\nIgnored: %s => %s' % (
+ old_status.title, new_status.title)
+
+ self.assertDSDComment(ds_diff, dsd_comment, expected_comment)
+
+ def test_blacklist_creates_comment(self):
+ old_status = DistroSeriesDifferenceStatus.NEEDS_ATTENTION
+ ds_diff = self.factory.makeDistroSeriesDifference(
+ status=old_status,
+ source_package_name_str="foo")
+ admin = self.factory.makeArchiveAdmin(
+ ds_diff.derived_series.main_archive)
+ with person_logged_in(admin):
+ dsd_comment = ds_diff.blacklist(
+ admin, True, "Wait until version 2.1")
+ new_status = DistroSeriesDifferenceStatus.BLACKLISTED_ALWAYS
+ expected_comment = 'Wait until version 2.1\n\nIgnored: %s => %s' % (
+ old_status.title, new_status.title)
+
+ self.assertDSDComment(ds_diff, dsd_comment, expected_comment)
+
def test_source_package_name_unique_for_derived_series(self):
# We cannot create two differences for the same derived series
# for the same package.
