launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05313
[Merge] lp:~wallyworld/launchpad/delete-bugtask-ui-878909 into lp:launchpad
Ian Booth has proposed merging lp:~wallyworld/launchpad/delete-bugtask-ui-878909 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #878909 in Launchpad itself: "allow users to delete bugtasks using the web UI"
https://bugs.launchpad.net/launchpad/+bug/878909
For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/delete-bugtask-ui-878909/+merge/80179
Update the ui to support bugtask deletion.
== Implementation ==
Add a new BugTaskDeleteView and render delete icons next to each bug task that can be deleted by the logged in user.
This mp performs the delete using html forms. A subsequent branch will provide ajax support.
The permission checking code was moved from the security adaptor to a method on BugTask so it could be reused by the view.
To recap: the delete capability is protected by a feature flag, is restricted to certain roles, and the last bug tasks for a bug cannot be deleted.
A change was made to the lazr-js activator css to ensure the edit icon lines up properly.
== Demo and QA ==
The delete icons are rendered as shown in the screenshot.
http://people.canonical.com/~ianb/bugtask-delete-icons.png
== Tests ==
Add tests to browser/tests/test_bugtask:
TestBugTaskDeleteLinks
(test that the delete links/icons are rendered as expected)
TestBugTaskDeleteView
(test that the delete view itself works as expected)
== Lint ==
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/app/javascript/activator/assets/skins/sam/activator-skin.css
lib/lp/bugs/configure.zcml
lib/lp/bugs/security.py
lib/lp/bugs/browser/bugtask.py
lib/lp/bugs/browser/configure.zcml
lib/lp/bugs/browser/tests/test_bugtask.py
lib/lp/bugs/interfaces/bugtask.py
lib/lp/bugs/model/bugtask.py
lib/lp/bugs/templates/bugtask-delete.pt
lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt
--
https://code.launchpad.net/~wallyworld/launchpad/delete-bugtask-ui-878909/+merge/80179
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/delete-bugtask-ui-878909 into lp:launchpad.
=== modified file 'lib/lp/app/javascript/activator/assets/skins/sam/activator-skin.css'
--- lib/lp/app/javascript/activator/assets/skins/sam/activator-skin.css 2011-06-29 14:56:15 +0000
+++ lib/lp/app/javascript/activator/assets/skins/sam/activator-skin.css 2011-10-25 08:17:26 +0000
@@ -2,6 +2,7 @@
.yui3-skin-sam button.yui3-activator-act {
background: url('edit.png') 0 0 no-repeat;
+ vertical-align: middle;
}
.yui3-skin-sam .yui3-activator-processing button.yui3-activator-act {
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2011-10-19 14:54:07 +0000
+++ lib/lp/bugs/browser/bugtask.py 2011-10-25 08:17:26 +0000
@@ -18,6 +18,7 @@
'BugTaskBreadcrumb',
'BugTaskContextMenu',
'BugTaskCreateQuestionView',
+ 'BugTaskDeletionView',
'BugTaskEditView',
'BugTaskExpirableListingView',
'BugTaskListingItem',
@@ -1757,6 +1758,38 @@
self.updateContextFromData(data)
+class BugTaskDeletionView(LaunchpadFormView):
+ """Used to delete a bugtask."""
+
+ schema = IBugTask
+ field_names = []
+
+ label = 'Remove bug task'
+ page_title = label
+
+ @property
+ def confirmation_message(self):
+ return ('<p>You are about to mark bug %s<br>'
+ 'as no longer affecting %s.</p>'
+ '<p>This operation will be permanent and cannot be '
+ 'undone.</p>'
+ % (self.context.bug.title,
+ self.context.target.bugtargetdisplayname))
+
+ @action('Delete', name='delete_bugtask')
+ def delete_bugtask_action(self, action, data):
+ bugtask = self.context
+ self.next_url = canonical_url(bugtask.bug)
+ message = ("This bug no longer affects %s."
+ % bugtask.target.bugtargetdisplayname)
+ bugtask.delete()
+ self.request.response.addNotification(message)
+
+ @property
+ def cancel_url(self):
+ return canonical_url(self.context)
+
+
class BugTaskListingView(LaunchpadView):
"""A view designed for displaying bug tasks in lists."""
# Note that this right now is only used in tests and to render
@@ -3552,6 +3585,8 @@
row_css_class='highlight' if is_primary else None,
target_link=canonical_url(self.context.target),
target_link_title=self.target_link_title,
+ user_can_delete=self.user_can_delete_bugtask,
+ delete_link=link + '/+delete',
user_can_edit_importance=self.context.userCanEditImportance(
self.user),
importance_css_class='importance' + self.context.importance.name,
@@ -3701,6 +3736,15 @@
"""
return self.context.userCanEditMilestone(self.user)
+ @cachedproperty
+ def user_can_delete_bugtask(self):
+ """Can the user delete the bug task?
+
+ If yes, return True, otherwise return False.
+ """
+ bugtask = self.context
+ return bugtask.userCanDelete(self.user) and bugtask.canBeDeleted()
+
@property
def style_for_add_milestone(self):
if self.context.milestone is None:
=== modified file 'lib/lp/bugs/browser/configure.zcml'
--- lib/lp/bugs/browser/configure.zcml 2011-10-03 07:49:31 +0000
+++ lib/lp/bugs/browser/configure.zcml 2011-10-25 08:17:26 +0000
@@ -589,6 +589,12 @@
template="../templates/bug-edit.pt"
permission="launchpad.Edit"/>
<browser:page
+ name="+delete"
+ for="lp.bugs.interfaces.bugtask.IBugTask"
+ class="lp.bugs.browser.bugtask.BugTaskDeletionView"
+ template="../templates/bugtask-delete.pt"
+ permission="launchpad.Delete"/>
+ <browser:page
name="+secrecy"
for="lp.bugs.interfaces.bugtask.IBugTask"
class="lp.bugs.browser.bug.BugSecrecyEditView"
=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py 2011-10-05 18:02:45 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py 2011-10-25 08:17:26 +0000
@@ -9,7 +9,10 @@
from lazr.lifecycle.snapshot import Snapshot
from pytz import UTC
from storm.store import Store
-from testtools.matchers import LessThan
+from testtools.matchers import (
+ LessThan,
+ MatchesRegex,
+ )
import transaction
from zope.component import (
getMultiAdapter,
@@ -51,10 +54,12 @@
FeatureFlag,
getFeatureStore,
)
+from lp.services.features.testing import FeatureFixture
from lp.services.propertycache import get_property_cache
from lp.soyuz.interfaces.component import IComponentSet
from lp.testing import (
celebrity_logged_in,
+ normalize_whitespace,
person_logged_in,
TestCaseWithFactory,
)
@@ -71,6 +76,9 @@
from lp.testing.views import create_initialized_view
+DELETE_BUGTASK_ENABLED = {u"disclosure.delete_bugtask.enabled": u"on"}
+
+
class TestBugTaskView(TestCaseWithFactory):
layer = LaunchpadFunctionalLayer
@@ -606,6 +614,99 @@
self.assertIn(series.product.displayname, content)
+class TestBugTaskDeleteLinks(TestCaseWithFactory):
+ """ Test that the delete icons/links for each relevant bug task are
+ correctly rendered.
+
+ Bug task deletion is protected by a feature flag.
+ """
+
+ layer = DatabaseFunctionalLayer
+
+ def test_cannot_delete_only_bugtask(self):
+ # The last bugtask cannot be deleted.
+ bug = self.factory.makeBug()
+ login_person(bug.owner)
+ view = create_initialized_view(
+ bug, name='+bugtasks-and-nominations-table')
+ row_view = view._getTableRowView(bug.default_bugtask, False, False)
+ self.assertFalse(row_view.user_can_delete_bugtask)
+ del get_property_cache(row_view).user_can_delete_bugtask
+ with FeatureFixture(DELETE_BUGTASK_ENABLED):
+ self.assertFalse(row_view.user_can_delete_bugtask)
+
+ def test_can_delete_bugtask_if_authorised(self):
+ # The bugtask can be deleted if the user if authorised.
+ bug = self.factory.makeBug()
+ bugtask = self.factory.makeBugTask(bug=bug)
+ login_person(bugtask.owner)
+ view = create_initialized_view(
+ bug, name='+bugtasks-and-nominations-table')
+ row_view = view._getTableRowView(bugtask, False, False)
+ self.assertFalse(row_view.user_can_delete_bugtask)
+ del get_property_cache(row_view).user_can_delete_bugtask
+ with FeatureFixture(DELETE_BUGTASK_ENABLED):
+ self.assertTrue(row_view.user_can_delete_bugtask)
+
+ def test_bugtask_delete_icon(self):
+ # The bugtask delete icon is rendered correctly for those tasks the
+ # user is allowed to delete.
+ bug = self.factory.makeBug()
+ bugtask = self.factory.makeBugTask(bug=bug)
+ with FeatureFixture(DELETE_BUGTASK_ENABLED):
+ login_person(bugtask.owner)
+ url = canonical_url(bugtask, rootsite='bugs')
+ browser = self.getUserBrowser(url, user=bugtask.owner)
+ # bugtask can be deleted because the user owns it.
+ delete_icon = find_tag_by_id(
+ browser.contents, 'bugtask-delete-task%d' % bugtask.id)
+ self.assertEqual(url + '/+delete', delete_icon['href'])
+ # default_bugtask cannot be deleted.
+ delete_icon = find_tag_by_id(
+ browser.contents,
+ 'bugtask-delete-task%d' % bug.default_bugtask.id)
+ self.assertIsNone(delete_icon)
+
+
+class TestBugTaskDeleteView(TestCaseWithFactory):
+ """Test the bug task delete form."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_delete_view_rendering(self):
+ # Test the view rendering, including confirmation message, cancel url.
+ bug = self.factory.makeBug()
+ bugtask = self.factory.makeBugTask(bug=bug)
+ with FeatureFixture(DELETE_BUGTASK_ENABLED):
+ login_person(bugtask.owner)
+ view = create_initialized_view(
+ bugtask, name='+delete', principal=bugtask.owner)
+ matches = MatchesRegex(
+ '.*You are about to mark bug.*'
+ 'This operation will be permanent and cannot be undone.*')
+ self.assertThat(normalize_whitespace(view.render()), matches)
+ url = canonical_url(bugtask, rootsite='bugs')
+ self.assertEqual(view.cancel_url, url)
+
+ def test_delete_action(self):
+ # Test that the delete action works as expected.
+ bug = self.factory.makeBug()
+ bugtask = self.factory.makeBugTask(bug=bug)
+ pillar_name = bugtask.pillar.displayname
+ with FeatureFixture(DELETE_BUGTASK_ENABLED):
+ login_person(bugtask.owner)
+ form = {
+ 'field.actions.delete_bugtask': 'Delete',
+ }
+ view = create_initialized_view(
+ bugtask, name='+delete', form=form, principal=bugtask.owner)
+ self.assertEqual([bug.default_bugtask], bug.bugtasks)
+ notifications = view.request.response.notifications
+ self.assertEqual(1, len(notifications))
+ expected = 'This bug no longer affects %s.' % pillar_name
+ self.assertEqual(expected, notifications[0].message)
+
+
class TestBugTaskEditViewStatusField(TestCaseWithFactory):
"""We show only those options as possible value in the status
field that the user can select.
=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml 2011-10-19 08:32:33 +0000
+++ lib/lp/bugs/configure.zcml 2011-10-25 08:17:26 +0000
@@ -194,10 +194,12 @@
task_age
bug_subscribers
is_complete
+ canBeDeleted
canTransitionToStatus
isSubscribed
getPackageComponent
maybeConfirm
+ userCanDelete
userCanEditImportance
userCanEditMilestone
userCanSetAnyAssignee
=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py 2011-10-19 07:13:40 +0000
+++ lib/lp/bugs/interfaces/bugtask.py 2011-10-25 08:17:26 +0000
@@ -706,6 +706,17 @@
"work required on this bug task."),
readonly=True))
+ def userCanDelete(user):
+ """Check that a user may delete a bugtask.
+
+ A user may delete a bugtask if:
+ - The disclosure.delete_bugtask.enabled feature flag is enabled,
+ and they are:
+ - project maintainer
+ - task creator
+ - bug supervisor
+ """
+
@operation_returns_collection_of(Interface) # Actually IBug.
@call_with(user=REQUEST_USER, limit=10)
@export_read_operation()
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-10-19 23:46:54 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-10-25 08:17:26 +0000
@@ -109,6 +109,7 @@
from lp.bugs.interfaces.bug import IBugSet
from lp.bugs.interfaces.bugattachment import BugAttachmentType
from lp.bugs.interfaces.bugnomination import BugNominationStatus
+from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
from lp.bugs.interfaces.bugtask import (
BUG_SUPERVISOR_BUGTASK_STATUSES,
BugBlueprintSearch,
@@ -155,6 +156,7 @@
from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.productseries import IProductSeries
from lp.registry.interfaces.projectgroup import IProjectGroup
+from lp.registry.interfaces.role import IHasOwner
from lp.registry.interfaces.sourcepackage import ISourcePackage
from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
from lp.registry.model.pillar import pillar_sort_key
@@ -630,6 +632,31 @@
return num_bugtasks > 1
+ def userCanDelete(self, user):
+ """See `IBugTask`."""
+ if user is None:
+ return False
+
+ # Admins can always delete bugtasks.
+ if user.inTeam(getUtility(ILaunchpadCelebrities).admin):
+ return True
+
+ delete_allowed = bool(features.getFeatureFlag(
+ 'disclosure.delete_bugtask.enabled'))
+ if not delete_allowed:
+ return False
+
+ owner = None
+ if IHasOwner.providedBy(self.pillar):
+ owner = self.pillar.owner
+ bugsupervisor = None
+ if IHasBugSupervisor.providedBy(self.pillar):
+ bugsupervisor = self.pillar.bug_supervisor
+ return (
+ user.inTeam(owner) or
+ user.inTeam(bugsupervisor) or
+ user.inTeam(self.owner))
+
def delete(self, who=None):
"""See `IBugTask`."""
if who is None:
=== modified file 'lib/lp/bugs/security.py'
--- lib/lp/bugs/security.py 2011-10-19 22:33:51 +0000
+++ lib/lp/bugs/security.py 2011-10-25 08:17:26 +0000
@@ -6,11 +6,8 @@
__metaclass__ = type
__all__ = []
-from zope.component import getUtility
-
from canonical.launchpad.interfaces.launchpad import IHasBug
from lp.services.messages.interfaces.message import IMessage
-from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.app.security import (
AnonymousAuthorization,
AuthorizationBase,
@@ -22,13 +19,10 @@
from lp.bugs.interfaces.bugnomination import IBugNomination
from lp.bugs.interfaces.bugsubscription import IBugSubscription
from lp.bugs.interfaces.bugsubscriptionfilter import IBugSubscriptionFilter
-from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
from lp.bugs.interfaces.bugtask import IBugTaskDelete
from lp.bugs.interfaces.bugtracker import IBugTracker
from lp.bugs.interfaces.bugwatch import IBugWatch
from lp.bugs.interfaces.structuralsubscription import IStructuralSubscription
-from lp.registry.interfaces.role import IHasOwner
-from lp.services.features import getFeatureFlag
class EditBugNominationStatus(AuthorizationBase):
@@ -59,38 +53,9 @@
usedfor = IBugTaskDelete
def checkAuthenticated(self, user):
- """Check that a user may delete a bugtask.
-
- A user may delete a bugtask if:
- - The disclosure.delete_bugtask.enabled feature flag is enabled,
- and they are:
- - project maintainer
- - task creator
- - bug supervisor
- """
- if user is None:
- return False
-
- # Admins can always delete bugtasks.
- if user.inTeam(getUtility(ILaunchpadCelebrities).admin):
- return True
-
- delete_allowed = bool(getFeatureFlag(
- 'disclosure.delete_bugtask.enabled'))
- if not delete_allowed:
- return False
-
+ """Check that a user may delete a bugtask."""
bugtask = self.obj
- owner = None
- if IHasOwner.providedBy(bugtask.pillar):
- owner = bugtask.pillar.owner
- bugsupervisor = None
- if IHasBugSupervisor.providedBy(bugtask.pillar):
- bugsupervisor = bugtask.pillar.bug_supervisor
- return (
- user.inTeam(owner) or
- user.inTeam(bugsupervisor) or
- user.inTeam(bugtask.owner))
+ return bugtask.userCanDelete(user)
class AdminDeleteBugTask(DeleteBugTask):
=== added file 'lib/lp/bugs/templates/bugtask-delete.pt'
--- lib/lp/bugs/templates/bugtask-delete.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/templates/bugtask-delete.pt 2011-10-25 08:17:26 +0000
@@ -0,0 +1,20 @@
+<html
+ xmlns="http://www.w3.org/1999/xhtml";
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+ metal:use-macro="view/macro:page/main_only"
+ i18n:domain="launchpad">
+<body>
+
+<div metal:fill-slot="main">
+ <div metal:use-macro="context/@@launchpad_form/form">
+ <div metal:fill-slot="extra_info">
+ <p tal:content="structure view/confirmation_message">
+ Are you sure?
+ </p>
+ </div>
+ </div>
+</div>
+</body>
+</html>
=== modified file 'lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt'
--- lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt 2011-08-19 08:55:04 +0000
+++ lib/lp/bugs/templates/bugtask-tasks-and-nominations-table-row.pt 2011-10-25 08:17:26 +0000
@@ -33,6 +33,12 @@
Edit
</button>
<div class="yui3-activator-message-box yui3-activator-hidden"></div>
+ <tal:delete-icon condition="data/user_can_delete">
+ <a tal:attributes="
+ id string:bugtask-delete-${data/form_row_id};
+ href data/delete_link"
+ class="sprite remove bugtask-delete" style="margin-left: 4px"></a>
+ </tal:delete-icon>
</span>
</td>
