Index: /main/trunk/greenstone2/perllib/cgiactions/baseaction.pm
===================================================================
--- /main/trunk/greenstone2/perllib/cgiactions/baseaction.pm	(revision 21714)
+++ /main/trunk/greenstone2/perllib/cgiactions/baseaction.pm	(revision 21715)
@@ -370,4 +370,39 @@
 
 
+# Release the gli.lck otherwise no one else will be able to use the collection again.
+sub unlock_collection
+{
+    my $self = shift @_;
+    my ($username, $collection) = @_;
+    my $gsdl_cgi = $self->{'gsdl_cgi'};
+
+    # Obtain the path to the collection GLI lock file
+    my $lock_file_path = &util::filename_cat($ENV{'GSDLHOME'}, "collect", $collection, "gli.lck");
+
+    # If the lock file does exist, check if it is ours
+    if (-e $lock_file_path)
+    {
+	my $lock_file_content = "";
+	open(LOCK_FILE, "<$lock_file_path");
+	while (<LOCK_FILE>) {
+	    $lock_file_content .= $_;
+	}
+	close(LOCK_FILE);
+
+	# Pick out the owner of the lock file
+	$lock_file_content =~ /\<User\>(.*?)\<\/User\>/;
+	my $lock_file_owner = $1;
+
+	# If we are the owner of this lock, we have the right to delete it
+	if ($lock_file_owner eq $username) {
+            unlink($lock_file_path );
+	}
+        else {
+	    $gsdl_cgi->generate_error("Collection is locked by: $lock_file_owner. Cannot be unlocked");
+        }
+    }
+}
+
+
 sub send_mail
 {
Index: /main/trunk/greenstone2/perllib/cgiactions/metadataaction.pm
===================================================================
--- /main/trunk/greenstone2/perllib/cgiactions/metadataaction.pm	(revision 21714)
+++ /main/trunk/greenstone2/perllib/cgiactions/metadataaction.pm	(revision 21715)
@@ -43,5 +43,4 @@
 
 
-
 my $action_table =
 { 
@@ -88,5 +87,4 @@
 
 
-
 sub get_live_metadata
 {
@@ -98,7 +96,8 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
+    # Note: Not sure why get_live_metadata doesn't need the authentication check
+
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -106,23 +105,24 @@
 
     # look up additional args
-
     my $docid  = $self->{'d'};
     if ((!defined $docid) || ($docid =~ m/^\s*$/)) {
-	$gsdl_cgi->generate_error("No docid (d=...) specified.");
-    }
-
+       $gsdl_cgi->generate_error("No docid (d=...) specified.");
+    }
+
+    # Generate the dbkey
     my $metaname  = $self->{'metaname'};
-
     my $dbkey = "$docid.$metaname";
 
+    # To people who know $collect_tail please add some comments
+    # Obtain path to the database
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/|\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", "live-$collect_tail", $index_text_directory);
-
+    
+    # Obtain the content of the key
     my $cmd = "gdbmget $infodb_file_path $dbkey";
-
     if (open(GIN,"$cmd |") == 0) {
+        # Catch error if gdbmget failed
 	my $mess = "Failed to get metadata key: $metaname\n";
 	$mess .= "$!\n";
@@ -130,8 +130,7 @@
 	$gsdl_cgi->generate_error($mess);
     }
-
     else {
+        # Read everything in and concatenate them into $metavalue
 	my $metavalue = "";
-
 	my $line;
 	while (defined ($line=<GIN>)) {
@@ -139,11 +138,11 @@
 	}
 	close(GIN);
-
-	chomp($metavalue);
-
+	chomp($metavalue); # Get rid off the tailing newlines
 	$gsdl_cgi->generate_ok_message("$metavalue");
     }
-}
-
+
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
+}
 
 
@@ -157,5 +156,5 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
+    # Authenticate user if it is enabled
     if ($baseaction::authentication_enabled) {
 	# Ensure the user is allowed to edit this collection
@@ -163,7 +162,6 @@
     }
 
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -171,16 +169,20 @@
 
     # look up additional args
-
     my $docid     = $self->{'d'};
     my $metaname  = $self->{'metaname'};
     my $metapos   = $self->{'metapos'};
 
