Bug 8215

Applying: Course Reserves
/home/koha/sites/koha_hackfest/.git/rebase-apply/patch:200: trailing whitespace.
      
/home/koha/sites/koha_hackfest/.git/rebase-apply/patch:1005: trailing whitespace.
  
/home/koha/sites/koha_hackfest/.git/rebase-apply/patch:1024: trailing whitespace.
    
/home/koha/sites/koha_hackfest/.git/rebase-apply/patch:1188: trailing whitespace.
    
/home/koha/sites/koha_hackfest/.git/rebase-apply/patch:1193: trailing whitespace.
    
error: patch failed: installer/data/mysql/updatedatabase.pl:5343
error: installer/data/mysql/updatedatabase.pl: patch does not apply
Patch failed at 0001 Course Reserves

This is very nice!
There are two things i discovered:

1. Instructor Search does not work

2. Not possible to add more than one course :)

I tried this patch with a standard install and got hung up on:

1) I think you are missing some of the code on the staff interface side of things. The link on the main menu pointed to cgi-bin/koha/course_reserves/course.pl  which did not exist.  It might have worked for you and Marijana in the past in a dev install, but you need some more install code for those parts. It looked ok on the opac side, so reference that.

2) Then i went to run updatedatabase and got
 Use of bare << to mean <<"" is deprecated at ./installer/data/mysql/updatedatabase.pl line 5318. I see <<<<<<< HEAD in updatedatabase.pl 
Folks said there must be a conflict inthe merge, but I didn't get prompted when I did a git am -i -u -3 <yourpatch>

So if I can get the tables created, it can be tested.

Please try this new edition of the patch. It is a rebase from my original patch. I think the previous version got a bit messed up from rebase after rebase.

(In reply to comment #7)
> I tried this patch with a standard install and got hung up on:
> 
> 1) I think you are missing some of the code on the staff interface side of
> things. The link on the main menu pointed to
> cgi-bin/koha/course_reserves/course.pl  which did not exist.  It might have
> worked for you and Marijana in the past in a dev install, but you need some
> more install code for those parts. It looked ok on the opac side, so
> reference that.
> 
> 2) Then i went to run updatedatabase and got
>  Use of bare << to mean <<"" is deprecated at
> ./installer/data/mysql/updatedatabase.pl line 5318. I see <<<<<<< HEAD in
> updatedatabase.pl 
> Folks said there must be a conflict inthe merge, but I didn't get prompted
> when I did a git am -i -u -3 <yourpatch>
> 
> So if I can get the tables created, it can be tested.

I tried again with.  Bug 8215 - Add Course Reserves 
I'm doing  a standard install.

The updatedatabse.pl fails:

Missing right curly or square bracket at ./installer/data/mysql/updatedatabase.pl line 5784, at end of line
syntax error at ./installer/data/mysql/updatedatabase.pl line 5784, at EOF
Execution of ./installer/data/mysql/updatedatabase.pl aborted due to compilation errors.

It might be a pain, but try a std install db with the version set early enough...

Also,

I think you need to add the course_reserves directory to the Makefile.PL
target_map.  I added one right after the circ line, and all that code got copied to the designated INTRANET_CGI_DIR.

  './circ'                      => 'INTRANET_CGI_DIR',
  './course_reserves',          => 'INTRANET_CGI_DIR',

I was able to access the Course Reserves from the staff interface.

But, without the tables...

wajasu

(In reply to comment #10)

Thanks for the feedback! I've submitted a followup patch that should resolve your problems. I was able to successfully run the db updater from the command line, and I've also added the directory to the Makefile. Thanks again.

Kyle

> I tried again with.  Bug 8215 - Add Course Reserves 
> I'm doing  a standard install.
...
> I was able to access the Course Reserves from the staff interface.
> 
> But, without the tables...
> 
> wajasu

OK. Now we are moving somewhere.  I applied these with no problem.
 Bug 8215 - Add Course Reserves
 Bug 8215 - Add Course Reserves - Followup - Fix DB Updater & Makefile
The updatedatabase.pl ran fine.

The things with a plus at the left I did.  I've attached snapshots of various
places in the workflow. (nice workflow)
Testing Scenario:
+ 1) Create new authorised value categories DEPARTMENT and TERM
  2) Create a new course, add instructors to that course.
+ create a new course (but without instructor)
- no instructor (added a patron as a teacher. Implemented yet?)
Note: Hitting Enter in instructor field Saves :( let me press the button.
  3) Reserve items for that course, verify item attributes have changed.
+ reserved items ( queried db and saw rows )
? tell me where to look to verify item attributes. I attached some images.
  I see a Course Reserves column with the Course Name in the Holdings tab.
  Is that what you mean?  Or some biblio MARC tag field OR other reserve table. 
  4) Disable course, verify item attributes have reverted.
- I still see the Course Reserves in Item's catalog detail page when course Inactive
  5) Enable course again, verify item attributes again.
  6) Delete course, verify item attributes again.
+ When delted the Course Reserves column if gone. As is the row in the DB.
  7) Create two new courses, add the same item(s) to both courses.
Note: I can create multiple courses with same name, etc. Whats the primary key
or natural key of this table going to be?
  8) Disable one course, verify item attributes have not reverted.
  9) Disable both courses, verify item attributes have reverted.
 10) Enable one course, verify item attributes are again set to the
     new values.
 11) Edit reserve item attributes, verify.
 12) Disable all courses, edit reserve item attributes, verify
     the item itself still has its original attributes, verify
     the reserve item attributes have been updated.
 13) Verify the ability to remove instructors from a course.
 14) Verify new permissions, top level coursereserves, with
     subpermissions add_reserves and delete_reserves.

I did see all fields being saved in these tables:
course_items
course_reserves
courses

In step #4, I think you need to check for each item's course being enabled=true to show the course name(s), though at the top of
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
you have the right idea to show the "Course Reserves" column if any one item is reserved.

The OPAC seemed fine to browse the course and its reserves.
 
So we are moving in the right direction, and with a little more clarity on what verifying item attributes means, after your next patch. With instructors, things may be in good standing.

