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">
_______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : launchpad-reviewers@lists.launchpad.net Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp