Bug 9303

Summary: relative's checkouts in the opac
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: PatronsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: brendan, cbrannon, chris, gmcharlt, gwilliams, jdemuth, jonathan.druart, jrobb, jsasse, katrin.fischer, kyle.m.hall, kyle, liz, martin.renvoize, mcoalwell, nick, nicole, sandboxes, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9305
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=3591
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9304
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11944
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13790
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13743
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 13790    
Bug Blocks: 15478, 15813, 15842    
Attachments: Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 [QA Followup] - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 [QA Followup] - relative's checkouts in the opac
Bug 9303 [QA Followup 2] - Allow librarians to set preference froms staff intranet
Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 - relative's checkouts in the opac
Bug 9303 [2/2] - relative's checkouts in the opac
Bug 9303 [1/2] - relative's checkouts in the opac - Schema Updates
Bug 9303 [2/2] - relative's checkouts in the opac
Bug 9303 [1/2] - relative's checkouts in the opac - Schema Updates
Bug 9303 [2/2] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Set AUTO_INCREMENT value for issues.issue_id
Bug 9303 [1] - relative's checkouts in the opac - Schema Updates
Bug 9303 [2] - relative's checkouts in the opac - Database Updates
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Set AUTO_INCREMENT value for issues.issue_id
Bug 9303 [QA Followup 2] - Undo changes to GetReserve
Bug 9303 [QA Followup 3] - Add borrowerRelationship syspref check
Bug 9303 [QA Followup 4] - Syspref the feature for staff
Bug 9303 [QA Followup 4] - Syspref the feature for staff
Bug 9303 [1] - relative's checkouts in the opac - Schema Updates
Bug 9303 [2] - relative's checkouts in the opac - Database Updates
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Set AUTO_INCREMENT value for issues.issue_id
Bug 9303 [QA Followup 2] - Undo changes to GetReserve
Bug 9303 [QA Followup 3] - Add borrowerRelationship syspref check
Bug 9303 [QA Followup 4] - Syspref the feature for staff
Bug 9303 [QA Followup 4] - Syspref the feature for staff
Bug 9393: Capitalization fixes
Bug 9303 [1] - relative's checkouts in the opac - Schema Updates
Bug 9303 [2] - relative's checkouts in the opac - Database Updates
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Set AUTO_INCREMENT value for issues.issue_id
Bug 9303 [QA Followup 2] - Undo changes to GetReserve
Bug 9303 [QA Followup 3] - Add borrowerRelationship syspref check
Bug 9303 [QA Followup 4] - Syspref the feature for staff
Bug 9393: Capitalization fixes
Bug 9303 [QA Followup 5] - Show setting on patron details page
Bug 9303 [QA Followup 6] - Allow patrons to set visibility from "your personal details"
Bug 9303 [4] [QA Followup]
Bug 9303 [4] [QA Followup]
Bug 9303 [4] [QA Followup]
Bug 9303 [5] [QA Followup 2]
Bug 9303 [5] [QA Followup 2]
Bug 9303 [5] [QA Followup 2]
Bug 9303 [6] [QA Followup 3] - Rename prefs and field
Bug 9303 [6] [QA Followup 3] - Rename prefs and field
Bug 9303 [7] [QA Followup 4] - Undo changes to prog and ccsr themes
Bug 9303 [5] [QA Followup 2]
Bug 9303 [6] [QA Followup 3] - Rename prefs and field
Bug 9303 [7] [QA Followup 4] - Undo changes to prog and ccsr themes
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates
Bug 9303 [3] - relative's checkouts in the opac
[SIGNED OFF] Bug 9303 [1] - relative's checkouts in the opac - Database Updates
[SIGNED OFF] Bug 9303 [1] - relative's checkouts in the opac - Database Updates
[SIGNED OFF] Bug 9303 [2] - relative's checkouts in the opac - Schema Updates
[SIGNED OFF] Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Don't drop non-existant keys
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Don't drop non-existant keys
Bug 9303 [QA Followup 2] - Fix db update, move 'show' field to guarantor area
Bug 9303 [QA Followup 3]
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Don't drop non-existant keys
Bug 9303 [QA Followup 2] - Fix db update, move 'show' field to guarantor area
Bug 9303 [QA Followup 3]
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Don't drop non-existant keys
Bug 9303 [QA Followup 2] - Fix db update, move 'show' field to guarantor area
Bug 9393 [QA Followup 3] - Remove use of EncodeUTF8
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Don't drop non-existant keys
Bug 9303 [QA Followup 2] - Fix db update, move 'show' field to guarantor area
Bug 9393 [QA Followup 3] - Remove use of EncodeUTF8
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Don't drop non-existant keys
Bug 9303 [QA Followup 2] - Fix db update, move 'show' field to guarantor area
Bug 9393 [QA Followup 3] - Remove use of EncodeUTF8
Bug 9303 [QA Followup] - Restore missing file
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Remove extra 'USE Koha', change "Call number" back to "Call no."
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Remove extra 'USE Koha', change "Call number" back to "Call no."
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Remove extra 'USE Koha', change "Call number" back to "Call no."
Bug 9303 [QA Followup] - Restore missing biblio relationship
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Remove extra 'USE Koha', change "Call number" back to "Call no."
Bug 9303 [QA Followup] - Restore missing biblio relationship
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Remove extra 'USE Koha', change "Call number" back to "Call no."
Bug 9303 [QA Followup] - Restore missing biblio relationship
Bug 9303 [QA Followup] - Restore ability for patron to control setting
Bug 9303 [QA Followup] - Restore ability for patron to control setting
Bug 9303 [QA Followup] - Restore missing svc script
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Remove extra 'USE Koha', change "Call number" back to "Call no."
Bug 9303 [QA Followup] - Restore missing biblio relationship
Bug 9303 [QA Followup] - Restore ability for patron to control setting
Bug 9303 [QA Followup] - Restore missing svc script
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Remove extra 'USE Koha', change "Call number" back to "Call no."
Bug 9303 [QA Followup] - Restore missing biblio relationship
Bug 9303 [QA Followup] - Restore ability for patron to control setting
Bug 9303 [QA Followup] - Restore missing svc script
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Remove extra 'USE Koha', change "Call number" back to "Call no."
Bug 9303 [QA Followup] - Restore missing biblio relationship
Bug 9303 [QA Followup] - Restore ability for patron to control setting
Bug 9303 [QA Followup] - Restore missing svc script
Bug 9303 [1] - relative's checkouts in the opac - Database Updates
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/
Bug 9303 [3] - relative's checkouts in the opac
Bug 9303 [QA Followup] - Remove extra 'USE Koha', change "Call number" back to "Call no."
Bug 9303 [QA Followup] - Restore missing biblio relationship
Bug 9303 [QA Followup] - Restore ability for patron to control setting
Bug 9303 [QA Followup] - Restore missing svc script

