84338 - RFE: enhanced attachment, patch and request tracking

Status: RESOLVED FIXED

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

Description

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

I recently developed an enhancement to Bugzilla to enable users to edit
information about attachments and flag them with statuses.  Attachment statuses
show up in the attachment list when viewing a bug, and users can query for bugs
with certain attachment statuses.

Other features of the tool include the ability to mark attachments invalid,
causing their descriptions to appear with strike-through style when viewing a
bug, and the ability to view all attachments on one page in a series of iframes.

I also created a second enhancement to Bugzilla to enable users to request
changes to data in Bugzilla from other users (f.e. setting a status on an
attachment).  Requests generate email to the requestee and go into a queue
stored in a database table.  When the requestee makes the change specified in
the request, the request is marked fulfilled in the database, and the requester
is notified via email.  Users can also view the queue of pending requests to
track them.

Both enhancements are implemented as a set of new files (module, script,
templates) along with hooks into several existing Bugzilla scripts.  While both
enhancements were developed to meet specific mozilla.org needs to track the
patch review process, they both store their definitions (attachment statuses and
request types) in database tables so they can be completely customized for other
uses.

I set up a test Bugzilla installation and filed a bug with several patches to
demonstrate the tools:

http://landfill.tequilarista.org/myk/bzpacman/show_bug.cgi?id=3

Create an account for yourself if you want to experiment in a more significant way.

http://landfill.tequilarista.org/myk/bzpacman/

I am writing technical documentation on the enhancements and will post it as it
becomes available.

Comment 1

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

Attachment: The set of new files implementing the enhancements.

Comment 2

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

Attachment: Hooks into existing Bugzilla files.

Comment 3

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

Attachment: A sample set of attachment status and request definitions.

Comment 4

[Andreas Franke (gone)]

This may be able to solve bug 75176 "marking attachments as 'obsolete'".
At least it's closely related.

Comment 5

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

If you try to view an attachment whose MIME type cannot be displayed by your web
browser (f.e. application/x-tar), your browser asks you if you want to download
it.  This happens on the "View All Attachments" page as well as the "Edit
Attachment" page.  This is basically just a pain in the butt since the solution
is to cancel the "Download" dialog.  On the other hand, many pains in the butt
add up to an unusable application.

I'm not sure what the solution is here.  One possibility is to only display
attachments whose MIME type is on a list of "approved viewable types," and then
periodically add types to the list as bugs get filed that "text/foo" is not
recognized.  I suppose we could also assume that "text/*" is fine while
"application/*" is not (unless otherwise set).

Comment 6

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

Attachment: Version 2 gzip archive

Comment 7

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

Attachment: Version 2 patches

Comment 8

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

Additions in this version include:

1. Ability to convert an attachment into a mail-quoted comment field for
reviewers who want to comment on specific portions of the patch without having
to cut-and-paste.
2. Changes to attachments show up in the bugs activity list.
3. Searches for bugs with attachments having certain statuses is much faster.
4. When submitting a request, there is a comment field for adding comments
specific to the request.
5. Bug requests have been implemented (but there isn't yet a link on the "show
bug" page for making a request against that bug).
6. Obsolete attachments appear in strike-through style.
7. Attachments that cannot be displayed aren't, so your browser doesn't ask you
if you want to download it (unless you click the link to "view" the attachment).
8. The table listing attachments on the "show bug" page resizes better in
Netscape 4.x.
9. A bunch of bug fixes.

The installation at landfill has been updated with the latest patches.

http://landfill.tequilarista.org/myk/bzpacman/

Comment 9

[Dave Miller [:justdave]]

Comments on bzpacman demo:

1) when I "view all" on the attachment list, attachments that are not text show 
as "can't view this attachment" with a link to download it (good) but text 
attachments do not show at all.  All I get is the headers for those attachments.

2) on the request screen, unless you're someone who is intimately familiar with 
"The Mozilla Process", you're going to have no clue what anything means on that 
screen.  I think it's going to need some more on-screen instructions on that 
screen about what it does.

