168804 - Document CheckCanChangeField so sites can modify it for local needs

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Gervase Markham [:gerv]]

The function CheckCanChangeField in process_bug.cgi defines who can change what.
Lots of sites want to tweak the default Bugzilla policy in small ways, and
editing this function is the easiest way currently. We should document that
function better, and make it more clear how it works, so this is easier for
admins to do.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here's a first patch which rearranges, comments and simplifies the Perl in
CheckCanChangeField(), and writes an Administration section documentation page
to explain how to change it. The new CheckCanChangeField() is probably best
reviewed as itself, rather than as a diff.

Comment 2

[Joel Peshkin]

This looks like a good way to handle this.

As long as we're editing this function, we may as well expunge the rest of the
groupset terminology from its variable names, since it will become a misnomer soon.

I presume that a customizer could fetch the bug's existing record from the DB if
they really wanted to make a decision based on other fields.  I can't think of a
compelling reason that the decision might be influenced by other fields in the
proposed new state, but we may as well consider the possibility.   Is it likely
that customizations would have to do things like requireing a comment on
changing an attachment status?

Comment 3

[Gervase Markham [:gerv]]

> As long as we're editing this function, we may as well expunge the rest of the
> groupset terminology from its variable names, since it will become a misnomer 
> soon.

We can expunge it when groupsets go. :-)

> I presume that a customizer could fetch the bug's existing record from the DB if
> they really wanted to make a decision based on other fields.  

Yes. They can just add the field name to the query which finds the reporterid,
qacontactid and ownerid. But this is, as you say, an edge case.

So, how about a review? :-)

Gerv

Comment 4

[Joel Peshkin]

Comment on attachment 99299 [details] [diff] [review]
Patch v.1

Change the SQL so the newlines are not sent to the query and 
r=joel 

I would like a second review for this. I have to rely on inspection to make
sure that there is no logic bug and there are too many nuances for me to leave
it as a single review.

Probably as a seperate ehnancement to this, ideally, we would be able to
specify here what error message gets sent back to the user.

Comment 5

[Gervase Markham [:gerv]]

Thanks for the review :-) But:

> Change the SQL so the newlines are not sent to the query and 

This is normal practice in Bugzilla code; is there some problem with this idiom?

Gerv

Comment 6

[Bradley Baetz (:bbaetz)]

The original newline thing was for sqllog. SQllog is going away, though...

OTOH, it is neater, and since the perl compiler will combine those strings at
compile time, theres no reason not to.

Comment 7

[Gervase Markham [:gerv]]

> OTOH, it is neater, and since the perl compiler will combine those strings at
> compile time, theres no reason not to.

"Compile time" currently means "every time it's run", of course. One of the
performance tips in the Camel Book is not to concatenate strings like that when
it's not necessary.

I'm afraid I have to disagree with you on the neatness thing - I'd say that
having loads of quotes and . characters is uglier. It also makes it much more of
a pain to reformat the SQL to keep it inside 80 chars if you change it.

Multi-line strings is a wonderful feature that Perl has that other languages
don't. We should love it and use it :-)

Gerv

Comment 8

[Bradley Baetz (:bbaetz)]

I agree, you just asked for the reasons...

Anyway, the patch looks fine, _BUT_ keeping this function where it is (or even
at all), or the interface with the rest of the code is not stable. It _will_
change. At least once (in the movement to Bugzilla::Bug).

When it changes is unknown, but I don't want to have to support this forever
because it works now....

Comment 9

[Gervase Markham [:gerv]]

OK - would a big disclaimer added to the documentation help?

Presumably, even if the function moves, it'll still work roughly the same way,
so people can just copy their changes across. I can probably write some sort of
migration guide.

Gerv

Comment 10

[Joel Peshkin]

Since the underlying reason for the no-newlines seems to have evaporated, it
seems to be only a pref of MattyT's, so disregard the newlines caveat. (unless
you are asking Matty to do the 2xr)

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 99299 [details] [diff] [review]
Patch v.1

Yeah, r=bbaetz with that doc change.

Its likely that the 'API' for this will remain the same, but there are so many
special cases in there that it should probably be rewritten at some point

Comment 12

[Gervase Markham [:gerv]]

Fixed.

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.144; previous revision: 1.143
done
Checking in docs/sgml/administration.sgml;
/cvsroot/mozilla/webtools/bugzilla/docs/sgml/administration.sgml,v  <-- 
administration.sgml
new revision: 1.15; previous revision: 1.14
done

Gerv