Bug 12159 - Duplicate borrower_add_additional_fields function
Summary: Duplicate borrower_add_additional_fields function
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Josef Moravec
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 18789
Blocks: 16846 20443 20648
  Show dependency treegraph
 
Reported: 2014-04-29 10:33 UTC by paxed
Modified: 2019-07-22 16:03 UTC (History)
9 users (show)

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


Attachments
Bug 12159: Move borrower_add_additional_fields to Koha::Patron (4.52 KB, patch)
2018-03-15 02:09 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 12159: Fix getting extended patron attributes for circ-menu (28.79 KB, patch)
2018-03-16 11:09 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 12159: Add tests (4.97 KB, patch)
2018-03-16 11:10 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 12159: Fix getting extended patron attributes for circ-menu (28.81 KB, patch)
2018-04-24 07:57 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 12159: Add tests (4.96 KB, patch)
2018-04-24 07:57 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 12159: Fix getting extended patron attributes for circ-menu (28.96 KB, patch)
2018-04-24 08:52 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12159: Add tests (5.11 KB, patch)
2018-04-24 08:52 UTC, Séverine Queune
Details | Diff | Splinter Review
Bug 12159: Fix getting extended patron attributes for circ-menu (29.08 KB, patch)
2018-06-05 11:42 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 12159: Add tests (5.11 KB, patch)
2018-06-05 11:42 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 12159: (QA follow-up) Silencing POD warn from qa tools (754 bytes, patch)
2018-06-05 14:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12159: Fix getting extended patron attributes for circ-menu (29.33 KB, patch)
2018-07-13 13:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12159: Add tests (5.12 KB, patch)
2018-07-13 13:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12159: (QA follow-up) Silencing POD warn from qa tools (721 bytes, patch)
2018-07-13 13:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12159: Fix getting extended patron attributes for circ-menu (29.84 KB, patch)
2018-07-17 14:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12159: Add tests (5.12 KB, patch)
2018-07-17 14:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12159: (QA follow-up) Silencing POD warn from qa tools (721 bytes, patch)
2018-07-17 14:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12159: Fix getting extended patron attributes for circ-menu (26.43 KB, patch)
2019-03-06 13:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 12159: Add tests (5.18 KB, patch)
2019-03-06 13:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 12159: (QA follow-up) Silencing POD warn from qa tools (723 bytes, patch)
2019-03-06 13:44 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 12159: Fix getting extended patron attributes for circ-menu (25.77 KB, patch)
2019-03-22 11:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12159: Add tests (5.28 KB, patch)
2019-03-22 11:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12159: (QA follow-up) Rename relation in Patron::Attributes (1.07 KB, patch)
2019-03-22 11:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12159: (QA follow-up) Remove double use module (717 bytes, patch)
2019-03-22 11:06 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 paxed 2014-04-29 10:33:56 UTC
The function borrower_add_additional_fields is defined twice, and used twice, in members/pay.pl and members/paycollect.pl

Perhaps fold the functionality into GetMember()? (Also take a look at tools/viewlog.pl, it does partially the same thing as borrower_add_additional_fields)
Comment 1 Josef Moravec 2018-02-16 07:04:48 UTC
Should be removed and functionality added to Koha::Patron
Comment 2 M. Tompsett 2018-03-15 02:09:42 UTC
Created attachment 72902 [details] [review]
Bug 12159: Move borrower_add_additional_fields to Koha::Patron

The same function was effectively coded twice.
It was removed from members/pay.pl and members/paycollect.pl
and placed in Koha/Patron.pm instead.
Comment 3 M. Tompsett 2018-03-15 02:12:35 UTC
Haven't written the tests yet, so feel free to take it, write them, and ask for sign off. :)
Comment 4 Josef Moravec 2018-03-16 11:09:56 UTC
Created attachment 73018 [details] [review]
Bug 12159: Fix getting extended patron attributes for circ-menu

Changes:
- Replace getting preference ExtendedPatronAttributes by Koha.Preference
in templates
- Add Koha::Patron->attributes for getting patrons extended attributes
- Use this method in circ-menu.inc
- Remove getting attributes from members perl scripts

