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?
*** Bug 20445 has been marked as a duplicate of this bug. ***
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.
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 ;)
Remote branch has been rebased against master.
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.
(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'...
(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.
I just rebased this on top of current code in bug 12159, see https://gitlab.com/josef.moravec/Koha/tree/bug_20443
Tests not passing (at least): t/db_dependent/Koha/Patrons/Import.t t/db_dependent/Auth_with_ldap.t
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?
(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
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.
Remote branch rebased against master.
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 ;)
Remote branch rebased against master. The failing test is fixed!
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)
Fixed, Josef!
(In reply to Jonathan Druart from comment #20) > Fixed, Josef! Thanks Jonathan Signed off!
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
(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?
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
(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.
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
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>
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>
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>
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>
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>
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>
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>
Created attachment 101129 [details] [review] Bug 20443: Remove DeleteBorrowerAttribute Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
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>
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>
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>
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>
Created attachment 101136 [details] [review] Bug 20443: Remove SearchIdMatchingAttribute Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
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>
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>
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>
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>
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>
Created attachment 101144 [details] [review] Bug 20443: Fix add/edit for checkboxes Signed-off-by: Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Created attachment 101212 [details] [review] Bug 20443: Move GetBorrowerAttributes to Koha::Patron - ILSDI FIXME See comment Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
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>
Created attachment 101216 [details] [review] Bug 20443: Remove DeleteBorrowerAttribute Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
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>
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>
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>
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>
Created attachment 101223 [details] [review] Bug 20443: Remove SearchIdMatchingAttribute Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101224 [details] [review] Bug 20443: Remove extended_attributes_merge TODO We need tests here! Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
Created attachment 101227 [details] [review] Bug 20443: (follow-up) Fix t/db_dependent/Koha/Patrons.t Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 101228 [details] [review] Bug 20443: Fix Auth_with_ldap.t Must be a hashref Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
Created attachment 101231 [details] [review] Bug 20443: Fix add/edit for checkboxes Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
Created attachment 101233 [details] [review] Bug 20443: Fix creation of new patron with attributes Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
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>
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>
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>
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>
Nice work everyone! Pushed to master for 20.05
Ohhh
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
not backported to 19.11.x
Created attachment 104519 [details] [review] Bug 20443: (follow-up) Remove C4::Members::AttributeTypes
(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