98801 - (request-tracker) request tracker

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

The request tracker is a mechanism for requesting changes to data in Bugzilla
from other users (f.e. flagging a patch with the "has-review" status) and
tracking the status of those requests.

It comprises a form for making requests which is linked from the show bug and
edit attachment screens, a mechanism for automatically fulfilling requests when
a user of whom changes has been requested makes an update to the relevant
bug/attachment, bug mail that gets sent whenever a request is submitted or
fulfilled, and the ability to view the queue of pending requests in total or
broken down by requestee.

The well-developed bug infrastructure will be used to support requests. 
Although requests are entered and resolved in a streamlined way, internally a
request will really be a bug filed on a specific product and component that has
been designated to accept these requests.  This means that all the standard
facilities for dealing with bugs (mail, comments, re-assignment, querying, etc.)
will be available for requests.  The fundamental difference is that requests
will be "filed" via a special form and "resolved" via updates to other bugs.

The request queue will be implemented as queries on the particular product(s)
and component(s) that accept requests.

The request tracker was originally implemented as part of the patch tracker in
bug 84338, but that architecture proved too limited.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: request tracker implementation

Comment 2

[Myk Melez [:myk] [@mykmelez]]

The patch I have just submitted is an implementation of the request tracker.  It
uses bugs as the underlying mechanism for requests and allows installations to
define requests for virtually any change to bugs or attachments.  Features include:

* requests are stored in the database as regular bugs with some additional
information stored in the "requests" table, so anything you can do with a bug
(query, reassign, comment) you can also do with requests;
* requests can be made for bugs or attachments;
* request types get defined via a web-based administration screen (requestdefs.cgi);
* request types are product-specific, so different products can have different
requests;
* all request UI is generated by the template toolkit;
* users make requests by clicking links in the "show bug" screen ("Make
Requests" for the bug itself and "Req" next to each attachment for attachments);
* the "make requests" screen requires only the username of the fulfiller of the
request and an optional comment from the requester;
* the script that process the "make requests" data automatically completes
partial usernames and displays a pull-down list of possibilities if there are
more than one;
* requests are automatically fulfilled when the fulfiller updates the bug or
attachment for which a request of them has been made;
* when defining a request type, the administrator can provide a query whose
result will determine whether the request was granted or rejected when the
request is fulfilled; these states are translated into "RESOLVED FIXED" and
"RESOLVED WONTFIX" in the bug that represents the request.

Remaining issues include:

* Still a few validation routines to write for request.cgi;
* It might be easier for administrators if they could use the template toolkit
to write the queries that determine whether a request is granted or rejected
when fulfilled;
* Variable and function names should be reviewed for consistency;
* rejected -> denied?  granted/denied sounds better than granted/rejected.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: resolves all known issues

Comment 4

[Myk Melez [:myk] [@mykmelez]]

The latest version resolves all known issues with the patch tracker:

* Still a few validation routines to write for request.cgi;

Done.

* It might be easier for administrators if they could use the template toolkit
to write the queries that determine whether a request is granted or rejected
when fulfilled;

Done.

* Variable and function names should be reviewed for consistency;

Reviewed and modified where appropriate.

* rejected -> denied?  granted/denied sounds better than granted/rejected.

Requests are now either granted or denied.
Reviewers, go to it!

Comment 5

[Axel Hecht]

I was wondering if using regular bugs is really such a cool idea. The issue I
have with that is that (in spite of duplicates) people still use the new bug
count or total bug count as a measure of quality.
Does raising two, three or even more bugs per patch raise a scalability issue?

Axel

Comment 6

[Matthew Tuck [:CodeMachine]]

That's what I was thinking.  Perhaps a different number stream would be a better
idea - they could keep all the bug report features.

Comment 7

[Ludovic Dubost]

Hi,

This is a very cool feature.. I think it can help allow bugzilla being a more
efficient tool for working on issues especially when you are responsible for a
bug and you need other people do work for you on a bug...

Here are some comments on the interface:

- allow some request types to apply to the full bugzilla installation
- allow to say that requests are put in the same product/component as the
original request
- insert a message in the bug when a request is added
- put the request as a dependency of the bug for which the request was added
- allow to view all requests associated with a bug

In any case it is a great feature that I want on our installation very soon !
Great work...

Comment 8

[Myk Melez [:myk] [@mykmelez]]

>I was wondering if using regular bugs is really such a cool idea. The issue I
>have with that is that (in spite of duplicates) people still use the new bug
>count or total bug count as a measure of quality.


Along with the large number of duplicate bugs, the large number of resolved
bugs, the significant number of bugs filed on products unrelated to the core
Mozilla project (f.e. Webtools, Bugzilla, mozilla.org, etc.), and the collection
of bugs that are feature requests or requests for technical evangelism all
combine to make the idea that the total bug count on bugzilla.mozilla.org is a
representation of the overall quality of the Mozilla project a completely
outdated one which people are going to have to give up on sooner or later.  It
does us no good to do anything that encourages people to maintain that illusion.

>Does raising two, three or even more bugs per patch raise a scalability issue?

Bugzilla scalability issues come largely from comments and attachments.  As of
today, 90% of all space taken up in the Bugzilla database on b.m.o. is taken up
by attachments (70%) and comments (20%).  The bugs table takes up a little over
2% of the space (about 32MB).

The bug_id column is of type SIGNED MEDIUMINT, which has a maximum value of
8388607, but that column could easily be converted to an UNSIGNED MEDIUMINT
(16777215) or an UNSIGNED INT (4294967295) OR BIGINT (18446744073709551615).

Reviews are unlikely to have significant amounts of attachments/comments (or
any), so I don't think there are any scalability issues with this approach.

Comment 9

[Bradley Baetz (:bbaetz)]

Theres also the issue I mentioned to you on irc, wrt bug groups.

A user must be able to see a request if and only if they can see the underlying
bug (and we should warn if you make a request for a bug the person can't access.
If [e-mail] is added to the cc list of the bug, then they should
automatically be able to see the request., and similarly if they are removed.
(Similarly, on b.m.o the qa for the request, [e-mail], should have
a groupset of 0) - Note that the summary of the original report is added to the
review request, which makes this a must.

Similarly, the target milestone for the request should be fixed to that of the
underlying bug, as should priority, and possibly keywords/status whiteboard (so
that people review nsFoo+ bugs before other, misc, fixes)

Why would someone VERIFY/CLOSE a review request? What does it mean to add an
attachment to a request, or to vote on one, or to be cc'd on the bug?
(reassigning obviously makes sense)

If the reviewer has a comment on the patch, should they comment in the bug,
close the review request, and have another one opened? Why can't we close the
request by changing the status of the original patch (optionally, via an
additional checkbox, maybe?)

ie, I'm not convinced that making a request a 'bug' is that accurate, given the
number of connections needed to the underlying bug.

Also, can we add an x-bugzilla-reason header for new requests, so that people
can filter on them?

Comment 10

[Myk Melez [:myk] [@mykmelez]]

------- Additional Comments From Bradley Baetz 2001-10-09 16:37 -------

>A user must be able to see a request if and only if they can see the underlying
>bug (and we should warn if you make a request for a bug the person can't access.
>If [e-mail] is added to the cc list of the bug, then they should
>automatically be able to see the request., and similarly if they are removed.
>(Similarly, on b.m.o the qa for the request, [e-mail], should have
>a groupset of 0) - Note that the summary of the original report is added to the
>review request, which makes this a must.

QA contacts for secure bugs will be able to see the bug whether or not they are
a member of the appropriate security group once bug 97471 is fixed, so the
[e-mail] situation is not an issue.  One solution to the
[e-mail] problem is to synchronize the security settings and cc: list of
the request with the security settings of the bug itself when the bug changes. 
Another solution is to rely on the bug's security settings when determining the
security of the request.  Of these two solutions, the second one is probably the
cleaner approach but may be more difficult given the complexity of the new
"CanSeeBug" process of determining whether or not a given user has access to a
given bug.

>Similarly, the target milestone for the request should be fixed to that of the
>underlying bug, as should priority, and possibly keywords/status whiteboard (so
>that people review nsFoo+ bugs before other, misc, fixes)

>Why would someone VERIFY/CLOSE a review request?

Because they reviewed the patch.

>What does it mean to add an attachment to a request, or to vote on one, or to
be cc'd on the bug?

Not much.


>If the reviewer has a comment on the patch, should they comment in the bug,
>close the review request, and have another one opened? Why can't we close the
>request by changing the status of the original patch (optionally, via an
>additional checkbox, maybe?)

They should comment on the patch from the "edit attachment" form, which will
automatically close the request.

>ie, I'm not convinced that making a request a 'bug' is that accurate, given the
>number of connections needed to the underlying bug.

On the other hand there are numerous parallels between the concept of a request
and the current functionality of bugs, including the resolution process, the
owner/qa contact division of responsibilities, the concept of reassignment, and
the query system for generating lists and viewing individual bugs.

>Also, can we add an x-bugzilla-reason header for new requests, so that people
>can filter on them?

Yes, but this is probably unnecessary since request summaries are highly
structured, making it easy to filter mail by subject.

Comment 11

[Bradley Baetz (:bbaetz)]

> Another solution is to rely on the bug's security settings when determining the
> security of the request.  Of these two solutions, the second one is probably the
> cleaner approach but may be more difficult given the complexity of the new
> "CanSeeBug" process of determining whether or not a given user has access to a
> given bug.

Agreed. Although CanSeeBug could first check if the 'bug' is a request.

>>Why would someone VERIFY/CLOSE a review request?

>Because they reviewed the patch.

No, VERIFY/CLOSE, as opposed to RESOLVE.

> QA contacts for secure bugs will be able to see the bug whether or not they are
> a member of the appropriate security group once bug 97471 is fixed, so the
> [e-mail] situation is not an issue. 

Actually, it is; We don't want to close all the "bugzilla shows the summary of a
security bug" holes only to have it mailed to a public mailing list when the fix
is ready.

Comment 12

[Stefan Seifert]

If I may comment on this:
For clean database design you should create a table with the similarities of
real bugs and requests (like "item" or so) and leave the special things of both
in special tables (like "bug" and "request"), deciding via a flag in the item
table if it's a bug or a request. This way things that should work on both (like
queries and lists) do work and there are no useless fields which are only needed
in one of the two kinds. Also this might be a cleaner solution for security reasons.

Of course this would be a lot of work but on the long term I think it would be a
better solution.

Comment 13

[Gervase Markham [:gerv]]

My view: there is no need to synchronise any info between the request bugs and
the original bug after the creation of the request bug. 

It's extremely unlikely that a bug will make it all the way to a review request
before someone decides it's so confidential that it must also be confidential
that people are requesting reviews of it and its title must be hidden (as
opposed to, say, that said the words "Netscape 7" in it). What is more likely is
the bug will be opened and the reviews will remain confidential. That's not a
big deal, as the original bug contains info as to who reviewed it.

There are people who will not use e.g. target milestone on review bugs. There
are people who might use it but it wouldn't necessarily be set to the same as
the original bug (it might be earlier). If people want them synced, they can
start of syncing them themselves. If a large number of people need this, we can
think again.

I'm convinced that modelling a request as a bug is correct, because it's an RFA
- a requirement for action for someone to do something, which is an issue. If
there are fields in show_bug.cgi which are inappropriate, they should be hidden
by the template if it's a request bug.

Gerv

Comment 14

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: security fixes

This patch sets the groupset of the request bug to the groupset of the object
bug and does not add the summary of the object bug to the summary of the
request bug when the object bug is secure.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

This one slipped through the cracks, but I would really like to see it make the
2.16 release and am prepared to put in the work this week to make that happen. 
This feature will make it much easier and more transparent to navigate and track
the complicated review process for Bugzilla as well as for Mozilla, and getting
the feature in for 2.16 means it will get onto b.m.o. sooner.

Re-triaging to 2.16 milestone and looking for review.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Latest patches are running at:

http://landfill.tequilarista.org/bz98801/

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: adds info re: pending and completed requests to "show bug"

This version adds information about pending and fulfilled requests to the "show
bug" page.  The list of requests for the bug is located under the "keywords"
field, and the list of requests for each attachment is located in a column of
the attachments table.

Each request is referenced by name (f.e. vouch, review, approve) and is linked
to the request bug.  A plus sign is appended to granted requests, and a minus
sign is appended to denied ones.

The latest version is running on a test installation:

http://landfill.tequilarista.org/bz98801/
http://landfill.tequilarista.org/bz98801/show_bug.cgi?id=1

Comment 19

[Håkan Waara]

I'm looking forward to this feature, but it lacks one of the major functions
that I have been looking for since I CC:ed to this bug.

One of the major benefits of this feature would be that super-reviewers and
reviewers could save default queries for bugs that need review in their
component. If people started using it, it would dramatically reduce the amount
of bitrotting for patches and thus make it much easier to contribute to the project.

So what I'm requesting here is the ability to search for bugs with
review/approve/super-review (etc.) requested. Wasn't something like that the
whole point of this feature from the beginning, anyway? - That you wouldn't have
to bug people about r=/sr=/a= because there'd be a way for them to find out what
bugs needed it by themselves.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

It's important to make a distinction between searching for review requests and
searching for bugs with review requested.  The patches for this feature enable
the former but do not currently enable the latter.

In other words, if a reviewer or super-reviewer wants to see the list of bugs
for which their review has been requested, they can do that by searching for all
"review" requests assigned to them, and from those "request bugs" they can reach
the bugs/patches for which the requests have been made.

So, yes, this feature was the point of the whole thing, and yes it exists, but
it exists via a search for request bugs and not for bugs with requests.

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: feature enhancements

Version 5 includes: 

1. Added a link for the "edit request types" script to the Command Menu.
2. Made it possible to specify types of requests that apply generally to the
entire Bugzilla installation or specifically to a particular component within a
particular product (it was already possible to restrict request types to a
specific product).
3. Made it possible for requests to get filed in the same product/component as
the bug for which the request is being made.

This version is now running on the landfill test installation.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v6: adds deactivation of request types

This version adds deactivation of request types so the administrator can
prevent users from filing requests of certain types (for example, the "approve"
request type will only be available on mozilla.org's installation of Bugzilla
during the periodic milestone freeze cycle).

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 61390 [details] [diff] [review]
patch v6: adds deactivation of request types

needs-working this patch while I consider an architectural change that may be
more useful.  Instead of fulfilling a request when the user modifies the
request target and determining whether the request was granted or denied by
looking at the target's data, it might be more useful to have users specify
when they want to grant or deny pending requests and make the necessary changes
to the target upon specification.

Comment 24

[Myk Melez [:myk] [@mykmelez]]

Attachment: work-in-progress: partial reimplementation of fulfillment process

This work-in-progress, in addition to doing code clean-up, partially
re-implements the request tracker fulfillment procedure so that users
explicitly fulfill requests.  In the previous patch, users fulfill requests
when they update a bug or attachment for which a request has been made.  In
this patch, users fulfill requests when they specify that they would like to
fulfill the request when updating the bug/attachment.  This makes the process
of request fulfillment much clearer.

Since a request always involves a change in the bug or attachment, two new
database fields have been added: grantactions and denyactions.	The first field
contains code to be run when a request is granted, while the second code
contains code to be run when a request is denied.  The code runs before the
bug/attachment update is processed, so it can modify the submitted HTML form
values (or do anything else).  For example, for a "review" request, granting
the request would set the "has-review" flag and add "r=joeblow" to the comment
field, while denying the request would clear the "has-review" flag.

So far this patch implements this change in functionality only for attachments,
and it doesn't actually do the things called for in the action.  Remaining
tasks are to implement this change for bugs, to get actions working, and to do
any additional code clean-up necessary.

Comment 25

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v7: reimplementation of fulfillment process and code clean-up

Code Clean-Up

1. License headers on template files.
2. atml -> tmpl
3. Miscellaneous other clean-up including code reuse.

Fulfillment Process Re-Implementation:

The way it works is that when you load a bug or an attachment that has pending
requests, you see a list of those requests along with options for granting or
denying them (if you are the requestee).  When you grant or deny a request,
code runs that makes modifications to the bug/attachment record in accordance
with that request (this code is configurable on a per-request-type basis).

This means that instead of users having to know that to fulfill a particular
request they have to make particular modifications to the bug/attachment, all
they have to do is explicitly grant or deny the request, and those
modifications are made for them (f.e. flagging an attachment with the "review"
status or adding "r=[e-mail]" to a comment.

The landfill testing server is down at the moment, so I can't put these patches
up on the test installation there, but I'll do so as soon as possible.

Comment 26

[Håkan Waara]

Will this patch take my comments from comment 19 into account? I really do think
being able to query for requests is crucial... sorry if I missed something.

Comment 27

[Myk Melez [:myk] [@mykmelez]]

>Will this patch take my comments from comment 19 into account? I really do think
>being able to query for requests is crucial... sorry if I missed something.

Yes, this patch takes your remarks in comment 19 into account.  It does so in
the way described in comment 20, in that it enables you to search for requests
with all the advanced querying capabilities currently available for bugs.

In others words, you can search by requester (i.e. reporter), fulfiller (i.e.
assignee), pending vs. fulfilled status (i.e. open vs. closed status), granted
vs. denied resolution (i.e. FIXED vs. WONTFIX), and by any other field.

Comment 28

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v8: reformatted per bugzilla coding standards

The code has now been reformatted to Bugzilla coding standards.  There is still
no sign of landfill, the test installation, but I'll put it up there when it
comes back up.

Comment 29

[Håkan Waara]

> In others words, you can search by requester (i.e. reporter), fulfiller (i.e.
> assignee), pending vs. fulfilled status (i.e. open vs. closed status), granted
> vs. denied resolution (i.e. FIXED vs. WONTFIX), and by any other field.

Awesome!

Comment 30

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v9: updates to latest CVS and minor fixes

Besides updating to the latest CVS, I fix the edit*.cgi routines so they update
the product and component fields in the requesttypes table when a product or
component name changes.

Comment 31

[Dave Miller [:justdave]]

After talking with Gerv in IRC the other night, we're going to hold this for
first thing after 2.16 ships, just because it's a big change. and a big deal to
review.  And I want to ship 2.16 :)

Comment 32

[Christian Reis]

A) This is a really cool feature

b) The label "Request Of" doesn't mean much to me. Brainstorm:

- Request recipients: 
- Request targets:
- Request from:
- Request from these emails:

c) When using multiple email addresses, separated by spaces, the confirmation
form just redisplayed itself with no visible warning. Are multiple emails not
supported, then?

Comment 33

[Christian Reis]

Requesting with commas between emails doesn't work. So I grasp it's a single
email. We should really display a message warning the user.

Note: I am testing on landfill, hoping it's up to date.

Comment 34

[Myk Melez [:myk] [@mykmelez]]

>b) The label "Request Of" doesn't mean much to me. Brainstorm:
>
>- Request recipients: 
>- Request targets:
>- Request from:
>- Request from these emails:

Good ideas, to which I'll add mpt's "ask".

>c) When using multiple email addresses, separated by spaces, the confirmation
>form just redisplayed itself with no visible warning. Are multiple emails not
>supported, then?

That's correct.  A request is a bug, and a bug can only have one assignee, so a
request can only have one requestee/fulfiller.

Comment 35

[Matthew Tuck [:CodeMachine]]

2.16+ ? =)

Comment 36

[Gervase Markham [:gerv]]

I had to fix a number of rotted things. New patch coming up. However, I fixed
one thing in a hacky way, which you will need to correct - Request::list
currently does the wrong thing passing variables back and forth from
bug_form.pl. (line 186.)

I got an error:
"Sorry, no request types are defined for attachments in the FoodReplicator
product and SaltSprinkler component."

This error should not appear; we should check this fact before show_bugging, and
not print the Req link if making a request is not possible.

Creating Request types: you do not need to explain all the fields at length on
the initial screen if they are explained on the Create Request Type screen. a
short description (bullets) is enough.

Request Tracker is going to remain a very niche feature if adding request types
requires writing Perl code to work. Also, having a UI element which allows
execution of arbitrary Perl makes a Bugzilla permissions security breach into a
system security breach. Is there any way to avoid this?

The Product and Component sections of this page should have links to create new
products and components; they should also suggest that requests have their own
product/component to make bug stats (e.g. open bugs in 'Browser') still vaguely
valid measures.

Do we really need sort keys? Can't we just do alphabetical or something? If not,
can we default this box to 0, and explain it means "don't care"?

"Request of" should be "Ask". In fact, it should look like:

<name>: <description>
Ask [        ] to <name> <bug/attachment># <bug/attach no>. (on bug # <bug no>)
Comments:

e.g.

Review: review the attachment
Ask [ [e-mail] ] to review attachment #76474 [details] [diff] [review] on bug #34523
Comments:
Please review my patch.

I've now filed my request, and go to look at it.

Summary is: <name>: <target bug summary>, <attachment title>

It should be:
<name>: "<attachment title>" on bug 12345 - "<bug summary>"

Thought: we need to provide guidance on request type titles and descriptions, in
terms of grammar. Should request types by verbs or nouns? Etc etc.

The initial text for the request looks like the following:

Gervase Markham <[e-mail]> has requested review for bug #34: testbug,
attachment #39 [details] [diff] [review]: foo.

 
http://cvs-mirror.mozilla.org/webtools/bugzilla/attachment.cgi?id=39&action=edi
t



Additional Comments from Requester:

Please review my patch.
  
(Note that the URL has got a \n in it and so doesn't work.)
  
This should be (note the removal of quite a lot of whitespace):

Gervase Markham <[e-mail]> has requested review for 
attachment #39 [details] [diff] [review]: foo
on bug #34: testbug

Edit the attachment:
http://cvs-mirror.mozilla.org/webtools/bugzilla/attachment.cgi?id=39&action=edit

Please review my patch.


The comment which gets added to the original bug upon fulfilment of the request
should also get added to the request. This is because it may list conditions or
caveats on the review or whatever.  

The severity of the request should initially match the severity of the bug; if
it's a blocker bug, it should be an important review. Or do we want to map sev
to priority here? No - that's a big can of worms.

When we parameterise the name by which all the templates refer to a "bug" (this
being one of the most commonly-requested customisations), we can probably make
the show_bug page talk about requests throughout as well. That would be cool.

There's an unclosed <i> tag on the Create Request Type page - my footer's in
italics :-)

Adding a bug-type request without Grant and Deny Perl appears to make no sense;
this should be forbidden by the Add Request Type interface.

Can the bug requests UI look a little more like (although distinguishable from)
the attachments UI?

That'll do to be going on with :-)

Gerv

Comment 37

[Gervase Markham [:gerv]]

Attachment: Patch v.9a

Unrotted version.

Gerv

Comment 38

[Myk Melez [:myk] [@mykmelez]]

Attachment: work-in-progress

This is a work-in-progress based on Gerv's review comments.

Comment 39

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v10: review fixes

>I had to fix a number of rotted things. New patch coming up. However, I fixed
>one thing in a hacky way, which you will need to correct - Request::list
>currently does the wrong thing passing variables back and forth from
>bug_form.pl. (line 186.)

Fixed.

>I got an error:
>"Sorry, no request types are defined for attachments in the FoodReplicator
>product and SaltSprinkler component."
>
>This error should not appear; we should check this fact before show_bugging,
>and not print the Req link if making a request is not possible.

Fixed.

>Creating Request types: you do not need to explain all the fields at length on

>the initial screen if they are explained on the Create Request Type screen. a
>short description (bullets) is enough.

Fixed.

>Request Tracker is going to remain a very niche feature if adding request
types
>requires writing Perl code to work. Also, having a UI element which allows
>execution of arbitrary Perl makes a Bugzilla permissions security breach into
a
>system security breach. Is there any way to avoid this?

Avoiding arbitrary Perl is hard, because it is what makes the request tracker
flexible (otherwise I could have just hard-coded the specific functionality
mozilla.org needs into the app, causing all sorts of problems for mozilla.org,
Bugzilla, or both).

Earlier versions which avoided this problem inverted the relationship between
grant and deny actions and request resolution by checking user changes for
tell-tale signs of request fulfillment and deducing whether the request was
granted or denied.  Besides being error-prone, this mechanism was thoroughly
unintuitive for end-user who were required to memorize which of their changes
would grant or deny certain requests.

