1240839 - Add a "locked" attribute to flagtypes, to be able to prevent them from being cleared

Status: ASSIGNED

(Bugzilla :: Administration, task)

Description

[Mahdi Mokhtari]

At FreeBSD project, we are seeing a need that maybe lot of Bugzilla users need that too.
So we preferred to share our ideas (and helps :D) with Bugzilla-dev team.
we need an attribute for flag-types that ensures a flag always has its value once it have been set.
And prevent flags value to be emptied while it has a correct one.

We have filed a bug (opened issue) in our Bugzilla installation (bugs.freebsd.org) with more explanations (if/sure you need more) [link is addressed in See Also field].

Comment 1

[Frédéric Buclin]

From the discussion on IRC, the request is to be able to prevent a flag from being cleared, independently of the user privs. So it's a all or nothing bit (aka boolean).

(My personal opinion is that I don't see the point to enforce this for flags in the "?" state, especially for obsoleted attachments and/or for users in the grant group.)

Comment 2

[Mahdi Mokhtari]

for clarifying my first comment, i refer to 'SeeAlso' (explanations on FreeBSD-Bugzilla).
in brief i can summarize that:
as you know, "Bugzilla flags are a custom 4-state attribute of bugs and/or attachments. These states are: granted (+), denied (-), requested (?) and undefined (not set)"

this attribute we are gonna develop for Bugzilla is to not let a flag become "undefined" once that flag become "defined".
so changes between "defined" values (+ - ?) is still OK.

Comment 3

[Mahdi Mokhtari]

Attachment: Set of patches (patch-per-file) for adding is_removable attribute to bugzilla (pre-pre-alpha)

Comment 4

[Mahdi Mokhtari]

Attachment: patch adds is_removable attribute to bugzilla (pre-pre-alpha, generated with git diff)

Comment 5

[Mahdi Mokhtari]

Attachment: patch that adds is_removable attribute to bugzilla (still pre-alpha :D)

as LpSolit told:
1) all tabs replaced with spaces.
2) Shebang fixed (in FreeBSD shebang for perl defaults to #!/usr/local/bin/perl that affected my diffs, ive fixed it back to #!/usr/bin/perl to avoid conflicts) :D

Comment 6

[Frédéric Buclin]

Comment on attachment 8710098 [details] [diff] [review]
patch that adds is_removable attribute to bugzilla (still pre-alpha :D)

>+++ b/Bugzilla/Flag.pm
>     if (!grep($status eq $_ , qw(X + - ?))
>+        || ($status eq '?' && $self->status ne '?' && !$self->type->is_requestable) || ($self->status ne 'X' && !$self->type->is_removable))

This line is too long. Please do not exceed 80 characters (unless there is a good reason to bypass this limit). You must also update the comment in this subroutine to explain why this rule is there.
Moreover, the logic is wrong. You say that if the old status is not empty and the flag is not removable, an error should be thrown. What you want is that the new status cannot be empty if it's not removable. $self->status is the old status. $status is the new status.



>+++ b/Bugzilla/Install/DB.pm
>@@ -305,6 +305,7 @@ sub update_table_definitions {

>+    $dbh->bz_add_column('flagtypes', 'is_removable', {TYPE => 'BOOLEAN'});

This DB change doesn't match what is in Bugzilla/DB/Schema.pm. Moreover, in this file, DB changes must be listed chronologically. So this line must be right before this comment, around line 750:

    ################################################################
    # New --TABLE-- changes should go *** A B O V E *** this point #
    ################################################################



>+++ b/template/en/default/admin/flag-type/edit.html.tmpl

>+        <label for="is_removable">is_removable (flag can not be empty)</label>

is_removable is an internal ID for this field and should not be displayed to the user. A more user friendly label should be used.


I didn't test your patch, so I may have more comments in my next review. :)

Comment 7

[Mahdi Mokhtari]

Attachment: patch that adds is_removable attribute to bugzilla

This is new patch respecting (and solving) last problems Frédéric told.

Comment 8

[Mahdi Mokhtari]

(In reply to Frédéric Buclin from comment #6)

> I didn't test your patch, so I may have more comments in my next review. :)
Sure i'll be happy for each problem you tell as it makes new feature, better ;D

Comment 9

[Frédéric Buclin]

Comment on attachment 8714004 [details] [diff] [review]
patch that adds is_removable attribute to bugzilla

>+++ b/Bugzilla/Flag.pm
>+    # - Make sure the user didn't change none_is_removable flag from none-empty value.

Reword it to: Make sure the user didn't try to clear non deletable flags.


>+        || ($status ne 'X' && !$self->type->is_removable))

This logic is still wrong. You want exactly the opposite. Did you test your patch? :(

This makes me think that you must also hide the empty value from the UI for existing flags if the is_removable bit is false.



>+++ b/template/en/default/admin/flag-type/edit.html.tmpl

>+        <label for="is_removable">is removable (flag can be set to empty 'X' value)</label>

"deletable" is a better choice than "removable", IMO. Also, no need for the "is" prefix, for consistency with other checkboxes. The "X" value means nothing to the user; this is only an internal state used in the backend code. Simply write: deletable (the status of flags of this type can be cleared).

Comment 10

[Mahdi Mokhtari]

Attachment: patch adds removable attribute

Changes applied respecting to former LpSolit's comment
s/removable/deletable/g

Comment 11

[Frédéric Buclin]

Comment on attachment 8714099 [details] [diff] [review]
patch adds removable attribute


>+++ b/template/en/default/admin/flag-type/list.html.tmpl
>+          [% IF type.is_deletable %]
>+            <span class="deletable">deletable</span>
>+          [% END %]

Display "not deletable" instead. Because in 99% of cases, the flags will be deletable.


Also, you forgot to address my previous comment: "you must also hide the empty value from the UI for existing flags if the is_removable bit is false.", see flag/list.html.tmpl.

Comment 12

[Mahdi Mokhtari]

Attachment: patch adds removable attribute

corrected UI issue (and now it shows 'not deletable' in UI by default)

Comment 13

[Mahdi Mokhtari]

Attachment: patch adds removable attribute

editing last patch respecting LpSolit's comment "not show 'X' when not deletable"

Comment 14

[Mahdi Mokhtari]

Attachment: patch adds is_deletable attribute

adding POD docs for new methods.

Comment 15

[Frédéric Buclin]

Comment on attachment 8717076 [details] [diff] [review]
patch adds is_deletable attribute

>+++ b/Bugzilla/Flag.pm

>+        || ($status eq 'X' && $self->status ne 'X' && !$self->type->is_deletable))

No need for |$self->status ne 'X'| as the flag status can never be X.



>+++ b/Bugzilla/FlagType.pm

>+=item C<is_deletable>
>+
>+Returns whether you can set flag to undefined value ('X')
>+Once it set to a defined value or not.

Please reword it to: "Returns whether you can clear a flag once it is set."



>+++ b/Bugzilla/Install/DB.pm

>+    $dbh->bz_add_column('flagtypes', 'is_deletable',
>+                        {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'TRUE'});

Please add a comment right above it pointing to this bug. See how this is done for previous SQL instructions.



>+++ b/template/en/default/admin/flag-type/list.html.tmpl

>+          [% IF !type.is_deletable %]
>+            <span class="not_deletable">not deletable</span>
>+          [% END %]

You must set the 'not_deletable' class in skins/standard/admin.css, else "not deletable" won't be displayed on its own line.


Otherwise looks good and works fine. The comments above are trivial to address, so the next patch should be fine.

Comment 16

[Mahdi Mokhtari]

Attachment: patch adds is_deletable attribute

Rewording/Adding some comments to follow conventions.
Adding not_deletable to admin.css

Comment 17

[Frédéric Buclin]

Comment on attachment 8717853 [details] [diff] [review]
patch adds is_deletable attribute

While doing more testing, I found 2 bugs:

- When you include/exclude products and components in the Category section of editflagtypes.cgi, the "deletable" checkbox is always reset to 1. You must list is_deletable to the list of booleans around line 114.

- When using WebServices/REST, the is_deletable attribute cannot be set/updated, and it's not returned by get().

Comment 18

[Mahdi Mokhtari]

Attachment: patch adds is_deletable attribute

adds XMLRPC and JSONRPC and REST code for is_deletable attribute.
completing POD docs for this attribute.

Comment 19

[Frédéric Buclin]

Comment on attachment 8717955 [details] [diff] [review]
patch adds is_deletable attribute

We are very close now. The code is working fine, but the documentation and the UI have not been fully updated. Those are easy to fix:


>+++ b/Bugzilla/API/1_0/Resource/FlagType.pm

For all get(), create() and update() methods, the "History" section in the POD must be updated to mention that is_deletable has been added in Bugzilla 6.0.


>+++ b/Bugzilla/WebService/FlagType.pm

Same here.


>+++ b/docs/en/rst/administering/flags.rst

>+Deletable
>+    is_deletable boolean Users can clear flags of this type once set.
>+    Default is true

is_deletable is not a user facing thing and should not be mentioned here. Please write something like:
"By default, existing flags can be cleared. To prevent a flag from being cleared, uncheck this box."


You must also fix the documentation in docs/en/rst/api/core/v1/flagtype.rst to mention is_deletable, see http://bugzilla.readthedocs.org/en/latest/api/core/v1/flagtype.html.


And last point: you must also fix global/messages.html.tmpl (around line 610) to report changes made to the is_deletable attribute from the admin page.