Bug 17578 - Replace GetMemberDetails with GetMember
Summary: Replace GetMemberDetails with GetMember
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 7058 (view as bug list)
Depends on: 17555 17579
Blocks: 16846 7058 17588 21832
  Show dependency treegraph
 
Reported: 2016-11-08 10:03 UTC by Jonathan Druart
Modified: 2018-11-14 10:19 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17578: GetMemberDetails - Add Koha::Patron::Category->effective_BlockExpiredPatronOpacActions (2.71 KB, patch)
2016-11-08 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove BlockExpiredPatronOpacActions (7.61 KB, patch)
2016-11-08 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove authflags - 2 (1.24 KB, patch)
2016-11-08 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove authflags - 1 (4.02 KB, patch)
2016-11-08 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove amountoutstanding (10.00 KB, patch)
2016-11-08 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove flags (6.83 KB, patch)
2016-11-08 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove is_expired (3.09 KB, patch)
2016-11-08 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove reservefee (1.88 KB, patch)
2016-11-08 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove enrolmentperiod (1.22 KB, patch)
2016-11-08 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove GetMemberDetails (22.91 KB, patch)
2016-11-08 11:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove flags (6.81 KB, patch)
2016-11-08 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove is_expired (3.09 KB, patch)
2016-11-08 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove reservefee (1.88 KB, patch)
2016-11-08 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove enrolmentperiod (1.22 KB, patch)
2016-11-08 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove GetMemberDetails (22.89 KB, patch)
2016-11-08 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove BlockExpiredPatronOpacActions (3.25 KB, patch)
2016-11-28 15:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove authflags - 1 (4.03 KB, patch)
2016-11-28 15:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove authflags - 2 (1.24 KB, patch)
2016-11-28 15:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove amountoutstanding (10.00 KB, patch)
2016-11-28 15:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove flags (6.81 KB, patch)
2016-11-28 15:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove is_expired (3.09 KB, patch)
2016-11-28 15:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove reservefee (1.88 KB, patch)
2016-11-28 15:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove enrolmentperiod (1.22 KB, patch)
2016-11-28 15:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove GetMemberDetails (22.39 KB, patch)
2016-11-28 15:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
But 17578: (followup) amountoutstanding (975 bytes, patch)
2016-11-29 18:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove BlockExpiredPatronOpacActions (3.32 KB, patch)
2016-11-30 07:17 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove authflags - 1 (4.10 KB, patch)
2016-11-30 07:17 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove authflags - 2 (1.31 KB, patch)
2016-11-30 07:17 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove amountoutstanding (10.07 KB, patch)
2016-11-30 07:17 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove flags (6.88 KB, patch)
2016-11-30 07:17 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove is_expired (3.16 KB, patch)
2016-11-30 07:17 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove reservefee (1.94 KB, patch)
2016-11-30 07:17 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove enrolmentperiod (1.29 KB, patch)
2016-11-30 07:18 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove GetMemberDetails (22.46 KB, patch)
2016-11-30 07:18 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] But 17578: (followup) amountoutstanding (1.02 KB, patch)
2016-11-30 07:18 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove GetMemberDetails (22.44 KB, patch)
2016-12-05 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
But 17578: (followup) amountoutstanding (1.01 KB, patch)
2016-12-05 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove BlockExpiredPatronOpacActions (3.36 KB, patch)
2016-12-05 16:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove authflags - 1 (4.14 KB, patch)
2016-12-05 16:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove authflags - 2 (1.35 KB, patch)
2016-12-05 16:42 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove amountoutstanding (10.11 KB, patch)
2016-12-05 16:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove flags (6.92 KB, patch)
2016-12-05 16:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove is_expired (3.20 KB, patch)
2016-12-05 16:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove reservefee (1.99 KB, patch)
2016-12-05 16:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove enrolmentperiod (1.33 KB, patch)
2016-12-05 16:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17578: GetMemberDetails - Remove GetMemberDetails (22.50 KB, patch)
2016-12-05 16:43 UTC, Nick Clemens
Details | Diff | Splinter Review
But 17578: (followup) amountoutstanding (1.06 KB, patch)
2016-12-05 16:44 UTC, Nick Clemens
Details | Diff | Splinter Review
QA Follow - Adjust number of tests (729 bytes, patch)
2016-12-05 16:44 UTC, Nick Clemens
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-11-08 10:03:33 UTC
In order to unify the different subroutine of C4::Members and move them to Koha::Patrons, one of the steps is to remove GetMemberDetails.
Here we will replace all occurrences of GetMemberDetails with GetMember.
Additional informations will be retrieved when needed.
Comment 1 Jonathan Druart 2016-11-08 11:16:33 UTC
Created attachment 57299 [details] [review]
Bug 17578: GetMemberDetails - Add Koha::Patron::Category->effective_BlockExpiredPatronOpacActions