I also tried making the actions be template snippets, but this proved not to be
as flexible as I wanted.  I don't think we can get around the problem of
requiring administrators to have Perl knowledge to use request tracker, but we
can solve the security problem by putting the action code into files in the
filesystem and merely referencing them in the grantactions and denyactions
fields of the requesttypes record.

>The Product and Component sections of this page should have links to create
new
>products and components; they should also suggest that requests have their own

>product/component to make bug stats (e.g. open bugs in 'Browser') still
vaguely
>valid measures.

Done.

>Do we really need sort keys? Can't we just do alphabetical or something? If
>not, can we default this box to 0, and explain it means "don't care"?

We need sort keys so "approval" sorts after "review" and "super-review", but I
gave the value a default.  Ultimately this should go into the back-end when we
have a super-duper administration console (with a GUI-re-orderable list of
types).

>"Request of" should be "Ask". In fact, it should look like:

Done, modulo verb/noun decisions discussed below.

>Summary is: <name>: <target bug summary>, <attachment title>
>
>It should be:
><name>: "<attachment title>" on bug 12345 - "<bug summary>"

I did it this way originally, but when I showed it to super-reviewers they said
the bug should be first even for attachments.  This makes sense: even if you
are reviewing an attachment, the name of the bug (f.e "request tracker for
tracking requests") still tells you more about the request than the name of the
attachment (f.e. "patch v10: review fixes").

>Thought: we need to provide guidance on request type titles and descriptions,
>in terms of grammar. Should request types by verbs or nouns? Etc etc.

We need to decide this ourselves before we can guide others.  If we say verbs,
we should structure sentences like so:

Ask [fulfiller] to [request] [bug/attachment].

If it's nouns, however, we want this instead:

Ask [fulfiller] for [request] on [bug/attachment].

Each one works better in some situations, but the verb form seems a little
clearer overall.

>The initial text for the request looks like the following:
  ...
>This should be (note the removal of quite a lot of whitespace):

White-space removed, but the rest depends on noun/verb decisions and per
comments above about bug info coming before attachment info.

>The comment which gets added to the original bug upon fulfilment of the
request
>should also get added to the request. This is because it may list conditions
or
>caveats on the review or whatever.  

Hunh?  No comment is added to either bug by default.

>The severity of the request should initially match the severity of the bug; if

>it's a blocker bug, it should be an important review. Or do we want to map sev

>to priority here? No - that's a big can of worms.

Done.

>There's an unclosed <i> tag on the Create Request Type page - my footer's in
>italics :-)

Actually it was an over-broad CSS rule.  Fixed.

>Adding a bug-type request without Grant and Deny Perl appears to make no
sense;
>this should be forbidden by the Add Request Type interface.

It does make sense in cases where you only want to track granting or denying of
the request without causing those actions to change the target.  This is also a
way for installations without Perl knowledge to use the request tracker.

>Can the bug requests UI look a little more like (although distinguishable
from)
>the attachments UI?

Done.

>That'll do to be going on with :-)

Thanks for the review!	Now take a look at this version! :->

Comment 40

[Myk Melez [:myk] [@mykmelez]]

Btw- I also converted the code to use the new message.html.tmpl in place of its
own message.tmpl.

Comment 41

[Myk Melez [:myk] [@mykmelez]]

cc:ing Gerv so he actually gets mail about this bug.

Comment 42

[Bradley Baetz (:bbaetz)]

I still that tagging onto the bugs table isn't the way to go. Lets look at the
fields on the bugs table:

bug_id - some sort of id is needed
groupset - N/A - can access request only if can access underlying bug
assigned_to - separate for request
bug_file_loc - N/A
bug_severity - N/A
bug_status - N/A - these are separate for requests, and include flags, and so
on, which are separate to those for a bug
creation_ts - I guess you could have this
delta_ts - not sure if you need this
short_desc - N/A - "Request for $job for $underlying_bug_no"
op_sys - N/A
priority - Separate, I guess, since people may want to prioritise their review
requests
product - taken from underlying bug
rep_platform - taken from underlying bug
reporter - => requestor, but needed
version - taken from underlying bug
component - taken from underlying bug
resolution - Separate issue, since hte resolutions aren't the same as for bugs
(esp given custres)
target_milestone - taken from underlying bug
qa_contact - N/A - I know that you want to use this to cc [e-mail],
but I thikn some form of component watching would be the best way for this, and
in the meantime bmo can just hack processmail/Bugmail.pm
status_whiteboard - maybe, I guess
votes - N/A
keywords - N/A
lastdiffed - may be needed for mail
everconfirmed - N/A
reporter_accessible - N/A - can only access a requst if they can access the
underlying bug
cclist_accessible - N/A - ditto

New:
'original bug no'
'type'
Is this per attachment or per bug, btw?

So out of the 26 columns in the bugs table:

12 aren't wanted at all
5 are taken from the underlying bug
4 are maybe
5 are usable, although the names won't be strictly accurate in the tables (this
doesn't matter, though)

That comes out to anywhere from < 20% to just over one third of the columns
being used from the bugs table.

And in addition we need the 2-3 new columns for requests mentioned above/

In addition, votes, keywords, and dependancies don't really make sense for
requests. As well, the generic fields stuff is probably not appropriate to requests.

Also, do comments and cc lists make sense? Are people likely to want to comment
on the request itsself, as opposed to the bug/attachment the request is made
for? We do want all discussion in the one place, right?

I think that sharing the output template is a good idea, but that doesn't mean
that the underlying backend has to be related. Whilst it makes sense from an OO
point of view, mysql doesn't do inheritance.

This is probably easier now that we've finished the front end templatisation. 

The disadvantage with my suggestion is that you'd have to make changes so that
mail is sent out. OTOH, if you just store it in the bug's bug_Activity table,
then you just send mail for the bug, similar to how attachments work now.

Also, in your patch, how would I search for open requests, as opposed to open bugs?

Comment 43

[Bradley Baetz (:bbaetz)]

Oh, and the 'perl code in the db' stuff means that we'd have to freeze our
schemas, to avoid breaking stuff in the future. Unless we're happy with that
maybe breaking sometimes...

Comment 44

[Myk Melez [:myk] [@mykmelez]]

>bug_status - N/A - these are separate for requests, and include flags, and so
>on, which are separate to those for a bug
>resolution - Separate issue, since hte resolutions aren't the same as for bugs
>(esp given custres)

Status and resolution both work the same way for requests as for bugs.
Every request has a status (PENDING, RESOLVED) and a resolution (GRANTED,
DENIED) if resolved.  The field values are overloaded for use with requests
at the moment, but that will change when custom resolutions (and a custom
statuses corollary) get implemented.

>delta_ts - not sure if you need this

You do if any fields can change, which they can.

>product - taken from underlying bug
>component - taken from underlying bug

Separate, actually, for the kinds of requests I plan to implement on b.m.o.
Review requests are a different category of bug and deserve their own product
and component (f.e. see "mozilla.org:CVS Account Request").  I implemented
the ability to file them into the same product/component as their bug because
someone asked for it, but we won't use it this way on b.m.o.

>qa_contact - N/A - I know that you want to use this to cc [e-mail],
>but I thikn some form of component watching would be the best way for this, and
>in the meantime bmo can just hack processmail/Bugmail.pm

The QA contact field is a perfect match for review requests, because 
[e-mail] really is the QA contact for those requests 
(i.e. they are responsible for making sure those requests get fulfilled).

>New:
>'original bug no'
>'type'
>Is this per attachment or per bug, btw?

I don't understand this question.  A record in the requests table exists 
per-request.

> As well, the generic fields stuff is probably not appropriate to requests.

The generic fields stuff, by definition, is appropriate for anything for which
people can find a use for it.

>Also, do comments and cc lists make sense? Are people likely to want to comment
>on the request itsself, as opposed to the bug/attachment the request is made
>for?  We do want all discussion in the one place, right?

Certainly, if they want to ping people about the request and keep a record 
of it.  Our current system (email and sometimes comments in the bug) makes 
it much harder to resolve issues when they come to staff because we can't 
easily look at a single record of all communication regarding a review request.

Discussion about the bug and how to fix it is different from discussion
about the process of reviewing it, and it makes sense to separate the two.

>I think that sharing the output template is a good idea, but that doesn't mean
>that the underlying backend has to be related.

I think the opposite is true.  The underlying back-end needed by requests
is sufficiently similar that it makes more sense to reuse it than reinvent 
it, while the front-end is sufficiently different that a different 
template/view/format would serve us well.

> Whilst it makes sense from an OO
>point of view, mysql doesn't do inheritance.

Not strictly speaking (we would need an OO database for that), but these kinds
of data hierarchies have been modeled in relational databases for decades.  The
way to do it is to keep common data in one table and split uncommon data into
separate tables for each type of issue (bug, request, etc.).

>This is probably easier now that we've finished the front end templatisation. 

Yes, agreed.

>The disadvantage with my suggestion is that you'd have to make changes so that
>mail is sent out. OTOH, if you just store it in the bug's bug_Activity table,
>then you just send mail for the bug, similar to how attachments work now.

This was how I originally implemented the request tracker.  When I showed it 
around, however, I kept being asked questions about whether it could do this
or that, all of which happened to be features that already exist for bugs.
There's some UI pain in the new approach that needs to be ameliorated over time,
with new templates/formats/views and some underlying architectural issues that
we'll need to shake out, but overall it's the most sensible approach, and I
suspect that users will find good uses for some of those "unused" fields once
they have the opportunity.

>Also, in your patch, how would I search for open requests, as opposed to open bugs?

On b.m.o you would search for bugs in the request product and component
(probably "mozilla.org/review requests", "mozilla.org/super-review requests",
etc.).  If requests were in the same product/component as their bugs you might
instead search for bugs with the characteristic summary.  In the future we may
add a special search for requests to the search form (i.e. search for bugs that
are requests, or search for bugs that have a certain kind of request of them).

>Oh, and the 'perl code in the db' stuff means that we'd have to freeze our
>schemas, to avoid breaking stuff in the future. Unless we're happy with that
>maybe breaking sometimes...

I'm going to move the Perl code to files in the filesystem for security.
Even so, your schema comment still applies, but that is true of any code 
that installations add to the core Bugzilla code.  Using grant and deny
actions is optional, and the request tracker is still useful without it,
so we should just warn administrators of the risks associated with them
and make them an advanced feature.

Comment 45

[Gervase Markham [:gerv]]

> The QA contact field is a perfect match for review requests, because 
> [e-mail] really is the QA contact for those requests 
> (i.e. they are responsible for making sure those requests get fulfilled).

I'm not convinced about this; normally only the first mail goes to reviewers,
not all the follow-up mail. If we could configure their email prefs so they only
got the first mail, that would be fine. But I don't think you can do that right now.

> I'm going to move the Perl code to files in the filesystem for security.

I was thinking about this the other day and came to the same conclusion. 

Gerv

Comment 46

[Bradley Baetz (:bbaetz)]

gerv: You can set it up so that they only get noified when they are
added/removed from teh bug, so that would work.

myk: I'm still not convinced. I think that fact that you're trying to shoehorn
these non-bugs into the bugs ui, and do field mapping (both in teh ui, and a
brand new product/component) and so on shows that. You're saying 'request isA
bug', which isn't true. If there was some common 'supertable' for bugs and
requests, then it might make more sense, I guess.

What, specifically, were the things which bugs can do but this can't? Querying
is the hardest part, but your patch doesn't let me search on 'bug is request'
'=' '1' anyway.

However, I suspect that I'm unlikly to convince you about this, so...

>>Is this per attachment or per bug, btw?

>I don't understand this question.  A record in the requests table exists 
>per-request.

I meant: Does a user make a request for the bug, or for a particular attachment
on the bug?

Comment 47

[Myk Melez [:myk] [@mykmelez]]

>I meant: Does a user make a request for the bug, or for a particular attachment
>on the bug?

Request types can be set up either way, so that (for example) review requests
are made for attachments and cvs voucher requests are made for bugs.

Comment 48

[Myk Melez [:myk] [@mykmelez]]

>I think that fact that you're trying to shoehorn
>these non-bugs into the bugs ui, and do field mapping (both in teh ui, and a
>brand new product/component) and so on shows that.  You're saying 'request
>isA bug', which isn't true.

It's not true in the literal sense, but the "bug system" has been used to track
"non-bug" requests by a number of installations, including b.m.o (see
product:mozilla.org, component:CVS Account Request, among others), and this
situation is not different.  Using Bugzilla as an issue-tracking and
service-request system is common, judging by the number of mentions of such use
in the newsgroup.

 If there was some common 'supertable' for bugs and
>requests, then it might make more sense, I guess.

This is likely to happen in the long run as we improve support for the various
kinds of "non-bugs".

>What, specifically, were the things which bugs can do but this can't?

I'm not sure what you mean by "this", but bug reports can be opened, assigned to
someone, reassigned, and resolved fixed or not-fixed.  They can be prioritized;
mail can be sent to interested parties when changes occur with them; someone can
be assigned to make sure they are resolved.  They can be queried, and queries
for them can organize the query results in various ways.

>Querying
>is the hardest part, but your patch doesn't let me search on 'bug is request'
>'=' '1' anyway.

Not by selecting "bug is request" "equals" "1" on the boolean chart, but it
certainly does let you query for bugs that are requests.

Comment 49

[Myk Melez [:myk] [@mykmelez]]

Attachment: work-in-progress

Work-in-progress of the Request Tracker updated to current architecture.  The
only thing left to do is change the template names, but I have run out of time
tonight.  I'll post the patch tomorrow, but in the meantime this works and can
be reviewed/tested.

Comment 50

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v11: updates to current BZ architecture

This patch updates the Request Tracker to the current Bugzilla architecture,
including Throw*Error functions, the template/en directory, a global template
object, and template file naming conventions.

Along the way I removed the requirement that the assignee be the one to fulfill
a request (silly since anyone is allowed to resolve the bug behind the request
and since there are situations where someone else should be able to fulfill the
request).

Ready for re-review.

Comment 51

[Gervase Markham [:gerv]]

Comment on attachment 83943 [details] [diff] [review]
patch v11: updates to current BZ architecture

+    # Uses an unnecessary conditional to suppress "used only once" warnings.
+    my $template = $::template || $::template;
+    my $vars = $::vars || $::vars;

Why not use the "use vars" pragma like everywhere else? Consistency is good.

+    $data{'bug_severity'} = &::SqlQuote($request->{'target'}->{'component'});

Is this really what you mean?

+    # Determine the product and component (into which to file the bug) by 
+    # looking at the request type, which will either define a
product/component
+    # or leave one or both undefined, in which case the request bug will get 
+    # filed into the same product/component as the target bug/attachment.
+    $data{'product'} = 
+      $request->{'type'}->{'product'} || $request->{'target'}->{'product'};
+    $data{'component'} = 
+      $request->{'type'}->{'component'} ||
$request->{'target'}->{'component'};
+    $data{'product'} = &::SqlQuote($data{'product'});
+    $data{'component'} = &::SqlQuote($data{'component'});
+
+    # Create a summary for the request containing the type of request and the
+    # summary of the target bug/attachment unless the target is a secure bug
+    # or an attachment of a secure bug, in which case display only the target
+    # ID to prevent leakage of information about the bug.

... unless you are filing requests in the same product as the bugs and it's a
product-based group, in which case you can use the title. (This, I think, might
actually be a quite-common case.)

+    # Return empty-handed if the request does not exist.
+    (&::PopGlobalSQLState() && return) if !&::MoreSQLData();

Won't this return the return value of the last-executed statement, i.e.
PopGlobalSQLState(), rather than the null which you might expect?

+sub MatchUsers {
+    # Generates a list of users whose login name (email address) or real name
+    # matches a substring.  Useful for finding the list of potential
fulfillers
+    # for a new request where the requester did not enter the full names
+    # of fulfillers.

This function is useful for editusers.cgi also? Can it be put somewhere more
accessible, so when the admin interface gets rewritten, we can use it?

+sub GetUser {
+    # Returns a hash of information about a particular user.

Can you use GetUserInfo() from CGI.pl for this?

 # Check whether or not the user is logged in and, if so, set the $::userid 
 # and $::usergroupset variables.
 quietly_check_login();
 $vars->{'userid'} = $::userid;

 This gets set anyway, by GetUserInfo(), as $vars->{'user'}{'userid'}.

 +sub validateRequests
+{
+  my @requestids = map(/^request-(\d+)$/ ? $1 : (), keys %::FORM);

Cool, but complicated. It took me a while to figure this out. An explanatory
comment, perhaps?

-  # If this installation has enabled the request manager, let the manager know
-  # an attachment was updated so it can check for requests on that attachment
-  # and fulfill them.	The request manager allows users to request database
-  # changes of other users and tracks the fulfillment of those requests.  When
-  # an attachment record is updated and the request manager is called, it will
-  # fulfill those requests that were requested of the user performing the
update
-  # which are requests for the attachment being updated.
-  #my $requests;
-  #if (Param('userequestmanager'))
-  #{
-  #  use Request;
-  #  # Specify the fieldnames that have been updated.
-  #  my @fieldnames = ('description', 'mimetype', 'status', 'ispatch',
'isobsolete');
-  #  # Fulfill pending requests.
-  #  $requests = Request::fulfillRequest('attachment', $::FORM{'id'},
@fieldnames);
-  #  $vars->{'requests'} = $requests; 
-  #}
-

Huh? :-) Is this stuff in current Bugzilla CVS?

     # Attachments
     $bug{'attachments'} = Attachment::query($id);
-
+   
+    if (Param('userequesttracker')) {
+	 # Whether or not any types of requests can be made for this bug.
+	 # If not, no UI for making requests will be displayed.
+	 $vars->{'has_request_types'} = 
+	   Request::HasTypes("bug", $bug{'product'}, $bug{'component'});

Surely $bug{'has_request_types'}?

+	 # Whether or not any types of requests can be made for attachments
+	 # on this bug.  If not, no UI for making requests will be displayed.
+	 $vars->{'attachments_have_request_types'} = 
+	   Request::HasTypes("attachment", $bug{'product'}, $bug{'component'});

Same here, I would say.

+	 $vars->{'userid'} = $::userid;

Again, GetUserInfo() makes this available to you.

+# Each request type has a record in the requesttypes table consisting of
+# a unique integer key, a name containing no commas or white-space,
+# a description, two pieces of Perl code that get evaluated when a request
+# of this type is granted or denied, respectively, 

Were you not going to move these out to the filesystem for security reasons? I
don't like the idea that, if someone steals an administrator's Bugzilla
password, they can execute arbitrary Perl on the server.

<looks below> 
Ah, you did. Comment needs updating :-)

+# into which request bugs will be filed (these can be null, in which case 
+# the bugs will get filed into the same product and component as the target 
+# bug/attachment), 

Presumably product cannot be null if component is non-null?

