Bug 20443 - Move C4::Members::Attributes to Koha namespace
Summary: Move C4::Members::Attributes to Koha namespace
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL: https://gitlab.com/joubu/Koha/commits...
Keywords:
: 20445 (view as bug list)
Depends on: 12159 20444
Blocks: 15449 24132 27957 35817 5161 8326 21083 23666 24151 25567 28293 28345
  Show dependency treegraph
 
Reported: 2018-03-19 08:35 UTC by Josef Moravec
Modified: 2024-01-16 12:41 UTC (History)
11 users (show)

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


Attachments
Bug 20443: Revert some stuffs from 12159 (7.37 KB, patch)
2020-03-20 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove opac_display and opac_editable from Patron::Attribute (5.86 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Move GetBorrowerAttributeValue to Koha::Patron->get_extended_attribute_value (13.44 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Move GetBorrowerAttributes to Koha::Patron - ILSDI (1.27 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Move GetBorrowerAttributes to Koha::Patron->extended_attributes (35.90 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Improve filtering method (5.25 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove DeleteBorrowerAttributes (2.98 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove DeleteBorrowerAttribute (11.84 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove UpdateBorrowerAttribute and SetBorrowerAttributes (35.88 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove CheckUniqueness (17.18 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Move C4::Members::AttributeTypes::GetAttributeTypes to Koha::Patron::Attribute::Types (35.91 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove num_patron (6.82 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove C4::Members::AttributeTypes (28.58 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove SearchIdMatchingAttribute - prove we are not cheating (2.45 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove SearchIdMatchingAttribute (3.93 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove extended_attributes_merge (9.65 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove extended_attributes_code_value_arrayref AND C4::Members::Attributes (22.20 KB, patch)
2020-03-20 14:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: (follow-up) Move GetBorrowerAttributes to Koha::Patron - ILSDI (2.59 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: (follow-up) Fix t/db_dependent/Koha/Patrons.t (1.45 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix Auth_with_ldap.t (1.13 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Use search_with_library_limits for attribute types (5.70 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix Patrons/Import.t (2.17 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix add/edit for checkboxes (1.80 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix POD (2.23 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix creation of new patron with attributes (1.53 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Do not explode if attribute does not have a valid av (1.32 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix patron modification approval (2.84 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Handle non existent attribute when importing patrons (3.53 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix merge_with behavior (4.82 KB, patch)
2020-03-20 14:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Revert some stuffs from 12159 (7.35 KB, patch)
2020-03-20 15:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove opac_display and opac_editable from Patron::Attribute (5.84 KB, patch)
2020-03-20 15:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Move GetBorrowerAttributeValue to Koha::Patron->get_extended_attribute_value (13.43 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Move GetBorrowerAttributes to Koha::Patron - ILSDI (1.25 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Move GetBorrowerAttributes to Koha::Patron->extended_attributes (35.88 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Improve filtering method (5.24 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove DeleteBorrowerAttributes (2.97 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove DeleteBorrowerAttribute (11.83 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove UpdateBorrowerAttribute and SetBorrowerAttributes (35.86 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove CheckUniqueness (17.16 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Move C4::Members::AttributeTypes::GetAttributeTypes to Koha::Patron::Attribute::Types (35.90 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove num_patron (6.81 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove C4::Members::AttributeTypes (28.56 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove SearchIdMatchingAttribute - prove we are not cheating (2.44 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove SearchIdMatchingAttribute (3.91 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove extended_attributes_merge (9.64 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Remove extended_attributes_code_value_arrayref AND C4::Members::Attributes (22.18 KB, patch)
2020-03-20 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: (follow-up) Move GetBorrowerAttributes to Koha::Patron - ILSDI (2.57 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: (follow-up) Fix t/db_dependent/Koha/Patrons.t (1.44 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix Auth_with_ldap.t (1.11 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Use search_with_library_limits for attribute types (5.69 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix Patrons/Import.t (2.15 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix add/edit for checkboxes (1.79 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix POD (2.21 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix creation of new patron with attributes (1.52 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Do not explode if attribute does not have a valid av (1.31 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix patron modification approval (2.83 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Handle non existent attribute when importing patrons (3.51 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Fix merge_with behavior (4.81 KB, patch)
2020-03-20 15:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: Add missing use statement (778 bytes, patch)
2020-03-23 15:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20443: (follow-up) Remove C4::Members::AttributeTypes (1.02 KB, patch)
2020-05-07 15:05 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Josef Moravec 2018-03-19 08:35:09 UTC

    
Comment 1 Jonathan Druart 2018-07-17 18:45:46 UTC
Here is a first draft of that work:
  https://gitlab.com/joubu/Koha/commits/bug_20443

It's quite a funky move:
 41 files changed, 713 insertions(+), 2017 deletions(-)

However some tests are missing for trivial methods added to Koha::Patrons.

Any remarks / suggestions?
Comment 2 Jonathan Druart 2018-08-08 13:41:29 UTC
*** Bug 20445 has been marked as a duplicate of this bug. ***
Comment 3 Martin Renvoize 2018-08-09 14:23:41 UTC
First QA comment.. and it's possibly a little mean..

You've introduced three 'filter_by_' methods: Awesome++.

However, there's an inconsistency with their chainability.. I personally feel such a method should always allow chaining and as such would request, although currently, you don't use them within a chain, that `filter_by_attribute_type` and `filter_by_attribute_value` are altered such that they act upon self rather than instantiating a new object to pass as the return.
Comment 4 Martin Renvoize 2018-08-09 14:32:52 UTC
I don't see the above as a blocker however and I'm happy to submit it as a qa followup myself once there are patches here ;)
Comment 5 Jonathan Druart 2018-08-09 14:55:23 UTC
Remote branch has been rebased against master.
Comment 6 Tomás Cohen Arazi 2018-08-09 15:13:35 UTC
I don't think we can break GetPatronInfo (ILS-DI) like this. We should keep the current behaviour and propose a behaviour change on a separate bug.
Comment 7 Tomás Cohen Arazi 2018-08-09 15:34:27 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> I don't think we can break GetPatronInfo (ILS-DI) like this. We should keep
> the current behaviour and propose a behaviour change on a separate bug.

This:

my $attrs = $patron->extended_attributes->search({ opac_display => 1 })->unblessed;

should be:

my $attrs = grep { $_->type->opac_display } @{ $patron->extended_attributes->search };
my @attrs_unblessed = map { $_->unblessed } @{ $attrs };

Highlighted by:
k$ prove t/db_dependent/ILSDI_Services.t
t/db_dependent/ILSDI_Services.t .. 1/4 DBD::mysql::st execute failed: Unknown column 'opac_display' in 'where clause'...
Comment 9 Jonathan Druart 2018-08-09 22:56:30 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> I don't think we can break GetPatronInfo (ILS-DI) like this. We should keep
> the current behaviour and propose a behaviour change on a separate bug.

I have pushed a follow-up to fix that.

NOTES:
1. I do not think we should display info from the attribute's type
2. We should test the value of the description if linked with AV (value_description). Are we sure we want "attribute" vs "value". And "value_description"? Let's stick to current behaviours for now.
Comment 10 Josef Moravec 2019-03-07 07:26:16 UTC
I just rebased this on top of current code in bug 12159, see https://gitlab.com/josef.moravec/Koha/tree/bug_20443
Comment 11 Josef Moravec 2019-03-07 07:27:33 UTC
Tests not passing (at least):
t/db_dependent/Koha/Patrons/Import.t
t/db_dependent/Auth_with_ldap.t
Comment 12 Jonathan Druart 2019-04-29 16:20:30 UTC
Hi Josef, thanks for the rebase.
I have also rebased them against master (so hard!) and added a fix for the Auth_with_ldap.t failure.
I am stuck with the other failure.

I think we need to know if we (dev team) want this patchset. If so we will need to have a task force to have it cleaned and pushed at the beginning of the next release cycle. It will be impossible to rebase this branch for months.

Who's in?
Comment 13 Tomás Cohen Arazi 2019-04-29 18:07:16 UTC
(In reply to Jonathan Druart from comment #12)
> Hi Josef, thanks for the rebase.
> I have also rebased them against master (so hard!) and added a fix for the
> Auth_with_ldap.t failure.
> I am stuck with the other failure.
> 
> I think we need to know if we (dev team) want this patchset. If so we will
> need to have a task force to have it cleaned and pushed at the beginning of
> the next release cycle. It will be impossible to rebase this branch for
> months.
> 
> Who's in?

I'm in
Comment 14 Katrin Fischer 2019-04-29 20:24:59 UTC
Do we have a choice? 

We decided on the move to the new namespace as a community... so I think we don't. I'll help where I can.
Comment 15 Jonathan Druart 2019-08-05 14:56:45 UTC
Remote branch rebased against master.
Comment 16 Martin Renvoize 2019-10-24 13:47:01 UTC
Lets schedule this for the beginning of month two in the next cycle.. that way it's early enough to push it and catch any fallout during the cycle, but late enough that we're not unduly making pain for the next stable maintainer whose trying to backport bugfixes ;)
Comment 17 Jonathan Druart 2019-11-27 15:35:31 UTC
Remote branch rebased against master.

The failing test is fixed!
Comment 18 Jonathan Druart 2020-01-14 11:44:50 UTC
Remote branch rebased against master.
Comment 19 Josef Moravec 2020-01-21 11:12:27 UTC
Hello Jonathan, thanks to rebasing this again.

Firstly, there are some trivials found by qa tools:

 FAIL	Koha/Patron/Attribute.pm
   FAIL	  pod
		*** ERROR: 
		empty =head3
		 
		 in file Koha/Patron/Attribute.pm
		
		*** ERROR: 
		empty =head3
		 
		 in file Koha/Patron/Attribute.pm

 FAIL	Koha/Patron/Attributes.pm
   FAIL	  pod
		*** ERROR: 
		empty =head3
		 
		 in file Koha/Patron/Attributes.pm
		
   FAIL	  pod coverage
		POD is missing for merge_with
		POD is missing for object_class


 FAIL	Koha/Patrons.pm
   FAIL	  spelling
		 paramter  ==> parameter


Otherwise it looks nice.



Note for QA: this is false positive:
 FAIL	Koha/Patron/Attribute.pm
   FAIL	  forbidden patterns
		forbidden pattern: Warning: The 'sub type' may be wrong is declared in a Koha::* package (see bug 15446) (line 63)
Comment 20 Jonathan Druart 2020-01-21 12:05:38 UTC
Fixed, Josef!
Comment 21 Josef Moravec 2020-01-21 13:04:10 UTC
(In reply to Jonathan Druart from comment #20)
> Fixed, Josef!

Thanks Jonathan

Signed off!
Comment 22 Nick Clemens 2020-03-06 13:55:42 UTC
I tested the following, let me know if i missed elsewhere:
  editing patron satff
  editing patron opac
  importing patron
  patron creation/update via ldap

Branch limitations do not save

Cannot load patron if attribute value not in authorized values (OK, but should have a way to identify)

Opac update of attributes fails, blank value passed to update request

Patron import with 'replace only included attributes' fails
Can't use string ("Koha::Patron::Attribute") as a HASH ref while "strict refs" in use at /kohadevbox/koha/Koha/Object.pm line 625

Attributes in ILSDI shows some extra info:
borrowernumber, authorised_value_category - is noisier than without patch


Test failures:
Test Summary Report
-------------------
t/00-testcritic.t                 (Wstat: 256 Tests: 1020 Failed: 1)
  Failed test:  732
  Non-zero exit status: 1
t/00-valid-xml.t                  (Wstat: 512 Tests: 135 Failed: 2)
  Failed tests:  1-2
  Non-zero exit status: 2
t/Logger.t                        (Wstat: 256 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Comment 23 Jonathan Druart 2020-03-09 11:21:05 UTC
(In reply to Nick Clemens from comment #22)

So basically nothing was working, thanks for having caught that Nick!

> I tested the following, let me know if i missed elsewhere:
>   editing patron satff
>   editing patron opac
>   importing patron
>   patron creation/update via ldap
> 
> Branch limitations do not save

Fixed.

> Cannot load patron if attribute value not in authorized values (OK, but
> should have a way to identify)

Fixed, I will also open a bug report.

> Opac update of attributes fails, blank value passed to update request

Fixed.

> Patron import with 'replace only included attributes' fails
> Can't use string ("Koha::Patron::Attribute") as a HASH ref while "strict
> refs" in use at /kohadevbox/koha/Koha/Object.pm line 625

Fixed.

> Attributes in ILSDI shows some extra info:
> borrowernumber, authorised_value_category - is noisier than without patch

Yes, I let a FIXME in the code (which refers to bug 14257 comment 15). I would be happy to fix it you have an idea about what's best here.

> Test failures:
> Test Summary Report
> -------------------
> t/00-testcritic.t                 (Wstat: 256 Tests: 1020 Failed: 1)
>   Failed test:  732
>   Non-zero exit status: 1
> t/00-valid-xml.t                  (Wstat: 512 Tests: 135 Failed: 2)
>   Failed tests:  1-2
>   Non-zero exit status: 2
> t/Logger.t                        (Wstat: 256 Tests: 1 Failed: 1)
>   Failed test:  1
>   Non-zero exit status: 1

They pass for me. Maybe not related to this patchset?
Comment 24 Nick Clemens 2020-03-19 19:41:37 UTC
Dies on importing wrong attribute
 To recreate:
   Create an import file like:
surname,firstname,categorycode,branchcode,patron_attributes,cardnumber
Acosta,Edna,T,FFL,"NOT_EXIST:1",23529001000463
   Attempt to import
   Kaboom

"Replace only included patron attributes" deletes all
   To test:
      Define attributes TEST1, TEST2, TEST3
      Assign values to these for a patron, Edna for example
      Import a file like:
surname,firstname,categorycode,branchcode,patron_attributes,cardnumber
Acosta,Edna,T,FFL,"TEST1:1",23529001000463
      All attributes deleted
Comment 25 Jonathan Druart 2020-03-20 10:01:54 UTC
(In reply to Nick Clemens from comment #24)
> Dies on importing wrong attribute
>  To recreate:
>    Create an import file like:
> surname,firstname,categorycode,branchcode,patron_attributes,cardnumber
> Acosta,Edna,T,FFL,"NOT_EXIST:1",23529001000463
>    Attempt to import
>    Kaboom
> 
> "Replace only included patron attributes" deletes all
>    To test:
>       Define attributes TEST1, TEST2, TEST3
>       Assign values to these for a patron, Edna for example
>       Import a file like:
> surname,firstname,categorycode,branchcode,patron_attributes,cardnumber
> Acosta,Edna,T,FFL,"TEST1:1",23529001000463
>       All attributes deleted

Thanks Nick, good catch!
I fixed these two and add more tests to cover them.
Comment 26 Nick Clemens 2020-03-20 11:12:52 UTC
Last changes fixed the issues, everything I have thought to test works well, tests pass, code looks good.

Passing QA, let's get this in
Comment 27 Jonathan Druart 2020-03-20 14:46:57 UTC
Created attachment 101122 [details] [review]
Bug 20443: Revert some stuffs from 12159

These methods have been added to Koha::Patron::Attribute but are wrong, see bug 18339

We should use ->type->$method instead

Moreover the tests exist in another subtest, we do not need them.

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Jonathan Druart 2020-03-20 14:47:01 UTC
Created attachment 101123 [details] [review]
Bug 20443: Remove opac_display and opac_editable from Patron::Attribute

Same as previously for methods that have been added by bug 17792.
It's better to be explicite and tell we are fetch the related attribute's type

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Jonathan Druart 2020-03-20 14:47:04 UTC
Created attachment 101124 [details] [review]
Bug 20443: Move GetBorrowerAttributeValue to Koha::Patron->get_extended_attribute_value

We want to retrieve a specific patron's attribute for a given patron.
We then add a new method to Koha::Patron.

This patch add a getter method ->get_extended_attribute_value
to use the DBIx::Class relation

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Jonathan Druart 2020-03-20 14:47:08 UTC
Created attachment 101125 [details] [review]
Bug 20443: Move GetBorrowerAttributes to Koha::Patron - ILSDI

FIXME See comment

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Jonathan Druart 2020-03-20 14:47:12 UTC
Created attachment 101126 [details] [review]
Bug 20443: Move GetBorrowerAttributes to Koha::Patron->extended_attributes

The GetBorrowerAttributes subroutine return the attributes for a given
patron.

Using get_extended_attributes we can acchieve it easily. The problematic
here is to restore the method's name (value vs attribute,
value_description vs description of the authorised value, as well as
display_checkout that should not be a method of Attribute, but
Attribute::Type instead)

value_description was used when the attribute types were attached to an
authorised value category. To avoid the necessary test in template and
controller there is now a $attribute->description method that will
display either the attribute's value OR the value of the authorised
value when needed. We should certainly use this one from few other
places.

Notes:
* This patch rename Koha::Patron->attributes with Koha::Patron->get_extended_attributes.
It will be renamed with Koha::Patron->extended_attributes in ones of the next
patches when it will become a setter as well.
* GetBorrowerAttributes did not care about the library limits, we still
do not
* The opac_only flag was not used outside of test, we drop it off.
* To maintain the existing behavior we add a default order-by clause to
the search method [code, attribute]
* From C4::Letters::_parseletter we always display the staff description
of the AV, There is now a FIXME to warn about it
* FIXMEs are not regressions, existing behaviors must be kept
* TODO add a new check to bug 21010 to search for inconsistencies in
patron's attributes attached to non-existent authorised values
* One test has been updated in Modifications.t, order_by is now
by default set to ['code', 'attribute']

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 32 Jonathan Druart 2020-03-20 14:47:16 UTC
Created attachment 101127 [details] [review]
Bug 20443: Improve filtering method

Koha::Patron::Attributes->search mimicks what is done in
Koha::AuthorisedValues->search.
But actually it should be more explicit when the caller use it.
For instance filter_by_branch_limitation (see discussion on bug 11983).

This will be useful for the following patches as we will need a way to
replace the $no_branch_limit flag.
When the $no_branch_limit flag is called, a simple ->search call should
be done.
When we want to limit on a specific library we can pass the branchcode
in paramter of filter_by_branch_limitation (this is not used yet).
If not passed the logged-in user library will be used by default.

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 33 Jonathan Druart 2020-03-20 14:47:19 UTC
Created attachment 101128 [details] [review]
Bug 20443: Remove DeleteBorrowerAttributes

This subroutine was only used once, easy to replace.
SetBorrowerAttributes must replace the attributes for the ones logged-in
user is allowed to edit, that's why we filter by the library limits

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 34 Jonathan Druart 2020-03-20 14:47:23 UTC
Created attachment 101129 [details] [review]
Bug 20443: Remove DeleteBorrowerAttribute

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 35 Jonathan Druart 2020-03-20 14:47:27 UTC
Created attachment 101130 [details] [review]
Bug 20443: Remove UpdateBorrowerAttribute and SetBorrowerAttributes

This patch replace Koha::Patron->get_extended_attributes with
->extended_attributes
It's now a getter a setter method.

It permits to replace UpdateBorrowerAttribute and use
create_related from DBIx::Class

Notes:
* We face the same variable names difference than in a previous patch
(value vs attribute)

Bug 20443: Remove SetBorrowerAttributes

squash  + RM get_extended_attributes

 RM get_extended_attributes

SQUASH Bug 20443: Remove UpdateBorrowerAttribute and SetBorrowerAttribute

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 36 Jonathan Druart 2020-03-20 14:47:30 UTC
Created attachment 101131 [details] [review]
Bug 20443: Remove CheckUniqueness

There is already a method in Koha::Patron::Attribute to check the
uniqueness constraint, let us it to replace CheckUniqueness

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 37 Jonathan Druart 2020-03-20 14:47:34 UTC
Created attachment 101132 [details] [review]
Bug 20443: Move C4::Members::AttributeTypes::GetAttributeTypes to Koha::Patron::Attribute::Types

We can then now start to move methods from C4::Members::AttributeTypes
as well.

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 38 Jonathan Druart 2020-03-20 14:47:38 UTC
Created attachment 101133 [details] [review]
Bug 20443: Remove num_patron

Replace C4::Members::AttributeTypes->num_patron with
Koha::Patrons->filter_by_attribute_type

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 39 Jonathan Druart 2020-03-20 14:47:42 UTC
Created attachment 101134 [details] [review]
Bug 20443: Remove C4::Members::AttributeTypes

We do no longer need this package, we can use
Koha::Patron::Attribute::Types directly instead.

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 40 Jonathan Druart 2020-03-20 14:47:45 UTC
Created attachment 101135 [details] [review]
Bug 20443: Remove SearchIdMatchingAttribute - prove we are not cheating

Tests are still passing that way, we can continue

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 41 Jonathan Druart 2020-03-20 14:47:49 UTC
Created attachment 101136 [details] [review]
Bug 20443: Remove SearchIdMatchingAttribute

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 42 Jonathan Druart 2020-03-20 14:47:53 UTC
Created attachment 101137 [details] [review]
Bug 20443: Remove extended_attributes_merge

TODO We need tests here!

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 43 Jonathan Druart 2020-03-20 14:47:57 UTC
Created attachment 101138 [details] [review]
Bug 20443: Remove extended_attributes_code_value_arrayref AND C4::Members::Attributes

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 44 Jonathan Druart 2020-03-20 14:48:00 UTC
Created attachment 101139 [details] [review]
Bug 20443: (follow-up) Move GetBorrowerAttributes to Koha::Patron - ILSDI

Either we display everything, or only the info from the attribute itself
(i.e. not its type)

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 45 Jonathan Druart 2020-03-20 14:48:04 UTC
Created attachment 101140 [details] [review]
Bug 20443: (follow-up) Fix t/db_dependent/Koha/Patrons.t

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 46 Jonathan Druart 2020-03-20 14:48:08 UTC
Created attachment 101141 [details] [review]
Bug 20443: Fix Auth_with_ldap.t

Must be a hashref

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 47 Jonathan Druart 2020-03-20 14:48:12 UTC
Created attachment 101142 [details] [review]
Bug 20443: Use search_with_library_limits for attribute types

Dealt with that previously in the module during the rebase.
It conflicted with bug 23281.

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 48 Jonathan Druart 2020-03-20 14:48:16 UTC
Created attachment 101143 [details] [review]
Bug 20443: Fix Patrons/Import.t

->get_patrons has been removed but not replaced!

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 49 Jonathan Druart 2020-03-20 14:48:20 UTC
Created attachment 101144 [details] [review]
Bug 20443: Fix add/edit for checkboxes

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 50 Jonathan Druart 2020-03-20 14:48:24 UTC
Created attachment 101145 [details] [review]
Bug 20443: Fix POD

Fixes the following failures:

 FAIL	Koha/Patron/Attribute.pm
   FAIL	  pod
		*** ERROR:
		empty =head3

		 in file Koha/Patron/Attribute.pm

		*** ERROR:
		empty =head3

		 in file Koha/Patron/Attribute.pm

 FAIL	Koha/Patron/Attributes.pm
   FAIL	  pod
		*** ERROR:
		empty =head3

		 in file Koha/Patron/Attributes.pm

   FAIL	  pod coverage
		POD is missing for merge_with
		POD is missing for object_class

 FAIL	Koha/Patrons.pm
   FAIL	  spelling
		 paramter  ==> parameter

except the missing pod for object_class, not needed.

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 51 Jonathan Druart 2020-03-20 14:48:27 UTC
Created attachment 101146 [details] [review]
Bug 20443: Fix creation of new patron with attributes

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 52 Jonathan Druart 2020-03-20 14:48:31 UTC
Created attachment 101147 [details] [review]
Bug 20443: Do not explode if attribute does not have a valid av

Template process failed: undef error - Can't call method "lib" on an undefined value at /kohadevbox/koha/Koha/Patron/Attribute.pm line 109.                                                                         at /kohadevbox/koha/C4/Templates.pm line 122

We should not crash as it if the attribute is linked to an invalid
authorised value (for instance the AV has been deleted)

We must provide a check in the search_for_data_inconsistencies.pl script

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 53 Jonathan Druart 2020-03-20 14:48:35 UTC
Created attachment 101148 [details] [review]
Bug 20443: Fix patron modification approval

There was a mismatch between "value" and "attribute". Before this
patchset, "value" was sometimes used, but then it comes "attribute" to
match the DB column's name.
We must keep both here, when an upgrade is done we could have "value"
that is still in the borrower_modifications.extended_attributes JSON

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 54 Jonathan Druart 2020-03-20 14:48:39 UTC
Created attachment 101149 [details] [review]
Bug 20443: Handle non existent attribute when importing patrons

There is much more to do here, but this patch has the same behavior than
before: a warn is displayed in the log, the UI is not aware of it

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 55 Jonathan Druart 2020-03-20 14:48:42 UTC
Created attachment 101150 [details] [review]
Bug 20443: Fix merge_with behavior

merge_with were returning Koha::Patron::Attribute for existing attribute
and hashref for new attribute.

Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 Jonathan Druart 2020-03-20 15:04:54 UTC
Created attachment 101209 [details] [review]
Bug 20443: Revert some stuffs from 12159

These methods have been added to Koha::Patron::Attribute but are wrong, see bug 18339

We should use ->type->$method instead

Moreover the tests exist in another subtest, we do not need them.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 57 Jonathan Druart 2020-03-20 15:04:58 UTC
Created attachment 101210 [details] [review]
Bug 20443: Remove opac_display and opac_editable from Patron::Attribute

Same as previously for methods that have been added by bug 17792.
It's better to be explicite and tell we are fetch the related attribute's type

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 58 Jonathan Druart 2020-03-20 15:05:03 UTC
Created attachment 101211 [details] [review]
Bug 20443: Move GetBorrowerAttributeValue to Koha::Patron->get_extended_attribute_value

We want to retrieve a specific patron's attribute for a given patron.
We then add a new method to Koha::Patron.

This patch add a getter method ->get_extended_attribute_value
to use the DBIx::Class relation

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 59 Jonathan Druart 2020-03-20 15:05:07 UTC
Created attachment 101212 [details] [review]
Bug 20443: Move GetBorrowerAttributes to Koha::Patron - ILSDI

FIXME See comment

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 60 Jonathan Druart 2020-03-20 15:05:11 UTC
Created attachment 101213 [details] [review]
Bug 20443: Move GetBorrowerAttributes to Koha::Patron->extended_attributes

The GetBorrowerAttributes subroutine return the attributes for a given
patron.

Using get_extended_attributes we can acchieve it easily. The problematic
here is to restore the method's name (value vs attribute,
value_description vs description of the authorised value, as well as
display_checkout that should not be a method of Attribute, but
Attribute::Type instead)

value_description was used when the attribute types were attached to an
authorised value category. To avoid the necessary test in template and
controller there is now a $attribute->description method that will
display either the attribute's value OR the value of the authorised
value when needed. We should certainly use this one from few other
places.

Notes:
* This patch rename Koha::Patron->attributes with Koha::Patron->get_extended_attributes.
It will be renamed with Koha::Patron->extended_attributes in ones of the next
patches when it will become a setter as well.
* GetBorrowerAttributes did not care about the library limits, we still
do not
* The opac_only flag was not used outside of test, we drop it off.
* To maintain the existing behavior we add a default order-by clause to
the search method [code, attribute]
* From C4::Letters::_parseletter we always display the staff description
of the AV, There is now a FIXME to warn about it
* FIXMEs are not regressions, existing behaviors must be kept
* TODO add a new check to bug 21010 to search for inconsistencies in
patron's attributes attached to non-existent authorised values
* One test has been updated in Modifications.t, order_by is now
by default set to ['code', 'attribute']

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 61 Jonathan Druart 2020-03-20 15:05:15 UTC
Created attachment 101214 [details] [review]
Bug 20443: Improve filtering method

Koha::Patron::Attributes->search mimicks what is done in
Koha::AuthorisedValues->search.
But actually it should be more explicit when the caller use it.
For instance filter_by_branch_limitation (see discussion on bug 11983).

This will be useful for the following patches as we will need a way to
replace the $no_branch_limit flag.
When the $no_branch_limit flag is called, a simple ->search call should
be done.
When we want to limit on a specific library we can pass the branchcode
in paramter of filter_by_branch_limitation (this is not used yet).
If not passed the logged-in user library will be used by default.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 62 Jonathan Druart 2020-03-20 15:05:19 UTC
Created attachment 101215 [details] [review]
Bug 20443: Remove DeleteBorrowerAttributes

This subroutine was only used once, easy to replace.
SetBorrowerAttributes must replace the attributes for the ones logged-in
user is allowed to edit, that's why we filter by the library limits

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 63 Jonathan Druart 2020-03-20 15:05:23 UTC
Created attachment 101216 [details] [review]
Bug 20443: Remove DeleteBorrowerAttribute

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 64 Jonathan Druart 2020-03-20 15:05:27 UTC
Created attachment 101217 [details] [review]
Bug 20443: Remove UpdateBorrowerAttribute and SetBorrowerAttributes

This patch replace Koha::Patron->get_extended_attributes with
->extended_attributes
It's now a getter a setter method.

It permits to replace UpdateBorrowerAttribute and use
create_related from DBIx::Class

Notes:
* We face the same variable names difference than in a previous patch
(value vs attribute)

Bug 20443: Remove SetBorrowerAttributes

squash  + RM get_extended_attributes

 RM get_extended_attributes

SQUASH Bug 20443: Remove UpdateBorrowerAttribute and SetBorrowerAttribute

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 65 Jonathan Druart 2020-03-20 15:05:31 UTC
Created attachment 101218 [details] [review]
Bug 20443: Remove CheckUniqueness

There is already a method in Koha::Patron::Attribute to check the
uniqueness constraint, let us it to replace CheckUniqueness

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 66 Jonathan Druart 2020-03-20 15:05:35 UTC
Created attachment 101219 [details] [review]
Bug 20443: Move C4::Members::AttributeTypes::GetAttributeTypes to Koha::Patron::Attribute::Types

We can then now start to move methods from C4::Members::AttributeTypes
as well.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 67 Jonathan Druart 2020-03-20 15:05:39 UTC
Created attachment 101220 [details] [review]
Bug 20443: Remove num_patron

Replace C4::Members::AttributeTypes->num_patron with
Koha::Patrons->filter_by_attribute_type

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 68 Jonathan Druart 2020-03-20 15:05:43 UTC
Created attachment 101221 [details] [review]
Bug 20443: Remove C4::Members::AttributeTypes

We do no longer need this package, we can use
Koha::Patron::Attribute::Types directly instead.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 69 Jonathan Druart 2020-03-20 15:05:47 UTC
Created attachment 101222 [details] [review]
Bug 20443: Remove SearchIdMatchingAttribute - prove we are not cheating

Tests are still passing that way, we can continue

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 70 Jonathan Druart 2020-03-20 15:05:51 UTC
Created attachment 101223 [details] [review]
Bug 20443: Remove SearchIdMatchingAttribute

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 71 Jonathan Druart 2020-03-20 15:05:55 UTC
Created attachment 101224 [details] [review]
Bug 20443: Remove extended_attributes_merge

TODO We need tests here!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 72 Jonathan Druart 2020-03-20 15:05:59 UTC
Created attachment 101225 [details] [review]
Bug 20443: Remove extended_attributes_code_value_arrayref AND C4::Members::Attributes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 73 Jonathan Druart 2020-03-20 15:06:03 UTC
Created attachment 101226 [details] [review]
Bug 20443: (follow-up) Move GetBorrowerAttributes to Koha::Patron - ILSDI

Either we display everything, or only the info from the attribute itself
(i.e. not its type)

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 74 Jonathan Druart 2020-03-20 15:06:08 UTC
Created attachment 101227 [details] [review]
Bug 20443: (follow-up) Fix t/db_dependent/Koha/Patrons.t

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 75 Jonathan Druart 2020-03-20 15:06:11 UTC
Created attachment 101228 [details] [review]
Bug 20443: Fix Auth_with_ldap.t

Must be a hashref

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 76 Jonathan Druart 2020-03-20 15:06:16 UTC
Created attachment 101229 [details] [review]
Bug 20443: Use search_with_library_limits for attribute types

Dealt with that previously in the module during the rebase.
It conflicted with bug 23281.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 77 Jonathan Druart 2020-03-20 15:06:20 UTC
Created attachment 101230 [details] [review]
Bug 20443: Fix Patrons/Import.t

->get_patrons has been removed but not replaced!

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 78 Jonathan Druart 2020-03-20 15:06:23 UTC
Created attachment 101231 [details] [review]
Bug 20443: Fix add/edit for checkboxes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 79 Jonathan Druart 2020-03-20 15:06:28 UTC
Created attachment 101232 [details] [review]
Bug 20443: Fix POD

Fixes the following failures:

 FAIL	Koha/Patron/Attribute.pm
   FAIL	  pod
		*** ERROR:
		empty =head3

		 in file Koha/Patron/Attribute.pm

		*** ERROR:
		empty =head3

		 in file Koha/Patron/Attribute.pm

 FAIL	Koha/Patron/Attributes.pm
   FAIL	  pod
		*** ERROR:
		empty =head3

		 in file Koha/Patron/Attributes.pm

   FAIL	  pod coverage
		POD is missing for merge_with
		POD is missing for object_class

 FAIL	Koha/Patrons.pm
   FAIL	  spelling
		 paramter  ==> parameter

except the missing pod for object_class, not needed.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 80 Jonathan Druart 2020-03-20 15:06:32 UTC
Created attachment 101233 [details] [review]
Bug 20443: Fix creation of new patron with attributes

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 81 Jonathan Druart 2020-03-20 15:06:35 UTC
Created attachment 101234 [details] [review]
Bug 20443: Do not explode if attribute does not have a valid av

Template process failed: undef error - Can't call method "lib" on an undefined value at /kohadevbox/koha/Koha/Patron/Attribute.pm line 109.                                                                         at /kohadevbox/koha/C4/Templates.pm line 122

We should not crash as it if the attribute is linked to an invalid
authorised value (for instance the AV has been deleted)

We must provide a check in the search_for_data_inconsistencies.pl script

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 82 Jonathan Druart 2020-03-20 15:06:39 UTC
Created attachment 101235 [details] [review]
Bug 20443: Fix patron modification approval

There was a mismatch between "value" and "attribute". Before this
patchset, "value" was sometimes used, but then it comes "attribute" to
match the DB column's name.
We must keep both here, when an upgrade is done we could have "value"
that is still in the borrower_modifications.extended_attributes JSON

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 83 Jonathan Druart 2020-03-20 15:06:44 UTC
Created attachment 101236 [details] [review]
Bug 20443: Handle non existent attribute when importing patrons

There is much more to do here, but this patch has the same behavior than
before: a warn is displayed in the log, the UI is not aware of it

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 84 Jonathan Druart 2020-03-20 15:06:48 UTC
Created attachment 101237 [details] [review]
Bug 20443: Fix merge_with behavior

merge_with were returning Koha::Patron::Attribute for existing attribute
and hashref for new attribute.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 85 Martin Renvoize 2020-03-23 13:47:48 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 86 Tomás Cohen Arazi 2020-03-23 14:18:55 UTC
Ohhh
Comment 87 Jonathan Druart 2020-03-23 15:48:20 UTC
Created attachment 101472 [details] [review]
Bug 20443: Add missing use statement

This is a follow-up for commit 836e950d25148ca04ed0561c487772e3b0f7f8ee
    Bug 20443: Remove SearchIdMatchingAttribute

We removed the use of C4::Members::Attributes, that was using Text::CSV
Comment 88 Joy Nelson 2020-04-03 21:18:12 UTC
not backported to 19.11.x
Comment 89 Jonathan Druart 2020-05-07 15:05:56 UTC
Created attachment 104519 [details] [review]
Bug 20443: (follow-up) Remove C4::Members::AttributeTypes
Comment 90 Jonathan Druart 2020-05-07 15:06:14 UTC
(In reply to Jonathan Druart from comment #89)
> Created attachment 104519 [details] [review] [review]
> Bug 20443: (follow-up) Remove C4::Members::AttributeTypes

Pushed to master for 20.05