Test plan:
0) Apply the patch
1) Add some patron attributes type - with free text, authorised value,
    limited by libraries...
2) Add some values to this attributes for some patrons
3) Go through as many patron pages as you can and confirm that
attributes are shown at side panel when they shoul and are not shown
when they should not be shown
Comment 5 Josef Moravec 2018-03-16 11:10:01 UTC
Created attachment 73019 [details] [review]
Bug 12159: Add tests

Test plan:
prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t
Comment 6 Josef Moravec 2018-03-16 11:11:22 UTC
The problem is a bit complicated, this sub was wrong at all (sorry for mystification you Mark).

I added new method Koha::Patron->attributes and use it instead
Comment 7 Séverine Queune 2018-04-24 07:33:57 UTC
I couldn't apply the patch on master branch (17.12.00.037) :

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 12159: Fix getting extended patron attributes for circ-menu
Using index info to reconstruct a base tree...
M       Koha/Patron.pm
M       circ/circulation.pl
M       koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc
M       koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
M       koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
M       koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt
M       members/boraccount.pl
M       members/member-flags.pl
M       members/memberentry.pl
M       members/moremember.pl
M       members/paycollect.pl
Falling back to patching base and 3-way merge...
Auto-merging members/paycollect.pl
Auto-merging members/moremember.pl
Auto-merging members/memberentry.pl
Auto-merging members/member-flags.pl
Auto-merging members/boraccount.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc
Auto-merging circ/circulation.pl
Auto-merging Koha/Patron.pm
CONFLICT (content): Merge conflict in Koha/Patron.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 12159: Fix getting extended patron attributes for circ-menu
The copy of the patch that failed is found in:
   /var/repositories/koha/.git/rebase-apply/patch
Comment 8 Josef Moravec 2018-04-24 07:57:54 UTC
Created attachment 74781 [details] [review]
Bug 12159: Fix getting extended patron attributes for circ-menu

Changes:
- Replace getting preference ExtendedPatronAttributes by Koha.Preference
in templates
- Add Koha::Patron->attributes for getting patrons extended attributes
- Use this method in circ-menu.inc
- Remove getting attributes from members perl scripts

Test plan:
0) Apply the patch
1) Add some patron attributes type - with free text, authorised value,
    limited by libraries...
2) Add some values to this attributes for some patrons
3) Go through as many patron pages as you can and confirm that
attributes are shown at side panel when they shoul and are not shown
when they should not be shown
Comment 9 Josef Moravec 2018-04-24 07:57:58 UTC
Created attachment 74782 [details] [review]
Bug 12159: Add tests

Test plan:
prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t
Comment 10 Séverine Queune 2018-04-24 08:52:48 UTC
Created attachment 74791 [details] [review]
Bug 12159: Fix getting extended patron attributes for circ-menu

Changes:
- Replace getting preference ExtendedPatronAttributes by Koha.Preference
in templates
- Add Koha::Patron->attributes for getting patrons extended attributes
- Use this method in circ-menu.inc
- Remove getting attributes from members perl scripts

Test plan:
0) Apply the patch
1) Add some patron attributes type - with free text, authorised value,
    limited by libraries...
2) Add some values to this attributes for some patrons
3) Go through as many patron pages as you can and confirm that
attributes are shown at side panel when they shoul and are not shown
when they should not be shown

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 11 Séverine Queune 2018-04-24 08:52:51 UTC
Created attachment 74792 [details] [review]
Bug 12159: Add tests