3) I'm not sure if it's in there or what kind of admin tools are provided for it, 
but it'd be nice to be able to disable some of those fields (for example, we 
don't have a "super-review" at any of my sites, so just review and checkin would 
be sufficient).  Disabling requests altogether might be appropriate for sites 
that use Bugzilla for tech support rather than tracking an open-source product.  
(you can attach screen shots and the like, but who would ever need to request a 
checkin for screen shots? ;)  Doing this by product may be desirable, too.  On 
InTrec, for example, we have one open-source product, one closed-source 
commercial product, a tech support issue tracking product, and a server issues 
product.  The open-source one we'd obviously want all this stuff.  The closed 
source one the bugs are public, but patches don't get posted to the system, for 
obvious reasons, so there'd be no point in requests for that one.  Likewise for 
tech support.

Comment 10

[Dave Miller [:justdave]]

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

Comment 11

[Dave Miller [:justdave]]

Since 2.16 has about 5 times the number of bugs targeted at it than 2.14 had, I 
would really like to see this be one of the first things checked in after 2.14 is 
released, and see bugzilla.mozilla.org update to use it before we go any further 
with 2.16 checkins. :-)

Comment 12

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

---
1) when I "view all" on the attachment list, attachments that are not text show 
as "can't view this attachment" with a link to download it (good) but text 
attachments do not show at all.  All I get is the headers for those attachments.
---

The "view all" screen uses IFRAMES to display the attachments, so your browser
must support IFRAMES to view them.

---
3) I'm not sure if it's in there or what kind of admin tools are provided for it, 
but it'd be nice to be able to disable some of those fields (for example, we 
don't have a "super-review" at any of my sites, so just review and checkin would 
be sufficient).
---

Attachment status flags are *completely* configurable at
yourinstallation/attachment.cgi?action=adminDefs.


---
  Disabling requests altogether might be appropriate for sites 
that use Bugzilla for tech support rather than tracking an open-source product.  
---

Disabling requests is just a matter of going to the "edit parameters" screen and
turning "userequestmanager" off.
 
---
Doing this by product may be desirable, too.
---

Hmm... that is a very interesting idea. :-)

Comment 13

[Dave Miller [:justdave]]

It should state on the page somewhere that it requires IFRAMEs then.  There's a 
lot of people out there that still use Navigator 4.x :-)

Comment 14

[Matthew Tuck [:CodeMachine]]

Can we sniff?

Comment 15

[Jacob Steenhagen]

Sure, just breath in through your nose :)

Seriously, IIRC the "correct" way to do this would be:
<iframe src="attachement_url">Your browser doesn't support IFRAME's, so you
can't see this attachment</iframe>

Which would display the error if the browser doesn't support IFRAME, but would
look "normal" if it does.

Comment 16

[Dave Miller [:justdave]]

I'm going to make a valiant attempt to get this included in 2.14 before b.m.o 
updates.  That way we have it available to us when triaging the patches for 2.16 
(of which we have over 120 waiting for review).

Comment 17

[Dave Miller [:justdave]]

There have been no new patches presented for this patch since the last time it
was reviewed, and the issues that Jake and I brought up last time still haven't
been dealt with (if they have been, the patches weren't posted)

i.e. it needs more work.

Comment 18

[Dave Miller [:justdave]]

To summarize the issues from above that need work still:

1) a suitable error should be shown to the user (Jake gave an example how) in
browsers that don't support IFRAME when showing a text/plain attachment.

2) the page needs a little more information in layman's terms about what you're
looking at, so that people who have no clue about the Mozilla Process can figure
it out.

3) The status prefs need to be by-product and not global.  Bugzilla's probably
not going to have the same status requirements as Mozilla for example.

Comment 19

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

The first two issues on Dave's list have been addressed on the test installation
on landfill (patches not yet posted to this bug).  I'm going to look into the
third one tomorrow.

Comment 20

[Dave Miller [:justdave]]

points 1 and 2 r=justdave.  Looks good so far :-)

Comment 21

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

Attachment: Version 3 gzip archive

Comment 22

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

Attachment: Version 3 patches

Comment 23

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

The latest version has been attached to this bug, and the landfill test
installation has been updated to it.  This version contains the following changes:

1. All three of Dave's issues are addressed.  In particular, attachment statuses
are now configurable on a per-product basis.

2. Templates have all been moved into the pre-existing "template" directory, and
the directory structure has been reorganized for compatibility with my proposal
in bug 86168.

3. Fixed a bug where creation timestamps were getting updated when an attachment
was modified.

Comment 24

[Dave Miller [:justdave]]

I have my local install cocked and ready to commit for this.