+    # To people who know $collect_tail please add some comments
+    # Obtain path to the database
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", $collect_tail, $index_text_directory);
+
+    # Read the docid entry
     my $doc_rec_string = &dbutil::read_infodb_entry("gdbm", $infodb_file_path, $docid);
     my $doc_rec = &dbutil::convert_infodb_string_to_hash($doc_rec_string);
+
+    # Basically loop through and unescape_html the values
     foreach my $k (keys %$doc_rec) {
 	my @escaped_v = ();
@@ -189,17 +191,15 @@
 	    push(@escaped_v, $ev);
 	}
-
 	$doc_rec->{$k} = \@escaped_v;
     }
 
+    # Obtain the specified metadata value
     $metapos = 0 if (!defined $metapos);
-
     my $metavalue = $doc_rec->{$metaname}->[$metapos];
-
     $gsdl_cgi->generate_ok_message("$metavalue");
-
-}
-
-
+    
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
+}
 
 
@@ -213,5 +213,4 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     # don't user authenticate for now
     if ($baseaction::authentication_enabled) {
@@ -220,7 +219,6 @@
     }
 
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -228,24 +226,28 @@
 
     # look up additional args
-
     my $docid     = $self->{'d'};
+    if ((!defined $docid) || ($docid =~ m/^\s*$/)) {
+      $gsdl_cgi->generate_error("No docid (d=...) specified.");
+    }
+    my $metavalue = $self->{'metavalue'};
+
+    # Generate the dbkey    
     my $metaname  = $self->{'metaname'};
-    my $metavalue = $self->{'metavalue'};
-
     my $dbkey = "$docid.$metaname";
 
+    # To people who know $collect_tail please add some comments
+    # Obtain path to the database
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", "live-$collect_tail", $index_text_directory);
 
+    # Set the new value
     my $cmd = "gdbmset \"$infodb_file_path\" \"$dbkey\" \"$metavalue\"";
-
     my $status = system($cmd);
-
     if ($status != 0) {
+        # Catch error if gdbmget failed
 	my $mess = "Failed to set metadata key: $dbkey\n";
-	
+
 	$mess .= "PATH: $ENV{'PATH'}\n";
 	$mess .= "cmd = $cmd\n";
@@ -258,5 +260,7 @@
 	$gsdl_cgi->generate_ok_message("set-live-metadata successful: Key[$metaname]=$metavalue");
     }
-
+    
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
 }
 
@@ -271,5 +275,4 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     # don't user authenticate for now
     if ($baseaction::authentication_enabled) {
@@ -278,7 +281,6 @@
     }
 
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -286,5 +288,4 @@
 
     # look up additional args
-
     my $docid     = $self->{'d'};
     my $metaname  = $self->{'metaname'};
@@ -292,9 +293,12 @@
     my $metavalue = $self->{'metavalue'};
 
+    # To people who know $collect_tail please add some comments
+    # Obtain path to the database
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", $collect_tail, $index_text_directory);
+
+    # Read the docid entry
     my $doc_rec_string = &dbutil::read_infodb_entry("gdbm", $infodb_file_path, $docid);
     my $doc_rec = &dbutil::convert_infodb_string_to_hash($doc_rec_string);
@@ -313,12 +317,12 @@
 	    }
 	}
-
 	$doc_rec->{$k} = \@escaped_v;
     }
-
     ## print STDERR "**** metavalue = $metavalue\n";
 
+    # Protect the quotes
     $metavalue =~ s/\"/\\\"/g;
 
+    # Set the metadata value
     if (defined $metapos) {
 	$doc_rec->{$metaname}->[$metapos] = $metavalue;
@@ -327,16 +331,15 @@
 	$doc_rec->{$metaname} = [ $metavalue ];
     }
-
     ## print STDERR "**** metavalue = $metavalue\n";
 
+    # Generate the record string
     my $serialized_doc_rec = &dbutil::convert_infodb_hash_to_string($doc_rec);
-
     ## print STDERR "**** ser dr\n$serialized_doc_rec\n\n\n";
 
+    # Store it into GDBM
     my $cmd = "gdbmset \"$infodb_file_path\" \"$docid\" \"$serialized_doc_rec\"";
-
     my $status = system($cmd);
