Bug 3516 - Display picture of patrons in OPAC
Summary: Display picture of patrons in OPAC
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Owen Leonard
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-10 01:04 UTC by Ata ur Rehman
Modified: 2012-10-25 23:09 UTC (History)
6 users (show)

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


Attachments
proposed display (63.36 KB, image/png)
2009-08-13 00:38 UTC, Chris Cormack
Details
patch submitted (2.48 KB, patch)
2009-08-13 00:41 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 3516 - Display picture of patrons in OPAC (9.48 KB, patch)
2012-02-17 14:06 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 3516 [REVISED] Display picture of patrons in OPAC (8.91 KB, patch)
2012-02-17 14:53 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 3516 [REVISED] Display picture of patrons in OPAC (9.09 KB, patch)
2012-02-20 06:56 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 3516 [REVISED] Display picture of patrons in OPAC (8.91 KB, patch)
2012-03-01 16:33 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 3516 [FOLLOW-UP] Display picture of patrons in OPAC (2.24 KB, patch)
2012-03-01 16:35 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:11:38 UTC


---- Reported by ata.rehman@gmail.com 2009-08-10 13:04:58 ----

Sir/Madam

I was looking for some way to put the picture of my patron / students in OPAC.  I just talked to Ms Nicole and she suggested to submit this issue at bugzilla.



---- Additional Comments From nengard@gmail.com 2009-08-13 00:38:02 ----

Created an attachment
proposed display





---- Additional Comments From nengard@gmail.com 2009-08-13 00:41:34 ----

Created an attachment
patch submitted





--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:11 UTC  ---

This bug was previously known as _bug_ 3516 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3516
Imported an attachment (id=1379)
Imported an attachment (id=1380)

Actual time not defined. Setting to 0.0
The original submitter of attachment 1379 [details] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.
The original submitter of attachment 1380 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.

Comment 1 Nicole C. Engard 2010-06-02 11:40:21 UTC
not sure how to fix this - reassigning to default assignee
Comment 2 Owen Leonard 2010-06-02 13:25:51 UTC
Some follow-up on this bug from the koha-patches list:

On Wed, Aug 12, 2009 at 9:46 PM, Galen Charlton <gmcharlt@gmail.com> wrote:
> Hi,
>
> On Wed, Aug 12, 2009 at 8:40 PM, Nicole Engard<nengard@gmail.com> wrote:
>> This patch requires that the staffClientBaseURL
>> system preference is set in order to show the
>> patron image in the OPAC.
>
> This won't work reliably - a patron who is logged into the OPAC will
> not in general have valid credentials or a valid session to pull stuff
> from the staff interface.  It is necessary to copy the patron image
> service to the OPAC - the pending patches for the web-based self
> checkout enhancements include an example of how to do this.
Comment 3 Owen Leonard 2012-02-17 14:06:25 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2012-02-17 14:39:10 UTC
The script devel/misc/testbugzillapatches.pl has detected that this bug does not apply anymore, so i'm marking this bug as "does not apply".

The script is supposed to handle conflicts related to updatedatabase, but no other cases of merge conflict. So the conflict may be trivial to fix.

If it happens that the patch attached does apply cleanly, please send me an email, there's something wrong in the script. It's possible, as it's a recent script.
If the patch applies does not apply as I think, please rebase and resubmit, thanks
Comment 5 Owen Leonard 2012-02-17 14:53:23 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2012-02-20 06:56:30 UTC
Created attachment 7768 [details] [review]
[SIGNED-OFF] Bug 3516 [REVISED] Display picture of patrons in OPAC

This patch adapts the patron image display script from the
self-checkout module to optionally display the image of the
logged-in user on their personal information page (opac-userupdate.pl).
The script changes the method choosing which image to display
from using a passed cardnumber to using the borrowernumber of
the logged-in user.

A system preference is added, OPACpatronimages, to control
whether these images are displayed.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Verified that patron image shows only when system preference is turned on
and that there are no display problems for patrons without patron image.
Comment 7 Marcel de Rooy 2012-02-27 08:47:53 UTC
Code looks good. Setting status to Passed QA.
My only remark would be that GetPatronImage is called twice and this probably could be done differently. In opac-userupdate the image is collected, but the only thing done is setting a flag. In the template, GetPatronImage is called the second time when the flag is set.
Comment 8 Paul Poulain 2012-02-27 12:35:32 UTC
2 questions before pushing this patch:
* is the syspref really needed ? If the librar(ian) take a picture of someone, then -at least in France-, it's asked by the law to give someone access to this information. I also can't see a use case where we would like to have it not displayed.

