Bug 7919 - Display of values depending on the connexion library (authorised values, patron categories, extended attributes)
Summary: Display of values depending on the connexion library (authorised values, patr...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: 3.12
Hardware: All All
: P1 - high new feature (vote)
Assignee: Jonathan Druart
QA Contact: Paul Poulain
URL:
Keywords:
Depends on:
Blocks: 9076
  Show dependency treegraph
 
Reported: 2012-04-06 08:43 UTC by delaye
Modified: 2014-05-26 21:04 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7919 : Display of values depending on the connexion librairy (43.72 KB, patch)
2012-04-06 10:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919 :Bug 7919 : Display of values depending on the connexion librairy Display of values depending on the connexion librairy (43.78 KB, patch)
2012-04-13 09:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919 : Display of values depending on the connexion librairy Display of values depending on the connexion librairy (43.73 KB, patch)
2012-06-07 11:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919 : Display of values depending on the connexion librairy (43.78 KB, patch)
2012-06-08 11:51 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 8272 - OPAC : date expiration format (1.72 KB, patch)
2012-06-22 08:25 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7919: Followup: using LEFT JOIN instead of JOIN for 2 files (3.29 KB, patch)
2012-07-27 12:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919: Followup: Add a column "Branches limitations" for the 3 pages (9.00 KB, patch)
2012-07-27 14:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919 : Display of values depending on the connexion library (50.85 KB, patch)
2012-08-06 08:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919: Followup FIX several behaviours (5.60 KB, patch)
2012-08-22 14:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919 : Display of values depending on the connexion library (54.32 KB, patch)
2012-11-28 15:37 UTC, Paul Poulain
Details | Diff | Splinter Review
[Signed off] Bug 7919 : Display of values depending on the connexion library (54.39 KB, patch)
2012-12-03 23:02 UTC, Melia Meggs
Details | Diff | Splinter Review
Bug 7919 : Display of values depending on the connexion library (54.37 KB, patch)
2012-12-04 09:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919: Followup: Fix plural and GetAuthvalueDropbox (5.01 KB, patch)
2012-12-04 09:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed off] Bug 7919: Followup: Fix plural and GetAuthvalueDropbox (5.04 KB, patch)
2012-12-04 23:32 UTC, Melia Meggs
Details | Diff | Splinter Review
Bug 7919: Fix unit test (Members_AttributeTypes.t) (1.68 KB, patch)
2012-12-06 09:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919: Fix unit test (Members_AttributeTypes.t) (1.74 KB, patch)
2012-12-07 22:58 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7919: Fix unit test (Members_AttributeTypes.t) [Signed-Off] [Passed QA] - Reintroduce the class column in a select statement (was present before this development) - FIX UT: If there are no arg passed in parameter, the routine calls C4::Context->useren (1.96 KB, patch)
2012-12-07 23:06 UTC, Elliott Davis
Details | Diff | Splinter Review
Bug 7919: FIX the "all" categories method (1.08 KB, patch)
2012-12-17 10:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919: FIX the "all" categories method (1.05 KB, patch)
2012-12-17 10:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919: FIX the "all" categories method (1.11 KB, patch)
2012-12-18 08:46 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7919: FIX the "all" categories method (1.73 KB, patch)
2012-12-27 10:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7919: FIX the "all" categories method (1.79 KB, patch)
2013-01-01 22:10 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7919: FIX the "all" categories method (1.91 KB, patch)
2013-01-02 21:02 UTC, Liz Rea
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7919: FIX the "all" categories method (2.16 KB, patch)
2013-01-02 21:40 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description delaye 2012-04-06 08:43:07 UTC
From Koha administration:

Patron categories
.. / cgi-bin / koha / admin / categorie.pl? & op = add_form CategoryCode = [^]

and
Authorized values
/ cgi-bin/koha/admin/authorised_values.pl? op = & id = add_form

and
Patron attribute types
gi-bin/koha/admin/patron-attr-types.pl? edit_attribute_type = op & code = [^]

