Bug 10276 - Extend IndependentBranches to support groups of libraries
Summary: Extend IndependentBranches to support groups of libraries
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 10277
Blocks: 10278
  Show dependency treegraph
 
Reported: 2013-05-17 14:59 UTC by Kyle M Hall
Modified: 2017-06-14 22:10 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Large patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10276 - Extend IndependentBranches to support groups of libraries (57.90 KB, patch)
2013-05-17 15:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (57.70 KB, patch)
2013-05-29 18:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (57.68 KB, patch)
2013-05-30 14:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (57.65 KB, patch)
2013-06-06 18:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (57.73 KB, patch)
2013-06-06 19:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (57.78 KB, patch)
2013-06-11 15:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (58.93 KB, patch)
2013-09-04 11:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (58.93 KB, patch)
2013-09-17 16:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (57.38 KB, patch)
2013-11-07 17:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (57.43 KB, patch)
2013-11-19 15:49 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (57.92 KB, patch)
2013-12-12 14:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (58.16 KB, patch)
2013-12-12 16:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Extend IndependetBranches to support groups of libraries (58.16 KB, patch)
2013-12-18 16:43 UTC, Melissa Lefebvre
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (58.43 KB, patch)
2013-12-18 17:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (58.67 KB, patch)
2014-01-06 13:47 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (65.70 KB, patch)
2014-01-07 19:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (65.79 KB, patch)
2014-01-15 20:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276i [QA Followup] - Extend IndependentBranches to support groups of libraries (2.44 KB, patch)
2014-01-15 21:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (65.81 KB, patch)
2014-02-28 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276i [QA Followup] - Extend IndependentBranches to support groups of libraries (2.44 KB, patch)
2014-02-28 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276i [QA Followup 2] - Remove stringify, use placeholders for db queries (19.94 KB, patch)
2014-02-28 15:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 3] - Fix inability to change patron's branch (2.27 KB, patch)
2014-03-10 15:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10276 - Extend IndependentBranches to support groups of libraries (65.86 KB, patch)
2014-03-10 16:22 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10276i [QA Followup] - Extend IndependentBranches to support groups of libraries (2.50 KB, patch)
2014-03-10 16:22 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10276i [QA Followup 2] - Remove stringify, use placeholders for db queries (20.01 KB, patch)
2014-03-10 16:23 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10276 [QA Followup 3] - Fix inability to change patron's branch (2.33 KB, patch)
2014-03-10 16:23 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (66.15 KB, patch)
2014-05-20 13:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276i [QA Followup] - Extend IndependentBranches to support groups of libraries (2.49 KB, patch)
2014-05-20 13:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276i [QA Followup 2] - Remove stringify, use placeholders for db queries (19.98 KB, patch)
2014-05-20 13:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 3] - Fix inability to change patron's branch (2.32 KB, patch)
2014-05-20 13:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (66.15 KB, patch)
2014-05-20 13:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup] - Extend IndependentBranches to support groups of libraries (2.49 KB, patch)
2014-05-20 13:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 2] - Remove stringify, use placeholders for db queries (19.98 KB, patch)
2014-05-20 13:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 3] - Fix inability to change patron's branch (2.32 KB, patch)
2014-05-20 13:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 3] - Fix edit links on catalogue/detail.pl (3.28 KB, patch)
2014-05-20 13:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 4] - Fix edit links on catalogue/detail.pl (3.28 KB, patch)
2014-05-20 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 4] - Fix edit links on catalogue/detail.pl (3.32 KB, patch)
2014-05-20 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (66.90 KB, patch)
2015-02-19 12:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (66.90 KB, patch)
2015-02-19 12:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup] - Extend IndependentBranches to support groups of libraries (2.49 KB, patch)
2015-02-19 12:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 2] - Remove stringify, use placeholders for db queries (19.94 KB, patch)
2015-02-19 12:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 3] - Fix inability to change patron's branch (2.35 KB, patch)
2015-02-19 12:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 4] - Fix edit links on catalogue/detail.pl (3.27 KB, patch)
2015-02-19 12:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (64.40 KB, patch)
2015-07-17 12:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (64.40 KB, patch)
2015-07-17 12:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 - Extend IndependentBranches to support groups of libraries (64.40 KB, patch)
2015-07-17 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup] - Extend IndependentBranches to support groups of libraries (2.49 KB, patch)
2015-07-17 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 2] - Remove stringify, use placeholders for db queries (19.94 KB, patch)
2015-07-17 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 3] - Fix inability to change patron's branch (2.35 KB, patch)
2015-07-17 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10276 [QA Followup 4] - Fix edit links on catalogue/detail.pl (3.26 KB, patch)
2015-07-17 12:43 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-05-17 14:59:14 UTC
Imagine a library consortium consisting of multiple library systems. This feature would allow a consortium to group libraries by system such that those systems could work independently from one another on a single installation.
Comment 1 Kyle M Hall 2013-05-17 15:00:21 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-05-29 18:31:03 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-05-30 14:06:13 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2013-06-06 18:13:10 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2013-06-06 19:18:01 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2013-06-11 15:03:22 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2013-09-04 11:14:21 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2013-09-17 16:58:10 UTC Comment hidden (obsolete)
Comment 9 I'm just a bot 2013-09-18 11:18:16 UTC
Applying: Bug 10276 - Extend IndependentBranches to support groups of libraries
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 10276 - Extend IndependentBranches to support groups of libraries
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 10 Kyle M Hall 2013-11-07 17:40:57 UTC Comment hidden (obsolete)
Comment 11 sandboxes@biblibre.com 2013-11-19 15:49:15 UTC
Patch tested with a sandbox, by Jerry Brock <elibristech@gmail.com>
Comment 12 sandboxes@biblibre.com 2013-11-19 15:49:36 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2013-12-12 14:12:38 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-12-12 16:33:06 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2013-12-13 12:11:56 UTC
Kyle,
Why did you submit new patches when the patch is signed off?
Why don't you submit followups?
What is the diff between the SO patch and the last one?
Comment 16 Jonathan Druart 2013-12-13 12:13:04 UTC
Maybe it is just a rebase...
Comment 17 Kyle M Hall 2013-12-18 15:02:05 UTC
(In reply to Jonathan Druart from comment #16)
> Maybe it is just a rebase...

Yep, I had to rebase this patch due to additions to bug 10277, which this one depends on.
Comment 18 Joel Sasse 2013-12-18 16:37:59 UTC
I tested this and want to sign off on this patch.

Joel Sasse
Comment 19 Melissa Lefebvre 2013-12-18 16:43:18 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2013-12-18 17:09:59 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2013-12-29 14:46:37 UTC
Patch currently doesn't apply. I tried applying it directly on master and on top of bug 10277:

Signed-off-by:  Joel Sasse <jsasse@plumcreeklibrary.net>
--------------------------
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 10276 - Extend IndependentBranches to support groups of libraries
fatal: sha1 information is lacking or useless (C4/Items.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 10276 - Extend IndependentBranches to support groups of libraries
The copy of the patch that failed is found in:
   /home/katrin/kohaclone/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 22 Katrin Fischer 2013-12-29 15:29:56 UTC
Comment on attachment 23641 [details] [review]
Bug 10276 - Extend IndependentBranches to support groups of libraries

Review of attachment 23641 [details] [review]:
-----------------------------------------------------------------

Patch doesn't apply, but doing a quick code review:

1) Commit message could be updated, dependencies noted are resolved and example is slightly wrong (should be group 1 and 3).

2) Patch touches a lot of routines in different modules, but none of this is backed up by unit tests.
   This is a big feature with a lot of possible consequences if we break something, I think unit tests
   are a requirement, at least for the new routine. (UT)

