Re: [Postfixadmin-devel] refactoring

[Christian B. <pos...@cb...>]

Hello,

this mail has two parts:

a) summing up the IRC session (copy and paste from the IRC log)

b) things we didn't talk about in IRC

Gingerdog, you might want to skip to b) ;-)


a)

Am Montag, 28. Juli 2008 schrieb David Goodwin:
> Christian Boltz wrote :
> > Am Mittwoch, 23. Juli 2008 schrieb Christian Boltz:
> > > My idea in short:
> > > - create a basic class with the required functions/methods.
> >
> > I have written a proof-of-concept class postfixadminBaseclass as
> > described in my proposal. It is attached to this mail.

> Right, I'll include the code inline, as I'm lazy (and using mutt,
> while my office computer is dead thanks to the thunder+lightening)

> class postfixadminBaseclass {
...
>     # name, also used for generating links
>     public function name($listmode=0) {
>         # usually:
>         return "baseclass";
...
>     }
>
> ^^ What's name() trying to do?

The intention was to use it as basename for generating links (to 
the "add new", "edit" and "list" pages). However, I'm not sure if this 
should be really inside the class, since it has to do with HTML 
rendering.

-> removal candidate, I'll comment it out in my draft (and already have 
an idea how to replace it ;-)

...
>     # handles add and edit calls internally
>     protected function addOrEdit($primarykey, Array $newvalues,
> $mode) { # mode: 0 = edit, 1 = new
>
>         #    calls: [TODO]
>         #    - [non-public] function validate_by_type() (simple check
> against #      field type given in table structure)
>
> ^^ I think this is unnecessary; database introspection might be a
> better bet.

[GingerDog] i think type checks etc would be better handled by asking 
            the model class to validate() itself

we can do most checks this way (nearly automatically - in the central 
class). And it even has the advantage that we can define our own types 
if needed - like "mail address"

...
>     # TODO
>     #  * [non-public] check_domain_permission() (check if the admin
> has #    permissions for this domain)
>     #  * [non-public] check_other_permission() (check other
> permissions, for #    example if editing mailbox aliases is allowed)
...
> It would be nice for the permission definitions to be done in one
> place, where ever that may be.
>
> Could permissions be listed a bit like :
>
> e.g.
>
> /**
>  * decorator for permission enforcement
>  */
> class PermissionProxy {
...

I'm not planning to implement a login($user, $pass) inside the class. 
But I'm thinking about a check "has permissions for this domain?" or "is 
superadmin?" which is basically what we have in each script right now.
The base class would handle this without modification in each module 
And the "special permission checks" I mentioned are things like checking 
$CONF['fetchmail'] ;-)

[GingerDog] Permissions checks (or at least enforcements) shouldn't 
            really be in the individual controller(s)  
            if they are moved to the model classes then they take effect 
            for any user, regardless of whether they're coming through  
            the web interface, xmlrpc or whatever 
            and there may be less repetition 

The checks would be in the base class - no need to redefine them each 
time. Just using the functions from the parent class doesn't look bad 
for me ;-)


Part b)

> So, exposed via xmlrpc etc...
>
> /**
>  * phpdoc needs adding IRL
>  * clients (whether 'local' or via xmlrpc/soap etc) connect to this.
>  */
> class AliasServer {
...
>         public function addAlias($p, $p, $p) {
>                 return $this->backend->addAlias($u, $p);
>         }

If possible, we should have common names ("add", "edit") instead of 
type-specific ones. If you simply add a $type-Parameter, you don't even 
need to implement a xmlrpc connector class for each type ;-)
(the data arguments can/should be passed as array to be flexible)

>         public function getAliasList($domain, $offset, $limit) {
>                 return $this->backend->getAliasList($domain, $offset,
> $limit); }

Ah, you just found two missing parameters in my items() function: 
$offset and $limit. I now have:
public function items 
    ($domain="", $admin="", $search="", $offset = -1, $limit = -1)

Looks like I should switch to a named parameter array instead of lots of 
optional parameters...

    # $filter can contain several parameters to filter the list.
    # All parameters in $filter are optional.
    # $filter = array(
    #   'domain' -> "",
    #   'admin'  -> "",
    #   'search  -> "",
    #   'offset' -> 0,
    #   'limit'  -> -1, # unlimited
    # )
    public function items (array $filter) {

Looks better ;-)

