* Added a section on common errors to Documentation.txt

* Added more comments to a few modules
* Changed the semantics of adding and removing user fields ("user data types")
* Implemented a factory service to perform those semantics ('registerField', 'removeField' and the convenience 'registerSetting')
* Added a missing return in the MySQL implementation of the user data source
* Added an assert in the MySQL version of 'addRight' to catch an easy error
* Added XXX comment in the MySQL version of 'addRight' (will fix this in the next patch)
* Fixed a case where sending an undefined value to the DBI database implementation would fail during the untainting
* Fixed MySQLID to actually work (in the DBI ResultsFrame)
* Fixed two typos in the user service ('settings' to 'setting')
* Added the requirement that a user must have certain rights to use the COSES editor
* Added a setupInstall implementation to the COSES editor

webtools/PLIF/Documentation.txt

@@ -473,6 +473,33 @@ CHAPTER 9: INPUT AND OUTPUT
 
 Some day.
 
+___________________________
+CHAPTER 10: COMMON MISTAKES
+| Wherein perls of wisdom are given out in order to save the reader
+| some time.
+
+If you get an error message of the form:
+
+   Can't locate object method "myMethod" via package "a.service.name" at SomePath/SomeModule.pm line 42
+
+...then you are probably calling 'getService' on the $self object
+instead of the $app object. The error happens because 
+
+   $aPLIFObject->getService('a.service.name')->myMethod();
+
+...results in |$aPLIFObject->{'a.service.name'}| being set to
+'a.service.name' and returns that exact value, which then results in
+the method call being invoked on the string, as in:
+
+   'a.service.name'->myMethod();
+
+...which of course will fail, since hte package 'a.service.name'
+doesn't exist and thus doesn't have a method called myMethod().
+
+For the same reason, this error can happen if you misspell the name of
+the service getter method (e.g., if you use |$app->getServiceList()|
+instead of |$app->getCollectingServiceList()|).
+
 _____________________
 CHAPTER n: CONCLUSION
 | Wherein it is revealed that all is subject to change, only available

webtools/PLIF/PLIF/DataSource/User.pm

@@ -161,14 +161,22 @@ sub setField {
     # if fieldID is undefined, then add a new entry and return the
     # fieldID. Typically data will be undefined then too.
     $self->notImplemented();
+    # The caller should make sure that the relevant service
+    # ('user.field.$type.manager') is then notified
+    # ('fieldAdded($fieldID)') so that any additional database setup
+    # can be performed. If this was merely a change and not a new
+    # addition, then call fieldChanged($fieldID) instead. (It is a
+    # change if you pass $fieldID, and it is an addition if $fieldID
+    # is undefined.)
+    # returns the fieldID.
 }
 
 sub removeField {
     my $self = shift;
     my($app, $fieldID) = @_;
     # The caller should make sure that the relevant service
-    # ('user.field.$type.remover') is notified
-    # ('removeField($fieldID)') so that any additional database
+    # ('user.field.$type.manager') is notified
+    # ('fieldRemoved($fieldID)') so that any additional database
     # cleanup can be performed.
     $self->notImplemented();
 }

webtools/PLIF/PLIF/DataSource/User/MySQL.pm