+DefParam("userequesttracker",
+	  "Whether or not to use the request tracker that enables users 
+	   to request changes to bugs and attachments.",

"request changes of status"? This makes it sound like users can now say "Can
someone please confirm this bug? I don't have permissions..."

editrequesttypes.cgi:

+else { 
+    ThrowCodeError("I could not figure out what you wanted to do.");

"Unrecognised value of action variable"? Let's be specific here; it's a
ThrowCodeError :-)

+    length($::FORM{'grantactions'}) <= 50
+      || ThrowUserError("The name of the file containing grant actions 
+			    cannot be more than 50 characters long.");

Is this a filename or a relative path? 50 chars is reasonably short for a
relative path...

+		$::FORM{'grantactions'} !~ /\//
+	|| ThrowUserError("You can't have a directory separator (/) in 
+			    the name of the file containing grant actions.");
+}

Are there ramifications for Windows here? (denyactions also.) Also, can
validateGrantActions and validateDenyActions be made a single, parameterised
sub?

+sub validateTargetProduct {
+    # An empty target product means targets in any product can have requests
+    # of this type made for them.
+    return if !$::FORM{'targetproduct'};
+
+    my $product = SqlQuote($::FORM{'targetproduct'});
+    SendSQL("SELECT 1 FROM products WHERE product = $product");
+    my ($productexists) = FetchSQLData();
+
+    $productexists
+      || ThrowCodeError("The target product you selected does not exist.") 
+	 && exit;
+}

And this and validateProduct? And the two component ones?

+    # Generate and return the UI (HTML page) from the appropriate template.
+    $template->process("requesttype/list.html.tmpl", $vars)
+      || ThrowTemplateError($template->error());

File locations. Given that you got yourself a top level directory for
"attachment", I guess I can't complain about having one for "request". But the
"requesttypes" stuff should go as a subdirectory of "admin", like
"attachstatus".

+sub deleteType {
+    SendSQL("LOCK TABLES requesttypes WRITE, requests WRITE");
+    SendSQL("DELETE FROM requests WHERE typeid = $::FORM{'id'}");
+    SendSQL("DELETE FROM requesttypes WHERE id = $::FORM{'id'}");
+    SendSQL("UNLOCK TABLES");
+
+    $vars->{'message'} = "The request type has been deleted.";

Does it have a name you can give?

+# make_js_array: iterates through the product array creating a
+# javascript array keyed by product with an alphabetically ordered array
+# for the corresponding elements in the components array passed in.
+# return a string with javascript definitions for the product in a nice
+# arrays which can be linearly appended later on.

This looks generally useful - in query.cgi and your "make Bugzilla config
available via JS" patch. If that's true, can we move it somewhere more general?
Or should this stuff be done in a template anyway?

+    if (Param('userequesttracker')) {
+      $template->process("request/fulfilled.html.tmpl", $vars)
+	 || DisplayError("Template process failed: " . $template->error() .
".")
+	   && exit;

Please use DisplayTemplateError() throughout. (Then there's no need for
"Template process failed".)







+function updateSelect( array, sel, target, sel_is_diff, single, blank ) {
+
+    var i, j, comp;
+
+    // if single, even if it's a diff (happens when you have nothing
+    // selected and select one item alone), skip this.
+    if ( ! single ) {
+
+	 // array merging/sorting in the case of multiple selections
+	 if ( sel_is_diff ) {
+
+	     // merge in the current options with the first selection
+	     comp = merge_arrays( array[sel[0]], target.options, 1 );
+
+	     // merge the rest of the selection with the results
+	     for ( i = 1 ; i < sel.length ; i++ ) {
+		 comp = merge_arrays( array[sel[i]], comp, 0 );
+	     }
+	 } else {
+	     // here we micro-optimize for two arrays to avoid merging with a
+	     // null array
+	     comp = merge_arrays( array[sel[0]],array[sel[1]], 0 );
+
+	     // merge the arrays. not very good for multiple selections.
+	     for ( i = 2; i < sel.length; i++ ) {
+		 comp = merge_arrays( comp, array[sel[i]], 0 );
+	     }
+	 }
+    } else {
+	 // single item in selection, just get me the list
+	 comp = array[sel[0]];
+    }
+
+    // save the selection in the target select so we can restore it later
+    var selections = new Array();
+    for ( i = 0; i < target.options.length; i++ )
+      if (target.options[i].selected)
selections.push(target.options[i].value);
+
+    // clear select
+    target.options.length = 0;
+
+    // add empty "Any" value back to the list
+    if (blank) target.options[0] = new Option( blank, "" );
+
+    // load elements of list into select
+    for ( i = 0; i < comp.length; i++ ) {
+	 target.options[target.options.length] = new Option( comp[i], comp[i]
);
+    }
+
+    // restore the selection
+    for ( i=0 ; i<selections.length ; i++ )
+      for ( j=0 ; j<target.options.length ; j++ )
+	 if (target.options[j].value == selections[i])
target.options[j].selected = true;
+
+}
+
+// Returns elements in a that are not in b.
+// NOT A REAL DIFF: does not check the reverse.
+//	- a,b: arrays of values to be compare.
+
+function fake_diff_array( a, b ) {
+    var newsel = new Array();
+
+    // do a boring array diff to see who's new
+	 for ( var ia in a ) {
+	     var found = 0;
+	     for ( var ib in b ) {
+		 if ( a[ia] == b[ib] ) {
+		     found = 1;
+		 }
+	     }
+	     if ( ! found ) {
+		 newsel[newsel.length] = a[ia];
+	     }
+	     found = 0;
+	 }
+	 return newsel;
+    }
+
+// takes two arrays and sorts them by string, returning a new, sorted
+// array. the merge removes dupes, too.
+//	- a, b: arrays to be merge.
+//	- b_is_select: if true, then b is actually an optionitem and as
+//	  such we need to use item.value on it.
+
+    function merge_arrays( a, b, b_is_select ) {
+	 var pos_a = 0;
+	 var pos_b = 0;
+	 var ret = new Array();
+	 var bitem, aitem;
+
+    // iterate through both arrays and add the larger item to the return
+    // list. remove dupes, too. Use toLowerCase to provide
+    // case-insensitivity.
+
+	 while ( ( pos_a < a.length ) && ( pos_b < b.length ) ) {
+
+	     if ( b_is_select ) {
+		 bitem = b[pos_b].value;
+	     } else {
+		 bitem = b[pos_b];
+	     }
+	     aitem = a[pos_a];
+
+	 // smaller item in list a
+	     if ( aitem.toLowerCase() < bitem.toLowerCase() ) {
+		 ret[ret.length] = aitem;
+		 pos_a++;
+	     } else {
+	     // smaller item in list b
+		 if ( aitem.toLowerCase() > bitem.toLowerCase() ) {
+		     ret[ret.length] = bitem;
+		     pos_b++;
+		 } else {
+		 // list contents are equal, inc both counters.
+		     ret[ret.length] = aitem;
+		 pos_a++;
+		 pos_b++;
+	     }
+	 }
+	 }
+
+    // catch leftovers here. these sections are ugly code-copying.
+	 if ( pos_a < a.length ) {
+	     for ( ; pos_a < a.length ; pos_a++ ) {
+		 ret[ret.length] = a[pos_a];
+	     }
+	 }
+
+	 if ( pos_b < b.length ) {
+	     for ( ; pos_b < b.length; pos_b++ ) {
+		 if ( b_is_select ) {
+		     bitem = b[pos_b].value;
+		 } else {
+		     bitem = b[pos_b];
+		 }
+		 ret[ret.length] = bitem;
+	     }
+	 }
+	 return ret;
+    }
+
+// selectProduct reads the selection from f[productfield] and updates
+// f.version, component and target_milestone accordingly.
+//	- f: a form containing product, component, varsion and
+//	  target_milestone select boxes.
+// globals (3vil!):
+//	- cpts, vers, tms: array of arrays, indexed by product name. the
+//	  subarrays contain a list of names to be fed to the respective
+//	  selectboxes. For bugzilla, these are generated with perl code
+//	  at page start.
+//	- usetms: this is a global boolean that is defined if the
+//	  bugzilla installation has it turned on. generated in perl too.
+//	- first_load: boolean, specifying if it's the first time we load
+//	  the query page.
+//	- last_sel: saves our last selection list so we know what has
+//	  changed, and optimize for additions.
+
+function selectProduct( f , productfield, componentfield, blank ) {
+
+    // this is to avoid handling events that occur before the form
+    // itself is ready, which happens in buggy browsers.
+
+    if ( ( !f ) || ( ! f[productfield] ) ) {
+	 return;
+    }
+
+    // if this is the first load and nothing is selected, no need to
+    // merge and sort all components; perl gives it to us sorted.
+
+    if ( ( first_load ) && ( f[productfield].selectedIndex == -1 ) ) {
+	     first_load = 0;
+	     return;
+    }
+
+    // turn first_load off. this is tricky, since it seems to be
+    // redundant with the above clause. It's not: if when we first load
+    // the page there is _one_ element selected, it won't fall into that
+    // clause, and first_load will remain 1. Then, if we unselect that
+    // item, selectProduct will be called but the clause will be valid
+    // (since selectedIndex == -1), and we will return - incorrectly -
+    // without merge/sorting.
+
+    first_load = 0;
+
+    // - sel keeps the array of products we are selected.
+    // - is_diff says if it's a full list or just a list of products that
+    //   were added to the current selection.
+    // - single indicates if a single item was selected
+    var sel = Array();
+    var is_diff = 0;
+    var single;
+
+    // if nothing selected, pick all
+    if ( f[productfield].selectedIndex == -1 ) {
+	 for ( var i = 0 ; i < f[productfield].length ; i++ ) {
+	     sel[sel.length] = f[productfield].options[i].value;
+	 }
+	 single = 0;
+    } else {
+
+	 for ( i = 0 ; i < f[productfield].length ; i++ ) {
+	     if ( f[productfield].options[i].selected ) {
+		 sel[sel.length] = f[productfield].options[i].value;
+	     }
+	 }
+
+	 single = ( sel.length == 1 );
+
+	 // save last_sel before we kill it
+	     var tmp = last_sel;
+	 last_sel = sel;
+
+	 // this is an optimization: if we've added components, no need
+	 // to remerge them; just merge the new ones with the existing
+	 // options.
+
+	 if ( ( tmp ) && ( tmp.length < sel.length ) ) {
+	     sel = fake_diff_array(sel, tmp);
+	     is_diff = 1;
+	 }
+    }
+
+    // do the actual fill/update
+    updateSelect( cpts, sel, f[componentfield], is_diff, single, blank );
+}
diff -Nur --exclude=CVS --exclude=data bztip/request.cgi bz98801/request.cgi
--- bztip/request.cgi	Wed Dec 31 16:00:00 1969
+++ bz98801/request.cgi Wed May 15 17:30:47 2002
@@ -0,0 +1,332 @@
+#!/usr/bonsaitools/bin/perl -w
+# -*- Mode: perl; indent-tabs-mode: nil -*-
+#
+# The contents of this file are subject to the Mozilla Public
+# License Version 1.1 (the "License"); you may not use this file
+# except in compliance with the License. You may obtain a copy of
+# the License at http://www.mozilla.org/MPL/
+#
+# Software distributed under the License is distributed on an "AS
+# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+# implied. See the License for the specific language governing
+# rights and limitations under the License.
+#
+# The Original Code is the Bugzilla Bug Tracking System.
+#
+# The Initial Developer of the Original Code is Netscape Communications
+# Corporation. Portions created by Netscape are
+# Copyright (C) 1998 Netscape Communications Corporation. All
+# Rights Reserved.
+#
+# Contributor(s): Myk Melez <[e-mail]>
+
+##############################################################################
##
+# Script Initialization
+##############################################################################
##
+
+# Make it harder for us to do dangerous things in Perl.
+use diagnostics;
+use strict;
+
+# Include the Bugzilla CGI and general utility library.
+require "CGI.pl";
+
+# Establish a connection to the database backend.
+ConnectToDatabase();
+
+# Use Bugzilla's Request module which contains utilities for handling
requests.
+use Request;
+
+use vars qw(
+  $template
+  $vars
+);
+
+# Make sure the user is logged in.
+confirm_login();
+
+##############################################################################
##
+# Main Body Execution
+##############################################################################
##
+
+# All calls to this script should contain an "action" variable whose value
+# determines what the user wants to do.  The code below checks the value of
+# that variable and runs the appropriate code.
+
+# Determine whether to use the action specified by the user or the default.
+my $action = $::FORM{'action'} || 'enter';
+
+if ($action eq 'enter') {
+    validateTargetType();
+    validateTargetID();
+    enter();
+}
+elsif ($action eq 'create') { 
+    validateTargetType();
+    validateTargetID();
+    validateTypeIDs();
+    create(); 
+}
+else { 
+    # If the action that the user wants to take (specified in the "action" 
+    # form field) is none of the above listed actions, display an error 
+    # telling the user that we do not understand what they would like to do.
+    ThrowCodeError("I could not figure out what you wanted to do.");
+}
+
+##############################################################################
##
+# Data Validation / Security Authorization
+##############################################################################
##
+
+sub validateTargetType {
+    if ($::FORM{'targettype'} ne 'bug' 
+     && $::FORM{'targettype'} ne 'attachment')
+    {
+	 my $targettype = html_quote($::FORM{'targettype'});
+	 ThrowCodeError("The target type ($targettype) is invalid. Valid types
are
+	   <em>bug</em> and <em>attachment</em>.");
+    }
+}
+
+my $target;
+sub validateTargetID {
+    if ($::FORM{'targetid'} !~ /^[1-9][0-9]*$/) {
+	 my $htmltargetid = html_quote($::FORM{'targetid'});
+	 ThrowUserError("The $::FORM{'targettype'} ID ($htmltargetid) must be 
+	   a positive integer.");
+    }
+
+    $target = Request::GetTarget($::FORM{'targettype'}, $::FORM{'targetid'});
+    $target->{'exists'}
+      || ThrowUserError("There is no $::FORM{'targettype'} 
+			  with ID #$::FORM{'targetid'}.");
+
+    # Make sure the user is authorized to access the bug for this target.
+    ValidateBugID($target->{'bugid'});
+}
+
+sub validateTypeIDs {
+    for my $typeid (@{$::MFORM{'typeid'}}) {
+	 if ($typeid !~ /^[1-9][0-9]*$/) {
+	     my $typeid = html_quote($typeid);
+	     ThrowUserError("The request type ID <em>$typeid</em> must be 
+			     a positive integer.");
+	 }
+
+	 my $requesttype = Request::GetType($typeid);
+	 $requesttype->{'exists'}
+	   || ThrowUserError("There is no type of request 
+			      with the ID <em>$typeid</em>.");
+
+	 if ($requesttype->{'targettype'} ne $target->{'type'}) {
+	     my $htmlname = html_quote($requesttype->{'name'});
+	     ThrowUserError("You cannot file a request of type
<em>$htmlname</em>
+	       for $target->{'type'}s.	Requests of this type can only be filed
+	       for $requesttype->{'targettype'}s.");
+	 }
+
+	 if ($requesttype->{'targetproduct'} 
+	  && $requesttype->{'targetproduct'} ne $target->{'product'})
+	 {
+	     my $htmlname = html_quote($requesttype->{'name'});
+	     my $htmltypeproduct = html_quote($requesttype->{'targetproduct'});
+	     my $htmltargetproduct = html_quote($target->{'product'});
+	     ThrowUserError("You cannot file a request of type
<em>$htmlname</em>
+	       for $target->{'type'} #$target->{'id'} because that type of 
+	       request can only be filed for $requesttype->{'targettype'}s 
+	       belonging to the <em>$htmltypeproduct</em> product, and 
+	       $target->{'type'} #$target->{'id'} belongs to the 
+	       <em>$htmltargetproduct</em> product.");
+	 }
+
+	 if ($requesttype->{'targetcomponent'} 
+	  && $requesttype->{'targetcomponent'} ne $target->{'component'})
+	 {
+	     my $htmlname = html_quote($requesttype->{'name'});
+	     my $htmltypecomponent =
html_quote($requesttype->{'targetcomponent'});
+	     my $htmltargetcomponent = html_quote($target->{'component'});
+	     ThrowUserError("You cannot file a request of type
<em>$htmlname</em>
+	       for $target->{'type'} #$target->{'id'} because that type of 
+	       request can only be filed for $requesttype->{'targettype'}s 
+	       belonging to the <em>$htmltypecomponent</em> component, and 
+	       $target->{'type'} #$target->{'id'} belongs to the 
+	       <em>$htmltargetcomponent</em> component.");
+	 }
+    }
+}
+
+##############################################################################
##
+# Functions
+##############################################################################
##
+
+sub enter {
+    # Display a form allowing the user to enter requests for a given target
+    # (identified by the "targetid" form variable).  The target can be either 
+    # a bug or an attachment (specified in the "targettype" form variable).
+
+    # Retrieve information about the target.
+    my $target = Request::GetTarget($::FORM{'targettype'}, 
+				     $::FORM{'targetid'});
+
+    # Retrieve the list of requests that can be made for this target.
+    my $types = Request::GetTypes($target->{'type'}, 
+				   $target->{'product'}, 
+				   $target->{'component'}, 
+				   1);
+
+    if (!scalar(@$types)) {
+	 my $htmlproduct = html_quote($target->{'product'});
+	 my $htmlcomponent = html_quote($target->{'component'});
+	 ThrowUserError("Sorry, no request types are defined for 
+	   $target->{'type'}s in the <em>$htmlproduct</em> product and 
+	   <em>$htmlcomponent</em> component.");
+    }
+
+    # Define the variables and functions that will be passed to the UI
template.
+    $vars->{'target'} = $target;
+    $vars->{'types'} = $types;
+
+    # Return the appropriate HTTP response headers.
+    print "Content-type: text/html\n\n";
+
+    # Generate and return the UI (HTML page) from the appropriate template.
+    $template->process("request/create.html.tmpl", $vars)
+      || ThrowTemplateError($template->error());
+}
+
+
+sub create {
+    # Create a bug in the database for each request the user entered into
+    # the web form, or display another form if the users of which the requests
+    # are being made cannot be uniquely identified.
+
+    # Retrieve information about the target.
+    my $target = Request::GetTarget($::FORM{'targettype'}, 
+				     $::FORM{'targetid'});
+
+    # Convert the form data into an array of requests where each request
+    # is represented as a hash.  If the "must verify fulfillers" flag gets 
+    # set, then the user did not uniquely identify a fulfiller for one or 
+    # more of the requests.
+    my ($requests, $mustVerifyFulfillers) = 
+      convertFormFieldsToRequestObjects();
+
+    if ($mustVerifyFulfillers) {
+	 # Display a form for verifying the fulfiller of each request.
+	 verifyFulfillers($requests);
+    }
+    else {
+	 # Insert the requests into the database.
+	 foreach my $request (@$requests) { Request::create($request) }
+	 
+	 # Define the variables and functions that will be passed 
+	 # to the UI template.
+	 $vars->{'target'} = $target;
+	 $vars->{'requests'} = $requests;
+
+	 # Return the appropriate HTTP response headers.
+	 print "Content-type: text/html\n\n";
+
+	 # Generate and return the UI (HTML page) from the appropriate
template.
+	 $template->process("request/created.html.tmpl", $vars)
+	   || ThrowTemplateError($template->error());
+    }
+}
+
+
+sub convertFormFieldsToRequestObjects {
+    # Flag for whether or not we must request verification of the fulfillers
+    # (if the user did not uniquely identify them).
+    my $mustVerifyFulfillers = 0;
+
+    # Get information about the requester to add to each request.
+    my $use_userid_again_to_avoid_warning = $::userid;
+    my $requester = Request::GetUser($::userid);
+
+    # Get information about the target to add to each request.
+    my $target = Request::GetTarget($::FORM{'targettype'},
$::FORM{'targetid'});
+
+    # Process the form data and create an array of requests.
+    my @requests;
+    my $use_MFORM_typeid_again_to_avoid_warning = $::MFORM{'typeid'};
+    for my $typeid (@{$::MFORM{'typeid'}}) {
+	 # Only deal with requests for which the user entered a fulfiller,
+	 # since this is how the user specifies which requests they are making.
+	 $::FORM{"fulfillername-$typeid"} || next;
+
+	 # To reduce the size of queries, require the user to enter at least 
+	 # three characters of each fulfiller's name.
+	 if (length($::FORM{"fulfillername-$typeid"}) < 3)
+	 {
+	     my $htmlfulfillername = 
+	       html_quote($::FORM{"fulfillername-$typeid"});
+	     ThrowUserError("Each name fragment must be at least three
characters
+	       long.  Enter more of the name for
<em>$htmlfulfillername</em>.");
+	 }
+
+	 # Create the request record and populate it with data from the form.
+	 my $request = 
+	   { 
+	     type => Request::GetType($typeid) , 
+	     comments => $::FORM{"comments-$typeid"} , 
+	     fulfillername => $::FORM{"fulfillername-$typeid"} , 
+	     requester => $requester , 
+	     target => $target
+	   };
+
+	 # Get a list of potential fulfillers whose login name (email address) 
+	 # or real name matches the substring entered by the user
+	 $request->{'fulfillers'} = 
+	   Request::MatchUsers($::FORM{"fulfillername-$typeid"}, 101);
+	 
+	 # If there is only one fulfiller match, make them the fulfiller.
+	 # Otherwise, set the "must verify fulfillers" flag.
+	 if (scalar(@{$request->{'fulfillers'}}) == 1) {
+	     $request->{'fulfiller'} = $request->{'fulfillers'}[0];
+	 } 
+	 elsif (scalar(@{$request->{'fulfillers'}}) > 100) {
+	     my $htmlfulfillername =
html_quote($::FORM{"fulfillername-$typeid"});
+	     ThrowUserError("The name fragment <em>$htmlfulfillername</em>
matched
+	       more than 100 users.  Enter more of the name to bring the number
+	       of matches down to a more reasonable amount.");
+	 }
+	 else {
+	     $mustVerifyFulfillers = 1;
+	 }
+
+	 # Add the request to the array of requests.
+	 push @requests , $request;
+    }
+
+    # Return the list of requests along with whether or not the fulfillers
+    # must be verified.
+    return \@requests , $mustVerifyFulfillers;
+}
+
+
+sub verifyFulfillers {
+    my ($requests) = @_;
+
+    # Retrieve information about the target.
+    my $target = Request::GetTarget($::FORM{'targettype'}, 
+				     $::FORM{'targetid'});
+
+    # Retrieve the list of requests that can be made for this target.
+    my $types = Request::GetTypes($target->{'type'}, 
+				   $target->{'product'}, 
+				   $target->{'component'}, 
+				   1);
+
+    # Define the variables and functions that will be passed 
+    # to the UI template.
+    $vars->{'target'} = $target;
+    $vars->{'types'} = $types;
+    $vars->{'requests'} = $requests;
+
+    # Return the appropriate HTTP response headers.
+    print "Content-type: text/html\n\n";
+
+    # Generate and return the UI (HTML page) from the appropriate template.
+    $template->process("request/verify.html.tmpl", $vars)
+      || ThrowTemplateError($template->error());
+}
diff -Nur --exclude=CVS --exclude=data
bztip/template/en/default/attachment/edit.html.tmpl
bz98801/template/en/default/attachment/edit.html.tmpl
--- bztip/template/en/default/attachment/edit.html.tmpl Fri Apr 26 10:52:04
2002
+++ bz98801/template/en/default/attachment/edit.html.tmpl	Thu May 16
14:09:04 2002
@@ -34,6 +34,11 @@
     td#info { text-align: right; vertical-align: top; }
     td#actions { text-align: right; vertical-align: bottom; }
     td#noview { text-align: left; vertical-align: center; }
+    
+    /* The Requests table lists pending requests and allows users to grant
+     * or deny them.  For this screen it should be in a smaller font size.
+     */
+    table#requests tr > th, td { font-size: small; }
   "
 %]

@@ -169,6 +174,8 @@
	     [% END %]
	 [% END %]

+	 [% INCLUDE "request/list.html.tmpl" targettype="attachment"
targetid=attachid IF Param('userequesttracker') %]
+	 
	 <div id="smallCommentFrame">
	   <b>Comment (on the bug):</b><br>
	     <textarea name="comment" rows="5" cols="25"
wrap="soft"></textarea><br>
diff -Nur --exclude=CVS --exclude=data
bztip/template/en/default/attachment/list.html.tmpl
bz98801/template/en/default/attachment/list.html.tmpl
--- bztip/template/en/default/attachment/list.html.tmpl Thu Apr 18 11:56:30
2002
+++ bz98801/template/en/default/attachment/list.html.tmpl	Mon May 13
17:28:49 2002
@@ -19,6 +19,10 @@
   # Contributor(s): Myk Melez <[e-mail]>
   #%]

+[%# Whether or not to include a list of pending requests. %]
+[% display_requests = Param('userequesttracker') 
+     && (attachments_have_request_types || attachment.requests.size > 0) %]

When would attachment.requests.size be > 0 but attachments_have_request_types
be false?

+	 [% ", <a href='editrequesttypes.cgi'> request&nbsp;types</a>" 
+		  IF user.groups.editcomponents AND Param('userequesttracker') 

You need to be a member of "tweakparams", according to the CGI itself. There's
a mismatch here.

+<form method="post" action="request.cgi">
+  <input type="hidden" name="action" value="create">
+  <input type="hidden" name="targettype" value="[% target.type %]">
+  <input type="hidden" name="targetid" value="[% target.id %]">
+
+
+[% FOREACH type = types %]
+  <input type="hidden" name="typeid" value="[% type.id %]">

Here, you have multiple inputs of name "typeid". Is this intentional?

+[% PROCESS global/footer.html.tmpl %]
diff -Nur --exclude=CVS --exclude=data
bztip/template/en/default/request/created.html.tmpl
bz98801/template/en/default/request/created.html.tmpl
--- bztip/template/en/default/request/created.html.tmpl Wed Dec 31 16:00:00
1969
+++ bz98801/template/en/default/request/created.html.tmpl	Mon May 13
17:42:10 2002

template/en/default/bug/process/results.html.tmpl is a generic template for
this sort of thing ("something happens which causes some email".) Can you use
it here, at least within the loop:

+[% FOREACH request = requests %]

? This keeps all mail tables identical if that file gets customised.

+<p>
+Requests are just a special type of bug, and you can do anything with a
request
+you can do with a regular bug, including 
+<a href="buglist.cgi?bug_id=[% FOREACH r = requests %][% r.id %],[% END
%]">tracking these requests</a>
+using Bugzilla's <a href="query.cgi">standard query interface</a>.
+</p>

You're missing an "and" somewhere here.

+[% USE wrap -%]
+[%- FILTER bullet = wrap(80) -%]
+[% request.requester.ident %] has requested [% request.type.name %] for bug #
+  [%- request.target.bugid %]: [% request.target.bugsummary %]
+[%- IF request.target.type == 'attachment' %], attachment #
+  [%- request.target.id %]: [% request.target.summary %][% END %].
+
+[%+ IF request.target.type == 'bug' -%]
+  [% Param('urlbase') %]show_bug.cgi?id=[% request.target.id %]
+[%- ELSIF request.target.type == 'attachment' -%]
+  [% Param('urlbase') %]attachment.cgi?id=[% request.target.id %]&action=edit
+[%- END %]
+
+[%+ IF request.comments -%]
+Additional Comments from Requester:
+
+[%+ request.comments %]
+[%- END %]
+[%- END %]

One too many ENDs? Or does one of the USE or FILTER clauses require an END?

diff -Nur --exclude=CVS --exclude=data
bztip/template/en/default/request/fulfilled.html.tmpl
bz98801/template/en/default/request/fulfilled.html.tmpl
--- bztip/template/en/default/request/fulfilled.html.tmpl	Wed Dec 31
16:00:00 1969
+++ bz98801/template/en/default/request/fulfilled.html.tmpl	Mon May 13
17:35:35 

Same thing about sending-mail templates applies here.



My brain is too fried to continue. Anyway, that should give you something to
think about :-)

If we can agree to put the request stuff in template/en/default/request and the
requesttypes stuff in template/en/default/admin/requesttypes/, then you can put
them there, cvs add them and provide CVS diffs, which would be much easier. I
had to apply this diff by hand, as it contains some install directory names
which don't match my setup...

Gerv

Comment 52

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v12: review fixes

> Why not use the "use vars" pragma like everywhere else? Consistency is good.

This situation is different since the file is a module, so 
"use vars qw( $template $vars );" doesn't work here (at least, I haven't
figured out a way for it to work, and there aren't any other working 
examples in the codebase).


> +    $data{'bug_severity'} =
&::SqlQuote($request->{'target'}->{'component'});
> 
> Is this really what you mean?

Nope, good catch.  Fixed.


> ... unless you are filing requests in the same product as the bugs and it's a

> product-based group, in which case you can use the title. (This, I think, 
> might actually be a quite-common case.)

Fixed.


> +    # Return empty-handed if the request does not exist.
> +    (&::PopGlobalSQLState() && return) if !&::MoreSQLData();
> 
> Won't this return the return value of the last-executed statement, i.e.
> PopGlobalSQLState(), rather than the null which you might expect?

My sources say no, f.e. as demonstrated by the following perl snippet
(which prints nothing but would print 1 if what you suggest were true):
(perl -e 'sub foo { return 1 } sub bar { foo() && return } print bar();')


> +sub MatchUsers {
> +    # Generates a list of users whose login name (email address) or real
name
> +    # matches a substring.  Useful for finding the list of potential
> fulfillers
> +    # for a new request where the requester did not enter the full names
> +    # of fulfillers.
> 
> This function is useful for editusers.cgi also? Can it be put somewhere more
> accessible, so when the admin interface gets rewritten, we can use it?

Done by creating a User.pm.


> +sub GetUser {
> +    # Returns a hash of information about a particular user.
> 
> Can you use GetUserInfo() from CGI.pl for this?

No, because GetUserInfo only returns info about the current user, 
but these functions should be combined long-term.  In the meantime, 
I have moved GetUser into User.pm.


>  # Check whether or not the user is logged in and, if so, set the $::userid 
>  # and $::usergroupset variables.
>  quietly_check_login();
>  $vars->{'userid'} = $::userid;
> 
>  This gets set anyway, by GetUserInfo(), as $vars->{'user'}{'userid'}.

Looks like this isn't needed anymore.  Removed.


> +sub validateRequests
> +{
> +  my @requestids = map(/^request-(\d+)$/ ? $1 : (), keys %::FORM);
> 
> Cool, but complicated. It took me a while to figure this out. An explanatory
> comment, perhaps?

Done.  I said this:

  # Get a list of request IDS to validate.  Uses the "map" function
  # to convert the array of form field names into an array of IDs
  # by matching form fields whose name looks like "request-nnn",
  # where "nnn" is a request ID, and returning just the ID.


> Huh? :-) Is this stuff in current Bugzilla CVS?

Yup. :-)  I coded the original request tracker at the same time as 
the attachment tracker, and this commented-out code made it in somehow.


> Surely $bug{'has_request_types'}?

> Same here, I would say.

Both variables get passed through to a secondary template that has 
no knowledge of the "bug" being displayed, and whether or not there
are request types is more a function of the product and component
than the bug itself, so I think this makes more sense as a separate flag.


> +	 $vars->{'userid'} = $::userid;
> 
> Again, GetUserInfo() makes this available to you.

Looks like this isn't needed anymore.  Removed.


> <looks below> 
> Ah, you did. Comment needs updating :-)

Oops, done.


> +# into which request bugs will be filed (these can be null, in which case 
> +# the bugs will get filed into the same product and component as the target 

> +# bug/attachment), 
> 
> Presumably product cannot be null if component is non-null?

Correct.


> "request changes of status"? This makes it sound like users can now say "Can
> someone please confirm this bug? I don't have permissions..."

"Whether or not to use the request tracker that enables you to create 
 arbitrary types of requests by which users can ask other users to do
 something with a bug or an attachment (f.e. change its status).",


> editrequesttypes.cgi:
> 
> +else { 
> +    ThrowCodeError("I could not figure out what you wanted to do.");
> 
> "Unrecognised value of action variable"? Let's be specific here; it's a
> ThrowCodeError :-)

"I did not recognize the value of the <em>action</em> form variable."


> +    length($::FORM{'grantactions'}) <= 50
> +	 || ThrowUserError("The name of the file containing grant actions 
> +			    cannot be more than 50 characters long.");
> 
> Is this a filename or a relative path? 50 chars is reasonably short for a
> relative path...

It's a filename for security.


> +		$::FORM{'grantactions'} !~ /\//
> +	|| ThrowUserError("You can't have a directory separator (/) in 
> +			    the name of the file containing grant actions.");
> +}
> 
> Are there ramifications for Windows here? (denyactions also.) Also, can
> validateGrantActions and validateDenyActions be made a single, parameterised
> sub?

Added a backslash to the list of characters not allowed in filenames,
and made a single parameterized function.


> +sub validateTargetProduct {
> +    # An empty target product means targets in any product can have requests

> +    # of this type made for them.
> +    return if !$::FORM{'targetproduct'};
> +
> +    my $product = SqlQuote($::FORM{'targetproduct'});
> +    SendSQL("SELECT 1 FROM products WHERE product = $product");
> +    my ($productexists) = FetchSQLData();
> +
> +    $productexists
> +	 || ThrowCodeError("The target product you selected does not exist.") 
> +	 && exit;
> +}
> 
> And this and validateProduct? And the two component ones?

Done for products, but not for the two component functions, 
which have different needs.


> File locations. Given that you got yourself a top level directory for
> "attachment", I guess I can't complain about having one for "request". But
the
> "requesttypes" stuff should go as a subdirectory of "admin", like
> "attachstatus".

Done.


> +sub deleteType {
  ...
> Does it have a name you can give?

Nope, unfortunately.


> +# make_js_array: iterates through the product array creating a
  ...
> This looks generally useful - in query.cgi and your "make Bugzilla config
> available via JS" patch. If that's true, can we move it somewhere more 
> general?  Or should this stuff be done in a template anyway?

I'm not sure it should be done in a template, as it isn't content,
and I think we should wait until someone alters another script to use it
before deciding where to move it.  By that time we may want to move it
to a different place (since, f.e., CGI.pl and globals.pl may no longer exist).


> Please use DisplayTemplateError() throughout. (Then there's no need for
> "Template process failed".)

Done.


> +[%# Whether or not to include a list of pending requests. %]
> +[% display_requests = Param('userequesttracker') 
> +	&& (attachments_have_request_types || attachment.requests.size > 0) %]
> 
> When would attachment.requests.size be > 0 but attachments_have_request_types

> be false?

When an request type is deactivated before all pending requests of that type
are resolved.


> +	 [% ", <a href='editrequesttypes.cgi'> request&nbsp;types</a>" 
> +		  IF user.groups.editcomponents AND Param('userequesttracker') 

> 
> You need to be a member of "tweakparams", according to the CGI itself.
There's
> a mismatch here.

Fixed.


> +<form method="post" action="request.cgi">
> +  <input type="hidden" name="action" value="create">
> +  <input type="hidden" name="targettype" value="[% target.type %]">
> +  <input type="hidden" name="targetid" value="[% target.id %]">
> +
> +
> +[% FOREACH type = types %]
> +  <input type="hidden" name="typeid" value="[% type.id %]">
> 
> Here, you have multiple inputs of name "typeid". Is this intentional?

Yup.  The processing code expects the values as a list within that field.


> template/en/default/bug/process/results.html.tmpl is a generic template for
> this sort of thing ("something happens which causes some email".) Can you use

> it here, at least within the loop:
  ...
> ? This keeps all mail tables identical if that file gets customised.

This mail table is customized for requests, so I think it's sensible
to keep it separate.


> +<p>
> +Requests are just a special type of bug, and you can do anything with a
> request
> +you can do with a regular bug, including 
> +<a href="buglist.cgi?bug_id=[% FOREACH r = requests %][% r.id %],[% END
> %]">tracking these requests</a>
> +using Bugzilla's <a href="query.cgi">standard query interface</a>.
> +</p>
> 
> You're missing an "and" somewhere here.

I'm not sure what you mean.  Here's the sentence without the links; where
should
the "and" go?

"Requests are just a special type of bug, and you can do anything with a
request
you can do with a regular bug, including tracking these requests using
Bugzilla's
standard query interface."


> One too many ENDs? Or does one of the USE or FILTER clauses require an END?

The FILTER directive requires an end; "END" mistakes are easy to spot, since
the template doesn't run when they occur.


> +++ bz98801/template/en/default/request/fulfilled.html.tmpl	Mon May 13
  ...
> Same thing about sending-mail templates applies here.

The mail table has been customized, so it makes sense to keep it separate.


> My brain is too fried to continue. Anyway, that should give you something to
> think about :-)

Yes, quite a bit. :-)  Thanks for the excellent review.  Here's another
version!
Frankly, I don't know how code like this can be completely reviewed without
thorough testing in a semi-production environment, but that's another issue
we don't have to solve right now. :-)

> If we can agree to put the request stuff in template/en/default/request and 
> the requesttypes stuff in template/en/default/admin/requesttypes/, then you 
> can put them there, cvs add them and provide CVS diffs, which would be much 
> easier.

Dave delegated decision-making authority to us in IRC, and you and I agreed
in email to the following:

template/en/default/request/
template/en/default/admin/request-type/

Comment 53

[Gervase Markham [:gerv]]

OK, some more thoughts:

userequesttracker param should be next to other "use*" params in the Params
list, and its text should be in the same form.


"Administer Request Types" screen:

" Use this screen to define the types of requests that users can make. ". This
screen doesn't do this - the "create" screen does. I suggest removing this sentence.

It might be sensible to display the info here in two tables - one for
"attachment" requests and one for "bug" requests.

The descriptions of the example request types should begin "Example request type:".


"Create Request Types" screen

The use of italics here is unusual; we don't do this anywhere else. Why not use
Roman?

Target Type should be a radio button.


"Submit Requests for Attachment #XX" screen

This is a tricky UI problem. There's a balance between allowing people to submit
more than one request at once, and the greater simplicity of something like:

Ask [[e-mail]   ] to [ review (V)] attachment XX:
Comment:
[                                                 ]
[                                                 ]
[                                                 ]

Possible Actions:
review: review a patch
twiddle: twiddle an attachment

Did you choose the current form over something like the above consciously? The
singular form would also eliminate the ugly s in "Make New Request(s)" on the
bug itself. <later> I keep coming across areas where this would be a conceptual
simplification. I'm more and more convinced it's worth making.


Using the tracker

When the request is created, the URL added to the comment (for the attachment)
gets line broken at 80 chars, and so doesn't work. Is there a way around this?

The comment that gets added should contain a line or two of additional
instructions, e.g.:
"Resolve the bug FIXED to grant the request, or WONTFIX to deny it."

(Sidenote: what happens if someone later reopens a request?)

Back on bug XX: it would be good to make the requests UI look a bit less like
the attachments UI. There is definitely opportunity for confusion here. Could we
use colour in some way?  

I added a request to an attachment, and then fulfilled it. Now, the original bug
and attachment have no record at all of the request ever having existed. Should
there not be a "fulfilled requests" column, for attachments at least? Or is this
where the external Perl comes in to add attachment statuses instead? Same
applies for bug request fulfilments.

Linking attachment request resolutions with attachment status changes may well
be a common thing for administrators to want. Can we provide sample code for
people who want to do that?

Bug requests: why include the full URL in the comment? The "bug XXXX" gets
autolinkified. It's fine for attachment requests, where the "attachment XX" gets
autolinkified to view, and the URL you add is edit, but it's redundant for bug
requests.

There is UI in a bug to directly fulfil bug requests, but not attachment
requests. Why is that? It seems inconsistent. Ah, I see - it's on the Edit
Attachment screen. There needs to be a bit of space both above and below this UI
on the Edit Attachments screen.

When an attachment request is submitted, there should be a link back to the bug
of which it is a part, as well as the attachment and the request.

Does only the person who has been requested get that UI? I can see a situation
where patch submitter asks A to review a patch, but B gets there first. If he
has no UI, he just puts "r=B", and then the request is still outstanding, which
isn't ideal. Is there a good solution to this?

I think we can afford "Request" rather than "Req" in the attachments UI. It's
not much longer, but a whole lot clearer.

You appear to have JS-only delete confirmation in list.html.tmpl.

Gerv

Comment 54

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v13: review fixes

> OK, some more thoughts:
> 
> userequesttracker param should be next to other "use*" params in the Params
> list, and its text should be in the same form.

Done.


> "Administer Request Types" screen:
> 
> " Use this screen to define the types of requests that users can make. ".
This
> screen doesn't do this - the "create" screen does. I suggest removing this 
> sentence.

Done.


> It might be sensible to display the info here in two tables - one for
> "attachment" requests and one for "bug" requests.

Done.  I also removed the sort key column from the table (since it was
redundant with the order in which the rows appear) and removed the short
descriptions of each field, since they distract from the main content of the
page, and since the fields are much better described on the "create" and "edit"
pages.


> The descriptions of the example request types should begin "Example request 
> type:".

Where are these?


> "Create Request Types" screen
> 
> The use of italics here is unusual; we don't do this anywhere else. Why not 
> use Roman?

We also do this on the "create attachment" page.  It's a useful way to
distinguish instructions from field titles.


> Target Type should be a radio button.

Done.


> "Submit Requests for Attachment #XX" screen
> 
> This is a tricky UI problem. There's a balance between allowing people to 
> submit more than one request at once, and the greater simplicity of something

> like:
  ...
> Did you choose the current form over something like the above consciously?

Yes, because people regularly ask for review, super-review, and approval at the
same time.  The current way allows them to do that more quickly than if they
have to enter each request separately.	I like the simplicity of your way a
lot, but I'm not sure it's worth sacrificing the ability to enter more than one
request at the same time.  Maybe there's a future enhancement (perhaps using
JavaScript) that can combine the best of both worlds.


> Using the tracker
> 
> When the request is created, the URL added to the comment (for the
attachment)
> gets line broken at 80 chars, and so doesn't work. Is there a way around
this?

I think there's a bug about not breaking long URLs, but I can't seem to find
it.


> The comment that gets added should contain a line or two of additional
> instructions, e.g.:
> "Resolve the bug FIXED to grant the request, or WONTFIX to deny it."

I don't want to encourage people to think about resolving the bug "FIXED" or
"WONTFIX"; I want them to think about fulfilling the request by "granting" or
"denying" it.  Long-term we need a new "show_bug" interface for requests along
with the custom resolutions patch.


> (Sidenote: what happens if someone later reopens a request?)

If they reopen a request, the request reappears in the bug/attachment and is
once again available for fulfillment.


> Back on bug XX: it would be good to make the requests UI look a bit less like

> the attachments UI. There is definitely opportunity for confusion here. Could

> we use colour in some way?  

Hmm, I'm not sure what the best way to differentiate these would be.  Color
might work, but the more colors we use the more difficult it is to maintain a
matching color scheme (which is why I chose grey for the header bar--it matches
the black and white very easily).


> I added a request to an attachment, and then fulfilled it. Now, the original 

> bug and attachment have no record at all of the request ever having existed. 

> Should there not be a "fulfilled requests" column, for attachments at least? 

> Or is this where the external Perl comes in to add attachment statuses 
> instead? Same applies for bug request fulfilments.

It's where the external Perl comes in, plus it's possible to run a query for
fulfilled requests for a given bug/attachment, although perhaps this could be
easier.


> Linking attachment request resolutions with attachment status changes may
well
> be a common thing for administrators to want. Can we provide sample code for
> people who want to do that?

Here's some sample code that sets the "foo" status flag if it isn't already
set:

grep("foo", @{$::MFORM{'status'}}) || push(@{$::MFORM{'status'}}, "foo");


> Bug requests: why include the full URL in the comment? The "bug XXXX" gets
> autolinkified. It's fine for attachment requests, where the "attachment XX" 
> gets autolinkified to view, and the URL you add is edit, but it's redundant 
> for bug requests.

Good point, fixed.


> There is UI in a bug to directly fulfil bug requests, but not attachment
> requests. Why is that? It seems inconsistent. Ah, I see - it's on the Edit
> Attachment screen. There needs to be a bit of space both above and below this

> UI on the Edit Attachments screen.

Done.


> When an attachment request is submitted, there should be a link back to the 
> bug of which it is a part, as well as the attachment and the request.

Done.


> Does only the person who has been requested get that UI? I can see a
situation
> where patch submitter asks A to review a patch, but B gets there first. If he

> has no UI, he just puts "r=B", and then the request is still outstanding, 
> which isn't ideal. Is there a good solution to this?

Everyone gets the UI when a request has been made, since everyone with editing
privileges has the ability to fulfill a request.  If B gets there first, B can
fulfill the request at the same time he types r=B.


> I think we can afford "Request" rather than "Req" in the attachments UI. It's

> not much longer, but a whole lot clearer.

Done.


> You appear to have JS-only delete confirmation in list.html.tmpl.

*grumble* *growl* *complain* Fixed. :-)

Comment 55

[Gervase Markham [:gerv]]

> > The descriptions of the example request types should begin "Example request 
> > type:".
> 
> Where are these?

<ahem> These would be the request types I added last time I looked at this
patch. Ignore this comment :-)

> > The use of italics here is unusual; we don't do this anywhere else. Why not 
> > use Roman?
> 
> We also do this on the "create attachment" page.  It's a useful way to
> distinguish instructions from field titles.

I can see the argument from consistency, but it's hard on the eyes. Elsewhere we
e.g. embolden the field titles rather than change the text. Could we do that
here (and file a follow-up bug to fix Create Attachment)? <looks> We seem to be
doing that here anyway. I think Roman would be better for the body text. There
would be no confusion.

> (Re: the UI for adding requests)
> Yes, because people regularly ask for review, super-review, and 
> approval at the same time.
> I like the simplicity of your way a lot, but I'm not sure it's worth
> sacrificing the ability to enter more than one request at the same time.

They should definitely not be asking for approval without review and
super-review, and we shouldn't encourage that. There's also a strong argument
that they shouldn't be asking for super-review without review, either. This
means they should never be asking for > 1 at once. 

Perhaps you could talk this one through with Brendan and Asa, and try and decide
whether people should be filing multiple requests on the same bug/attachment at
once and, if so, whether it would be common enough to require a more complex UI
(rather than a "submit another request"-style link on the results page, which is
what we do with other similar things elsewhere in Bugzilla.)

> I think there's a bug about not breaking long URLs, but I can't seem to find
> it.

Isn't your [%- FILTER bullet = wrap(80) -%] in description.txt.tmpl causing this
problem?

> Hmm, I'm not sure what the best way to differentiate these would be.  Color
> might work, but the more colors we use the more difficult it is to maintain a
> matching color scheme (which is why I chose grey for the header bar--it
> matches the black and white very easily).

How about moving it under the CC box instead? It's not that wide, so it would
go. Alternatively, use a different (lighter?) shade of grey for the header.

Another line of thought: is this patch supposed to help eliminate
nsbeta1/nsbeta1-/nsbeta1+ keyword bloat? Because it could do that job really
well, if the results of a request were made visible after it had been granted,
rather than it just disappearing as now.

You would ask [e-mail] for nsbeta1 on bug X. While you were waiting,
everyone could see nsbeta1 was requested. (Equivalent of nsbeta1 keyword.) If it
were given, the request would be granted (equivalent of nsbeta1+), and is not
given, the request would be refused (nsbeta1-). These statuses would be seen on
the bug under "Fulfilled Requests". If you renominated later, you'd reopen the
request.

This would have several advantages:
- massively cut down on keyword bloat
- reduce the need for people to write the extra Perl stuff; if all they wanted
was a "yes/no" style flag on the bug or attachment (like this) then they'd get
one because the UI would show the results of the request.

> Here's some sample code that sets the "foo" status flag if it isn't already
> set:

Looking at that, how to write these _definitely_ needs documenting :-)


OK, some more review-y thoughts:

Suggestion: don't linkify the names of the Pending Requests for attachments in
the attachments table on the view bug page. Why? Because people will click it,
and get used to fulfilling requests by directly editing the bug. If it wasn't
linked, they'd click Edit ("Change something about the attachment") and see the
correct UI for messing with it. Access to the underlying bug is still available
from the Edit Attachment screen.

The bug page says "Make New Request(s)" and the Edit Attachment page says
"Create New Request(s)". Consistency, please :-)

editrequesttypes.cgi:

Are request types (for bugs and attachments) not going to be per-product? If
Bugzilla wants a "twiddle" request type, the entire Mozilla project has to see
it at the moment. Other things of this sort (e.g. attachment statuses) are
per-product. (At first I thought the "Product" and "Component" lines in
editrequesttypes.cgi were for this, but then I realised.) <looks more closely>
Ah - you do have Target Product and Target Component. Great. a) Do we need
Target Component? b) These need to be in the table on editrequesttypes.cgi.

This is all a bit confusing. Rename "Product" as "Filing Product", or something
like that. Same for "Component".

Can Description contain HTML? If so, say so.

It's currently possible to select an invalid Product/Component pair.

"data/request" should be <tt>.

selectProduct() needs to run onload(), otherwise it's possible to go back,
forwards, and then select an invalid Component for the current Product.

Unify {Target Product, Target Component} and {Product, Component} under one
description. 
"Select the product and component...".

"grants/denies _a_ request" -> "grants/denies a request of this type".

Selecting e.g. the following combination makes no sense:
Product: FoodReplicator
Component: Same
Target Product: Any
Target Component: Any
because if you file a request on a bug in WorldControl/EconomicControl, it'll
try and file the request in FoodReplicator/EconomicControl, which doesn't
necessarily exist. 

I think the relationship between these four fields needs careful thought. Would
it be simpler to limit it to just two options: "Same product and component as
bug" or "This specified product _and_ component"? Radio button for the if, with
dropdowns if the second option is chosen.

<later> I notice illegal combinations give an error; but we should change the UI
to make it not possible to specify one. I still think the above is a good
simplification.

Also, there's a bug here - you are using target_product for the field name in
the template, but $::FORM{'targetproduct'} in the CGI in several places. I
suggest removing the underscore for consistency with database field names.

By clicking Copy and then Create immediately, I managed to create two
requesttypes with exactly the same name and attributes. Request type names
should be unique for a given scope of target product and component (although I
don't envy you writing the code to enforce this...)

Nit: "Note that you can deactivate the type instead of deleting it by unchecking
the "is active" flag while editing it. "
-> "Note: to deactivate the type instead of deleting it, edit it and uncheck its
"is active" flag."

Nit: The JS delete confirm message could do with a couple of "\n"s before the
final question.

Perhaps we could have two "create new request" links on this page, one per table
- the only difference would be the initial state of the Target Type radio
button. It would be conceptually more correct; I can't help thinking the current
link is only for attachment request types, because it's at the bottom.

Code Read

Request.pm

Looking at the groups stuff, I'm still not convinced. :-) If the target bug is
in a non-product group (e.g. "security"), we should simply put the request into
the same group. I don't think we do that right now.

Side thought: are PushGlobal...() and PopGlobal...() cheap if there's no SQL
state? We seem to be calling them a lot, almost certainly sometimes when we
don't need to.

Use of WHERE 0 = 0 - this is a bit of a mental stumbling block, at least for me,
because I think of WHERE 0, which is different :-) Can we use WHERE 1 = 1 ?

editcomponents.cgi/editproduct.cgi:

Should the "delete" warning contain some text about nuking request types?

Gerv

[Note: no attachment status change, because I want my review comments (prepared
in a text editor) to wrap properly, and they won't if I do them on the edit
attachment page.]

Comment 56

[Håkan Waara]

> They should definitely not be asking for approval without review and
> super-review, and we shouldn't encourage that. There's also a strong argument
> that they shouldn't be asking for super-review without review, either. This
> means they should never be asking for > 1 at once. 

I agree; people should not be requesting r and sr at the same time - a r= is
supposed to cover the basic part of a patch, while a super-review is different
in that it looks at the more fine details of the patch, while still keeping in
mind that the code should be compatible with the rest of the code base.  I think
you should need to have a r= in order to request sr= (and as far as I have
noticed, it is in this order people ask anyways).

Comment 57

[Jouni Heikniemi]

>I agree; people should not be requesting r and sr at the same time - a r= is
>supposed to cover the basic part of a patch, while a super-review is different
>in that it looks at the more fine details of the patch, while still keeping in
>mind that the code should be compatible with the rest of the code base.  I think
>you should need to have a r= in order to request sr= (and as far as I have
>noticed, it is in this order people ask anyways).

... but you should also remember that this feature will have use even outside
the Mozilla organization, and it's not always review people are asking for. So,
the possibility of asking many things at once is good. If it needs to be hidden
on b.m.o, that's another thing.

Comment 58

[Gervase Markham [:gerv]]

We don't have UI for changing the status on multiple patches at once (obsoletion
excepted) - this is something you could probably invent some scenario to use as
well. 

My point is that the UI and conceptual simplification which results from a
one-request-at-once model outweighs the possible speed increases for a few
people who want to issue multiple requests at the same time. I mean, the speed
increase is minimal anyway, really - one or two extra clicks for the second and
subsequent requests.

Gerv 

Comment 59

[Gervase Markham [:gerv]]

Another analogy I thought of - we don't let people attach multiple patches from
the same page, and that's a reasonably common operation (e.g. for testcases.)

Gerv

Comment 60

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v14: review fixes

> I can see the argument from consistency, but it's hard on the eyes. Elsewhere
we
> e.g. embolden the field titles rather than change the text. Could we do that
> here (and file a follow-up bug to fix Create Attachment)? <looks> We seem to
be
> doing that here anyway. I think Roman would be better for the body text.
There
> would be no confusion.

I still think italics (or something else other than roman) are the way to go,
but let's duke this out in another bug.  Italics removed.


> > (Re: the UI for adding requests)
> > Yes, because people regularly ask for review, super-review, and 
> > approval at the same time.
> > I like the simplicity of your way a lot, but I'm not sure it's worth
> > sacrificing the ability to enter more than one request at the same time.
> 
> They should definitely not be asking for approval without review and
> super-review, and we shouldn't encourage that. There's also a strong argument

> that they shouldn't be asking for super-review without review, either. This
> means they should never be asking for > 1 at once. 

I'm beginning to see your point, not because reviews are in-lined but for
simplicity's sake.  Also, the current interface leaves something to be desired
usability-wise, and it's easier to add multi-creates than to remove them later.
 Fixed in a way that makes it possible to add a format implementing multiple
simultaneous requests without any code changes.


> > I think there's a bug about not breaking long URLs, but I can't seem to
find
> > it.
> 
> Isn't your [%- FILTER bullet = wrap(80) -%] in description.txt.tmpl causing
this
> problem?

So it is.  Hmm, the solution is to fix the Template Toolkit to wrap long
unbroken lines correctly.  We might be able to work around the problem in the
meantime by not using TT to format the comment, although that would suck.

> How about moving it under the CC box instead? It's not that wide, so it would

> go. Alternatively, use a different (lighter?) shade of grey for the header.

Hmm, not a bad idea.  Moved in this version.


> Another line of thought: is this patch supposed to help eliminate
> nsbeta1/nsbeta1-/nsbeta1+ keyword bloat? Because it could do that job really
> well, if the results of a request were made visible after it had been
granted,
> rather than it just disappearing as now.

At one point I had requests show up with pluses and minuses to indicate whether
they had been granted or denied, but that duplicated attachment statuses, so I
gave that up.  It's a good idea, and it would be good to get rid of all the
crazy keyword bloat, but I'm not sure how to integrate the two different kinds
of requests.


> Suggestion: don't linkify the names of the Pending Requests for attachments
in
> the attachments table on the view bug page. Why? Because people will click
it,
> and get used to fulfilling requests by directly editing the bug. If it wasn't

> linked, they'd click Edit ("Change something about the attachment") and see
the
> correct UI for messing with it. Access to the underlying bug is still
available
> from the Edit Attachment screen.

Good idea, done.


> The bug page says "Make New Request(s)" and the Edit Attachment page says
> "Create New Request(s)". Consistency, please :-)

Done: "Make a Request" sounds the most natural.


> editrequesttypes.cgi:
> 
> Are request types (for bugs and attachments) not going to be per-product? If
> Bugzilla wants a "twiddle" request type, the entire Mozilla project has to
see
> it at the moment. Other things of this sort (e.g. attachment statuses) are
> per-product. (At first I thought the "Product" and "Component" lines in
> editrequesttypes.cgi were for this, but then I realised.) <looks more
closely>
> Ah - you do have Target Product and Target Component. Great. a) Do we need
> Target Component? b) These need to be in the table on editrequesttypes.cgi.
> 
> This is all a bit confusing. Rename "Product" as "Filing Product", or
something
> like that. Same for "Component".

Done.

> Can Description contain HTML? If so, say so.

Nope.

> It's currently possible to select an invalid Product/Component pair.

How so?


> "data/request" should be <tt>.

Done.

> selectProduct() needs to run onload(), otherwise it's possible to go back,
> forwards, and then select an invalid Component for the current Product.

Done.


> Unify {Target Product, Target Component} and {Product, Component} under one
> description. 
> "Select the product and component...".

Done.


> "grants/denies _a_ request" -> "grants/denies a request of this type".

Done.


> Selecting e.g. the following combination makes no sense:
> Product: FoodReplicator
> Component: Same
> Target Product: Any
> Target Component: Any
> because if you file a request on a bug in WorldControl/EconomicControl, it'll

> try and file the request in FoodReplicator/EconomicControl, which doesn't
> necessarily exist. 
> 
> I think the relationship between these four fields needs careful thought.
Would
> it be simpler to limit it to just two options: "Same product and component as

> bug" or "This specified product _and_ component"? Radio button for the if,
with
> dropdowns if the second option is chosen.

The problem with that is if an installation sets up f.e. a "Requests" component
in each of their products and specifies product=same, component=Requests.


> <later> I notice illegal combinations give an error; but we should change the
UI
> to make it not possible to specify one. I still think the above is a good
> simplification.

It would probably work for b.m.o, but perhaps not for others. Ludovic Dubost
was the one who originally requested the "same product/component" feature.  I
wonder what he thinks about it.


> Also, there's a bug here - you are using target_product for the field name in

> the template, but $::FORM{'targetproduct'} in the CGI in several places. I
> suggest removing the underscore for consistency with database field names.

Bug fixed.  The underscore is how I merged validate*** functions in response to
your previous review comment.


> By clicking Copy and then Create immediately, I managed to create two
> requesttypes with exactly the same name and attributes. Request type names
> should be unique for a given scope of target product and component (although
I
> don't envy you writing the code to enforce this...)

Heh.  The perils of Copying.  I hope this isn't a review blocker.


> Nit: "Note that you can deactivate the type instead of deleting it by
unchecking
> the "is active" flag while editing it. "
> -> "Note: to deactivate the type instead of deleting it, edit it and uncheck
its
> "is active" flag."

Done


> Nit: The JS delete confirm message could do with a couple of "\n"s before the

> final question.

Done.


> Perhaps we could have two "create new request" links on this page, one per
table
> - the only difference would be the initial state of the Target Type radio
> button. It would be conceptually more correct; I can't help thinking the
current
> link is only for attachment request types, because it's at the bottom.

Done.


> Looking at the groups stuff, I'm still not convinced. :-) If the target bug
is
> in a non-product group (e.g. "security"), we should simply put the request
into
> the same group. I don't think we do that right now.

Sure we do, on line 175 of Request.pm.


> Side thought: are PushGlobal...() and PopGlobal...() cheap if there's no SQL
> state? We seem to be calling them a lot, almost certainly sometimes when we
> don't need to.

As far as I can tell they are very cheap, and in my experience they have often
become necessary (meaning bugs were uncovered that they were needed to fix) as
code gets modified and reused over time.


> Use of WHERE 0 = 0 - this is a bit of a mental stumbling block, at least for
me,
> because I think of WHERE 0, which is different :-) Can we use WHERE 1 = 1 ?

Done.


> editcomponents.cgi/editproduct.cgi:
> 
> Should the "delete" warning contain some text about nuking request types?

What would you suggest?

Comment 61

[Gervase Markham [:gerv]]

> So it is.  Hmm, the solution is to fix the Template Toolkit to wrap long
> unbroken lines correctly.  We might be able to work around the problem in the
> meantime by not using TT to format the comment, although that would suck.

I won't block the review for this; because it won't be a problem on b.m.o or
most installations with a sensible length of base URL. But we should file a
separate bug on this.

> At one point I had requests show up with pluses and minuses to indicate
> whether they had been granted or denied, but that duplicated attachment
> statuses, so I gave that up.

Maybe this is a hint that we did attachment statuses wrong, and we should have
done them as resolved requests? :-) Perhaps, given that we don't have "bug
statuses", we could implement this for bug requests only? Anyway, we've run out
of time, and this seems to be addable later.

> The problem with that is if an installation sets up f.e. a "Requests" 
> component in each of their products and specifies product=same,
> component=Requests.

Ideally, then, we'd only display the components whose names existed in all
products. But maybe we need to rely on the admins to get this one right ;-)

> Heh.  The perils of Copying.  I hope this isn't a review blocker.

Could we do something like: store the fact that it's a copy, and the ID of the
request type it's a copy of, in hidden fields in the create page, and do a
comparison on submit? It wouldn't stop someone creating a copy by hand, but it
would help with the common case...

> > editcomponents.cgi/editproduct.cgi:
> > 
> > Should the "delete" warning contain some text about nuking request types?
> 
> What would you suggest?

Line 608: add "relevant request types, ". Also, around line 550: "In addition,
request types specific to this component will also be deleted."

A side note: Everything else in Bugzilla is granular to product level. Is there
a need to make it so that request types can be configured to component
granularity? I suppose that there's an argument that, now you've done it, we
might as well keep it, but it is inconsistent, in a way.


editrequesttypes.cgi:

Target before Filing makes more sense to me. Also, put a space either side of
the colon; it rather gets lost in proportional fonts otherwise. Alternatively,
replace the colon with a /, as in the title of the column.

"Create Bug Request Type" rather than "Create Bug Request". Same for Attachments.

request.cgi:

Make new request: can the Possible Requests become a table, or have the request
names emboldened, or be otherwise enhanced for readability?

Can we avoid displaying the "make a request for this bug" UI on bugs which are
requests? I know eventually we'll have a separate view, but this seems worth
doing now, if it can be. Having it there seems rather silly.

+    # If the action that the user wants to take (specified in the "action" 
+    # form field) is none of the above listed actions, display an error 
+    # telling the user that we do not understand what they would like to do.
+    ThrowCodeError("I could not figure out what you wanted to do.");

Another one of these wooly ones ;-) Can we say what the dodgy action field was,
etc.?

