* Correcting a typo in Input.pm

* Added a comment to DataSource/User.pm explaining how (typically) to search for a username
* Fleshed out the DataSource/User.pm API by adding some schema management methods
* Added notes on which fields in the database schema should be keys
* Added comment to Service/User.pm noting the difference between Objects, Services, and Service Instances
* Changed 'user.field.factory' to 'user.fieldFactory' to prevent a namespace collision with 'user.field.(type)'
* Calling 'insertField' in one case which I missed when adding the method
* Implemented 'hash', 'joinGroup', 'invalidateRights', 'writeProperties' and 'writeGroups'
* Changed Service/UserField.pm so that one user field class can be used for any category
* Added a 'username' convenience method and implemented 'write'
* Added a comment to Service/UserFieldFactory.pm explaining how it should work
* Removed the 'user.field.generic.generic' field implementation, replaced it with a simpler 'user.field.string' implementation

webtools/PLIF/PLIF/DataSource/User.pm

@@ -55,6 +55,11 @@ sub getUserByUsername {
 sub getUserIDByUsername {
     my $self = shift;
     my($app, $username) = @_;
+    # the username for a user field is created by appending the 'data'
+    # of the user field to the type data of the field description. For
+    # example, for the field 'contact.icq', the type data field might
+    # contain the string 'ICQ:' and the user field might be '55378571'
+    # making the username 'ICQ:55378571'.
     $self->notImplemented();
     # return userID
 }
@@ -83,12 +88,19 @@ sub setUserField
     $self->notImplemented();
 }
 
+sub removeUserField
+    my $self = shift;
+    my($app, $userID, $fieldID) = @_;
+    $self->notImplemented();
+}
+
 sub setUserGroups
     my $self = shift;
     my($app, $userID, @groupIDs) = @_;
     $self->notImplemented();
 }
 