* in the templates, there is a hardcoded style:
<p style="border:1px solid #EEE;padding:1em;">
there should not be here, right ?
Comment 9 Owen Leonard 2012-02-27 14:10:37 UTC
(In reply to comment #8)
> * is the syspref really needed ? If the librar(ian) take a picture of someone,
> then -at least in France-, it's asked by the law to give someone access to this
> information. I also can't see a use case where we would like to have it not
> displayed.

I could see a library deciding that it increased concern about what data is available on in-library OPACs when a patron hasn't properly logged out. I think it would be much safer to keep the preference. 
 
> * in the templates, there is a hardcoded style:
> <p style="border:1px solid #EEE;padding:1em;">
> there should not be here, right ?

I sometimes use an inline style for something which is unique, but I see that it might present an obstacle for customization. I can submit a follow-up to move it to opac.css.
Comment 10 Paul Poulain 2012-02-27 14:24:25 UTC
(In reply to comment #9)
> (In reply to comment #8)
> > * is the syspref really needed ? If the librar(ian) take a picture of someone,
> > then -at least in France-, it's asked by the law to give someone access to this
> > information. I also can't see a use case where we would like to have it not
> > displayed.
> 
> I could see a library deciding that it increased concern about what data is
> available on in-library OPACs when a patron hasn't properly logged out. I think
> it would be much safer to keep the preference. 
I'm not really convinced by this argument, but i'll accept it.
There's another reason why it would be usefull: some libraries may not want to have images displayed after upgrading because some pictures maybe ... how to say ... not the real one, risking to upset the patron...

> > * in the templates, there is a hardcoded style:
> > <p style="border:1px solid #EEE;padding:1em;">
> > there should not be here, right ?
> 
> I sometimes use an inline style for something which is unique, but I see that
> it might present an obstacle for customization. I can submit a follow-up to
> move it to opac.css.

please do (no need to change the bug status, i'll take care of the follow-up)
Comment 11 Marcel de Rooy 2012-02-27 14:30:24 UTC
If you do not want to show pictures, you should not upload them. When not logging out, the information on personal details is more sensitive than the picture (address, birth date, etc.)
How would a "funny" picture come up after an upgrade?
Comment 12 Owen Leonard 2012-02-27 14:49:04 UTC
(In reply to comment #11)
> If you do not want to show pictures, you should not upload them.

Is the system preference really such a problem?

> When not
> logging out, the information on personal details is more sensitive than the
> picture (address, birth date, etc.)

A patron might feel that the combination of their photo + their personal details is more sensitive than just their personal details.
Comment 13 Katrin Fischer 2012-02-27 14:52:18 UTC
I think there might be different use cases here and different laws. 
I think a system preference is a good idea to add enough flexibility.
Comment 14 Chris Cormack 2012-02-27 18:23:15 UTC
+1 for the syspref from me too. If a hosted Library wants to switch this off for whatever reason (and who are we to decide what are valid reasons?) They don't have access to the file system to go deleting files.
Comment 15 Paul Poulain 2012-02-28 08:56:08 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > If you do not want to show pictures, you should not upload them.
> 
> Is the system preference really such a problem?
You know, I'm a founder and member of AROSS (Association for Removal of Superfluous Systempreferences), so i'm always wondering when I see a new one. Because the more there are, the hardest it is for a newbie to find find core things. I think/feel it could be usefull to separate "central" systempreferences from "secondary" ones.
For example IndependantBranches is a major one, the one introduced in this patch is a secondary one.

but that's another topic, let's go with this new one.

> > > * in the templates, there is a hardcoded style:
> > > <p style="border:1px solid #EEE;padding:1em;">
> > > there should not be here, right ?
> > 
> > I sometimes use an inline style for something which is unique, but I see that
> > it might present an obstacle for customization. I can submit a follow-up to
> > move it to opac.css.
please, submite a follow-up for this, I'll push the patch.
Comment 16 Owen Leonard 2012-03-01 16:33:21 UTC Comment hidden (obsolete)
Comment 17 Owen Leonard 2012-03-01 16:34:02 UTC
Comment on attachment 7972 [details] [review]
Bug 3516 [REVISED] Display picture of patrons in OPAC

Sorry, wrong patch.
Comment 18 Owen Leonard 2012-03-01 16:35:47 UTC
Created attachment 7973 [details] [review]
Bug 3516 [FOLLOW-UP] Display picture of patrons in OPAC

    This patch moves inline CSS to opac.css following QA comments.
    Doing so improves users ability to customize the affected
    markup with custom CSS.