Bug 15635 - Move the patron images related code to Koha::Patron::Images
Summary: Move the patron images related code to Koha::Patron::Images
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 15353 15548
Blocks: 15449 16747
  Show dependency treegraph
 
Reported: 2016-01-21 14:29 UTC by Jonathan Druart
Modified: 2017-12-07 22:16 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Manual merge request:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15635: Koha::Patron::Images - Add new classes (5.78 KB, patch)
2016-01-21 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove PutPatronImage (7.53 KB, patch)
2016-01-21 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove RmPatronImage (2.37 KB, patch)
2016-01-21 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove GetPatronImage (27.61 KB, patch)
2016-01-21 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Add new classes (5.78 KB, patch)
2016-02-16 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove PutPatronImage (7.53 KB, patch)
2016-02-16 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove RmPatronImage (2.37 KB, patch)
2016-02-16 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove GetPatronImage (27.50 KB, patch)
2016-02-16 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15635: Change method type to _type for bug 15446 (1018 bytes, patch)
2016-02-16 11:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Add new classes (5.83 KB, patch)
2016-02-16 14:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove PutPatronImage (7.58 KB, patch)
2016-02-16 14:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove RmPatronImage (2.42 KB, patch)
2016-02-16 14:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove GetPatronImage (27.56 KB, patch)
2016-02-16 14:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15635: Change method type to _type for bug 15446 (1.05 KB, patch)
2016-02-16 14:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Add new classes (5.89 KB, patch)
2016-02-19 15:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Add new classes (5.89 KB, patch)
2016-02-19 15:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove PutPatronImage (7.64 KB, patch)
2016-02-19 15:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove RmPatronImage (2.48 KB, patch)
2016-02-19 15:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15635: Koha::Patron::Images - Remove GetPatronImage (27.62 KB, patch)
2016-02-19 15:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15635: Change method type to _type for bug 15446 (1.10 KB, patch)
2016-02-19 15:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15635 [QA Followup] - Fix number of unit tests (725 bytes, patch)
2016-02-19 15:26 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-01-21 14:29:46 UTC
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.
Comment 1 Jonathan Druart 2016-01-21 14:53:40 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-01-21 14:53:51 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-01-21 14:53:54 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-01-21 14:53:57 UTC Comment hidden (obsolete)
Comment 5 Mark Tompsett 2016-01-25 17:50:41 UTC
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 6 Mark Tompsett 2016-01-25 17:57:47 UTC
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. :)
Comment 7 Jonathan Druart 2016-01-27 10:18:10 UTC
(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.
Comment 8 Jonathan Druart 2016-01-27 10:19:40 UTC
(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 :)
Comment 9 Josef Moravec 2016-02-16 10:40:25 UTC
Could you please rebase?
Comment 10 Jonathan Druart 2016-02-16 11:15:32 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2016-02-16 11:15:36 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2016-02-16 11:15:39 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2016-02-16 11:15:43 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2016-02-16 11:15:46 UTC Comment hidden (obsolete)
Comment 15 Josef Moravec 2016-02-16 13:27:09 UTC
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...
Comment 16 Jonathan Druart 2016-02-16 13:50:06 UTC
(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.
Comment 17 Josef Moravec 2016-02-16 14:26:35 UTC Comment hidden (obsolete)
Comment 18 Josef Moravec 2016-02-16 14:26:39 UTC Comment hidden (obsolete)
Comment 19 Josef Moravec 2016-02-16 14:26:45 UTC Comment hidden (obsolete)
Comment 20 Josef Moravec 2016-02-16 14:26:53 UTC Comment hidden (obsolete)
Comment 21 Josef Moravec 2016-02-16 14:27:01 UTC Comment hidden (obsolete)
Comment 22 Josef Moravec 2016-02-16 14:29:27 UTC
(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.
Comment 23 Kyle M Hall 2016-02-19 15:25:59 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2016-02-19 15:26:28 UTC
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>
Comment 25 Kyle M Hall 2016-02-19 15:26:35 UTC
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>
Comment 26 Kyle M Hall 2016-02-19 15:26:38 UTC
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>
Comment 27 Kyle M Hall 2016-02-19 15:26:40 UTC
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>
Comment 28 Kyle M Hall 2016-02-19 15:26:43 UTC
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>
Comment 29 Kyle M Hall 2016-02-19 15:26:45 UTC
Created attachment 48249 [details] [review]
Bug 15635 [QA Followup] - Fix number of unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Brendan Gallagher 2016-03-02 22:40:11 UTC
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
Comment 31 Jonathan Druart 2016-03-03 08:26:18 UTC
Bug 15548 has been rebased, they apply now cleanly.
Comment 32 Kyle M Hall 2016-03-04 12:56:58 UTC
Pushed to Master, should be in the May 2106 Release.  Thanks Jonathan!