I think one we get this signed off, other smaller enhancements/bugs can be addressed separately.

wajasu

> - no instructor (added a patron as a teacher. Implemented yet?)

Teachers *are* patrons, so what you did is correct.

> Note: Hitting Enter in instructor field Saves :( let me press the button.

I'll see what I can do about that.

> ? tell me where to look to verify item attributes. 

I just meant the change in item type and collection code. You can check those from the standard record details page.

>   4) Disable course, verify item attributes have reverted.
> - I still see the Course Reserves in Item's catalog detail page when course
> Inactive

I'll have to check on that.

(In reply to comment #24)
> Created attachment 11862 [details] [review]
> Fixed by adding some jquery, such that any form field
> with the class "noEnterSubmit" will but trigger a form submission.

This duplicates functionality which is already in staff-global.js (function checkEnter(), line 77). I like that your implementation uses a class, though. Could there be a way to combine these scripts?

If not, at the very least this script should be added to staff-global.js instead of included in doc-head-close.inc since it doesn't require template processing.

Here's a followup that should do what you are recommending.

(In reply to comment #25)
> (In reply to comment #24)
> > Created attachment 11862 [details] [review]
> > Fixed by adding some jquery, such that any form field
> > with the class "noEnterSubmit" will but trigger a form submission.
> 
> This duplicates functionality which is already in staff-global.js (function
> checkEnter(), line 77). I like that your implementation uses a class,
> though. Could there be a way to combine these scripts?
> 
> If not, at the very least this script should be added to staff-global.js
> instead of included in doc-head-close.inc since it doesn't require template
> processing.

did a fresh standard install.

- can't add instructors (tried placing barcode number, and borrowoer id in both add and edit course forms).  I'm suggesting you make this a button with popup form allowing selection from the list of "teachers".  Maybe you can leverage patron search. (is that a template yet?)
(if you clear you dev database course tables, maybe this would show up. or am i missing something?)
 
- the "Course Reserves" column in the OPAC does show at the end of the holdings tab row (Nice!).   But, not in the staff client holdings.  For staff holdings, it might need to be near the column showing status (Not for Loan), so you can see if its already checked out and highlight in red if its a course reserve and "On Loan" conflict.  If its course reserved, should we disallow loaning it?
Note: If the course is made inactive, it should allow it to be for loan. (in item table)

- Are you integrating with the "holdings" subsystem?  If an item is "course reserved" what does that mean?  Is the On Loan flag set in the biblo item? etc

- the "Course reserves" column inthe OPAC, shows even when the course is InActive.  Your logic to decide to show the column should check the course active flag as well.

- mod_course.pl  has template_name  => "about.tmpl"    (is this what you want?)

- when adding an item for "course resserve", you have "LEAVE UNCHANGED" and show "Unchanged" in the course_details page, for "Item Type", "Collection", "Location", "Library".  Can you add the current data underneath the  italicized "Unchanged" for easier usability.  You already have the code t show it.

- I checked out an item that was a "course reserve"  is that allowed?  Should a staff get a popup warning it is a course reserve at checkout time?  Also, in OPAC item view I see the status as checked out, when I click on the "cousre reserve item link" I get the course-details which show the item is available, so maybe you need to supply the model with those fields in that item-status.inc.

Note; Hopefully in the future, each column would have its own tt template, and one would compose a row (for holdings, etc).  The logic to determine sophisticated "Availability" might be moved to the service/manager  (ie. ItemManager.GetStatus.GetAvailabilityDescription or such)

wajasu

> - can't add instructors (tried placing barcode number, and borrowoer id in
> both add and edit course forms).  I'm suggesting you make this a button with
> popup form allowing selection from the list of "teachers".  Maybe you can
> leverage patron search. (is that a template yet?)
> (if you clear you dev database course tables, maybe this would show up. or
> am i missing something?)

This was an ajax based search, but it seems to have broken at some point. I think I will replace it with a more traditional popup search box.
  
> - the "Course Reserves" column in the OPAC does show at the end of the
> holdings tab row (Nice!).   But, not in the staff client holdings.  For
> staff holdings, it might need to be near the column showing status (Not for
> Loan), so you can see if its already checked out and highlight in red if its
> a course reserve and "On Loan" conflict.  If its course reserved, should we
> disallow loaning it?

I'll have to add that.

> Note: If the course is made inactive, it should allow it to be for loan. (in
> item table)

That is actually something you can choose by defining the 'standard' item type an item has, and the new item type it has while on reserve!

> - Are you integrating with the "holdings" subsystem?  If an item is "course
> reserved" what does that mean?  Is the On Loan flag set in the biblo item?
> etc

When an item is on "course reserve" it means that the the item how has the new holding branch, category code, 

> - the "Course reserves" column inthe OPAC, shows even when the course is
> InActive.  Your logic to decide to show the column should check the course
> active flag as well.

The course reserves column should only appear when one or more items on the record have course reserves. I'll have to check on this.

> - mod_course.pl  has template_name  => "about.tmpl"    (is this what you
> want?)

Yes, it doesn't use a template, but it needs to call get_template in order to check permissions. It seems to be standard practice to use about.tmpl in these cases.

> - when adding an item for "course resserve", you have "LEAVE UNCHANGED" and
> show "Unchanged" in the course_details page, for "Item Type", "Collection",
> "Location", "Library".  Can you add the current data underneath the 
> italicized "Unchanged" for easier usability.  You already have the code t
> show it.

I removed it for readability, but I can re-add it.

> - I checked out an item that was a "course reserve"  is that allowed? 
> Should a staff get a popup warning it is a course reserve at checkout time? 
> Also, in OPAC item view I see the status as checked out, when I click on the
> "cousre reserve item link" I get the course-details which show the item is
> available, so maybe you need to supply the model with those fields in that
> item-status.inc.

It is allowed if you've defined the item type to allow it. The system is designed for flexibility. Putting an item on course reserve does not in itself change the behavior of an item. The behavior as altered by setting new item types, holding branches, etc.

> Note; Hopefully in the future, each column would have its own tt template,
> and one would compose a row (for holdings, etc).  The logic to determine
> sophisticated "Availability" might be moved to the service/manager  (ie.
> ItemManager.GetStatus.GetAvailabilityDescription or such)

This is an interesting notion!

Just an fyi, I was able to get the instructor search working again. Between my writing it, and your testing it, the YUI ajax search was replaced with a jQuery based one. It works just like the "checkout" borrower search. It appears that you need to add at least three letters of the persons first or last name before you will see the results.

This is working now!

Everything seems to work now!!!
CRUD - courses, adding instructors(teachers), to courses, 
adding reserves to sources, fields, and menus hiding when appropriate.

Except: 
Permissions related:
- if one wanted to delegate ADD/DEL reserves permissions without also the manage(ADE/EDIT?DEL) courses permission, that can't be done.  Without manage courses, course_reserves is not an accessible page, so one can't get to the links to add/del  reserves.
- Also, while i had permissions to delete reserves, I copied the url of the remove link to the clipboard.  then i removed the staff permssion allowing delete reserves.  i logged in as staff and saw only the edit link (no remove), BUT I pasted the remove action url from my clipboard and the delete worked. So a check needs to be in the script/controller/security layer to see what is an accepted action.  The same probably goes for courses.

Ideas to consider - take em or leave em.
- I can add multiple courses all with the SAME fields.  Maybe a check to dissalow duplicates for (name, term, dept, course_number, section).
- Maybe the TERM and DEPARTMENT could be added to the optional data that folks choose to load. The future testers would be helped.
- I don't know the repercussion of having no library set. (NULL in columns?),
You might want a check and popup to prevent adding rows unless a library is set.
- I know after this is signed off, you will get other enhancements.  A course_schedule ( Days/Hours) and course_location ( North Campus/Bldg A/Edgars Hall/Room 202)   is something the "students" could use. Lots more work for future effort.


Otherwise:
Greate work!!!  A lot to test.

wajasu

> Permissions related:
> - if one wanted to delegate ADD/DEL reserves permissions without also the
> manage(ADE/EDIT?DEL) courses permission, that can't be done.  Without manage
> courses, course_reserves is not an accessible page, so one can't get to the
> links to add/del  reserves.

This should be fixed in this new patch.

> - Also, while i had permissions to delete reserves, I copied the url of the
> remove link to the clipboard.  then i removed the staff permssion allowing
> delete reserves.  i logged in as staff and saw only the edit link (no
> remove), BUT I pasted the remove action url from my clipboard and the delete
> worked. So a check needs to be in the script/controller/security layer to
> see what is an accepted action.  The same probably goes for courses.

Also fixed in the new patch.

> - Maybe the TERM and DEPARTMENT could be added to the optional data that
> folks choose to load. The future testers would be helped.

We could definitely do this, but the values for TERM and DEPARTMENT are so library-specific that any data we add would certainly not be usable out of the box. I think it is just as good if not better to just document them as part of the setup process in the manual.

> - I don't know the repercussion of having no library set. (NULL in columns?),
> You might want a check and popup to prevent adding rows unless a library is
> set.

There are no repercussions for not having a library set. None of the code for this feature uses the logged in user's set branch for anything, so no worries there!

Everything worked, permissions etc.

Except one behavior:
-When I've added the permissions to add courses to a new staff patron, the autosearch javascript for adding an instructor doesn't work.  It works for the koha sysadmin, but my staff user only had catalog and manage courses set for permissions.

If you figure out why that call doesn't get through security or such, then I think things will be signed off.

wajasu

The ajax borrower search requires that the logged in librarian have full circulation permissions. I don't think this would be considered a bug.

Kyle

(In reply to comment #38)
> Everything worked, permissions etc.
> 
> Except one behavior:
> -When I've added the permissions to add courses to a new staff patron, the
> autosearch javascript for adding an instructor doesn't work.  It works for
> the koha sysadmin, but my staff user only had catalog and manage courses set
> for permissions.
> 
> If you figure out why that call doesn't get through security or such, then I
> think things will be signed off.
> 
> wajasu

Hi Kyle, 

sorry to reset this to 'failed qa', but I spotted a translation problem. 

You only add the new permissoins to the English files, which means they will not be available for anyone using not the English sample files. Could you add them to the other languages as well please? You don't have to translate the descriptions - translators will take care of it. But this way functionality is there and it's easier to fix a missing translation than to figure out a missing permission.

I found some other minor things, but wanted to note them:

Another request I have would be to add some documentation to your new tables - I am sure Nicole would get to it sometime, but I think it would much easier for you to do.

Some of the templates don't follow the capitalization rules (example: Course Reserves)

I am wondering about some changes that don't seem to belong to the course reserves module:

Example: maninvoice.tt:
 //<![CDATA[
 $(document).ready(function(){
-	$("fieldset.rows input").keydown(function(e){ return checkEnter(e); });
+	$("fieldset.rows input").addClass("noEnterSubmit");

Can you explain why the change? Maybe this should be a separate bug as it seems to change a general pattern that is used in different files.

> You only add the new permissoins to the English files, which means they will
> not be available for anyone using not the English sample files. Could you
> add them to the other languages as well please? You don't have to translate
> the descriptions - translators will take care of it. But this way
> functionality is there and it's easier to fix a missing translation than to
> figure out a missing permission.

Will do.

> I found some other minor things, but wanted to note them:
> 
> Another request I have would be to add some documentation to your new tables
> - I am sure Nicole would get to it sometime, but I think it would much
> easier for you to do.

Will do. I'll just add the documentation to the body of my followup patch.

> Some of the templates don't follow the capitalization rules (example: Course
> Reserves)

Will do.

> I am wondering about some changes that don't seem to belong to the course
> reserves module:
> 
> Example: maninvoice.tt:
>  //<![CDATA[
>  $(document).ready(function(){
> -	$("fieldset.rows input").keydown(function(e){ return checkEnter(e); });
> +	$("fieldset.rows input").addClass("noEnterSubmit");
>
> Can you explain why the change? Maybe this should be a separate bug as it
> seems to change a general pattern that is used in different files.

Please look at comments 24 and 25.

(In reply to comment #42)
> > You only add the new permissoins to the English files, which means they will
> > not be available for anyone using not the English sample files. Could you
> > add them to the other languages as well please? You don't have to translate
> > the descriptions - translators will take care of it. But this way
> > functionality is there and it's easier to fix a missing translation than to
> > figure out a missing permission.
> 
> Will do.

For future reference, there is a unit test that will catch this- xt/permissions.t.

I'm going to set this to signed off for now, as the followup does not affect the functionality in any way. It's just changing the name of one system pref, and changing the capitalization of some words. If anyone feels this needs it's own sign-off, please reset the status.

Thanks go to wajasu for the extensive and stringent testing this patch has gone through!

QA Comments:

Great patch :)
Code clean and easy to read.

Some remarks:
Blocker:
* perlcritic does not pass on course_reserves/add_items.pl:
Variable declared in conditional statement at line 60, column 5.  Declare variables outside of the condition.  (Severity: 5)
easy to solve.
* It would be good to have an information (close to "Department" and "Term" on the create course page) to explain how to add departments and terms.

not blocker:
- There is a "dot" before the li element of the instructor result list (overflow).
- the edit is on the course.pl and redirects to mod_course.pl, it is ambiguous but the code is short and clean so I understand this choice.
- add_items-step1.tt and add_items-step2.tt could be merged into 1 template
- It would be easier to use DataTable for the course reserves list. Thus the search form would have become useless

The VA is DEPARTMENT, not DEPT :) (the patch displays "No DEPT authorised values found!")

Fixed. Good catch!

QA comments:
To begin, I'm really sorry to fail QA this large patch, with a great feature, but I think it's needed

 * I don't like at all the introduction of
+sub GetItemTypesFlat {
+    my $dbh   = C4::Context->dbh;
+    my $query = "SELECT * FROM itemtypes ORDER BY description";
+    my $sth = $dbh->prepare($query);
+    $sth->execute;
+
+    return $sth->fetchall_arrayref({});
+}
That's another way to access itemtype. We need consistency, not more and more various way to access the same information
That would have been a wonderful time to introduce Koha::Service::Itemtype.pm ! Can you find a way to avoid introducing this function ?

* The introduction of Koha/CourseReserves.pm is wrong:
 - it must follow the structure we've defined at the hackfest and updated after some tests I made (see mail on koha-devel, june 25th, Re: [Koha-devel] http://wiki.koha-community.org/wiki/Koha_Namespace_RFC, moving ahead
 - it must be OOP, as everything that enters Koha::
 - parameters must be passed by hashes (this one is OK)

I let you 2 options here :
 * you rewrite CourseReserves.pm in Koha:: namespace to respect the rule
 * you move it to C4/ as it's an 'old style' stuff
(I bet you'll follow the 2nd patch, that is *much* easier, but that would have been a great occasion to start filling Koha:: :\ )

* mod_course.pl says:
+#script to modify reserves/requests
+#written 2/1/00 by chris@katipo.oc.nz
+#last update 27/1/2000 by chris@katipo.co.nz
+# Copyright 2000-2002 Katipo Communications

are you sure this code is 12 years old ? ;-)

* There are no foreign keys, we can have some (not sure this list is complete):
 - instructors.borrowernumbers => borrowers.borrowernumber
 - instructors.course_id => courses.course_id
 - course_items.itemnumber => items.itemnumber
 - course_items.holdingbranch => branches.branchcode
 - course_reserves.course_id => courses.course_id

Not failing QA for that but:
 * scripts names sounds a little bit long/duplicated : course_reserves/course-details.pl => could be course/detail, don't you think ?
 - do we really need course_reserve as directory ?
 - do we need to repeas course in course-detail.pl
 * the course_reserve table has an index with a probable typo in the name:
+   UNIQUE KEY `psuedo_key` (`course_id`,`ci_id`), => PSEUDO, not PSUEDO ?

QUESTIONS:
 * what are the changes to 
 .../prog/en/modules/members/mancredit.tt           |    2 +-
 .../prog/en/modules/members/maninvoice.tt          |    2 +-
 .../prog/en/modules/members/memberentrygen.tt      |    2 +-
 done for ? Why is it in this patch ?

PS : if you decide to go to Koha::Service::Itemtype.pm, don't do it with DBIx::Class, just with standard SQL. Introducing DBIx::Class is a huge thing, that must be done separately

(In reply to comment #52)
> QA comments:
> To begin, I'm really sorry to fail QA this large patch, with a great
> feature, but I think it's needed

No problem, better a patch be failed 1000 times, than let bad code make it into master!

>  * I don't like at all the introduction of
> +sub GetItemTypesFlat {
> +    my $dbh   = C4::Context->dbh;
> +    my $query = "SELECT * FROM itemtypes ORDER BY description";
> +    my $sth = $dbh->prepare($query);
> +    $sth->execute;
> +
> +    return $sth->fetchall_arrayref({});
> +}
> That's another way to access itemtype. We need consistency, not more and
> more various way to access the same information
> That would have been a wonderful time to introduce
> Koha::Service::Itemtype.pm ! Can you find a way to avoid introducing this
> function ?

I've fixed this by merging both options into GetItemTypes. I would write something like Koha::Service::Itemtype, but I'm waiting for DBIX::Class for Koha.

> * The introduction of Koha/CourseReserves.pm is wrong:
>  - it must follow the structure we've defined at the hackfest and updated
> after some tests I made (see mail on koha-devel, june 25th, Re: [Koha-devel]
> http://wiki.koha-community.org/wiki/Koha_Namespace_RFC, moving ahead
>  - it must be OOP, as everything that enters Koha::
>  - parameters must be passed by hashes (this one is OK)
> 
> I let you 2 options here :
>  * you rewrite CourseReserves.pm in Koha:: namespace to respect the rule
>  * you move it to C4/ as it's an 'old style' stuff
> (I bet you'll follow the 2nd patch, that is *much* easier, but that would
> have been a great occasion to start filling Koha:: :\ )

I have opted for the latter option. I'd love to make this module OOP, but it's already taken so long to develop! I think I would just end up breaking it ; )

> * mod_course.pl says:
> +#script to modify reserves/requests
> +#written 2/1/00 by chris@katipo.oc.nz
> +#last update 27/1/2000 by chris@katipo.co.nz
> +# Copyright 2000-2002 Katipo Communications
> 
> are you sure this code is 12 years old ? ;-)

Good catch!

> * There are no foreign keys, we can have some (not sure this list is
> complete):
>  - instructors.borrowernumbers => borrowers.borrowernumber
>  - instructors.course_id => courses.course_id
>  - course_items.itemnumber => items.itemnumber
>  - course_items.holdingbranch => branches.branchcode
>  - course_reserves.course_id => courses.course_id

Will do!

> QUESTIONS:
>  * what are the changes to 
>  .../prog/en/modules/members/mancredit.tt           |    2 +-
>  .../prog/en/modules/members/maninvoice.tt          |    2 +-
>  .../prog/en/modules/members/memberentrygen.tt      |    2 +-
>  done for ? Why is it in this patch ?

Please look at comments 24 and 25. It was a request from Owen.

> PS : if you decide to go to Koha::Service::Itemtype.pm, don't do it with
> DBIx::Class, just with standard SQL. Introducing DBIx::Class is a huge
> thing, that must be done separately

I'd rather wait for DBIx::Class, than reinvent the wheel! I think the fix I've implemented is fine for now.

OK, it could be OK this time. but as I pushed a lot of things recently, there is a conflict in C4/Koha.pm that I'll let you fix.
Plus, the follow-ups on updatedatabase.pl won't apply nicely.

So, the best option here is that you:
 * squash all patches in one
 * then rebase against master
 * test everything is OK
 * git bz attach rebased-squashed patch

and i'll be able to test & hopefully push !

Note that this feature is not heavily nested in the rest of Koha. So I could push it after the Feature Freeze, as there is only a side effect. ( But I would also be OK to push before the FF if you rebase quickly ;-) )

Here you go! Rebased, squashed, and tested.

(In reply to comment #55)
> OK, it could be OK this time. but as I pushed a lot of things recently,
> there is a conflict in C4/Koha.pm that I'll let you fix.
> Plus, the follow-ups on updatedatabase.pl won't apply nicely.
> 
> So, the best option here is that you:
>  * squash all patches in one
>  * then rebase against master
>  * test everything is OK
>  * git bz attach rebased-squashed patch
> 
> and i'll be able to test & hopefully push !
> 
> Note that this feature is not heavily nested in the rest of Koha. So I could
> push it after the Feature Freeze, as there is only a side effect. ( But I
> would also be OK to push before the FF if you rebase quickly ;-) )

QA comment:
 * why is CourseReserve in Circulation tab ? Sounds strange to me (not a reason to fail QA)
 * Authorised_values = hardcoded ones are usually hardcoded on admin/authorised_values.pl. I think you can add safely DEPARTMENT and TERM. Even if there is no DEPARTMENT And TERM, they appear, and the user know they are here.
Look here:
    foreach (qw(Asort1 Asort2 Bsort1 Bsort2 SUGGEST DAMAGED LOST REPORT_GROUP REPORT_SUBGROUP)) {
(in admin/authorised_values.pl)

 * I've added a DEPARTMENT: "Département Sciences". On course.pl, I get a nasty:
D�partement Lettres after saving my 1st course (the diacritic reencoded or something like that) I've added a diacritic to my TERM, and same problem: 2eme trimestre ����. other fields are OK. I've checked other authorised values (like in cataloguing, no encoding problem, it's specific to course.pl)

 * GRRR... you don't respect HTML4 coding guideline : "Add Reserves" should be "Add reserves". There are many many strings like that

 * Step 2 of course/add_items.pl, I'm not sure I understand why& when collection/itemtype/location/branch should change, and when it will. Some documentation (online help ?) welcomed !
 
 * After adding a reserve to my course I see my branchname encoding broken as well:  Unchanged (M�diath�que Cannet des Maures)

 * After adding a reserve to my course, I see that the itemtype is "Unchanged ()" the () make me think you don't handle the case where items.itype is empty, the itemtype is in biblioitems.itemtype (that's what I have in my database)

 * I use the ccsr theme for OPAC, and couldn't find where I could access opac-course.pl. I understood it's because you add a line to masthead.inc
+[% IF ( UseCourseReserves ) %]<span class="pipe"> | </span><a href="/cgi-bin/koha/opac-course-reserves.pl">Course Reserves</a>[% END %]
that is not in ccsr masthead.inc
 Please add it !

 *Something I don't understand functionnally: I places a course reserve on an item that was on loan. When I check-in the document, nothing happens (ie: i'm not warned the item is for "course X" or something like that. Is it the expected behaviour ? Some functionnal explanations are worth...

(In reply to comment #58)
> QA comment:
>  * why is CourseReserve in Circulation tab ? Sounds strange to me (not a
> reason to fail QA)

I'm nor sure what you mean by this. Could you post a screen shot?

>  * Authorised_values = hardcoded ones are usually hardcoded on
> admin/authorised_values.pl. I think you can add safely DEPARTMENT and TERM.
> Even if there is no DEPARTMENT And TERM, they appear, and the user know they
> are here.
> Look here:
>     foreach (qw(Asort1 Asort2 Bsort1 Bsort2 SUGGEST DAMAGED LOST
> REPORT_GROUP REPORT_SUBGROUP)) {
> (in admin/authorised_values.pl)

Will fix.

>  * I've added a DEPARTMENT: "Département Sciences". On course.pl, I get a
> nasty:
> D�partement Lettres after saving my 1st course (the diacritic reencoded or
> something like that) I've added a diacritic to my TERM, and same problem:
> 2eme trimestre ����. other fields are OK. I've checked other authorised
> values (like in cataloguing, no encoding problem, it's specific to course.pl)

I cannot really say. It must have something to due with the AuthorisedValues plugin. If I use Dumper from the template, I get this:

$VAR1 = "D\x{e9}partement Sciences";

\x{e9} is indeed the code for é. The select pulldown for TERM and DEPARTMENT on the course edit page look fine, but they use GetAuthorisedValues passed in as a template parameter, rather than the TT plugin.

I have no idea what is going on here.

>  * GRRR... you don't respect HTML4 coding guideline : "Add Reserves" should
> be "Add reserves". There are many many strings like that

Sorry! I though I had caught all those. I've gone through the template again and I think I've probably fixed them all now.

>  * Step 2 of course/add_items.pl, I'm not sure I understand why& when
> collection/itemtype/location/branch should change, and when it will. Some
> documentation (online help ?) welcomed !

The idea is to be able to change those fields while the item is on reserve for at least one course. This is so we can change the circulation rules. For example, an item might have a loan period of 2 weeks, but if it is on reserve for an active course, it may change itemtypes and now have a loan period of 2 hours.

>  * After adding a reserve to my course I see my branchname encoding broken
> as well:  Unchanged (M�diath�que Cannet des Maures)

Again, it seems to be related to the plugin, but I don't know how to fix it :(

>  * After adding a reserve to my course, I see that the itemtype is
> "Unchanged ()" the () make me think you don't handle the case where
> items.itype is empty, the itemtype is in biblioitems.itemtype (that's what I
> have in my database)

I don't think this feature can play well with bib-level itemtypes. If one record had 5 items and only 1 was on course reserve, it would change the itemtype for all 5! I think I could just hide the itemtype field in the case that itemtypes are set to bib-level, rather than item-level.

>  * I use the ccsr theme for OPAC, and couldn't find where I could access
> opac-course.pl. I understood it's because you add a line to masthead.inc
> +[% IF ( UseCourseReserves ) %]<span class="pipe"> | </span><a
> href="/cgi-bin/koha/opac-course-reserves.pl">Course Reserves</a>[% END %]
> that is not in ccsr masthead.inc
>  Please add it !

Will do!

>  *Something I don't understand functionnally: I places a course reserve on
> an item that was on loan. When I check-in the document, nothing happens (ie:
> i'm not warned the item is for "course X" or something like that. Is it the
> expected behaviour ? Some functionnal explanations are worth...

That is indeed expected behavior. The course reserves system was written based on a specification I was given. I will definitely work with Nicole to make sure it is fully documented in the manual.

Good news! I figured out how to fix the diacritic problem! This new followup should address all your concerns, except the bib-level itemtypes. I'll post another patch for that.

Another question:
once an item has been placed for a course reserve, it should not be possible to check it out, isn't it ? Or at least, the librarian should be warned !

That's not the case:
 * I placed barcode=1 on a course reserve.
 * I checked on catalogue/detail.pl that the item was reserved
 * I tried to check-out the item, and got no warning or anything

Imo: we should have a syspref deciding if a check-out for an item placed on reserve result in a warning or it forbidden.

(In reply to comment #59)
> >  *Something I don't understand functionally: I places a course reserve on
> > an item that was on loan. When I check-in the document, nothing happens (ie:
> > I'm not warned the item is for "course X" or something like that. Is it the
> > expected behavior ? Some functional explanations are worth...
> 
> That is indeed expected behavior. The course reserves system was written
> based on a specification I was given. I will definitely work with Nicole to
> make sure it is fully documented in the manual.

Definitely, I need the complete specifications! because I think I'm missing something about the expected behavior.

> That's not the case:
>  * I placed barcode=1 on a course reserve.
>  * I checked on catalogue/detail.pl that the item was reserved
>  * I tried to check-out the item, and got no warning or anything
> 
> Imo: we should have a syspref deciding if a check-out for an item placed on
> reserve result in a warning or it forbidden.

Paul, this feature isn't meant to limit the ability to checkout an item ( though it can ). What this feature does is give use the ability to change an item's category code, holding branch, shelving location, and item type *while it's on reserve*. So, an item may usually have a checkout period of 14 days, but while it's on course reserve, the item type may change such that the checkout period is only 2 hours ( and the shelving location can change to 'course reserve shelf' ).

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.

Could I please get sign-offs on the last three patches? Once these have been signed off this feature can be reviewed by the QA team, and hopefully marked "Passed QA." I am assigning Katrin as QA contact. As QAM she gets to be annoying and make sure that it gets QAed by someone on the QA team.

Can this go to QA now? The QA Followups haven't been signed, but they also don't alter the functionality of the feature.

Hi Kyle, 

I would like to see the last 3 patches signed off. Also, they don't have any useful commit messages. Please add information about what they are supposed to fix and how to test it. Once they are signed off I will try to come back to this as soon as possible. 

Failing QA for the commit messages - switch to 'Needs Signoff' once that is fixed. Thx!

*** Bug 8551 has been marked as a duplicate of this bug. ***

Patch tested with a sandbox, by Corinne Bulac <corinne.hayet@bulac.fr>

QA comment:

I didn't find any bugs, all looks good on a functional point of view.

However, I found some issues regarding the code.

1/ QA tools complains, I will submit a followup for that.
2/ course.tt : I think the TODO could be removed
3/ the exported variable $VERSION is useless in C4::CourseReserves
4/ It would be better to used hashref instead of hash for some routines in C4::CourseReserves
5/ In C4::CourseReserves, @FIELDS would be returned by a routine (instead of using a global var).
6/ Some use of the loop var ($_)
7/ I am not sure about the consistence of changes in C4::Items::GetItemTypes
To specify the type of the return in parameter is quite weird.
8/ 3 pl scripts should show an error for the user in the template if no course_id exist instead of dying.
9/ Is it normal there is no flag required for course_reserves/course-reserves.pl ?
10/ In kohastructure: Why do you add constraints using an alter table command?  Constraints should be added with the CREATE TABLE command
11/ ShowCourseReserves could be easily calculated in .pl file
12/ YUI buttons should be replaced with Bootstrap
13/ What is the use of the class "noEnterSubmit" (in template files)? I didn't find any reference in .js or .css files.
14/ Template names is now .tt, not .tmpl

Please detail 9 and 13.
Marked as Failed QA for 8, 10 and 12.

> 8/ 3 pl scripts should show an error for the user in the template if no
> course_id exist instead of dying.

Fixed!

> 9/ Is it normal there is no flag required for
> course_reserves/course-reserves.pl ?

Fixed!

> 12/ YUI buttons should be replaced with Bootstrap

Fixed!

> 13/ What is the use of the class "noEnterSubmit" (in template files)? I
> didn't find any reference in .js or .css files.

There is a conversation about it on this ticket. The code must have been removed by accident. I have added it back in.

(In reply to comment #101)
> > 13/ What is the use of the class "noEnterSubmit" (in template files)? I
> > didn't find any reference in .js or .css files.
> 
> There is a conversation about it on this ticket. The code must have been
> removed by accident. I have added it back in.

I create a js file for that, but patch is not pushed (bug 9507).

Your last patch breaks link for confirm deletion, I will provide a followup, please sign-off it.

Kyle, I switched back to Needs Signoff, but of course you can sign off it :)

All still looks good.

I rebased patches (some conflicts).
I will provide a followup in order to fix an ergonomic issue: on the course detail page, if ccode, location, etc. are unchanged and no value exist, the cell contains "Unchanged ()".

I don't see any reasons not to pass QA this feature: all remarks had taken into account.

Marked as Passed QA.

I have attached six follow-up patches to deal with the following issues:

- perltidy
- ensuring that the new C4::CourseReserves module does not export any symbols by default
- dealing with a copy-pasto that would have allowed unprivileges users to delete course reserves via the OPAC
- cleaning up after a couple spurious warning messages that could appear in the Apachelogs
- a template typo fix
- renaming a method that could be confused with one that might be used for reserves qua hold requests

All of this is also available in the new/bug_8215_merge_candidate branch as well.

Upon QA review of my follow-up patches, I am prepared to merge this to master.

Not that I feel strongly about it, but I thought our official perltidy stance was to use the default perltidy configuration ( http://wiki.koha-community.org/wiki/Perltidy ).

Also, thanks for the followups Galen! Much appreciated!

Created attachment 18282 [details] [review]
Bug 8215 - Course Reserves

Adds a course reserves system for academic libraries.

The course reserves system allows libraries to create courses
and put items on reserves for those courses.

Each item with at least one reserve can have some of its attributes
modified while it is on reserve for at least one active course.
These attributes include item type, collection code, shelving location,
and holding library. If there are no active courses with this item
on reserve, it's attributes will revert to the original attributes
it had before going on reserve.

Test Plan:
  1) Create new authorised value categories DEPARTMENT and TERM
  2) Create a new course, add instructors to that course.
  3) Reserve items for that course, verify item attributes have changed.
  4) Disable course, verify item attributes have reverted.
  5) Enable course again, verify item attributes again.
  6) Delete course, verify item attributes again.
  7) Create two new courses, add the same item(s) to both courses.
  8) Disable one course, verify item attributes have not reverted.
  9) Disable both courses, verify item attributes have reverted.
 10) Enable one course, verify item attributes are again set to the
     new values.
 11) Edit reserve item attributes, verify.
 12) Disable all courses, edit reserve item attributes, verify
     the item itself still has its original attributes, verify
     the reserve item attributes have been updated.
 13) Verify the ability to remove instructors from a course.
 14) Verify new permissions, top level coursereserves, with
     subpermissions add_reserves and delete_reserves.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Corinne Bulac <corinne.hayet@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