3) Patch has bug 10277 notes as dependency, but is not using the new function consequently (C4::Context->IsSuperLibrarian())

4) Patch seems to remove 'properties' from the interface. Currently libraries can add properties library groups.
   I think we need to discuss this before removing the feature. Also we need to think about databases with property groups
   defined and how to migrate them. Or keep the option. (properties)

::: C4/Branch.pm
@@ +43,4 @@
>  		&GetBranchCategories
>  		&GetBranchesInCategory
>  		&ModBranchCategoryInfo
> +        &GetIndependentGroupModificationRights

UT

@@ +376,4 @@
>  
>  	#TODO  manage category types.  rename possibly to 'agency domains' ? as borrowergroups are called categories.
>  sub GetCategoryTypes {
> + return ( 'searchdomain','independent_groups');

properties

@@ +446,5 @@
> +
> +    If 'branch' is not provided, it will be looked up via
> +    C4::Context->userenv->{branch}.
> +
> +    If 'for' is provided, the lookup is limited to that branch.

Could you explain the behaviour/use of 'for' a bit more?

@@ +452,5 @@
> +    If called in a list context, returns a list of
> +    branchcodes ( including $this_branch ).
> +
> +    If called in a scalar context, it returns
> +    a count of matching branchcodes. Returns 1 if

...? :)