However, there are people (myself included) who are nervous about checking in
something this big so close to release.  What I need to know is if mozilla.org
is going to be willing to update to the tip again within a week AFTER 2.14 being
released as well as the week before?  If so, I'd like to push this off for 2.16
and it'll be the first thing we check in after the version is bumped to 2.15.

On the other hand, Myk, if you're going to be around the week the initial update
is done to catch all the issues that crop up as a result of having this live,
I'll feel safer checking it in as part of 2.14.

(what's here now has my r=, although I can think of a couple small issues, it's
quite functional now, and we can make it pretty later :)

Comment 25

[Dave Miller [:justdave]]

Dawn says updating twice will be no biggie since there'll be very few changes in
between, so this is going back to 2.16.  This will be the FIRST thing we check
in as soon as the version number is rolled to 2.15.

Comment 26

[Dave Miller [:justdave]]

dangit, I said 2.16 :-)

Comment 27

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

I'll be around after the upgrade(s) to catch any changes, so I think it's safe
to get this in for 2.14 (especially since it touchs minimal existing code). 
However, I'm ok with pushing it off to 2.16 as long as we can coordinate the
landing such that I can update b.m.o. to the tip right after it goes in but
before the flood of 2.16 patches start getting checked in.

(I'd like to get the flood on b.m.o. as well, but I want the patch tracker up
and running first and also give the flood a chance to bake on the trunk before
taking it into b.m.o.)

Comment 28

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

Attachment: version 4 gzip archive

Comment 29

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

Attachment: version 4 patches

Comment 30

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

Attachment: version 4 patches -- ignore previous attachment

Comment 31

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

Changes in the latest version:

1. Removed the request tracker from the patch.  The request tracker needs to be
rearchitected, but that shouldn't prevent the patch tracker from going in.

2. Added a field to the bugs_activity table that stores the attachment id for
changes to attachments.  The UI for this is sub-optimal and may need to be
refined.  It may even be the case that attachments should have their own
activity table.  In the meantime, however, this field will ensure no data loss
occurs.

3. Separated attachment tracking (attachment.cgi) from attachment status
administration (attachstatus.cgi) for code clarity and performance.

4. Removed some obsolete code that was still hanging out (in particular the
"console" and "editold" methods and their corresponding templates) and cleaned
up code in general.

5. Added validation checks for all data submitted on the "edit attachment" and
"add/edit attachment statuses" forms.

6. Removed the ability to move attachment status definitions to different
products.  Allowing this to happen would create data integrity problems with
attachments whose statuses move to a different product, and it is not clear
whether those flags should be cleared automatically during the move, the move
should be blocked until no attachments have that status, or the move should
never be allowed at all.  The latter option is the safest until and unless a
reason emerges for one of the former two options to be allowed.

Comment 32

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

The bugs you might have used to test this patch on landfill have been fried in
order for me to ensure the patches install on a fresh installation, so check out
bug #1 instead.

Comment 33

[Dave Miller [:justdave]]

correcting version field lost in product move

Comment 34

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

Attachment: version 4 patches w/updates for bugs_activity and processmail

Comment 35

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

The latest patches modify processmail and the DumpBugActivity function in CGI.pl
to include the attachment number when displaying changes made to the attachments
of a bug.

Before, processmail would not include the attachment number at all, and CGI.pl
would include the attachment number in an ugly way, f.e.: 

Attachment is obsolete (12345)
Now both routines display the attachment number in a pretty way, f.e.:

Attachment #12345 [details] is obsolete
The one significant change this entails is to use fielddefs.description rather
than fielddefs.name when displaying changes to fields in the bugs activity
table.  This means that we are displaying "Attachment is obsolete" rather than
"attachments.isobsolete" as well as "URL" instead of "bug_file_loc".

Doing this also enables me to insert the attachment number into the field name
("Attachment is obsolete" => "Attachment #12345 [details] is obsolete"), and it also looks
a lot nicer.  This change seems to be such a no-brainer, in fact, that I am
suspicious there was some reason why it was not done before.  Therefore,
reviewers should pay close attention to the ramifications of this change.

Comment 36

[Dave Miller [:justdave]]