http://bugs.koha-community.org/show_bug.cgi?id=8125

Created attachment 18283 [details] [review]
Bug 8215 follow-up encoding for branches & itemtypes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Corinne Bulac <corinne.hayet@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Created attachment 18284 [details] [review]
Bug 8215: Followup FIX QA issues

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Created attachment 18285 [details] [review]
Bug 8215 - Add Course Reserves - QA Followup

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Created attachment 18286 [details] [review]
Bug 8215: Followup Fix confirmation links for delete course and item

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Created attachment 18287 [details] [review]
Bug 8215: FIX ergonomic issue : 'Unchanged()'

Created attachment 18288 [details] [review]
bug 8215: (followup) run new files through perltidy

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 18289 [details] [review]
bug 8215: (followup) make sure C4::CourseReserves doesn't export anything

New modules should not export any symbols by default
without a very good reason.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 18290 [details] [review]
bug 8215: (followup) don't allow deleting course reserves from OPAC

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 18291 [details] [review]
bug 8215: (followup) avoid SQL syntax error in logs

ModItem currently will attempt to update an item
even if no field updates are specified.  This patch
avoids (harmless) error messages in the Apache
logs if an item is not actually being changed when it
is placed or taken off reserve.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 18292 [details] [review]
bug 8215: (followup) rename GetItemReservesInfo