Description Nicole C. Engard 2012-12-19 13:33:42 UTC
It would be nice if there were a way to show patrons their relative's checkouts in the OPAC (like you can in the staff client). It should probably be controlled by an option on each patron record - so that patrons can decide if their family can see their checkouts.
Comment 1 Kyle M Hall 2013-11-06 21:04:17 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-11-07 13:14:46 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-11-07 13:16:29 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2013-11-07 13:24:25 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2013-11-07 15:49:29 UTC Comment hidden (obsolete)
Comment 6 Biblibre Sandboxes 2013-11-07 16:15:14 UTC
Patch tested with a sandbox, by Joel Sasse <jsasse@plumcreeklibrary.net>
Comment 7 Biblibre Sandboxes 2013-11-07 16:15:33 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2013-11-07 16:23:30 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2013-11-07 16:35:06 UTC
Kyle, why do you add the new field issue_id? You don't use it in your code.
I mark this patch as failed QA. I did't test it but you modify the structure of the issues table without modifying the old_issues table.
Comment 10 Kyle M Hall 2013-11-07 17:52:08 UTC
(In reply to Jonathan Druart from comment #9)
> Kyle, why do you add the new field issue_id? You don't use it in your code.
> I mark this patch as failed QA. I did't test it but you modify the structure
> of the issues table without modifying the old_issues table.

The issue's table needs a primary key in order to fetch related rows via DBIx::Class. I'm not sure where you say I'm not modifying the old_issues table. I see my changes in both updastedatabase.pl and kohastructure.sql
Comment 11 Kyle M Hall 2013-11-08 12:05:01 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2013-11-08 13:11:41 UTC
(In reply to Kyle M Hall from comment #10)
> The issue's table needs a primary key in order to fetch related rows via
> DBIx::Class. I'm not sure where you say I'm not modifying the old_issues
> table. I see my changes in both updastedatabase.pl and kohastructure.sql

Whaou, yes! I don't know what I did. Maybe I looked to an older patch (where there was no change on old_issues)...
Comment 13 Jonathan Druart 2013-11-13 11:27:15 UTC
Kyle, I don't understand some changes in the DB schema:
"datetime_undef_if_invalid => 1" and "is_deferrable => 1" are removed, is it intentional?
Comment 14 Jonathan Druart 2013-11-13 13:08:05 UTC
I am not sure to understand the test plan.
I did:
- A is guarantor for B
- I check out 1 item to A and 1 item to B
- On the staff interface, I see the relative's checkouts.
- At the OPAC:
log as A: there is a relative's checkouts tab but with an empty table.
(debug: opac-user.pl l.358: warn Dumper \@borrowernumbers; displays [borrowernumber_B] and @relatives contains 1 object Koha::Schema::Result::Borrower)
log as B: there is no relative's checkouts tab.
(debug: \@borrowernumbers => [borrowernumber_A], @relatives is empty)

Marked as Failed QA.
Comment 15 Kyle M Hall 2013-11-15 16:48:11 UTC
(In reply to Jonathan Druart from comment #13)
> Kyle, I don't understand some changes in the DB schema:
> "datetime_undef_if_invalid => 1" and "is_deferrable => 1" are removed, is it
> intentional?

That is just a product of the dbix class schema re-generator. It updates it based on the schema, so I imagine we should keep those changes.
Comment 16 Kyle M Hall 2013-11-15 16:57:22 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2013-11-15 16:57:53 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2013-11-15 16:58:06 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2013-11-15 17:09:51 UTC
(In reply to Jonathan Druart from comment #14)
> I am not sure to understand the test plan.
> I did:
> - A is guarantor for B
> - I check out 1 item to A and 1 item to B
> - On the staff interface, I see the relative's checkouts.
> - At the OPAC:
> log as A: there is a relative's checkouts tab but with an empty table.
> (debug: opac-user.pl l.358: warn Dumper \@borrowernumbers; displays
> [borrowernumber_B] and @relatives contains 1 object
> Koha::Schema::Result::Borrower)
> log as B: there is no relative's checkouts tab.
> (debug: \@borrowernumbers => [borrowernumber_A], @relatives is empty)
> 
> Marked as Failed QA.

I was able to test this just fine in a sandbox. Try http://catalogue.test8.biblibre.com/cgi-bin/koha/opac-user.pl

The users/passwords are ace, ron, and lisa. Admin is "test" as usual.

I don't know how long this will stay up before someone blows it away of course.
Comment 20 Kyle M Hall 2013-11-15 19:58:47 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2013-11-15 19:59:34 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2013-11-18 10:20:39 UTC
Hi Kyle,
I like that it is an option for the borrower to set, but could we also tie the whole feature to a system preference maybe? I am a bit worried about data privacy implications and that would make it a bit easier.
Comment 23 Kyle M Hall 2013-11-22 15:03:46 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2013-11-22 15:16:10 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2013-11-22 15:21:36 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2013-11-22 15:47:04 UTC Comment hidden (obsolete)
Comment 27 Biblibre Sandboxes 2013-11-22 15:56:13 UTC
Patch tested with a sandbox, by Heather Braum <hbraum@nekls.org>
Comment 28 Biblibre Sandboxes 2013-11-22 15:56:33 UTC Comment hidden (obsolete)
Comment 29 Jonathan Druart 2013-12-13 15:04:06 UTC
(In reply to Katrin Fischer from comment #22)
> Hi Kyle,
> I like that it is an option for the borrower to set, but could we also tie
> the whole feature to a system preference maybe? I am a bit worried about
> data privacy implications and that would make it a bit easier.

Kyle,
I don't see this change in your last patch. Did you see the Katrin's comment?
And could you please split your patch in, at least, 2 patches: 1 for DB changes and 1 for rest of the feature.

Marked as Failed QA.
Comment 30 Kyle M Hall 2013-12-13 17:34:29 UTC
(In reply to Katrin Fischer from comment #22)
> Hi Kyle,
> I like that it is an option for the borrower to set, but could we also tie
> the whole feature to a system preference maybe? I am a bit worried about
> data privacy implications and that would make it a bit easier.

Can you expand a bit? The option is off by default, and if you opt to leave opacprivacy disabled, there's no even a way to turn it on. I guess I want to avoid YASP syndrome without absolute necessity.

If you can describe to me a way that it could cause a privacy concern in its current state, I'll be more than happy to address the issue!
Comment 31 Kyle M Hall 2013-12-13 17:42:38 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2013-12-13 17:43:02 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2013-12-13 17:43:25 UTC Comment hidden (obsolete)
Comment 34 Katrin Fischer 2013-12-14 11:25:07 UTC
I am just super careful with these kind of things as in Germany data privacy is a big topic. We are not using OpacPrivacy right now, so this would not be a big problem. I can only imagine that some libraries don't want to offer this option, but are using parent/child accounts, but I am not going to block this.
Comment 35 Jonathan Druart 2013-12-19 11:16:25 UTC
(In reply to Kyle M Hall from comment #19)
> I was able to test this just fine in a sandbox. Try
> http://catalogue.test8.biblibre.com/cgi-bin/koha/opac-user.pl
> 
> The users/passwords are ace, ron, and lisa. Admin is "test" as usual.
> 
> I don't know how long this will stay up before someone blows it away of
> course.

Sandboxes are reset all nights.
Could you confirm (or not), the tab "relatives's checkouts" should have the same contain on both interfaces?
Comment 36 Katrin Fischer 2013-12-29 10:51:53 UTC
Comment on attachment 23526 [details] [review]
Bug 9303 [1/2] - relative's checkouts in the opac - Schema Updates

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

Hi Kyle, starting with a code review:

::: Koha/Schema/Result/Borrower.pm
@@ -191,4 @@
>  =head2 dateofbirth
>  
>    data_type: 'date'
> -  datetime_undef_if_invalid: 1

Can you explain why those lines have been removed by your patch? I know that those files are generated automatically, but I have difficulty to see how those changes are related to the canges done in kohastructure.sql.

@@ +1020,2 @@
>  
> +# Created by DBIx::Class::Schema::Loader v0.07000 @ 2013-11-07 08:15:21

I notice, that your version is a bit older than the one used before, could that explain some of the differences?

::: installer/data/mysql/updatedatabase.pl
@@ +7826,5 @@
>  
> +$DBversion = "3.15.00.XXX";
> +if(CheckVersion($DBversion)) {
> +    $dbh->do(q{
> +        ALTER TABLE borrowers ADD privacy_relative_checkouts BOOLEAN NOT NULL DEFAULT '0'

I'd feel a bit better about this line and the next, if there was an AFTER/BEFORE making sure that the sequence of rows is the same for old and new installations.
Comment 37 Katrin Fischer 2013-12-29 11:08:55 UTC
Comment on attachment 23527 [details] [review]
Bug 9303 [2/2] - relative's checkouts in the opac

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

No major findings - mostly noting a few things:

I think maybe it would be better to separate the Schema and kohastructure/updatedatabase changes into 2 separate patches.
Also adding a new PK to issues and old_issues could have been a separate bug/patch.

::: C4/Reserves.pm
@@ +270,5 @@
> +        $sth->execute($reserve_id);
> +        $res = $sth->fetchrow_hashref();
> +    }
> +
> +    carp("No hold for for reserve_id $reserve_id") unless $res;

Hm, this change seems unrelated to the bug description?

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
@@ +1168,5 @@
> +                    <option value="1">Yes</option>
> +                [% END %]
> +            </select>
> +            <div class="hint">Allow linked patron accounts to view this patron's checkouts from the OPAC</div>
> +        </li>

This is different to how we handle the other privacy seetings in that we don't allow staff to change those for a patron. I see why staff will want to change that setting for a parent requesting it, but at the same time it bothers me a bit.

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
@@ +1,3 @@
>  [% USE Koha %]
>  [% USE KohaDates %]
> +[% USE EncodeUTF8 %]

The only script using this so far is renew.pl - can you explain a bit?
Comment 38 Katrin Fischer 2013-12-29 11:18:25 UTC
Ok, found a major problem:

New PK for issues and old_issues:
- Sequential numbering done by updatedatabase worked correctly in
  my test database.
- Returning checked out items moves them correctly to old_issues,
  issue_id is kept.
- Checking out a new item after the updates generates an issue_id
  that was already used in old_issues!

In my database, afer update:
old_issues - 1-18
issues - 19-20
- returned 19, 20
old_issues 1-20
issues empty
- checked out 2 items
old_issues 1-20
issues 3,4

Please consider splitting the introduction of a new PK into a separate bug report.
Comment 39 Kyle M Hall 2014-01-08 12:16:48 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2014-01-08 12:17:06 UTC Comment hidden (obsolete)
Comment 41 Kyle M Hall 2014-01-08 12:59:28 UTC Comment hidden (obsolete)
Comment 42 Kyle M Hall 2014-01-08 13:05:57 UTC
(In reply to Katrin Fischer from comment #38)
> In my database, afer update:
> old_issues - 1-18
> issues - 19-20
> - returned 19, 20
> old_issues 1-20
> issues empty
> - checked out 2 items
> old_issues 1-20
> issues 3,4

My bad! I forget to update the auto_increment value. Silly me expected mysql to use the max(issue_id) + 1 by default ; )
Comment 43 Katrin Fischer 2014-01-25 10:54:48 UTC
Kyle, please also check my other comments (comment 36 and comment 37). 

I would feel better about this if you split the first patch, so that the "traditional" database update parts and the Schema update were separate.
Comment 44 Kyle M Hall 2014-01-27 17:53:30 UTC Comment hidden (obsolete)
Comment 45 Kyle M Hall 2014-01-27 17:53:36 UTC Comment hidden (obsolete)
Comment 46 Kyle M Hall 2014-01-27 17:53:41 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2014-01-27 17:53:47 UTC Comment hidden (obsolete)
Comment 48 Katrin Fischer 2014-01-27 22:44:41 UTC
Hi Kyle, I can see that you split the database update, but not sure which other changes you included - could you leave a comment about my comments?
Comment 49 Kyle M Hall 2014-01-28 15:36:20 UTC
(In reply to Katrin Fischer from comment #48)
> Hi Kyle, I can see that you split the database update, but not sure which
> other changes you included - could you leave a comment about my comments?

No changes were made, I split the patch in two, with all schema file updates in one patch, and the database updates in a second patch. The last 2 patches are unchanged. I had to renumber them due to the splitting.

Hope this answers your question!
Comment 50 Katrin Fischer 2014-01-28 15:47:36 UTC
I had asked a few more questions in comment 37 - especially about a change related to reserves.
Comment 51 Kyle M Hall 2014-02-03 16:27:31 UTC Comment hidden (obsolete)
Comment 52 Kyle M Hall 2014-02-03 16:42:25 UTC
(In reply to Katrin Fischer from comment #37)

> ::: C4/Reserves.pm
> @@ +270,5 @@
> > +        $sth->execute($reserve_id);
> > +        $res = $sth->fetchrow_hashref();
> > +    }
> > +
> > +    carp("No hold for for reserve_id $reserve_id") unless $res;
> 
> Hm, this change seems unrelated to the bug description?

Yep, looks like an accidental commit. I added another followup to revert that code change.

> ::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
> @@ +1168,5 @@
> > +                    <option value="1">Yes</option>
> > +                [% END %]
> > +            </select>
> > +            <div class="hint">Allow linked patron accounts to view this patron's checkouts from the OPAC</div>
> > +        </li>
> 
> This is different to how we handle the other privacy seetings in that we
> don't allow staff to change those for a patron. I see why staff will want to
> change that setting for a parent requesting it, but at the same time it
> bothers me a bit.

This is a specific requirement of the sponsor, specifically so that a guarantor can request to be able to view a guaranteed accounts checkouts without the need to log into the opac as that guaranteed patron to make the change. This option can easily be hidden via jquery if needed.
 
> ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
> @@ +1,3 @@
> >  [% USE Koha %]
> >  [% USE KohaDates %]
> > +[% USE EncodeUTF8 %]
> 
> The only script using this so far is renew.pl - can you explain a bit?

This is a simple filter plugin that encodes the given variable to utf8. It addresses some of the encoding issues we have in Koha. Right now our encoding of data to utf is mixed in throughout the code base when it really should be at one of two places, the very beginning ( e.g. database queries ) or at the very end ( e.g. template output ). This plugin allows us to handle utf8 encoding from the very end of the output chain. In fact, it would be even better if we could configure Koha's TT to filter every value though this filter before outputting. At this time however, I have been unable to find a way to do so. I'm certain we could do this by extending Template::Parser but I'm not sure if we want to go that far.
Comment 53 Katrin Fischer 2014-02-03 21:31:00 UTC
Hi Kyle, as this is a data privacy relevant feature jQuery would not suffice to hide as it would still be possible to just turn Javasript off to change the setting. But that's just a note - I am going to test again ;)
Comment 54 HB-NEKLS 2014-02-04 05:23:30 UTC
Katrin, just to help in the explanation, since Kyle mentioned the sponsor requirement in his most recent comment back to you: 

I represent the organization that is sponsoring this development and since we requested the additional setting on the staff side to be able to turn this feature on and off through a borrower account, we are having to do this, because if you have the sys pref OPACPrivacy set to Don't Allow (which we have), patrons have no ability to even turn this new feature on/off in an OPAC account (if I remember correctly from our testing a few months ago). We have that sys pref turned off because it currently wrecks havoc on checkout history links for items if patrons "opt-out" of checkout history being left. In a consortia of 42 libraries with materials going everywhere, we have to have that checkout history to track down materials. 

Additionally, if you have a guarantor with 2 or more kids, it's much easier for this feature to be turned on from the staff side as well -- instead of having to log into each guarantee's account to turn this feature on. We thought adding the ability to turn this on and off from the staff side would actually help out patrons. 

I understand the privacy concerns -- we have them too -- and I'll let you all continue to sort this out, but I wanted to explain where we're coming from as the sponsor of this development.
Comment 55 Katrin Fischer 2014-02-04 05:38:44 UTC
Hi Heather, thank you for taking the time to explain. I understand why it makes sense to have the option in the patron account. I might be a bit overly careful at times as we are quite strict with data privacy here. I just couldn't resist to point out that jQuery won't be a solution if you might not be allowed to use this feature for privacy concerns. There is no staff permission and no pref to turn it totally off currently.
Comment 56 Galen Charlton 2014-02-04 16:35:30 UTC
(In reply to Kyle M Hall from comment #30)
> (In reply to Katrin Fischer from comment #22)
> > Hi Kyle,
> > I like that it is an option for the borrower to set, but could we also tie
> > the whole feature to a system preference maybe? I am a bit worried about
> > data privacy implications and that would make it a bit easier.
> 
> Can you expand a bit? The option is off by default, and if you opt to leave
> opacprivacy disabled, there's no even a way to turn it on. I guess I want to
> avoid YASP syndrome without absolute necessity.

There is a difference between retaining circ history (and OpacPrivacy might be batter called PatronCircHistoryRetentionPolicy) and making  patron's information available to other patrons.

As you know, I'm not a fan of increasing the number of system preferences without due cause -- but in this case, YASP is called.

Katrin can speak more to the specifics, but my understanding is that European data privacy laws are a Big Legal Deal, to the point that I feel strongly that we should give weight to concerns raised by Koha users who work in those jurisdictions.
Comment 57 Galen Charlton 2014-02-04 16:39:06 UTC
(In reply to Katrin Fischer from comment #55)
> Hi Heather, thank you for taking the time to explain. I understand why it
> makes sense to have the option in the patron account. I might be a bit
> overly careful at times as we are quite strict with data privacy here. I
> just couldn't resist to point out that jQuery won't be a solution if you
> might not be allowed to use this feature for privacy concerns. There is no
> staff permission and no pref to turn it totally off currently.

+1 to the general point.
Comment 58 Katrin Fischer 2014-02-07 14:30:20 UTC
Hi Kyle,

could you please syspref the feature in staff/intranet? Additionally I think the options in the patron account shoudl only be visible when BorrowerRelationships is not empty.
Comment 59 Kyle M Hall 2014-02-24 14:11:36 UTC Comment hidden (obsolete)
Comment 60 Kyle M Hall 2014-02-24 14:32:22 UTC Comment hidden (obsolete)
Comment 61 Kyle M Hall 2014-02-24 14:35:46 UTC Comment hidden (obsolete)
Comment 62 Katrin Fischer 2014-04-09 09:41:01 UTC
Hi Kyle, could you please rebase? Sorry it took me so long to get back to this. I fixed a conflict in the third patch, but then there is another on the last, that I can't resolve:

Applying: Bug 9303 [QA Followup] - Set AUTO_INCREMENT value for issues.issue_id
Applying: Bug 9303 [QA Followup 2] - Undo changes to GetReserve
Applying: Bug 9303 [QA Followup 3] - Add borrowerRelationship syspref check
Applying: Bug 9303 [QA Followup 4] - Syspref the feature for staff
fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 9303 [QA Followup 4] - Syspref the feature for staff
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 bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-9303-QA-Followup-4---Syspref-the-feature-for-s-hwC6Eh.patch
Comment 63 Kyle M Hall 2014-04-24 14:32:04 UTC Comment hidden (obsolete)
Comment 64 Kyle M Hall 2014-04-24 14:32:19 UTC Comment hidden (obsolete)
Comment 65 Kyle M Hall 2014-04-24 14:32:22 UTC Comment hidden (obsolete)
Comment 66 Kyle M Hall 2014-04-24 14:32:24 UTC Comment hidden (obsolete)
Comment 67 Kyle M Hall 2014-04-24 14:32:27 UTC Comment hidden (obsolete)
Comment 68 Kyle M Hall 2014-04-24 14:32:29 UTC Comment hidden (obsolete)
Comment 69 Kyle M Hall 2014-04-24 14:32:31 UTC Comment hidden (obsolete)
Comment 70 Katrin Fischer 2014-04-28 05:39:29 UTC
> There is a difference between retaining circ history (and OpacPrivacy might
> be batter called PatronCircHistoryRetentionPolicy) and making  patron's
> information available to other patrons.
> 

The new system preference (AllowStaffToSetRelativesCheckoutsVisibility) will allow to disallow staff to change the relative's checkouts sharing in OPAC. I like that, but I still see a problem with the reading history and the relative's checkout being tied together. If you allow one, you HAVE to allow the other - but both are quite different features. I think both can be on the same page, but you should be able to configure both separately.
Comment 71 Katrin Fischer 2014-04-28 05:41:24 UTC
(In reply to Katrin Fischer from comment #58)
> Hi Kyle,
> 
> could you please syspref the feature in staff/intranet? Additionally I think
> the options in the patron account shoudl only be visible when
> BorrowerRelationships is not empty.

I might have been a cause of confusion here... I am not sure now if I had meant to type staff/opac :(

I am testing this further, but I still have concerns about the 2 features being tied together.
Comment 72 Katrin Fischer 2014-04-28 06:28:39 UTC Comment hidden (obsolete)
Comment 73 Katrin Fischer 2014-04-28 06:28:55 UTC Comment hidden (obsolete)
Comment 74 Katrin Fischer 2014-04-28 06:41:31 UTC
Attaching 2 patches - one follow up with capitalization changes and an amended patch for the system preference, putting sysprefs.sql in correct alphabetic order.

I am not sure I can finish this today, so here is what I have tested so far:

1) issue_id added to issues and old_issues
- Checked numbering was correct, starting with old_issues and continueing in issues
- Checked in an item, verified it got moved to old_issues correctly.
- Checked out an item, verified next unused issue_id was assigned

2) AllowStaffToSetRelativesCheckoutsVisibility
- off by default on new installations, option doesn't show on memberentry
- If activated, option can be set and saves correctly
- [BUG] The new setting doesn't show on the patron's detail page. The privacy
  setting for the reading history does. This is important as some libraries print
  this page, when patrons ask what is stored about them.

3) OPAC: bootstrap
- relative's checkouts display after being allowed to

4) OPAC: prog
- relative's checkouts display after being allowed to

5) Turning off OPAC privacy
- [BUG?] When OPAC privacy is turned off, the patron is left without an option to deactivate the visibility of his/her account to others - the checkouts still show.
Comment 75 Katrin Fischer 2014-04-28 16:06:25 UTC
I had hoped someone would take this on where I have left off - I think 5) is a blocker.
Comment 76 Kyle M Hall 2014-05-09 17:22:11 UTC Comment hidden (obsolete)
Comment 77 Kyle M Hall 2014-05-09 17:22:43 UTC Comment hidden (obsolete)
Comment 78 Kyle M Hall 2014-05-09 17:22:46 UTC Comment hidden (obsolete)
Comment 79 Kyle M Hall 2014-05-09 17:22:49 UTC Comment hidden (obsolete)
Comment 80 Kyle M Hall 2014-05-09 17:22:52 UTC Comment hidden (obsolete)
Comment 81 Kyle M Hall 2014-05-09 17:22:55 UTC Comment hidden (obsolete)
Comment 82 Kyle M Hall 2014-05-09 17:22:58 UTC Comment hidden (obsolete)
Comment 83 Kyle M Hall 2014-05-09 17:23:01 UTC Comment hidden (obsolete)
Comment 84 Kyle M Hall 2014-05-09 17:23:04 UTC Comment hidden (obsolete)
Comment 85 Kyle M Hall 2014-05-09 17:27:59 UTC
> - [BUG] The new setting doesn't show on the patron's detail page. The privacy
>   setting for the reading history does. This is important as some libraries
> print
>   this page, when patrons ask what is stored about them.

Fixed!

> 5) Turning off OPAC privacy
> - [BUG?] When OPAC privacy is turned off, the patron is left without an
> option to deactivate the visibility of his/her account to others - the
> checkouts still show.

I would *not* consider this a bug. If OPAC privacy was turned off, the patron would need to go to a librarian to have it enabled in the first place. In that case, I see no issue with the patron needing to have a librarian to disable it in the same way.

Setting to "In Discussion" for now. Katrin, please set to failed or passed qa.
Comment 86 Kyle M Hall 2014-06-16 15:03:03 UTC
It's been a month with no reply, setting to passed qa.

Kyle
Comment 87 Katrin Fischer 2014-06-16 19:22:29 UTC
I have just retested this and after giving it some more thought... I am still worried. My point is:

As a user, I should be able to find out if my checkouts are visible to others at any time. If the library opts to disable OpacPrivacy (and I still think it's not a good thing to tie different functionalities to the same pref), I don't see anywhere in my account that my data is shared with others.

Please feel free to get more opinions, eyes on this. I don't want to be unreasonable and I hope I am not.
Comment 88 Mickey Coalwell 2014-06-16 19:40:45 UTC
Katrin, while I appreciate your point of view, it does seem that you didn't address the point that Kyle made a month ago, i.e.,

"I would *not* consider this a bug. If OPAC privacy was turned off, the patron would need to go to a librarian to have it enabled in the first place. In that case, I see no issue with the patron needing to have a librarian to disable it in the same way."

I agree with Kyle, and share that point of view. At some point, the library simply has to be allowed/trusted to manage the library user's account in an appropriate manner. 

Barring any specific (rather than philosophical) objection to this view, I would ask that you reconsider your decision to mark this FAILED QA. This discussion has dragged on long enough, and I don't see that it's gaining us anything to go back and forth in the absence of any support for your view or a consensus arising from a compelling debate. 

Will you agree to allow this to development to move forward at this time? Thank you.

Mickey
Comment 89 Galen Charlton 2014-06-16 20:00:06 UTC
(In reply to Mickey Coalwell from comment #88)
> I agree with Kyle, and share that point of view. At some point, the library
> simply has to be allowed/trusted to manage the library user's account in an
> appropriate manner. 

Not all legal jurisdictions would agree, and it would not surprise me if there are some where there is an absolute right, even for juveniles, to prevents relatives from seeing checkouts.

I believe that Koha should respect stronger data privacy laws when applicable, and this is one such case.

I believe that a resolution is the move the OPAC page for toggling privacy_relative_checkout so that it is not dependent on the value OpacPrivacy.  This change would be small and would allow general circ privacy to be orthogonal to linked patron checkout visibility.
Comment 90 Chris Cormack 2014-06-16 20:01:20 UTC
(In reply to Galen Charlton from comment #89)
> (In reply to Mickey Coalwell from comment #88)
> > I agree with Kyle, and share that point of view. At some point, the library
> > simply has to be allowed/trusted to manage the library user's account in an
> > appropriate manner. 
> 
> Not all legal jurisdictions would agree, and it would not surprise me if
> there are some where there is an absolute right, even for juveniles, to
> prevents relatives from seeing checkouts.
> 
> I believe that Koha should respect stronger data privacy laws when
> applicable, and this is one such case.
> 
> I believe that a resolution is the move the OPAC page for toggling
> privacy_relative_checkout so that it is not dependent on the value
> OpacPrivacy.  This change would be small and would allow general circ
> privacy to be orthogonal to linked patron checkout visibility.

I agree with this entirely.
Comment 91 Owen Leonard 2014-06-16 20:03:58 UTC
Picture this scenario:

1. Husband and wife enable each other to see the other's checkouts.
2. Marriage goes bad, wife wants to check out books on divorce.
3. Library has turned off OpacPrivacy, wife can't block husband from seeing her checkouts.

It's not hard to see situations like this where this bug could have very real and negative effects on people's use of the library. I agree that this is a bug and there should be a way to control it separate from OpacPrivacy.
Comment 92 Mickey Coalwell 2014-06-16 20:08:09 UTC
(In reply to Owen Leonard from comment #91)
> Picture this scenario:
> 
> 1. Husband and wife enable each other to see the other's checkouts.
> 2. Marriage goes bad, wife wants to check out books on divorce.
> 3. Library has turned off OpacPrivacy, wife can't block husband from seeing
> her checkouts.
> 
> It's not hard to see situations like this where this bug could have very
> real and negative effects on people's use of the library. I agree that this
> is a bug and there should be a way to control it separate from OpacPrivacy.

Excellent points, and Owen's (and others') suggestion sounds good to me. Kyle, is providing the necessary control through OpacPrivacy a solution you can live with? If so, can we move ahead on that basis with this development? Thank you all for your input. 

Mickey
Comment 93 Kyle M Hall 2014-06-17 16:29:30 UTC Comment hidden (obsolete)
Comment 94 Jason Robb 2014-06-17 17:00:08 UTC
I'm a little late to this party but I wanted clarification and to express a concern we have.

Currently in the staff version of this, not only can guarantors see guarantee checkouts, but if multiple guarantees are linked to one guarantor, the kids can see each other's checkouts.

Put into a real scenario: a parent(A) has two guarantees, a child(B) and a foster child(C).

For this situation we'd want A to see B & C's checkouts, but we don't want B to see C's checkouts or the opposite. It doesn't seem like that's possible right now if I'm reading correctly. If B wants to share with A, they must also share with C.

It would solve a lot of possible issues and create flexibility for different family situations to have it set up like:
Allow Relative A to see checkouts? Y/N
Allow Relative B to see checkouts? Y/N
Allow Relative C to see checkouts? Y/N

I don't want to throw additional wrenches into this as it's been through enough but maybe for future consideration?
Comment 95 Chris Cormack 2014-06-17 21:55:08 UTC
(In reply to Kyle M Hall from comment #93)
> Created attachment 28912 [details] [review] [review]
> Bug 9303 [QA Followup 6] - Allow patrons to set visibility from "your
> personal details"

While this now decouples from the opac privacy, there is now no way to turn this feature on or off. It's on all the time.

Was that the desired outcome?
Comment 96 Chris Cormack 2014-06-17 22:02:33 UTC
(In reply to Jason Robb from comment #94)
> I'm a little late to this party but I wanted clarification and to express a
> concern we have.
> 
> Currently in the staff version of this, not only can guarantors see
> guarantee checkouts, but if multiple guarantees are linked to one guarantor,
> the kids can see each other's checkouts.
> 
> Put into a real scenario: a parent(A) has two guarantees, a child(B) and a
> foster child(C).
> 
> For this situation we'd want A to see B & C's checkouts, but we don't want B
> to see C's checkouts or the opposite. It doesn't seem like that's possible
> right now if I'm reading correctly. If B wants to share with A, they must
> also share with C.
> 
> It would solve a lot of possible issues and create flexibility for different
> family situations to have it set up like:
> Allow Relative A to see checkouts? Y/N
> Allow Relative B to see checkouts? Y/N
> Allow Relative C to see checkouts? Y/N
> 
> I don't want to throw additional wrenches into this as it's been through
> enough but maybe for future consideration?

I also agree with this, younger brother seeing an older sisters (and vice versa) reading list could be awkward and undesired.
Comment 97 Liz Rea 2014-06-18 00:16:28 UTC
Eek,yes, this should work just as the staff client does, that a guarantee only sees their own issues, and a guarantor sees all guarantee's issues.

Another example is in a corporate library, the supervisor may want to see all subordinates issues, but each subordinate should have the expectation of privacy. This feature isn't only used for familial relationships.

Liz
Comment 98 Owen Leonard 2014-06-18 13:18:18 UTC
(In reply to Liz Rea from comment #97)
> Eek,yes, this should work just as the staff client does, that a guarantee
> only sees their own issues, and a guarantor sees all guarantee's issues.

I agree.

> Allow Relative A to see checkouts? Y/N
> Allow Relative B to see checkouts? Y/N
> Allow Relative C to see checkouts? Y/N

This system, while it might be useful to some, is an enhancement outside the scope of this bug.
Comment 99 Owen Leonard 2014-06-18 14:46:38 UTC
In my opinion this would be a good time to get a single squashed patch to re-test and get a fresh signoff. With so many follow-ups I find it hard to track what really needs to be tested.
Comment 100 Kyle M Hall 2014-06-23 12:01:35 UTC
(In reply to Chris Cormack from comment #95)
> (In reply to Kyle M Hall from comment #93)
> > Created attachment 28912 [details] [review] [review] [review]
> > Bug 9303 [QA Followup 6] - Allow patrons to set visibility from "your
> > personal details"
> 
> While this now decouples from the opac privacy, there is now no way to turn
> this feature on or off. It's on all the time.
> 
> Was that the desired outcome?

Yes, I don't think there is a need for YASP, as the borrower flag that controls this is off by default.

Kyle
Comment 101 Kyle M Hall 2014-06-23 12:30:51 UTC
(In reply to Liz Rea from comment #97)
> Eek,yes, this should work just as the staff client does, that a guarantee
> only sees their own issues, and a guarantor sees all guarantee's issues.

But that's not how it works in the staff client! In the staff client, viewing a guarantee will show all guarantors checkouts. I just retested this assumption and it still holds.
Comment 102 Kyle M Hall 2014-06-23 13:03:59 UTC Comment hidden (obsolete)
Comment 103 Kyle M Hall 2014-06-23 13:16:12 UTC Comment hidden (obsolete)
Comment 104 Kyle M Hall 2014-06-23 13:20:11 UTC
> Yes, I don't think there is a need for YASP, as the borrower flag that
> controls this is off by default.

After talking to Katrin, I take this back ; )
Comment 105 Kyle M Hall 2014-06-23 13:45:49 UTC
Now that I've squashed all the followups together, would someone be able to sign off on the patch?

* Test the syspref for allowing staff to set relatives' checkouts visibility
* Test the ability to set relatives' checkouts visibility from opac patron details page, and the syspref that enables it.
* Ensure only guarantors can see checkouts of guarantees, and not visa versa
* Only show the relatives' checkouts in the OPAC if either of the new system preferences is enabled.
Comment 106 Katrin Fischer 2014-06-23 13:53:43 UTC
Thx Kyle for incorporating the feedback in the new set of patches!
Comment 107 Kyle M Hall 2014-06-25 12:36:41 UTC Comment hidden (obsolete)
Comment 108 Christopher Brannon 2014-06-25 22:54:01 UTC
Kyle,
I'm trying this out on a sandbox.  I have:

AllowPatronToSetRelativesCheckoutsVisibility -> ALLOW
OPACPrivacy -> ALLOW
AllowStaffToSetRelativesCheckoutsVisibility -> ALLOW

Set patron B as child
Set patron A as guarantor of B

Checked out item to patron A
Checked out item to patron B

Logged in as patron A on the OPAC and set Allow relatives to view your current checkouts to YES.
Logged in as patron B on the OPAC and set Allow relatives to view your current checkouts to YES.

Logged in as patron A on the OPAC.  Could see patron B checkouts.  GOOD
Logged in as patron B on the OPAC.  Could NOT see patron A checkouts.  GOOD

It is a little confusing (as a parent), to see and give permission in the OPAC to Allow relatives to view your current checkouts.  What's missing is an understanding from the parent's point of view that they will NEVER be able to allow their kids to see what they have checked out.

Should this option even be available to the patron unless they are listed as someone's guarantee?  Or should that be spelled out better next to the permission?  It seems that the permission should rather say "Allow guarantor(parent) to view your current checkouts.  And then there should be a status next to the permission and in the personal details tab that says "Your guarantor is: (GUARANTOR NAME/NOT SPECIFIED)".

While this could be classified as a separate enhancement, I think it would be better to be clear from the get-go rather than introduce a feature with ambiguity.

With that said, here are some other issues I ran into:

Katrin's comment in comment 73 still needs to be addressed.  I do see that Call No. exists in the Checked out tab, so if it is going to be changed on the Relatives' checkouts tab, it needs to be changed in both.

I set the AllowPatronToSetRelativesCheckoutsVisibility permission to Don't Allow and the option doesn't go away.  Patrons are still able to set this option in the OPAC.  This is the case in all three themes.  BUSTED

Because the above is busted, I can't confirm if the patron can still see what the privacy is set to if AllowPatronToSetRelativesCheckoutsVisibility is set to Don't Allow.

I set the AllowStaffToSetRelativesCheckoutsVisibility to Don't Allow and this appears to work properly.

Because comment 73 isn't addressed (that I can see) and the AllowPatronToSetRelativesCheckoutsVisibility preference doesn't work, I am setting the status to failed.  :(
Comment 109 Christopher Brannon 2014-06-25 23:01:25 UTC
One other small observation.  It seems we keep bouncing between the terms of relatives and guarantor/guarantee.

If the purpose of this feature encompasses more than parents and children, I think the term "relative" needs to be ditched and changed to "guarantor" in the labels (moremember.pl) and in all permissions (memberentry.pl, preferences.pl, opac-privacy.pl).  It should also be changed out on the tabs on opac-user.pl.

Just my nit-picky two cents.

Christopher
Comment 110 Katrin Fischer 2014-06-26 05:49:17 UTC
Hi Christopher, thx for your testing!
Comment 111 Kyle M Hall 2014-06-26 16:37:29 UTC
> It is a little confusing (as a parent), to see and give permission in the
> OPAC to Allow relatives to view your current checkouts.  What's missing is
> an understanding from the parent's point of view that they will NEVER be
> able to allow their kids to see what they have checked out.

> Should this option even be available to the patron unless they are listed as
> someone's guarantee?  Or should that be spelled out better next to the
> permission?  It seems that the permission should rather say "Allow
> guarantor(parent) to view your current checkouts.  And then there should be
> a status next to the permission and in the personal details tab that says
> "Your guarantor is: (GUARANTOR NAME/NOT SPECIFIED)".

I agree, the option shouldn't even be presented to a patron that isn't guaranteed by another patron. I can also add the guarantor name.
 
> Katrin's comment in comment 73 still needs to be addressed.  I do see that
> Call No. exists in the Checked out tab, so if it is going to be changed on
> the Relatives' checkouts tab, it needs to be changed in both.

I'll take care of this.

> I set the AllowPatronToSetRelativesCheckoutsVisibility permission to Don't
> Allow and the option doesn't go away.  Patrons are still able to set this
> option in the OPAC.  This is the case in all three themes.  BUSTED

Please read the details of the system preference. It only controls the option from opac-memberentry.pl, and does *not* affect it being available from opac-privacy.pl. BUSTED ;)=

However, I see no reason to not to have the setting control it in both places. In fact, I think also that we should hide it from "your personal details" if OPACPrivacy is enabled. No point in having the setting in two different places!
Comment 112 Kyle M Hall 2014-06-26 16:37:58 UTC Comment hidden (obsolete)
Comment 113 Kyle M Hall 2014-06-26 16:41:15 UTC Comment hidden (obsolete)
Comment 114 Kyle M Hall 2014-06-26 17:03:49 UTC Comment hidden (obsolete)
Comment 115 Kyle M Hall 2014-06-26 17:04:02 UTC Comment hidden (obsolete)
Comment 116 Kyle M Hall 2014-06-26 17:15:11 UTC Comment hidden (obsolete)
Comment 117 Christopher Brannon 2014-06-26 17:22:03 UTC
(In reply to Kyle M Hall from comment #113)

As far as bootstrap goes:

> * Hides the ability to set visibility of checkouts if patron has no guarantor

GOOD

> * Hides the setting on "your personal details" if OPACPrivacy is enabled

GOOD

> * Adds the name of the guarantor near the setting on both opac pages

GOOD

> * Changes "Call No." to "Call number". There is plenty of space for the
>   real word, so why not use it?

GOOD

I failed due to results in ccsr and prog themes.  I know these themes are being phased out, but as long as these themes remain it use and are accessible, I think they all need to be addressed.

I am also failing because the setting shows in "my privacy" opac-privacy.pl and "my personal details" opac-memberentry.pl under ccsr and prog whether setting is on or off.

I still think the tabs in all three themes should read "Guarantees' checkouts" rather than "Relatives' checkouts".

Thanks for addressing the terminology issues that you have so far.  :)

I know you are still working on some stuff, so I am replying specifically to what you have done so far.

Christopher
Comment 118 Christopher Brannon 2014-06-26 17:30:35 UTC
(In reply to Kyle M Hall from comment #116)
> Created attachment 29290 [details] [review] [review]
> Bug 9303 [6] [QA Followup 3] - Rename prefs and field
> 
> * Change AllowStaffToSetRelativesCheckoutsVisibility to
> AllowStaffToSetCheckoutsVisibilityForGuarantor
> * Change AllowPatronToSetRelativesCheckoutsVisibility to
> AllowPatronToSetCheckoutsVisibilityForGuarantor
> * Change borrowers field privacy_relative_checkouts to
> privacy_guarantor_checkouts

Looks good.  Thanks for changing.  :)

Christopher
Comment 119 Katrin Fischer 2014-06-26 17:33:52 UTC
Hi Kyle and Christopher,
the prog and ccsr themes are officially deprecated now - I think best would probably be to remove the proc and ccsr files from the patch totally as we don't want to add any new features to them.
Comment 120 Christopher Brannon 2014-06-26 17:38:43 UTC
(In reply to Katrin Fischer from comment #119)
> Hi Kyle and Christopher,
> the prog and ccsr themes are officially deprecated now - I think best would
> probably be to remove the proc and ccsr files from the patch totally as we
> don't want to add any new features to them.

I think that is probably the best solution.  I know they are depreciated, but since they were affected by the patch, I needed to cover everything.

Christopher
Comment 121 Katrin Fischer 2014-06-26 17:42:16 UTC
:)
Comment 122 Kyle M Hall 2014-06-26 18:21:16 UTC Comment hidden (obsolete)
Comment 123 Kyle M Hall 2014-06-26 18:57:47 UTC Comment hidden (obsolete)
Comment 124 Kyle M Hall 2014-06-26 18:58:01 UTC Comment hidden (obsolete)
Comment 125 Kyle M Hall 2014-06-26 18:58:06 UTC Comment hidden (obsolete)
Comment 126 Kyle M Hall 2014-06-26 19:00:34 UTC
I'm fairly sure we've got our bases cover now! Please retest. Thanks again for your testing and feedback Christopher and Katrin!
Comment 127 Christopher Brannon 2014-06-26 21:33:00 UTC
(In reply to Kyle M Hall from comment #126)
> I'm fairly sure we've got our bases cover now! Please retest. Thanks again
> for your testing and feedback Christopher and Katrin!

Kyle,

Looks good.

As I mentioned in comment 117:

I still think the tabs ... should read "Guarantees' checkouts" rather than "Relatives' checkouts".

Also, do you think the secondary checkout tab (Relatives/Gaurentees) should have a count next to it like the Checked out tab does, for consistency?  Or is that another enhancement?

Other than that, once we address the labeling on the tabs, I would be happy to sign off.  Let me know your thoughts.
Comment 128 HB-NEKLS 2014-07-01 22:14:54 UTC
Kyle, did you see Christopher's comments/concerns? Otherwise, I'd also be happy to sign off on this.
Comment 129 Kyle M Hall 2014-07-11 16:13:41 UTC
> I still think the tabs ... should read "Guarantees' checkouts" rather than
> "Relatives' checkouts".

I did not change that because I think that will only cause patrons to ask "What is a Guarantee?" I think that question should be left to the QA'er.
 
> Also, do you think the secondary checkout tab (Relatives/Gaurentees) should
> have a count next to it like the Checked out tab does, for consistency?  Or
> is that another enhancement?

That would be a great addition, but definitely an enhancement outside the scope of this bug.
Comment 130 HB-NEKLS 2014-07-18 21:38:27 UTC
Kyle, I went to sign off on this patch again using the sandboxes, and got this error that the sandbox wasn't ready:  

Some problems occurred applying patches from bug 9303:
<h1>Something went wrong !</h1>Applying: Bug 9303 [1] - relative's checkouts in the opac - Schema Updates
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging Koha/Schema/Result/Borrower.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/Borrower.pm
Auto-merging Koha/Schema/Result/Issue.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/Issue.pm
Auto-merging Koha/Schema/Result/OldIssue.pm
CONFLICT (content): Merge conflict in Koha/Schema/Result/OldIssue.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 9303 [1] - relative's checkouts in the opac - Schema Updates
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 9303 - relative's checkouts in the opac

28153 - Bug 9303 [1] - relative's checkouts in the opac - Schema Updates
28154 - Bug 9303 [2] - relative's checkouts in the opac - Database Updates
28155 - Bug 9303 [3] - relative's checkouts in the opac
29215 - Bug 9303 [4] [QA Followup]
29302 - Bug 9303 [5] [QA Followup 2]
29303 - Bug 9303 [6] [QA Followup 3] - Rename prefs and field
29304 - Bug 9303 [7] [QA Followup 4] - Undo changes to prog and ccsr themes
Comment 131 Kyle M Hall 2014-07-21 15:53:14 UTC Comment hidden (obsolete)
Comment 132 Kyle M Hall 2014-07-21 15:53:30 UTC Comment hidden (obsolete)
Comment 133 Kyle M Hall 2014-07-21 15:53:33 UTC Comment hidden (obsolete)
Comment 134 Kyle M Hall 2014-07-21 16:13:18 UTC Comment hidden (obsolete)
Comment 135 Kyle M Hall 2014-07-21 16:13:32 UTC Comment hidden (obsolete)
Comment 136 Kyle M Hall 2014-07-21 16:13:36 UTC Comment hidden (obsolete)
Comment 137 Kyle M Hall 2014-07-21 16:36:36 UTC
Schema updates in master were causing merge conflicts. I've fixed those, and also merged all the followups into the main patch set to make it easier to read for qa.

You should be able to sandbox these patches now!

Kyle

(In reply to Heather Braum from comment #130)
> Kyle, I went to sign off on this patch again using the sandboxes, and got
> this error that the sandbox wasn't ready:
Comment 138 Nick Clemens 2014-07-24 18:45:08 UTC
This needs rebased on master

I tested on sandbox and got Error 500 when trying to sign in to patron's account

I tested in VM and all seems good after clearing the merge conflict in updatedatabase.pl  but I don't know the procedure to submit rebased patches so I will retest and sign off ASAP when rebased
Comment 139 Kyle M Hall 2014-07-29 14:15:15 UTC Comment hidden (obsolete)
Comment 140 Kyle M Hall 2014-07-29 14:17:03 UTC Comment hidden (obsolete)
Comment 141 Kyle M Hall 2014-07-29 14:17:09 UTC Comment hidden (obsolete)
Comment 142 Christopher Brannon 2014-07-29 14:48:43 UTC
The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 9303:
<h1>Something went wrong !</h1>Applying: Bug 9303 [1] - relative's checkouts in the opac - Database Updates
fatal: sha1 information is lacking or useless (installer/data/mysql/sysprefs.sql).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 9303 [1] - relative's checkouts in the opac - Database Updates When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 9303 - relative's checkouts in the opac

30287 - Bug 9303 [1] - relative's checkouts in the opac - Database Updates
30288 - Bug 9303 [2] - relative's checkouts in the opac - Schema Updates
30289 - Bug 9303 [3] - relative's checkouts in the opac

Christopher
Comment 143 Nick Clemens 2014-07-29 17:06:02 UTC
Tested on VM, patch applied cleanly.

Test plan:
1 - Apply patch and run updatedatabase.pl
2 - Create three patrons A, B, and C.  A is guarantor for B and C
3 - Checkout one item to each B and C
4 - Set AllowPatronToSetCheckoutsVisibilityForGuarantor to Allow - option appears on B and C's 'Your personal details' tab in OPAC and not on A's
5 - Set OPACPrivacy  to allow - the guarantor option moves to the privacy tab for B & C, still not visible for A
6 - Set Allow guarantor to view.... For B, then C, then both, appears as expected on 'Relatives checkouts' tab for borrower A
7 - Set AllowPatronToSetCheckoutsVisibilityForGuarantor to Don't & set AllowStaffToSetCheckoutsVisibilityForGuarantor to allow - option is removed from B&C, A can still see their checkouts.  Staff can view B's checkouts on C's account and vice versa.  Patron A can see checkouts for B&C.  Staff can edit the option for patrons B &C's accounts but not on A's account
8 - Set AllowStaffToSetCheckoutsVisibilityForGuarantor to Don't - Patron A can no longer see B or C's checkouts, staff can see B&C's checkouts on each others accounts and both B&C on A's account
9 - Set both New prefs on and both staff and patrons can edit and view as expected.  Toggle OPACPrivacy and the patron option moves

I think I hit all the relevant combinations and did not run into any bugs.

The only minor error I see is that all patrons now have the 'Show checkouts to guarantor' flag listed under 'Library Use' whether or not the preferences are on or whether or not they have guarantors. If they don't have a guarantor you can see the option but not edit it regardless of the AllowStaff... syspref.
Comment 144 Nick Clemens 2014-07-29 17:26:22 UTC Comment hidden (obsolete)
Comment 145 Nick Clemens 2014-07-29 17:27:31 UTC Comment hidden (obsolete)
Comment 146 Nick Clemens 2014-07-29 17:27:44 UTC Comment hidden (obsolete)
Comment 147 Nick Clemens 2014-07-29 17:27:52 UTC Comment hidden (obsolete)
Comment 148 Jonathan Druart 2014-09-18 14:52:26 UTC
Kyle,
I got these 2 errors on launching the updatedb script:

DBD::mysql::db do failed: Can't DROP 'PRIMARY'; check that column/key exists at installer/data/mysql/updatedatabase.pl line 8788.
DBD::mysql::db do failed: Can't DROP 'itemnumber_idx'; check that column/key exists at installer/data/mysql/updatedatabase.pl line 8794.
Comment 149 Kyle M Hall 2014-10-08 10:06:50 UTC Comment hidden (obsolete)
Comment 150 Kyle M Hall 2014-10-08 10:07:09 UTC Comment hidden (obsolete)
Comment 151 Kyle M Hall 2014-10-08 10:07:13 UTC Comment hidden (obsolete)
Comment 152 Kyle M Hall 2014-10-08 10:09:35 UTC
Those are not a problem. Unfortunately there is no IF EXISTS for adding and dropping indexes. You should have had those indexes, but since you didn't, it's not an issue. The situation is similar to if a db update were to attempt to insert a system preference that already exists, or create a table column that already exists. It will fail, but cause absolutely no harm.

(In reply to Jonathan Druart from comment #148)
> Kyle,
> I got these 2 errors on launching the updatedb script:
> 
> DBD::mysql::db do failed: Can't DROP 'PRIMARY'; check that column/key exists
> at installer/data/mysql/updatedatabase.pl line 8788.
> DBD::mysql::db do failed: Can't DROP 'itemnumber_idx'; check that column/key
> exists at installer/data/mysql/updatedatabase.pl line 8794.
Comment 153 Katrin Fischer 2014-10-30 19:54:48 UTC
Sorry Kyle, but I tried with various old databases I have and the databaes update always gives me trouble :(


Upgrade to 3.17.00.043 done (Bug 7162: Add aqorders.cancellationreason)
DBD::mysql::db do failed: Can't DROP 'PRIMARY'; check that column/key exists at ./installer/data/mysql/updatedatabase.pl line 8964.
DBD::mysql::db do failed: Can't DROP 'itemnumber_idx'; check that column/key exists at ./installer/data/mysql/updatedatabase.pl line 8970.
DBD::mysql::db do failed: Can't DROP 'PRIMARY'; check that column/key exists at ./installer/data/mysql/updatedatabase.pl line 8973.
DBD::mysql::db do failed: Cannot drop index 'itemnumber_idx': needed in a foreign key constraint at ./installer/data/mysql/updatedatabase.pl line 8979.
DBD::mysql::db do failed: Duplicate key name 'itemnumber_idx' at ./installer/data/mysql/updatedatabase.pl line 8982.

Can you please take a look?
Comment 154 Kyle M Hall 2014-10-31 12:06:30 UTC Comment hidden (obsolete)
Comment 155 Katrin Fischer 2014-10-31 21:11:49 UTC
Working on this.
Comment 156 Katrin Fischer 2014-11-01 06:57:31 UTC
... and still working on it right now :)
Comment 157 Katrin Fischer 2014-11-01 08:49:13 UTC
Hi Kyle,

I tried to test this last minute, but I found a blocker:

Changing the allowance from the 'personal details' page is not being
saved. I tried using the update button and also make a full change
request, nothing worked.

Firefox shows a network error: 
NetworkError: 404 Not Found - http://localhost/cgi-bin/koha/svc/patron/show_checkouts_to_relatives

If Javscript is needed to make this work, please also consider a 
non-Javascript fallback.

The stars * mark questions or problems, questions are questions :)

1)* Code review
- OK passes QA script and tests
- OK ModPrivacy is removed, no other references found
* Note: There is some use of the TT plugin $EncodeUTF8, which might
  be obsoleted if the UTF-8 patches get pushes.

2)* Database structure
- OK Patch adds an issue_id PK column to issues/old_issues
* Problem (normal): I think old_issues PK should not be AUTO_INCREMENT 
  (see old_reserves)
- OK Patch adds privacy_guarantor_checkouts to borrowers/deletedborrowers
* Problem (trivial): Please add 'AFTER privacy' to updatedatabase to 
  ensure the correct sequence for new and updated installations
- OK 2 new system preferences are added:
  - AllowPatronToSetCheckoutsVisibilityForGuarantor
  - AllowStaffToSetCheckoutsVisibilityForGuarantor
- OK updatedatabase works
- OK changes with kohastructure are consistent

3) Testing the new issue_id
- OK Numbering after update appears ok:
  old_issues: 1-23
  issues: 24-26
- OK New Check-outs get the next higher number.
- OK Check-ins are correctly moved to old_issues.

4) Testing changes on borrower record (staff)
- OK Deleted patron is moved to deletedborrowers.
- OK Editing patrons.
- OK Adding patrons.

5) Testing 'Show checkouts to guarantor' (staff)
* Details: The information now always shows on the patron record,
  if yes or no
  independent from the system preference settings
  independent from the patron being guarantor/guarantee
  Question: Can we think of a better way here? 
  Maybe only show the information when (either pref is ON and the 
  patron is a guarantee) OR (setting is set to yes)?
  Really not sure what is best here, sounds too complicated.
* Edit form: Settings shows next to privacy
  Question: Maybe it would make sense to group it with the
  guarantor information? 
* Terminology: The setting is "...to guarantor" but the tab is labelled
  "Relative's checkouts"
  Question: Should we make them match?
  
6)* Testing options and visibility in OPAC
* AllowPatronToSetCheckoutsVisibilityForGuarantor is ON
  OpacPrivacy is OFF
  Problem (blocker) changes to the display made from the personal
  details tab won't save.
- OpacPrivacy is ON
  OK Setting can be changed.
- AllowPatronToSetCheckoutsVisibilityForGuarantor is ON
  OK Setting is not displayed.

Found preexisting BUGS!
* Problem (also in master): Author's don't display correctly on the 
  relative's checkouts issues list in staff: 
    Vogelfang und Vogelberingung / by _AUTHOR_ Bub, Hans
* Problem (also in master) Table layout on the 'details' tab in staff 
   is broken.

Notes: 
- Should we not generally display information about an existing
guarantor in the patron's details in OPAC?
- If both settings are OFF the user has no information in the OPAC,
but we still show it on staff - not sure how to make this better.
It feels a bit like this is not 'round' yet.
Comment 158 Kyle M Hall 2014-12-22 12:50:35 UTC Comment hidden (obsolete)
Comment 159 Kyle M Hall 2014-12-22 12:50:45 UTC Comment hidden (obsolete)
Comment 160 Kyle M Hall 2014-12-22 12:50:49 UTC Comment hidden (obsolete)
Comment 161 Kyle M Hall 2014-12-22 12:50:53 UTC Comment hidden (obsolete)
Comment 162 Kyle M Hall 2014-12-22 12:50:57 UTC Comment hidden (obsolete)
Comment 163 Kyle M Hall 2014-12-22 12:57:42 UTC
> Changing the allowance from the 'personal details' page is not being
> saved. I tried using the update button and also make a full change
> request, nothing worked.
> 
> Firefox shows a network error: 
> NetworkError: 404 Not Found -
> http://localhost/cgi-bin/koha/svc/patron/show_checkouts_to_relatives
> 
> If Javscript is needed to make this work, please also consider a 
> non-Javascript fallback.

I would be happy to add a non-js fallback after this is passed qa. For now, I'm reticent to further modify the patch set that much, considering the length of time this has been in process!
 
> * Problem (normal): I think old_issues PK should not be AUTO_INCREMENT 
>   (see old_reserves)

Fixed!

> * Problem (trivial): Please add 'AFTER privacy' to updatedatabase to 
>   ensure the correct sequence for new and updated installations

Fixed!

> 5) Testing 'Show checkouts to guarantor' (staff)
> * Details: The information now always shows on the patron record,
>   if yes or no
>   independent from the system preference settings
>   independent from the patron being guarantor/guarantee
>   Question: Can we think of a better way here? 
>   Maybe only show the information when (either pref is ON and the 
>   patron is a guarantee) OR (setting is set to yes)?
>   Really not sure what is best here, sounds too complicated.

This is outside the scope of this bug report.

> * Edit form: Settings shows next to privacy
>   Question: Maybe it would make sense to group it with the
>   guarantor information? 

Done!

> * Terminology: The setting is "...to guarantor" but the tab is labelled
>   "Relative's checkouts"
>   Question: Should we make them match?

Both words are used throughout Koha. I think this is also outside the scope of this bug and would be better resolved in a separate report. I think it's tough because relative is easier for patrons to understand, but guarantor is more accurate.

> 6)* Testing options and visibility in OPAC
> * AllowPatronToSetCheckoutsVisibilityForGuarantor is ON
>   OpacPrivacy is OFF
>   Problem (blocker) changes to the display made from the personal
>   details tab won't save.

Can't reproduce. When you submit new data, the changes must still be approved by a librarian. Perhaps that's the part you missed?

> Notes: 
> - Should we not generally display information about an existing
> guarantor in the patron's details in OPAC?

Not sure what you mean by this, but it sounds like another further enhancement.

> - If both settings are OFF the user has no information in the OPAC,
> but we still show it on staff - not sure how to make this better.
> It feels a bit like this is not 'round' yet.

Also outside the scope of this bug, but could easily be set to be controlled by the patron level setting. This of course wouldn't prevent a librarian from looking it up separately, but would provide a bit of a 'wall' between them.
Comment 164 Katrin Fischer 2014-12-22 13:44:07 UTC
Hi Kyle,

have you been able to reproduce/fix the first issue with the javascript error in the OPAC?
Comment 165 Kyle M Hall 2015-01-02 12:23:15 UTC
(In reply to Katrin Fischer from comment #164)
> Hi Kyle,
> 
> have you been able to reproduce/fix the first issue with the javascript
> error in the OPAC?

Not yet, would it be possible for you to create a video to demonstrate the issue? That way I can see if I'm making mistakes in my attempt to reproduce the problem.
Comment 166 Katrin Fischer 2015-01-20 21:17:36 UTC
Hi Kyle,

I tried to retest the problem, but the database update gives me problems :(

bumblebee:~/kohaclone (38-9303-relativecheckouts|AM 1/1) $ ./installer/data/mysql/updatedatabase.pl 
DBD::mysql::db do failed: Can't DROP 'PRIMARY'; check that column/key exists at ./installer/data/mysql/updatedatabase.pl line 9681.
DBD::mysql::db do failed: Can't DROP 'itemnumber_idx'; check that column/key exists at ./installer/data/mysql/updatedatabase.pl line 9687.
DBD::mysql::db do failed: Can't DROP 'PRIMARY'; check that column/key exists at ./installer/data/mysql/updatedatabase.pl line 9690.
DBD::mysql::db do failed: Cannot drop index 'itemnumber_idx': needed in a foreign key constraint at ./installer/data/mysql/updatedatabase.pl line 9696.
DBD::mysql::db do failed: Duplicate key name 'itemnumber_idx' at ./installer/data/mysql/updatedatabase.pl line 9699.
Upgrade to 3.19.00.XXX done (Bug 9303 - relative's checkouts in the opac)
Comment 167 Katrin Fischer 2015-01-20 21:19:04 UTC
Just realized I didn't have all the patches... and that it might be a bit too late tonight for this
Comment 168 Kyle M Hall 2015-01-26 13:02:17 UTC Comment hidden (obsolete)
Comment 169 Kyle M Hall 2015-01-26 13:05:22 UTC Comment hidden (obsolete)
Comment 170 Kyle M Hall 2015-01-26 13:05:32 UTC Comment hidden (obsolete)
Comment 171 Kyle M Hall 2015-01-26 13:05:37 UTC Comment hidden (obsolete)
Comment 172 Kyle M Hall 2015-01-26 13:05:41 UTC Comment hidden (obsolete)
Comment 173 Kyle M Hall 2015-01-26 13:05:46 UTC Comment hidden (obsolete)
Comment 174 Kyle M Hall 2015-01-26 13:05:50 UTC Comment hidden (obsolete)
Comment 175 Kyle M Hall 2015-01-26 13:28:58 UTC Comment hidden (obsolete)
Comment 176 Kyle M Hall 2015-01-26 13:29:20 UTC Comment hidden (obsolete)
Comment 177 Kyle M Hall 2015-01-26 13:29:31 UTC Comment hidden (obsolete)
Comment 178 Kyle M Hall 2015-01-26 13:29:35 UTC Comment hidden (obsolete)
Comment 179 Kyle M Hall 2015-01-26 13:29:40 UTC Comment hidden (obsolete)
Comment 180 Kyle M Hall 2015-01-26 13:29:44 UTC Comment hidden (obsolete)
Comment 181 Kyle M Hall 2015-01-26 13:29:49 UTC Comment hidden (obsolete)
Comment 182 Kyle M Hall 2015-01-26 13:31:46 UTC
(In reply to Katrin Fischer from comment #166)
> Hi Kyle,
> 
> I tried to retest the problem, but the database update gives me problems :(
> 
> bumblebee:~/kohaclone (38-9303-relativecheckouts|AM 1/1) $
> ./installer/data/mysql/updatedatabase.pl 
> DBD::mysql::db do failed: Can't DROP 'PRIMARY'; check that column/key exists
> at ./installer/data/mysql/updatedatabase.pl line 9681.
> DBD::mysql::db do failed: Can't DROP 'itemnumber_idx'; check that column/key
> exists at ./installer/data/mysql/updatedatabase.pl line 9687.
> DBD::mysql::db do failed: Can't DROP 'PRIMARY'; check that column/key exists
> at ./installer/data/mysql/updatedatabase.pl line 9690.
> DBD::mysql::db do failed: Cannot drop index 'itemnumber_idx': needed in a
> foreign key constraint at ./installer/data/mysql/updatedatabase.pl line 9696.
> DBD::mysql::db do failed: Duplicate key name 'itemnumber_idx' at
> ./installer/data/mysql/updatedatabase.pl line 9699.
> Upgrade to 3.19.00.XXX done (Bug 9303 - relative's checkouts in the opac)

Please try again. Are you sure your database is using the stock Koha schema?
Comment 183 Katrin Fischer 2015-01-26 13:38:12 UTC
Kyle, ignore comment#166 - see comment#167.
Comment 184 Kyle M Hall 2015-02-05 18:20:57 UTC Comment hidden (obsolete)
Comment 185 Kyle M Hall 2015-02-05 18:21:17 UTC Comment hidden (obsolete)
Comment 186 Kyle M Hall 2015-02-05 18:21:29 UTC Comment hidden (obsolete)
Comment 187 Kyle M Hall 2015-02-05 18:21:34 UTC Comment hidden (obsolete)
Comment 188 Kyle M Hall 2015-02-05 18:21:38 UTC Comment hidden (obsolete)
Comment 189 Kyle M Hall 2015-02-05 18:21:43 UTC Comment hidden (obsolete)
Comment 190 Kyle M Hall 2015-02-05 18:21:47 UTC Comment hidden (obsolete)
Comment 191 Brendan Gallagher 2015-02-05 18:50:42 UTC Comment hidden (obsolete)
Comment 192 Brendan Gallagher 2015-02-05 18:50:52 UTC Comment hidden (obsolete)
Comment 193 Brendan Gallagher 2015-02-05 18:51:00 UTC Comment hidden (obsolete)
Comment 194 Brendan Gallagher 2015-02-05 18:51:08 UTC Comment hidden (obsolete)
Comment 195 Brendan Gallagher 2015-02-05 18:51:15 UTC Comment hidden (obsolete)
Comment 196 Brendan Gallagher 2015-02-05 18:51:22 UTC Comment hidden (obsolete)
Comment 197 Katrin Fischer 2015-02-05 22:20:20 UTC
Hi Brendan, 

could you please comment if the problem I notes earlier worked for you?

> Changing the allowance from the 'personal details' page [in OPAC] is not being
> saved. I tried using the update button and also make a full change
> request, nothing worked.
Comment 198 Katrin Fischer 2015-02-08 22:27:34 UTC
(In reply to Kyle M Hall from comment #165)
> (In reply to Katrin Fischer from comment #164)
> > Hi Kyle,
> > 
> > have you been able to reproduce/fix the first issue with the javascript
> > error in the OPAC?
> 
> Not yet, would it be possible for you to create a video to demonstrate the
> issue? That way I can see if I'm making mistakes in my attempt to reproduce
> the problem.

I can't do a video - but I found the reason:
"NetworkError: 404 Not Found - http://localhost/cgi-bin/koha/svc/patron/show_checkouts_to_relatives"

This file is not added by this patch set, but referenced in opac-memberentry.tt on line 829.
Comment 199 Kyle M Hall 2015-02-11 14:14:27 UTC Comment hidden (obsolete)
Comment 200 Jonathan Druart 2015-02-23 14:42:25 UTC
Kyle,

Any chances to see this patch set resubmitted in a more readable way?
The schema is updated in another patch than the "Schema Updates" one, "Remove use of EncodeUTF8" could be squashed with another one, the missing file should be part of the main patch, etc.

Moreover I don't see the point (except a too lazy way) of the Koha::Template::Plugin::Koha::Find method, IMO the data should be fetch in the pl file. Another QA point of view is needed here.
Comment 201 Kyle M Hall 2015-03-02 11:51:12 UTC Comment hidden (obsolete)
Comment 202 Kyle M Hall 2015-03-02 11:51:47 UTC Comment hidden (obsolete)
Comment 203 Kyle M Hall 2015-03-02 11:52:22 UTC Comment hidden (obsolete)
Comment 204 Katrin Fischer 2015-03-22 06:28:34 UTC
Please see 13743 and 13790. Also there is a mismatch in this patch - auto_increment for old_issues is in kohastructure.sql but not in the database update.
Comment 205 Kyle M Hall 2015-03-23 11:48:32 UTC Comment hidden (obsolete)
Comment 206 Kyle M Hall 2015-03-23 11:48:43 UTC Comment hidden (obsolete)
Comment 207 Kyle M Hall 2015-03-23 11:48:47 UTC Comment hidden (obsolete)
Comment 208 Martin Renvoize 2015-04-20 15:43:11 UTC
Comment on attachment 37112 [details] [review]
Bug 9303 [3] - relative's checkouts in the opac

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

Generally looks good to me.. just these two minor comments?

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
@@ +1,3 @@
>  [% USE Koha %]
>  [% USE KohaDates %]
> +[% USE Koha %]

There's another USE Koha just above here already ;)

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt
@@ +133,4 @@
>                                                  [% IF ( show_barcode ) %]
>                                                      <th>Barcode</th>
>                                                  [% END %]
> +                                                    <th>Call number</th>

Did you mean to make this string change here?

@@ +423,4 @@
>                                              <th class="anti-the">Title</th>
>                                              [% UNLESS ( item_level_itypes ) %]<th>Item type</th> [% END %]
>                                              [% IF ( show_barcode ) %]<th>Barcode</th>[% END %]
> +                                            <th>Call number</th>

Did you mean to make this string change here?
Comment 209 Kyle M Hall 2015-04-21 14:22:46 UTC Comment hidden (obsolete)
Comment 210 Kyle M Hall 2015-04-21 14:22:56 UTC Comment hidden (obsolete)
Comment 211 Kyle M Hall 2015-04-21 14:23:00 UTC Comment hidden (obsolete)
Comment 212 Kyle M Hall 2015-04-21 14:23:05 UTC Comment hidden (obsolete)
Comment 213 Martin Renvoize 2015-04-22 12:31:02 UTC Comment hidden (obsolete)
Comment 214 Martin Renvoize 2015-04-22 12:31:08 UTC Comment hidden (obsolete)
Comment 215 Martin Renvoize 2015-04-22 12:31:15 UTC Comment hidden (obsolete)
Comment 216 Martin Renvoize 2015-04-22 12:31:20 UTC Comment hidden (obsolete)
Comment 217 Martin Renvoize 2015-04-22 12:33:09 UTC
I've not spotted any issues in my testing.. so am happy to pass qa on this one.

The original sign-off lines appear to be missing from the commits though.. think they must have got lost along the way somewhere.. but I don't see that as a reason to hold this one up.
Comment 218 Martin Renvoize 2015-04-22 12:35:50 UTC
(In reply to Martin Renvoize from comment #217)
> I've not spotted any issues in my testing.. so am happy to pass qa on this
> one.
> 
> The original sign-off lines appear to be missing from the commits though..
> think they must have got lost along the way somewhere.. but I don't see that
> as a reason to hold this one up.

Going through the bug history.. is it the signoff by nick - 2014-07-29 17:26:19 UTC we're still using.. has he tested all this since?
Comment 219 Nick Clemens 2015-04-22 22:00:01 UTC
I tried retesting today just for due diligence but I think I am doing something wrong.  As soon as I enable either syspref I get the following error on OPAC:

DBIx::Class::ResultSource::_resolve_join(): No such relationship biblio on Item at /home/kidclamp/kohaclone/opac/opac-user.pl line 390
Comment 220 Kyle M Hall 2015-04-23 18:01:06 UTC Comment hidden (obsolete)
Comment 221 Kyle M Hall 2015-04-23 18:01:17 UTC Comment hidden (obsolete)
Comment 222 Kyle M Hall 2015-04-23 18:01:22 UTC Comment hidden (obsolete)
Comment 223 Kyle M Hall 2015-04-23 18:01:29 UTC Comment hidden (obsolete)
Comment 224 Kyle M Hall 2015-04-23 18:01:33 UTC Comment hidden (obsolete)
Comment 225 Nick Clemens 2015-04-24 20:06:09 UTC
Okay, new patches clear the previous error, but seeing two:

With OPACPrivacy set to allow and AllowPatronToSetCheckoutsVisibilityForGuarantor set to allow I cannot see a place to set the visibility anywhere.  

With OPACPrivacy set to Don't Allow and AllowPatronToSetCheckoutsVisibilityForGuarantor to Allow 
I get the option under 'Personal details' however, I cannot actually change the value or see the guarantor's name.


Good news is that the AllowStaffToSetCheckoutsVisibilityForGuarantor pref seems to work as expected.  Patron aaccounts are visible/not as expected with different combinations of settings,
Comment 226 Kyle M Hall 2015-05-04 15:34:58 UTC Comment hidden (obsolete)
Comment 227 Kyle M Hall 2015-05-04 15:35:09 UTC Comment hidden (obsolete)
Comment 228 Kyle M Hall 2015-05-04 15:35:14 UTC Comment hidden (obsolete)
Comment 229 Kyle M Hall 2015-05-04 15:35:19 UTC Comment hidden (obsolete)
Comment 230 Kyle M Hall 2015-05-04 15:35:24 UTC Comment hidden (obsolete)
Comment 231 Nick Clemens 2015-05-08 19:14:27 UTC
I am still having issues making this work correctly, I cannot see option to show to guarantor when OPACPrivacy is enabled and I can see but not edit (on opac-memberentry.pl) when disabled

Digging into it a bit, the line in opac-privacy.tt:
[% SET b = Koha.Find('Borrower', borrower.borrowernumber) %]
doesn't seem to be returning anything into 'b' for me.

Am I missing something?
Comment 232 Kyle M Hall 2015-05-11 11:33:32 UTC Comment hidden (obsolete)
Comment 233 Kyle M Hall 2015-05-11 11:33:41 UTC Comment hidden (obsolete)
Comment 234 Kyle M Hall 2015-05-11 11:33:46 UTC Comment hidden (obsolete)
Comment 235 Kyle M Hall 2015-05-11 11:33:50 UTC Comment hidden (obsolete)
Comment 236 Kyle M Hall 2015-05-11 11:33:55 UTC Comment hidden (obsolete)
Comment 237 Kyle M Hall 2015-05-11 11:33:59 UTC Comment hidden (obsolete)
Comment 238 Kyle M Hall 2015-05-11 11:46:44 UTC Comment hidden (obsolete)
Comment 239 Nick Clemens 2015-05-13 22:12:35 UTC
Okay, with OPACprivacy enabled and AllowPatronToSetCheckoutsVisibilityForGuarantor enabled a patron can set wether guarantor can view or not

with OPACprivacy disabled and AllowPatronToSetCheckoutsVisibilityForGuarantor enabled a patron can see their guarantor but cannot save changes to whether their checkouts are shown.  The update button doesn't work and a change to this field doesn't register if using the "submit update request" button

Just a minor question too:  
If AllowPatronToSetCheckoutsVisibilityForGuarantor is disallow
and AllowStaffToSetCheckoutsVisibilityForGuarantor is allow
the patrons checkouts can be shown to their guarantor but they can neither see who their guarantor is or whether or not their checkouts are visible.  That seems slightly concerning to me.  The same goes for when both are set to disallow.  To my mind if there is a situation in which your checkouts are being shown to a guarantor that info should be visible whether or not you can edit it

All else seems to work as expected
Comment 240 Katrin Fischer 2015-05-14 16:34:19 UTC
> with OPACprivacy disabled and
> AllowPatronToSetCheckoutsVisibilityForGuarantor enabled a patron can see
> their guarantor but cannot save changes to whether their checkouts are
> shown.  The update button doesn't work and a change to this field doesn't
> register if using the "submit update request" button

Should this be failed QA?
Comment 241 Kyle M Hall 2015-06-03 17:55:30 UTC Comment hidden (obsolete)
Comment 242 Kyle M Hall 2015-06-03 17:56:22 UTC
Script was inadvertently removed at some point. Restoring Passed QA status but feel free to re-QA.
Comment 243 Nick Clemens 2015-06-03 18:55:18 UTC Comment hidden (obsolete)
Comment 244 Nick Clemens 2015-06-03 18:55:27 UTC Comment hidden (obsolete)
Comment 245 Nick Clemens 2015-06-03 18:55:34 UTC Comment hidden (obsolete)
Comment 246 Nick Clemens 2015-06-03 18:55:41 UTC Comment hidden (obsolete)
Comment 247 Nick Clemens 2015-06-03 18:55:47 UTC Comment hidden (obsolete)
Comment 248 Nick Clemens 2015-06-03 18:55:55 UTC Comment hidden (obsolete)
Comment 249 Nick Clemens 2015-06-03 18:56:03 UTC Comment hidden (obsolete)
Comment 250 Katrin Fischer 2015-06-06 11:27:02 UTC
Thx Nick for your testing!
Comment 251 HB-NEKLS 2015-08-26 14:24:40 UTC
I hope this appropriate to ask -- any chance this could get pushed to master soon, since it looks like it's passed QA?
Comment 252 Chris Cormack 2015-08-26 21:14:39 UTC
(In reply to Heather Braum from comment #251)
> I hope this appropriate to ask -- any chance this could get pushed to master
> soon, since it looks like it's passed QA?

It's always appropriate to ask, the step after Passed QA is generally pushed to master. But that step involves the RM testing still. So it's not just a formality. Changes like this which could potentially cause a privacy breach if there was a mistake of course take a lot of testing.

I suspect it is in the queue of things for Tomas to look at when he gets time.
Comment 253 HB-NEKLS 2015-08-26 21:40:52 UTC
Thanks for the explanation Chris, which makes perfect sense -- I forgot about the RM testing. In that case, I'll keep waiting for this one.
Comment 254 HB-NEKLS 2015-10-20 16:31:13 UTC
As the 3.22 feature flush approaches, I was wondering if this one was going to be considered a candidate for 3.22. Thanks.
Comment 255 Nicole C. Engard 2015-10-30 20:10:29 UTC
My understanding is that if it's passed qa it's going to be considered for 3.22.
Comment 256 Tomás Cohen Arazi 2015-11-04 14:28:59 UTC
I'm sorry, could you please rebase?
Comment 257 Kyle M Hall 2015-11-04 16:58:56 UTC Comment hidden (obsolete)
Comment 258 Kyle M Hall 2015-11-04 16:59:09 UTC Comment hidden (obsolete)
Comment 259 Kyle M Hall 2015-11-04 16:59:14 UTC Comment hidden (obsolete)
Comment 260 Kyle M Hall 2015-11-04 16:59:20 UTC Comment hidden (obsolete)
Comment 261 Kyle M Hall 2015-11-04 16:59:26 UTC Comment hidden (obsolete)
Comment 262 Kyle M Hall 2015-11-04 16:59:31 UTC Comment hidden (obsolete)
Comment 263 Kyle M Hall 2015-11-04 16:59:37 UTC Comment hidden (obsolete)
Comment 264 Kyle M Hall 2015-11-05 14:03:48 UTC Comment hidden (obsolete)
Comment 265 Kyle M Hall 2015-11-05 14:04:22 UTC Comment hidden (obsolete)
Comment 266 Kyle M Hall 2015-11-05 14:04:27 UTC Comment hidden (obsolete)
Comment 267 Kyle M Hall 2015-11-05 14:04:32 UTC Comment hidden (obsolete)
Comment 268 Kyle M Hall 2015-11-05 14:04:38 UTC Comment hidden (obsolete)
Comment 269 Kyle M Hall 2015-11-05 14:04:43 UTC Comment hidden (obsolete)
Comment 270 Kyle M Hall 2015-11-05 14:04:48 UTC Comment hidden (obsolete)
Comment 271 Kyle M Hall 2015-12-31 13:10:53 UTC
Created attachment 46136 [details] [review]
Bug 9303 [1] - relative's checkouts in the opac - Database Updates

This patch gives patrons the option of allowing other
linked relatives to view his or her checkouts from
opac-user.pl in a manner similiar to the patron's own
checkouts tab. By default the setting is not enabled.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Create three borrowers, link them as relatives
4) In the OPAC, go to "my privacy" and choose to allow
   relatives to view current checkouts for borrowers A and B
5) Check out some items to each borrower
6) Log into the OPAC as borrower A, you should see a "Relatives'
   checkouts" tab. In this tab you should see the checkouts for
   borrower B
7) Log into the OPAC as borrower C, you should be able to view
   the current checkouts for both borrower A and borrower B

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 272 Kyle M Hall 2015-12-31 13:11:04 UTC
Created attachment 46137 [details] [review]
Bug 9303 [2] - relative's checkouts in the opac - Schema Updates e opac/svc/patron/

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 273 Kyle M Hall 2015-12-31 13:11:09 UTC
Created attachment 46138 [details] [review]
Bug 9303 [3] - relative's checkouts in the opac

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 274 Kyle M Hall 2015-12-31 13:11:13 UTC
Created attachment 46139 [details] [review]
Bug 9303 [QA Followup] - Remove extra 'USE Koha', change "Call number" back to "Call no."

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 275 Kyle M Hall 2015-12-31 13:11:18 UTC
Created attachment 46140 [details] [review]
Bug 9303 [QA Followup] - Restore missing biblio relationship

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 276 Kyle M Hall 2015-12-31 13:11:23 UTC
Created attachment 46141 [details] [review]
Bug 9303 [QA Followup] - Restore ability for patron to control setting

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 277 Kyle M Hall 2015-12-31 13:11:28 UTC
Created attachment 46142 [details] [review]
Bug 9303 [QA Followup] - Restore missing svc script

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 278 Kyle M Hall 2015-12-31 13:26:28 UTC
Pushed to master!