and
Item types
/ cgi-bin/koha/admin/itemtypes.pl? op = add_form & itemtype =


link each code to a branchcode list.
Then the administrator can choose to link a code (category, attribute, value allowed) to a specific branchcode

Interest: each librarie  will see that its codes. Each librarie will be able to secure the  access and the use of its codes in a network.

The filter should apply to the following screens:

MEMBERS
.. / cgi-bin / koha / members / members-home.pl [^]
.. / cgi-bin / koha / members / memberentry.pl? op = add & CategoryCode = [^]
.. / cgi-bin / koha / members / memberentry.pl? op = add & CategoryCode = [^]

PERIODICAL
... / cgi-bin / koha / serials / subscription-add.pl? op = & mod = SubscriptionId ...

ACQUISITIONS
... / cgi-bin / koha / admin / aqbudgets.pl? & op = add_form budget_parent_id = 131 & budget_period_id = ..
... / cgi-bin / koha / acquisition / neworderempty.pl? OrderNumber = 26 & booksellerid = 2 & basketno = ...

TOOLS
.. / cgi-bin / koha / tools / modborrowers.pl
SORT1, SORT2, borrower_attribute, CategoryCode, branchcode

... / cgi-bin / koha / tools / batchMod.p
==> Items.location
==> Items.notforloan
==> items.ccode

CATALOGUING
/ cgi-bin/koha/cataloguing/additem.pl? biblionumber =
/ cgi-bin/koha/cataloguing/addbiblio.pl? biblionumber = 37202 & op =

OPAC
.. / cgi-bin / koha / opac-search.pl
Comment 1 Jonathan Druart 2012-04-06 10:18:04 UTC Comment hidden (obsolete)
Comment 2 delaye 2012-04-10 09:09:02 UTC
i have this error message when i go in ..cgi-bin/koha/members/members-home.pl

Software error:

Can't use an undefined value as an ARRAY reference at /home/koha/src/C4/Category.pm line 86.
Comment 3 Jonathan Druart 2012-04-13 09:12:02 UTC Comment hidden (obsolete)
Comment 4 delaye 2012-05-07 12:50:55 UTC
I do not have the 3 tables(categories_branches,authorised_values_branches,borrower_attribute_types_branches) in my database after apply the patch in a  sandbox ; so I can not test.
Can you give me the script mysql  to add the tables in the database?
Comment 5 Jonathan Druart 2012-06-07 11:59:24 UTC Comment hidden (obsolete)
Comment 6 Paul Poulain 2012-06-08 11:51:10 UTC
Patch tested with a sandbox, by Delaye Stephane <stephane.delaye@biblibre.com>
Comment 7 Paul Poulain 2012-06-08 11:51:30 UTC Comment hidden (obsolete)
Comment 8 Ian Walls 2012-06-08 19:04:50 UTC
I'm all for having more granular control of system features on the branch level, but this patch seems to be doing it in a very ad-hoc, bolt-on way for only certain features.  There are lots of other things that would be good to control on a per-branch level, and I'd like to see clean, well thought out approach that can handle these use cases and many others with one approach.  While it's certainly more work to do it this way, it would lead to more uniform, clean and understandable code, with lower maintenance costs and better extensibility.
Comment 9 Jonathan Druart 2012-06-13 13:51:26 UTC
(In reply to comment #8)

Hi Ian,

Yes you are right, it is not very clean. But... it is Koha and I cannot find 10 days to rewrite completely an Authorised Values module.
I don't know how filter authorised values functions of the branch level without adding a join on each SELECT query.

The authorised value management is, for me, one of the most dirty part of Koha and it could be difficult to make something clean.

Likewise, I do not know how it is possible to filter elements simply (here AV, borrower attributes, categories, what else do you think about ?), with the current code of Koha.

> I'd like to see clean, well thought out approach that can handle these use cases and many others with one approach

How do you see this approach ?

Thanks for your review :)
Comment 10 Paul Poulain 2012-06-18 14:56:52 UTC
I agree with Jonathan's comment here : it is as clean as possible with the current code imo.