Renamed that routine to GetItemCourseReservesInfo in
order to avoid any potential confusion with reserves
qua hold requests.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 18293 [details] [review]
bug 8215: (followup) remove extraneous underscore in template

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Passes koha-qa.pl, tested and works with all followups, inspection of followups reveals nothing untoward. Passing QA on Galen's followups.

I am the author of the original patch, so if it is deemed inappropriate for me to pass qa on this bug, please slap me with a large trout and reset the status.

(In reply to comment #141)
> Passes koha-qa.pl, tested and works with all followups, inspection of
> followups reveals nothing untoward. Passing QA on Galen's followups.
> 
> I am the author of the original patch, so if it is deemed inappropriate for
> me to pass qa on this bug, please slap me with a large trout and reset the
> status.

No trout necessary, IMO -- the patch series as a whole has seen sufficient attention by other QA team members, and as original author of the patch, you're in a good position to evaluate the followups.

Pushed (along with a one-liner follow-up that I missed attaching to this bug) to master.

Thanks, Kyle!

*** Bug 9426 has been marked as a duplicate of this bug. ***

With this feature, we get new template plugins but are there not duplicated for : 
- authorised value : AuthorisedValues.pm / KohaAuthorisedValues.pm
- branch name : Branches.pm / KohaBranchName.pm

Should the old Koha*.pm ones be removed?

(In reply to comment #144)
> With this feature, we get new template plugins but are there not duplicated
> for : 
> - authorised value : AuthorisedValues.pm / KohaAuthorisedValues.pm
> - branch name : Branches.pm / KohaBranchName.pm
> 
> Should the old Koha*.pm ones be removed?

Yep I think. Especially because Koha* plugins don't deal with encoding issues.

*** Bug 9425 has been marked as a duplicate of this bug. ***

Created attachment 21885 [details] [review]
Bug 8215 - Course Reserves - Omnibus - v3.12.05

Bug 8215 - Course Reserves

Adds a course reserves system for academic libraries.

The course reserves system allows libraries to create courses
and put items on reserves for those courses.

Each item with at least one reserve can have some of its attributes
modified while it is on reserve for at least one active course.
These attributes include item type, collection code, shelving location,
and holding library. If there are no active courses with this item
on reserve, it's attributes will revert to the original attributes
it had before going on reserve.

Test Plan:
  1) Create new authorised value categories DEPARTMENT and TERM
  2) Create a new course, add instructors to that course.
  3) Reserve items for that course, verify item attributes have changed.
  4) Disable course, verify item attributes have reverted.
  5) Enable course again, verify item attributes again.
  6) Delete course, verify item attributes again.
  7) Create two new courses, add the same item(s) to both courses.
  8) Disable one course, verify item attributes have not reverted.
  9) Disable both courses, verify item attributes have reverted.
 10) Enable one course, verify item attributes are again set to the
     new values.
 11) Edit reserve item attributes, verify.
 12) Disable all courses, edit reserve item attributes, verify
     the item itself still has its original attributes, verify
     the reserve item attributes have been updated.
 13) Verify the ability to remove instructors from a course.
 14) Verify new permissions, top level coursereserves, with
     subpermissions add_reserves and delete_reserves.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Corinne Bulac <corinne.hayet@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

