Bug 445607

Created attachment 304813 [details]
script using the requested method

Created attachment 306946 [details]
Patch adding bugzilla.updateAttachMimeType() to 3.2 (v1)

Attaching patch to add support for updating mime type of an attachment based on
attach_id to Bugzilla 3.2 (devel).

Noura, can you take a look at this please?

Thanks
Dave

Created attachment 306947 [details]
Patch adding bugzilla.updateAttachMimeType() to 2.18 (v1)

Attaching patch to add support for updating mime type of an attachment based on

attach_id to Bugzilla 2.18 (production).

Noura, can you take a look at this please?

Thanks
Dave

Created attachment 306948 [details]
Sample perl script showing use of bugzilla.updateAttachMimeType()

Comment on attachment 306946 [details]
Patch adding bugzilla.updateAttachMimeType() to 3.2 (v1)

Hi Dave,

I reviewed the patch and few comments are inline.

>+    $attach_id || ThrowUserError('invalid_attach_id');
>+    $mime_type || ThrowUserError('invalid_content_type');
>+

should those 2 be:

$attach_id || ThrowCodeError('param_required', { param => 'attach_id' });
$mime_type || ThrowCodeError('param_required', { param => 'mime_type' });


>+    if ( not Bugzilla->user->in_group("editbugs") 
>+            && Bugzilla->user->id != $submitter_id ) {

also the not needs to be in brackets or it is applied to both conditions:

    if ( (not Bugzilla->user->in_group("editbugs"))
	    && Bugzilla->user->id != $submitter_id ) {


>+        
>+        $data_ref->{nomail} ||= 0;
>+        
>+        # Generate email
>+        return Bugzilla::BugMail::Send( $bug_id,
>+            { changer => Bugzilla->user->login }, $data_ref->{nomail} );
>+    }
>+} 
>+

the nomail is not part of the BugMail args in 3.2 so I guess this can be
implemented as an if condition:

	$data_ref->{nomail} ||= 0;

	# Generate email
	if(not $data_ref->{nomail}) {
	    return Bugzilla::BugMail::Send( $bug_id,
		{ changer => Bugzilla->user->login });
	}


The rest looks good to me, I tested it and it works as expected with the above
fixes.

Thanks,
Noura

Comment on attachment 306947 [details]
Patch adding bugzilla.updateAttachMimeType() to 2.18 (v1)


>+    if ( not ::UserInGroup("editbugs") 
>+            && Bugzilla->user->id != $submitter_id ) {
>+        ThrowUserError( "illegal_attachment_edit",
>+            { attach_id => $attach_id } );
>

Hi Dave , that patch works pretty good except for the not bit :

    if ( (not ::UserInGroup("editbugs"))
	    && Bugzilla->user->id != $submitter_id ) {


Cheers,
Noura

Created attachment 307011 [details]
Patch adding bugzilla.updateAttachMimeType() to 3.2 (v2)

Thanks Noura, I have made the changes you suggested.

Please review
Dave

Created attachment 307012 [details]
Patch adding bugzilla.updateAttachMimeType() to 2.18 (v2)

Thanks Noura. I have made your suggested changes.

Please review.
Dave

(In reply to comment #8)
> Created an attachment (id=307012) [edit]
> Patch adding bugzilla.updateAttachMimeType() to 2.18 (v2)
> 
> Thanks Noura. I have made your suggested changes.
> 
> Please review.
> Dave

Actually nevermind this one. You already gave a review+.

Will check in.
Dave

Thanks a lot.

This has now been pushed live to both 2.18 and 3.2. The following gives some
info on how to use the new method.

Dave

"updateAttachMimeType($data_ref, $username, $password)"
           Update the attachment mime type of an attachment. The first argument
is a data hash containing information on the new MIME type and the attachment id
that you want to
           act on.

               $data_ref = {
                   "attach_id" => "<Attachment ID>",
                       # Attachment ID to perform MIME type change on.
                   "mime_type"  => "<New MIME Type Value>",
                       # Legal MIME type value that you want to change the
attachment to.
                   "nomail" => 0,
                       # OPTIONAL Flag that is either 1 or 0 if you want email
to be sent or not for this change
               };

Comment on attachment 307011 [details]
Patch adding bugzilla.updateAttachMimeType() to 3.2 (v2)

reviewed a previous patch