OK, I've had enough. Consider and fix this lot, then stick it up on b.m.o and
see what breaks :-)

Gerv

Comment 62

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v15

>Could we do something like: store the fact that it's a copy, and the ID of the

>request type it's a copy of, in hidden fields in the create page, and do a
>comparison on submit? It wouldn't stop someone creating a copy by hand, but it

>would help with the common case...

Hmm, that might work.  Not done in this patch, though.

>Line 608: add "relevant request types, ". Also, around line 550: "In addition,

>request types specific to this component will also be deleted."

Done.

>A side note: Everything else in Bugzilla is granular to product level. Is
there
>a need to make it so that request types can be configured to component
>granularity? I suppose that there's an argument that, now you've done it, we
>might as well keep it, but it is inconsistent, in a way.

Good question.

>Target before Filing makes more sense to me. Also, put a space either side of
>the colon; it rather gets lost in proportional fonts otherwise. Alternatively,

>replace the colon with a /, as in the title of the column.

Done.

>"Create Bug Request Type" rather than "Create Bug Request". Same for
Attachments.

Done.

>Make new request: can the Possible Requests become a table, or have the
request
>names emboldened, or be otherwise enhanced for readability?

Done.

>Can we avoid displaying the "make a request for this bug" UI on bugs which are

>requests? I know eventually we'll have a separate view, but this seems worth
>doing now, if it can be. Having it there seems rather silly.

