302669 - showbug.cgi?ctype=xml should allow the option of exporting attachment data

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Greg Hendricks]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: 

This is a general enhancement request that we have received. 
A number of groups here use automated build scripts and would like to be able to
get patches, etc from a group of bugs at once via xml.



Reproducible: Always

Comment 1

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

Attachment: patch v1: infrastructure supporting this feature

This patch makes Attachment::query() provide real Attachment objects instead of
pseudo-objects and adds a "data" accessor so callers with a reference to an
attachment can retrieve its data.  I've tested attachment functionality in
show_bug.cgi (both HTML and XML ctypes) and attachment.cgi and note no
regressions.

In addition to the functionality improvements, I also clean up the code
considerably and add POD documentation.  Frederic, this is the kind of cleanup
and code improvements I think you like to see.	Can you take a look at this
patch?

Comment 2

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

Attachment: patch v2: fixes bug in last patch

The last patch mistakenly used selectcol_arrayref where it should have used
selectrow_array.  This patch fixes the problem.

Comment 3

[Joel Peshkin]

I may as well confirm this since it seems to have people working on it.

Comment 4

[Frédéric Buclin]

Comment on attachment 193220 [details] [diff] [review]
patch v2: fixes bug in last patch

>Index: Bugzilla/Attachment.pm

> use strict;
> 
>+=head1 NAME
>+
>+Bugzilla::Attachment - a related file that a user has uploaded to the server
>+                       and associated with a specific bug

Nit: I much prefer to see "package Bugzilla::Attachment;" *before* the POD,
just after "use strict;".

I'm not sure "a related file that a user has uploaded to the server" is correct
as you can now store attachments locally.


>-# Use the Flag module to handle flags.
>+use Bugzilla;
> use Bugzilla::Flag;

"use Bugzilla" has been banned from .pm files, see bug 303413.