-
     if ($status != 0) {
+        # Catch error if gdbmget failed
 	my $mess = "Failed to set metadata key: $docid\n";
 	
@@ -356,32 +359,30 @@
 	$gsdl_cgi->generate_ok_message($mess);
     }
-
-}
-
+    
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
+}
 
 
 sub dxml_metadata
 {
-
     my ($tagname, $attrHash, $contextArray, $parentDataArray, $parser) = @_;
-
-
     my $metaname = $parser->{'parameters'}->{'metaname'};
     my $metamode = $parser->{'parameters'}->{'metamode'};
 
+    # Find the right metadata tag and checks if we are going to override it
+    # Note: This over writes the first metadata block it encountered. If there are multiple Sections in the doc.xml, it might not behave as you would expect
     my $name_attr = $attrHash->{'name'};
-
     if (($name_attr eq $metaname) && ($metamode eq "override")) {
-
+        # Get the value and override the current value
 	my $metavalue = $parser->{'parameters'}->{'metavalue'};
-
 	$attrHash->{'_content'} = $metavalue;
 
 	# Don't want it to wipe out any other pieces of metadata
 	$parser->{'parameters'}->{'metamode'} = "done";
-
     }
 
     # raw extended
+    # Someone please write some comments on why adding ':'.$tagname => $attrHash
     return (':'.$tagname => $attrHash, [$tagname => $attrHash]);
 }
@@ -390,17 +391,15 @@
 sub dxml_description
 {
-
     my ($tagname, $attrHash, $contextArray, $parentDataArray, $parser) = @_;
-
     my $metamode = $parser->{'parameters'}->{'metamode'};
 
-
+    # Accumulate the metadata
+    # NOTE: This appends new metadata element to all description fields.
+    # If there are multiple Sections/SubSections, the new metadata block will get appended to all of them
     if ($metamode eq "accumulate") {
 	# tack a new metadata tag on to the end of the <Metadata>+ block
-
 	my $metaname = $parser->{'parameters'}->{'metaname'};
 	my $metavalue = $parser->{'parameters'}->{'metavalue'};
 	
-
 	my $metadata_attr = { '_content' => $metavalue, 
 			      'name'     => $metaname, 
@@ -408,9 +407,7 @@
 
 	my $append_metadata = [ "Metadata" => $metadata_attr ];
-
 	my $description_content = $attrHash->{'_content'};
 
 	push(@$description_content,"    ", $append_metadata ,"\n        ");
-
     }
 
@@ -419,12 +416,11 @@
 }
 
+
 sub edit_xml_file
 {
     my $self = shift @_;
-
     my ($gsdl_cgi, $filename, $rules, $options) = @_;
 
     # use XML::Rules to add it in (read in and out again)
-
     my $parser = XML::Rules->new(rules => $rules, 
 				 style => 'filter' );
@@ -435,4 +431,5 @@
     }
     else {
+        # Read all the text in
 	my $line;
 	while (defined ($line=<MIN>)) {
@@ -441,5 +438,5 @@
 	close(MIN);
 	
-	
+	# Matched lines will get handled by the call backs
 	my $xml_out = "";
 	$parser->filter($xml_in,\$xml_out, $options);
@@ -450,10 +447,8 @@
 	else {
 	    print MOUT $xml_out;
-	    close(MOUT);
-	    
-	}
-    }
-}
-
+	    close(MOUT);	    
+	}
+    }
+}
 
 
@@ -461,9 +456,8 @@
 {
     my $self = shift @_;
-
     my ($gsdl_cgi, $doc_xml_filename, $metaname, $metavalue, $metapos) = @_;
 
     # use XML::Rules to add it in (read in and out again)
-
+    # Set the call back functions
     my @rules = 
 	( _default => 'raw extended',
@@ -471,8 +465,8 @@
 	  'Description' => \&dxml_description );
 	  
+    # Sets the parameters
     my $options = { 'metaname'  => $metaname,
 		    'metapos'   => $metapos,
 		    'metavalue' => $metavalue };
-
     $self->edit_xml_file($gsdl_cgi,$doc_xml_filename,\@rules,$options);
 }
@@ -488,5 +482,4 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     # don't user authenticate for now
     if ($baseaction::authentication_enabled) {
@@ -495,7 +488,6 @@
     }
 
+    # Obtain the collect and archive dir    
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
     my $archive_dir = &util::filename_cat($collect_dir,$collect,"archives");
 
