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

[Joe Atzberger]

Subquerying is the way I initially approached such problems also, but the DB
updates are clearly built around explicitly trying to keep the ids the same
between installations.  To solve the problem that users might add their own
values, the counter for the "next" id in almost all tables is set to 1000,
like:

SELECT SETVAL('permission.perm_list_id_seq'::TEXT, 1000);

Subsequent mainline updates then explicitly target the sub-1000 id-space.

That performance would be quicker than subquerying, but I don't consider
performance an issue for db updates that are applied with the system down.
Reliability and predictability are most important.

Notably, I just turned in a patch using id 351 in perm_list, so it is not
inconceivable the 1000-slot namespace could be exhausted eventually.  The
additional risk of collision between different developers in the incremental
id space is much higher than in the arbitrary string keyspace.  That is to
say, if you added a new permission, you would almost certainly take 351 too,
and necessarily have a conflict with my new patch, whereas the chance that
you would select "HOLD_LOCAL_AVAIL_OVERRIDE" for the key is minimal.

The explicit approach certainly makes it easier to target or link to values
from other table updates, but I'm not sure what else there is to it.
Perhaps one of the architects of the current approach can vouch for it.  A
lot of us are writing DB update patches now so a guideline/policy would be
appreciated.

--Joe

On Wed, Nov 11, 2009 at 10:00 PM, Warren Layton <warren.layton at gmail.com>wrote:

> I don't know what you think of this 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). 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.
>
> 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.
> 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).
>
> Let me know what you think and feel free to kick it back to me if
> changes are needed.
>
> Thanks!
>   Warren Layton
>  NRCan Library / Bibliothèque RNCan
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://libmail.georgialibraries.org/pipermail/open-ils-dev/attachments/20091112/35c1f92d/attachment.htm