>         public function search($domain, $string) {
>                 // whatever
>         }

Already included in items(), see above ;-)

> /* The above isn't quite right yet, as $username/$password need
>  * passing in to each method call... as xmlrpc isn't very stateful
> ... * perhaps my decorator idea can't/won't work
>  */

This makes another two parameters to the generic xmlrpc calls: username 
and password. (I'll forget about this for now - first I need a working 
baseclass and then I'll think about xmlrpc again ;-)

> Then for the actual Model object (e.g. Alias) :
...
> class Alias extends Zend_Db_Abstract {
>         protected $_name = 'alias';
>         protected $_primary = 'id'; // whatever the pkey field is
>         protected $_sequence = false; // force use of pkey field
> value in insert . public function __construct() {
>                 $db = Zend_Db::factory('....whatever...'); //?
>                 parent::__construct($db);
>         }
...

Or just use our db_insert (and db_update, db_delete etc.) function we 
already have. I'll probably do this when filling the baseclass with 
code.

And again: Do _not_ make such things specific to a table or module 
unless really needed ;-)

> > Comments welcome: (ditto)
> > - do you think this could become the basis of all postfixadmin
> > pages?
>
> I'm keen to separate a page from the underlying logic used for it.

/me too ;-)

> We should provide e.g. xmlrpc or soap integration, and decouple the
> business logic from the controller scripts that generate html pages.

ACK - even if my main target is to make maintenance and writing new 
modules easier. Adding xmlrpc or soap integration doesn't conflict with 
this target.

> Having some sort of base controller and/or model class will be a good
> thing.

Yes. I'm sure we don't even foresee how many good things we will get by 
having a class-based implementation :-)

Last IRC quote for this mail:
If my class works as I expect, most modules will contain of "overwrite 
table name, overwrite database scheme/field list, overwrite default 
values" - and nothing else :-)

> > - what should I/we do better? (We have 35 °C outside, so I'm sure
> > the baseclass isn't perfect ;-)
>
> Heh; it was warm here, but also humid (as is nearly always the case
> in the UK); thankfully we've just had a few thunderstorms and things
> seem better. The dog wasn't happy though.

;-)

> I think a good start is perhaps to list all classes, and the
> functions they need :

Yes - and we should also keep in mind how to map them on the standard 
method names in my baseclass.

> Vacation:
>   - isEnabled($email);

items() with a search parameter (empty result = no vacation)

The easier way would be an itemExists() function, however I'm not sure 
if we need this.

>   - create($email, $subject, $body, $whatever);
>   - delete($email, $subject, $body, $whatever);
>   - retrieve($email);
>
> Domain:
>   - list($offset, $limit = 20)
>   - retrieve($name);
>   - create($name, $backup_mx, etc);
>   - delete($name);
>
> Mailbox:
>   - list($offset, $limit = 20);
>   - retrieve($name);
>   - create($name, $fullname, $password, ... etc)
>   - delete($name);

All those will work with the functions I already have
[retrieve -> item(), create -> add()].

> There would need to be some sort of shared 'authentication'/'user'
> data/object so we can enforce permission checks (e.g that a user can
> only delete their own mailbox, and not someone elses; but a global
> admin can delete any... etc)

Yes - as written above, the login details could just be additional 
parameters.

The username has to be passed to the baseclass somehow (parameter to 
each function or as a global variable - what's better?)

The password check should be done outside because it differs on the 
implementation (HTML output has a session, xmlrpc might need to pass it 
each time).


My personal summary of the mail is that my baseclass isn't that bad.
If nobody objects, I'll create a classes/ directory in SVN and check it 
in in the next days ;-)


Regards,

Christian Boltz
-- 
Und dann war da noch der junge Mann, der unbedingt Schriftsteller 
werden wollte. Er wollte Emotionen wecken und die Leute zum Weinen
bringen. Sein Traum wurde wahr, er verfaßt heute die Fehlermeldungen
bei Microsoft.