No, it's sql. You make me really nervous every time you refer to 
localsql as if it was something other than the default name for a 
source in an example configuration file. It should *not* be more.

A nit, but caused me a minute of trying to find the typo, after I 
uncommented calFBurl...

Index: sources.php.dist
===================================================================
RCS file: /repository/turba/config/sources.php.dist,v
retrieving revision 1.128
diff -Usources.php.dist -r1.128 sources.php.dist
cvs diff: invalid context length argument
-bash-3.00# cvs diff -u sources.php.dist
Index: sources.php.dist
===================================================================
RCS file: /repository/turba/config/sources.php.dist,v
retrieving revision 1.128
diff -u -r1.128 sources.php.dist
--- sources.php.dist    23 Aug 2005 18:03:55 -0000      1.128
+++ sources.php.dist    24 Aug 2005 00:30:02 -0000
@@ -192,7 +192,7 @@
          'homePhone' => 'homephone',
          'workPhone' => 'telephonenumber',
          'cellPhone' => 'mobiletelephonenumber',
-        'homeAddress' => 'homepostaladdress'
+        'homeAddress' => 'homepostaladdress',
          // 'freebusyUrl' => 'calFBURL'
      ),
      'search' => array(

Comitted to HEAD

OK

Changes made as discussed:

Removed the $true check in the upgrade script.

Removed the display_type attribute from sources.php and added a new 
config field in conf.xml for setting the source that will allow the 
creation of new shares.  Also related changes to the addressbook.php   
and templates/addressbook.inc files to reflect this change.

I'm going to make sure I've merged my changes with anything new from 
HEAD and commit...I'll also make an entry in docs/UPGRADING regarding 
the upgrade script.

That would still be possible. But only one backend will be made 
availabe to the users.
Don't make it too complicated.

I agree with you about user's not really understanding about backends. 
  I guess I just liked providing the ability for any driver to be able 
to use shares...and in the case of backends like IMSP and possibly 
Kolab, the changes made via Horde could be reflected in other clients 
the user might be using.

Ah well, it does make sense though to have only one backend provide 
this to avoid confusion.  How about in addition to a config option for 
which backend should supply this we make one of the choices "Allow 
user to choose" or something similar?  If you don't feel that makes 
sense either, I don't feel *that* strongly about it...

As always, thanks for the feedback.

Does it really make sense to allow the users to create shared address 
books for more than one backend? Most of them probably don't even know 
what a backend is. It would make more sense to me if there is only one 
backend providing the ability to share address books. Maybe through a 
configuration option, like the client backend?

Yep, I'll drop it.  I had that in there before I added the command 
line dialogs and never took it out.
This was meant as a 'user friendly' name for the source type.  For 
example, on the 'My Addressbooks' page, the user can create a new 
addressbook and must choose what type (source/backend - whatever you 
want to call it) of addressbook to create.  I thought that "localsql" 
might not be user friendly enough.  The display_type (I guess not the 
best name for this attribute) setting  holds the title that wi

Right now the only backend supporting shares is localsql, so I guess 
we could just not show that menu until there are more than one 
choices...hopefully other drivers will move to support shares in the 
future - I'll be moving the IMSP stuff over to use shares in the near 
future.

The $live parameter in the upgrade script doesn't do anything. You 
confirm the change later anyway, and setting $live to false simply 
halts the script. Either use $live = false to show the admin what 
would happen, but don't apply any changes, or drop it completely.

I don't understand what the display_type is for, and the explanation 
in sources.php doesn't help much either.

Any objections to this being commited to head so we can test further?

Ooops.  I forgot to merge changes into source.php.dist.  Please ignore 
the previous turba.patch

Thanks!

OK. Had some more time to spend on this - new patch and files attached.
I moved this logic into Turba_Driver.
I looked more closely at this.  The Ingo migration script actually is 
written only against SQL backend (at least that's what it states in 
the code).  Probably because of the fact the just about any other 
driver would require  username AND password to access username's 
prefs.  So, even if we got a complete list of users (either through an 
Auth driver or through a provided list (as in the case of the Ingo 
script) and used the Prefs API, we would still only be able to update 
the prefs on a SQL backend without user passwords.  So, what I did was 
change the upgrade script slightly to allow allow updating the prefs 
if we are using a SQL backend, and provide a notice that this is not 
possible otherwise.  The pref is only adding the newly created public 
source to the 'addressbooks' pref, so it's not a huge deal (IMHO, of 
course). Again, the problem of having user's passwords.  For some backends, we 
would need to be logged in as the user who owns the source we are 
updating.  I'm not really sure this is an issue though, as the script 
would only be needed for people using a SQL backend with 'public' -> 
true anyway.
Made the script more configurable and moved most of the options to be 
configured via $CLI->prompt().  Also, grab the table name from 
$driver->db->dsn['table'] in case we aren't using horde default of 
localsql.

Also-
     -Minor logic improvements
     -Actually delete the source instead of just removing the share info.
     -UI impovements to the My Addressbooks page.
     -Probably some other stuff I'm forgetting ;)

I was never able to reproduce this, but I'm glad it's fixed for you...
I'll can add the functionality to edit name and description and put 
that, along with permissions, into an "Edit" drop down.  I seperated 
the delete because I didn't want to present addressbooks that couldn't 
be deleted in a drop down that included a delete button.  As it stands 
now, the delete section is not shown if there are no removable 
addressbooks. I currently don't allow the user's default addressbook 
for each source that supports shares (right now, only sql) to be 
deleted.  I could be conviced that this should be changed...anyone 
else have strong opinions one way or the other?

While we are on the topic of deletions, I *am* aware that the code, as 
it stands now, does not actually delete the addressbook data itself, 
just the share that points to it.  I'm trying to figure out the best 
way to do this.  Probably going to have to add a 
$driver->deleteAddressbook or something similar.  For SQL sources, it 
would just remove all the entries in the turba_objects table that 
match the owner.  Maybe add this to the driver's capabilities array?   
Again, for some of the other types of sources, the deletion can be 
done via the share hooks if needed.
Yep, already working on this, but wanted to get the core of the 
functionality worked out first ;)