Done.

>Another one of these wooly ones ;-) Can we say what the dodgy action field
was,
>etc.?

Done.

Comment 63

[Gervase Markham [:gerv]]

Comment on attachment 87156 [details] [diff] [review]
patch v15

OK, go for it :-) When creating request types, try and avoid creating
component-specific ones.
Then, if we decide to remove that ability, you won't get stuffed.

Good night all :-)

Gerv

Comment 64

[Bradley Baetz (:bbaetz)]

The css for requests in the attachment edit screen has:

/* The Requests table lists pending requests and allows users to grant
 * or deny them.  For this screen it should be in a smaller font size.
 */
  table#requests tr > th, td { font-size: small; }

This causes all the |td|s to match. You want:

table#requests tr > th, table#requests tr > td { font-size: small; }

I think - there may be a better way.

Comment 65

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v16: stylesheet correction

bbaetz pointed out that the stylesheet I was using was incorrectly resizing all
table cells, not just the ones I wanted to be sized.  That problem is fixed in
this patch.

Comment 66

[Jouni Heikniemi]

I was considering reviewing this, but didn't get very far yet. I'll note this
one particular problem here so I won't forget it.

IE 5 gives a Javascript error on editrequesttypes.cgi?action=enter&target=bug on
line 35: "'forms' is undefined.".

Comment 67

[Bugzilla Reviewers]

[edited IRC log with things i need to fix]

<bbaetz> myk: you need to patch request.cgi
<bbaetz> just like, um
<bbaetz> bug 110980, rather
<ssdbot> bbaetz: Bug http://bugzilla.mozilla.org/show_bug.cgi?id=110980 nor, P2,
Bugzilla 2.16, [e-mail], RESO FIXED, no email to CC list when opening
new bug

<bbaetz> myk: also, looking at that patch, some of the admin stuff is missing
FILTER html

<bbaetz> why does it ask me to "Verify the users of whom you are making
requests." this time?
<bbaetz> myk: the interface is confusing
<myk> bbaetz: what part of the interface is confusing?
<bbaetz> myk: the fact that it wants to you 'verify', but doesnt' tell you whats
wrong
<bbaetz> I tried entering two emails to test it, and it asked me to verify
<bbaetz> I did (they were both right, after all)
<bbaetz> and it just gave me back the same screen

<bbaetz> myk: oh, btw
<bbaetz> doing 'use Request' bease don a param won't work
<bbaetz> since use is compile time
<bbaetz> it will always be brought in

Comment 68

[Bradley Baetz (:bbaetz)]

Also, http://bugzilla.mozilla.org/request.cgi (with no paramaters) gives an error.

Comment 69

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v17: fixes bbaetz' issues and updated for tip

1. Adapts the fix in bug 110980 so the QA contact etc. get emailed when the
request bug is created.
2. Adds FILTER html to some of the admin stuff.
3. Clears up the "verify fulfiller" interface.
4. Throws a more meaningful error message when request.cgi is called without
parameters.

Comment 70

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v18: fixes errant reference to "requestdefs" instead of "requesttypes"

Comment 71

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v19: includes comment when verifying fulfiller

The last patch didn't include the request comment when verifying who is
supposed to fulfill a newly submitted request.	This patch rectifies that
problem.

Comment 72

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v20: fixes "exact name subset of other name" problem

When a name (f.e. "[e-mail]") is a subset of another name (f.e.
"[e-mail]") the previous version of the request tracker would
eternally report that the name was not unique.	This version checks to see if
the name the requester submitted is an exact match before getting a list of
possible matches.

Comment 73

[Bradley Baetz (:bbaetz)]

See http://bugzilla.mozilla.org/show_activity.cgi?id=157834 where the version
goes from '' to 2.10 - you need to set the version when you create the 'bug'.

Also, if you attach a new versoin of the patch, shouldn't the review request
carry on automatically, rather than having to request a new one manually?

Comment 74

[Tobias Burnus]

Is there any way to go from a Bug to its associated reviewed bug?
Clicking on the edit link (if available) or on 'request' or reading the comments
or looking at changes doesn't bring you closer.

Suggestions: Make the "review" in "Pending Requests" a hyperlink to the request
tracer bug.

Comment 75

[Joel Peshkin]

Here's an interesting case...  look at bug 143826.   When a review request is
pending for a patch that gets marked obsolete, something should probably happen.
(Mark the review request resolved??  Shift the pending review to the new patch??)

Comment 76

[Myk Melez [:myk] [@mykmelez]]

*** Bug 158274 has been marked as a duplicate of this bug. ***

Comment 77

[Gervase Markham [:gerv]]

If a patch is attached which obsoletes a patch with an outstanding request, the
request should shift to that patch. I think this will be the right thing in
almost all cases.

This also allows a "pre-review" stage, whereby the reviewer gives some initial
comments without a full review, the patch author adds a new patch, and the
review request remains valid as it shifts.

Gerv

Comment 78

[Jouni Heikniemi]

We should also have the possibility to search for requests/non-requests (query
screen), at least in the advanced query section.

Comment 79

[Tobias Burnus]

Coming back to my comment 74.
The problem is: If you are not in the canedit group you cannot easily follow the
discussion regarding one patch since the link to the request is on the 
attachment.cgi?action=edit page to which leads no link if the user is not in
canedit.

Comment 80

[Gervase Markham [:gerv]]

The "Sorry, I can't find a user whose name or email address contains
bbaetz@usyd." message should be in red. Otherwise, it looks very much like the
instruction line it replaces.

Gerv

Comment 81

[Joel Peshkin]

Another odd way to break the tracker :-)

Request a review and do not specify a reviewer.

Tracker will not complain, but no request will be generated.

Comment 82

[Myk Melez [:myk] [@mykmelez]]

After thinking about it for a while and investigating the options, I must
reluctantly agree with bbaetz that using bugs for requests has led us down the
wrong path.  The potential and actual difficulties of this approach are greater
than I anticipated, while the benefits I expected to gain are fewer and less
significant.

I anticipated two primary benefits from reusing the bug infrastructure for
requests, reduced coding time and a larger feature set.  The former hasn't
happened.  If anything, coding requests as bugs has turned out to take longer
because it means hacking code to do things it was never intended to do.  Of the
features that remain to be added, some (like a custom view for requests) are
probably easy, while others (like custom emails) are likely to be much harder to
hack into the current codebase.

Bugs as requests does bring with it a larger feature set, on the other hand, but
there's no clear indication that such a feature set will really come in handy. 
As far as I can tell, we still need only very basic functionality: UI for making
and fulfilling requests, a modifiable requestee role, and email notifications
when something changes.  The rest of bug functionality seems not only useless
but perhaps even dangerous if it blurs the line between the two types of objects
and encourages people to use the wrong one for the task.

In short, I think it was a mistake to make requests be bugs, and they should be
separated out.  Fortunately this doesn't mean rewriting everything, because a
lot of the basic design, database schema, text, and behavior can be harvested
from the current patch.  Some things, of course, like editing a request to
change the fulfiller and sending email notifications when something happens,
will need code written.

Comment 83

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress

Here's a work in progress that implements requests as separate database objects
rather than bugs.

Comment 84

[Gervase Markham [:gerv]]

"Plan to throw one away. You will anyway."

I've also come to agree with the both of you. That which seems elegant in theory
is, sadly, often not in practice.

I look forward to reviewing a new patch.

Gerv

Comment 85

[Myk Melez [:myk] [@mykmelez]]

Attachment: another work in progress

Here's another work in progress that shows the direction I'm going.  It is
updated to the tip and functionally complete but needs some UI polish and code
reformatting.

Comment 86

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress

Another work in progress, now very close to the final version.	Requests are
now flags, and they replace (and convert) attachment statuses.

http://landfill.tequilarista.org/bz98801/attachment.cgi?id=59&action=edit

Comment 87

[Bradley Baetz (:bbaetz)]

Comment on attachment 97279 [details] [diff] [review]
work in progress

>Index: checksetup.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
>retrieving revision 1.180
>diff -u -r1.180 checksetup.pl
>--- checksetup.pl	29 Aug 2002 09:25:42 -0000	1.180
>+++ checksetup.pl	30 Aug 2002 02:31:42 -0000
>@@ -1316,24 +1316,43 @@

>+# 2001-05-05 [e-mail]: Tables to support status flags.

Thats the wrong date.....

>+# "flagtypes" defines the flags that can be set on bugs/attachments.
>+# "flags" stores one record for each flag on each bug/attachment.
>+
>+$table{flagtypes} =
>+   'id              SMALLINT      NOT NULL  PRIMARY KEY , 
>+    name            VARCHAR(50)   NOT NULL , 
>+    description     MEDIUMTEXT    NULL , 

NOT NULL, surely? (You don't need the NULL stuff, anyway; its explicit)

>+    cc_list         VARCHAR(200)  NULL , 

Absolutely not :) Do _not_ make this a varchar, do it properly (via a separate
mapping table), else we'll just have all sorts of problems later.

>+    
>+    product_id      SMALLINT      NULL ,
>+    component_id    SMALLINT      NULL , 
>+    target_type     VARCHAR(10)   NOT NULL  DEFAULT \'bug\' , 

Wouldn't this be better done with a smallint (or a CHAR(1) and some |use
constant|s somewhere? The 'target type' is never going to be user modifiable.

>+    
>+    sortkey         SMALLINT      NOT NULL  DEFAULT 0 , 
>+    
>+    is_active       TINYINT NOT   NULL      DEFAULT 1 , 
>+    is_requestable  TINYINT NOT   NULL      DEFAULT 0 , 
>+    allow_multiple  TINYINT NOT   NULL      DEFAULT 0 , 
>+    

This reads like type="TINYINT NOT", nullability="NULL"

>+    INDEX(product_id) , 
>+    INDEX(component_id) 
>    ';
> 

>+$table{flags} =
>+    'id                 MEDIUMINT   NOT NULL  PRIMARY KEY , 
>+     type_id            SMALLINT    NOT NULL , 
>+     status             VARCHAR(1)  NOT NULL , 


CHAR, not VARCHAR.

>+     
>+     bug_id             MEDIUMINT   NOT NULL , 
>+     attach_id          MEDIUMINT   NULL , 
>+     
>+     creation_date      DATETIME    NOT NULL , 
>+     modification_date  DATETIME    NULL , 
>+     
>+     requester_id       MEDIUMINT   NULL , 
>+     requestee_id       MEDIUMINT   NULL 

NOT NULL for these? someone must have requested it....

>@@ -1791,7 +1811,7 @@
> AddFDef("attachments.ispatch", "Attachment is patch", 0);
> AddFDef("attachments.isobsolete", "Attachment is obsolete", 0);
> AddFDef("attachments.isprivate", "Attachment is private", 0);
>-AddFDef("attachstatusdefs.name", "Attachment Status", 0);
>+AddFDef("flagtypes.name", "Attachment Status", 0);

If you do this, you need to convert the old entries, below (they will still be
syntactically valid)


>+    while (my ($attach_id, $type_id, $bug_id) = $sth->fetchrow_array()) {
>+        ++$id;
>+        $dbh->do("INSERT INTO flags (id, type_id, status, bug_id, attach_id, 
>+                              creation_date, modification_date,
>+                              requester_id, requestee_id)
>+                  VALUES     ($id, $type_id, 'granted', $bug_id, $attach_id,
>+                              NOW(), NULL, NULL, NULL)");

NOW() isn't correct, and will lead to a whole lot of entries with the same
time.

Either leave this NULL, or (better) after teh conversion, trawl the
bug_activity table:

SELECT bug_when FROM bugs_activity WHERE attach_id = $attach_id AND fieldid =
<whatever it was> ORDER BY bug_when DESC LIMIT 1

for each attachment.

<skip lots of stuff>

>Index: Bugzilla/Flag.pm
>===================================================================
>RCS file: Bugzilla/Flag.pm
>diff -N Bugzilla/Flag.pm
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ Bugzilla/Flag.pm	30 Aug 2002 02:31:42 -0000
>@@ -0,0 +1,577 @@

>+use diagnostics;

no diagnostics any more.

>+# !!! Implement a cache for this function!

Wouldn't all this stuff be better off as an OO infrastructure. Caches will have
mod_perl issues....


>+    
>+    # Create new flags and update existing flags.
>+    my $new_flags = FormToNewFlags($target, $data);
>+    foreach my $flag (@$new_flags) { create($flag, $timestamp) }
>+    modify($data, $timestamp);
>+    
>+    # Take a snapshot of flags after changes.
>+    $flags = match({ 'bug_id'    => $target->{'bug'}->{'id'} , 
>+                        'attach_id' => $target->{'attachment'}->{'id'} });
>+    my @new_summaries;
>+    foreach my $flag (@$flags) {
>+        my $summary = $flag->{'type'}->{'name'} . $flag->{'status'};
>+        push(@new_summaries, $summary);
>+    }
>+
>+    my $old_summaries = join(", ", @old_summaries);
>+    my $new_summaries = join(", ", @new_summaries);
>+    my ($removed, $added) = &::DiffStrings($old_summaries, $new_summaries);
>+    my $sql_removed = &::SqlQuote($removed);
>+    my $sql_added = &::SqlQuote($added);
>+    my $field_id = &::GetFieldID('flagtypes.name');
>+    my $attach_id = $target->{'attachment'}->{'id'} || 'NULL';
>+    &::SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, " . 
>+               "fieldid, removed, added) VALUES ($target->{'bug'}->{'id'}, " . 
>+               "$attach_id, $::userid, $timestamp, $field_id, $sql_removed, " . 
>+               "$sql_added)");

Why do you need to do this as strings? If you know what ids were there before,
and you know what are there afterwards, then just deal withthe numbers.


>+    
>+    # Send an email notifying the relevant parties about the flag creation.
>+    notify($flag, "request/created-email.txt.tmpl") if $requestee_id ne "NULL";

Eww. Please, use defined/not defined to work with SQL NULL/NOT NULL. IF this
was done for sqlQuote reasons, then note that the DBI quotes undefined values
to NULL, and so we will too, soon.

