368311 - custom field info is not imported into database from xml file

Status: RESOLVED FIXED

(Bugzilla :: Bug Import/Export & Moving, defect)

Description

[Joe]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: Bugzilla 2.23.3

Test export/import: An xml file containing the bug information was generated from the show_bug form.  The xml file was modified slightly to allow it to be imported back into the original database.
The importxml.pl was used on the command line to import the new bug.  No errors occurred.  In verbose mode, the importxml script indicated that the custom field was parsed however the SQL statement to update the bug database does not contain the field name or field data.
On examination of the new bug in show_bugs, the custom field is empty.  

Reproducible: Always

Steps to Reproduce:
1.Select bug in database and export to xml file.
2.Modify file slightly i.e. new bug id, url etc.
3.import bug from file using importxml.pl on command line.
Actual Results:  
The bug imported using importxml.pl does not contain the information associated with its custom fields.

Expected Results:  
The custom field information is also imported.

Comment 1

[Frédéric Buclin]

Valid request. Note that importxml.pl will have to make sure the imported custom fields exist on the target installation, and if the field is a dropdown menu, then the imported value must be a valid one.

Comment 2

[Erik Purins]

Attachment: Add custom field data to bug query and values, or add error line if illegal value encountered. Skip to next custom field if this field isn't in this bug record.

Please review. I've tested this patch on a live 2.23.4 installation.
Added work e-mail address as contributor since this was paid for by them.
Any style or additional pointers appreciated.
-e

Comment 3

[Frédéric Buclin]

Comment on attachment 259457 [details] [diff] [review]
Add custom field data to bug query and values, or add error line if illegal value encountered.

Skip to next custom field  if this field isn't in this bug record.

>+    foreach my $custom_field (Bugzilla->custom_field_names) {
>+        my $is_defined = defined( $bug_fields{$custom_field} );

Instead of defining $is_defined, you should call "next" if the value is undefined. This would make your code smaller and cleaner.


>+        my $is_well_formed = check_field( $custom_field, scalar $bug_fields{$custom_field}, undef, ERR_LEVEL );

This is an invalid call for all free-text fields. check_field() will look at a table named $custom_field which doesn't exist. You get a SQL crash.

Comment 4

[Erik Purins]

Thanks, Frederic. I only used this with dropdown fields. I'll take a look at handling all types of custom fields.

Regarding $is_defined, code is laid out so I can inspect the contents of each value to boolean in the debugger. The code I recycled from inside importxml.pl (I think it was the component section) originally uses the style of putting the code inside the boolean, but it makes inspection difficult.

If I call next to skip to next item in loop, won't I skip the opportunity to insert an error message into the log?

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> If I call next to skip to next item in loop, won't I skip the opportunity to
> insert an error message into the log?

No, because you already skip the error message if the value is undefined.

Comment 6

[Erik Purins]

Comment on attachment 259457 [details] [diff] [review]
Add custom field data to bug query and values, or add error line if illegal value encountered.

Skip to next custom field  if this field isn't in this bug record.

>Index: importxml.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/importxml.pl,v
>retrieving revision 1.74
>diff -u -r1.74 importxml.pl
>--- importxml.pl	6 Feb 2007 14:57:01 -0000	1.74
>+++ importxml.pl	23 Mar 2007 21:52:58 -0000
>@@ -22,6 +22,7 @@
> #                 Gregary Hendricks <[e-mail]>
> #                 Vance Baarda <[e-mail]>
> #                 Guzman Braso <[e-mail]>
>+#                 Erik Purins <[e-mail]>
> 
> # This script reads in xml bug data from standard input and inserts
> # a new bug into bugzilla. Everything before the beginning <?xml line
>@@ -1007,6 +1008,19 @@
>     push( @query,  "bug_status" );
>     push( @values, $status );
> 
>+    # Custom fields
>+    foreach my $custom_field (Bugzilla->custom_field_names) {
>+        next unless my $is_defined = defined( $bug_fields{$custom_field} );
>+        my $is_well_formed = check_field( $custom_field, scalar $bug_fields{$custom_field}, undef, ERR_LEVEL );
>+        if ($is_defined && $is_well_formed) {
>+            push( @query,  $custom_field );
>+            push( @values, $bug_fields{$custom_field} );
>+        } else {
>+            if ($is_defined) {
>+                $err .= "Skipping illegal value \"$bug_fields{$custom_field}\" in $custom_field.\n" ;
>+            }
>+        }
>+    }
> 
>     # For the sake of sanitycheck.cgi we do this.
>     # Update lastdiffed if you do not want to have mail sent
>

Comment 7

[Erik Purins]

Frederic, does that look ok?

Comment 8

[Erik Purins]

No, I see. The boolean is no longer needed because I'll be in the next loop. I'll take another look on monday.

Comment 9

[Erik Purins]

Attachment: updated to check field type, next if field is undefined in this record.

Follow-up patch based on review. Any additional pointers and comments appreciated.

Comment 10

[Frédéric Buclin]

Comment on attachment 259818 [details] [diff] [review]
updated to check field type, next if field is undefined in this record.

>+                push( @values, $bug_fields{$custom_field} ); # Should I clean_text() on this text?

Yes, you should.


>+            $err .= "Type of custom field $custom_field is an unhandled FIELD_TYPE_: $field->type\n" ;

Nit: put $field->type outside quotes.


Your patch is working fine and both comments can be addressed on checkin. r=LpSolit

Comment 11

[Frédéric Buclin]

Let's take it for 3.0 as we may consider this as dataloss. And the code is safe anyway.

Comment 12

[Frédéric Buclin]

Attachment: patch with comments fixed

Here is the patch I'm going to commit, which removes tabs and fixes the comments I made.

Comment 13

[Frédéric Buclin]

tip:

Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.75; previous revision: 1.74
done

3.0 RC1:

Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.74.2.1; previous revision: 1.74
done