There are 3 subroutines in C4::Members to get, add and delete patron images. By creating 2 Koha::Patron::Image[s] classes, we could remove them.
Created attachment 47122 [details] [review] Bug 15635: Koha::Patron::Images - Add new classes There are 3 subroutines in C4::Members to get, add and delete patron images: - GetPatronImage - PutPatronImage - RmPatronImage By creating these 2 Koha::Patron::Image[s] classes, we could remove them easily.
Created attachment 47124 [details] [review] Bug 15635: Koha::Patron::Images - Remove PutPatronImage The C4::Members::PutPatronImage inserted/updated the image of a patron. This can be done easily with ->find->set->store or ->new->store Test plan: 1/ Modify the image of a patron from the patron detail page 2/ Add an image to a new patron 3/ Use the "Upload patron images" tools (tools/picture-upload.pl) to add or modify the image of a patron 4/ Use the "Upload patron images" tools (tools/picture-upload.pl) to add or modify the image of several patrons, using a zip file. Stress the script trying to get as many errors as possible (wrong cardnumber, wrong mimetype, file does not exist, etc.) With this patch, if the cardnumber does not exist, you will get a specific error "Image not imported because this patron does not exist in the database"
Created attachment 47125 [details] [review] Bug 15635: Koha::Patron::Images - Remove RmPatronImage The C4::Members::RmPatronImage just removed a row in patronimage. This can be accomplished using the delete method of Koha::Patron::Image. Test plan: From the patron defail page, try to delete the image of a patron.
Created attachment 47126 [details] [review] Bug 15635: Koha::Patron::Images - Remove GetPatronImage To retrieve a patron image, we can call Koha::Patron::Images->find or Koha::Patrons->find->image Both will return a Koha::Patron::Image object. Test plan: 1/ From the patron/member module, open all tabs on the left (Checkouts, detail, fines, etc.) The image should be correctly displayed. 2/ At the OPAC, on the patron details page (opac-memberentry.pl) the image should be displayed as well. 3/ Same on the sco module.
Comment on attachment 47125 [details] [review] Bug 15635: Koha::Patron::Images - Remove RmPatronImage Review of attachment 47125 [details] [review]: ----------------------------------------------------------------- ::: tools/picture-upload.pl @@ +163,5 @@ > + my $deleted = eval { > + Koha::Patron::Images->find( $borrowernumber )->delete; > + }; > + if ( $@ or not $deleted ) { > + warn "Image for patron '$borrowernumber' has not been deleted"; Should we not distinguish between $@ and not $deleted?
Comment on attachment 47126 [details] [review] Bug 15635: Koha::Patron::Images - Remove GetPatronImage Review of attachment 47126 [details] [review]: ----------------------------------------------------------------- ::: opac/opac-memberentry.pl @@ +232,5 @@ > ); > > if (C4::Context->preference('OPACpatronimages')) { > + my $patron_image = Koha::Patron::Images->find($borrower->{borrowernumber}); > + $template->param( display_patron_image => 1 ) if $patron_image; After this many changes, I can understand a cut-and-paste mentality. :)
(In reply to M. Tompsett from comment #5) > Comment on attachment 47125 [details] [review] [review] > Bug 15635: Koha::Patron::Images - Remove RmPatronImage > > Review of attachment 47125 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: tools/picture-upload.pl > @@ +163,5 @@ > > + my $deleted = eval { > > + Koha::Patron::Images->find( $borrowernumber )->delete; > > + }; > > + if ( $@ or not $deleted ) { > > + warn "Image for patron '$borrowernumber' has not been deleted"; > > Should we not distinguish between $@ and not $deleted? It's what I did for other admin scripts, we could display the error ($@) if needed but could be done later.
(In reply to M. Tompsett from comment #6) > Comment on attachment 47126 [details] [review] [review] > Bug 15635: Koha::Patron::Images - Remove GetPatronImage > > Review of attachment 47126 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: opac/opac-memberentry.pl > @@ +232,5 @@ > > ); > > > > if (C4::Context->preference('OPACpatronimages')) { > > + my $patron_image = Koha::Patron::Images->find($borrower->{borrowernumber}); > > + $template->param( display_patron_image => 1 ) if $patron_image; > > After this many changes, I can understand a cut-and-paste mentality. :) What's wrong? I don't manage to see the error :)
Could you please rebase?
Created attachment 48113 [details] [review] Bug 15635: Koha::Patron::Images - Add new classes There are 3 subroutines in C4::Members to get, add and delete patron images: - GetPatronImage - PutPatronImage - RmPatronImage By creating these 2 Koha::Patron::Image[s] classes, we could remove them easily.
Created attachment 48114 [details] [review] Bug 15635: Koha::Patron::Images - Remove PutPatronImage The C4::Members::PutPatronImage inserted/updated the image of a patron. This can be done easily with ->find->set->store or ->new->store Test plan: 1/ Modify the image of a patron from the patron detail page 2/ Add an image to a new patron 3/ Use the "Upload patron images" tools (tools/picture-upload.pl) to add or modify the image of a patron 4/ Use the "Upload patron images" tools (tools/picture-upload.pl) to add or modify the image of several patrons, using a zip file. Stress the script trying to get as many errors as possible (wrong cardnumber, wrong mimetype, file does not exist, etc.) With this patch, if the cardnumber does not exist, you will get a specific error "Image not imported because this patron does not exist in the database"
Created attachment 48115 [details] [review] Bug 15635: Koha::Patron::Images - Remove RmPatronImage The C4::Members::RmPatronImage just removed a row in patronimage. This can be accomplished using the delete method of Koha::Patron::Image. Test plan: From the patron defail page, try to delete the image of a patron.
Created attachment 48116 [details] [review] Bug 15635: Koha::Patron::Images - Remove GetPatronImage To retrieve a patron image, we can call Koha::Patron::Images->find or Koha::Patrons->find->image Both will return a Koha::Patron::Image object. Test plan: 1/ From the patron/member module, open all tabs on the left (Checkouts, detail, fines, etc.) The image should be correctly displayed. 2/ At the OPAC, on the patron details page (opac-memberentry.pl) the image should be displayed as well. 3/ Same on the sco module.
Created attachment 48117 [details] [review] Bug 15635: Change method type to _type for bug 15446
I tested these patches and they work fine for me. I found just few issues: when I tried to upload "bad" file (pdf, too big jpg, etc.), the file is not uploaded (of course) but no error message appears... Also I wasn't able to see "Image not imported because this patron does not exist in the database" error message. Maybe I'm doing something wrong... there are error messages in templates as I can see...
(In reply to Josef Moravec from comment #15) > I tested these patches and they work fine for me. I found just few issues: > when I tried to upload "bad" file (pdf, too big jpg, etc.), the file is not > uploaded (of course) but no error message appears... Also I wasn't able to > see "Image not imported because this patron does not exist in > the database" error message. > > Maybe I'm doing something wrong... there are error messages in templates as > I can see... Did you test without this patchset? Just to know if I have to submit follow-ups or open a new bug report.
Created attachment 48136 [details] [review] Bug 15635: Koha::Patron::Images - Add new classes There are 3 subroutines in C4::Members to get, add and delete patron images: - GetPatronImage - PutPatronImage - RmPatronImage By creating these 2 Koha::Patron::Image[s] classes, we could remove them easily. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 48137 [details] [review] Bug 15635: Koha::Patron::Images - Remove PutPatronImage The C4::Members::PutPatronImage inserted/updated the image of a patron. This can be done easily with ->find->set->store or ->new->store Test plan: 1/ Modify the image of a patron from the patron detail page 2/ Add an image to a new patron 3/ Use the "Upload patron images" tools (tools/picture-upload.pl) to add or modify the image of a patron 4/ Use the "Upload patron images" tools (tools/picture-upload.pl) to add or modify the image of several patrons, using a zip file. Stress the script trying to get as many errors as possible (wrong cardnumber, wrong mimetype, file does not exist, etc.) With this patch, if the cardnumber does not exist, you will get a specific error "Image not imported because this patron does not exist in the database" Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 48138 [details] [review] Bug 15635: Koha::Patron::Images - Remove RmPatronImage The C4::Members::RmPatronImage just removed a row in patronimage. This can be accomplished using the delete method of Koha::Patron::Image. Test plan: From the patron defail page, try to delete the image of a patron. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 48139 [details] [review] Bug 15635: Koha::Patron::Images - Remove GetPatronImage To retrieve a patron image, we can call Koha::Patron::Images->find or Koha::Patrons->find->image Both will return a Koha::Patron::Image object. Test plan: 1/ From the patron/member module, open all tabs on the left (Checkouts, detail, fines, etc.) The image should be correctly displayed. 2/ At the OPAC, on the patron details page (opac-memberentry.pl) the image should be displayed as well. 3/ Same on the sco module. Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 48140 [details] [review] Bug 15635: Change method type to _type for bug 15446 Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
(In reply to Jonathan Druart from comment #16) > (In reply to Josef Moravec from comment #15) > > I tested these patches and they work fine for me. I found just few issues: > > when I tried to upload "bad" file (pdf, too big jpg, etc.), the file is not > > uploaded (of course) but no error message appears... Also I wasn't able to > > see "Image not imported because this patron does not exist in > > the database" error message. > > > > Maybe I'm doing something wrong... there are error messages in templates as > > I can see... > > Did you test without this patchset? > Just to know if I have to submit follow-ups or open a new bug report. I've just tested it on clean master and there is the same issue. So as it looks that it's not related with these patches, I'm signing off.
Created attachment 48243 [details] [review] Bug 15635: Koha::Patron::Images - Add new classes There are 3 subroutines in C4::Members to get, add and delete patron images: - GetPatronImage - PutPatronImage - RmPatronImage By creating these 2 Koha::Patron::Image[s] classes, we could remove them easily. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 48244 [details] [review] Bug 15635: Koha::Patron::Images - Add new classes There are 3 subroutines in C4::Members to get, add and delete patron images: - GetPatronImage - PutPatronImage - RmPatronImage By creating these 2 Koha::Patron::Image[s] classes, we could remove them easily. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 48245 [details] [review] Bug 15635: Koha::Patron::Images - Remove PutPatronImage The C4::Members::PutPatronImage inserted/updated the image of a patron. This can be done easily with ->find->set->store or ->new->store Test plan: 1/ Modify the image of a patron from the patron detail page 2/ Add an image to a new patron 3/ Use the "Upload patron images" tools (tools/picture-upload.pl) to add or modify the image of a patron 4/ Use the "Upload patron images" tools (tools/picture-upload.pl) to add or modify the image of several patrons, using a zip file. Stress the script trying to get as many errors as possible (wrong cardnumber, wrong mimetype, file does not exist, etc.) With this patch, if the cardnumber does not exist, you will get a specific error "Image not imported because this patron does not exist in the database" Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 48246 [details] [review] Bug 15635: Koha::Patron::Images - Remove RmPatronImage The C4::Members::RmPatronImage just removed a row in patronimage. This can be accomplished using the delete method of Koha::Patron::Image. Test plan: From the patron defail page, try to delete the image of a patron. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 48247 [details] [review] Bug 15635: Koha::Patron::Images - Remove GetPatronImage To retrieve a patron image, we can call Koha::Patron::Images->find or Koha::Patrons->find->image Both will return a Koha::Patron::Image object. Test plan: 1/ From the patron/member module, open all tabs on the left (Checkouts, detail, fines, etc.) The image should be correctly displayed. 2/ At the OPAC, on the patron details page (opac-memberentry.pl) the image should be displayed as well. 3/ Same on the sco module. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 48248 [details] [review] Bug 15635: Change method type to _type for bug 15446 Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 48249 [details] [review] Bug 15635 [QA Followup] - Fix number of unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Didn't apply cleanly - I know because of the moving target that master is. Please rebase. Once it's done - leave me an @later in #koha and I'll jump right on it. Thank you so much for the time and bother of rebasing. Brendan
Bug 15548 has been rebased, they apply now cleanly.
Pushed to Master, should be in the May 2106 Release. Thanks Jonathan!