Bug 8604 - Patron cards made for patrons which don't have patron images use preceding card's image
Summary: Patron cards made for patrons which don't have patron images use preceding ca...
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: master
Hardware: All All
: P1 - high major (vote)
Assignee: Chris Nighswonger
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-09 06:46 UTC by Derryn
Modified: 2018-08-28 13:16 UTC (History)
10 users (show)

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


Attachments
This patch corrects referential problems which caused the script to "re-use" images as well as other sorts of bad image behaviour. (7.91 KB, patch)
2018-07-04 15:36 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 8604: Patron cards made for patrons which don't have patron images use preceding card's image (8.07 KB, patch)
2018-07-09 09:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8604: Patron cards made for patrons which don't have patron images use preceding card's image (8.29 KB, patch)
2018-07-20 06:42 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Derryn 2012-08-09 06:46:37 UTC
Steps to reproduce:

1. In the patron creator, set up a layout with patron images enabled
2. Create a batch starting with a patron that does NOT have a picture stored
3. Next choose one that DOES
4. Finally choose one that does NOT
5. Export the batch
6. Resulting PDF will show
   i) the first one is correct (no picture)
   ii) the second one will correctly show it's own picture
   iii) the third one will (incorrectly) show the previous patron's picture
Comment 1 Chris Nighswonger 2013-07-18 19:02:07 UTC
Removing myself from the CC since I am the default assignee.
Comment 2 lightpurpledye 2014-10-01 16:24:01 UTC
This problem goes away if you comment out lines 152 - 159 of /usr/share/koha/intranet/cgi-bin/patroncards (lines beginning "elsif ($images->{$_}->{'data_source'}->[0]->{'image_source'} eq 'creator_images') {" ).

However, doing this also breaks the display of any "Other Image"s (non-patron photo images) on the card layout.

I have tried adding "$image_data = undef" to the beginning of the foreach loop, and between lines 152 and 153, under the assumption that $image_data still contains the image data from the previous iteration until it is reassigned, but this had no effect.