Test plan:
prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 12 Séverine Queune 2018-04-24 08:59:12 UTC
Attributes checked "Display in check-out" are now displayed on side panel of all tabs in patron account (that's useful).

Maybe the description of this option on the "Attribute menu" should be modifiy now.
"Display in check-out: Check to show this attribute in patron check-out."
--> "Display in patron's brief information" ?
Comment 13 Josef Moravec 2018-04-24 10:11:51 UTC
(In reply to Séverine Queune from comment #12)
> Attributes checked "Display in check-out" are now displayed on side panel of
> all tabs in patron account (that's useful).
> 
> Maybe the description of this option on the "Attribute menu" should be
> modifiy now.
> "Display in check-out: Check to show this attribute in patron check-out."
> --> "Display in patron's brief information" ?

Definitely makes sense to me, would you mind to fill a bug for this?
Comment 14 Séverine Queune 2018-04-24 10:41:44 UTC
(In reply to Josef Moravec from comment #13)
> (In reply to Séverine Queune from comment #12)
> > Attributes checked "Display in check-out" are now displayed on side panel of
> > all tabs in patron account (that's useful).
> > 
> > Maybe the description of this option on the "Attribute menu" should be
> > modifiy now.
> > "Display in check-out: Check to show this attribute in patron check-out."
> > --> "Display in patron's brief information" ?
> 
> Definitely makes sense to me, would you mind to fill a bug for this?

No problem ! Done with Bug 20648.
Comment 15 Josef Moravec 2018-06-05 11:42:15 UTC
Created attachment 75811 [details] [review]
Bug 12159: Fix getting extended patron attributes for circ-menu

Changes:
- Replace getting preference ExtendedPatronAttributes by Koha.Preference
in templates
- Add Koha::Patron->attributes for getting patrons extended attributes
- Use this method in circ-menu.inc
- Remove getting attributes from members perl scripts

Test plan:
0) Apply the patch
1) Add some patron attributes type - with free text, authorised value,
    limited by libraries...
2) Add some values to this attributes for some patrons
3) Go through as many patron pages as you can and confirm that
attributes are shown at side panel when they shoul and are not shown
when they should not be shown

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 16 Josef Moravec 2018-06-05 11:42:27 UTC
Created attachment 75812 [details] [review]
Bug 12159: Add tests

Test plan:
prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 17 Marcel de Rooy 2018-06-05 14:49:20 UTC
Created attachment 75822 [details] [review]
Bug 12159: (QA follow-up) Silencing POD warn from qa tools

*** ERROR: Spurious text after =cut at line 954 in file Koha/Patron.pm
Comment 18 Marcel de Rooy 2018-06-05 14:50:57 UTC
FAIL Koha/Patron/Attribute.pm
forbidden pattern: Warning: The 'sub type' may be wrong is declared in a Koha::* package (see bug 15446) (line 127)

Josef:
This is a more nasty one.
Could you consider renaming this method?
The Koha objects also use _type and it might be confusing.
What do you think?
Comment 19 Jonathan Druart 2018-06-05 14:59:55 UTC
(In reply to Marcel de Rooy from comment #18)
> FAIL Koha/Patron/Attribute.pm
> forbidden pattern: Warning: The 'sub type' may be wrong is declared in a
> Koha::* package (see bug 15446) (line 127)
> 
> Josef:
> This is a more nasty one.
> Could you consider renaming this method?
> The Koha objects also use _type and it might be confusing.
> What do you think?

It already exists.
Comment 20 Jonathan Druart 2018-06-05 15:10:19 UTC
IMO we should push this one along with bug 20443 (which does not have patches yet). Otherwise we are going to have 2 different ways to fetch patron's attributes.
Before we only had GetBorrowerAttributes.
Comment 21 Martin Renvoize 2018-06-06 06:18:44 UTC
I agree with Jonathan here
Comment 22 Josef Moravec 2018-06-06 07:54:25 UTC
(In reply to Marcel de Rooy from comment #18)
> FAIL Koha/Patron/Attribute.pm
> forbidden pattern: Warning: The 'sub type' may be wrong is declared in a
> Koha::* package (see bug 15446) (line 127)
> 
> Josef:
> This is a more nasty one.
> Could you consider renaming this method?
> The Koha objects also use _type and it might be confusing.
> What do you think?

We could rename it of course, but isn't that the reason why the type in Koha::Object was renamed to _type? To avoid conflicts with type accessor if needed?
Comment 23 Josef Moravec 2018-06-06 07:55:08 UTC
(In reply to Jonathan Druart from comment #20)
> IMO we should push this one along with bug 20443 (which does not have
> patches yet). Otherwise we are going to have 2 different ways to fetch
> patron's attributes.
> Before we only had GetBorrowerAttributes.

Does make sense. I'll try to look at it.
Comment 24 Katrin Fischer 2018-07-02 06:27:30 UTC
How can we make progress here? Waiting for patches for bug 20443?
Comment 25 Jonathan Druart 2018-07-13 13:08:29 UTC
Created attachment 76945 [details] [review]
Bug 12159: Fix getting extended patron attributes for circ-menu

Changes:
- Replace getting preference ExtendedPatronAttributes by Koha.Preference
in templates
- Add Koha::Patron->attributes for getting patrons extended attributes
- Use this method in circ-menu.inc
- Remove getting attributes from members perl scripts

Test plan:
0) Apply the patch
1) Add some patron attributes type - with free text, authorised value,
    limited by libraries...
2) Add some values to this attributes for some patrons
3) Go through as many patron pages as you can and confirm that
attributes are shown at side panel when they shoul and are not shown
when they should not be shown

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 26 Jonathan Druart 2018-07-13 13:08:35 UTC
Created attachment 76946 [details] [review]
Bug 12159: Add tests

Test plan:
prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 27 Jonathan Druart 2018-07-13 13:08:41 UTC
Created attachment 76947 [details] [review]
Bug 12159: (QA follow-up) Silencing POD warn from qa tools

*** ERROR: Spurious text after =cut at line 954 in file Koha/Patron.pm
Comment 28 Jonathan Druart 2018-07-13 13:10:05 UTC
Patches rebased.
Comment 29 Katrin Fischer 2018-07-14 12:18:56 UTC
Why BLOCKED?
Comment 30 Martin Renvoize 2018-07-16 05:53:09 UTC
I believe Tomas is working on bug 20443.. which I think is the blocker here.
Comment 31 Jonathan Druart 2018-07-16 13:12:46 UTC
(In reply to Katrin Fischer from comment #29)
> Why BLOCKED?

See comment 20.

(In reply to Martin Renvoize from comment #30)
> I believe Tomas is working on bug 20443.. which I think is the blocker here.

See assignee field, I am working on it. Please do not change anything here.
Comment 32 Jonathan Druart 2018-07-17 14:36:17 UTC
Created attachment 77050 [details] [review]
Bug 12159: Fix getting extended patron attributes for circ-menu

Changes:
- Replace getting preference ExtendedPatronAttributes by Koha.Preference
in templates
- Add Koha::Patron->attributes for getting patrons extended attributes
- Use this method in circ-menu.inc
- Remove getting attributes from members perl scripts

Test plan:
0) Apply the patch
1) Add some patron attributes type - with free text, authorised value,
    limited by libraries...
2) Add some values to this attributes for some patrons
3) Go through as many patron pages as you can and confirm that
attributes are shown at side panel when they shoul and are not shown
when they should not be shown

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 33 Jonathan Druart 2018-07-17 14:36:22 UTC
Created attachment 77051 [details] [review]
Bug 12159: Add tests