@@ -504,57 +496,52 @@
 
     # look up additional args
-
     my $docid  = $self->{'d'};
-
     my $metaname   = $self->{'metaname'};
     my $metavalue  = $self->{'metavalue'};
     my $metapos    = $self->{'metapos'};
-
     $metapos = 0 if (!defined $metapos);
-
+    
+    # Obtain the doc.xml path for the specified docID
     my $arcinfo_doc_filename = &dbutil::get_infodb_file_path("gdbm", "archiveinf-doc", $archive_dir);
-
     my $doc_rec_string = &dbutil::read_infodb_entry("gdbm", $arcinfo_doc_filename, $docid);
-    my $doc_rec = &dbutil::convert_infodb_string_to_hash($doc_rec_string);
-
+    my $doc_rec = &dbutil::convert_infodb_string_to_hash($doc_rec_string);    
     my $doc_xml_file = $doc_rec->{'doc-file'}->[0];
-
-    
-    my $archives_dir = &util::filename_cat($collect_dir,$collect,"archives");
-    
+    
+    # The $doc_xml_file is relative to the archives, and now let's get the full path
+    my $archives_dir = &util::filename_cat($collect_dir,$collect,"archives");    
     my $doc_xml_filename = &util::filename_cat($archives_dir,$doc_xml_file);
-
-    
+    
+    # Edit the doc.xml file with the specified metadata name, value and position.
+    # TODO: there is a potential problem here as this edit_doc_xml function 
+    # is assuming the simple doc.xml situation where there is only one Section and no SubSections.
+    # Running import.pl -groupsize will cause this to have multiple sections in one doc.xml
     $self->edit_doc_xml($gsdl_cgi,$doc_xml_filename,
 			$metaname,$metavalue,$metapos);
 
-}
-
-
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
+}
 
 
 sub mxml_metadata
 {
-
     my ($tagname, $attrHash, $contextArray, $parentDataArray, $parser) = @_;
-
-
     my $metaname = $parser->{'parameters'}->{'metaname'};
     my $metamode = $parser->{'parameters'}->{'metamode'};
 
+    # Find the right metadata tag and checks if we are going to override it
+    # Note: This over writes the first metadata block it encountered even if it doesn't belong to the source file we specified
     my $name_attr = $attrHash->{'name'};
-
     if (($name_attr eq $metaname) && ($metamode eq "override")) {
-
+        # Get the value and override the current value
 	my $metavalue = $parser->{'parameters'}->{'metavalue'};
-
 	$attrHash->{'_content'} = $metavalue;
 
 	# Don't want it to wipe out any other pieces of metadata
 	$parser->{'parameters'}->{'metamode'} = "done";
-
     }
 
     # raw extended
+    # Someone please write some comments on why adding ':'.$tagname => $attrHash
     return (':'.$tagname => $attrHash, [$tagname => $attrHash]);
 }
@@ -563,17 +550,15 @@
 sub mxml_description
 {
-
     my ($tagname, $attrHash, $contextArray, $parentDataArray, $parser) = @_;
-
     my $metamode = $parser->{'parameters'}->{'metamode'};
 
-
+    # Accumulate the metadata block to the end of the description block
+    # Note: This adds metadata block to all description blocks, so if there are 
+    # multiple FileSets, it will add to all of them
     if ($metamode eq "accumulate") {
 	# tack a new metadata tag on to the end of the <Metadata>+ block
-
 	my $metaname = $parser->{'parameters'}->{'metaname'};
 	my $metavalue = $parser->{'parameters'}->{'metavalue'};
 	
-
 	my $metadata_attr = { '_content' => $metavalue, 
 			      'name'     => $metaname, 
@@ -581,9 +566,7 @@
 
 	my $append_metadata = [ "Metadata" => $metadata_attr ];
-
 	my $description_content = $attrHash->{'_content'};
 
 	push(@$description_content,"    ", $append_metadata ,"\n        ");
-
     }
 
@@ -592,21 +575,19 @@
 }
 
+
 sub edit_metadata_xml
 {
     my $self = shift @_;
-
     my ($gsdl_cgi, $metadata_xml_filename, $metaname, $metavalue, $metamode) = @_;
 
-    # use XML::Rules to add it in (read in and out again)
-
+    # Set the call-back functions for the metadata tags
     my @rules = 
 	( _default => 'raw extended',
 	  'Metadata' => \&mxml_metadata,
 	  'Description' => \&mxml_description );
-	  
-
+
+    # use XML::Rules to add it in (read in and out again)
     my $parser = XML::Rules->new(rules => \@rules, 
 				 style => 'filter' );
-
 
     my $xml_in = "";
@@ -615,16 +596,17 @@
     }
     else {
+        # Read them in
 	my $line;
 	while (defined ($line=<MIN>)) {
 	    $xml_in .= $line;
 	}
-	close(MIN);
+	close(MIN);	
 	
-	
+        # Filter with the call-back functions
 	my $xml_out = "";
 	$parser->filter($xml_in,\$xml_out, { metaname => $metaname,
 					     metavalue => $metavalue,
-					     metamode => $metamode } );
-    
+                                             metamode => $metamode } );
+        
 	if (!open(MOUT,">$metadata_xml_filename")) {
 	    $gsdl_cgi->generate_error("Unable to write out to $metadata_xml_filename: $!");
@@ -632,6 +614,5 @@
 	else {
 	    print MOUT $xml_out;
-	    close(MOUT);
-	    
+	    close(MOUT);	    
 	}
     }