</me ignores a whole lot more>

>Index: Bugzilla/User.pm
>===================================================================
>RCS file: Bugzilla/User.pm
>diff -N Bugzilla/User.pm
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ Bugzilla/User.pm	30 Aug 2002 02:31:42 -0000
>@@ -0,0 +1,128 @@
>+#!/usr/bonsaitools/bin/perl -w
>+# -*- Mode: perl; indent-tabs-mode: nil -*-
>+#
>+# The contents of this file are subject to the Mozilla Public
>+# License Version 1.1 (the "License"); you may not use this file
>+# except in compliance with the License. You may obtain a copy of
>+# the License at http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS
>+# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
>+# implied. See the License for the specific language governing
>+# rights and limitations under the License.
>+#
>+# The Original Code is the Bugzilla Bug Tracking System.
>+#
>+# The Initial Developer of the Original Code is Netscape Communications
>+# Corporation. Portions created by Netscape are
>+# Copyright (C) 1998 Netscape Communications Corporation. All
>+# Rights Reserved.
>+#
>+# Contributor(s): Myk Melez <[e-mail]>
>+
>+################################################################################
>+# Module Initialization
>+################################################################################
>+
>+# Make it harder for us to do dangerous things in Perl.
>+use diagnostics;
>+use strict;
>+
>+# This module implements utilities for dealing with Bugzilla users.
>+package User;

package Bugzilla::User;