Rewritting it "from scratch" is another thing. Maybe, as we spoke of the Koha:: during the hackfest, that's something that could emerge here. But it's another topic.

I propose to Switch back to "signed off"
Comment 11 Paul Poulain 2012-06-22 08:25:12 UTC
Patch tested with a sandbox, by Delaye Stephane <stephane.delaye@biblibre.com>
Comment 12 Paul Poulain 2012-06-22 08:25:32 UTC Comment hidden (obsolete)
Comment 13 Paul Poulain 2012-07-26 13:02:15 UTC
Comment on attachment 9973 [details] [review]
Bug 7919 : Display of values depending on the connexion librairy Display of values depending on the connexion librairy

obsoleted by signed off patch
Comment 14 Paul Poulain 2012-07-26 14:19:23 UTC
Extensive testing this patch that has not been QAed yet, signed-off since 1 month and a half.

QA Comments
 1 Patch applies with a small conflict I've solved
 2 Usability comment: If there is a library limit on a given value, there's nothing specific displayed on lists in admin. It would be worth to have a column with applicable branches for each value -when there are-. Not worth failing QA though
 3 tested = updated a category only visible to librarians from library B. Being connected as librarian from library A, reaching members pages, and checked I can't see this category.
 4 tested = adding a limit to afr authorised_values LANG, connected to 101$a (language in UNIMARC). In addbiblio.pl, all the lists appear as empty !!! switching back to master it work well. Seems it's because of the SQL that is built, that returns nothing: SELECT id,authorised_value, lib FROM authorised_values RIGHT JOIN authorised_values_branches ON ( id = av_id AND ( branchcode = 'BDP' OR branchcode IS NULL ) ) WHERE category = 'LANG' GROUP BY lib ORDER BY lib, lib_opac;
 5 perlcritic, valid-templates, translatable-templates OK


Without point 4, this would have earned QA, but with point 4, it's failed QA, sorry
Comment 15 Jonathan Druart 2012-07-27 12:24:24 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2012-07-27 12:27:55 UTC
Please test last patch on pages cataloguing/addbiblio.pl and tools/batchMod.pl
Comment 17 Jonathan Druart 2012-07-27 14:42:56 UTC Comment hidden (obsolete)
Comment 18 Alexandre Lengereau 2012-08-06 07:22:38 UTC
Sandbox test result:

Something went wrong !
Applying: Bug 7919 : Display of values depending on the connexion librairy
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Budgets.pm
Auto-merging C4/Input.pm
Auto-merging C4/Koha.pm
Auto-merging C4/Members.pm
Auto-merging C4/Members/AttributeTypes.pm
Auto-merging C4/Members/Attributes.pm
Auto-merging admin/authorised_values.pl
Auto-merging admin/categorie.pl
CONFLICT (content): Merge conflict in admin/categorie.pl
Auto-merging admin/patron-attr-types.pl
Auto-merging cataloguing/addbiblio.pl
Auto-merging cataloguing/additem.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/authorised_values.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/categorie.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/categorie.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 7919 : Display of values depending on the connexion librairy
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 7919 - Display of values depending on the connexion library

Bug 7919 : Display of values depending on the connexion librairy
Apply? [yn]
Patch left in /tmp/Bug-7919--Display-of-values-depending-on-the-conne-hVli22.patch
Comment 19 Jonathan Druart 2012-08-06 08:17:07 UTC Comment hidden (obsolete)
Comment 20 Koha Team Lyon 3 2012-08-22 14:50:50 UTC
test with a sandbox :
a few points about this patch :
1. In tables patron category administration, authorized values,,patron attributes types, when appears "1 branches limitations" there's a link which point on the actual page : either it doesn't point on the good URL or it wasn't usefull.
2.In the table Patron attribute types, there's an inversion between column actions et column Branches limitations
3.In page members/memberentry.pl, there's no Drop-down menu for sort1 and sort2, so restrictions on Bsort1 and Bsort2 in authorized values can't be apply
4.In additem.pl, i try a restriction on LOST, it works but the value isn't saved
5.In acqui/neworderempty.pl, restrictions about localisation don't work
6.There's no Drop-down menu in tools/import_borrowers.pl 