I think that for some reason the elsif condition on line 152 is matching users which don't have a photo assigned, but I still don't know why that would result in the behaviour observed.
Comment 3 Chris Nighswonger 2014-10-01 16:56:50 UTC
(In reply to lightpurpledye from comment #2)
> This problem goes away if you comment out lines 152 - 159 of
> /usr/share/koha/intranet/cgi-bin/patroncards (lines beginning "elsif
> ($images->{$_}->{'data_source'}->[0]->{'image_source'} eq 'creator_images')
> {" ).
> 
> However, doing this also breaks the display of any "Other Image"s
> (non-patron photo images) on the card layout.
> 

The file in question is create-pdf.pl

Your test indicates that at least on your system there may be patron images stored where "creator_images" should be. This entire block of code (PROCESS_IMAGES [1]) is prefaced on layout xml like this:

  <images name="image_1" Dx="139.4" Ox="0" Oy="0" Sx="0" Sy="0" Tx="0" Ty="0">
    <data_source image_name="Temp Card Front PNG" image_source="creator_images" />
  </images>

It looks at the data_source key in particular for the "source" of the image. "creator_images" are stored in a table by the same name in the db. This is a different source than patron images which are stored in the patronimage table.

The fact that commenting out the condition which allows for retrieval of creator_images "fixes" the problem would seem to indicate a misunderstanding of what images should be stored where. An image stored in the creator_images table and used in a patroncard layout will appear on every card in any batch using that layout. Images stored in the patronimage table will (should) appear only on cards which include that patron.

I have not had time to try this bug out, but what you describe seems to be more related to misunderstanding what images should be stored where. Hopefully that clarifies things a bit. If I've not understood, feel free to let me know.

[1] http://tinyurl.com/kfh5vbn
Comment 4 lightpurpledye 2014-10-01 18:07:35 UTC
Oops, I did mean create-pdf.pl, well spotted.

I don't think I've misunderstood where images should be stored, however. I only have one image in the creator_images table in the db, which I wish to be displayed on every card.

The patron images are all in the patronimage table (uploaded using the bulk Upload Patron Images tool). It's just that some patrons do not have images set, but creating a card for them still displays an image, but of a different patron.

N.B. The self-checkout module does not display any images for these patrons, and there is no image set for them in the patronimage table, so I am fairly sure it's a bug in the Patron Card Creator module rather than anything else.
Comment 5 Chris Nighswonger 2014-10-01 18:29:33 UTC
(In reply to lightpurpledye from comment #4)
> Oops, I did mean create-pdf.pl, well spotted.
> 
> I don't think I've misunderstood where images should be stored, however. I
> only have one image in the creator_images table in the db, which I wish to
> be displayed on every card.
> 
> The patron images are all in the patronimage table (uploaded using the bulk
> Upload Patron Images tool). It's just that some patrons do not have images
> set, but creating a card for them still displays an image, but of a
> different patron.

So it sounds like your images are in the correct location. However, that only makes it a bit more confusing as to why commenting out code which inserts the image for every card would "fix" the duplicate patron image issue. So I'll just have to find time to duplicate the bug and look at what's going on there.
Comment 6 lightpurpledye 2014-10-02 12:00:18 UTC
Possibly related: If the image file for one patron has a greater horizontal:vertical ratio than the image file for the patron immediately after them in the batch, then the first patron's image is shown below the second patron's image.

e.g. Steps to reproduce
1. In the patron creator, set up a layout with patron images enabled.
2. Create a batch starting with a patron that has a 300x100 image stored.
3. Next choose one with a 200x100 image.
4. Export the batch.
5. Resulting PDF will show:
   i) The first one is correct (patron a's picture)
   ii) The second one is incorrect (There are 100 pixels of patron a showing underneath patron b).

This suggests to me that when the PROCESS_IMAGES loop has an iteration it actually prints the image in every subsequent image position and then relies on the incorrect images being printed over in the next iteration. This would explain why, if a patron does not have an image stored, they would get the previous patron's image.

If this turns out to be unrelated, I can open a separate bug report for it.
Comment 7 Marc Véron 2015-04-30 21:19:25 UTC
Is this still valid for current master?
Comment 8 Katrin Fischer 2018-07-02 17:17:49 UTC
We've just seen this behaviour in 17.11. The problem seems to appear as soon as another graphic in addition to the photo is added to the patron cards.

This should be fixed. If it goes unnoticed, this could be quite a privacy issue.
Comment 9 Chris Nighswonger 2018-07-02 20:32:31 UTC
(In reply to Derryn from comment #0)
> Steps to reproduce:
> 
> 1. In the patron creator, set up a layout with patron images enabled
> 2. Create a batch starting with a patron that does NOT have a picture stored
> 3. Next choose one that DOES
> 4. Finally choose one that does NOT
> 5. Export the batch
> 6. Resulting PDF will show
>    i) the first one is correct (no picture)
>    ii) the second one will correctly show it's own picture
>    iii) the third one will (incorrectly) show the previous patron's picture

I cannot duplicate this in 18.05.01.000. Maybe you can attach a screen-shot of your layout.
Comment 10 Chris Nighswonger 2018-07-02 20:38:45 UTC
(In reply to lightpurpledye from comment #6)
> Possibly related: If the image file for one patron has a greater
> horizontal:vertical ratio than the image file for the patron immediately
> after them in the batch, then the first patron's image is shown below the
> second patron's image.
> 
> e.g. Steps to reproduce
> 1. In the patron creator, set up a layout with patron images enabled.
> 2. Create a batch starting with a patron that has a 300x100 image stored.
> 3. Next choose one with a 200x100 image.
> 4. Export the batch.
> 5. Resulting PDF will show:
>    i) The first one is correct (patron a's picture)
>    ii) The second one is incorrect (There are 100 pixels of patron a showing
> underneath patron b).
> 
> This suggests to me that when the PROCESS_IMAGES loop has an iteration it
> actually prints the image in every subsequent image position and then relies
> on the incorrect images being printed over in the next iteration. This would
> explain why, if a patron does not have an image stored, they would get the
> previous patron's image.
> 
> If this turns out to be unrelated, I can open a separate bug report for it.

I cannot duplicate this in 18.05.01.000 either.
Comment 11 Chris Nighswonger 2018-07-02 20:40:00 UTC
(In reply to Katrin Fischer from comment #8)
> We've just seen this behaviour in 17.11. The problem seems to appear as soon
> as another graphic in addition to the photo is added to the patron cards.
> 
> This should be fixed. If it goes unnoticed, this could be quite a privacy
> issue.

Katrin, can you provide exact steps to duplicate the problem you are seeing? Also, can you confirm the issue exists in 18.05.01.000?

I seem to be unable to reproduce any of the variations of this bug locally on 18.05.01.000.
Comment 12 Chris Nighswonger 2018-07-02 20:47:30 UTC
Better yet, could someone(s) provide me with the results of the following query:

SELECT layout_xml FROM creator_layouts\G
Comment 13 Christian Stelzenmüller 2018-07-03 09:30:22 UTC
Here are the results from your requested query:

<opt guide_box="1" guide_grid="1" page_side="F" units="MM">
  <images name="image_1" Dx="50" Ox="0" Oy="0" Sx="0" Sy="0" Tx="50" Ty="30">
    <data_source image_name="none" image_source="patronimages" />
  </images>
  <images name="image_2" Dx="25" Ox="0" Oy="0" Sx="0" Sy="0" Tx="7" Ty="12">
    <data_source image_name="BSZ" image_source="creator_images" />
  </images>
  <text>BSZ Bibliothek</text>
  <text enable="1" font="HB" font_size="12" llx="7" lly="40" text_alignment="L" />
  <text>Ausweis</text>
  <text enable="1" font="H" font_size="10" llx="7" lly="33" text_alignment="L" />
  <text>&lt;firstname&gt; &lt;surname&gt;</text>
  <text font="H" font_size="10" llx="7" lly="26" text_alignment="L" />
</opt>
Comment 14 Chris Nighswonger 2018-07-03 13:05:13 UTC
(In reply to Christian Stelzenmüller from comment #13)
> Here are the results from your requested query:
> 
> <opt guide_box="1" guide_grid="1" page_side="F" units="MM">
>   <images name="image_1" Dx="50" Ox="0" Oy="0" Sx="0" Sy="0" Tx="50" Ty="30">
>     <data_source image_name="none" image_source="patronimages" />
>   </images>
>   <images name="image_2" Dx="25" Ox="0" Oy="0" Sx="0" Sy="0" Tx="7" Ty="12">
>     <data_source image_name="BSZ" image_source="creator_images" />
>   </images>
>   <text>BSZ Bibliothek</text>
>   <text enable="1" font="HB" font_size="12" llx="7" lly="40"
> text_alignment="L" />
>   <text>Ausweis</text>
>   <text enable="1" font="H" font_size="10" llx="7" lly="33"
> text_alignment="L" />
>   <text>&lt;firstname&gt; &lt;surname&gt;</text>
>   <text font="H" font_size="10" llx="7" lly="26" text_alignment="L" />
> </opt>

Just to confirm: This layout does produce one of the above described symptoms?
Comment 15 Katrin Fischer 2018-07-03 13:21:37 UTC
Yes
Comment 16 Chris Nighswonger 2018-07-04 15:36:06 UTC
Created attachment 76690 [details] [review]
This patch corrects referential problems which caused the script to "re-use" images as well as other sorts of bad image behaviour.
Comment 17 Chris Nighswonger 2018-07-04 15:42:56 UTC
Comment on attachment 76690 [details] [review]
This patch corrects referential problems which caused the script to "re-use" images as well as other sorts of bad image behaviour.

Some kind soul please correct the spelling of "relegated" on line 180 during QA!
Comment 18 Katrin Fischer 2018-07-09 09:30:14 UTC
Created attachment 76779 [details] [review]
Bug 8604: Patron cards made for patrons which don't have patron images use preceding card's image

This patch corrects referential problems which caused the script to
"re-use" images as well as other sorts of bad image behaviour.

To test:

1. Ensure that you can reproduce the original bug or some variation thereof
using the steps described in either of these two comments:

    https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8604#c0

    https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8604#c6

    NOTE: This is not an easy bug to reproduce.

2. Once you can reproduce the bug, apply this patch and follow the same steps
used to reproduce the bug.

3. Observe that the symptoms are corrected.

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>
Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Comment 19 Marcel de Rooy 2018-07-13 07:45:30 UTC
Chris:
I have the impression that you are clearing the wrong variables at the end of the for loop. You undef image, alt_image and binary_data. These three vars are all created in the for loop. I guess they should not present a problem? But $image_data is created outside the loop and is not cleared. Isn't that the cause of the issue?

Look at
my $binary_data = $image_data->{'imagefile'}

Since $image_data is not cleared, bad things may still happen??
Changing status for feedback.
Comment 20 Marcel de Rooy 2018-07-13 07:47:13 UTC
(In reply to Chris Nighswonger from comment #17)
> Some kind soul please correct the spelling of "relegated" on line 180 during
> QA!
Your kind soul? :)
Comment 21 Chris Nighswonger 2018-07-13 10:20:34 UTC
Changing status to more accurately reflect the current discussion.
Comment 22 Chris Nighswonger 2018-07-13 10:40:26 UTC
(In reply to Marcel de Rooy from comment #19)
> Chris:
> I have the impression

Just an impression? ;-) Did you test and find a case where this fix does not work?

 that you are clearing the wrong variables at the end
> of the for loop. You undef image, alt_image and binary_data. These three
> vars are all created in the for loop. I guess they should not present a
> problem? But $image_data is created outside the loop and is not cleared.
> Isn't that the cause of the issue?

I don't think so.

> 
> Look at
> my $binary_data = $image_data->{'imagefile'}
> 
> Since $image_data is not cleared, bad things may still happen??

$binary_data is a reference to $image_data->{'imagefile'} which, in turn, is a reference to a hash created by DBI containing the results of the SELECT. DBI creates a new hash on every row retrieval[1], ensuring that the data contained at our reference is fresh every time.

Image::Magick, on the other hand, does not automatically "destroy" the original structure on each call, and so we need to take care of that.[2]


[1] "By default a reference to a new hash is returned for each row." https://metacpan.org/pod/DBI#fetchrow_hashref

[2] "Once you are finished with a PerlMagick object you should consider destroying it." http://www.graphicsmagick.org/perl.html
Comment 23 Katrin Fischer 2018-07-13 11:25:10 UTC
Pushing this back into the QA queue (for discussion between QA and developer we usually don't use "In Discussion" - only when you can't agree)
Comment 24 Marcel de Rooy 2018-07-16 07:20:42 UTC
(In reply to Chris Nighswonger from comment #22)
> (In reply to Marcel de Rooy from comment #19)
> > Chris:
> > I have the impression
> 
> Just an impression? ;-) Did you test and find a case where this fix does not
> work?
Please keep in mind that we both have the same goal: fix bugs and improve code. QAing is more than testing. A careful look at the code can in my experience reveal bugs just as easy or show you what to test.

> $binary_data is a reference to $image_data->{'imagefile'} which, in turn, is
> a reference to a hash created by DBI containing the results of the SELECT.
> DBI creates a new hash on every row retrieval[1], ensuring that the data
> contained at our reference is fresh every time.
The crux is here indeed. This is only partially true. The code does only update $image_data in two of the if statement branches. So my observation and request is just to clear $image_data each time in the loop or put its definition into the loop. Clearing the other variables might be unneeded since they are created in the loop.
So this reveals a test case: Make sure that record 1 in the loop is a patron image and record 2 comes in the eq 'none' branch. Record 2 will use the old value of $image_data.

> Image::Magick, on the other hand, does not automatically "destroy" the
> original structure on each call, and so we need to take care of that.[2]
Fine with me.
Comment 25 Marcel de Rooy 2018-07-16 07:21:30 UTC
(In reply to Katrin Fischer from comment #23)
> Pushing this back into the QA queue (for discussion between QA and developer
> we usually don't use "In Discussion" - only when you can't agree)
It is a FQA which I have set the last time.
Comment 26 Katrin Fischer 2018-07-16 07:34:03 UTC
I think setting back to SO after the dev has commented or provided a follow-up is process-wise ok and standard - requires another check then by QA.
Comment 27 Marcel de Rooy 2018-07-16 07:36:14 UTC
(In reply to Katrin Fischer from comment #26)
> I think setting back to SO after the dev has commented or provided a
> follow-up is process-wise ok and standard - requires another check then by
> QA.

No problem. But normally this process between author and QAer does not need additional interfering.
Comment 28 Katrin Fischer 2018-07-16 08:19:43 UTC
This is a privacy issue, I just didn't want it to die "in discussion".
Comment 29 Marcel de Rooy 2018-07-16 08:22:00 UTC
(In reply to Katrin Fischer from comment #28)
> This is a privacy issue, I just didn't want it to die "in discussion".

Yeah it is. No problem! Thx for following up.. Imo we are just clearing one variable away from PQA. I only need to convince Chris..
Comment 30 Chris Nighswonger 2018-07-16 14:43:30 UTC
(In reply to Marcel de Rooy from comment #24)
> (In reply to Chris Nighswonger from comment #22)
> > (In reply to Marcel de Rooy from comment #19)
> > > Chris:
> > > I have the impression
> > 
> > Just an impression? ;-) Did you test and find a case where this fix does not
> > work?
> Please keep in mind that we both have the same goal: fix bugs and improve
> code. QAing is more than testing. A careful look at the code can in my
> experience reveal bugs just as easy or show you what to test.

No offense intended. I understand that we have the same goal. I appreciate your contribution to the QA process. Please keep in mind that my time is extremely limited. Much more so than devs who are paid to work on Koha as I am paid to work on other projects. When you provide QA feedback without a supporting proof-case, it means that the dev has to spend time ensuring 1. your observation is valid and 2. Ensuring that the fix does not cause other problems. Time is expensive to be spending on an impression without accompanying demonstrable failure cases.

> 
> > $binary_data is a reference to $image_data->{'imagefile'} which, in turn, is
> > a reference to a hash created by DBI containing the results of the SELECT.
> > DBI creates a new hash on every row retrieval[1], ensuring that the data
> > contained at our reference is fresh every time.
> The crux is here indeed. This is only partially true. The code does only
> update $image_data in two of the if statement branches. So my observation
> and request is just to clear $image_data each time in the loop or put its
> definition into the loop. Clearing the other variables might be unneeded
> since they are created in the loop.
> So this reveals a test case: Make sure that record 1 in the loop is a patron
> image and record 2 comes in the eq 'none' branch. Record 2 will use the old
> value of $image_data.

FTR: I have no problem undefing the entire $image_data reference. As soon as I have time, I will submit a follow-up patch. I also inadvertently removed the statements which short-circuit the loop in the case of no image data. That needs to be corrected as well.

That said: Your suggested case does not cause the failure you suggest. I understand that $image_data never drops out of scope during the loop execution. I understand that in theory the reference can contain "stale" data. I just have not been handed and cannot establish a test case where it does.
Comment 31 Katrin Fischer 2018-07-16 21:15:18 UTC
I feel like Chris makes some valid points here when he says there is no proof that the additional changes are needed. We have tested this patch and found it to fix the problem we had reported. Patron images don't longer overlay and repeat.

If we change the code further - what is the bug it fixes? What should be in the test plan?


>FTR: I have no problem undefing the entire $image_data reference. As soon as I >have time, I will submit a follow-up patch. I also inadvertently removed the >statements which short-circuit the loop in the case of no image data. That needs >to be corrected as well.

Does this mean the patch should not be used as is? 

Could the additional requested changes moved to another bug report for attending to them later?
Comment 32 Marcel de Rooy 2018-07-17 12:14:30 UTC
I will finish this up Friday. No need to make it more difficult. This can wait until then.
Comment 33 Marcel de Rooy 2018-07-20 06:42:33 UTC
Created attachment 77150 [details] [review]
Bug 8604: Patron cards made for patrons which don't have patron images use preceding card's image

This patch corrects referential problems which caused the script to
"re-use" images as well as other sorts of bad image behaviour.

To test:

1. Ensure that you can reproduce the original bug or some variation thereof
using the steps described in either of these two comments:

    https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8604#c0

    https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8604#c6

    NOTE: This is not an easy bug to reproduce.

2. Once you can reproduce the bug, apply this patch and follow the same steps
used to reproduce the bug.

3. Observe that the symptoms are corrected.

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>
Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Reproduced the bug and resolved it with this patch.
Code is hard to read; exact cause of the problem was not fully uncovered.
Comment 34 Marcel de Rooy 2018-07-20 06:48:48 UTC
Chris:
You fixed the problem without clearing $image_data too. Further testing and looking at the code now helped me reach that conclusion. Sorry for the misunderstanding raised.
What remains, is what was the exact cause if it was not $image_data? Might be a problem in Graphics::Magick or in draw_image or still somewhere else?? My time is limited too..

Passing QA, no follow-ups.
Comment 35 Nick Clemens 2018-07-23 15:39:27 UTC
Awesome work all!

Pushed to master for 18.11
Comment 36 Martin Renvoize 2018-08-02 14:22:20 UTC
Pushed to 18.05.x for 18.05.03
Comment 37 Fridolin SOMERS 2018-08-28 13:16:06 UTC
Pushed to 17.11.x for 17.11.09