::: C4/Serials.pm
@@ +238,5 @@
> +               subscription.subscriptionid as subsid
> +    |;
> +    if (   C4::Context->preference('IndependentBranches')
> +        && C4::Context->userenv
> +        && C4::Context->userenv->{'flags'} % 2 != 1

C4::Context->IsSuperLibrarian()

@@ +355,5 @@
> +    |;
> +
> +    if (   C4::Context->preference('IndependentBranches')
> +        && C4::Context->userenv
> +        && C4::Context->userenv->{'flags'} % 2 != 1

C4::Context->IsSuperLibrarian()

@@ +413,5 @@
> +    |;
> +
> +    if (   C4::Context->preference('IndependentBranches')
> +        && C4::Context->userenv
> +        && C4::Context->userenv->{'flags'} % 2 != 1

C4::Context->IsSuperLibrarian()

@@ +594,5 @@
> +    |;
> +
> +    if (   C4::Context->preference('IndependentBranches')
> +        && C4::Context->userenv
> +        && C4::Context->userenv->{'flags'} != 1

C4::Context->IsSuperLibrarian()

::: catalogue/moredetail.pl
@@ +177,3 @@
>          my $userenv = C4::Context->userenv();
> +        unless (
> +            $userenv->{'flags'} % 2 != 1

C4::Context->IsSuperLibrarian()

::: cataloguing/additem.pl
@@ +702,4 @@
>              #verifying rights
>              my $userenv = C4::Context->userenv();
> +            unless (
> +                $userenv->{'flags'} % 2 == 1

C4::Context->IsSuperLibrarian()f

::: circ/circulation-home.pl
@@ +39,5 @@
>  $template->param( fast_cataloging => 1 ) if (defined $fa);
>  
>  # Checking if the transfer page needs to be displayed
> +$template->param( display_transfer => 1 )
> +  if ( $flags->{'superlibrarian'} == 1

C4::Context->IsSuperLibrarian()

::: installer/data/mysql/kohastructure.sql
@@ +365,4 @@
>    `categorycode` varchar(10) NOT NULL default '', -- unique identifier for the library/branch group
>    `categoryname` varchar(32), -- name of the library/branch group
>    `codedescription` mediumtext, -- longer description of the library/branch group
> +  `categorytype` ENUM(  'searchdomain',  'independent_group' ) NULL DEFAULT NULL, -- says whether this is a search group or an independent group

properties
Comment 23 Kyle M Hall 2014-01-06 13:47:58 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2014-01-07 19:11:30 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2014-01-12 22:47:12 UTC
Hi Kyle,

there is a conflict in Branches.pm. I tried to resolve it, but started wondering about @bind_parameters. You remove the push line, so it never gets assigned, but is used later on. Also I think we should use ? instead of $branches.

     my $query="SELECT * FROM branches";
     my @bind_parameters;
     if ($onlymine && C4::Context->userenv && C4::Context->userenv->{branch}){
-      $query .= ' WHERE branchcode = ? ';
-      push @bind_parameters, C4::Context->userenv->{branch};
+      my $branches = GetIndependentGroupModificationRights({ stringify => 1 });
+      $query .= qq{ WHERE branchcode IN ( $branches ) };
     }
    $query .= " ORDER BY branchname";
    $sth = $dbh->prepare($query);
    $sth->execute(@bind_parameters);
Comment 26 Katrin Fischer 2014-01-12 22:53:26 UTC
Hi Kyle, I am a bit worried about the stringify because it will break escaping by dbi. I think it would be better using a list:

There is an example for that in our coding guidelines:
http://wiki.koha-community.org/wiki/Coding_Guidelines#SQL10:_Placeholders
Comment 27 Kyle M Hall 2014-01-15 20:40:00 UTC
(In reply to Katrin Fischer from comment #26)
> Hi Kyle, I am a bit worried about the stringify because it will break
> escaping by dbi. I think it would be better using a list:
> 
> There is an example for that in our coding guidelines:
> http://wiki.koha-community.org/wiki/Coding_Guidelines#SQL10:_Placeholders

Using placeholders would end up complicating every single query in an extreme manner. That practice of using placeholders is to prevent SQL injection attacks. That is not an issue here. An attack such as that is absolutely not possible in this case, since what we are turning into a string is a list of branchcodes that were just pulled from the database. Good question though!
Comment 28 Kyle M Hall 2014-01-15 20:50:25 UTC
Actually, there is one case where it's possible that the passed in branch parameter could be stringified directly. I will add a followup to escape that parameter.
Comment 29 Kyle M Hall 2014-01-15 20:56:09 UTC Comment hidden (obsolete)
Comment 30 Katrin Fischer 2014-01-15 21:12:00 UTC
Kyle, the attached patch introduces a lot of old super librarian checks. Please take a look. I just did a quick search for "flags".
Comment 31 Kyle M Hall 2014-01-15 21:14:20 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2014-01-15 21:19:32 UTC
Indeed it does! I'll add another followup for that.

(In reply to Katrin Fischer from comment #30)
> Kyle, the attached patch introduces a lot of old super librarian checks.
> Please take a look. I just did a quick search for "flags".
Comment 33 Chris Cormack 2014-01-15 22:42:20 UTC
(In reply to Kyle M Hall from comment #27)
> (In reply to Katrin Fischer from comment #26)
> > Hi Kyle, I am a bit worried about the stringify because it will break
> > escaping by dbi. I think it would be better using a list:
> > 
> > There is an example for that in our coding guidelines:
> > http://wiki.koha-community.org/wiki/Coding_Guidelines#SQL10:_Placeholders
> 
> Using placeholders would end up complicating every single query in an
> extreme manner. That practice of using placeholders is to prevent SQL
> injection attacks. That is not an issue here. An attack such as that is
> absolutely not possible in this case, since what we are turning into a
> string is a list of branchcodes that were just pulled from the database.
> Good question though!

It is absolutely possible, if someone, through XSS or CRSF or another means has managed to get sql into the db, this will then fetch and run it. The only time we should not use placeholders is in a case like

SELECT * FROM fish WHERE name="fish"; 

However, SQL injection is not the only reason, for using placeholders. They are for db independence, they will escape characters in the way that is safe for the rdbms running underneath. 

So we dont need to bother doing the escaping ourself, eg, what if branchcode had a character that would bust the query, 
GetIndependentGroupModificationRights is doing no escaping/sanitation, ie it is handing back what is in the db, with , '.
Comment 34 Galen Charlton 2014-01-15 23:02:11 UTC
(In reply to Chris Cormack from comment #33)
> So we dont need to bother doing the escaping ourself, eg, what if branchcode
> had a character that would bust the query, 
> GetIndependentGroupModificationRights is doing no escaping/sanitation, ie it
> is handing back what is in the db, with , '.

And $dbh->quote() is not the answer, either.

I am taking a hard line on this: I will never knowingly push patches that add violations of SQL10 (and certainly not a bunch of them in one fell swoop).  I see no upside for the short- and long-term health of the codebase to do so.

(In reply to Kyle M Hall from comment #27)
> Using placeholders would end up complicating every single query in an
> extreme manner.

No, it wouldn't.  There is an example of how to handle it in code you've written yourself, i.e., ModCourseInstructors().
Comment 35 Kyle M Hall 2014-02-28 15:39:03 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2014-02-28 15:39:25 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2014-02-28 15:39:53 UTC Comment hidden (obsolete)
Comment 38 M. Tompsett 2014-03-10 00:41:42 UTC
I want to make sure I understand this.
Let's say I enable patron self registration. And self registration puts them into BranchA. Let's say I mark BranchA as part of a group which also includes BranchB. Should not a patron with all staff permissions (not super librarian) in BranchB, be able to edit the patron which belongs to BranchA such that they are moved to BranchB?
Comment 39 Kyle M Hall 2014-03-10 15:02:37 UTC Comment hidden (obsolete)
Comment 40 M. Tompsett 2014-03-10 16:22:42 UTC Comment hidden (obsolete)
Comment 41 M. Tompsett 2014-03-10 16:22:57 UTC Comment hidden (obsolete)
Comment 42 M. Tompsett 2014-03-10 16:23:11 UTC Comment hidden (obsolete)
Comment 43 M. Tompsett 2014-03-10 16:23:26 UTC Comment hidden (obsolete)
Comment 44 M. Tompsett 2014-03-10 16:34:06 UTC
TEST PLAN (I used)
-------------------
1) Back up DB
2) Drop the koha database, create a blank one.
3) Apply the patch
4) Run the koha qa test tool: koha-qa.pl -v 2 -c 4
5) Do the web install from the staff client (everything less patrons and branches).
6) Run the test: prove -v t/db_dependent/Branch.t
7) Log into Koha staff client with system user
8) Create three branches: BRA, BRB, BRC
9) Create an independent group: AB
10) Make sure BRA and BRB are in AB, but BRC is not.
11) Create two users: staff with everything checked and not super librarian,
    and patron. Put them in BRB and BRA respectively.
12) Change IndependentBranches to 'Prevent' (turn it on).
12) Log out of system user
13) Log in as staff user.
14) Can staff user find both himself and the other patron?
    -- this should be true