>+use constant DB_COLUMNS => (

>+    Bugzilla->dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i')." AS date",

Nit: my vote to name it "creation_date", in opposition to "modification_date".
"date" alone isn't precise enough, IMO.


>+sub new {
>+    my $invocant = shift;
>+    my $id = shift;
>+    my $self = shift || Bugzilla->dbh->selectrow_hashref("SELECT $columns
>+                                                          FROM attachments
>+                                                          WHERE attach_id = ?",
>+                                                         undef,
>+                                                         $id);
>     my $class = ref($invocant) || $invocant;
>     bless($self, $class);
>     return $self;
> }

Looking at http://perldoc.perl.org/perlobj.html#An-Object-is-Simply-a-Reference
, it appears that the object should be blessed first, and then be initialized
using a separate routine; see how I did it in my patch in bug 304699:
https://bugzilla.mozilla.org/attachment.cgi?id=192763&action=diff

This also the way we used in other recent .pm files.


>+=pod
>+
>+=head2 Instance Properties

There is POD, but no accessors. You should have a list of
sub foo { return $_[0]->{'foo'}; }
for each property of the attachment object, see again my patch in bug 304699.


>+=item C<submitter_id>
>+
>+the ID of the user who attached the attachment

I think we should not mention this. The submitter ID should be accessed using
$attachment->submitter->id instead of $attachment->submitter_id.


>+sub submitter {
>+    my ($self) = @_;

Nit: When parameters are passed to methods, you cannot write "my ($self) = @_;"
anymore. For consistency among methods (and to anticipate possible future
changes to some methods), I much prefer "my $self = shift;". This way, you can
easily get other parameters using
"my ($foo, $bar, ...) = @_;".


>+sub datasize {
>+    my ($self) = @_;
>+
>+    # If we have already retrieved the data, just return its size.
>+    return length($self->{data}) if exists $self->{data};

This means $self->{datasize} will never be defined?!? Please define
$self->{datasize} in all cases. :(


>+    # If there's no attachment data in the database, the attachment
>+    # is stored in a local file, so retrieve its size from the file.
>+    if ($size == 0) {
>+        my $hash = ($self->{id} % 100) + 100;
>+        $hash =~ s/.*(\d\d)$/group.$1/;
>+        if (open(AH, "$attachdir/$hash/attachment.$self->{id}")) {
>+            binmode AH;
>+            $size = (stat(AH))[7];
>+        }
>+    }

Nit: I don't really like this code duplication.


>+sub flags {
>+    my ($self) = @_;
>+    return Bugzilla::Flag::match({ attach_id => $self->{id}, is_active => 1 });
>+}

Nit: maybe should you wrap this long line:
     $self->{'flags'} = Bugzilla::Flag::match({'attach_id' => $self->{id},
					       'is_active' => 1});


>+=item C<query($bug_id)>
>+
>+Description: retrieves and returns the attachments for the given bug.

>+sub query {

This function should be called get_attachments_by_bug(). query() is by far too
vague.


>+    my ($bug_id) = @_;
>+    my $records = Bugzilla->dbh->selectall_arrayref("SELECT $columns
>+                                                     FROM attachments
>+                                                     WHERE bug_id = ?
>+                                                     ORDER BY attach_id",
>+                                                    {Slice=>{}}, $bug_id);
>+    my @attachments = map( new Bugzilla::Attachment(undef, $_), @$records );
>+    return \@attachments;
> }

This doesn't make sense. get_attachments_by_bug() (let's talk using it new name
;) ) should only query for attachment IDs and let new
Bugzilla::Attachment($attach_id) create them.



>Index: Bugzilla/Flag.pm

>-            $bug_id == $target->{'attachment'}->{'bug_id'}
>-              || return { 'exists' => 0 };
>+            if ($target->{'attachment'}
>+                && $target->{'attachment'}->{'bug_id'} != $bug_id)
>+            {
>+              return { 'exists' => 0 };
>+            }

I think this change is incorrect. If the attachment ID is invalid, the
attachment object doesn't exist and { 'exists' => 0 } should be returned. So
you should write:
 if (!$target->{'attachment'}
     || $target->{'attachment'}->{'bug_id'} != $bug_id)



>Index: template/en/default/bug/show.xml.tmpl

>             <desc>[% a.description FILTER xml %]</desc>
>+            <data>[% a.data FILTER xml %]</data>
>             <ctype>[% a.contenttype FILTER xml %]</ctype>

How is the data returned if the attachment is not plain text (e.g. a picture or
an executable file)?



>Index: template/en/default/global/user-error.html.tmpl

>+    [% ELSIF action == "move" %]
>+      move

>+    [% ELSIF object == "bugs" %]
>+      [%+ terms.bugs %]

Unrelated to this bug.

Comment 5

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

Attachment: patch v3: fixes review issues

Thanks for the review!	Here's an updated patch!


> Nit: I much prefer to see "package Bugzilla::Attachment;" *before* the POD,
> just after "use strict;".

Ok, switched.


> I'm not sure "a related file that a user has uploaded to the server" is
correct
> as you can now store attachments locally.

Whether attachments get stored in the database or the Bugzilla installation
server's filesystem, they still get uploaded from the user's computer to that
server.


> "use Bugzilla" has been banned from .pm files, see bug 303413.

Ok, but if I remove it now, Attachment.pm breaks when called from Flag.pm in
attachment.cgi.  Perhaps we can leave this in now and fix it in bug 304699 once
this patch goes in?


> Nit: my vote to name it "creation_date", in opposition to
"modification_date".
> "date" alone isn't precise enough, IMO.

That's reasonable, but attachments don't really get created, and we should keep
properties single words where possible, so I've changed this to "attached".  I
also changed "submitter" to "attacher" for consistency and to clean up this
API.


> Looking at
http://perldoc.perl.org/perlobj.html#An-Object-is-Simply-a-Reference
> , it appears that the object should be blessed first, and then be initialized

> using a separate routine; see how I did it in my patch in bug 304699:
> https://bugzilla.mozilla.org/attachment.cgi?id=192763&action=diff

There's nothing in that document which says blessing should happen before
initialization, even though the simple examples in that document happen to
follow that order.  I know of no Perl preference for blessing empty hashes
before initializing them, and initializing them first makes this code
considerably simpler, particularly since get_attachments_by_bug() passes an
already-existing hash into the constructor (about which more below).


> There is POD, but no accessors. You should have a list of
> sub foo { return $_[0]->{'foo'}; }
> for each property of the attachment object, see again my patch in bug 304699.


Ok, done.


> I think we should not mention this. The submitter ID should be accessed using

> $attachment->submitter->id instead of $attachment->submitter_id.

Yes, good idea.  I now retrieve this from the database as _submitter_id and
don't mention it in the POD documentation.


> Nit: When parameters are passed to methods, you cannot write "my ($self) =
@_;"
> anymore. For consistency among methods (and to anticipate possible future
> changes to some methods), I much prefer "my $self = shift;". This way, you
can
> easily get other parameters using
> "my ($foo, $bar, ...) = @_;".

Makes sense to me; fixed.


> This means $self->{datasize} will never be defined?!? Please define
> $self->{datasize} in all cases. :(

Ok, fixed.  Note that datasize is a property of the data itself, and it's
unclear whether we should expose it at all, since you can easily derive it from
the data itself (in TT, attachment.data.size; in Perl
length($attachment->{data})).  But perhaps it makes sense for performance
reasons, since accessing attachment.data forces the data to be retrieved from
the database/filesystem and loaded into memory, while datasize avoids loading
the attachment into memory, calling SQL's LENGTH() function or stat()ing the
file.  I've left it in for now with a comment about this.


> >+	# If there's no attachment data in the database, the attachment
> >+	# is stored in a local file, so retrieve its size from the file.
> >+	if ($size == 0) {
> >+	    my $hash = ($self->{id} % 100) + 100;
> >+	    $hash =~ s/.*(\d\d)$/group.$1/;
> >+	    if (open(AH, "$attachdir/$hash/attachment.$self->{id}")) {
> >+		binmode AH;
> >+		$size = (stat(AH))[7];
> >+	    }
> >+	}
> 
> Nit: I don't really like this code duplication.

Ok, I factored out the $hash calculation and filename generation into
_get_attachment_filename().  This seems like the right level of abstraction, as
future methods might want to do something other than reading the file (f.e.
opening it for writing).


> Nit: maybe should you wrap this long line:
>      $self->{'flags'} = Bugzilla::Flag::match({'attach_id' => $self->{id},
>					       'is_active' => 1});

Flags can change during a processes lifetime, and this function should return
the current state of flags, so it shouldn't cache them in an instance property
like $self->{flags}.


> >+sub query {
> 
> This function should be called get_attachments_by_bug(). query() is by far
too
> vague.

Ok, changed to get_attachments_by_bug().


> This doesn't make sense. get_attachments_by_bug() (let's talk using it new
name
> ;) ) should only query for attachment IDs and let new
> Bugzilla::Attachment($attach_id) create them.

Passing the data to the constructor is a significant performance improvement,
since it means we only have to query the database once, whereas making the
constructor retrieve the data itself means a query for every attachment.  For a
bug with a dozen attachments, this means one query instead of thirteen, a
significant improvement, and since both the constructor and
get_attachments_by_bug() use the factored out $columns, it's also robust and
elegant.


> I think this change is incorrect. If the attachment ID is invalid, the
> attachment object doesn't exist and { 'exists' => 0 } should be returned. So
> you should write:
>  if (!$target->{'attachment'}
>      || $target->{'attachment'}->{'bug_id'} != $bug_id)

Right.	Fixed.


> >Index: template/en/default/bug/show.xml.tmpl
> 
> >		<desc>[% a.description FILTER xml %]</desc>
> >+		<data>[% a.data FILTER xml %]</data>
> >		<ctype>[% a.contenttype FILTER xml %]</ctype>
> 
> How is the data returned if the attachment is not plain text (e.g. a picture
or
> an executable file)?

Oops, sorry, this shouldn't have been in there.  It was just for testing.  My
patch is actually just about updating Attachment.pm's API, while ghendricks is
going to work on how to insert the data into the XML output (probably he'll
Base64-encode binary data and XML-escape the rest).  Removed.


> >Index: template/en/default/global/user-error.html.tmpl
> 
> >+	[% ELSIF action == "move" %]
> >+	  move
> 
> >+	[% ELSIF object == "bugs" %]
> >+	  [%+ terms.bugs %]
> 
> Unrelated to this bug.

Right.	Removed.

Comment 6

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

Attachment: patch v4: fixes "use Bugzilla"

LpSolit: myk: you should add "use Bugzilla;" in attachment.cgi. This will fix
your problem :)

Comment 7

[Frédéric Buclin]

Comment on attachment 193534 [details] [diff] [review]
patch v4: fixes "use Bugzilla"

>Index: Bugzilla/Attachment.pm

> sub new {
>-    # Returns a hash of information about the attachment with the given ID.
>+    my $invocant = shift;
>+    my $id = shift;
>+    my $self = shift || Bugzilla->dbh->selectrow_hashref("SELECT $columns
>+                                                          FROM attachments
>+                                                          WHERE attach_id = ?",
>+                                                         undef,
>+                                                         $id);
>+    bless($self, ref($invocant) || $invocant);
>+    return $self;
>+}

>+sub get_attachments_by_bug {
>+    my ($bug_id) = @_;
>+    my $records = Bugzilla->dbh->selectall_arrayref("SELECT $columns
>+                                                     FROM attachments
>+                                                     WHERE bug_id = ?
>+                                                     ORDER BY attach_id",
>+                                                    {Slice=>{}}, $bug_id);
>+    my @attachments = map( new Bugzilla::Attachment(undef, $_), @$records );
>+    return \@attachments;
> }


Could you please resubmit a new patch where get_attachments_by_bug() only
passes a list of IDs to Attachment::new_list() and where both Attachment::new
and Attachment::new_list call _create() depending on whether we want a single
attachment object only (new) or an array of attachment objects (new_list), as
discussed on IRC?

Comment 8

[Frédéric Buclin]

You should wait for bug 305333 to land before updating your patch.

Comment 9

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

Attachment: patch v5: unrots, addresses review comments, and addresses perlobj issues

This version updates the patch to take the checkin for bug 305333 and recent
review comments into account.  Also, since only Bug.pm calls
get_attachments_by_bug(), I moved the query for the list of attachment IDs into
Bug->attachments(), eliminating get_attachments_by_bug().  Finally, I read
through the perlobj documentation and noted the following:

1. "new" isn't special, you can use any name for the constructor;
2. "new" could be worse than some other name, since it might mislead coders
   into thinking it works like C++'s "new";
3. the indirect object syntax (i.e. "new Foo()") is bad for a variety
   of reasons, and you should always use the -> notation (i.e. "Foo->new()");

I also note that "new" is suboptimal because it's similar to "create" and could
mislead coders into thinking it creates an attachment when it merely creates an
object to represent that attachment and retrieves data about the attachment
from the database.  Accordingly I have renamed "new" to "get" in this patch and
added "get_list" to retrieve a list of attachments.

Comment 10

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

Comment on attachment 194354 [details] [diff] [review]
patch v5: unrots, addresses review comments, and addresses perlobj issues

Looks like my fix for bug 302669 rotted this patch.  I'll attach an unrotted
version.

Comment 11

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

Attachment: patch v6: unrots

Here's the unrotted version of v5.  Nothing is different functionally.

Comment 12

[Frédéric Buclin]

Comment on attachment 194409 [details] [diff] [review]
patch v6: unrots

>Index: Bugzilla/Attachment.pm

>+use constant DB_COLUMNS => (
>+    'attach_id AS id',
>+    'bug_id',
>+    'description',
>+    'mimetype AS contenttype',
>+    'submitter_id AS _attacher_id',
>+    Bugzilla->dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i')
>+      . " AS attached",
>+    'ispatch',
>+    'isobsolete',
>+    'isprivate'
>+);

The 'attachments.filename' column is missing.

Nit: also, I think it would be safer to add 'attachments.' in front of each
columns in case we later add additional tables to SQL queries.


>+sub attached {
>+    my $self = shift;
>+    return $self->{attached};
>+}

Nit: I think 'attached_date' or something containing the word 'date' would make
this method more explicit.


>+sub flags {
>+    my $self = shift;
>+    return Bugzilla::Flag::match({ attach_id => $self->{id}, is_active => 1 });
>+}

You should check whether flags have already been requested.



>Index: Bugzilla/Bug.pm

>-    $self->{'attachments'} = Bugzilla::Attachment::query($self->{bug_id});
>-    return $self->{'attachments'};
>+
>+    my $attach_ids = Bugzilla->dbh->selectcol_arrayref("SELECT attach_id
>+                                                        FROM attachments
>+                                                        WHERE bug_id = ?
>+                                                        ORDER BY attach_id",
>+                                                       undef, $self->bug_id);
>+    $self->{'attachments'} = Bugzilla::Attachment->get_list($attach_ids);
>+    return $attachments;

$attachments doesn't exist. Moreover, I think
Attachment::get_attachments_by_bug() makes more sense. Here you extract
attachments IDs to give them back to Attachment::get_list. This should be done
"internally", i.e. in Attachment.pm. And I think that other .cgi scripts may
use it too as soon as Flag.pm and Flag.pm have been rewritten.

Comment 13

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

Attachment: patch v7

(In reply to comment #12)
> (From update of attachment 194409 [details] [diff] [review] [edit])
> >Index: Bugzilla/Attachment.pm
> 
> >+use constant DB_COLUMNS => (
> >+	'attach_id AS id',
> >+	'bug_id',
> >+	'description',
> >+	'mimetype AS contenttype',
> >+	'submitter_id AS _attacher_id',
> >+	Bugzilla->dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i')
> >+	  . " AS attached",
> >+	'ispatch',
> >+	'isobsolete',
> >+	'isprivate'
> >+);
> 
> The 'attachments.filename' column is missing.

Added.


> Nit: also, I think it would be safer to add 'attachments.' in front of each
> columns in case we later add additional tables to SQL queries.

Ok, done.


> Nit: I think 'attached_date' or something containing the word 'date' would 
> make this method more explicit.

Sure, but that has other downsides.  I think "attached" will work ok.


> You should check whether flags have already been requested.

Ok, done.


> $attachments doesn't exist. Moreover, I think
> Attachment::get_attachments_by_bug() makes more sense. Here you extract
> attachments IDs to give them back to Attachment::get_list. This should be
done
> "internally", i.e. in Attachment.pm. And I think that other .cgi scripts may
> use it too as soon as Flag.pm and Flag.pm have been rewritten.

Ok, done.

Comment 14

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

Attachment: patch v8: fixes compile errors for scripts that "use Bugzilla::Bug"

The tests reported compile-time errors with nine scripts.  Six are CGI scripts
that "use Bugzilla::Bug", which itself does "use Bugzilla::Attachment", so
those scripts must "use Bugzilla" so that Bugzilla->dbh is available to
Bugzilla::Attachment.  I've fixed those in this patch.

Unfortunately, the other three errors are with the Perl modules
Bugzilla/Bug.pm, Bugzilla/Attachment.pm, and Bugzilla/Flag.pm.	How do I fix
those?

Comment 15

[Frédéric Buclin]

Comment on attachment 194501 [details] [diff] [review]
patch v8: fixes compile errors for scripts that "use Bugzilla::Bug"

>Index: attachment.cgi
>Index: editcomponents.cgi
>Index: editmilestones.cgi
>Index: editproducts.cgi
>Index: show_activity.cgi
>Index: showdependencytree.cgi
>Index: summarize_time.cgi

OK, I finally found what the problem was. Note that adding "use Bugzilla;" in
these .cgi files is now useless. It will be added when killing globals.pl (in
another bug).



>Index: Bugzilla/Attachment.pm

>+use constant DB_COLUMNS => (
>+    Bugzilla->dbh->sql_date_format('attachments.creation_ts', '%Y.%m.%d %H:%i')
>+      . " AS attached",

Here is the culprit. Bugzilla->dbh is called outside a routine and it's the
reason this doesn't work (don't ask me why). I fixed the problem by removing it
from the column list and added it in Attachment::attached itself:

sub attached {
    my $self = shift;
    return $self->{attached} if defined $self->{attached};

    my $dbh = Bugzilla->dbh;
    my $date = $dbh->sql_date_format('attachments.creation_ts', '%Y.%m.%d
%H:%i');
    $self->{attached} = $dbh->selectrow_array("SELECT $date
					       FROM attachments
					       WHERE attach_id = ?",
					       undef, $self->id);
    return $self->{attached};
}

With this routine, all tests are now successful and the expected result is
returned.


>+sub flags {

>+    $self->{flags} = Bugzilla::Flag->match({ attach_id => $self->id,
>+                                             is_active => 1 });

There is a mistake in this method. 'Bugzilla::Flag->match' is incorrect as
'match' is not a method of the Flag class. An big red error message is
returned! You have to write 'Bugzilla::Flag::match', with '::'.



>Index: Bugzilla/Bug.pm

>+    $self->{'attachments'} =
>+        Bugzilla::Attachment->get_attachments_by_bug($self->bug_id);

Here again, this doesn't work as get_attachments_by_bug() is not a method of
the Attachment class. You have to write
'Bugzilla::Attachment::get_attachments_by_bug' with '::'.


I tested your patch with these problems fixed and everything works fine now.

Comment 16

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

Attachment: patch v9: fixes bugs in previous version

> With this routine, all tests are now successful and the expected result is
> returned.

We should really figure out why Bugzilla->dbh causes the module to fail to
compile, since we should be able to use that call in this package outside any
subroutine.

In the meantime, however, I've implemented the workaround of putting the call
inside _retrieve().  I didn't put it inside attached() because that means an
extra SQL query for almost every retrieved attachment, since most retrieved
attachments want that information, f.e. every attachment listed in
show_bug.cgi.  Also, this change isolates the column list data structures to
the only block of code that actually uses them, a worthy code cleanup.


> >+sub flags {
> 
> >+	$self->{flags} = Bugzilla::Flag->match({ attach_id => $self->id,
> >+						 is_active => 1 });
> 
> There is a mistake in this method. 'Bugzilla::Flag->match' is incorrect as
> 'match' is not a method of the Flag class. An big red error message is
> returned! You have to write 'Bugzilla::Flag::match', with '::'.

Right.	Fixed.


> >+	$self->{'attachments'} =
> >+	    Bugzilla::Attachment->get_attachments_by_bug($self->bug_id);
> 
> Here again, this doesn't work as get_attachments_by_bug() is not a method of
> the Attachment class.  You have to write 
> 'Bugzilla::Attachment::get_attachments_by_bug' with '::'.

Actually that subroutine is a method of the class (specifically, it's a class
method) per the perlobj doc: "A Class is Simply a Package... A Method is Simply
a Subroutine... A class method expects a class name as the first argument..."

The "::" notation is correct for Flag.pm, which doesn't define a real class,
but for Attachment.pm, which does define a real class, we should treat every
public subroutine in the package as a method, whether a class method or an
instance method, and per perlobj use the -> operator "exclusively."

So the bug here is not the use of -> but the failure of
get_attachments_by_bug() to "expect a class name as the first argument." 
Fixed.


> I tested your patch with these problems fixed and everything works fine now.

Sorry that I didn't test these changes better before requesting review.

Comment 17

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

Attachment: patch v10: fixes crash when no attachments for bug

Comment 18

[Frédéric Buclin]

Comment on attachment 194529 [details] [diff] [review]
patch v10: fixes crash when no attachments for bug

I think we have found all bugs now. :) Everything works fine and ./runtests.pl
reports no error.

r=LpSolit

Comment 19

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

Thanks Frederic!  Checking in my patch (but leaving this bug open so ghendricks
can attach his feature patch):

Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.25; previous revision: 1.24
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.93; previous revision: 1.92
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.53; previous revision: 1.52
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.51; previous revision: 1.50
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.24; previous revision: 1.23
done
Checking in template/en/default/bug/show.xml.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.xml.tmpl,v  <--
 show.xml.tmpl
new revision: 1.9; previous revision: 1.8
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.123; previous revision: 1.122
done

Comment 20

[Greg Hendricks]

Attachment: Encoded Attachment Support v1

This patch also includes links to export the xml data on buglist and show_bug.
If preffered, we can split this off into it's own bug.

Comment 21

[Greg Hendricks]

Attachment: Encoded Attachment Support v2

This patch implements the encoding using a template filter.

Comment 22

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

Comment on attachment 194665 [details] [diff] [review]
Encoded Attachment Support v2

>+        [% IF displayfields.attachmentdata %]
>+            <thedata>[% a.data FILTER base64 %]</thedata>
>+        [% END %]        

The name of the tag should be "data".  Otherwise this patch looks great. r=myk
on a patch with this change made.

Comment 23

[Frédéric Buclin]

Clearing myk's approval in prevision of ghendricks' patch.

Comment 24

[Colin Ogilvie [:cso]]

If you are changing the XML, you should check the DTD is still correct for it...
not sure if thedata / data is already defined in the DTD or not.

Comment 25

[Greg Hendricks]

Attachment: Encoded Attachment Support v3

Guess the dtd changed. *Shrug* 
Done.

Comment 26

[Frédéric Buclin]

Checking in show_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/show_bug.cgi,v  <--  show_bug.cgi
new revision: 1.36; previous revision: 1.35
done
Checking in Bugzilla/Template.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v  <--  Template.pm
new revision: 1.33; previous revision: 1.32
done
Checking in t/004template.t;
/cvsroot/mozilla/webtools/bugzilla/t/004template.t,v  <--  004template.t
new revision: 1.37; previous revision: 1.36
done
Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.19; previous revision: 1.18
done
Checking in template/en/default/bug/knob.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/knob.html.tmpl,v  <--
 knob.html.tmpl
new revision: 1.19; previous revision: 1.18
done
Checking in template/en/default/bug/show.xml.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/show.xml.tmpl,v  <--
 show.xml.tmpl
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/list/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.html.tmpl,v 
<--  list.html.tmpl
new revision: 1.39; previous revision: 1.38
done