Everything is fine in tools/batchMod.pl and with authorised_values LANG in page cataloguing/addbiblio.pl
Comment 21 Jonathan Druart 2012-08-22 14:59:02 UTC Comment hidden (obsolete)
Comment 22 Koha Team Lyon 3 2012-08-23 08:17:45 UTC
Hi Jonathan,
> 6.There's no Drop-down menu in tools/import_borrowers.pl 

Normaly you can filter on Patron Categories.
Now you have Sort1 and 2 too.

-> Can you just check with my sandbox, but for me, there's no drop-down menu to choose some value in this page http://pro.salers.biblibre.com/cgi-bin/koha/tools/import_borrowers.pl
But, I can miss somthing.

Thanks !!
Comment 23 Jonathan Druart 2012-08-23 08:45:05 UTC
(In reply to comment #22)
> -> Can you just check with my sandbox, but for me, there's no drop-down menu
> to choose some value in this page
> http://pro.salers.biblibre.com/cgi-bin/koha/tools/import_borrowers.pl
> But, I can miss somthing.

ho, sorry ! It is not import_borrowers.pl but tools/modborrowers.pl :-/
Comment 24 Koha Team Lyon 3 2012-08-23 10:05:36 UTC
For point 4 "In additem.pl, i try a restriction on LOST, it works but the value isn't saved", it's the same this another patch, it must be a problem of settings.

Ok for the page tools/modborrowers.pl

So everything is ok for me !
Comment 25 Jared Camins-Esakov 2012-10-06 04:18:34 UTC
Changing target version so this bug report will show up in the list of 3.12-targeted features and doesn't get lost in the shuffle during feature freeze.
Comment 26 Paul Poulain 2012-11-28 15:34:29 UTC
QA comment:

 * from comment 27 I understand that Koha Team Lyon 3 sign-off the patch, I add their signature
 * squashed the main patch and the follow-up
 * koha-qa.pl complain because of POD in CA/Members/Attributes.pm, but the problem was already here. I fixed it during QA
 * koha-qa.pl complain because of tabs in admin/categorie.pl, but they were already here before, not failing QA for that
 * koha-qa complain because of
		"my" variable $sth masks earlier declaration in same scope 
		"my" variable $dbh masks earlier declaration in same scope 
   in admin/categorie.pl, that's added byt this patch, I've fixed that too
 * checked that things worked, could not find anything wrong
 * If I had written it myself, I would have added a 'branch' column, with a '*' for default, and used the same rule as for circulation rules, but the way it's done is OK. It will just make caching harder to develop (but that's another topic)
 * in templates, I feel the span with explanations could be styled a little bit better, but won't fail QA for that, someone (Owen ?) will for sure improve the look if needed

Passed QA !
Comment 27 Paul Poulain 2012-11-28 15:37:35 UTC Comment hidden (obsolete)
Comment 28 Jared Camins-Esakov 2012-11-30 23:01:14 UTC
Could I please get another signoff on this patch? It touches some rather core functionality so I'd feel better if it had a little more testing, since I don't use the Acquisitions or Patrons modules very heavily in my day-to-day work.
Comment 29 Melia Meggs 2012-11-30 23:17:59 UTC
Hi,

I tried to test this patch, but I don't think it's working.

After applying the patch, I went to Patron Categories and choose to edit an existing patron category.  Under "Branch Limitation," I clicked on one branch name, then clicked save.  But when the Patron Category Administration page came back up, it still shows no branch limitation.

I then went to Patron Attribute Types and chose to edit an existing attribute.  The same thing happened.  After choosing a branch and clicking on save, the Patron Attribute Types page reloaded with a message: Modified patron attribute type "HBR" but it still says no branch limitation in the column below.

