launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #28104
[Merge] ~lgp171188/launchpad:add-ui-bug-lock-unlock into launchpad:master
Guruprasad has proposed merging ~lgp171188/launchpad:add-ui-bug-lock-unlock into launchpad:master.
Commit message:
Implement the UI for changing a bug's lock status and reason
And allow only the users with the 'launchpad.Moderate' permission to
be able to access the page and change the lock status and reason.
Also display a read-only icon for the locked bugs.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/415453
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:add-ui-bug-lock-unlock into launchpad:master.
diff --git a/lib/canonical/launchpad/icing/css/typography.scss b/lib/canonical/launchpad/icing/css/typography.scss
index 1bdd3a2..1e58f29 100644
--- a/lib/canonical/launchpad/icing/css/typography.scss
+++ b/lib/canonical/launchpad/icing/css/typography.scss
@@ -58,6 +58,15 @@ h1, h2, h3, h4, h5, h6 {
max-width: 100%;
}
+ .inline-block {
+ display: inline-block;
+ }
+
+ .badge.read-only {
+ width: 20px;
+ height: 16px;
+ }
+
table.wide {
width: $wider-page;
}
diff --git a/lib/lp/bugs/browser/bug.py b/lib/lp/bugs/browser/bug.py
index b67c65f..ef8b6c1 100644
--- a/lib/lp/bugs/browser/bug.py
+++ b/lib/lp/bugs/browser/bug.py
@@ -8,6 +8,7 @@ __all__ = [
'BugContextMenu',
'BugEditView',
'BugInformationTypePortletView',
+ 'BugLockStatusEditView',
'BugMarkAsAffectingUserView',
'BugMarkAsDuplicateView',
'BugNavigation',
@@ -76,7 +77,10 @@ from lp.app.widgets.product import GhostCheckBoxWidget
from lp.app.widgets.project import ProjectScopeWidget
from lp.bugs.browser.bugsubscription import BugPortletSubscribersWithDetails
from lp.bugs.browser.widgets.bug import BugTagsWidget
-from lp.bugs.enums import BugNotificationLevel
+from lp.bugs.enums import (
+ BugLockStatus,
+ BugNotificationLevel,
+ )
from lp.bugs.interfaces.bug import (
IBug,
IBugSet,
@@ -218,7 +222,7 @@ class BugContextMenu(ContextMenu):
'adddistro', 'subscription', 'addsubscriber', 'editsubscriptions',
'addcomment', 'nominate', 'addbranch', 'linktocve', 'unlinkcve',
'createquestion', 'mute_subscription', 'removequestion',
- 'activitylog', 'affectsmetoo']
+ 'activitylog', 'affectsmetoo', 'change_lock_status']
def __init__(self, context):
# Always force the context to be the current bugtask, so that we don't
@@ -238,6 +242,12 @@ class BugContextMenu(ContextMenu):
text = 'Change privacy/security'
return Link('+secrecy', text)
+ @enabled_with_permission('launchpad.Moderate')
+ def change_lock_status(self):
+ """Return the 'Change lock status' Link."""
+ text = 'Change lock status'
+ return Link('+lock-status', text, icon='edit')
+
@enabled_with_permission('launchpad.Edit')
def markduplicate(self):
"""Return the 'Mark as duplicate' Link."""
@@ -766,6 +776,73 @@ class BugEditView(BugEditViewBase):
self.updateBugFromData(data)
+class BugLockStatusEditView(LaunchpadEditFormView):
+ """The view for editing the bug lock status and lock reason."""
+
+ class schema(Interface):
+ # Duplicating the fields is necessary because these fields are
+ # read-only in `IBug`.
+ lock_status = copy_field(IBug['lock_status'], readonly=False)
+ lock_reason = copy_field(IBug['lock_reason'], readonly=False)
+
+ @property
+ def adapters(self):
+ """See `LaunchpadFormView`."""
+ return {self.schema: self.context.bug}
+
+ field_names = ['lock_status', 'lock_reason']
+
+ custom_widget_lock_status = LaunchpadRadioWidgetWithDescription
+ custom_widget_lock_reason = CustomWidgetFactory(
+ TextWidget,
+ displayWidth=30
+ )
+
+ def initialize(self):
+ super().initialize()
+ lock_status_widget = self.widgets['lock_status']
+ lock_status_widget._displayItemForMissingValue = False
+
+ @property
+ def label(self):
+ """The form label."""
+ return (
+ "Edit the lock status and reason for bug #%d" % self.context.bug.id
+ )
+
+ page_title = label
+
+ @action('Change', name='change')
+ def change_action(self, action, data):
+ """Update the bug lock status and reason with submitted changes."""
+ bug = self.context.bug
+ if bug.lock_status != data['lock_status']:
+ locked_states = (
+ BugLockStatus.COMMENT_ONLY,
+ )
+ if data['lock_status'] in locked_states:
+ bug.lock(
+ status=data['lock_status'],
+ reason=data['lock_reason'] or '',
+ who=self.user
+ )
+ else:
+ bug.unlock(who=self.user)
+
+ elif (bug.lock_status != BugLockStatus.UNLOCKED and
+ bug.lock_reason != data['lock_reason']):
+ bug.setLockReason(data['lock_reason'], who=self.user)
+
+ @property
+ def next_url(self):
+ """Return the next URL to call when this call completes."""
+ if not self.request.is_ajax:
+ return canonical_url(self.context)
+ return None
+
+ cancel_url = next_url
+
+
class BugMarkAsDuplicateView(BugEditViewBase):
"""Page for marking a bug as a duplicate."""
diff --git a/lib/lp/bugs/browser/configure.zcml b/lib/lp/bugs/browser/configure.zcml
index 85e1340..d6a365e 100644
--- a/lib/lp/bugs/browser/configure.zcml
+++ b/lib/lp/bugs/browser/configure.zcml
@@ -507,6 +507,12 @@
template="../../app/templates/generic-edit.pt"
permission="launchpad.Edit"/>
<browser:page
+ name="+lock-status"
+ for="lp.bugs.interfaces.bug.IBugTask"
+ class="lp.bugs.browser.bug.BugLockStatusEditView"
+ template="../../app/templates/generic-edit.pt"
+ permission="launchpad.Moderate"/>
+ <browser:page
name="+delete"
for="lp.bugs.interfaces.bugtask.IBugTask"
class="lp.bugs.browser.bugtask.BugTaskDeletionView"
diff --git a/lib/lp/bugs/browser/tests/test_edit_bug_lock_status.py b/lib/lp/bugs/browser/tests/test_edit_bug_lock_status.py
new file mode 100644
index 0000000..2df1f80
--- /dev/null
+++ b/lib/lp/bugs/browser/tests/test_edit_bug_lock_status.py
@@ -0,0 +1,414 @@
+# Copyright 2011-2021 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests related to the view for editing the bug lock status."""
+
+from soupmatchers import (
+ HTMLContains,
+ Tag,
+ )
+from testtools.matchers import (
+ MatchesStructure,
+ Not,
+ )
+from zope.security.interfaces import Unauthorized
+
+from lp.bugs.enums import BugLockStatus
+from lp.services.webapp import canonical_url
+from lp.services.webapp.servers import LaunchpadTestRequest
+from lp.testing import (
+ anonymous_logged_in,
+ BrowserTestCase,
+ person_logged_in,
+ TestCaseWithFactory,
+ )
+from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.views import create_initialized_view
+
+
+class TestBugLockStatusEditView(TestCaseWithFactory):
+ """
+ Tests for the view to edit the bug lock status.
+ """
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super().setUp()
+ self.person = self.factory.makePerson()
+ self.target = self.factory.makeProduct()
+
+ def test_form_submission_missing_required_fields(self):
+ bug = self.factory.makeBug(target=self.target)
+ form = {
+ 'a': 1,
+ 'b': 2,
+ }
+ with person_logged_in(self.target.owner):
+ request = LaunchpadTestRequest(
+ method='POST',
+ form=form,
+ )
+ view = create_initialized_view(
+ bug.default_bugtask,
+ name='+lock-status',
+ request=request
+ )
+ self.assertEqual([], view.errors)
+
+ self.assertEqual(BugLockStatus.UNLOCKED, bug.lock_status)
+ self.assertIsNone(bug.lock_reason)
+
+ def test_users_without_moderate_permission_cannot_edit_lock_status(self):
+ bug = self.factory.makeBug(target=self.target)
+ with person_logged_in(self.target.owner):
+ bug.lock(who=self.target.owner, status=BugLockStatus.COMMENT_ONLY)
+
+ form = {
+ "field.actions.change": "Change",
+ "field.lock_status": "Unlocked",
+ "field.lock_reason": "",
+ "field.lock_status-empty-marker": "1",
+ }
+
+ with anonymous_logged_in():
+ self.assertRaises(
+ Unauthorized, create_initialized_view,
+ bug.default_bugtask, name='+lock-status',
+ form=form
+ )
+
+ with person_logged_in(self.person):
+ self.assertRaises(
+ Unauthorized, create_initialized_view,
+ bug.default_bugtask, name='+lock-status',
+ form=form
+ )
+
+ def test_locking_a_locked_bug(self):
+ bug = self.factory.makeBug(target=self.target)
+ with person_logged_in(self.target.owner):
+ bug.lock(who=self.target.owner, status=BugLockStatus.COMMENT_ONLY)
+
+ self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status)
+ self.assertIsNone(bug.lock_reason)
+
+ form = {
+ "field.actions.change": "Change",
+ "field.lock_status": "Comment-only",
+ "field.lock_reason": "",
+ "field.lock_status-empty-marker": "1",
+ }
+ with person_logged_in(self.target.owner):
+ request = LaunchpadTestRequest(
+ method='POST',
+ form=form,
+ )
+ view = create_initialized_view(
+ bug.default_bugtask,
+ name='+lock-status',
+ request=request
+ )
+ self.assertEqual([], view.errors)
+
+ self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status)
+ self.assertIsNone(bug.lock_reason)
+
+ def test_unlocking_an_unlocked_bug(self):
+ bug = self.factory.makeBug(target=self.target)
+
+ self.assertEqual(BugLockStatus.UNLOCKED, bug.lock_status)
+ self.assertIsNone(bug.lock_reason)
+
+ form = {
+ "field.actions.change": "Change",
+ "field.lock_status": "Unlocked",
+ "field.lock_reason": "too hot",
+ "field.lock_status-empty-marker": "1",
+ }
+ with person_logged_in(self.target.owner):
+ request = LaunchpadTestRequest(
+ method='POST',
+ form=form,
+ )
+ view = create_initialized_view(
+ bug.default_bugtask,
+ name='+lock-status',
+ request=request
+ )
+ self.assertEqual([], view.errors)
+
+ self.assertEqual(BugLockStatus.UNLOCKED, bug.lock_status)
+ self.assertIsNone(bug.lock_reason)
+
+ def test_unlocking_a_bug_locked_with_reason_clears_the_reason(self):
+ bug = self.factory.makeBug(target=self.target)
+
+ with person_logged_in(self.target.owner):
+ bug.lock(
+ who=self.target.owner,
+ status=BugLockStatus.COMMENT_ONLY,
+ reason='too hot'
+ )
+ self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status)
+ self.assertEqual('too hot', bug.lock_reason)
+
+ form = {
+ "field.actions.change": "Change",
+ "field.lock_status": "Unlocked",
+ "field.lock_reason": "too hot!",
+ "field.lock_status-empty-marker": "1",
+ }
+ with person_logged_in(self.target.owner):
+ request = LaunchpadTestRequest(
+ method='POST',
+ form=form,
+ )
+ view = create_initialized_view(
+ bug.default_bugtask,
+ name='+lock-status',
+ request=request
+ )
+ self.assertEqual([], view.errors)
+
+ self.assertEqual(BugLockStatus.UNLOCKED, bug.lock_status)
+ self.assertIsNone(bug.lock_reason)
+
+ def test_locking_an_unlocked_bug(self):
+ bug = self.factory.makeBug(target=self.target)
+
+ self.assertEqual(BugLockStatus.UNLOCKED, bug.lock_status)
+ self.assertIsNone(bug.lock_reason)
+ self.assertEqual(1, bug.activity.count())
+
+ form = {
+ "field.actions.change": "Change",
+ "field.lock_status": "Comment-only",
+ "field.lock_reason": "too hot",
+ "field.lock_status-empty-marker": "1",
+ }
+ with person_logged_in(self.target.owner):
+ request = LaunchpadTestRequest(
+ method='POST',
+ form=form,
+ )
+ view = create_initialized_view(
+ bug.default_bugtask,
+ name='+lock-status',
+ request=request
+ )
+ self.assertEqual([], view.errors)
+
+ self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status)
+ self.assertEqual('too hot', bug.lock_reason)
+ self.assertEqual(2, bug.activity.count())
+ self.assertThat(
+ bug.activity[1],
+ MatchesStructure.byEquality(
+ person=self.target.owner,
+ whatchanged='lock status',
+ oldvalue=str(BugLockStatus.UNLOCKED),
+ newvalue=str(BugLockStatus.COMMENT_ONLY),
+ )
+ )
+
+ def test_unlocking_a_locked_bug(self):
+ bug = self.factory.makeBug(target=self.target)
+ self.assertEqual(1, bug.activity.count())
+
+ with person_logged_in(self.target.owner):
+ bug.lock(
+ who=self.target.owner,
+ status=BugLockStatus.COMMENT_ONLY,
+ reason='too hot'
+ )
+ self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status)
+ self.assertEqual('too hot', bug.lock_reason)
+ self.assertEqual(2, bug.activity.count())
+
+ form = {
+ "field.actions.change": "Change",
+ "field.lock_status": "Unlocked",
+ "field.lock_reason": "too hot!!",
+ "field.lock_status-empty-marker": "1",
+ }
+ with person_logged_in(self.target.owner):
+ request = LaunchpadTestRequest(
+ method='POST',
+ form=form,
+ )
+ view = create_initialized_view(
+ bug.default_bugtask,
+ name='+lock-status',
+ request=request
+ )
+ self.assertEqual([], view.errors)
+
+ self.assertEqual(BugLockStatus.UNLOCKED, bug.lock_status)
+ self.assertIsNone(bug.lock_reason)
+ self.assertEqual(3, bug.activity.count())
+ self.assertThat(
+ bug.activity[2],
+ MatchesStructure.byEquality(
+ person=self.target.owner,
+ whatchanged='lock status',
+ oldvalue=str(BugLockStatus.COMMENT_ONLY),
+ newvalue=str(BugLockStatus.UNLOCKED),
+ )
+ )
+
+ def test_changing_lock_reason_of_a_locked_bug(self):
+ bug = self.factory.makeBug(target=self.target)
+ self.assertEqual(1, bug.activity.count())
+
+ with person_logged_in(self.target.owner):
+ bug.lock(
+ who=self.target.owner,
+ status=BugLockStatus.COMMENT_ONLY,
+ reason='too hot'
+ )
+ self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status)
+ self.assertEqual('too hot', bug.lock_reason)
+ self.assertEqual(2, bug.activity.count())
+
+ form = {
+ "field.actions.change": "Change",
+ "field.lock_status": "Comment-only",
+ "field.lock_reason": "too hot!",
+ "field.lock_status-empty-marker": "1",
+ }
+ with person_logged_in(self.target.owner):
+ request = LaunchpadTestRequest(
+ method='POST',
+ form=form,
+ )
+ view = create_initialized_view(
+ bug.default_bugtask,
+ name='+lock-status',
+ request=request
+ )
+ self.assertEqual([], view.errors)
+
+ self.assertEqual(BugLockStatus.COMMENT_ONLY, bug.lock_status)
+ self.assertEqual('too hot!', bug.lock_reason)
+ self.assertEqual(3, bug.activity.count())
+ self.assertThat(
+ bug.activity[2],
+ MatchesStructure.byEquality(
+ person=self.target.owner,
+ whatchanged='lock reason',
+ oldvalue='too hot',
+ newvalue='too hot!',
+ )
+ )
+
+class TestBugLockFeatures(BrowserTestCase):
+ """Test for the features related to the locking, unlocking a bug."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super().setUp()
+ self.person = self.factory.makePerson()
+ self.target = self.factory.makeProduct()
+
+ def test_bug_lock_status_page_not_linked_for_non_moderators(self):
+ bug = self.factory.makeBug(target=self.target)
+ bugtask_url = canonical_url(bug.default_bugtask)
+ browser = self.getUserBrowser(
+ bugtask_url,
+ user=self.person,
+ )
+ self.assertThat(
+ browser.contents,
+ Not(
+ HTMLContains(
+ Tag(
+ 'change lock status link tag',
+ 'a',
+ text='Change lock status',
+ attrs={
+ 'class': "edit",
+ 'href': '{}/+lock-status'.format(bugtask_url),
+ }
+ )
+ )
+ )
+ )
+
+ def test_bug_lock_status_page_linked_for_moderators(self):
+ bug = self.factory.makeBug(target=self.target)
+ bugtask_url = canonical_url(bug.default_bugtask)
+
+ browser = self.getUserBrowser(
+ bugtask_url,
+ user=self.target.owner,
+ )
+ self.assertThat(
+ browser.contents,
+ HTMLContains(
+ Tag(
+ 'change lock status link tag',
+ 'a',
+ text='Change lock status',
+ attrs={
+ 'class': "edit",
+ 'href': '{}/+lock-status'.format(bugtask_url),
+ }
+ )
+ )
+ )
+
+ def test_bug_readonly_icon_displayed_when_bug_is_locked(self):
+ bug = self.factory.makeBug(target=self.target)
+ with person_logged_in(self.target.owner):
+ bug.lock(
+ who=self.target.owner,
+ status=BugLockStatus.COMMENT_ONLY,
+ reason='too hot'
+ )
+
+ bugtask_url = canonical_url(bug.default_bugtask)
+
+ browser = self.getUserBrowser(
+ bugtask_url,
+ user=self.target.owner,
+ )
+ self.assertThat(
+ browser.contents,
+ HTMLContains(
+ Tag(
+ 'read-only icon tag',
+ 'span',
+ attrs={
+ 'class': 'read-only',
+ 'title': 'Locked'
+ }
+ )
+ )
+ )
+
+ def test_bug_readonly_icon_not_displayed_when_bug_is_unlocked(self):
+ bug = self.factory.makeBug(target=self.target)
+
+ bugtask_url = canonical_url(bug.default_bugtask)
+
+ browser = self.getUserBrowser(
+ bugtask_url,
+ user=self.target.owner,
+ )
+ self.assertThat(
+ browser.contents,
+ Not(
+ HTMLContains(
+ Tag(
+ 'read-only icon tag',
+ 'span',
+ attrs={
+ 'class': 'read-only',
+ 'title': 'Locked'
+ }
+ )
+ )
+ )
+ )
diff --git a/lib/lp/bugs/interfaces/bug.py b/lib/lp/bugs/interfaces/bug.py
index 7459bd5..100396e 100644
--- a/lib/lp/bugs/interfaces/bug.py
+++ b/lib/lp/bugs/interfaces/bug.py
@@ -387,6 +387,14 @@ class IBugView(Interface):
readonly=True,
value_type=Reference(schema=IMessage)),
exported_as='messages'))
+ locked = exported(
+ Bool(
+ title=_('Locked?'),
+ description=_('Is this bug locked?'),
+ readonly=True
+ ),
+ as_of="devel"
+ )
lock_status = exported(
Choice(
title=_('Lock Status'), vocabulary=BugLockStatus,
diff --git a/lib/lp/bugs/model/bug.py b/lib/lp/bugs/model/bug.py
index f663baa..7299cd6 100644
--- a/lib/lp/bugs/model/bug.py
+++ b/lib/lp/bugs/model/bug.py
@@ -107,6 +107,7 @@ from lp.bugs.adapters.bugchange import (
UnsubscribedFromBug,
)
from lp.bugs.enums import (
+ BugLockedStatus,
BugLockStatus,
BugNotificationLevel,
)
@@ -414,6 +415,28 @@ class Bug(SQLBase, InformationTypeMixin):
lock_reason = StringCol(notNull=False, default=None)
@property
+<<<<<<< lib/lp/bugs/model/bug.py
+=======
+ def locked(self):
+ try:
+ BugLockedStatus.items[self._lock_status.value]
+ return True
+ except KeyError:
+ return False
+
+ @property
+ def lock_status(self):
+ return (
+ BugLockStatus.UNLOCKED if self._lock_status is None
+ else self._lock_status
+ )
+
+ @lock_status.setter
+ def lock_status(self, value):
+ self._lock_status = value
+
+ @property
+>>>>>>> lib/lp/bugs/model/bug.py
def linked_branches(self):
return [link.branch for link in self.linked_bugbranches]
@@ -2252,8 +2275,8 @@ class Bug(SQLBase, InformationTypeMixin):
"""See `IBug`."""
if self.lock_status != BugLockStatus.UNLOCKED:
old_lock_status = self.lock_status
- self.lock_status = BugLockStatus.UNLOCKED
self.lock_reason = None
+ self.lock_status = BugLockStatus.UNLOCKED
self.addChange(
BugUnlocked(
diff --git a/lib/lp/bugs/security.py b/lib/lp/bugs/security.py
index 57ad9fa..5e40c90 100644
--- a/lib/lp/bugs/security.py
+++ b/lib/lp/bugs/security.py
@@ -229,6 +229,21 @@ class ModerateBug(AuthorizationBase):
)
+class ModerateBugTask(DelegatedAuthorization):
+ """
+ Security adapter for moderating bug tasks.
+
+ This has the same semantics as `ModerateBug`, but can be used where
+ the context is a bug task rather than a bug.
+ """
+
+ permission = 'launchpad.Moderate'
+ usedfor = IHasBug
+
+ def __init__(self, obj):
+ super().__init__(obj, obj.bug)
+
+
class PublicToAllOrPrivateToExplicitSubscribersForBug(AuthorizationBase):
permission = 'launchpad.View'
usedfor = IBug
diff --git a/lib/lp/bugs/templates/bug-portlet-actions.pt b/lib/lp/bugs/templates/bug-portlet-actions.pt
index c2762fd..f6b9b24 100644
--- a/lib/lp/bugs/templates/bug-portlet-actions.pt
+++ b/lib/lp/bugs/templates/bug-portlet-actions.pt
@@ -81,4 +81,11 @@
tal:condition="link/enabled"
tal:content="structure link/render" />
</ul>
+ <ul id="lock-status-actions">
+ <li
+ tal:define="link context_menu/change_lock_status"
+ tal:condition="python: link.enabled"
+ tal:content="structure context_menu/change_lock_status/render"
+ />
+ </ul>
</div>
diff --git a/lib/lp/bugs/templates/bugtask-index.pt b/lib/lp/bugs/templates/bugtask-index.pt
index 287bb85..ac2d1ba 100644
--- a/lib/lp/bugs/templates/bugtask-index.pt
+++ b/lib/lp/bugs/templates/bugtask-index.pt
@@ -76,6 +76,7 @@
<tal:reporter replace="structure context/bug/owner/fmt:link" />
<tal:created
replace="structure context/bug/datecreated/fmt:displaydatetitle" />
+ <span class="badge read-only inline-block" title="Locked" tal:condition="context/bug/locked"></span>
</tal:registering>
<metal:heading fill-slot="heading" tal:define="context_menu context/menu:context">
Follow ups
