Added some minor corrections and clarifications to the documentation.

Added some comments to various user-related files.
Factored out some code that started becoming common when inserting fields in Service/User.pm.
Added code to deal with adding new contact details.
Added code to support removing fields from a user.

webtools/PLIF/Documentation.txt

@@ -193,7 +193,10 @@ five debugging aids:
    it, not the people who use it on a daily basis) can change the
    debugging level that is printed, so if you have a lot of
    dump(9,'verbose debugging information') calls they can easily turn
-   them off. 0 is the most serious, 9 is the most trivial.
+   them off. 0 is the most serious, 9 is the most trivial. Actually,
+   10 is the most trivial, but 10 is so trivial as to be unusable
+   unless you are trying to get a headache -- the core PLIF code calls
+   dump(10,'...')  for almost every method call that goes through it.
 
    warn(level, message)
    Same as dump(), but includes a stack trace.
@@ -221,6 +224,12 @@ They also allow us to later reimplement the debugging code to add
 better support for debuggers or pretty printing or mailing errors to
 admins or whatever.
 
+If you've been paying attention you'll notice there were actually six
+debugging aids, and not five. This is just to demonstrate that they
+will only actually help if you use them, not just if you look at
+them. Clearly here I should have been using them if I wanted to catch
+that fence-post error.
+
 Unfortunately, using these utility methods to report errors can result
 in suboptimal feedback to the user, and so should only be used to
 report errors that you really were not expecting, such as missing
@@ -364,7 +373,7 @@ initialise the amount of money to zero (as opposed to undefined).
 
 Wowee, lots of PLIFisms there! Let's look at each one in turn. The
 first line of the body sets the $self variable to be the reference to
-the object. If you are familiar with JavaScript, think "this".
+the object. If you are familiar with JavaScript or C++, think "this".
 
 The second line calls the inherited constructor with the same
 arguments as was passed to _this_ constructor.

webtools/PLIF/PLIF/DataSource/User.pm

@@ -100,7 +100,7 @@ sub setField {
     my $self = shift;
     my($app, $fieldID, $category, $name, $type, $data) = @_;
     # if fieldID is undefined, then add a new entry and return the
-    # fieldID.
+    # fieldID. Typically data will be undefined then too.
     $self->notImplemented();
 }
 

webtools/PLIF/PLIF/Service/User.pm

@@ -33,6 +33,11 @@ use PLIF::Service::Session;
 @ISA = qw(PLIF::Service::Session);
 1;
 
+# XXX It would be interesting to implement crack detection (time since
+# last incorrect login, number of login attempts performed with time
+# since last incorrect login < a global delta, address changing
+# timeout, etc).
+
 sub provides {
     my $class = shift;
     my($service) = @_;
@@ -91,11 +96,10 @@ sub objectInit {
     $self->newFieldPassword($newFieldPassword);
     $self->fields({});
     $self->fieldsByID({});
-    my $fieldFactory = $self->app->getService('user.field.factory');
+    # don't forget to update the 'hash' function if you add more fields
+    my $fieldFactory = $app->getService('user.field.factory');
     foreach my $fieldID (keys($fields)) {
-        my $field = $fieldFactory->createFieldByID($app, $self, $fieldID, $fields->{$fieldID});
-        $self->fields->{$field->category}->{$field->name} = $field;
-        $self->fieldsByID->{$field->fieldID} = $field;
+        $self->insertField($fieldFactory->createFieldByID($app, $self, $fieldID, $fields->{$fieldID}));
     }
     $self->groups({%$groups}); # hash of groupID => groupName
     $self->rights(map {$_ => 1} @$rights); # map a list of strings into a hash for easy access
@@ -129,6 +133,14 @@ sub getField {
     return $field;
 }
 
+sub insertField {
+    my $self = shift;
+    my($field) = @_;
+    $self->fields->{$field->category}->{$field->name} = $field;
+    $self->fieldsByID->{$field->fieldID} = $field;
+    return $field;
+}
+
 sub getAddress {
     my $self = shift;
     my($protocol) = @_;
@@ -155,16 +167,33 @@ sub prepareAddressChange {
     }
 }
 
+sub prepareAddressAddition {
+    my $self = shift;
+    my($fieldName, $newAddress, $password) = @_;
+    my $field = $self->insertField($self->app->getService('user.field.factory')->createFieldByName($self->app, $self, 'contact', $fieldName, undef));
+
+    if ($field->validate($newAddress)) {
+        $self->newFieldID($field->fieldID);
+        $self->newFieldValue($newAddress);
+        $self->newFieldPassword($password);
+        return $self->objectCreate($self->app, $self->userID, $self->disabled, $self->adminMessage, 
+                                   $self->newFieldID, $self->newFieldValue, $self->newFieldPassword,
+                                   {$field->FieldID => $newAddress}, $self->{'groups'}, keys(%{$self->rights}));
+    } else {
+        return undef;
+    }
+}
+
 sub doAddressChange {
     my $self = shift;
     my($password) = @_;
-    if ($self->newFieldID and $self->newFieldValue) {
-        my $field;
-        if ($self->app->getService('service.passwords')->checkPassword($self->newFieldPassword, $password)) {
-            $field = $self->fieldsByID->{$self->newFieldID};
-            if (defined($field)) {
-                $field->data = $self->newFieldValue;
-            } # else XXX create it? How do we enable the user to add contact fields?
+    if ($self->newFieldID) {
+        my $field = $self->fieldsByID->{$self->newFieldID};
+        $self->assert(defined($field), 1, 'Database integrity error: newFieldID doesn\'t map to a field!');
+        if (defined($password) and ($self->app->getService('service.passwords')->checkPassword($self->newFieldPassword, $password))) {
+            $field->data = $self->newFieldValue;
+        } elsif (not defined($field->data)) {
+            $field->remove();
         }
         $self->newFieldID(undef);
         $self->newFieldValue(undef);
@@ -175,6 +204,14 @@ sub doAddressChange {
     }
 }
 
+sub resetAddressChange {
+    my $self = shift;
+    # calling the doAddressChange() function with no arguments is the
+    # same as calling doAddressChange() with the wrong password, which
+    # resets the fields (to prevent multiple attempts)
+    return defined($self->doAddressChange());
+}
+
 sub hash {
     my $self = shift;
     return %$self; # XXX should expand fields too

webtools/PLIF/PLIF/Service/UserField.pm

@@ -48,6 +48,7 @@ sub init {
     $self->typeData($fieldTypeData); # change this at your peril
     $self->name($fieldName); # change this at your peril
     $self->data($fieldData); # this is the only thing you should be changing
+    # don't forget to update the user's 'hash' function if you add more fields
     $self->{'_DIRTY'} = 0;
 }
 
@@ -65,6 +66,13 @@ sub validate {
     return 1;
 }
 
+# deletes this field from the database
+sub remove {
+    my $self = shift;
+    $self->{'_DELETE'} = 1;
+    $self->{'_DIRTY'} = 1;
+}
+
 # Contact fields should generate usernames (for their 'username'
 # member function) with the syntax "ServiceName: username" e.g., my
 # AIM username would be "AIM: HixieDaPixie". Services are fully
@@ -85,8 +93,11 @@ sub DESTROY {
     if ($self->{'_DIRTY'}) {
         $self->write();
     }
+    $self->SUPER::DESTROY(@_);
 }
 
 sub write {
     # XXX
+    # check $self->{'_DELETE'} to see if we have to remove it altogether
+    # otherwise, commit the changes
 }