http://bugs.koha-community.org/show_bug.cgi?id=8125

Bug 8215 follow-up encoding for branches & itemtypes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Corinne Bulac <corinne.hayet@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Bug 8215: Followup FIX QA issues

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Bug 8215 - Add Course Reserves - QA Followup

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Bug 8215: Followup Fix confirmation links for delete course and item

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Bug 8215: FIX ergonomic issue : 'Unchanged()'

bug 8215: (followup) run new files through perltidy

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

bug 8215: (followup) make sure C4::CourseReserves doesn't export anything

New modules should not export any symbols by default
without a very good reason.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

bug 8215: (followup) don't allow deleting course reserves from OPAC

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

bug 8215: (followup) avoid SQL syntax error in logs

ModItem currently will attempt to update an item
even if no field updates are specified.  This patch
avoids (harmless) error messages in the Apache
logs if an item is not actually being changed when it
is placed or taken off reserve.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

bug 8215: (followup) rename GetItemReservesInfo

Renamed that routine to GetItemCourseReservesInfo in
order to avoid any potential confusion with reserves
qua hold requests.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

bug 8215: (followup) remove extraneous underscore in template

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

This patch series introduce new TT plugins. They have been grabbed and pushed to 3.12.x, will be in 3.12.6 as they have been actively used for some bugfixes on the stable branch.