Test plan:
prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 34 Jonathan Druart 2018-07-17 14:36:26 UTC
Created attachment 77052 [details] [review]
Bug 12159: (QA follow-up) Silencing POD warn from qa tools

*** ERROR: Spurious text after =cut at line 954 in file Koha/Patron.pm
Comment 35 Jonathan Druart 2018-07-17 14:37:07 UTC
Conflicts with bug 20805 fixed.
Comment 36 Josef Moravec 2019-03-06 13:44:21 UTC
Created attachment 86166 [details] [review]
Bug 12159: Fix getting extended patron attributes for circ-menu

Changes:
- Replace getting preference ExtendedPatronAttributes by Koha.Preference
in templates
- Add Koha::Patron->attributes for getting patrons extended attributes
- Use this method in circ-menu.inc
- Remove getting attributes from members perl scripts

Test plan:
0) Apply the patch
1) Add some patron attributes type - with free text, authorised value,
    limited by libraries...
2) Add some values to this attributes for some patrons
3) Go through as many patron pages as you can and confirm that
attributes are shown at side panel when they shoul and are not shown
when they should not be shown

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 37 Josef Moravec 2019-03-06 13:44:25 UTC
Created attachment 86167 [details] [review]
Bug 12159: Add tests

Test plan:
prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 38 Josef Moravec 2019-03-06 13:44:29 UTC
Created attachment 86168 [details] [review]
Bug 12159: (QA follow-up) Silencing POD warn from qa tools