15) view the other patron, and edit
    -- this should be allowed, since BRB and BRA are in the same group.
16) move the patron from BRA into BRB and click save.
    -- this should work.
17) view the other patron, and edit
18) This time, move the patron to BRC and click save.
    -- this should work. It's bad to do, but it should work.
19) Try to find the other patron.
    -- this should be possible now, because BRC is not in group.
Comment 45 M. Tompsett 2014-03-10 20:23:21 UTC
Oops... this should NOT be possible now, because BRC is not in group.
Comment 46 M. Tompsett 2014-04-11 14:46:05 UTC
Bug 11746 has been pushed to master, which is good. However, this means that the links displayed in the catalogue/details.pl page are displaying Edit on items which the user should not have edit links for.
Comment 47 Kyle M Hall 2014-05-20 13:04:06 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2014-05-20 13:04:20 UTC Comment hidden (obsolete)
Comment 49 Kyle M Hall 2014-05-20 13:04:23 UTC Comment hidden (obsolete)
Comment 50 Kyle M Hall 2014-05-20 13:04:26 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2014-05-20 13:05:25 UTC Comment hidden (obsolete)
Comment 52 Kyle M Hall 2014-05-20 13:05:40 UTC Comment hidden (obsolete)
Comment 53 Kyle M Hall 2014-05-20 13:05:43 UTC Comment hidden (obsolete)
Comment 54 Kyle M Hall 2014-05-20 13:05:46 UTC Comment hidden (obsolete)
Comment 55 Kyle M Hall 2014-05-20 13:38:22 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2014-05-20 13:42:19 UTC Comment hidden (obsolete)
Comment 57 Kyle M Hall 2014-05-20 13:56:50 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2014-06-26 14:30:12 UTC
Partial QA comments:

1/ There is a lack of unit tests here.
2/ I don't understand the interest of the stringify param, it's just a 1 line join.
3/ QA Followup 2 could be squashed with the main patch for the readability.
4/ Clone is a new dependency. But that could be avoid. I think something like
    my $clone = { %$hash_ref };
does the same think.
5/ How to be sure your changes are exhaustive? Everywhere IndependentBranches is used, that's it?

Marked as Failed QA.
Comment 59 Kyle M Hall 2014-07-25 12:07:54 UTC
> 1/ There is a lack of unit tests here.

I'll add some.

> 2/ I don't understand the interest of the stringify param, it's just a 1
> line join.

That's cruft from previous versions of the feature. I'll find and remove those.

> 3/ QA Followup 2 could be squashed with the main patch for the readability.

Will do!

> 4/ Clone is a new dependency. But that could be avoid. I think something like
>     my $clone = { %$hash_ref };
> does the same think.
I'll take a look at that.


> 5/ How to be sure your changes are exhaustive? Everywhere
> IndependentBranches is used, that's it?

Yep!
Comment 60 M. Tompsett 2014-07-28 20:39:42 UTC
Also, it fails to apply cleanly now. Make sure to rebase. :)
Comment 61 M. Tompsett 2014-09-18 13:52:36 UTC
Still needs a rebase and fix. :)
Comment 62 M. Tompsett 2015-02-09 06:52:20 UTC
Comment on attachment 28360 [details] [review]
Bug 10276 [QA Followup 2] - Remove stringify, use placeholders for db queries