From a user's perspective, this is a very useful addition.  After 
wiping out shares from your previous code, I'm no longer seeing 
multiple copies of my own address book, (i.e. one named My Addressbook 
and another named Kevin Myer's Addressbook).

For simplicity, I'd like to see the My Addressbooks screen pattern the 
My {Calendars, Notes, Tasks} more, in the sense that those screens 
have only one menu to choose the share that is being acted on, then 
they have an Edit button, a description field, and a Save and Delete 
button.  So you'd end up with a "Create Addressbook" and "Edit 
Addressbook".

Also, a user isn't going to know much about the addressbook type.  I 
can see confusion when their choices are localsql and none.  If only 
one type is available, can this type menu just be not shown?  If more 
than one type is available, maybe a way for an admin to provide 
user-friendly text?

I thought that as well, but thought that since the logic was backend 
specific (checking that the share representing the user's private 
localsql share exists) it really didn't belong in lib/base.php.  The 
other thing I thought of is to incorporate this check into 
Turba_Driver::factory(), but then we would need a new method in each 
concrete driver that supports shares.  Something like 
$driver->_createDefaultShare() ?  The only driver I think that we 
would really need to worry about this code being in Turba is the SQL 
driver, since it's really the only 'Horde-Only' source.  Other 
backends could make use of the share hooks to do this checking, right?
Ah.  Good point.  I knew I'd overlook something that obvious...
I'll take a look at this, but I thought that the localsql source was  really the only backend that had the potential for being used as 
either a single public source OR as multiple private sources...and 
it's really only the installations that are currently using 
public=>true that would need to run this script.
Yep, and those people won't need to run the update script...their 
localsql books will work just as before whether or not they have 
'use_shares' => true.  The only difference is they will be able to 
share their addressbooks with other users and create new 
addressbooks...but I should make the upgrade script configurable as 
far as the name of the source (and the table name if not using the 
API)...Another one of those obvious things I knew I would miss ;)

Thanks for the feedback.

I didn't look closely at the code, so I can't say anything on the 
implementation, but these are the issues I found at a quick glance:

1) There's too much logic in sources.php. This must be moved somewhere 
else, maybe lib/base.php?
2) Don't assume an SQL preference backend in the migration script. Use 
the Prefs API to update the preferences. Ah, now that I read it again, 
I see that you're updating all users' preferences. Maybe this could be 
done like the filter migration script in Ingo.
3) It would be great if you could use Turba's API to update the 
contact owners, but I don't know without looking at the code, if this 
is possible. This would allow using the script with different backends.
4) Make the source (and table, if not using Turba's API) configurable. 
Most people are using localsql for private adress books.

Forget to mention that if anyone has used the earlier patch from 7/16, 
be sure to clear out any turba shares from the datatree before using 
this, as there were minor changes to the data that is stored.  A 
simple <code>DELETE FROM horde_datatree WHERE 
group_uid='horde.shares.turba';</code> should do the trick.

turba/scripts/upgrades

turba/templates/addressbooks.inc

Ok. This patch, along with the other uploaded files, fully implements 
a Horde_Share system for Turba.  Right now, only the SQL driver 
supports using shares, but other backends could easily be configured 
to tie into Turba shares via the share hooks. (The IMSP driver will be 
converted to use shares, then I can remove the IMSP Addressbook 
section in Turba's Options).  Other existing sources will continue to 
function as before.

I added a "My Addressbooks" page similar to Kronolith's "My Calendars" 
page for maintaining the shares.  This page will only be available if 
at least one source is available via shares.

I've also included an upgrade script to be used if the site was using 
'public' => true for the localsql source.  Essentially, it moves all 
existing contacts in the turba_objects table into a new globally 
shared addressbook...after a fair number of warnings to be sure it's 
not run unintentionally ;)

Feedback before it's committed?

Cool.  Sounds good to me...
Ah.  One of those forehead slapping moments...  It's amazing how clear 
some things can become after you get a nudge in the right direction ;)

A related question - If we are using Shares for permission setting on 
a source, is there a way to still allow setting max_contacts without 
allowing source level permissions to be set?  I can see a problem with 
it *looking* like you can set read/write permissions from the 
permissions UI...which, of course, would be ignored by turba.  I can 
filter out the source from appearing in the permissions UI, but then 
we loose the ability of letting an administrator set the maximum 
number of contacts.

Strange, I can't reproduce this.  Can you post or email me your 
sources.php - removing any sensitive data, of course.

Turba->Options->Address Books

My localsql is showing up twice - once as My Addressbook (like it used 
too - localsql) and also once as Kevin M. Myer's Addressbook 
(presumably localsql:myuid)

I haven't actually shared localsql with anything yet, but its 
essentially showing up as if I shared it to myself.

Yup, you did; I misread/misremembered.

This perms system hasn't been truly supported in a stable release yet. 
No need to make allowances for that. IMO, of course.
No, it's the same as setting 'public' => false on a SQL source. And 
having 'use_shares' is a *much* clearer setting than public, imo.

I thought I *did* name the attribute 'use_shares', but will double 
check when I get back to my dev. machine.

I put that attribute there in addition to 'public' for a few 
reasons...of course, my reasoning could be (and often is) misguided ;)

1 -  This allows administrators to continuing using the "old" 
permissions system after upgrading turba - in case the installation 
already has a fair number of permissions set or they are using the 
'public' localsql source.

2 - Setting 'use_shares' => true isn't really the same as setting 
'public' => true.  In the first case, using shares allows the end user 
to decide to share his/her address book and what level of permissions 
to give etc.  In the latter case, everyone would have access to the 
same source...unless maybe permissions were explicitly set against it, 
in which case it would have to be an administrator making that 
decision.  Either way, the people that had access to the source would 
see *all* the entries in the table, essentially, all the users' sql 
addressbooks.  By using shares, only the user's that choose to share 
would have their entries visible, and they would appear as a seperate 
source...not grouped together in the same address book as is the case 
with public => true.

If we were to decide to go this route, and force the use of 
Horde_Shares on those sources that will support it (localsql for now), 
an upgrade script *would* be needed.  It would basically have to 
create a new share seperate from individual user's shares, perhaps 
owned by an admin, and "copy" the existing entries into this share, 
changing the 'owner' attribute in the process.

Why is nothing ever as simple as it seems at first? ;)

The 'public' item essentially duplicates the 'user_shares' attribute 
you added. Something like 'use_shares' would be clearer, but either 
way there should only be one of these configuration items, and an 
upgrade script should be created to convert current 'public' 
addressbooks into having the appropriate permissions and share use.

Yeah, your probably right...I'll move it over when it's more complete. 
  It was just easier for me to plop it into the options to get it up 
and running quicker...
Yep, thanks.  Actually, the error checking there isn't even necessary. 
  That code was left over from an earlier implementation I was trying. 
  Turba::getConfigFromShares takes care of the error checking and 
