[OPEN-ILS-DEV] (Patch) Make group permission setup a bit easier to decipher

[Dan Scott]

On Wed, 2009-11-11 at 22:00 -0500, Warren Layton wrote:
> I don't know what you think of this patch.

I think it's a great patch.

>  I wrote it up after
> noticing the following comment in the
> Open-ILS/src/sql/Pg/950.data.seed-values.sql file:
> 
>    -- XXX Incomplete base permission setup.  A patch would be appreciated.
> 
> Now, I have no idea what the base permission setup should look like
> for the average library. However, figuring out what permissions are
> currently being applied is difficult with lines such as:
> 
>    INSERT INTO permission.grp_perm_map VALUES (57, 2, 15, 0, false);
> 
> ...when it could be rewritten as:
> 
>    -- Add basic patron permissions to the Patrons group
>    INSERT INTO permission.grp_perm_map (grp, perm, depth, grantable)
> VALUES (2, (SELECT id FROM permission.perm_list WHERE code =
> 'RENEW_CIRC'), 0, false);
> 
> To me, this patch makes it very clear which permissions are assigned
> to which groups (the permissions for the Acquisition groups are
> currently set in this way, too). 

I'm with you. Confession: that's one of the reasons I added the
acquisitions permissions this way.

> This added clarity may help when
> someone more knowledgeable than me tries to figure out what the "base
> permission setup" should be for each group.

Absolutely.

> The only possible problems are:
> 1) The id in permission.grp_perm_map is not explicitly set with these
> changes (e.g., in the first INSERT statement, above, the id is
> explicitly set to 57). This didn't seem like a problem to me, but I
> could be wrong.

Shouldn't be a problem.

> 2) I noticed that just before the Acquisitions group permissions, the
> following line is present:
>     SELECT SETVAL('permission.grp_perm_map_id_seq'::TEXT, (SELECT
> MAX(id) FROM permission.grp_perm_map));
> I'm not sure if I should have included it at the the start of the
> block that I changed (given that I'm not explicitly setting the id
> value).

We needed that there because the previous permissions were added with an
explicit ID, and therefore the sequence needed to be brought into sync.
Shouldn't be necessary with your patch.

> Let me know what you think and feel free to kick it back to me if
> changes are needed.

In my opinion, the only thing that we would want is a DCO. Nice work
once again, Warren!