Review of attachment 28360 [details] [review]:
-----------------------------------------------------------------

::: C4/Acquisition.pm
@@ +2414,5 @@
> +        my @branches = GetIndependentGroupModificationRights();
> +        $query .=
> +          " AND ( borrowers.branchcode = ? OR borrowers.branchcode IN ( "
> +          . join( ',', ('?') x @branches ) . " ) ) ";
> +        push( @query_params, @branches );

2416 has a ? in addition to the join'd ones on 2417. This leads to an off by one error. :)
Comment 63 Kyle M Hall 2015-02-19 12:26:53 UTC Comment hidden (obsolete)
Comment 64 Kyle M Hall 2015-02-19 12:27:19 UTC Comment hidden (obsolete)
Comment 65 Kyle M Hall 2015-02-19 12:27:30 UTC Comment hidden (obsolete)
Comment 66 Kyle M Hall 2015-02-19 12:27:34 UTC Comment hidden (obsolete)
Comment 67 Kyle M Hall 2015-02-19 12:27:37 UTC Comment hidden (obsolete)
Comment 68 Kyle M Hall 2015-02-19 12:27:41 UTC Comment hidden (obsolete)
Comment 69 M. Tompsett 2015-02-19 13:38:10 UTC
Comment on attachment 36030 [details] [review]
Bug 10276 [QA Followup 2] - Remove stringify, use placeholders for db queries

Review of attachment 36030 [details] [review]:
-----------------------------------------------------------------

::: C4/Acquisition.pm
@@ +2401,5 @@
> +        my @branches = GetIndependentGroupModificationRights();
> +        $query .=
> +          " AND ( borrowers.branchcode = ? OR borrowers.branchcode IN ( "
> +          . join( ',', ('?') x @branches ) . " ) ) ";
> +        push( @query_params, @branches );

There is @branches+1 ?'s, but only @branches parameters.
Comment 70 M. Tompsett 2015-02-19 16:35:32 UTC
For reason given in comment #69.
Comment 71 M. Tompsett 2015-04-25 14:47:41 UTC
Comment on attachment 36030 [details] [review]
Bug 10276 [QA Followup 2] - Remove stringify, use placeholders for db queries

Review of attachment 36030 [details] [review]:
-----------------------------------------------------------------

::: C4/Acquisition.pm
@@ +2401,5 @@
> +        my @branches = GetIndependentGroupModificationRights();
> +        $query .=
> +          " AND ( borrowers.branchcode = ? OR borrowers.branchcode IN ( "
> +          . join( ',', ('?') x @branches ) . " ) ) ";
> +        push( @query_params, @branches );