Next, I clicked on Patrons in the top menu bar and got this software error:

Can't use an undefined value as an ARRAY reference at /home/melia/kohaclone/C4/Category.pm line 89.
For help, please send mail to the webmaster (webmaster@ByWater-Test), giving this error message and the time and date of the error.

Hope that helps!

Thanks,

Melia
Comment 30 Paul Poulain 2012-12-03 09:08:01 UTC
(In reply to comment #29)
> Hi,
> 
> I tried to test this patch, but I don't think it's working.

Are you sure your database had been updated ? This patch add the following tables:
categories_branches
authorised_values_branches
borrower_attribute_types_branches
Comment 31 Jonathan Druart 2012-12-03 09:27:14 UTC
Hi Melia,
Thanks for testing.
I tested again and it works for me. Could you please check if the 3 queries of the updatedatabase (3 new tables: borrower_attribute_types_branches, categories_branches and authorised_values_branches) are correctly executed ?
When you save your changes, there is certainly an error in your Koha log, could you please copy it here ?
Comment 32 Melia Meggs 2012-12-03 23:01:35 UTC
Ok, thanks!  Updatedatabase was the problem.  I've run
./installer/data/mysql/updatedatabase.pl and things are looking better now.

Here is what I have done to test:
1. Modified two existing patron categories ("Board" and "Kid").  I limited "Board" to Centerville branch and "Kid" to Fairview branch.
2. Modified existing patron attributes ("Driver's License" and "Resident").  I limited "Driver's License" to Midway Branch and "Resident" to Centerville branch.
3. Created two new authorized values (one for the MARC 504 field, and a new collection code) and limited them both to Centerville branch.

Then I set the library as Centerville branch.
- I went to cataloging to add a new bib record and verified that I can see my new MARC 504 field authorized value in the dropdown menu there.
- I searched the catalog for an existing Centerville item to edit and verified that I can see my new collection code authorized value there.
- I added a new patron and verified that I can see "Board" as a possible patron category but cannot see "Kid" as a possible patron category
- I edited an existing Centerville patron record and verified that I can see "Resident" under Patron Attributes but cannot see "Driver's License"
- I went to Acquisitions and created a new order, but I'm not sure what to look at on this page, sorry...
- I went to Tools > Import Patrons, but I'm not sure what to look for on this page either.  I can see patron_attributes as a choice here, but it doesn't list the specific attributes, so I can't verify whether the one that is limited to this branch shows up or not.

Then I set the library as Midway branch.
- I went to cataloging to add a new bib record verified that I CANNOT see my new MARC 504 field authorized value there (correct - it was limited to Centerville branch).
- I searched the catalog for an existing Midway item to edit and verified that I CANNOT see my new collection code authorized value there (correct - it was limited to Centerville branch).
- I added a new patron and verified that I CANNOT see "Board" or "Kid" as possible patron categories (correct - "Board" is limited to Centerville branch, and "Kid" is limited to Fairview branch.)
- I edited an existing Midway patron record and verified that I CANNOT see "Resident" under Patron Attributes but can see "Driver's License" (correct - "Resident" is limited to Centerville, but "Driver's License" is limited to Midway)

For the pages I was able to test (cataloguing and members), this seems to be working well.  I will add my sign off, with the understanding that I did not test acqui/neworderempty.pl or tools/import_borrowers.pl.

Thanks,

Melia
Comment 33 Melia Meggs 2012-12-03 23:02:25 UTC Comment hidden (obsolete)
Comment 34 Melia Meggs 2012-12-03 23:05:31 UTC
Oh also, one tiny thing - if you choose to limit to just one branch, it shows up as "1 branches limitations," which should be "1 branch limitation."
Comment 35 Jonathan Druart 2012-12-04 09:24:11 UTC
(In reply to comment #32)
> For the pages I was able to test (cataloguing and members), this seems to be
> working well.  I will add my sign off, with the understanding that I did not
> test acqui/neworderempty.pl or tools/import_borrowers.pl.

It is not tools/import_borrowers.pl but tools/modborrowers.pl. I did not update the commit message, sorry.
In acqui/neworderempty you can test the statistics (1 and 2) values after selecting a fund (this one has to be linked to 1 or 2 authorized value(s)).

(In reply to comment #34)
> Oh also, one tiny thing - if you choose to limit to just one branch, it
> shows up as "1 branches limitations," which should be "1 branch limitation."

A patch is coming.
Comment 36 Jonathan Druart 2012-12-04 09:25:06 UTC Comment hidden (obsolete)
Comment 37 Jonathan Druart 2012-12-04 09:26:01 UTC
I updated the commit message in the last patch
Comment 38 Jonathan Druart 2012-12-04 09:26:14 UTC Comment hidden (obsolete)
Comment 39 Jonathan Druart 2012-12-04 09:28:32 UTC
The last patch needs a SO.

There was a bug in the neworderempty page, fixed.
Plurals in 3 pages, fixed.
Comment 40 Melia Meggs 2012-12-04 23:31:07 UTC
Thanks, Jonathan!  I have now tested tools/modborrowers.pl, and the patron categories on that page look good.  I also applied your follow up and tested acqui/neworderempty.pl using the Asort1 and Asort2 for Statistics.  This works perfectly as well.  

And thanks for the "1 branch limitation" correction.

I can now call this one completely signed off!
Comment 41 Melia Meggs 2012-12-04 23:32:09 UTC Comment hidden (obsolete)
Comment 42 Paul Poulain 2012-12-05 10:51:36 UTC
passed QA again, the follow-up is small, not a big deal from QA POV
Comment 43 Jared Camins-Esakov 2012-12-05 15:10:52 UTC
I will be pushing these patches shortly, since they work great, and add a useful feature. However, I'd like to suggest a follow-up. Right now the branch limitations are based on the staff user's branch, but it seems to me that it would be more helpful if the limitations were based on the branch of the object being edited.
Comment 44 Jared Camins-Esakov 2012-12-05 15:13:13 UTC
This patch has been pushed to master.
Comment 45 Jared Camins-Esakov 2012-12-05 15:25:41 UTC
(In reply to comment #43)
> I will be pushing these patches shortly, since they work great, and add a
> useful feature. However, I'd like to suggest a follow-up. Right now the
> branch limitations are based on the staff user's branch, but it seems to me
> that it would be more helpful if the limitations were based on the branch of
> the object being edited.

I've been told that comment was unclear. By "staff user's branch," I mean "branch currently set by the staff user," not "staff user's home branch."
Comment 46 Paul Poulain 2012-12-05 16:43:45 UTC
(In reply to comment #45)
> (In reply to comment #43)
> > I will be pushing these patches shortly, since they work great, and add a
> > useful feature. However, I'd like to suggest a follow-up. Right now the
> > branch limitations are based on the staff user's branch, but it seems to me
> > that it would be more helpful if the limitations were based on the branch of
> > the object being edited.
> 
> I've been told that comment was unclear. By "staff user's branch," I mean
> "branch currently set by the staff user," not "staff user's home branch."

Are you saying we should change the behaviour or add a syspref to let the library choose ? The library that sponsored this development wanted that way, and I think most of our libraries will be happy to have it that way.

OTOH, I think that, it's like the HomeOrHoldingBranch syspref, some libraries could be interested by the other behaviour (ie= AV depends on the object library, not staff user's branch. I'm not sure I see one of libary needing it this way, but why not.
(just to be clear: I don't thin we will do that in a predictable time, as none of our library is requesting it for now)
Comment 47 Jared Camins-Esakov 2012-12-05 16:53:25 UTC
(In reply to comment #46)
> (In reply to comment #45)
> > (In reply to comment #43)
> > > I will be pushing these patches shortly, since they work great, and add a
> > > useful feature. However, I'd like to suggest a follow-up. Right now the
> > > branch limitations are based on the staff user's branch, but it seems to me
> > > that it would be more helpful if the limitations were based on the branch of
> > > the object being edited.
> > 
> > I've been told that comment was unclear. By "staff user's branch," I mean
> > "branch currently set by the staff user," not "staff user's home branch."
> 
> Are you saying we should change the behaviour or add a syspref to let the
> library choose ? The library that sponsored this development wanted that
> way, and I think most of our libraries will be happy to have it that way.

Probably a syspref would be useful. As I said, this is not something to block the feature going in, just a potentially useful addition.

> OTOH, I think that, it's like the HomeOrHoldingBranch syspref, some
> libraries could be interested by the other behaviour (ie= AV depends on the
> object library, not staff user's branch. I'm not sure I see one of libary
> needing it this way, but why not.
> (just to be clear: I don't thin we will do that in a predictable time, as
> none of our library is requesting it for now)
Comment 48 Jared Camins-Esakov 2012-12-05 16:54:35 UTC
Jenkins caught a problem in t/Members_AttributeTypes.t that I missed (this will teach me to check my PERL5LIB environment variable!). Could I please get a follow-up that fixes this test?
Comment 49 Jonathan Druart 2012-12-06 09:28:02 UTC Comment hidden (obsolete)
Comment 50 Chris Cormack 2012-12-07 22:58:36 UTC Comment hidden (obsolete)
Comment 51 Elliott Davis 2012-12-07 23:06:03 UTC Comment hidden (obsolete)
Comment 52 Jared Camins-Esakov 2012-12-07 23:32:48 UTC
Thanks for the follow-up. Pushed to master.
Comment 53 Owen Leonard 2012-12-14 16:38:57 UTC
This has broken the "new patron" menu on members-home.pl. categorycodes are not being passed to the template, so although all the category descriptions are listed, each link is the same:

/cgi-bin/koha/members/memberentry.pl?op=add&amp;categorycode=
Comment 54 Owen Leonard 2012-12-14 17:18:13 UTC
Display of patron categories in patron search results (member.pl) is now broken as well.
Comment 55 Liz Rea 2012-12-17 03:00:08 UTC
I confirm Owen's two bugs.
Comment 56 Jonathan Druart 2012-12-17 10:14:32 UTC Comment hidden (obsolete)
Comment 57 Jonathan Druart 2012-12-17 10:15:34 UTC Comment hidden (obsolete)
Comment 58 Chris Cormack 2012-12-18 08:46:05 UTC Comment hidden (obsolete)
Comment 59 Jonathan Druart 2012-12-27 10:03:32 UTC Comment hidden (obsolete)
Comment 60 Jonathan Druart 2012-12-27 10:04:53 UTC
I found another place where the problem exists.

I increase the priority to critical
Comment 61 Chris Cormack 2013-01-01 22:10:06 UTC Comment hidden (obsolete)
Comment 62 Liz Rea 2013-01-02 21:02:15 UTC Comment hidden (obsolete)
Comment 63 Katrin Fischer 2013-01-02 21:40:56 UTC
Created attachment 14377 [details] [review]
[SIGNED-OFF] Bug 7919: FIX the "all" categories method

The C4::Category->all method must return fields from the categories
table only.
Without this patch, there is 2 "categorycode" values, the second one is
sometimes undef (if no branch limitation).

same for the GetBorrowercategoryList routine (some category codes are
undefined)

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Signed off for also fixing problems with patron searching noted by Katrin.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Problems fixed by this patch include:
- on patron search result list the category description is shown again
- it's possible to limit a patron search to a patron category again

All tests pass.
Comment 64 Jared Camins-Esakov 2013-01-02 21:51:47 UTC
This patch has been pushed to master.
Comment 65 Chris Cormack 2013-01-12 19:00:05 UTC
Doesn't apply for 3.10.x but I don't think it is needed, if it is please submit a patch formatted for 3.10.x