launchpad-reviewers team mailing list archive

[Colin Watson <mp+414097@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/bugs/enums.py b/lib/lp/bugs/enums.py
> index 6290763..3a0a09d 100644
> --- a/lib/lp/bugs/enums.py
> +++ b/lib/lp/bugs/enums.py
> @@ -74,3 +74,24 @@ class BugNotificationStatus(DBEnumeratedType):
>          The notification is deferred.  The recipient list was not calculated
>          at creation time but is done when processed.
>          """)
> +
> +
> +class BugLockStatus(DBEnumeratedType):
> +    """
> +    The lock status of a bug.
> +
> +    The lock status may be unlocked, comment-only.

No need for the second paragraph here; it duplicates the list of items, and it'll just be more work to keep up to date.

> +    """
> +
> +    UNLOCKED = DBItem(0, """
> +        Unlocked
> +
> +        The bug is unlocked and the usual permissions apply.
> +        """)
> +
> +    COMMENT_ONLY = DBItem(1, """
> +        Comment-only
> +
> +        The bug allows those without a bug role or an admin role
> +        to comment.

This seems to have picked up an extra negation by accident, and I don't think we need to spell out the admin behaviour here.  How about:

  The bug allows only those with a relevant role on the bug to comment.

> +        """)
> diff --git a/lib/lp/bugs/interfaces/bug.py b/lib/lp/bugs/interfaces/bug.py
> index 057b9eb..44f0493 100644
> --- a/lib/lp/bugs/interfaces/bug.py
> +++ b/lib/lp/bugs/interfaces/bug.py
> @@ -382,6 +385,16 @@ class IBugView(Interface):
>              readonly=True,
>              value_type=Reference(schema=IMessage)),
>          exported_as='messages'))
> +    lock_status = exported(
> +        Choice(
> +            title=_('Lock Status'), vocabulary=BugLockStatus,
> +            default=BugLockStatus.UNLOCKED,
> +            description=_("The lock status of this bug.")))

This should have an explicit `required=False`.

> +    lock_reason = exported(
> +        Text(
> +            title=_('Lock Reason'),
> +            required=False,
> +            description=_('The reason for locking this bug.')))

Could you add `as_of='devel'` to both `lock_status` and `lock_reason` so that it only appears in the devel API version?  (Note that this is an argument to `exported`, not an argument to `Choice` or `Text`.)

>  
>      def getSpecifications(user):
>          """List of related specifications that the user can view."""
> diff --git a/lib/lp/bugs/model/bug.py b/lib/lp/bugs/model/bug.py
> index 0a3ff8c..0575305 100644
> --- a/lib/lp/bugs/model/bug.py
> +++ b/lib/lp/bugs/model/bug.py
> @@ -387,6 +390,9 @@ class Bug(SQLBase, InformationTypeMixin):
>      heat = IntCol(notNull=True, default=0)
>      heat_last_updated = UtcDateTimeCol(default=None)
>      latest_patch_uploaded = UtcDateTimeCol(default=None)
> +    lock_status = DBEnum(
> +        enum=BugLockStatus, allow_none=False, default=BugLockStatus.UNLOCKED)

Should be `allow_none=True`.  (Yes, the `notNull` argument used for old-style SQLObject columns has the opposite sense, annoyingly.)

> +    lock_reason = StringCol(notNull=False, default=None)
>  
>      @property
>      def linked_branches(self):


-- 
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/414097
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:enforce-stricter-permission-check-when-a-bug-is-locked-down into launchpad:master.