+# returns the userDataTypes table, basically...
 sub getFields {
     my $self = shift;
     my($app) = @_;
@@ -96,6 +108,20 @@ sub getFields {
     # return [fieldID, category, name, type, data]*
 }
 
+sub getFieldFromID {
+    my $self = shift;
+    my($app, $fieldID) = @_;
+    $self->notImplemented();
+    # return [fieldID, category, name, type, data]
+}
+
+sub getFieldFromCategoryAndName {
+    my $self = shift;
+    my($app, $category, $name) = @_;
+    $self->notImplemented();
+    # return [fieldID, category, name, type, data]
+}
+
 sub setField {
     my $self = shift;
     my($app, $fieldID, $category, $name, $type, $data) = @_;
@@ -104,6 +130,14 @@ sub setField {
     $self->notImplemented();
 }
 
+sub removeField {
+    my $self = shift;
+    my($app, $fieldID) = @_;
+    # This should handle the case where the field to be removed is
+    # still referenced by some users
+    $self->notImplemented();
+}
+
 sub getGroups {
     my $self = shift;
     my($app) = @_;
@@ -120,8 +154,12 @@ sub getGroupName {
 
 sub setGroup {
     my $self = shift;
-    my($app, $groupID, @rightNames) = @_;
-    # if groupID is undefined, yada yada.
+    my($app, $groupID, $groupName, @rightNames) = @_;
+    # If groupID is undefined, then add a new entry and return the new
+    # groupID. If groupName is undefined, then leave it as is. If both
+    # groupID and groupName are undefined, there is an error.
+    $self->assert(defined($groupID) or defined($groupName), 1, 
+                  'Invalid arguments to DataSource::User::setGroup: \'groupID\' and \'groupName\' both undefined');
     $self->notImplemented();
 }
 
@@ -148,7 +186,7 @@ __END__
 +-------------------+
 | user              |
 +-------------------+
-| userID            | auto_increment
+| userID         K1 | auto_increment
 | password          |
 | disabled          | boolean
 | adminMessage      | string displayed when user (tries to) log in
@@ -160,45 +198,47 @@ __END__
 +-------------------+
 | userData          |
 +-------------------+
-| userID            | points to entries in the table above
-| fieldID           | points to entries in the table below
+| userID         K1 | points to entries in the table above
+| fieldID        K1 | points to entries in the table below
 | data              | e.g. "ian@hixie.ch" or "1979-12-27" or an index into another table
 +-------------------+
 
 +-------------------+
 | userDataTypes     |
 +-------------------+
-| fieldID           | auto_increment
-| category          | e.g. contact, personal, setting
-| name              | e.g. sms, homepage, notifications
-| type              | e.g. number, string, notifications
-| data              | e.g. "[0-9- ]*", "uri", null
+| fieldID        K1 | auto_increment
+| category       K2 | e.g. contact, personal, setting [1]
+| name           K2 | e.g. sms, homepage, notifications [1]
+| type              | e.g. number, string, notifications [2]
+| data              | e.g. "SMS", "optional", null
 +-------------------+
+ [1] used to find the fieldID for a particular category.name combination
+ [2] used to find the factory for the relevant user field object
 
 +-------------------+
 | userGroupMapping  |
 +-------------------+
-| userID            | 
-| groupID           |
+| userID         K1 | 
+| groupID        K1 |
 +-------------------+
 
 +-------------------+
 | groups            |
 +-------------------+
-| groupID           |
+| groupID        K1 |
 | name              | user defined name (can be changed)
 +-------------------+
 
 +-------------------+
 | groupRightsMapping|
 +-------------------+
-| groupID           |
-| rightID           |
+| groupID        K1 |
+| rightID        K1 |
 +-------------------+
 
 +-------------------+
 | rights            |
 +-------------------+
-| rightID           | implementation detail - not ever passed to other parts of the code
-| name              | the internal name for the right, as used by the code
+| rightID        K1 | implementation detail - not ever passed to other parts of the code
+| name           K2 | the internal name for the right, as used by the code
 +-------------------+

webtools/PLIF/PLIF/Input.pm

@@ -74,7 +74,7 @@ sub peekArgument {
     return undef;
 }
 
-# 'username' and 'password' are two out of bands arguments that may be
+# 'username' and 'password' are two out of band arguments that may be
 # provided as well, they are accessed as properties of the input
 # object (e.g., |if (defined($input->username)) {...}|).  Input
 # services that have their own username syntaxes (e.g. AIM, ICQ)

webtools/PLIF/PLIF/Service/User.pm

@@ -33,6 +33,11 @@ use PLIF::Service::Session;
 @ISA = qw(PLIF::Service::Session);
 1;
 
+# This class implements an object and its associated factory service.
+# Compare this with the UserFieldFactory class which implements a
+# factory service only, and the various UserField descendant classes
+# which implement Service Instances.
+
 # 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
@@ -97,7 +102,7 @@ sub objectInit {
     $self->fields({});
     $self->fieldsByID({});
     # don't forget to update the 'hash' function if you add more fields
-    my $fieldFactory = $app->getService('user.field.factory');
+    my $fieldFactory = $app->getService('user.fieldFactory');
     foreach my $fieldID (keys($fields)) {
         $self->insertField($fieldFactory->createFieldByID($app, $self, $fieldID, $fields->{$fieldID}));
     }
@@ -126,9 +131,7 @@ sub getField {
     my($category, $name) = @_;
     my $field = $self->hasField($category, $name);
     if (not defined($field)) {
-        my $field = $fieldFactory->createFieldByName($app, $self, $fieldCategory, $fieldName);
-        $self->fields->{$field->category}->{$field->name} = $field;
-        $self->fieldsByID->{$field->fieldID} = $field;        
+        $field = $self->insertField($fieldFactory->createFieldByName($app, $self, $fieldCategory, $fieldName));
     }
     return $field;
 }
@@ -170,8 +173,7 @@ 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));
-
+    my $field = $self->insertField($self->app->getService('user.fieldFactory')->createFieldByName($self->app, $self, 'contact', $fieldName, undef));
     if ($field->validate($newAddress)) {
         $self->newFieldID($field->fieldID);
         $self->newFieldValue($newAddress);
@@ -214,7 +216,19 @@ sub resetAddressChange {
 
 sub hash {
     my $self = shift;
-    return %$self; # XXX should expand fields too
+    my $result = {
+        'userID' => $self->userID,
+        'disabled' => $self->disabled,
+        'adminMessage' => $self->adminMessage,
+        'fields' => {},
+        'groups' => $self->groups,
+        'rights' => keys(%{$self->rights});
+    };
+    foreach my $field (values(%{$self->fieldsByID})) {
+        $result->{'fields'}->{$field->fieldID} = $field->data;
+        $result->{'fields'}->{$field->category.':'.$field->name} = $field->data;
+    }
+    return $result;
 }
 
 sub checkPassword {
@@ -226,7 +240,7 @@ sub checkPassword {
 sub joinGroup {
     my $self = shift;
     my($groupID) = @_;
-    $self->{'groups'}->{$groupID} = XXX;
+    $self->{'groups'}->{$groupID} = $self->app->getService('dataSource.user')->getGroupName($self->app, $groupID);
     $self->invalidateRights();
     $self->{'_DIRTY'}->{'groups'} = 1;
 }
@@ -241,7 +255,13 @@ sub leaveGroup {
 
 sub invalidateRights {
     my $self = shift;
-    # XXX redo all the rights
+    my $rights = $self->app->getService('dataSource.user')->getRights($self->app, keys(%{$self->{'groups'}}));
+    $self->rights(map {$_ => 1} @$rights); # map a list of strings into a hash for easy access
+    # don't set a dirty flag, because rights are merely a convenient
+    # cached expansion of the rights data. Changing this externally
+    # makes no sense -- what rights one has is dependent on what
+    # groups one is in, and changing the rights won't magically change
+    # what groups you are in.
 }
 
 sub propertySet {
@@ -278,11 +298,15 @@ sub DESTROY {
 }
 
 sub writeProperties {
-    # XXX
+    my $self = shift;
+    $self->app->getService('dataSource.user')->setUser($elf->app, $self->userID, $self->disabled, 
+                                                       $self->password, $self->adminMessage, 
+                                                       $self->newFieldID, $self->newFieldValue, $self->newFieldKey);
 }
 
 sub writeGroups {
-    # XXX
+    my $self = shift;
+    $self->app->getService('dataSource.user')->setUserGroups($self->app, $self->userID, keys(%{$self->{'groups'}}));
 }
 
 # fields write themselves out

webtools/PLIF/PLIF/Service/UserField.pm

@@ -33,30 +33,36 @@ use PLIF::Service;
 @ISA = qw(PLIF::Service);
 1;
 
+# This class implements a service instance -- you should never call
+# getService() to obtain a copy of this class or its descendants.
+
 sub provides {
     my $class = shift;
     my($service) = @_;
-    return ($service eq 'user.field.'.$class->category.'.'.$class->type or $class->SUPER::provides($service));
+    return ($service eq 'user.field.'.$class->type or $class->SUPER::provides($service));
 }
 
+# the 'data' field of field descriptions means different things
+# depending on the category. For 'contact' category user fields, it
+# represents a string that is prefixed to the data of the user field
+# to obtain the username that should be used for this user field.
+
 sub init {
     my $self = shift;
-    my($app, $user, $fieldID, $fieldTypeData, $fieldName, $fieldData) = @_;
+    my($app, $user, $fieldID, $fieldTypeData, $fieldCategory, $fieldName, $fieldData) = @_;
     # do not hold on to $user!
+    $self->app($app);
     $self->userID($user->userID); # change this at your peril
     $self->fieldID($fieldID); # change this at your peril
     $self->typeData($fieldTypeData); # change this at your peril
+    $self->category($fieldCategory); # 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->{'_DELETE'} = 0;
     $self->{'_DIRTY'} = 0;
 }
 
-sub category {
-    my $self = shift;
-    $self->notImplemented();
-}
-
 sub type {
     my $self = shift;
     $self->notImplemented();
@@ -66,6 +72,14 @@ sub validate {
     return 1;
 }
 
+# contact fields have usernames made of the field type data part
+# followed by the field data itself
+sub username {
+    my $self = shift;
+    $self->assert($self->category eq 'contact', 1, 'Tried to get the username from the non-contact field \''.($self->fieldID).'\'');
+    return $self->typeData.$self->data;
+}
+
 # deletes this field from the database
 sub remove {
     my $self = shift;
@@ -73,14 +87,6 @@ sub remove {
     $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
-# allowed to make an exception to this if they have very
-# distinguishable username syntaxes, for example e-mail addresses
-# should be returned "raw", as in "ian@hixie.ch" and not "E-MAIL:
-# ian@hixie.ch".
-
 sub propertySet {
     my $self = shift;
     my $result = $self->SUPER::propertySet(@_);
@@ -97,7 +103,10 @@ sub DESTROY {
 }
 
 sub write {
-    # XXX
-    # check $self->{'_DELETE'} to see if we have to remove it altogether
-    # otherwise, commit the changes
+    my $self = shift;
+    if ($self->{'_DELETE'}) {
+        $self->app->getService('dataSource.user')->removeUserField($elf->app, $self->userID, $self->fieldID);
+    } else {
+        $self->app->getService('dataSource.user')->setUserField($elf->app, $self->userID, $self->fieldID, $self->data);
+    }
 }

webtools/PLIF/PLIF/Service/UserField/String.pm

@@ -26,23 +26,15 @@
 # provisions above, a recipient may use your version of this file
 # under either the MPL or the GPL.
 
-package PLIF::Service::UserField::Generic;
+package PLIF::Service::UserField::String;
 use strict;
 use vars qw(@ISA);
 use PLIF::Service::UserField;
 @ISA = qw(PLIF::Service::UserField);
 1;
 
-sub provides {
-    my $class = shift;
-    my($service) = @_;
-    return ($service =~ /^user\.field\.[^.]+\.[^.]+$/ or $class->SUPER::provides($service));
-}
-
-sub category {
-    return 'generic';
-}
-
 sub type {
-    return 'generic';
+    return 'string';
 }
+
+# XXX anything else required here?

webtools/PLIF/PLIF/Service/UserFieldFactory.pm

@@ -36,17 +36,30 @@ use PLIF::Service;
 sub provides {
     my $class = shift;
     my($service) = @_;
-    return ($service eq 'user.field.factory' or $class->SUPER::provides($service));
+    return ($service eq 'user.fieldFactory' or $class->SUPER::provides($service));
 }
 
+# Field Factory
+#
+# The factory methods below should return service instances (not
+# objects or pointers to services in the controller's service
+# list!). These service instances should provide the 'user.field.X'
+# service where 'X' is a field type. The field type should be
+# determined from the fieldID or fieldCategory.fieldName identifiers
+# passed to the factory methods.
+
+# typically used when the data comes from the database
 sub createFieldByID {
     my $self = shift;
     my($app, $user, $fieldID, $fieldData) = @_;
     return undef; # XXX
 }
 
+# typically used when the field is being created
 sub createFieldByName {
     my $self = shift;
     my($app, $user, $fieldCategory, $fieldName, $fieldData) = @_;
+    # fieldData is likely to be undefined, as the field is unlikely to
+    # exist for this user.
     return undef; # XXX
 }