@@ -642,5 +623,5 @@
 {
     my $self = shift @_;
-
+    
     my $username  = $self->{'username'};
     my $collect   = $self->{'collect'};
@@ -648,5 +629,4 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     # don't user authenticate for now
     if ($baseaction::authentication_enabled) {
@@ -655,9 +635,7 @@
     }
 
+    # Obtain the collect and archive dir    
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
     my $archive_dir = &util::filename_cat($collect_dir,$collect,"archives");
-
 
     # Make sure the collection isn't locked by someone else
@@ -665,28 +643,25 @@
 
     # look up additional args
-
     # want either d= or f=
     my $docid  = $self->{'d'};
     my $import_file  = $self->{'f'};
-
     if ((!defined $docid) && (!defined $import_file)) {
 	$gsdl_cgi->generate_error("No docid (d=...) or import file (f=) specified.");
     } 
 
+    # Get the parameters and set default mode to "accumulate"
     my $metaname   = $self->{'metaname'};
     my $metavalue  = $self->{'metavalue'};
     my $metamode   = $self->{'metamode'};
-
     if ((!defined $metamode) || ($metamode =~ m/^\s*$/)) {
 	# make "accumulate" the default (less destructive, as won't actually 
 	# delete any existing values)
-
 	$metamode = "accumulate";
     }
 
+    # Obtain where the metadata.xml is from the archiveinfo-doc.gdb file
+    # If the doc oid is not specified, we assume the metadata.xml is next to the specified "f"
     my $metadata_xml_file;
-
     my $import_filename = undef;
-
     if (defined $docid) {
 	my $arcinfo_doc_filename = &dbutil::get_infodb_file_path("gdbm", "archiveinf-doc", $archive_dir);
@@ -698,21 +673,25 @@
     }
     else {
-	$import_filename = &util::filename_cat($collect_dir,$collect,$import_file);
-    }
-    
-    # figure out correct metadata.xml file
-
+        $import_filename = &util::filename_cat($collect_dir,$collect,$import_file);
+    }
+    
+    
+    # figure out correct metadata.xml file [?]
+    # Assuming the metadata.xml file is next to the source file
+    # Note: This will not work if it is using the inherited metadata from the parent folder
     my ($import_tailname, $import_dirname) 
 	= File::Basename::fileparse($import_filename);
-
     my $metadata_xml_filename = &util::filename_cat($import_dirname,"metadata.xml");
 
+    # Edit the metadata.xml
+    # Note: At moment it doesn't correctly on metadata.xml with multiple FileSets
+    # "accumulate" mode will add new metadata block to all FileSets
+    # "override" mode will over write the first encountered metadata block, even if it doesn't have the right source
     $self->edit_metadata_xml($gsdl_cgi,$metadata_xml_filename,
 			     $metaname,$metavalue,$metamode);
 