*** ERROR: Spurious text after =cut at line 954 in file Koha/Patron.pm
Comment 39 Josef Moravec 2019-03-06 16:39:13 UTC
Rebased
Comment 40 Marcel de Rooy 2019-03-22 10:06:13 UTC
Comment on attachment 86168 [details] [review]
Bug 12159: (QA follow-up) Silencing POD warn from qa tools

No longer needed
Comment 41 Marcel de Rooy 2019-03-22 11:06:07 UTC
QA Comment:
Saw this warning once:
Argument "" isn't numeric in numeric gt (>) at /usr/share/koha/koha-tmpl/intranet-tmpl/prog/en/includes/members-toolbar.inc line 84.

False warning from qa tools:
Koha/Patron/Attribute.pm
critic/forbidden patterns: forbidden pattern: Warning: The 'sub type' may be wrong is declared in a Koha::* package (see bug 15446) (line 127)

A dumb question:
+ return Koha::Patron::Attributes->search({
+        borrowernumber => $self->borrowernumber,
+        branchcode     => $self->branchcode,
+    });
=> Is the branchcode needed?

Koha/Patron/Attribute.pm L114
return $av->next->lib if $av->count
Amended: adding a semicolon for future ease.

Koha/Schema/Result/BorrowerAttribute.pm 
=> Please do not add DBIx schema files in a regular patch. I removed it now. Btw it seems no longer necessary if we rename the relation in the Koha object. See follow-up.
Comment 42 Marcel de Rooy 2019-03-22 11:06:33 UTC
Created attachment 86882 [details] [review]
Bug 12159: Fix getting extended patron attributes for circ-menu

Changes:
- Replace getting preference ExtendedPatronAttributes by Koha.Preference
in templates
- Add Koha::Patron->attributes for getting patrons extended attributes
- Use this method in circ-menu.inc
- Remove getting attributes from members perl scripts

Test plan:
0) Apply the patch
1) Add some patron attributes type - with free text, authorised value,
    limited by libraries...
2) Add some values to this attributes for some patrons
3) Go through as many patron pages as you can and confirm that
attributes are shown at side panel when they shoul and are not shown
when they should not be shown

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Removed Koha/Schema/Result/BorrowerAttribute.pm
[EDIT] Added missing semicolon on L114 in Koha/Patron/Attribute.pm
Comment 43 Marcel de Rooy 2019-03-22 11:06:38 UTC
Created attachment 86883 [details] [review]
Bug 12159: Add tests

Test plan:
prove t/db_dependent/Koha/Patron/Attributes.t t/db_dependent/Koha/Patrons.t

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2019-03-22 11:06:42 UTC
Created attachment 86884 [details] [review]
Bug 12159: (QA follow-up) Rename relation in Patron::Attributes

It seems that we do not need the extra belongs_to in the schema result
files if we rename the relation to code.

Test plan:
Run t/db_dependent/Koha/Patrons.t
Run t/db_dependent/Koha/Patron/Attributes.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2019-03-22 11:06:47 UTC
Created attachment 86885 [details] [review]
Bug 12159: (QA follow-up) Remove double use module

Speaks for itself.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Nick Clemens 2019-03-28 12:30:47 UTC
Discussion is unclear - this now blocks 20443 - do we want these pushed together, or can this one go now and then be followed by 20443 when it is ready?
Comment 47 Nick Clemens 2019-03-28 13:28:18 UTC
Awesome work all!

Pushed to master for 19.05
Comment 48 Martin Renvoize 2019-04-01 11:14:09 UTC
Architectural enhancement will not be backported to 18.11.x series.