Patch tested with a sandbox, by Joel Sasse <jsasse@plumcreeklibrary.net>

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.

(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

(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)...

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?

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.

(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.

(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.

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.

Patch tested with a sandbox, by Heather Braum <hbraum@nekls.org>

(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.

(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!

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.

(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 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 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?

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.

(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 ; )

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.

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?

(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!

I had asked a few more questions in comment 37 - especially about a change related to reserves.

(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.

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 ;)

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.

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.

(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.

(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.

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.

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

> 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.

(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.

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.

I had hoped someone would take this on where I have left off - I think 5) is a blocker.

> - [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.

It's been a month with no reply, setting to passed qa.

Kyle

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.

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

(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.

(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.

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.

(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

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?

(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?

(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.

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

(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.

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.

(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

(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.

> 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 ; )

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.

Thx Kyle for incorporating the feedback in the new set of patches!

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.  :(

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

Hi Christopher, thx for your testing!

> 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!

(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

(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

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.

(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

:)

I'm fairly sure we've got our bases cover now! Please retest. Thanks again for your testing and feedback Christopher and Katrin!

(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.

Kyle, did you see Christopher's comments/concerns? Otherwise, I'd also be happy to sign off on this.

> 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.

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

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:

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

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

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.

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.

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.

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?

Working on this.

... and still working on it right now :)

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.

> 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.

Hi Kyle,

have you been able to reproduce/fix the first issue with the javascript error in the OPAC?

(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.

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)

Just realized I didn't have all the patches... and that it might be a bit too late tonight for this

(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?

Kyle, ignore comment#166 - see comment#167.

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.

(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.

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.

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 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?

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.

(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?

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

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,

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?

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

> 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?

Script was inadvertently removed at some point. Restoring Passed QA status but feel free to re-QA.

Thx Nick for your testing!

I hope this appropriate to ask -- any chance this could get pushed to master soon, since it looks like it's passed QA?

(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.

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.

As the 3.22 feature flush approaches, I was wondering if this one was going to be considered a candidate for 3.22. Thanks.

My understanding is that if it's passed qa it's going to be considered for 3.22.

I'm sorry, could you please rebase?

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>

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>

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>

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>

Created attachment 46140 [details] [review]
Bug 9303 [QA Followup] - Restore missing biblio relationship

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Created attachment 46141 [details] [review]
Bug 9303 [QA Followup] - Restore ability for patron to control setting

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Created attachment 46142 [details] [review]
Bug 9303 [QA Followup] - Restore missing svc script

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Pushed to master!