[OPEN-ILS-DEV] opt-in patch
Mike Rylander
mrylander at gmail.com
Wed Jun 10 22:26:26 EDT 2009
On Wed, Jun 10, 2009 at 6:15 PM, James
Fournie<jfournie at sitka.bclibraries.ca> wrote:
> Hi all,
>
> As per my discussion long ago with Mike, I've created a big patch for
> the opting functionality, which we are using at SITKA. We needed to
> improve the opting functionality to better acommodate mutli-branch
> sites. SITKA is currently using 1.2, and I can provide the patch for
> that version if anyone wants, however the patch I have attached here
> has been modified for trunk.
Very cool, James. Thanks!
I'm running through the patch right now (pasted here for context) and
I've spotted a couple things I want to point out for your current use,
and I have some thoughts on reworking it a bit for trunk, which I'll
add below the patch and in-line comments. My interjections will start
with >>>>>>>>>>> and end with <<<<<<<<<<<. I'll use the following
example tree to simplify things (you might want to use a monospace
font ...):
A
|
|--- A-A
| |--- A-A-A
| |--- A-A-B
|
|---A-B
|--- A-B-A
|--- A-B-B
===================================================================
--- Open-ILS/src/perlmods/OpenILS/Application/Actor.pm (revision 13335)
+++ Open-ILS/src/perlmods/OpenILS/Application/Actor.pm (working copy)
@@ -1124,14 +1124,23 @@
api_name => "open-ils.actor.patron.search.advanced" );
sub patron_adv_search {
my( $self, $client, $auth, $search_hash,
- $search_limit, $search_sort, $include_inactive, $search_depth ) = @_;
+ $search_limit, $search_sort, $include_inactive, $search_ou ) = @_;
my $e = new_editor(authtoken=>$auth);
return $e->event unless $e->checkauth;
- return $e->event unless $e->allowed('VIEW_USER');
+ return $e->event unless $e->allowed('VIEW_USER', $search_ou);
>>>>>>>>
If I'm reading this right, staff are effectively required to have
global VIEW_USER permissions in order to search "higher" in the tree
than their home or work org unit for the purpose of finding opted in
users from foreign libraries. For instance, A staff member at A-A-A
would need consortial VIEW_USER permissions in order to search
"Everywhere" and find users that are opted in at their library but
belong to A-B-A. However, if they don't have the perm everywhere, the
search will simply fail instead of returning users they can see from
the scope they're allowed. IOW, the perm scoping there is new, and
it's really the opt-in stuff that is preventing patron visibility, so
I think I'm missing the purpose of this change.
<<<<<<<<
+
+ my $opt_depth = 0;
+ if( user_opt_in_enabled($self) ){
+ my $orgid = check_user_perms3($self, $client, $auth,
$e->requestor->id, "patron.opt_in");
>>>>>>>>
So, the staffs "patron.opt_in" permission depth defines which foreign
libraries they can search for patrons, even though the patrons have
opted in to a local library? (see the section of the patch starting at
@@ -576,14 +577,17 @@)
<<<<<<<<
+ my $org = $e->retrieve_actor_org_unit($orgid) or return $e->event;
+ my $ou_type = $e->retrieve_actor_org_unit_type($org->ou_type)
or return $e->event;
+ $opt_depth = $ou_type->depth || 0;
+ }
+
return $U->storagereq(
"open-ils.storage.actor.user.crazy_search", $search_hash,
- $search_limit, $search_sort, $include_inactive,
$e->requestor->ws_ou, $search_depth);
+ $search_limit, $search_sort, $include_inactive,
$e->requestor->ws_ou, $search_ou, $opt_depth);
}
@@ -1286,7 +1295,6 @@
return $e->event unless $e->allowed('VIEW_PERMISSION', $user->home_ou);
return $U->find_highest_perm_org($perm, $user_id,
$user->home_ou, $tree );
}
-
return $U->find_highest_perm_org($perm, $user_id,
$e->requestor->ws_ou, $tree);
}
@@ -2898,8 +2906,12 @@
# user is automatically opted-in at the home org
return 1 if $user->home_ou eq $org_id;
+ return 1 if($e->allowed('patron.opt_in', $user->home_ou));
+
+ my @orgunits = $U->get_org_ancestors($org_id);
+
my $vals = $e->search_actor_usr_org_unit_opt_in(
- {org_unit=>$org_id, usr=>$user_id},{idlist=>1});
+ {org_unit=>@orgunits, usr=>$user_id},{idlist=>1});
>>>>>>>>
I think you want that to be: {org_unit=>\@orgunits, usr=>$user_id},{idlist=>1});
Note that \@orgunits is an arrayref instead of a bare array.
<<<<<<<<
return 1 if @$vals;
return 0;
@@ -2914,15 +2926,17 @@
@return The ID of the newly created object, event on error./);
sub create_user_opt_in_at_org {
- my($self, $conn, $auth, $user_id) = @_;
+ my($self, $conn, $auth, $user_id, $org_id) = @_;
my $e = new_editor(authtoken => $auth, xact=>1);
return $e->die_event unless $e->checkauth;
- my $org_id = $e->requestor->ws_ou;
my $user = $e->retrieve_actor_user($user_id) or return $e->die_event;
- return $e->die_event unless $e->allowed('UPDATE_USER', $user->home_ou);
+ return $e->die_event unless $e->allowed('patron.opt_in',
$e->requestor->ws_ou);
+ $org_id = check_user_perms3($self, $conn, $auth, $e->requestor->id,
'patron.opt_in') unless $org_id;
+ $org_id = $e->requestor->ws_ou unless ($org_id >= 0);
>>>>>>>
Here it looks like you're using the permission of the staff member to
decided how widely the user is allowed to opt in. It seems like that
should be an org unit setting that chooses a depth, and you find the
ou at that depth relative to the ws_ou, no?
In fact, I have a feeling that the "patron.opt_in" permission could be
replaced with an org setting (patron.opt_in_depth?) altoghter ... Am I
missing something big? More on that at the end.
<<<<<<<
+
my $opt_in = Fieldmapper::actor::usr_org_unit_opt_in->new;
$opt_in->org_unit($org_id);
Index: Open-ILS/src/perlmods/OpenILS/Application/Storage/Publisher/actor.pm
===================================================================
--- Open-ILS/src/perlmods/OpenILS/Application/Storage/Publisher/actor.pm (revision
13335)
+++ Open-ILS/src/perlmods/OpenILS/Application/Storage/Publisher/actor.pm (working
copy)
@@ -472,7 +472,8 @@
my $sort = shift;
my $inactive = shift;
my $ws_ou = shift;
- my $ws_ou_depth = shift || 0;
+ my $search_ou = shift || $ws_ou;
+ my $opt_depth = shift || 0;
my $strict_opt_in =
OpenSRF::Utils::SettingsClient->new->config_value( share => user =>
'opt_in' );
@@ -576,14 +577,17 @@
$ws_ou = actor::org_unit->search( { parent_ou => undef } )->next->id;
}
- my $opt_in_join = '';
my $opt_in_where = '';
if (lc($strict_opt_in) eq 'true') {
- $opt_in_join = "LEFT JOIN $opt_in_table oi ON (oi.org_unit = $ws_ou
AND users.id = oi.usr)";
- $opt_in_where = "AND (oi.id IS NOT NULL OR users.home_ou = $ws_ou)";
+ $opt_in_where = "AND (";
+ $opt_in_where .= "EXISTS (select id FROM $opt_in_table ";
+ $opt_in_where .= " WHERE org_unit in (select
(actor.org_unit_ancestors($ws_ou)).id)";
+ $opt_in_where .= " AND usr = users.id) ";
+ $opt_in_where .= "OR";
+ $opt_in_where .= " users.home_ou IN (select
(actor.org_unit_descendants($ws_ou,$opt_depth)).id))";
}
- my $descendants = "actor.org_unit_descendants($ws_ou, $ws_ou_depth)";
+ my $descendants = "actor.org_unit_descendants($search_ou)";
$select = "JOIN ($select) AS search ON (search.id = users.id)" if ($select);
$select = <<" SQL";
@@ -591,7 +595,6 @@
FROM $u_table AS users $card
JOIN $descendants d ON (d.id = users.home_ou)
$select
- $opt_in_join
$clone_select
WHERE users.deleted = FALSE
$inactive
Index: Open-ILS/xul/staff_client/server/patron/search_form.js
===================================================================
--- Open-ILS/xul/staff_client/server/patron/search_form.js (revision 13335)
+++ Open-ILS/xul/staff_client/server/patron/search_form.js (working copy)
@@ -251,9 +251,9 @@
JSAN.use('util.file'); JSAN.use('util.widgets');
JSAN.use('util.functional');
util.widgets.remove_children(obj.controller.view.search_depth);
var ml = util.widgets.make_menulist(
- util.functional.map_list( obj.OpenILS.data.list.aout,
+ util.functional.map_list( obj.OpenILS.data.list.my_aou,
>>>>>>>
I haven't tested this yet, but does this display ancestors as well as
descendants (or vice versa_? Just want to be sure -- I don't recall
if my_aou is grabbing full_path or not.
<<<<<<<
function(el,idx) {
- return [ el.opac_label(), el.depth() ]
+ return [ findOrgType(el.ou_type()).opac_label(), el.id()]
}
).sort(
function(a,b) {
---------------------------------------------------------------------------------------
Overall, it strikes me as more logical to tie this to the organization
instead of the staff groups -- the setting is an organizational policy
instead of a staff action, really. Using permissions also seems like
it could be prone to human error, perhaps giving different perm depths
to different staff groups and the like. I think the following
captures what you need, and at the same time avoids some of the
fragility of the permission-based approach.
* create a new inheritable org unit setting called
patron.opt_in_depth, which is set to an actor.org_unit_type.depth
value. Because this can be set at a specific org, you can use it in a
varied, complex org structure, and have a default (or for simpler
structures, like the canonical 3-tier, consortium/system/branch, the
only) setting at the consortium level (set to, say, System, as I think
would be your case) [UPDATE: side-conversation in IRC brought up
security. In 1.4 and beyond, org settings each have their own magical
permission check. Add a permission called
UPDATE_ORG_UNIT_SETTING.$name (where name is the org setting) and give
that to users that should be allowed to adjust it.]
* when opting a user in, instead of creating an opt-in for $ws_ou,
set it for actor.org_unit_anscestor_at_depth($ws_ou,
$opt_in_org_depth)
* change the search SQL to use
actor.org_unit_full_path($ws_ou,$opt_in_org_depth) instead of just
ancestors.
Thoughts?
--
Mike Rylander
| VP, Research and Design
| Equinox Software, Inc. / The Evergreen Experts
| phone: 1-877-OPEN-ILS (673-6457)
| email: miker at esilibrary.com
| web: http://www.esilibrary.com
More information about the Open-ils-dev
mailing list