This patch simply adds a new method to retrieve the correct value of
BlockExpiredPatronOpacActions.
Comment 2 Jonathan Druart 2016-11-08 11:16:35 UTC
Created attachment 57300 [details] [review]
Bug 17578: GetMemberDetails - Remove BlockExpiredPatronOpacActions

The correct way to get the value of BlockExpiredPatronOpacActions from a
patron object is to get the patron category then call the
effective_BlockExpiredPatronOpacActions:
  $patron->category->effective_BlockExpiredPatronOpacActions

So this patch applies this change and remove this value from the
GetMemberDetails subroutine.
Comment 3 Jonathan Druart 2016-11-08 11:16:38 UTC
Created attachment 57301 [details] [review]
Bug 17578: GetMemberDetails - Remove authflags - 2

This script is not used from the Koha codebase and does not seem very
useful.
We could rewrite it if needed later (ie. if someone complains I will
rewrite it).
Comment 4 Jonathan Druart 2016-11-08 11:16:41 UTC
Created attachment 57302 [details] [review]
Bug 17578: GetMemberDetails - Remove authflags - 1

GetMemberDetails create a authflags key, but this key is only used from
2 different places.
One is a very simple script, which does not seem very usefull
C4/SIP/interactive_members_dump.pl. I propose to simply remove it.
The other one is the member-flags.pl script. What is done in this one is
a bit weird since we a doing twice the same query (it was not highlighted
before this patch). We will need to fix that later.
At the moment the goal it to remove the GetMemberDetails subroutine
without introducing any regressions (and so without adding big changes)

Test plan:
Select/unselect permissions for a patron, save and edit again.
The behavior of the permission checkboxes should be ok
Comment 5 Jonathan Druart 2016-11-08 11:16:44 UTC
Created attachment 57303 [details] [review]
Bug 17578: GetMemberDetails - Remove amountoutstanding

The amountoutstanding value set by GetMemberDetails was only used in a
few places. In that case it makes sense to only retrieve it when needed.

Test plan:
1/ Add fines to a patron, on the OPAC patron info page, you should see a
"Fines" tab
2/ Add credit to a patron, you should see the credit displayed
3/ Set the pref maxoutstanding to 3
4/ Add a fine of 4 to a patron
5/ Try to place an hold for this patron
=> You should get a "too much oweing" message
Comment 6 Jonathan Druart 2016-11-08 11:16:47 UTC
Created attachment 57304 [details] [review]
Bug 17578: GetMemberDetails - Remove flags

Same as authflags, a flags key is set containing all the patron flags.
It is only used in a few places and it's better to call
C4::Members::patronflags when we need it.

Test plan:
Look at the diff and confirm that the change make sense
Use git grep to confirm we do not use the flags somewhere else.
Comment 7 Jonathan Druart 2016-11-08 11:16:50 UTC
Created attachment 57305 [details] [review]
Bug 17578: GetMemberDetails - Remove is_expired

The is_expired value is used in 2 places, let's use
Koha::Patron->is_expired instead.

Test plan:
Depending on the different value of BlockExpiredPatronOpacActions for
the patron category, a patron must be blocked if he has expired.
Confirm that behavior from opac-renew and opac-reserve scripts
Comment 8 Jonathan Druart 2016-11-08 11:16:52 UTC
Created attachment 57306 [details] [review]
Bug 17578: GetMemberDetails - Remove reservefee

Same as other patches, reservefee is only used in opac-reserve.pl