This is the only function which checks borrowers.branchcode. Is this correct?
Comment 72 M. Tompsett 2015-04-25 14:51:48 UTC
Since this is taking time to get signed off, perhaps convert the updatedatabase.pl to an atomicupdate, so this will need less tweaks when applying the patch.
Comment 73 M. Tompsett 2015-07-08 03:23:42 UTC
Please rebase and fix the issue raised in comment #71.
Comment 74 M. Tompsett 2015-07-08 03:47:18 UTC
(In reply to M. Tompsett from comment #73)
> Please rebase and fix the issue raised in comment #71.

And comment #69.
Comment 75 Kyle M Hall 2015-07-17 12:42:25 UTC Comment hidden (obsolete)
Comment 76 Kyle M Hall 2015-07-17 12:42:50 UTC Comment hidden (obsolete)
Comment 77 Kyle M Hall 2015-07-17 12:43:17 UTC
Created attachment 41066 [details] [review]
Bug 10276 - Extend IndependentBranches to support groups of libraries

This patch adds the ability to have independent library groups. To
enable this feature, enable the IndependentBranches system preference.
If no libraries are in any independent library groups,
IndependentBranches will behave as it always have. If a library is
part of an independent library group with one or more libraries, that
library will be able to access and modify patrons and items from any
library in that group.

Let's say we have 3 groups:
Group1:
  LibA
  LibB
  LibC
Group1:
  LibD
  LibE
  LibF
Group3:
  LibG
  LibH
  LibA

Note how LibA is in two groups ( Group1 and Group3 ). With this
configuration, if IndependentBranches is enabled, libraries will be able
to access and edit patrons and items in the following configuration:
LibA => LibB, LibC, LibG, LibH
LibB => LibA, LibC
LibC => LibA, LibB
LibD => LibE, LibF
LibF => LibD, LibE
LibG => LibH, LibA
LibH => LibG, LibA

Furthermore, let us assume there is a library LibI, which does not
belong to any group. That library will only be able to view and edit
patrons and items from it's own library.

Imagine a library consortium consisting of multiple library systems.
This feature would allow a consortium to group libraries by system such
that those systems could work independently from one another on a single
installation.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Enable IndependentBranches
4) Test independent branches, no changes should be noted
5) Navigate to admin/branches.pl
6) Create a new Independent library group
7) Add your library and some other libraries to that group
8) Re-test IndependentBranches, everything should work as previously,
   but instead of being limited to just your library, you should have
   access to everything within your library group.
   Example: Try to edit an item. If the item is owned by a library in
            your group, you should be able to edit it.

Signed-off-by:  Joel Sasse <jsasse@plumcreeklibrary.net>
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 78 Kyle M Hall 2015-07-17 12:43:25 UTC
Created attachment 41067 [details] [review]
Bug 10276 [QA Followup] - Extend IndependentBranches to support groups of libraries

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 79 Kyle M Hall 2015-07-17 12:43:29 UTC
Created attachment 41068 [details] [review]
Bug 10276 [QA Followup 2] - Remove stringify, use placeholders for db queries

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 80 Kyle M Hall 2015-07-17 12:43:33 UTC
Created attachment 41069 [details] [review]
Bug 10276 [QA Followup 3] - Fix inability to change patron's branch

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 81 Kyle M Hall 2015-07-17 12:43:37 UTC
Created attachment 41070 [details] [review]
Bug 10276 [QA Followup 4] - Fix edit links on catalogue/detail.pl

Bug 11746 has been pushed to master, which is good. However, this means
that the links displayed in the catalogue/details.pl page are displaying
Edit on items which the user should not have edit links for.
Comment 82 Jonathan Druart 2015-08-05 14:38:34 UTC
Comment on attachment 41066 [details] [review]
Bug 10276 - Extend IndependentBranches to support groups of libraries

Review of attachment 41066 [details] [review]:
-----------------------------------------------------------------

::: C4/Acquisition.pm
@@ +1912,5 @@
> +C<@results> is sorted alphabetically by book title.
> +
> +=cut
> +
> +sub GetParcel {

Has been removed by Bug 13318