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.

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.

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!

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

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

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...

>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.

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 john@mozilla.org 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, reviewers@mozilla.org, 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?

------- 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 john@mozilla.org 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, reviewers@mozilla.org, 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 reviewers@mozilla.org situation is not an issue. One solution to the john@mozilla.org 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.

> 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 > reviewers@mozilla.org 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.

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.

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

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.

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.

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.

Latest patches are running at: http://landfill.tequilarista.org/bz98801/

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

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.

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.

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.

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 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.

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.

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=foo@bar.com" 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.

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.

>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.

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.

> 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!

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.

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 :)

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?

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.

>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.

2.16+ ? =)

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 [ gerv@mozilla.org ] 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 <gerv@mozilla.org> 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 <gerv@mozilla.org> 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

Unrotted version. Gerv

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

>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! :->

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

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

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 reviewers@mozilla.org, 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?

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...

>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 reviewers@mozilla.org, >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 reviewers@mozilla.org 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.

> The QA contact field is a perfect match for review requests, because > reviewers@mozilla.org 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

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?

>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.

>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.

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.

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 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 <myk@mozilla.org> + +############################################################################## ## +# 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 <myk@mozilla.org> #%] +[%# 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

> 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/

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 [gerv@mozilla.org ] 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

> 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. :-)

> > 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 pdt@netscape.com 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.]

> 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).

>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.

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

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

> 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?

> 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

>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 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

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.

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.

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.".

[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, jake@bugzilla.org, 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

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

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.

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.

When a name (f.e. "gerv@mozilla.org") is a subset of another name (f.e. "gerv@mozilla.org.uk") 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.

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?

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.

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??)

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

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

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

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.

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

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.

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.

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

"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

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.

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 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 myk@mozilla.org: 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 <myk@mozilla.org> >+ >+################################################################################ >+# 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 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 myk@mozilla.org: 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 <myk@mozilla.org> >+ >+################################################################################ >+# 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. >+}

>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.

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.

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.

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.

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 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.

>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 "?").

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

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

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.

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.

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

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.

>>+ 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 reviewers@mozilla.org? >>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

>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 reviewers@mozilla.org? 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?

> >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

>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).

This patch is updated for the groups check-in and Throw(User|Code)Error API changes.

This patch finishes unrotting the code from the groups check-in and fixes a bug in the FlagTypes matching code.

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.

Enjoy!

Review fixes. Review response to follow shortly.

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 ;)

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

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!"

fixed a few more problems in the conversion code and drastically improved its performance (thanks 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.

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 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

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

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.

hmm, exclusion/inclusion also this?

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/.