I can't seem to find any way to edit the attachment statuses aside from knowing
the URL to get to it.  Can we get some sort of link either from the editproducts
page (since they're product specific) or in the command footer if you have
access to it?

Comment 37

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

The schema:

The "attachstatusdefs" table stores the list of statuses that can be set on an
attachment.  Each record consists of a unique integer ID (id), the name of the
status (name), an optional description of the status (description), and an
integer indicating the order in which the status should appear in the UI
relative to other statuses (sortkey).

mysql> describe attachstatusdefs;
+-------------+-------------+------+-----+---------+-------+
| Field       | Type        | Null | Key | Default | Extra |
+-------------+-------------+------+-----+---------+-------+
| id          | smallint(6) |      | PRI | 0       |       |
| name        | varchar(50) |      |     |         |       |
| description | mediumtext  | YES  |     | NULL    |       |
| sortkey     | smallint(6) |      |     | 0       |       |
+-------------+-------------+------+-----+---------+-------+
4 rows in set (0.00 sec)

The "attachstatuses" table stores one record for each status that has been set
on each attachment.  Each record consists of a reference to the ID of the
attachment (attach_id) and a reference to the ID of the status (statusid).

mysql> describe attachstatuses;
+-----------+--------------+------+-----+---------+-------+
| Field     | Type         | Null | Key | Default | Extra |
+-----------+--------------+------+-----+---------+-------+
| attach_id | mediumint(9) |      | PRI | 0       |       |
| statusid  | smallint(6)  |      | PRI | 0       |       |
+-----------+--------------+------+-----+---------+-------+
2 rows in set (0.00 sec)

Comment 38

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

And the REAL schema:

mysql> describe attachstatusdefs;
+-------------+-------------+------+-----+---------+-------+
| Field       | Type        | Null | Key | Default | Extra |
+-------------+-------------+------+-----+---------+-------+
| id          | smallint(6) |      | PRI | 0       |       |
| name        | varchar(50) |      |     |         |       |
| description | mediumtext  | YES  |     | NULL    |       |
| sortkey     | smallint(6) |      |     | 0       |       |
| product     | varchar(64) |      |     |         |       |
+-------------+-------------+------+-----+---------+-------+
5 rows in set (0.03 sec)
 
mysql> describe attachstatuses;
+-----------+--------------+------+-----+---------+-------+
| Field     | Type         | Null | Key | Default | Extra |
+-----------+--------------+------+-----+---------+-------+
| attach_id | mediumint(9) |      | PRI | 0       |       |
| statusid  | smallint(6)  |      | PRI | 0       |       |
+-----------+--------------+------+-----+---------+-------+
2 rows in set (0.00 sec)

Comment 39

[Dave Miller [:justdave]]

Is there a particular reason for the attachmentstatuses table?  Couldn't we just
add a column to the attachments table for that?

Comment 40

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

>I can't seem to find any way to edit the attachment statuses aside from knowing
>the URL to get to it.  Can we get some sort of link either from the 
>editproducts page (since they're product specific) or in the command footer if 
>you have access to it?

I'll look into this.

>Is there a particular reason for the attachmentstatuses table?  Couldn't we 
>just add a column to the attachments table for that?

No, because there is a 1->N relationship between attachments and their statuses,
and a column in the attachments table would only allow us to store a 1->1
relationship (i.e. one status for each attachment).

Comment 41

[Matthew Tuck [:CodeMachine]]

Well I guess we could have a statusset on attachments like we have a groupset on
bugs.  But then again, maybe not.  =)

Dave, this feature works basically the same as do keywords except it's product
specific and has a sortkey.

Comment 42

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

>Well I guess we could have a statusset on attachments like we have a groupset 
>on bugs.  But then again, maybe not.  =)

Ha ha ha.

Comment 43

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

Attachment: version 6 patches

Comment 44

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

Attachment: version 5 gzip archive

Comment 45

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

Changes in the latest version include:

1. attachstatus.cgi -> editattachstatuses.cgi.
2. Added link to attachment status administration page into page footer.
3. Changed privileges required to administer attachment statuses from
"tweakparams" to "editcomponents".

Comment 46

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

Attachment: version 5 corrected patches

Comment 47

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

Attachment: version 6 patches: requires Template Toolkit

Comment 48

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

Patches checked in.  Resolving fixed.

Comment 49

[timeless]

I never can find this bug [fixing].

I need a bug to track the dogfood status of this feature. This bug should not be verified unless a new bug is created for it or people agree that all the features required (eg searching for one [attachment that is not obsolete and needs review]) for dogfood are present.

Comment 50

[Dave Miller [:justdave]]

Just FYI, search capabilities for the new patch statuses are bug 97947.