Test plan;
Set reserve fee for a patron category
Place a hold at the OPAC with one of these patrons.
You must get a message about the reserve fee.
Comment 9 Jonathan Druart 2016-11-08 11:16:55 UTC
Created attachment 57307 [details] [review]
Bug 17578: GetMemberDetails - Remove enrolmentperiod

This value is not used anywhere

Test plan:
  git grep enrolmentperiod| grep -v installer| grep -v translator|vim -

should show you that I am right
Comment 10 Jonathan Druart 2016-11-08 11:16:59 UTC
Created attachment 57308 [details] [review]
Bug 17578: GetMemberDetails - Remove GetMemberDetails

All the values different from the ones GetMember returned has been
managed outside of GetMemberDetails.
It looks safe to replace all the occurrences of GetMemberDetails with
GetMember.
Comment 11 Josef Moravec 2016-11-08 13:14:56 UTC
When I go to circulation -> checking out, or to permission settings of patron I get:

Can't use string ("19") as a HASH ref while "strict refs" in use at /home/vagrant/kohaclone/C4/Members.pm line 194

19 is borrowernumber of my patron
Comment 12 Jonathan Druart 2016-11-08 13:59:16 UTC
Created attachment 57322 [details] [review]
Bug 17578: GetMemberDetails - Remove flags

Same as authflags, a flags key is set containing all the patron flags.
It is only used in a few places and it's better to call
C4::Members::patronflags when we need it.

Test plan:
Look at the diff and confirm that the change make sense
Use git grep to confirm we do not use the flags somewhere else.
Comment 13 Jonathan Druart 2016-11-08 13:59:20 UTC
Created attachment 57323 [details] [review]
Bug 17578: GetMemberDetails - Remove is_expired

The is_expired value is used in 2 places, let's use
Koha::Patron->is_expired instead.

Test plan:
Depending on the different value of BlockExpiredPatronOpacActions for
the patron category, a patron must be blocked if he has expired.
Confirm that behavior from opac-renew and opac-reserve scripts
Comment 14 Jonathan Druart 2016-11-08 13:59:23 UTC
Created attachment 57324 [details] [review]
Bug 17578: GetMemberDetails - Remove reservefee

Same as other patches, reservefee is only used in opac-reserve.pl

Test plan;
Set reserve fee for a patron category
Place a hold at the OPAC with one of these patrons.
You must get a message about the reserve fee.
Comment 15 Jonathan Druart 2016-11-08 13:59:26 UTC
Created attachment 57325 [details] [review]
Bug 17578: GetMemberDetails - Remove enrolmentperiod

This value is not used anywhere

Test plan:
  git grep enrolmentperiod| grep -v installer| grep -v translator|vim -

should show you that I am right
Comment 16 Jonathan Druart 2016-11-08 13:59:29 UTC
Created attachment 57326 [details] [review]
Bug 17578: GetMemberDetails - Remove GetMemberDetails

All the values different from the ones GetMember returned has been
managed outside of GetMemberDetails.
It looks safe to replace all the occurrences of GetMemberDetails with
GetMember.
Comment 17 Josef Moravec 2016-11-09 09:20:01 UTC
Continue testing:

On the patron permission setting page I always see the same set of permisions... but when I tried to set some permissions, it is saved to the database

Also there are these error messages in intranet-error.log, I'm not sure if it is related:

moremember.pl: Problem = a value of activeBorrowerRelationship has been passed to param without key at /home/vagrant/kohaclone/C4/Templates.pm line 137., referer: http://localhost:8081/cgi-bin/koha/members/member-flags.pl?member=19

moremember.pl: Problem = a value of relatives_borrowernumbers has been passed to param without key at /home/vagrant/kohaclone/C4/Templates.pm line 137., referer: http://localhost:8081/cgi-bin/koha/members/member-flags.pl?member=19

When trying to place a hold in OPAC I got:

The method category is not covered by tests!