$notification->push() - and will always return a cfgSources 
array...even if it's just the original one that was passed in.
I'm not exactly sure what your saying here.  The share code wouldn't 
have created an LDAP share.  Where are you seeing these three shares?

In a nutshell, what  should happen is this - When sources.php loads, 
it checks if there is a share called 'localsql:kmyer' if it is not 
there, it is created and given the title "Kevin M. Myer's 
Addressbook", along with appropriate owner permissions.  This 
represents the same source that's in $cfgSources['localsql'].  When a 
user is logged in, this 'localsql' addressbook will be displayed to 
them as "My Addressbook" - since that's what it is and that's probably 
what they are used to seeing it called.

If you decide to give me access to your "My Addressbook", then turba 
will see that I have access to a share called 'localsql:kmyer', it 
will then create a $cfgSources['localsql:kmyer'] entry with the 
appropriate information.  When the address books are displayed, your 
address book will be shown to me as "Kevin M. Myer's Addressbook" 
while my own address book will be shown to me as "My Addressbook".  If 
I also had an LDAP source configured, I would see three addressbooks, 
"My Addressbook", "Kevin M. Myer's Addressbook", and "Some LDAP 
Directory".  If you were logged in and did not have rights to any 
other user's address book, then you should only see the two sources - 
"My Addressbook", and "Some LDAP Directory".

Scratch the initial $result - that wasn't from your patch but I had 
thrown that in locally to work around it being missing.

Just my opinion - it should be located in the same place as My 
{Calendars, Notes, Tasks}, for consistency.
Just two minor items:

lib/base.php contains an undefined reference to $result - I'm assuming 
something like the following was intentioned:

--- base.php.bak        Sun Jul 17 18:11:27 2005
+++ base.php    Sun Jul 17 18:12:02 2005
@@ -48,9 +48,9 @@
  require TURBA_BASE . '/config/sources.php';

  // Check for any shares we have access to.
-$result = $GLOBALS['cfgSources'] = Turba::getConfigFromShares($cfgSources);
-if (is_a($result, 'PEAR_Error')) {
-    $notification->push($result);
+$GLOBALS['cfgSources'] = Turba::getConfigFromShares($cfgSources);
+if (is_a($GLOBALS['cfgSources'], 'PEAR_Error')) {
+    $notification->push($GLOBALS['cfgSources']);
  }

  $GLOBALS['cfgSources'] = 
Turba::permissionsFilter($GLOBALS['cfgSources'], 'source');

Also, where I previously had two shares (My Addressbook -> localsql, 
Shared Directory -> ldap), I now have three (including a new Kevin M. 
Myer's Addressbook).  Not sure if that is an uninentional side effect 
or not.

Attached file patches turba to enable the use of Horde_Share from 
within Turba.  Right now, only support for the localsql source is 
included, but it wouldn't be difficult to expand this to include other 
sources.

I added the UI to edit the shares under the addressbook option 
settings.  This can obviously be changed if there is a better place 
for this.  I also  haven't implemented adding completly new shares 
yet, as I wanted to get feedback on this before I continued further.

So, basically, right now this allows the user to share his/her 
localsql addressbook with any other user / group etc...  If things 
look good to everybody, I'll procede with implementing adding new 
shares (new addressbooks) and adding support for syncing up IMSP acls 
with the share system.

This was mplemented in such a way that the use of shares can be 
enabled on a per source basis, enabling administrators to continue to 
use the existing permissions system on any (or all) sources.  Shares, 
if defined, will still continue to honor any extended permissions 
defined using the Horde_Perms system (such as max_contacts).

Let me know what you think...I'm sure the code could be cleaned up a 
bit, but I wanted to get feedback before spending much more time on it 
;)

How about letting Turba_Driver objects act like Share objects, or have 
a getShare() method that would return a Share object that behaved like 
a share but had the backend's permissions?

It's a little hard for me to suggest more without seeing your design, I guess.

Thinking this through, this would break existing installations that 
have permissions already set (other than max_contacts).  Should this 
be implemented in such a way that it can coexist with the existing 
permissions based system - maybe make it configurable on a per-source 
basis whether to use Horde Perms or Horde Share?

I've been tinkering with implementing a Share implemention for Turba 
to allow users to more easily share their personal addressbooks with 
other users.  I've got some *very* basic code worked out, but nothing 
ready for committing yet.

Just wanted to create the ticket to get this "on the books" so to speak...