Thanks Kyle!

Created attachment 23264 [details] [review]
Bug 8215 - Course Reserves - Omnibus - v3.12.07

Bug 8215 - Course Reserves

Adds a course reserves system for academic libraries.

The course reserves system allows libraries to create courses
and put items on reserves for those courses.

Each item with at least one reserve can have some of its attributes
modified while it is on reserve for at least one active course.
These attributes include item type, collection code, shelving location,
and holding library. If there are no active courses with this item
on reserve, it's attributes will revert to the original attributes
it had before going on reserve.

Test Plan:
  1) Create new authorised value categories DEPARTMENT and TERM
  2) Create a new course, add instructors to that course.
  3) Reserve items for that course, verify item attributes have changed.
  4) Disable course, verify item attributes have reverted.
  5) Enable course again, verify item attributes again.
  6) Delete course, verify item attributes again.
  7) Create two new courses, add the same item(s) to both courses.
  8) Disable one course, verify item attributes have not reverted.
  9) Disable both courses, verify item attributes have reverted.
 10) Enable one course, verify item attributes are again set to the
     new values.
 11) Edit reserve item attributes, verify.
 12) Disable all courses, edit reserve item attributes, verify
     the item itself still has its original attributes, verify
     the reserve item attributes have been updated.
 13) Verify the ability to remove instructors from a course.
 14) Verify new permissions, top level coursereserves, with
     subpermissions add_reserves and delete_reserves.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Corinne Bulac <corinne.hayet@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

http://bugs.koha-community.org/show_bug.cgi?id=8125

Bug 8215 follow-up encoding for branches & itemtypes

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Corinne Bulac <corinne.hayet@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Bug 8215: Followup FIX QA issues

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Bug 8215 - Add Course Reserves - QA Followup

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Bug 8215: Followup Fix confirmation links for delete course and item

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Bug 8215: FIX ergonomic issue : 'Unchanged()'

bug 8215: (followup) run new files through perltidy

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

bug 8215: (followup) make sure C4::CourseReserves doesn't export anything

New modules should not export any symbols by default
without a very good reason.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

bug 8215: (followup) don't allow deleting course reserves from OPAC

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

bug 8215: (followup) avoid SQL syntax error in logs

ModItem currently will attempt to update an item
even if no field updates are specified.  This patch
avoids (harmless) error messages in the Apache
logs if an item is not actually being changed when it
is placed or taken off reserve.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

bug 8215: (followup) rename GetItemReservesInfo

Renamed that routine to GetItemCourseReservesInfo in
order to avoid any potential confusion with reserves
qua hold requests.

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

bug 8215: (followup) remove extraneous underscore in template

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>