So I assume this bug should depend on bug 17555. Right? 
I think there will be conflict in t/db_dependent/Koha/Patrons.t (at least). Could you rebase on 17555 please? (If I am right, of course ;) )
Comment 18 Jonathan Druart 2016-11-09 09:38:10 UTC
Josef, yes you are right about the dependency. But I am stuck at the moment, I do not manage to maintain a dependency tree without conflicts.
I am marking this one as blocker, hoping other will be pushed soon.
Comment 19 Josef Moravec 2016-11-09 09:41:02 UTC
OK, thank you. I'll try to get back when dependecies will be pushed.
Comment 20 Jonathan Druart 2016-11-10 08:43:49 UTC
(In reply to Jonathan Druart from comment #1)
> Created attachment 57299 [details] [review] [review]
> Bug 17578: GetMemberDetails - Add
> Koha::Patron::Category->effective_BlockExpiredPatronOpacActions
> 
> This patch simply adds a new method to retrieve the correct value of
> BlockExpiredPatronOpacActions.

This patch has been moved to its own bug, see bug 17604.
Comment 21 Josef Moravec 2016-11-28 12:15:50 UTC
Dependecies were pushed, could you please look at other issues from comment 17?
Comment 22 Jonathan Druart 2016-11-28 15:52:22 UTC
Created attachment 57793 [details] [review]
Bug 17578: GetMemberDetails - Remove BlockExpiredPatronOpacActions

The correct way to get the value of BlockExpiredPatronOpacActions from a
patron object is to get the patron category then call the
effective_BlockExpiredPatronOpacActions:
  $patron->category->effective_BlockExpiredPatronOpacActions

So this patch applies this change and remove this value from the
GetMemberDetails subroutine.
Comment 23 Jonathan Druart 2016-11-28 15:52:26 UTC
Created attachment 57794 [details] [review]
Bug 17578: GetMemberDetails - Remove authflags - 1

GetMemberDetails create a authflags key, but this key is only used from
2 different places.
One is a very simple script, which does not seem very usefull
C4/SIP/interactive_members_dump.pl. I propose to simply remove it.
The other one is the member-flags.pl script. What is done in this one is
a bit weird since we a doing twice the same query (it was not highlighted
before this patch). We will need to fix that later.
At the moment the goal it to remove the GetMemberDetails subroutine
without introducing any regressions (and so without adding big changes)

Test plan:
Select/unselect permissions for a patron, save and edit again.
The behavior of the permission checkboxes should be ok
Comment 24 Jonathan Druart 2016-11-28 15:52:30 UTC
Created attachment 57795 [details] [review]
Bug 17578: GetMemberDetails - Remove authflags - 2

This script is not used from the Koha codebase and does not seem very
useful.
We could rewrite it if needed later (ie. if someone complains I will
rewrite it).
Comment 25 Jonathan Druart 2016-11-28 15:52:33 UTC
Created attachment 57796 [details] [review]
Bug 17578: GetMemberDetails - Remove amountoutstanding

The amountoutstanding value set by GetMemberDetails was only used in a
few places. In that case it makes sense to only retrieve it when needed.

Test plan:
1/ Add fines to a patron, on the OPAC patron info page, you should see a
"Fines" tab
2/ Add credit to a patron, you should see the credit displayed
3/ Set the pref maxoutstanding to 3
4/ Add a fine of 4 to a patron
5/ Try to place an hold for this patron
=> You should get a "too much oweing" message
Comment 26 Jonathan Druart 2016-11-28 15:52:37 UTC
Created attachment 57797 [details] [review]
Bug 17578: GetMemberDetails - Remove flags

Same as authflags, a flags key is set containing all the patron flags.
It is only used in a few places and it's better to call
C4::Members::patronflags when we need it.

Test plan:
Look at the diff and confirm that the change make sense
Use git grep to confirm we do not use the flags somewhere else.
Comment 27 Jonathan Druart 2016-11-28 15:52:41 UTC
Created attachment 57798 [details] [review]
Bug 17578: GetMemberDetails - Remove is_expired

The is_expired value is used in 2 places, let's use
Koha::Patron->is_expired instead.

Test plan:
Depending on the different value of BlockExpiredPatronOpacActions for
the patron category, a patron must be blocked if he has expired.
Confirm that behavior from opac-renew and opac-reserve scripts
Comment 28 Jonathan Druart 2016-11-28 15:52:45 UTC
Created attachment 57799 [details] [review]
Bug 17578: GetMemberDetails - Remove reservefee

Same as other patches, reservefee is only used in opac-reserve.pl

Test plan;
Set reserve fee for a patron category
Place a hold at the OPAC with one of these patrons.
You must get a message about the reserve fee.
Comment 29 Jonathan Druart 2016-11-28 15:52:48 UTC
Created attachment 57800 [details] [review]
Bug 17578: GetMemberDetails - Remove enrolmentperiod

This value is not used anywhere

Test plan:
  git grep enrolmentperiod| grep -v installer| grep -v translator|vim -

should show you that I am right
Comment 30 Jonathan Druart 2016-11-28 15:52:51 UTC
Created attachment 57801 [details] [review]
Bug 17578: GetMemberDetails - Remove GetMemberDetails

All the values different from the ones GetMember returned has been
managed outside of GetMemberDetails.
It looks safe to replace all the occurrences of GetMemberDetails with
GetMember.
Comment 31 Josef Moravec 2016-11-29 10:24:07 UTC
(In reply to Jonathan Druart from comment #25)
> Created attachment 57796 [details] [review] [review]
> Bug 17578: GetMemberDetails - Remove amountoutstanding
> 
> The amountoutstanding value set by GetMemberDetails was only used in a
> few places. In that case it makes sense to only retrieve it when needed.
> 
> Test plan:
> 1/ Add fines to a patron, on the OPAC patron info page, you should see a
> "Fines" tab
> 2/ Add credit to a patron, you should see the credit displayed
> 3/ Set the pref maxoutstanding to 3
> 4/ Add a fine of 4 to a patron
> 5/ Try to place an hold for this patron
> => You should get a "too much oweing" message

Does not work for me, sorry...
Comment 32 Josef Moravec 2016-11-29 10:25:04 UTC
(In reply to Jonathan Druart from comment #26)
> Created attachment 57797 [details] [review] [review]
> Bug 17578: GetMemberDetails - Remove flags
> 
> Same as authflags, a flags key is set containing all the patron flags.
> It is only used in a few places and it's better to call
> C4::Members::patronflags when we need it.
> 
> Test plan:
> Look at the diff and confirm that the change make sense
> Use git grep to confirm we do not use the flags somewhere else.

in members/members-flags.pl, line 96:

        if ( $bor->{flags} && $bor->{flags} & 2**$bit ) {
Comment 33 Josef Moravec 2016-11-29 10:33:58 UTC
(In reply to Jonathan Druart from comment #27)
> Created attachment 57798 [details] [review] [review]
> Bug 17578: GetMemberDetails - Remove is_expired
> 
> The is_expired value is used in 2 places, let's use
> Koha::Patron->is_expired instead.
> 
> Test plan:
> Depending on the different value of BlockExpiredPatronOpacActions for
> the patron category, a patron must be blocked if he has expired.
> Confirm that behavior from opac-renew and opac-reserve scripts

Does not work for me
Comment 34 Josef Moravec 2016-11-29 10:38:38 UTC
The other patches does work as expected.
Comment 35 Jonathan Druart 2016-11-29 18:20:41 UTC
Created attachment 57808 [details] [review]
But 17578: (followup) amountoutstanding
Comment 36 Jonathan Druart 2016-11-29 18:23:16 UTC
(In reply to Josef Moravec from comment #31)
> (In reply to Jonathan Druart from comment #25)
> > Created attachment 57796 [details] [review] [review] [review]
> > Bug 17578: GetMemberDetails - Remove amountoutstanding
> > 
> > The amountoutstanding value set by GetMemberDetails was only used in a
> > few places. In that case it makes sense to only retrieve it when needed.
> > 
> > Test plan:
> > 1/ Add fines to a patron, on the OPAC patron info page, you should see a
> > "Fines" tab
> > 2/ Add credit to a patron, you should see the credit displayed
> > 3/ Set the pref maxoutstanding to 3
> > 4/ Add a fine of 4 to a patron
> > 5/ Try to place an hold for this patron
> > => You should get a "too much oweing" message
> 
> Does not work for me, sorry...

Good catch, fixed. It worked in some situations (lot of account lines).

(In reply to Josef Moravec from comment #32)
> (In reply to Jonathan Druart from comment #26)
> > Created attachment 57797 [details] [review] [review] [review]
> > Bug 17578: GetMemberDetails - Remove flags
> > 
> > Same as authflags, a flags key is set containing all the patron flags.
> > It is only used in a few places and it's better to call
> > C4::Members::patronflags when we need it.
> > 
> > Test plan:
> > Look at the diff and confirm that the change make sense
> > Use git grep to confirm we do not use the flags somewhere else.
> 
> in members/members-flags.pl, line 96:
> 
>         if ( $bor->{flags} && $bor->{flags} & 2**$bit ) {

Yes actually this one is ok because we need the borrowers.flags value from the DB and not the "flags" set by C4::Members::patronflags.

(In reply to Josef Moravec from comment #33)
> (In reply to Jonathan Druart from comment #27)
> > Created attachment 57798 [details] [review] [review] [review]
> > Bug 17578: GetMemberDetails - Remove is_expired
> > 
> > The is_expired value is used in 2 places, let's use
> > Koha::Patron->is_expired instead.
> > 
> > Test plan:
> > Depending on the different value of BlockExpiredPatronOpacActions for
> > the patron category, a patron must be blocked if he has expired.
> > Confirm that behavior from opac-renew and opac-reserve scripts
> 
> Does not work for me

Could you please detail, it looks ok here.
Comment 37 Josef Moravec 2016-11-30 07:00:23 UTC
> > (In reply to Jonathan Druart from comment #27)
> > > Created attachment 57798 [details] [review] [review] [review] [review]
> > > Bug 17578: GetMemberDetails - Remove is_expired
> > > 
> > > The is_expired value is used in 2 places, let's use
> > > Koha::Patron->is_expired instead.
> > > 
> > > Test plan:
> > > Depending on the different value of BlockExpiredPatronOpacActions for
> > > the patron category, a patron must be blocked if he has expired.
> > > Confirm that behavior from opac-renew and opac-reserve scripts
> > 
> > Does not work for me
> 
> Could you please detail, it looks ok here.

Sorry, my mistake, it works ok. Continue testing
Comment 38 Josef Moravec 2016-11-30 07:17:25 UTC
Created attachment 57817 [details] [review]
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove BlockExpiredPatronOpacActions

The correct way to get the value of BlockExpiredPatronOpacActions from a
patron object is to get the patron category then call the
effective_BlockExpiredPatronOpacActions:
  $patron->category->effective_BlockExpiredPatronOpacActions

So this patch applies this change and remove this value from the
GetMemberDetails subroutine.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 39 Josef Moravec 2016-11-30 07:17:30 UTC
Created attachment 57818 [details] [review]
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove authflags - 1

GetMemberDetails create a authflags key, but this key is only used from
2 different places.
One is a very simple script, which does not seem very usefull
C4/SIP/interactive_members_dump.pl. I propose to simply remove it.
The other one is the member-flags.pl script. What is done in this one is
a bit weird since we a doing twice the same query (it was not highlighted
before this patch). We will need to fix that later.
At the moment the goal it to remove the GetMemberDetails subroutine
without introducing any regressions (and so without adding big changes)

Test plan:
Select/unselect permissions for a patron, save and edit again.
The behavior of the permission checkboxes should be ok

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 40 Josef Moravec 2016-11-30 07:17:35 UTC
Created attachment 57819 [details] [review]
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove authflags - 2

This script is not used from the Koha codebase and does not seem very
useful.
We could rewrite it if needed later (ie. if someone complains I will
rewrite it).

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 41 Josef Moravec 2016-11-30 07:17:40 UTC
Created attachment 57820 [details] [review]
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove amountoutstanding

The amountoutstanding value set by GetMemberDetails was only used in a
few places. In that case it makes sense to only retrieve it when needed.

Test plan:
1/ Add fines to a patron, on the OPAC patron info page, you should see a
"Fines" tab
2/ Add credit to a patron, you should see the credit displayed
3/ Set the pref maxoutstanding to 3
4/ Add a fine of 4 to a patron
5/ Try to place an hold for this patron
=> You should get a "too much oweing" message

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 42 Josef Moravec 2016-11-30 07:17:45 UTC
Created attachment 57821 [details] [review]
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove flags

Same as authflags, a flags key is set containing all the patron flags.
It is only used in a few places and it's better to call
C4::Members::patronflags when we need it.

Test plan:
Look at the diff and confirm that the change make sense
Use git grep to confirm we do not use the flags somewhere else.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 43 Josef Moravec 2016-11-30 07:17:50 UTC
Created attachment 57822 [details] [review]
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove is_expired

The is_expired value is used in 2 places, let's use
Koha::Patron->is_expired instead.

Test plan:
Depending on the different value of BlockExpiredPatronOpacActions for
the patron category, a patron must be blocked if he has expired.
Confirm that behavior from opac-renew and opac-reserve scripts

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 44 Josef Moravec 2016-11-30 07:17:55 UTC
Created attachment 57823 [details] [review]
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove reservefee

Same as other patches, reservefee is only used in opac-reserve.pl

Test plan;
Set reserve fee for a patron category
Place a hold at the OPAC with one of these patrons.
You must get a message about the reserve fee.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 45 Josef Moravec 2016-11-30 07:18:00 UTC
Created attachment 57824 [details] [review]
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove enrolmentperiod

This value is not used anywhere

Test plan:
  git grep enrolmentperiod| grep -v installer| grep -v translator|vim -

should show you that I am right

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 46 Josef Moravec 2016-11-30 07:18:06 UTC
Created attachment 57825 [details] [review]
[SIGNED-OFF] Bug 17578: GetMemberDetails - Remove GetMemberDetails

All the values different from the ones GetMember returned has been
managed outside of GetMemberDetails.
It looks safe to replace all the occurrences of GetMemberDetails with
GetMember.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 47 Josef Moravec 2016-11-30 07:18:20 UTC
Created attachment 57826 [details] [review]
[SIGNED-OFF] But 17578: (followup) amountoutstanding

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 48 Kyle M Hall 2016-12-02 15:14:31 UTC
t/db_dependent/Members.t ............................... 1/79
#   Failed test 'TrackLastPatronActivity - 3 patrons must be deleted'
#   at t/db_dependent/Members.t line 355.
#          got: '2'
#     expected: '3'
# Looks like you planned 79 tests but ran 77.
# Looks like you failed 1 test of 77 run.
t/db_dependent/Members.t ............................... Dubious, test returned 1 (wstat 256, 0x100)
Failed 3/79 subtests

Test Summary Report
-------------------
t/db_dependent/Members.t                             (Wstat: 256 Tests: 77 Failed: 1)
  Failed test:  64
  Non-zero exit status: 1
  Parse errors: Bad plan.  You planned 79 tests but ran 77.
Comment 49 Jonathan Druart 2016-12-02 15:19:21 UTC
(In reply to Kyle M Hall from comment #48)
> t/db_dependent/Members.t ............................... 1/79
> #   Failed test 'TrackLastPatronActivity - 3 patrons must be deleted'

Not related, see bug 17713
Comment 50 Nick Clemens 2016-12-05 15:20:54 UTC
fatal: sha1 information is lacking or useless (circ/circulation.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 17578: GetMemberDetails - Remove GetMemberDetails
Comment 51 Jonathan Druart 2016-12-05 15:35:15 UTC
Created attachment 57972 [details] [review]
Bug 17578: GetMemberDetails - Remove GetMemberDetails

All the values different from the ones GetMember returned has been
managed outside of GetMemberDetails.
It looks safe to replace all the occurrences of GetMemberDetails with
GetMember.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 52 Jonathan Druart 2016-12-05 15:35:23 UTC
Created attachment 57973 [details] [review]
But 17578: (followup) amountoutstanding

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 53 Nick Clemens 2016-12-05 16:42:29 UTC
Created attachment 57974 [details] [review]
Bug 17578: GetMemberDetails - Remove BlockExpiredPatronOpacActions

The correct way to get the value of BlockExpiredPatronOpacActions from a
patron object is to get the patron category then call the
effective_BlockExpiredPatronOpacActions:
  $patron->category->effective_BlockExpiredPatronOpacActions

So this patch applies this change and remove this value from the
GetMemberDetails subroutine.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 54 Nick Clemens 2016-12-05 16:42:44 UTC
Created attachment 57975 [details] [review]
Bug 17578: GetMemberDetails - Remove authflags - 1

GetMemberDetails create a authflags key, but this key is only used from
2 different places.
One is a very simple script, which does not seem very usefull
C4/SIP/interactive_members_dump.pl. I propose to simply remove it.
The other one is the member-flags.pl script. What is done in this one is
a bit weird since we a doing twice the same query (it was not highlighted
before this patch). We will need to fix that later.
At the moment the goal it to remove the GetMemberDetails subroutine
without introducing any regressions (and so without adding big changes)

Test plan:
Select/unselect permissions for a patron, save and edit again.
The behavior of the permission checkboxes should be ok

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 55 Nick Clemens 2016-12-05 16:42:52 UTC
Created attachment 57976 [details] [review]
Bug 17578: GetMemberDetails - Remove authflags - 2

This script is not used from the Koha codebase and does not seem very
useful.
We could rewrite it if needed later (ie. if someone complains I will
rewrite it).

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 Nick Clemens 2016-12-05 16:43:01 UTC
Created attachment 57977 [details] [review]
Bug 17578: GetMemberDetails - Remove amountoutstanding

The amountoutstanding value set by GetMemberDetails was only used in a
few places. In that case it makes sense to only retrieve it when needed.

Test plan:
1/ Add fines to a patron, on the OPAC patron info page, you should see a
"Fines" tab
2/ Add credit to a patron, you should see the credit displayed
3/ Set the pref maxoutstanding to 3
4/ Add a fine of 4 to a patron
5/ Try to place an hold for this patron
=> You should get a "too much oweing" message

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 57 Nick Clemens 2016-12-05 16:43:09 UTC
Created attachment 57978 [details] [review]
Bug 17578: GetMemberDetails - Remove flags

Same as authflags, a flags key is set containing all the patron flags.
It is only used in a few places and it's better to call
C4::Members::patronflags when we need it.

Test plan:
Look at the diff and confirm that the change make sense
Use git grep to confirm we do not use the flags somewhere else.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 58 Nick Clemens 2016-12-05 16:43:16 UTC
Created attachment 57979 [details] [review]
Bug 17578: GetMemberDetails - Remove is_expired

The is_expired value is used in 2 places, let's use
Koha::Patron->is_expired instead.

Test plan:
Depending on the different value of BlockExpiredPatronOpacActions for
the patron category, a patron must be blocked if he has expired.
Confirm that behavior from opac-renew and opac-reserve scripts

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 59 Nick Clemens 2016-12-05 16:43:24 UTC
Created attachment 57980 [details] [review]
Bug 17578: GetMemberDetails - Remove reservefee

Same as other patches, reservefee is only used in opac-reserve.pl

Test plan;
Set reserve fee for a patron category
Place a hold at the OPAC with one of these patrons.
You must get a message about the reserve fee.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 60 Nick Clemens 2016-12-05 16:43:34 UTC
Created attachment 57981 [details] [review]
Bug 17578: GetMemberDetails - Remove enrolmentperiod

This value is not used anywhere

Test plan:
  git grep enrolmentperiod| grep -v installer| grep -v translator|vim -

should show you that I am right

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 61 Nick Clemens 2016-12-05 16:43:42 UTC
Created attachment 57982 [details] [review]
Bug 17578: GetMemberDetails - Remove GetMemberDetails

All the values different from the ones GetMember returned has been
managed outside of GetMemberDetails.
It looks safe to replace all the occurrences of GetMemberDetails with
GetMember.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 62 Nick Clemens 2016-12-05 16:44:06 UTC
Created attachment 57983 [details] [review]
But 17578: (followup) amountoutstanding

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 63 Nick Clemens 2016-12-05 16:44:13 UTC
Created attachment 57984 [details] [review]
QA Follow - Adjust number of tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 64 Jonathan Druart 2016-12-13 11:26:44 UTC
*** Bug 7058 has been marked as a duplicate of this bug. ***
Comment 65 Kyle M Hall 2016-12-16 14:39:15 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 66 Katrin Fischer 2016-12-18 20:42:09 UTC
This won't get backported to 16.11.x as it is an enhancement.