@@ -179,6 +179,7 @@ sub setField {
     if (defined($fieldID)) {
         $self->database($app)->execute('UPDATE userDataTypes SET category=?, name=?, type=?, data=?, mode=? WHERE fieldID = ?',
                                        $category, $name, $type, $data, $mode, $fieldID);
+        return $fieldID;
     } else {
         return $self->database($app)->execute('INSERT INTO userDataTypes SET category=?, name=?, type=?, data=?, mode=?',
                                               $category, $name, $type, $data, $mode)->MySQLID;
@@ -263,7 +264,10 @@ sub getRights {
 sub addRight {
     my $self = shift;
     my($app, $name) = @_;
+    $self->assert($name, 1, 'Tried to add a right without a right name');
     # only adds $name if it isn't there already, because name is a unique index
+    # XXX this currently causes an error instead of silently doing nothing.
+    # XXX this must be fixed; can we do it in one step, without a query?
     my $rightID = $self->database($app)->execute('INSERT INTO rights SET name=?', $name)->MySQLID;
 }
 

webtools/PLIF/PLIF/Database/DBI.pm

@@ -103,8 +103,12 @@ sub createResultsFrame {
     my $handle = $self->handle->prepare($statement);
     # untaint the values... (XXX?)
     foreach my $value (@values) {
-        $value =~ /^(.*)$/os;
-        $value = $1;
+        if (defined($value)) {
+            $value =~ /^(.*)$/os;
+            $value = $1;
+        } else {
+            $value = '';
+        }
     }
     if ($handle and ((not defined($execute)) or $handle->execute(@values))) {
         return PLIF::Database::ResultsFrame::DBI->create($handle, $self, $execute);

webtools/PLIF/PLIF/Database/ResultsFrame/DBI.pm

@@ -69,7 +69,7 @@ sub reexecute {
 # This should only be used by MySQL-specific DBI data sources
 sub MySQLID {
     my $self = shift;
-    return $self->handle->database->{'mysql_insertid'};
+    return $self->handle->{'mysql_insertid'};
 }
 
 # other possible APIs:

webtools/PLIF/PLIF/Program.pm

@@ -172,6 +172,8 @@ sub hash {
 # you couldn't dispatch the command.
 # Note: Don't confuse this method with the identically named method in
 # the Service class hierarchy that does something similar!
+# Also Note: Application.pm overrides this to forward commands to
+# services implementing the 'dispatcher.commands' service.
 sub dispatch {
     my $self = shift;
     my($command) = @_;

webtools/PLIF/PLIF/Service/Components/AdminCommands.pm

@@ -73,6 +73,7 @@ sub cmdSetup {
     my $self = shift;
     my($app) = @_;
     my $result;
+    # call all the setup handlers until one fails:
     $result = $app->getSelectingServiceList('setup.configure')->setupConfigure($app);
     if (not $result) {
         $result = $app->getSelectingServiceList('setup.install')->setupInstall($app);

webtools/PLIF/PLIF/Service/Components/CosesEditor.pm

@@ -33,7 +33,9 @@ use PLIF::Service;
 @ISA = qw(PLIF::Service);
 1;
 
-# XXX I need to register and require rights
+# XXX Add more fine grained control over rights (this would have the
+# side-effect of removing the redundancy in each of the cmdXXX calls
+# below, which would be nice...)
 
 sub provides {
     my $class = shift;
@@ -43,6 +45,7 @@ sub provides {
             $service eq 'dispatcher.output.generic' or
             $service eq 'dispatcher.output' or
             $service eq 'dataSource.strings.default' or
+            $service eq 'setup.install' or
             $class->SUPER::provides($service));
 }
 
@@ -51,152 +54,177 @@ sub cmdCosesEditor {
     # warning: this is also called from other methods below
     my $self = shift;
     my($app) = @_;
-    my %variants = $app->getService('dataSource.strings')->getDescribedVariants();
-    my $variantsSortColumn = $app->input->getArgument('cosesEditorVariantsSortColumn');
-    my %strings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
-    my $stringsSortColumn = $app->input->getArgument('cosesEditorStringsSortColumn');
-    my $user = $app->getObject('user');
+    my $user = $app->getService('user.login')->hasRight($app, 'cosesEditor');
     if (defined($user)) {
-        # XXX have to register these settings
+        my %variants = $app->getService('dataSource.strings')->getDescribedVariants();
+        my $variantsSortColumn = $app->input->getArgument('cosesEditorVariantsSortColumn');
+        my %strings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
+        my $stringsSortColumn = $app->input->getArgument('cosesEditorStringsSortColumn');
+        # if (defined($user)) {
         $user->setting(\$variantsSortColumn, 'cosesEditor.index.variantsSortColumn');
         $user->setting(\$stringsSortColumn, 'cosesEditor.index.stringsSortColumn');
-    }
-    $app->output->cosesEditorIndex(\%variants, $variantsSortColumn, \%strings, $stringsSortColumn);
+        # }
+        $app->output->cosesEditorIndex(\%variants, $variantsSortColumn, \%strings, $stringsSortColumn);
+    } # else, user has been notified
 }
 
 # dispatcher.commands
 sub cmdCosesVariantAdd {
     my $self = shift;
     my($app) = @_;
-    my @data = ('', '', 1.0, '', '', '', '', '', '');
-    my $id = $app->getService('dataSource.strings')->setVariant($app, undef, @data);
-    my %expectedStrings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
-    $app->output->cosesEditorVariant($id, @data, \%expectedStrings, {});
+    my $user = $app->getService('user.login')->hasRight($app, 'cosesEditor');
+    if (defined($user)) {
+        my @data = ('', '', 1.0, '', '', '', '', '', '');
+        my $id = $app->getService('dataSource.strings')->setVariant($app, undef, @data);
+        my %expectedStrings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
+        $app->output->cosesEditorVariant($id, @data, \%expectedStrings, {});
+    } # else, user has been notified
 }
 
 # dispatcher.commands
 sub cmdCosesVariantEdit {
     my $self = shift;
     my($app) = @_;
-    my $id = $app->input->getArgument('cosesEditorVariantID');
-    my $dataSource = $app->getService('dataSource.strings');
-    my @data = $dataSource->getVariant($app, $id);
-    my %expectedStrings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
-    my %variantStrings = $dataSource->getVariantStrings($app, $id);
-    $app->output->cosesEditorVariant($id, @data, \%expectedStrings, \%variantStrings);
+    my $user = $app->getService('user.login')->hasRight($app, 'cosesEditor');
+    if (defined($user)) {
+        my $id = $app->input->getArgument('cosesEditorVariantID');
+        my $dataSource = $app->getService('dataSource.strings');
+        my @data = $dataSource->getVariant($app, $id);
+        my %expectedStrings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
+        my %variantStrings = $dataSource->getVariantStrings($app, $id);
+        $app->output->cosesEditorVariant($id, @data, \%expectedStrings, \%variantStrings);
+    } # else, user has been notified
 }
 
 # dispatcher.commands
 sub cmdCosesVariantAddString {
     my $self = shift;
     my($app) = @_;
-    my %expectedStrings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
-    my($id, $data, $variantStrings) = $self->getVariantEditorArguments($app);
-    $app->output->cosesEditorVariant($id, @$data, \%expectedStrings, $variantStrings);
+    my $user = $app->getService('user.login')->hasRight($app, 'cosesEditor');
+    if (defined($user)) {
+        my %expectedStrings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
+        my($id, $data, $variantStrings) = $self->getVariantEditorArguments($app);
+        $app->output->cosesEditorVariant($id, @$data, \%expectedStrings, $variantStrings);
+    } # else, user has been notified
 }
 
 # dispatcher.commands
 sub cmdCosesVariantCommit {
     my $self = shift;
     my($app) = @_;
-    my($id, $data, $variantStrings) = $self->getVariantEditorArguments($app);
-    my $dataSource = $app->getService('dataSource.strings');
-    $dataSource->setVariant($app, $id, @$data);
-    foreach my $string (keys(%$variantStrings)) {
-        $dataSource->setString($app, $id, $string, $variantStrings->{$string});
-    }
-    $self->cmdCosesEditor($app);
+    my $user = $app->getService('user.login')->hasRight($app, 'cosesEditor');
+    if (defined($user)) {
+        my($id, $data, $variantStrings) = $self->getVariantEditorArguments($app);
+        my $dataSource = $app->getService('dataSource.strings');
+        $dataSource->setVariant($app, $id, @$data);
+        foreach my $string (keys(%$variantStrings)) {
+            $dataSource->setString($app, $id, $string, $variantStrings->{$string});
+        }
+        $self->cmdCosesEditor($app);
+    } # else, user has been notified
 }
 
 # dispatcher.commands
 sub cmdCosesStringEdit {
     my $self = shift;
     my($app) = @_;
-    my $id = $app->input->getArgument('cosesEditorStringID');
-    my %strings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
-    my $dataSource = $app->getService('dataSource.strings');
-    my %expectedVariants = $dataSource->getDescribedVariants();    
-    my %stringVariants = $dataSource->getStringVariants($app, $id);
-    $app->output->cosesEditorString($id, $strings{$id}, \%expectedVariants, \%stringVariants);
+    my $user = $app->getService('user.login')->hasRight($app, 'cosesEditor');
+    if (defined($user)) {
+        my $id = $app->input->getArgument('cosesEditorStringID');
+        my %strings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
+        my $dataSource = $app->getService('dataSource.strings');
+        my %expectedVariants = $dataSource->getDescribedVariants();    
+        my %stringVariants = $dataSource->getStringVariants($app, $id);
+        $app->output->cosesEditorString($id, $strings{$id}, \%expectedVariants, \%stringVariants);
+    } # else, user has been notified
 }
 
 # dispatcher.commands
 sub cmdCosesStringCommit {
     my $self = shift;
     my($app) = @_;
-    my $input = $app->input;
-    my $id = $input->getArgument('cosesEditorStringID');
-    my %variants = ();
-    my $index = 0;
-    while (defined(my $name = $input->getArgument('cosesEditorStringVariant${index}Name'))) {
-        $variants{$name} = $input->getArgument('cosesEditorStringVariant${index}Value');
-        $index += 1;
-    }
-    my $dataSource = $app->getService('dataSource.strings');
-    foreach my $variant (keys(%variants)) {
-        $dataSource->setString($app, $variant, $id, $variants{$variant});
-    }
-    $self->cmdCosesEditor($app);
+    my $user = $app->getService('user.login')->hasRight($app, 'cosesEditor');
+    if (defined($user)) {
+        my $input = $app->input;
+        my $id = $input->getArgument('cosesEditorStringID');
+        my %variants = ();
+        my $index = 0;
+        while (defined(my $name = $input->getArgument('cosesEditorStringVariant${index}Name'))) {
+            $variants{$name} = $input->getArgument('cosesEditorStringVariant${index}Value');
+            $index += 1;
+        }
+        my $dataSource = $app->getService('dataSource.strings');
+        foreach my $variant (keys(%variants)) {
+            $dataSource->setString($app, $variant, $id, $variants{$variant});
+        }
+        $self->cmdCosesEditor($app);
+    } # else, user has been notified
 }
 
 # dispatcher.commands
 sub cmdCosesVariantExport {
     my $self = shift;
     my($app) = @_;
+    my $user = $app->getService('user.login')->hasRight($app, 'cosesEditor');
+    if (defined($user)) {
 
-    # get data
-    my $id = $app->input->getArgument('cosesEditorVariantID');
-    my $dataSource = $app->getService('dataSource.strings');
-    my @data = $dataSource->getVariant($app, $id);
-    my %strings = $dataSource->getVariantStrings($app, $id);
+        # get data
+        my $id = $app->input->getArgument('cosesEditorVariantID');
+        my $dataSource = $app->getService('dataSource.strings');
+        my @data = $dataSource->getVariant($app, $id);
+        my %strings = $dataSource->getVariantStrings($app, $id);
 
-    # serialise variant
-    my $XML = $app->getService('service.xml');
-    # note. This namespace is certainly not set in stone. Please make better suggestions. XXX
-    my $result = '<variant xmlns="http://bugzilla.mozilla.org/variant/1"';
-    foreach my $name (qw(protocol quality type encoding charset language description translator)) {
-        my $value = $XML->escape(shift(@data));
-        $result .=   "\n         $name=\"$value\"";
-    }
-    $result .= ">\n";
-    foreach my $string (keys(%strings)) {
-        my $name = $XML->escape($string);
-        my $value = $XML->escape($strings{$string});
-        $result.= "  <string name=\"$name\">$value</string>\n";
-    }
-    $result .= '</variant>';
-    $app->output->cosesEditorExport($id, $result);
+        # serialise variant
+        my $XML = $app->getService('service.xml');
+        # note. This namespace is certainly not set in stone. Please make better suggestions. XXX
+        my $result = '<variant xmlns="http://bugzilla.mozilla.org/variant/1"';
+        foreach my $name (qw(protocol quality type encoding charset language description translator)) {
+            my $value = $XML->escape(shift(@data));
+            $result .=   "\n         $name=\"$value\"";
+        }
+        $result .= ">\n";
+        foreach my $string (keys(%strings)) {
+            my $name = $XML->escape($string);
+            my $value = $XML->escape($strings{$string});
+            $result.= "  <string name=\"$name\">$value</string>\n";
+        }
+        $result .= '</variant>';
+        $app->output->cosesEditorExport($id, $result);
+    } # else, user has been notified
 }
 
 # dispatcher.commands
 sub cmdCosesVariantImport {
     my $self = shift;
     my($app) = @_;
+    my $user = $app->getService('user.login')->hasRight($app, 'cosesEditor');
+    if (defined($user)) {
 
-    # get data
-    my $file = $app->input->getArgument('cosesEditorImportData');
+        # get data
+        my $file = $app->input->getArgument('cosesEditorImportData');
 
-    # parse data
-    my $XML = $app->getService('service.xml');
-    my $data = {
-        'depth' => 0,
-        'string' => undef,
-        'variant' => [], # same at all scopes (because walkNesting() is not a deep copy)
-        'strings' => {}, # same at all scopes (because walkNesting() is not a deep copy)
-    };
-    $XML->walk($self, $XML->parse($file), $data);
+        # parse data
+        my $XML = $app->getService('service.xml');
+        my $data = {
+            'depth' => 0,
+            'string' => undef,
+            'variant' => [], # same at all scopes (because walkNesting() is not a deep copy)
+            'strings' => {}, # same at all scopes (because walkNesting() is not a deep copy)
+        };
+        $XML->walk($self, $XML->parse($file), $data);
 
-    # add data
-    my $dataSource = $app->getService('dataSource.strings');
-    my $id = $dataSource->setVariant($app, undef, @{$data->{'variant'}});
-    foreach my $string (keys(%{$data->{'strings'}})) {
-        $dataSource->setString($app, $id, $string, $data->{'strings'}->{$string});
-    }
+        # add data
+        my $dataSource = $app->getService('dataSource.strings');
+        my $id = $dataSource->setVariant($app, undef, @{$data->{'variant'}});
+        foreach my $string (keys(%{$data->{'strings'}})) {
+            $dataSource->setString($app, $id, $string, $data->{'strings'}->{$string});
+        }
 
-    # display data
-    my %expectedStrings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
-    my %variantStrings = $dataSource->getVariantStrings($app, $id);
-    $app->output->cosesEditorVariant($id, @{$data->{'variant'}}, \%expectedStrings, \%variantStrings);
+        # display data
+        my %expectedStrings = @{$app->getCollectingServiceList('dispatcher.output')->strings};
+        my %variantStrings = $dataSource->getVariantStrings($app, $id);
+        $app->output->cosesEditorVariant($id, @{$data->{'variant'}}, \%expectedStrings, \%variantStrings);
+    } # else, user has been notified
 }
 
 # service.xml.sink
@@ -342,6 +370,19 @@ sub getDefaultString {
     return; # nope, sorry
 }
 
+# setup.install
+sub setupInstall {
+    my $self = shift;
+    my($app) = @_;
+    $self->dump(9, 'about to configure COSES editor...');
+    my $fieldFactory = $app->getService('user.fieldFactory');
+    $fieldFactory->registerSetting($app, 'cosesEditor.index.stringsSortColumn', 'string');
+    $fieldFactory->registerSetting($app, 'cosesEditor.index.variantsSortColumn', 'string');
+    my $userDataSource = $app->getService('dataSource.user');
+    $userDataSource->addRight($app, 'cosesEditor');
+    $self->dump(9, 'done configuring COSES editor');
+}
+
 
 
 # Internal Routines

webtools/PLIF/PLIF/Service/Components/Login.pm

@@ -109,6 +109,8 @@ sub cmdSendPassword {
 }
 
 # user.login
+# if this returns undef, don't do anything!
+# XXX need a quieter version of this to enable/disable UI elements
 sub hasRight {
     my $self = shift;
     my($app, $right) = @_;

webtools/PLIF/PLIF/Service/User.pm

@@ -228,9 +228,9 @@ sub setting {
     my($variable, $setting) = @_;
     $self->assert(ref($variable) eq 'SCALAR', 1, 'Internal Error: User object was expecting a scalar ref for setting() but didn\'t get one');
     if (defined($$variable)) {
-        $self->getField('settings', $setting)->data($$variable);
+        $self->getField('setting', $setting)->data($$variable);
     } else {
-        my $field = $self->hasField('settings', $setting);
+        my $field = $self->hasField('setting', $setting);
         if (defined($field)) {
             $$variable = $field->data;
         }
@@ -295,7 +295,7 @@ sub invalidateRights {
     # 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.
+    # what groups you are in (how could it).
 }
 
 sub propertySet {

webtools/PLIF/PLIF/Service/UserField.pm

@@ -65,7 +65,7 @@ sub init {
     $self->typeData($fieldTypeData); # change this at your peril
     $self->mode($fieldMode); # 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
+    # don't forget to update the user's 'hash' function if you add more member variables here
     $self->{'_DELETE'} = 0;
     $self->{'_DIRTY'} = 0;
 }

webtools/PLIF/PLIF/Service/UserFieldFactory.pm

@@ -39,7 +39,7 @@ sub provides {
     return ($service eq 'user.fieldFactory' or $class->SUPER::provides($service));
 }
 
-# Field Factory
+# Field Factory (Factory for Field Instances)
 #
 # The factory methods below should return service instances (not
 # objects or pointers to services in the controller's service
@@ -71,3 +71,53 @@ sub createFieldByName {
     $app->assert(defined($field), 1, "Database contains a field of type '$type' but there is no service providing that type");
     return $field;
 }
+
+
+# Field Factory (Factory for Field Types)
+#
+# These methods add (and remove) field types to (and from) the
+# database. They don't return anything in particular.
+# These methods are not expected to be called during normal
+# operations, only during installations and upgrades. 
+
+sub registerField {
+    my $self = shift;
+    my($app, $fieldCategory, $fieldName, $fieldType, @data) = @_;
+    my $dataSource = $app->getService('dataSource.user');
+    # see if the field already exists, so that we can keep the fieldID
+    # the same:
+    my($oldType, $fieldID) = $dataSource->getFieldByName($app, $fieldCategory, $fieldName);
+    # if the type changes, then act as if it was deleted:
+    if ((defined($oldType)) and ($oldType ne $fieldType)) {
+        $app->getCollectingServiceList("user.field.$oldType.manager")->fieldRemoved($fieldID);
+    }
+    $fieldID = $dataSource->setField($app, $fieldID, $fieldCategory, $fieldName, $fieldType, @data);
+    # if the field is new or if the type changed, then notify the field type's manager of this:
+    if ((not defined($oldType)) or ($oldType ne $fieldType)) {
+        $app->getCollectingServiceList("user.field.$oldType.manager")->fieldAdded($fieldID);
+    } else {
+        # otherwise, just do a change notification
+        $app->getCollectingServiceList("user.field.$oldType.manager")->fieldChanged($fieldID);
+    }
+    # return the fieldID
+    return $fieldID
+}
+
+sub registerSetting {
+    my $self = shift;
+    my($app, $setting, @data) = @_;
+    return $self->registerField($app, 'setting', $setting, @data);
+}
+
+sub removeField {
+    my $self = shift;
+    my($app, $fieldCategory, $fieldName) = @_;
+    my $dataSource = $app->getService('dataSource.user');
+    # get the field's data (ID and type, in particular)
+    my($fieldType, $fieldID) = $dataSource->getFieldByName($app, $fieldCategory, $fieldName);
+    if (defined($fieldType)) {
+        $app->getCollectingService("user.field.$fieldType.manager")->fieldRemoved($fieldID);
+        $dataSource->removeField($app, $fieldID);
+    } # else, field wasn't there to start with, so...
+    return $fieldID;
+}