>+
>+# Note!  This module requires that its caller have said "require CGI.pl" 
>+# to import relevant functions from that script and its companion globals.pl.
>+
>+################################################################################
>+# Functions
>+################################################################################
>+
>+sub match {
>+    # Generates a list of users whose login name (email address) or real name
>+    # matches a substring.
>+
>+    # $str contains the string to match against, while $limit contains the
>+    # maximum number of records to retrieve.
>+    my ($str, $limit) = @_;
>+
>+    # Do not have a limit by default.
>+    $limit ||= 0;
>+
>+    # Quote the string and add percent signs so it works as a string search
>+    # inside a SQL LIKE statement.
>+    my $sqlstr = &::SqlQuote('%' . $str . '%');
>+

user strings can contain % signs; use SUBSTRING instead.

>+    my $qry = "
>+          SELECT  userid, realname, login_name
>+            FROM  profiles
>+           WHERE  login_name LIKE $sqlstr
>+              OR  realname LIKE $sqlstr
>+        ORDER BY  realname , login_name
>+    ";
>+
>+    $qry .= " LIMIT  $limit " if $limit;

Id you do this, then you don't need the ||= bit above

>+    while (&::MoreSQLData()) {
>+        my $user = {};
>+        ($user->{'id'}, $user->{'name'}, $user->{'email'}) = &::FetchSQLData();
>+        # Generate a string to identify the user by name + email if the user
>+        # has a name or by email only if she doesn't.
>+        $user->{'identity'} = 
>+          $user->{'name'} ? "$user->{'name'} <$user->{'email'}>" 
>+                          : $user->{'email'};

I think that this package should be an object. it makes more sense that way,
and then it can be used gnerically.

So this code would select the id, and then push |new Bugzilla::User($id)|;

This isn't mod_perl safe.

>+my $user_cache = {};
>+sub get {

... and this would be the constructor.

Note that things could be lazily loaded, so that stuff like groups weren't
queries unless asked for.

>+}

Comment 88

[Bradley Baetz (:bbaetz)]

Comment on attachment 97279 [details] [diff] [review]
work in progress

>Index: checksetup.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v
>retrieving revision 1.180
>diff -u -r1.180 checksetup.pl
>--- checksetup.pl	29 Aug 2002 09:25:42 -0000	1.180
>+++ checksetup.pl	30 Aug 2002 02:31:42 -0000
>@@ -1316,24 +1316,43 @@

>+# 2001-05-05 [e-mail]: Tables to support status flags.

Thats the wrong date.....

>+# "flagtypes" defines the flags that can be set on bugs/attachments.
>+# "flags" stores one record for each flag on each bug/attachment.
>+
>+$table{flagtypes} =
>+   'id              SMALLINT      NOT NULL  PRIMARY KEY , 
>+    name            VARCHAR(50)   NOT NULL , 
>+    description     MEDIUMTEXT    NULL , 

NOT NULL, surely? (You don't need the NULL stuff, anyway; its explicit)

>+    cc_list         VARCHAR(200)  NULL , 

Absolutely not :) Do _not_ make this a varchar, do it properly (via a separate
mapping table), else we'll just have all sorts of problems later.

>+    
>+    product_id      SMALLINT      NULL ,
>+    component_id    SMALLINT      NULL , 
>+    target_type     VARCHAR(10)   NOT NULL  DEFAULT \'bug\' , 

Wouldn't this be better done with a smallint (or a CHAR(1) and some |use
constant|s somewhere? The 'target type' is never going to be user modifiable.

>+    
>+    sortkey         SMALLINT      NOT NULL  DEFAULT 0 , 
>+    
>+    is_active       TINYINT NOT   NULL      DEFAULT 1 , 
>+    is_requestable  TINYINT NOT   NULL      DEFAULT 0 , 
>+    allow_multiple  TINYINT NOT   NULL      DEFAULT 0 , 
>+    

This reads like type="TINYINT NOT", nullability="NULL"

>+    INDEX(product_id) , 
>+    INDEX(component_id) 
>    ';
> 

>+$table{flags} =
>+    'id                 MEDIUMINT   NOT NULL  PRIMARY KEY , 
>+     type_id            SMALLINT    NOT NULL , 
>+     status             VARCHAR(1)  NOT NULL , 


CHAR, not VARCHAR.

>+     
>+     bug_id             MEDIUMINT   NOT NULL , 
>+     attach_id          MEDIUMINT   NULL , 
>+     
>+     creation_date      DATETIME    NOT NULL , 
>+     modification_date  DATETIME    NULL , 
>+     
>+     requester_id       MEDIUMINT   NULL , 
>+     requestee_id       MEDIUMINT   NULL 

NOT NULL for these? someone must have requested it....

>@@ -1791,7 +1811,7 @@
> AddFDef("attachments.ispatch", "Attachment is patch", 0);
> AddFDef("attachments.isobsolete", "Attachment is obsolete", 0);
> AddFDef("attachments.isprivate", "Attachment is private", 0);
>-AddFDef("attachstatusdefs.name", "Attachment Status", 0);
>+AddFDef("flagtypes.name", "Attachment Status", 0);

If you do this, you need to convert the old entries, below (they will still be
syntactically valid)


>+    while (my ($attach_id, $type_id, $bug_id) = $sth->fetchrow_array()) {
>+        ++$id;
>+        $dbh->do("INSERT INTO flags (id, type_id, status, bug_id, attach_id, 
>+                              creation_date, modification_date,
>+                              requester_id, requestee_id)
>+                  VALUES     ($id, $type_id, 'granted', $bug_id, $attach_id,
>+                              NOW(), NULL, NULL, NULL)");

NOW() isn't correct, and will lead to a whole lot of entries with the same
time.

Either leave this NULL, or (better) after teh conversion, trawl the
bug_activity table:

SELECT bug_when FROM bugs_activity WHERE attach_id = $attach_id AND fieldid =
<whatever it was> ORDER BY bug_when DESC LIMIT 1

for each attachment.

<skip lots of stuff>

>Index: Bugzilla/Flag.pm
>===================================================================
>RCS file: Bugzilla/Flag.pm
>diff -N Bugzilla/Flag.pm
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ Bugzilla/Flag.pm	30 Aug 2002 02:31:42 -0000
>@@ -0,0 +1,577 @@

>+use diagnostics;

no diagnostics any more.

>+# !!! Implement a cache for this function!

Wouldn't all this stuff be better off as an OO infrastructure. Caches will have
mod_perl issues....


>+    
>+    # Create new flags and update existing flags.
>+    my $new_flags = FormToNewFlags($target, $data);
>+    foreach my $flag (@$new_flags) { create($flag, $timestamp) }
>+    modify($data, $timestamp);
>+    
>+    # Take a snapshot of flags after changes.
>+    $flags = match({ 'bug_id'    => $target->{'bug'}->{'id'} , 
>+                        'attach_id' => $target->{'attachment'}->{'id'} });
>+    my @new_summaries;
>+    foreach my $flag (@$flags) {
>+        my $summary = $flag->{'type'}->{'name'} . $flag->{'status'};
>+        push(@new_summaries, $summary);
>+    }
>+
>+    my $old_summaries = join(", ", @old_summaries);
>+    my $new_summaries = join(", ", @new_summaries);
>+    my ($removed, $added) = &::DiffStrings($old_summaries, $new_summaries);
>+    my $sql_removed = &::SqlQuote($removed);
>+    my $sql_added = &::SqlQuote($added);
>+    my $field_id = &::GetFieldID('flagtypes.name');
>+    my $attach_id = $target->{'attachment'}->{'id'} || 'NULL';
>+    &::SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, " . 
>+               "fieldid, removed, added) VALUES ($target->{'bug'}->{'id'}, " . 
>+               "$attach_id, $::userid, $timestamp, $field_id, $sql_removed, " . 
>+               "$sql_added)");

Why do you need to do this as strings? If you know what ids were there before,
and you know what are there afterwards, then just deal withthe numbers.


>+    
>+    # Send an email notifying the relevant parties about the flag creation.
>+    notify($flag, "request/created-email.txt.tmpl") if $requestee_id ne "NULL";

Eww. Please, use defined/not defined to work with SQL NULL/NOT NULL. IF this
was done for sqlQuote reasons, then note that the DBI quotes undefined values
to NULL, and so we will too, soon.

</me ignores a whole lot more>

>Index: Bugzilla/User.pm
>===================================================================
>RCS file: Bugzilla/User.pm
>diff -N Bugzilla/User.pm
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ Bugzilla/User.pm	30 Aug 2002 02:31:42 -0000
>@@ -0,0 +1,128 @@
>+#!/usr/bonsaitools/bin/perl -w
>+# -*- Mode: perl; indent-tabs-mode: nil -*-
>+#
>+# The contents of this file are subject to the Mozilla Public
>+# License Version 1.1 (the "License"); you may not use this file
>+# except in compliance with the License. You may obtain a copy of
>+# the License at http://www.mozilla.org/MPL/
>+#
>+# Software distributed under the License is distributed on an "AS
>+# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
>+# implied. See the License for the specific language governing
>+# rights and limitations under the License.
>+#
>+# The Original Code is the Bugzilla Bug Tracking System.
>+#
>+# The Initial Developer of the Original Code is Netscape Communications
>+# Corporation. Portions created by Netscape are
>+# Copyright (C) 1998 Netscape Communications Corporation. All
>+# Rights Reserved.
>+#
>+# Contributor(s): Myk Melez <[e-mail]>
>+
>+################################################################################
>+# Module Initialization
>+################################################################################
>+
>+# Make it harder for us to do dangerous things in Perl.
>+use diagnostics;
>+use strict;
>+
>+# This module implements utilities for dealing with Bugzilla users.
>+package User;

package Bugzilla::User;

>+
>+# Note!  This module requires that its caller have said "require CGI.pl" 
>+# to import relevant functions from that script and its companion globals.pl.
>+
>+################################################################################
>+# Functions
>+################################################################################
>+
>+sub match {
>+    # Generates a list of users whose login name (email address) or real name
>+    # matches a substring.
>+
>+    # $str contains the string to match against, while $limit contains the
>+    # maximum number of records to retrieve.
>+    my ($str, $limit) = @_;
>+
>+    # Do not have a limit by default.
>+    $limit ||= 0;
>+
>+    # Quote the string and add percent signs so it works as a string search
>+    # inside a SQL LIKE statement.
>+    my $sqlstr = &::SqlQuote('%' . $str . '%');
>+

user strings can contain % signs; use SUBSTRING instead.

>+    my $qry = "
>+          SELECT  userid, realname, login_name
>+            FROM  profiles
>+           WHERE  login_name LIKE $sqlstr
>+              OR  realname LIKE $sqlstr
>+        ORDER BY  realname , login_name
>+    ";
>+
>+    $qry .= " LIMIT  $limit " if $limit;

Id you do this, then you don't need the ||= bit above

>+    while (&::MoreSQLData()) {
>+        my $user = {};
>+        ($user->{'id'}, $user->{'name'}, $user->{'email'}) = &::FetchSQLData();
>+        # Generate a string to identify the user by name + email if the user
>+        # has a name or by email only if she doesn't.
>+        $user->{'identity'} = 
>+          $user->{'name'} ? "$user->{'name'} <$user->{'email'}>" 
>+                          : $user->{'email'};

I think that this package should be an object. it makes more sense that way,
and then it can be used gnerically.

So this code would select the id, and then push |new Bugzilla::User($id)|;

This isn't mod_perl safe.

>+my $user_cache = {};
>+sub get {

... and this would be the constructor.

Note that things could be lazily loaded, so that stuff like groups weren't
queries unless asked for.

>+}

Comment 89

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v21: review fixes

>Thats the wrong date.....

Fixed.


>>+$table{flagtypes} =
>>+   'id	       SMALLINT      NOT NULL  PRIMARY KEY , 
>>+    name	       VARCHAR(50)   NOT NULL , 
>>+    description     MEDIUMTEXT    NULL , 
>
>NOT NULL, surely? (You don't need the NULL stuff, anyway; its explicit)

Fixed.


>>+    cc_list	       VARCHAR(200)  NULL , 
>
>Absolutely not :) Do _not_ make this a varchar, do it properly (via a separate

>mapping table), else we'll just have all sorts of problems later.

Absolutely not. :-)  There's nothing to map here; CCs don't have to be
Bugzilla members, and this feature is intentionally simple and should
remain so.


>>+    product_id      SMALLINT      NULL ,
>>+    component_id    SMALLINT      NULL , 
>>+    target_type     VARCHAR(10)   NOT NULL  DEFAULT \'bug\' , 
>
>Wouldn't this be better done with a smallint (or a CHAR(1) and some |use
>constant|s somewhere? The 'target type' is never going to be user modifiable.

Yes, done.


>This reads like type="TINYINT NOT", nullability="NULL"

D'oh, fixed.


>CHAR, not VARCHAR.

Fixed.


>>+	requester_id	   MEDIUMINT   NULL , 
>>+	requestee_id	   MEDIUMINT   NULL 
>
>NOT NULL for these? someone must have requested it....

Nope.  Some flags (like attachment statuses currently)
are set without being requested.  Other flags (like
mozilla1.2) are requested of no one in particular.


>@@ -1791,7 +1811,7 @@
> AddFDef("attachments.ispatch", "Attachment is patch", 0);
> AddFDef("attachments.isobsolete", "Attachment is obsolete", 0);
> AddFDef("attachments.isprivate", "Attachment is private", 0);
>-AddFDef("attachstatusdefs.name", "Attachment Status", 0);
>+AddFDef("flagtypes.name", "Attachment Status", 0);

If you do this, you need to convert the old entries, below (they will still be
syntactically valid)


>NOW() isn't correct, and will lead to a whole lot of entries with the same
>time.
>
>Either leave this NULL, or (better) after teh conversion, trawl the
>bug_activity table:
>
>SELECT bug_when FROM bugs_activity WHERE attach_id = $attach_id AND fieldid =
><whatever it was> ORDER BY bug_when DESC LIMIT 1
>
>for each attachment.

Done.


>no diagnostics any more.

Done.

>Wouldn't all this stuff be better off as an OO infrastructure. Caches will
have
>mod_perl issues....

I'm not sure this stuff would be better off OO, and we'll just have to work out

the cache/mod_perl issues, since caches can also provide a significant 
performance boost.

>Why do you need to do this as strings? If you know what ids were there before,

>and you know what are there afterwards, then just deal withthe numbers.

Because this is for the bugs_activity table, where we show users a human-
readable set of changes.

>>+    # Send an email notifying the relevant parties about the flag creation.
>>+    notify($flag, "request/created-email.txt.tmpl") if $requestee_id ne
"NULL";
>
>Eww. Please, use defined/not defined to work with SQL NULL/NOT NULL. IF this
>was done for sqlQuote reasons, then note that the DBI quotes undefined values
>to NULL, and so we will too, soon.

Fixed.


>package Bugzilla::User;

Fixed.

>user strings can contain % signs; use SUBSTRING instead.

Used INSTR.


>Id you do this, then you don't need the ||= bit above

Right, fixed.


>I think that this package should be an object. it makes more sense that way,
>and then it can be used gnerically.

Done.

>This isn't mod_perl safe.

But it is useful, so we need to figure out how to make it mod_perl-safe.

bbaetz- Take a look at what I do in Search.pm.	I think I've solved
the problem of searching for statuses not equal to some value.

Comment 90

[Bradley Baetz (:bbaetz)]

Couple of things I'm noting on landfill:

- filtering is broken - gives sql errors

- if you add a name for a request, but not a flag, then the activiy log has an
'attachment status' entry changed from '' to ''.

- bug_Activity shows it as 'attachment status' even when its a bug flag changed.

- disabled users should not be on the list of valid requestors

- If I move a bug which has a product-specific flag set, then the status is
still in the db. (As an aside, I think that the status should be valid iff the
new product has the same flag set, similar to how milestone moves work)
- There should be a sanity check for that, too.

Actually, maybe rather than restricting flags to a component or anything,
restrict them to a set of products. That would allow the same flags to be set
for browser as for mailnews, for example.

Comment 91

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v22: review fixes

Note that landfill is not yet updated with this patch.

>- filtering is broken - gives sql errors

Fixed.

>- if you add a name for a request, but not a flag, then the activiy log has an

>'attachment status' entry changed from '' to ''.

Fixed.

>- bug_Activity shows it as 'attachment status' even when its a bug flag 
>changed.

Fixed.

>- disabled users should not be on the list of valid requestors

Fixed.

>- If I move a bug which has a product-specific flag set, then the status is
>still in the db. (As an aside, I think that the status should be valid iff the

>new product has the same flag set, similar to how milestone moves work)
>- There should be a sanity check for that, too.

Not fixed yet.

>Actually, maybe rather than restricting flags to a component or anything,
>restrict them to a set of products. That would allow the same flags to be set
>for browser as for mailnews, for example.

Fixed by adding the ability for a flag to apply to multiple product/component
combinations).	The backend was fairly trivial; the UI was tricky (and is still
quite ugly but works).

In addition to these fixes, I fixed several issues in the attachment status
migration code.

Comment 92

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v23: updates flags on product/component changes

When a bug's product/component changes and there are flags on that bug that
can't be set for bugs in the new product/component, this version of the patch
deletes those flags automagically.

Comment 93

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v24: minor fixes

This patch adds minor fixes for a few bugs.  The only thing I can see that's
left to worry about is placement of table locks, but that's a minor issue and
shouldn't prevent anyone from doing a code review on the existing patch.  Don't
all volunteer at once. :-)

Comment 94

[Bradley Baetz (:bbaetz)]

Comment on attachment 98076 [details] [diff] [review]
patch v24: minor fixes

+    AddField("flagtypes", "target_type", "VARCHAR(10) NOT NULL DEFAULT
'attachment'");

wrong type/default

+	 $dbh->do("UPDATE bugs_activity SET fieldid = $new_field_id, " . 
+		  "added = $added, removed = $removed " . 
+		  "WHERE attach_id = $attach_id AND who = $who " . 
+		  "AND bug_when = $when AND fieldid = $old_field_id");

You should have added/removed in the WHERE condition, because multiple changes
can happen simultaniously.

The get_component_id change is almost certainly wrong - for a start, not using
detaint_natural means that anything which was causing you to make that change
would fail taint checks. get_component_id should only be called if you've
already validated the product, though

request.cgi needs taint mode enabled. (which may have been why you didn't see
the above issue)
Also, the .pms shouldn't have a #! line

The error message for cc-list validation says 50, even though its 200 (in
several places). You should probably do extra validity checks for comma
separated email addresses, too.

validateComponent in editflagtypes should use get_component_id instead of
manual SQL (probably in a few other places, too)

Some of your SELECT 1, foo ... could be redone, since the foos are NOT NULL in
the schema

Thats a quick overview of most of hte cgis, which is all I'm going to get to
this week.

Comment 95

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v25: more review fixes and enhancements.

>wrong type/default

Fixed.

>You should have added/removed in the WHERE condition, because multiple 
>changes can happen simultaniously.

Fixed.

>The get_component_id change is almost certainly wrong - for a start, not 
>using detaint_natural means that anything which was causing you to make 
>that change would fail taint checks. get_component_id should only be called
>if you've already validated the product, though

The code wasn't using detaint_natural before, but in any case 
get_component_id returns undefined if it receives an invalid component
name, so it should do the same for product_id (and leave it up to
the calling code to decide whether or not to die).  This is much more
friendly and useful for code that only optionally needs a valid
component ID.

>request.cgi needs taint mode enabled. (which may have been why you didn't
>see the above issue)

Fixed.

>Also, the .pms shouldn't have a #! line

Fixed.

>The error message for cc-list validation says 50, even though its 200 (in
>several places). You should probably do extra validity checks for comma
>separated email addresses, too.

Fixed and fixed.

>validateComponent in editflagtypes should use get_component_id instead of
>manual SQL (probably in a few other places, too)

Fixed.

>Some of your SELECT 1, foo ... could be redone, since the foos are NOT NULL
>in the schema

Maybe; not sure that makes more sense.	"1 AS exists" works pretty well.

This patch implements "exclusions" so an admin can say "this flag type 
is valid for all products/components EXCEPT FOR foo/bar...".  It also
cleans up some code and clarifies the difference between a requester,
a setter, and a requestee (requesters are just setters of flags whose
status is "?").

Comment 96

[Gervase Markham [:gerv]]

Myk,

I'd like to review this patch, but (as you know) it's fairly large. It would
probably save me a lot of time if you spent five minutes writing up how it
works, and the design philosophy. I could then read the patch thinking "Ah, that
bit fits _there_ in the big picture" and "Hang on - if this section does X, then
why doesn't it do Y, because that's needed" and so on.

Any chance of that?

Gerv

Comment 97

[Myk Melez [:myk] [@mykmelez]]

Yes, I'll write up an overview of philosophy and code and post it here today or
tomorrow.

Comment 98

[Myk Melez [:myk] [@mykmelez]]

Attachment: overview and description

Here's a start on a description of how the request tracker works.  Please look
it over and let me know how it helps and what other information you would need.

Comment 99

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v26: more enhancements

This patch adds email notification preferences for receiving emails when
someone asks you to set a flag or someone sets a flag you ask them to set.  It
also implements flags that can be requested but cannot be requested of a
specific user (like the generally requestable project management keywords used
on b.m.o).  It also contains some code cleanup.

Comment 100

[Gervase Markham [:gerv]]

OK, hold onto your hats, here it is:

Index: attachment.cgi
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v
retrieving revision 1.20
diff -u -r1.20 attachment.cgi
--- attachment.cgi	26 Aug 2002 06:16:51 -0000	1.20
+++ attachment.cgi	12 Sep 2002 02:16:40 -0000
   if ($oldisobsolete ne $::FORM{'isobsolete'}) {
     my $fieldid = GetFieldID('attachments.isobsolete');
     SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when,
fieldid, removed, added) 
-             VALUES ($bugid, $::FORM{'id'}, $::userid, NOW(), $fieldid,
$oldisobsolete, $::FORM{'isobsolete'})");
+             VALUES ($bugid, $::FORM{'id'}, $::userid, $sql_timestamp,
$fieldid, $oldisobsolete, $::FORM{'isobsolete'})");

Please restrict to 80 columns where possible; it makes it all so much easier to
read :-)

Index: bug_form.pl
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v
retrieving revision 1.102
diff -u -r1.102 bug_form.pl
--- bug_form.pl	26 Aug 2002 06:16:51 -0000	1.102
+++ bug_form.pl	12 Sep 2002 02:16:40 -0000
@@ -198,6 +203,27 @@
 
     # Attachments
     $bug{'attachments'} = Attachment::query($id);
+   
+    # The types of flags that can be set on this bug.
+    # If none, no UI for setting flags will be displayed.
+    my $flag_types = 
+      FlagType::match({ 'target_type'  => 'b', 

Can we please use "bug" and "attachment" instead of "b" and "a"?

+$table{flags} =
+    'id                 MEDIUMINT     NOT NULL  PRIMARY KEY , 
+     type_id            SMALLINT      NOT NULL , 
+     status             CHAR(1)       NOT NULL , 
+     
+     bug_id             MEDIUMINT     NOT NULL , 
+     attach_id          MEDIUMINT     NULL , 

Is exactly one of these two ever valid, or do we also use bug_id if we are using
attach_id?

+$table{flagtypes} =
+   'id                  SMALLINT      NOT NULL  PRIMARY KEY , 
+    name                VARCHAR(50)   NOT NULL , 
+    description         MEDIUMTEXT    NOT NULL , 

I think we have a bug open about "mediumtext abuse", although I don't know what
the correct replacement type is.

+    cc_list             VARCHAR(200)  NULL , 

Er... huh? :-)

+    target_type         CHAR(1)       NOT NULL  DEFAULT \'b\' , 

(The fact that we are using "b" and "a" internally doesn't mean we need them on
the module's external interface.)
    
+    is_active           TINYINT       NOT NULL  DEFAULT 1 , 
+    is_requestable      TINYINT       NOT NULL  DEFAULT 0 , 
+    is_requesteeble     TINYINT       NOT NULL  DEFAULT 0 , 

Ouch. :-)

+    sortkey             SMALLINT      NOT NULL  DEFAULT 0 

Are we really going to mess with the whole sortkey thing again? I can see why
you might need them for e.g. milestones, which is a long list, but are bugs and
attachments really going to have more than about six possible statuses?

Index: editflagtypes.cgi
===================================================================
RCS file: editflagtypes.cgi
diff -N editflagtypes.cgi
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ editflagtypes.cgi	12 Sep 2002 02:16:40 -0000
+# Suppress "used only once" warnings.
+my $x = \@::legal_product;
+$x = \@::legal_components;
+$x = \%::components;

Can we "use vars", now that we've figured out the syntax? :-)

+elsif ($action eq 'delete')         { deleteType();     }
+else { 
+    ThrowCodeError("action_unrecognized", {action=>$action});
+}

That reminds me; we don't "FILTER html" the variables we print in the
code-error.html.tmpl template, so this is an XSS hole at the moment. Oops.

+        $vars->{'type'} = { 'target_type' => $::FORM{'target_type'} } 

Nit: missing semi-colon on very end, which is conventionally present.

+        my $category = ($::FORM{'product'} || "Any") . ":" .
($::FORM{'component'} || "Any");
+        push(@inclusions, $category) unless grep($_ eq $category, @inclusions);

This looks suspiciously like we can't have a product or component named "Any".
Is this restriction enforced at creation time? An alternative would be picking a
more unlikely string.

+    $vars->{'make_js_array'} = \&make_js_array;

Can this be a filter?
var foo = [% wibblelist FILTER js_array %];

+    SendSQL("
+        SELECT flags.id 
+        FROM flags, bugs LEFT OUTER JOIN flaginclusions i

I thought there was an SQL keyword "AS" which is used here? (flaginclusions AS i)

+# Can't be "delete" because that's a Perl reserved word.

I don't think this should be a problem, perhaps as long as you call the sub as
&delete(...). One of the advantages of the whole $%@ thing in Perl is that
reserved words don't clash with variable names. At least, that's what I thought.

+sub validateID {
+    if ( $::FORM{'id'} !~ /^[1-9][0-9]*$/ ) {
+        my $id = html_quote($::FORM{'id'});
+        ThrowUserError("The flag type ID ($id) must be a positive integer.");
+    }

Can you please templatise all these calls? I recently checked in a patch which
will cause calls like this to fail the tests, so you don't actually have much
choice :-P

+sub validateName {
+    $::FORM{'name'}
+      || ThrowUserError("You must enter a name for the flag type.");
+
+    length($::FORM{'name'}) <= 50
+      || ThrowUserError("The flag type name cannot be more than 50 characters
long.");
+}

Is this enough checking to then pass this name to the database? Same question
for validateDescription.

+sub validateCCList {
+    length($::FORM{'cc_list'}) <= 200
+      || ThrowUserError("The CC list cannot be more than 200 characters long.");

At this point in the review, I still haven't worked out what this CC list is
for... but 200 characters seems like an arbitrary limit. Can't we use one of the
SQL types which grow as you use it (like, er, MEDIUMTEXT ;-) ?    

+sub validateSortKey {
+    $::FORM{'sortkey'} =~ /^\d+$/
+      && $::FORM{'sortkey'} < 32768
+        || ThrowUserError("The sort key must be an integer between 0 and 32767
inclusive.");
+}

Why? Can't we use a larger type in the DB? If someone enters a sort key > 4
billion, they deserve what they get (bad sorting because of truncation, presumably.)

+# make_js_array: iterates through the product array creating a
+# javascript array keyed by product with an alphabetically ordered array
+# for the corresponding elements in the components array passed in.
+# return a string with javascript definitions for the product in a nice
+# arrays which can be linearly appended later on.

OK, maybe it can't be a filter :-)

+    my $ret = "\nvar $arr = new Array();\n";
+    foreach my $p ( @prods ) {
+        # join each element with a "," case-insensitively alpha sorted
+        if ( $data{$p} ) {
+            $ret .= $arr."[".SqlQuote($p)."] = [";

Don't we have a js_quote function? I seem to remember moving towards this and
away from SqlQuote() for some good reason or other.

Index: productmenu.js
===================================================================
RCS file: productmenu.js
diff -N productmenu.js
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ productmenu.js	12 Sep 2002 02:16:40 -0000
@@ -0,0 +1,242 @@
+// Adds to the target select object all elements in array that
+// correspond to the elements selected in source.
+//     - array should be a array of arrays, indexed by product name. the
+//       array should contain the elements that correspont to that
+//       product. Example:
+//         var array = Array();
+//         array['ProductOne'] = [ 'ComponentA', 'ComponentB' ];
+//         updateSelect(array, source, target);
+//     - sel is a list of selected items, either whole or a diff
+//       depending on sel_is_diff.
+//     - sel_is_diff determines if we are sending in just a diff or the
+//       whole selection. a diff is used to optimize adding selections.
+//     - target should be the target select object.
+//     - single specifies if we selected a single item. if we did, no
+//       need to merge.

+function updateSelect( array, sel, target, sel_is_diff, single, blank ) {
+
+    var i, j, comp;

Is all this stuff chopped out of form.html.tmpl (part of the search page)? If
so, can we use this new file there too, please? I'd accept the filing of a new
bug and a promise to fix it before 2.18. ;-)

On the basis that this is all old code, I haven't reviewed it. Please tell me if
it needs reviewing. One thing I do notice, though, is that it's back to using JS
instead of TT comments, which is bad.

Index: request.cgi
===================================================================

I still can't figure it out - does this CGI show the queue for a given bug? Or a
bug and all its attachments? Can we show the queue for a given user? How do you
get the UI for sending it parameters?

Index: userprefs.cgi
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v
retrieving revision 1.39
diff -u -r1.39 userprefs.cgi
--- userprefs.cgi	26 Aug 2002 06:17:12 -0000	1.39
+++ userprefs.cgi	12 Sep 2002 02:16:40 -0000
@@ -208,6 +208,11 @@
         $vars->{'excludeself'} = 0;
     }
     
+    foreach my $flag qw(FlagRequestee FlagRequester) {
+        $vars->{$flag} = 
+          !exists($emailflags{$flag}) || $emailflags{$flag} eq 'on';
+    }
+    

Are we really allowing people to turn off flag requests? Shouldn't we mail the
requester if the requestee hasn't seen their message?

Index: Bugzilla/Flag.pm
===================================================================
+# !!! Implement a cache for this function!
+sub get {

Do you plan to do this, then? :-)

+        $flag || &::ThrowCodeError("flag_nonexistent", 0, {'id'=>$id});

Hmm. Doesn't at least one of your calls to ThrowCodeError() (which I've already
deleted from my scratch buffer and so can't find) omit the 0? Anyway (see below)
this should be done differently.

+        # Make sure the user is authorized to set flags.
+        $::vars->{'user'}->{'groups'}{'editbugs'}
+          || &::ThrowUserError("Sorry, you are not authorized to set flags.");

<sigh> This sort of thing needs centralising eventually. CheckCanChangeField()
doesn't cover so many things...
        
+    # In case the bug's product/component has changed, clear flags that are
+    # no longer valid.
+    &::SendSQL("
+        SELECT flags.id 
+        FROM flags, bugs LEFT OUTER JOIN flaginclusions i
+        ON (flags.type_id = i.type_id 
+            AND (bugs.product_id = i.product_id OR i.product_id IS NULL)
+            AND (bugs.component_id = i.component_id OR i.component_id IS NULL))
+        WHERE flags.type_id = $target->{'bug'}->{'id'} 
+        AND flags.bug_id = bugs.bug_id
+        AND i.type_id IS NULL
+    ");
+    clear(&::FetchOneColumn()) while &::MoreSQLData();

Could we have a version of clear() which took a list and made one SQL call? At
first glance, this seems inefficient.

+sub create {
+    # Creates a bug report in the database for a new flag and populates it
+    # with the data from the flag.

"Creates a bug report"? :-) I must be reviewing the wrong patch, then...

+    # Get information about the setter to add to each flag.
+    # Uses a conditional to suppress Perl's "used only once" warnings.
+    my $setter = new Bugzilla::User($::userid || $::userid);

Can we suppress it in the standard way (use vars) instead? Everyone understands
that... (at least, they do now. ;-)

+    if ($verify_requestees) {
+        # Work around the intricacies of globals.pl not being templatized
+        # by defining local variables for the $::template and $::vars globals.
+        my $template = $::template;
+        my $vars = $::vars;

use vars qw($template $vars); 
should do the trick, shouldn't it?

+sub GetBug {
+    # Returns a hash of information about a target bug.
+    my ($id) = @_;
+
+    # Save the currently running query (if any) so we do not overwrite it.
+    &::PushGlobalSQLState();
+
+    &::SendSQL("SELECT  1, short_desc, groupset, product_id, component_id
+                  FROM  bugs
+                 WHERE  bug_id = $id");
+
+    my $bug = { 'id' => $id };
+    
+    ($bug->{'exists'}, $bug->{'summary'}, $bug->{'groupset'}, 
+     $bug->{'product_id'}, $bug->{'component_id'})
+       = &::FetchSQLData();
+
+    # Restore the previously running query (if any).
+    &::PopGlobalSQLState();
+
+    return $bug;
+}

Do we not have a Bug.pm for this?

+sub GetAttachment {
+    # Returns a hash of information about a target attachment.
+    my ($id) = @_;
+
+    my $attachment = { 'id' => $id };
+    
+    &::PushGlobalSQLState();
+    &::SendSQL("SELECT 1, description, bug_id FROM attachments 
+                WHERE attach_id = $id");
+    ($attachment->{'exists'} , 
+     $attachment->{'summary'} , 
+     $attachment->{'bug_id'}) = &::FetchSQLData();
+    &::PopGlobalSQLState();
+
+    return $attachment;
+}

And an Attachment.pm?
If they are too heavyweight, we need a way of creating lightweight ones.

+    my $delivery_mode = &::Param("sendmailnow") ? "" : "-ODeliveryMode=deferred";
+    open(SENDMAIL, "|/usr/lib/sendmail $delivery_mode -t -i") 
+      || die "Can't open sendmail";
+    print SENDMAIL $message;
+    close(SENDMAIL);

<sigh>

Index: Bugzilla/FlagType.pm
===================================================================
+# Note: when adding tables to the tables list, make sure to include the
+# separator (i.e. a comma or words like "LEFT OUTER JOIN") in the table name, 
+# since tables take multiple separators based on the join type, and therefore 
+# it is not possible to join them later using a single known separator.

I don't understand this comment. The below list has no separators with the names.

<later> Oh, I see. Say "@tables" instead of "the tables list". You should also
specify whether the separator goes before or after the name (I assume before,
but you should say.)

+sub sqlify_criteria {
+    # Converts a hash of criteria into a list of SQL criteria.
+    # $criteria is a reference to the criteria (field => value), 
+    # $tables is a reference to an array of tables being accessed 
+    # by the query, $columns is a reference to an array of columns
+    # being returned by the query, and $having is a reference to
+    # a criterion to put into the HAVING clause.

Shouldn't there be a query object which deals with all of this stuff, with
methods like add_table($table) and so on? We already have the start of one in
Search.pm. 

Index: Bugzilla/User.pm
===================================================================

Will an instance of this be replacing the current $vars->{'user'} object? That
would make sense, but we have to think about interface compatibility.

+    # Execute the query, retrieve the results, and make them into User objects.
+    my @users;
+    &::PushGlobalSQLState();
+    &::SendSQL($qry);
+    push(@users, new Bugzilla::User(&::FetchSQLData())) while &::MoreSQLData();
+    &::PopGlobalSQLState();

Isn't it inefficient to do one query for each user you care about, rather than
asking about the lot in one go?

+sub email_prefs {
+    # Get or set (not implemented) the user's email notification preferences.
+    
+    my $self = shift;
+    
+    # If the calling code is setting the email preferences, update the object
+    # but don't do anything else.  This needs to write email preferences back
+    # to the database.
+    if (@_) { $self->{email_prefs} = shift; return; }

I'm not convinced by this approach. I think getting and setting should be two
separate functions (given that you are not declaring the sub as an lvalue (yes,
I've been reading Programming Perl today.)) Two separate functions is the OO norm.

[some template whose name I deleted]    
+<p>
+   There are [% flag_count %] flags of type [% name FILTER html %].  
+   If you delete this type, those flags will also be deleted.  Note that 
+   you can deactivate the type instead of deleting it by unchecking the 
+   "is active" flag while editing it.
+</p>

Can we offer a third link here - "deactivate instead"?

+[%# The javascript and header_html blocks get used in header.html.tmpl. %]
+[% javascript = BLOCK %]
+  var usetms = 0; // do we have target milestone?
+  var first_load = 1; // is this the first time we load the page?
+  var last_sel = []; // caches last selection
+  [% make_js_array(products, components_by_product, "cpts") %]

OK, if it can't be a FILTER, can it be a template instead? :-)

Index: template/en/default/global/code-error.html.tmpl
===================================================================
+  [% ELSIF error == "action_unrecognized" %]
+    I don't recognize the value <em>[% variables.action %]</em>
+    of the <em>action</em> variable.
+ 

FILTER html? But also, I may not have made the expected use of the "variables"
hash clear enough. In general, ThrowCodeError messages will use names in the top
level namespace, like anything else, e.g. $vars->{'action'} and [% action %].
This is for both simplicity and consistency. "variables" is only for when you
have some techie information to print out for us to use in debugging, and the
contents of the hash are automatically printed out and appended to the error page. 
 
+  [% ELSIF error == "flag_nonexistent" %]
+    There is no flag with ID #[% variables.id %].
+  
+  [% ELSIF error == "flag_status_unknown" %]
+    I don't understand the flag status [% variables.status %].
+  

FILTERs also needed.

Index: template/en/default/global/messages.html.tmpl
===================================================================
+  [% ELSIF message_tag == "flag_type_created" %]
+    [% title = "Flag Type Created" %]
+    <p>
+      The flag type <em>[% name FILTER html %]</em> has been created.
+      <a href="editflagtypes.cgi">Back to flag types.</a>
+    </p>

The a href business here can also be achieved by setting "url" and "link"
parameters. But I have sort of thought they sucked for a while; is your non-use
of them deliberate?
    
Index: template/en/default/global/select-menu.html.tmpl
===================================================================
-  [% ELSIF values_type.search("HASH") %]
+  [% ELSIF options_type.search("HASH") %]

Is this a bug fix?

Index: template/en/default/request/created-email.txt.tmpl
===================================================================
+From: bugzilla-request-daemon
+To: [% flag.requestee.email IF flag.requestee.email_prefs.FlagRequestee %]
+CC: [% flag.type.cc_list %]

What happens if the requestee has email off and the CC list is empty? Same
question for the other emails.

Index: template/en/default/request/queue.html.tmpl
===================================================================
+      [% FOREACH column = display_columns %]
+        [% NEXT IF column == group_field || excluded_columns.contains(column) %]
+        <td>[% PROCESS "display_$column" %]</td>

Is all this BLOCK-related stuff worth it just to avoid typing <td></td> ten
times? :-) I think it might actually be less verbose if you just included all
the code up here...

Gerv

Comment 101

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v27: review fixes

Here's a patch with review fixes; ready for re-review, see additional comments
below...

Index: attachment.cgi
   
>Please restrict to 80 columns where possible; it makes it all so much 
>easier to read :-)

Sure, although in this case the line length blends in with the rest
of that file.


Index: bug_form.pl

>Can we please use "bug" and "attachment" instead of "b" and "a"?

Done.


>Is exactly one of these two ever valid, or do we also use bug_id if we 
>are using attach_id?

We also use bug_id, which is useful when we want to extract the list of all
pending flags for a given bug (including those pending for its attachments)
for the request queue or when we want to use query.cgi to search for bugs
with certain flags set on either the bug or its attachments.


>I think we have a bug open about "mediumtext abuse", although I don't 
>know what the correct replacement type is.

Probably TEXT (32K), done.


>+    cc_list		  VARCHAR(200)	NULL , 
>
>Er... huh? :-)

It should be possible to copy addresses that aren't users in Bugzilla 
(f.e. mailing lists), so we don't want this to be connected to 
the profiles table, and it shouldn't be possible to add too many addresses
here, since installations should use mailing lists for that.


>+    target_type	  CHAR(1)	NOT NULL  DEFAULT \'b\' , 
>
>(The fact that we are using "b" and "a" internally doesn't mean we need 
>them on the module's external interface.)
 
Fixed.

   
>+    is_requesteeble	  TINYINT	NOT NULL  DEFAULT 0 , 
>
>Ouch. :-)

I think it's quite funny and memorable myself, but I'm open to suggestions.


>Are we really going to mess with the whole sortkey thing again? I can 
>see why you might need them for e.g. milestones, which is a long list, 
>but are bugs and attachments really going to have more than about six 
>possible statuses?

Not on b.m.o, but even so we want "approval" to appear after "review"
and "super-review".  The problem with sort keys is not their existence,
it's the lack of a good UI for setting them.  We shouldn't make our
administrators enter these values; they should just have a GUI interface 
by means of which to order the list of flags (ala the Mozilla mail filters).
This is, nevertheless, a low priority, and it's not a great sin to make
administrators enter the numbers in the meantime.



Index: editflagtypes.cgi

>Can we "use vars", now that we've figured out the syntax? :-)

Well, ok, why not. (figures out syntax)  Fixed.


>That reminds me; we don't "FILTER html" the variables we print in the
>code-error.html.tmpl template, so this is an XSS hole at the moment. Oops.

Fixed for this patch; I'll leave the rest to a separate bug.


>Nit: missing semi-colon on very end, which is conventionally present.

Fixed.


>This looks suspiciously like we can't have a product or component named 
>"Any".  Is this restriction enforced at creation time? An alternative 
>would be picking a more unlikely string.

The string used is now "__Any__".


>I thought there was an SQL keyword "AS" which is used here? 
>(flaginclusions AS i)

The "AS" is optional but useful.  Added.


>+# Can't be "delete" because that's a Perl reserved word.
>
>I don't think this should be a problem, perhaps as long as you call 
>the sub as &delete(...). One of the advantages of the whole $%@ thing 
>in Perl is that reserved words don't clash with variable names. At least, 
>that's what I thought.

That seems to work.  Changed.


>+	  ThrowUserError("The flag type ID ($id) must be a positive integer.");


>Can you please templatise all these calls? I recently checked in a patch which

>will cause calls like this to fail the tests, so you don't actually have much
>choice :-P

Done.


>Is this enough checking to then pass this name to the database? Same question
>for validateDescription.

Yes and yes.

>At this point in the review, I still haven't worked out what this CC list is
>for... but 200 characters seems like an arbitrary limit. Can't we use one of 
>the SQL types which grow as you use it (like, er, MEDIUMTEXT ;-) ?    

Once 200 characters is too little, admins should be setting up mailing lists
for all the addresses on the CC: list.	I want this feature to be and remain
simple (and it would be trivial to change this size later if necessary), so
I think 200 is the right size.


>Why? Can't we use a larger type in the DB? If someone enters a sort key > 4
>billion, they deserve what they get (bad sorting because of truncation, 
>presumably.)

Because we don't need larger numbers, and columns should be of the smallest
usable type for performance and efficiency.


>Don't we have a js_quote function? I seem to remember moving towards this 
>and away from SqlQuote() for some good reason or other.

The code to which this comment refers has been removed.


Index: productmenu.js

>Is all this stuff chopped out of form.html.tmpl (part of the search page)? 
>If so, can we use this new file there too, please? I'd accept the filing 
>of a new bug and a promise to fix it before 2.18. ;-)

I promise. :-)

http://bugzilla.mozilla.org/show_bug.cgi?id=123030


>On the basis that this is all old code, I haven't reviewed it. Please 
>tell me if it needs reviewing. 

There are some changes.  In particular, the old code assumes that all 
updateable fields are present, while the new code only updates the ones
the calling code specifies (just "component" for flag type editing).


>One thing I do notice, though, is that it's back to using JS
>instead of TT comments, which is bad.

... but necessary now that the code is in a separate file rather than
a template (and the performance costs of templates are most likely
greater than the cost of sending comments over the wire).



Index: request.cgi
===================================================================

>I still can't figure it out - does this CGI show the queue for a given bug? 
>Or a bug and all its attachments? Can we show the queue for a given user? 
>How do you get the UI for sending it parameters?

It shows the queue for all bugs, or the queue for a specific user
(requester or requestee), or the queue filtered by some other criteria.
You should see the UI at the bottom of the screen under the heading
"Filter the Queue".


>Are we really allowing people to turn off flag requests? 

Yup.

>Shouldn't we mail the requester if the requestee hasn't seen their message?

Nope.  Not all requestees will want to watch their email.  Some (especially
those who get bombarded with them) will want to keep track of their requests
by regularly looking at their queue.


>Do you plan to do this, then? :-)

Not at the moment; the comment is there for the future. :-)


>Hmm. Doesn't at least one of your calls to ThrowCodeError() ... omit the 0?

Yes two of them; fixed.


>Anyway (see below) this should be done differently.

Nope (see below).


>Could we have a version of clear() which took a list and made one SQL call? 
>At first glance, this seems inefficient.

We could delete the records all at once, but we'd still have to retrieve them
first to send mail about the clearing.	Since this code is rarely run on many
flags at once (that only happens when the admin deletes a flag type), I think
it's a rare enough inefficiency that it isn't worth spending much time on it.


>"Creates a bug report"? :-) I must be reviewing the wrong patch, then...

Hehe.  Fixed.


>Can we suppress it in the standard way (use vars) instead? Everyone 
>understands that... (at least, they do now. ;-)

(/me acquires an understanding) Done.


>use vars qw($template $vars); 
>should do the trick, shouldn't it?

So it does, although strangely I have to refer to vars as "$vars"
but template as "$::template".


>Do we not have a Bug.pm for this?
>And an Attachment.pm?
>If they are too heavyweight, we need a way of creating lightweight ones.

Bug.pm is way too heavyweight, and lightening it would require serious
(a.k.a. complete) rewriting.  GetBug() is a useful hack in that case.
Attachment.pm, on the other hand, has no "get()" function at all, so
I moved GetAttachment to "new" in it.


Index: Bugzilla/FlagType.pm

><later> Oh, I see. Say "@tables" instead of "the tables list". You should 
>also specify whether the separator goes before or after the name (I assume 
>before, but you should say.)

Ok, done.

>Shouldn't there be a query object which deals with all of this stuff, with
>methods like add_table($table) and so on? 

Maybe, but probably not; it seems like overkill.  I only separated out
the various aspects of query generation as much as I did (giving you the
opportunity to envision object orientation) so I could share code between
the get, match, and count functions.

>We already have the start of one in Search.pm. 

That one is very query.cgi-specific.


Index: Bugzilla/User.pm

>Will an instance of this be replacing the current $vars->{'user'} object? 
>That would make sense, but we have to think about interface compatibility.

Yes, and yes.


>+    push(@users, new Bugzilla::User(&::FetchSQLData())) while
&::MoreSQLData();

>Isn't it inefficient to do one query for each user you care about, rather than

>asking about the lot in one go?

Yes, but that's not what I'm doing.  The User object constructor only queries
the database if it hasn't been handed the data with which to initialize
the user.  In this case I hand it the array returned by &::FetchSQLData(),
which contains just the data it needs to initialize the user, so it uses
that data instead of querying.


>+sub email_prefs {
>+    # Get or set (not implemented) the user's email notification preferences.


>I'm not convinced by this approach. I think getting and setting should be 
>two separate functions (given that you are not declaring the sub as an 
>lvalue (yes, I've been reading Programming Perl today.)) Two separate 
>functions is the OO norm.

Not in Perl, where combining the getter and setter functions is the standard.


>Can we offer a third link here - "deactivate instead"?

Done.


>+  [% make_js_array(products, components_by_product, "cpts") %]

>OK, if it can't be a FILTER, can it be a template instead? :-)

Done, or rather, it was added to the existing templates, since it's pretty
much a one-liner in TT.


>+  [% ELSIF error == "action_unrecognized" %]
>+    I don't recognize the value <em>[% variables.action %]</em>

>FILTER html?

Fixed.


>But also, I may not have made the expected use of the "variables"
>hash clear enough.

You did, but here's the problem: it's much easier and more convenient to pass
variables to the error template via a parameter to ThrowUserError.  In the
typical case where I have two variables to pass, using the intended approach
means three lines of code in the CGI, while passing it as a parameter means
one (unless the call spans multiple lines because of length).  The single-
line approach also makes it possible to say "DoSomething || ThrowUserError".

I imagine you designed it with the assumption that data needing to be
displayed to the end user was probably already in $vars, but that isn't
the case for almost any of my calls.  Passing this data via a parameter
doesn't do any harm and helps out a lot.  It's so useful, in fact, that
this version of the patch adds an "addl_vars" parameter to ThrowCodeError
whose values are slurped into $vars by that function.  I imagine we could
do this with ThrowUserError as well, not to mention change the order of
the parameters so the "unlock tables" parameter, which is rarely used,
is last.


>+  [% ELSIF error == "flag_nonexistent" %]
>+    There is no flag with ID #[% variables.id %].
>+  
>+  [% ELSIF error == "flag_status_unknown" %]
>+    I don't understand the flag status [% variables.status %].

>FILTERs also needed.

Fixed for the second one.  The first is already known to be an integer.


>The a href business here can also be achieved by setting "url" and "link"
>parameters. But I have sort of thought they sucked for a while; is your 
>non-use of them deliberate?

Nope, I just didn't think of them.  In TT land they may not help much.
    

Index: template/en/default/global/select-menu.html.tmpl

>-  [% ELSIF values_type.search("HASH") %]
>+  [% ELSIF options_type.search("HASH") %]

>Is this a bug fix?

Yup, probably wasn't noticed before because no code uses the hash version.


Index: template/en/default/request/created-email.txt.tmpl

>What happens if the requestee has email off and the CC list is empty? Same
>question for the other emails.

That gets checked by the module, and the email doesn't get sent in that case.

Index: template/en/default/request/queue.html.tmpl

>Is all this BLOCK-related stuff worth it just to avoid typing <td></td> ten
>times? :-) I think it might actually be less verbose if you just included all
>the code up here...

Perhaps, but then I'd have to surround every <td> with an [% IF %] statement
to satisfy the requirements of the NEXT line.

Comment 102

[Gervase Markham [:gerv]]

>>+    cc_list		  VARCHAR(200)	NULL , 
>>
>>Er... huh? :-)
> 
> It should be possible to copy addresses that aren't users in Bugzilla 
> (f.e. mailing lists), so we don't want this to be connected to 
> the profiles table, and it shouldn't be possible to add too many addresses
> here, since installations should use mailing lists for that.

Allowing CC of non-account email addresses is a reasonably big policy change,
isn't it? 

Also, I don't understand why this field is attached to the flag_types table.
Does this mean an address is CCed on all requests for changes in that particular
flag?

Is this a function to support [e-mail]?
 
>>One thing I do notice, though, is that it's back to using JS
>>instead of TT comments, which is bad.
> 
> ... but necessary now that the code is in a separate file rather than
> a template (and the performance costs of templates are most likely
> greater than the cost of sending comments over the wire).

If it's only skipping over comments, and given that we've instantiated the
template engine anyway, the cost can't be that great. The size of the comments,
without any of the whitespace surrounding them, is 4.2K - a second or so on a
56k modem.
 
>>I still can't figure it out - does this CGI show the queue for a given bug? 
>>Or a bug and all its attachments? Can we show the queue for a given user? 
>>How do you get the UI for sending it parameters?
> 
> It shows the queue for all bugs, or the queue for a specific user
> (requester or requestee), or the queue filtered by some other criteria.
> You should see the UI at the bottom of the screen under the heading
> "Filter the Queue".

Over in the generic reporting patch, I've made a new format for query.cgi which
presents the entire "select a set of bugs" UI, with a couple of axes to plot
with the bugs you've selected. Could we not do a similar thing here? Have a
query.cgi template which gives you the full query UI, then extra email filter
checkboxes for "requester" and "requestee"? You could also have a second copy of
the bug number box prominently near the top.
 
>>use vars qw($template $vars); 
>>should do the trick, shouldn't it?
> 
> So it does, although strangely I have to refer to vars as "$vars"
> but template as "$::template".

That's very odd. $template works for me, and in most of the other CGIs :-) 
 
>>Do we not have a Bug.pm for this?
>>And an Attachment.pm?
>>If they are too heavyweight, we need a way of creating lightweight ones.
> 
> Bug.pm is way too heavyweight, and lightening it would require serious
> (a.k.a. complete) rewriting.

Fair enough; but could you add a comment saying "Ideally, this would use Bug.pm,
but..."?

> doesn't do any harm and helps out a lot.  It's so useful, in fact, that
> this version of the patch adds an "addl_vars" parameter to ThrowCodeError
> whose values are slurped into $vars by that function.  I imagine we could
> do this with ThrowUserError as well, not to mention change the order of
> the parameters so the "unlock tables" parameter, which is rarely used,
> is last.

<sigh> Not another change in the error APIs... :-| I see your point, though. OK,
I'll buy this - but I think we should change the API in a separate patch, so we
can do it all in one go and get the $unlock_tables thing right as well. I'll
file a bug and knock one up ASAP.

Gerv

Comment 103

[Myk Melez [:myk] [@mykmelez]]

>Allowing CC of non-account email addresses is a reasonably big policy change,
>isn't it? 

It's just for request mail, not for mail in general.


>Also, I don't understand why this field is attached to the flag_types table.
>Does this mean an address is CCed on all requests for changes in that 
>particular flag?

Yup.


>Is this a function to support [e-mail]?

Yup.
 

>If it's only skipping over comments, and given that we've instantiated the
>template engine anyway, the cost can't be that great. The size of the 
>comments, without any of the whitespace surrounding them, is 4.2K - a second
>or so on a 56k modem.

But a separate JS file is cached by the browser, whereas inline JS is not.

 
>Over in the generic reporting patch, I've made a new format for query.cgi 
>which presents the entire "select a set of bugs" UI, with a couple of axes 
>to plot with the bugs you've selected. Could we not do a similar thing here?

Maybe, but I think there's value in searching directly against flags rather than
searching for bugs with flags set, if only for efficiency.  There's also value
in presenting a very simple interface to the queue.  The queue also supports
grouping, and the regular query.cgi has options in the boolean chart for
searching flags if that's your thing, so altogether I don't think this is
necessary, at least not now.
 

>That's very odd. $template works for me, and in most of the other CGIs :-)

This call isn't in a CGI however, it's in a module.  Things seem surreally
different there, if only slightly, kind of like being in the Mozilla community.

 
>Fair enough; but could you add a comment saying "Ideally, this would use 
>Bug.pm, but..."?

Ok, will do.  Got any more review comments, or shall I post an updated patch
with this change?

Comment 104

[Gervase Markham [:gerv]]

> >If it's only skipping over comments, and given that we've instantiated the
> >template engine anyway, the cost can't be that great. The size of the 
> >comments, without any of the whitespace surrounding them, is 4.2K - a second
> >or so on a 56k modem.
> 
> But a separate JS file is cached by the browser, whereas inline JS is not.

Separate is good. My idea was that it could be generated by page.cgi. But then
would the parameter on it mean that it wouldn't be cached?
 
Other than this dicussion, no further comments. Attach a new patch and go
looking for a second reviewer. :-) When groups has landed, you'll have to merge,
and I'll take another look then, at both that, and the implementation of my last
set of suggestions. Does that sound OK?

Gerv

 

Comment 105

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v28: review fix, unrotted

>Separate is good. My idea was that it could be generated by page.cgi. But then

>would the parameter on it mean that it wouldn't be cached?

Caching is complicated with CGIs, but in any case running it through page.cgi
means a separate CGI script and template object instantiation, which negates
any template comment performance advantage.

>When groups has landed, you'll have to merge, and I'll take another look then,
>at both that, and the implementation of my last set of suggestions. Does that
>sound OK?

As long as groups lands soon, that's fine.  There's so little groups stuff in
my patch, however, that it doesn't make sense to wave one off for the other (in
either direction) for too long.

Here's a new patch with the added comment, plus it applies to the tip (grr, the
error templates are constantly full of conflicts).

Comment 106

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v29: unrotted

This patch is updated for the groups check-in and Throw(User|Code)Error API
changes.

Comment 107

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch 30: more unrotting and a bug fix

This patch finishes unrotting the code from the groups check-in and fixes a bug
in the FlagTypes matching code.

Comment 108

[Bradley Baetz (:bbaetz)]

Functional stuff:

- the checksetup conversion doesn't handle renamed attachment statuses. You
should lose the INSTR too.
- updated attachment status->flags have empty inclusion/exclusion lists - they
should have a sinlge entry of <product>:__ANY__. Do we want to merge existing
statuses with the same name/desc on different products automatically? If not, we
want a way of merging them later, so we don't lose them (eg combining
browser/mailnews)

- the 'Flag:' header in show_bug appears even if there are no valid bug flags.
- What happens when a bug/attachment has a flag set, then that product/component
is added to the exclusion list/removed from teh inclusion list?

I have started on teh code review, but I got sidetracked this evening and didn't
get to finish. Hopefully tomorrow.

Comment 109

[Bradley Baetz (:bbaetz)]

Attachment: review

Enjoy!

Comment 110

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v31: review fixes

Review fixes.  Review response to follow shortly.

Comment 111

[Myk Melez [:myk] [@mykmelez]]

Attachment: response to bbaetz' review

Comment 112

[Bradley Baetz (:bbaetz)]

Comments on the review (w/o having looked at the patch):

>>Notice the duplication here - I have half a patch which moves some of this
>>code to Bug.pm, and ends up deleting bug_form.pl alltogether. Not finished
>>yet, though.
>I'm not sure which duplication you mean, but I can't wait to see your patch,
>which is sorely needed.

Yeah, I should tidy that up after RT goes in. The duplication here was referring
to how you have to change Bug.pm too, although, now that I think of it - did you
change that? ie does the xml output include bug flags now?

>> Why doesn't validateStatus do that? [detaint teh element]

> Because the status is part of %::FORM, and I can't detaint an individual
> member of that hash, can I?

Sure you can - each scalar is considered separately.

[The only thing you have to be careful of is that there seems to be some sort of
perl5.005 issue if you convert between arays and hashes (tahts why TT ends up
considering all of the vars to be tainted if just one of them is, I think).
You're not doing that here, though.]

>>You can from the products/components table from this join then, I think.
>Only if I look up the product and component names later, since they are used
>by various parts of that script.  I think the JOIN is more efficient than
>three separate queries.

Oh, I misread that. NM

>>+my $user_cache = {};
>>
>>I'm wary about this breaking if someone changes their settings - is that
>>possible (ignoring mod_perl)
>Ignoring mod_perl, the cache is reset for each CGI, and User.pm doesn't deal
>with user settings changes, so this isn't an issue until then, at which point
>user settings changes can be made to go through User.pm, which will update
>the cache as needed.

I was thinking about someone using editusers/prefs to edit their own settings
(w/o going through User.pm), then footer text being wrong, or something. I think
we can agree not to care about that for the moment, though.

>> multiple="multiple"
>Aren't we not doing that now?

No, its the <foo /> which we're not doing.

> Remaining issues:
> 1. Where to put the template files.

/me points to gerv ;)

Comment 113

[Gervase Markham [:gerv]]

Index: attachment.cgi
===================================================================
-    print qq{Content-Disposition: attachment; filename=$filename\n} if
$usedisposition;
+#    print qq{Content-Disposition: attachment; filename=$filename\n} if
$usedisposition;

Oops.

Index: defparams.pl
===================================================================
+  {
+   name => 'wildcardminchars',
+   desc => 'Some user fields in Bugzilla accept inexact strings and match ' .
+           'them to actual users.  To prevent users from entering strings ' .
+           'that match too many users (and use too many system resources), ' .
+           'set this value to the minimum length the strings they enter ' .
+           'have to be.  Note that this cannot be shorter than the smallest ' . 
+           'username in your system (and be careful with emailsuffix).',
+   type => 't',
+   default => '3',
+   checker => \&check_numeric
+  },

When people add usernames, do we check that they are longer than this value?
Also, you should explain that emailsuffix is (or isn't) included in the count,
rather than saying "be careful of it.".

+  {
+   name => 'showrequestqueue',
+   desc => 'If one or more of your flags are requestable, do you want ' . 
+           'a link at the bottom of the page to a queue of pending requests?',
+   type => 'b',
+   default => 0
+  },

Why is this a Param at all, and why does it default to 0? Surely this is a case
for saying that you can edit the templates if you don't want this link.

Index: Bugzilla/Flag.pm
===================================================================

+sub MatchRequestees {
+    my ($flag) = @_;

Wasn't this going to be a generic implementation of this autocomplete function?
 Or did that idea die?
    
+    my $requestee_str = $flag->{'requestee_str'};
+    
+    # To reduce the size of queries, require the user to enter at least 
+    # three characters of each requestee's name.
+    if (length($requestee_str) < &::Param("wildcardminchars")) {
+        &::ThrowUserError("requestee_too_short");
+    }

Can we do exact matching only if it's too short? This would almost eliminate the
need for a Param - you could hard-coded it at three, and assume if someone put
in "bz", they are looking for the user named bz.

Index: template/en/default/admin/request-type/edit.html.tmpl
===================================================================

+[% header_html = BLOCK %]
+  <script language="JavaScript" type="text/javascript"
src="productmenu.js"></script>
+[% END %]

I'm sure I've got a patch somewhere getting rid of header_html... Ah, well. I'll
have to fix either it or this :-)

+[% IF type.target_type == "bug" %]
+  [% title = "Create Flag Type for Bugs" %]
+[% ELSE %]
+  [% title = "Create Flag Type for Attachments" %]
+[% END %]

bbaetz: are you reading this? :-P

+  [% ELSIF error == "flag_type_description_invalid" %]
+    [% title = "Flag Type Description Invalid" %]
+    The description must be less than 32K.

Why do we bother? It's tempting just to truncate. ;-) Don't change it, though.
    
+  [% ELSIF error == "requestee_too_short" %]
+    [% title = "Requestee Name Too Short" %]
+    One or two characters match too many users, so please enter at least 
+    three characters of the name/email address of the user you want to set 
+    the flag.

Oops. Param required for "three".
  
Fix this lot, check that there are no other accidental inclusions in the patch,
and r=gerv.

Gerv

Comment 114

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v32: more review fixes

This patch fixes an issue in checksetup.pl which prevented attachment statuses
from being migrated to flags correctly and also responds to the following
review (sent to me from Gerv via email):

Index: attachment.cgi

>-    print qq{Content-Disposition: attachment; filename=$filename\n} if
$usedisposition;
>+#    print qq{Content-Disposition: attachment; filename=$filename\n} if
$usedisposition;
>
>Oops.

Something's wrong with Bugzilla tip, although I've heard it's Mozilla's fault.
Going to "edit attachment" makes Mozilla ask you to download the attachment
instead of seeing it in the iframe.  This hack made it go away.  Removed.


Index: defparams.pl

>+   name => 'wildcardminchars',
   ...
>When people add usernames, do we check that they are longer than this
value?...

I changed tactics, now I hard-code the three-character minimum
but make an exception for sites with emailsuffix set to something.
Since those sites are likely to be small (otherwise everyone
wouldn't have the same email suffix), this should work fine.

>+   name => 'showrequestqueue',
>
>Why is this a Param at all, and why does it default to 0?
>Surely this is a case for saying that you can edit the templates
>if you don't want this link.

Or vice-versa perhaps.	It's not clear that most installations will have
requestable flags, so this makes it easy for those that do to turn on
the queue.  Let's see how this shakes out in practice.


Index: Bugzilla/Flag.pm

>+sub MatchRequestees {
>+    my ($flag) = @_;
>
>Wasn't this going to be a generic implementation of this autocomplete 
>function?  Or did that idea die?

Bug 162990.  But I reviewed it today, and it still needs work.


>+    # To reduce the size of queries, require the user to enter at least
>+    # three characters of each requestee's name.
   ...
>Can we do exact matching only if it's too short?...

Not a bad idea, but see tactic outlined above, which I think is 
Good Enough (TM).


Index: template/en/default/admin/request-type/edit.html.tmpl
===================================================================

>+  [% ELSIF error == "requestee_too_short" %]
>
>Oops. Param required for "three".

Not anymore. :-)


>Fix this, check that there are no other accidental inclusions 
>in the patch, and r=gerv. Good luck checking it in :-)

As the evil Russian says in the classic American film Hot Dog: "Luck, who needs
it!"

Comment 115

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v33: more review fixes

fixed a few more problems in the conversion code and drastically improved its
performance (thanks bbaetz)

Comment 116

[Bradley Baetz (:bbaetz)]

Comment on attachment 100974 [details] [diff] [review]
patch v33: more review fixes

   + my $old_field_id = $sth->fetchrow_arrayref()->[0]

This is actually identical to |$sth->fetchrow_array()|, since you are only
selecting one column. (fetchrow_array uses |wantarray| to short circuit
returning all teh values when called in a scalar context, although its
behaviour is undefined if you were selecting more than one column)

Also, I may have changed my mind on the footer param :) I was thinking it
should default to on, but then it may as well just be template removed if
someone doesn't want it.

r=bbaetz either way

(One thing I did notice, in the search code with the CONCAT you use - would it
be better to split that off in teh perl code, and then search on teh two
elements separately? Taht would allow you to give error for people who search
on 'foo' instead of 'foo+', and possible also speed up the searching, too.

Comment 117

[Myk Melez [:myk] [@mykmelez]]

Re: CONCAT in search, I'll run some tests, and if it helps to split them up in
Perl code I'll make a patch to do it.

Re: "requests" footer link, ok, I'll remove the param and just let people remove
it from the template if they don't want it.

Gerv- The only remaining issue is whether to move template files to flag/ and
admin/flag-type/ or leave them in request/ and admin/request-type/.  My
preference is to move them (except for queue.html.tmpl, which belongs in
request/), as it creates less long-term confusion to name things appropriately,
even if we have extra empty directories around.  bbaetz didn't express a
preference.  Do you feel strongly one way or the other?

Also, I made some changes between v31 and v33 (the latest version).  Please take
a look and confirm that they are ok with you.

Comment 118

[Gervase Markham [:gerv]]

Comment on attachment 100974 [details] [diff] [review]
patch v33: more review fixes

> Re: "requests" footer link, ok, I'll remove the param 

Great.

> Gerv- The only remaining issue is whether to move template files to flag/ and
> admin/flag-type/ or leave them in request/ and admin/request-type/.  

I have a strong-ish preference for you creating new, sensibly named
directories. Fight cruft! :-)

r=gerv.

Gerv

Gerv

Comment 119

[Myk Melez [:myk] [@mykmelez]]

Checking in Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.11; previous revision: 1.10
done
Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.26; previous revision: 1.25
done
Checking in bug_form.pl;
/cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v  <--  bug_form.pl
new revision: 1.105; previous revision: 1.104
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.193; previous revision: 1.192
done
Removing editattachstatuses.cgi;
/cvsroot/mozilla/webtools/bugzilla/editattachstatuses.cgi,v  <-- 
editattachstatuses.cgi
new revision: delete; previous revision: 1.9
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.28; previous revision: 1.27
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
initial revision: 1.1
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.30; previous revision: 1.29
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.206; previous revision: 1.205
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.149; previous revision: 1.148
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/productmenu.js,v
done
Checking in productmenu.js;
/cvsroot/mozilla/webtools/bugzilla/productmenu.js,v  <--  productmenu.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/request.cgi,v
done
Checking in request.cgi;
/cvsroot/mozilla/webtools/bugzilla/request.cgi,v  <--  request.cgi
initial revision: 1.1
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.49; previous revision: 1.48
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.42; previous revision: 1.41
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
initial revision: 1.1
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
initial revision: 1.1
done
Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.7; previous revision: 1.6
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/confirm-delete.html.tmpl,v
done
Checking in template/en/default/admin/flag-type/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/confirm-delete.html.tmpl,v
 <--  confirm-delete.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/edit.html.tmpl,v
done
Checking in template/en/default/admin/flag-type/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/edit.html.tmpl,v
 <--  edit.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/list.html.tmpl,v
done
Checking in template/en/default/admin/flag-type/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/list.html.tmpl,v
 <--  list.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.12; previous revision: 1.11
done
Checking in template/en/default/attachment/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v
 <--  list.html.tmpl
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--
 edit.html.tmpl
new revision: 1.20; previous revision: 1.19
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/flag/list.html.tmpl,v
done
Checking in template/en/default/flag/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/flag/list.html.tmpl,v 
<--  list.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v
 <--  code-error.html.tmpl
new revision: 1.12; previous revision: 1.11
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v
 <--  messages.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/global/select-menu.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/select-menu.html.tmpl,v
 <--  select-menu.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/global/useful-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl,v
 <--  useful-links.html.tmpl
new revision: 1.11; previous revision: 1.10
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.10; previous revision: 1.9
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/created-email.txt.tmpl,v
done
Checking in template/en/default/request/created-email.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/created-email.txt.tmpl,v
 <--  created-email.txt.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/fulfilled-email.txt.tmpl,v
done
Checking in template/en/default/request/fulfilled-email.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/fulfilled-email.txt.tmpl,v
 <--  fulfilled-email.txt.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/queue.html.tmpl,v
done
Checking in template/en/default/request/queue.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/queue.html.tmpl,v
 <--  queue.html.tmpl
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/verify.html.tmpl,v
done
Checking in template/en/default/request/verify.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/request/verify.html.tmpl,v
 <--  verify.html.tmpl
initial revision: 1.1
done

Comment 120

[Myk Melez [:myk] [@mykmelez]]

Attachment: the version that was checked in

Here's the version that was checked in.  The only changes are the previously
discussed removal of the param around the "requests" link in the footer, moving
of template files to more appropriate locations, and some whitespace and
inadvertent patch inclusion clean-up.

Comment 121

[victory <[e-mail]>]

Attachment: docs patch about 'Bugzilla Database Tables' section, for 2.18

Comment 122

[victory <[e-mail]>]

Attachment: docs patch about 'Bugzilla Database Tables' section, for 2.18 v2

hmm, exclusion/inclusion also this?

Comment 123

[timeless]

Comment on attachment 214283 [details] [diff] [review]
docs patch about 'Bugzilla Database Tables' section, for 2.18 v2

>+flagexclusions:  This table stores which products/components are
>+explicitly excluded from the flags.

from /specific/ flags ?

>+flaginclusions:  This table stores which products/components are
>+explicitly included from the flags.

from /specific/ flags ?

>+flags:  This table stores which bugs/attachments the flags used, when
>+the flag was created/modified, requester, requestee, status.

I'm not sure what this is trying to say. Is this for instances of flags?

If so...

This table stores flags, the bugs/attachments with which they are associated, when they were created/modified, the requester, requestee and status.

If not. I dunno. I'm tired.

>+flagtypes:  This table stores definition of the flags.

stores /the flag definitions/.