-}
-
-
-
+    # Release the lock once it is done
+    $self->unlock_collection($username, $collect);
+}
 
 
@@ -726,5 +705,4 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     if ($baseaction::authentication_enabled) {
 	# Ensure the user is allowed to edit this collection
@@ -732,7 +710,6 @@
     }
 
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -740,21 +717,25 @@
 
     # look up additional args
-
     my $docid     = $self->{'d'};
+    if ((!defined $docid) || ($docid =~ m/^\s*$/)) {
+      $gsdl_cgi->generate_error("No docid (d=...) specified.");
+    }
+    
+    # Generate the dbkey
     my $metaname  = $self->{'metaname'};
-
     my $dbkey = "$docid.$metaname";
 
+    # To people who know $collect_tail please add some comments
+    # Obtain the live gdbm_db path 
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", "live-$collect_tail", $index_text_directory);
 
+    # Remove the key
     my $cmd = "gdbmdel \"$infodb_file_path\" \"$dbkey\"";
-
     my $status = system($cmd);
-
     if ($status != 0) {
+        # Catch error if gdbmdel failed
 	my $mess = "Failed to set metadata key: $dbkey\n";
 	
@@ -782,5 +763,4 @@
     my $gsdlhome  = $self->{'gsdlhome'};
 
-
     # don't user authenticate for now
     if ($baseaction::authentication_enabled) {
@@ -789,7 +769,6 @@
     }
 
+    # Obtain the collect dir
     my $collect_dir = &util::filename_cat($gsdlhome, "collect");
-#    $gsdl_cgi->checked_chdir($collect_dir);
-
 
     # Make sure the collection isn't locked by someone else
@@ -797,16 +776,23 @@
 
     # look up additional args
-
     my $docid     = $self->{'d'};
+    if ((!defined $docid) || ($docid =~ m/^\s*$/)) {
+      $gsdl_cgi->generate_error("No docid (d=...) specified.");
+    }
     my $metaname  = $self->{'metaname'};
     my $metapos   = $self->{'metapos'};
 
+    # To people who know $collect_tail please add some comments
+    # Obtain the path to the database
     my $collect_tail = $collect;
     $collect_tail =~ s/^.*[\/\\]//;
-
     my $index_text_directory = &util::filename_cat($collect_dir,$collect,"index","text");
     my $infodb_file_path = &dbutil::get_infodb_file_path("gdbm", $collect_tail, $index_text_directory);
+
+    # Read the docid entry
     my $doc_rec_string = &dbutil::read_infodb_entry("gdbm", $infodb_file_path, $docid);
     my $doc_rec = &dbutil::convert_infodb_string_to_hash($doc_rec_string);
+
+    # Basically loop through and unescape_html the values
     foreach my $k (keys %$doc_rec) {
 	my @escaped_v = ();
@@ -823,15 +809,19 @@
 	    }
 	}
-
 	$doc_rec->{$k} = \@escaped_v;
     }
 
+    # Check to make sure the key does exist
+    if (!defined ($doc_rec->{$metaname})) {
+        $gsdl_cgi->generate_error("No metadata field \"" . $metaname . "\" in the specified document: [" . $docid . "]");
+    }
+
+    # Obtain the specified metadata pos
     $metapos = 0 if (!defined $metapos);
 
     # consider check key is defined before deleting?
-
+    # Loop through the metadata array and ignore the specified position
     my $filtered_metadata = [];
-    my $num_metadata_vals = scalar(@{$doc_rec->{$metaname}});
-
+    my $num_metadata_vals = scalar(@{$doc_rec->{$metaname}});    
     for (my $i=0; $i<$num_metadata_vals; $i++) {
 	my $metavalue = shift(@{$doc_rec->{$metaname}});
@@ -841,13 +831,12 @@
 	}
     }
-
     $doc_rec->{$metaname} = $filtered_metadata;
 
+    # Turn the record back to string
     my $serialized_doc_rec = &dbutil::convert_infodb_hash_to_string($doc_rec);
 
+    # Store it back to the database
     my $cmd = "gdbmset \"$infodb_file_path\" \"$docid\" \"$serialized_doc_rec\"";
-
     my $status = system($cmd);
-
     if ($status != 0) {
 	my $mess = "Failed to set metadata key: $docid\n";
@@ -867,10 +856,5 @@
 	$gsdl_cgi->generate_ok_message($mess);
     }
-
-}
-
-
-
-
+}
 
 
