On bug 17591 we discovered that there was something weird going on with
the way we export and use subroutines/modules.
This patch tries to standardize our EXPORT to use EXPORT_OK only.
That way we will need to explicitely define the subroutine we want to
use from a module.
This patch is a squashed version of:
Bug 17600: After export.pl
Bug 17600: After perlimport
Bug 17600: Manual changes
Bug 17600: Other manual changes after second perlimports run
Bug 17600: Fix tests
And a lot of other manual changes.
export.pl is a dirty script that can be found on bug 17600.
"perlimport" is:
git clone https://github.com/oalders/App-perlimports.git
cd App-perlimports/
cpanm --installdeps .
export PERL5LIB="$PERL5LIB:/kohadevbox/koha/App-perlimports/lib"
find . \( -name "*.pl" -o -name "*.pm" \) -exec perl App-perlimports/script/perlimports --inplace-edit --no-preserve-unused --filename {} \;
The ideas of this patch are to:
* use EXPORT_OK instead of EXPORT
* perltidy the EXPORT_OK list
* remove '&' before the subroutine names
* remove some uneeded use statements
* explicitely import the subroutines we need within the controllers or
modules
Note that the private subroutines (starting with _) should not be
exported (and not used from outside of the module except from tests).
EXPORT vs EXPORT_OK (from
https://www.thegeekstuff.com/2010/06/perl-exporter-examples/)
"""
Export allows to export the functions and variables of modules to user’s namespace using the standard import method. This way, we don’t need to create the objects for the modules to access it’s members.
@EXPORT and @EXPORT_OK are the two main variables used during export operation.
@EXPORT contains list of symbols (subroutines and variables) of the module to be exported into the caller namespace.
@EXPORT_OK does export of symbols on demand basis.
"""
If this patch caused a conflict with a patch you wrote prior to its
push:
* Make sure you are not reintroducing a "use" statement that has been
removed
* "$subroutine" is not exported by the C4::$MODULE module
means that you need to add the subroutine to the @EXPORT_OK list
* Bareword "$subroutine" not allowed while "strict subs"
means that you didn't imported the subroutine from the module:
- use $MODULE qw( $subroutine list );
You can also use the fully qualified namespace: C4::$MODULE::$subroutine
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
# Failed test 'checkpw_ldap tests'
# at t/db_dependent/Auth_with_ldap.t line 324.
{UNKNOWN}: Configuration not defined at /usr/share/perl5/Log/Log4perl/Config.pm line 579. at /kohadevbox/koha/Koha/Patron.pm line 395
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
To test:
Ideally tested on a working ldap server with bind by auth and no
anonymous bind
1 - Define an LDAP config with bind by auth
2 - Don't define user/pass
3 - Define anonymous_bind = 0
4 - Attempt bind by auth
5 - Error is something like:
LDAP search failed to return object : XXXXXXXXX: LdapErr: XXXX-XXXXXX,
comment: In order to perform this operation a successful bind must
be completed on the connection., data 0, v2580 at
/usr/share/koha/lib/C4/Auth_with_ldap.pm line 102.
6 - Define user/pass
7 - Now bind by auth should work
8 - remove user/pass
9 - Apply patch
10 - Attempt again
11 - Bind by auth shoudl succeed
prove -v t/db_dependent/Auth_with_ldap.t
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Brendan A Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
The test files do not need to return 1
Patch generated with:
perl -p -i -e "s/^1;\n//xsm" t/**/*.t
Test plan:
git grep '^1;$' t/**/*.t
should not return any results
NOTE: does not fix C4/SIP/t, nor xt tests.
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Oliver Bock <oliver.bock@aei.mpg.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Koha::Patron->delete create a transaction and so we need to use
txn_begin to have nested txn
DBD::mysql::db begin_work failed: Already in a transaction
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
This patch moves the C4::Members::DelMember subroutine to the
Koha::Patron module.
The delete method must be overwritten to permit handling of patron's
holds.
Test plan:
(With the 2 patches applied)
1/ Create a patron with holds and owner of lists
2/ Delete patrons using the web interface:
- More > Delete on a patron page
- Batch patron deletion tools
3/ and the cronjob script
- perl misc/cronjobs/delete_patrons.pl -c [more options]
The patron should have been moved to the deletedborrowers table, his/her
holds and lists should have been deleted.
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
perltidy
old new calls to new call format
attrType -> attr_type (perlcritic friendlier)
double quotes to single quotes
'' combinations to q{} (perlcritic friendlier)
refactored parameters to mock_net_ldap_search into a HASH
This piece is not necessary, but I think it is nicer.
Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as expected. No koha-qa erros
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Revisiting this bug, I noticed that C4::Context->config() was mocked
poorly. I expanded the mock function a little and have quieted the
test which seemed to be working in a broken manner before.
TEST PLAN
---------
1) prove t/db_dependend/Auth_with_ldap.t
-- shibboleth noise
2) apply this patch
3) prove t/db_dependend/Auth_with_ldap.t
-- no noise
4) run koha qa test tools
Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Test plan:
- Update your configuration file to use LDAP authentication and enable update
(<update>1</update>) option,
- login with an existing user with extended attrbitutes that are not in
LDAP mapping,
- check that all attributes are still here.
Signed-off-by: Chris <chrisc@catalyst.net.nz>
Signed-off-by: Philippe Blouin <philippe.blouin@inlibro.com>
Signed-off-by: Jesse Weaver <jweaver@bywatersolutions.com>
checkpw_ldap should return 0 if it is not an anonymous bind, and authentication
fails. This is better explained on the bug comments. This is just a regression
test for the revised functionality.
To test:
- Run
$ prove t/db_dependent/Auth_with_ldap.t
=> FAIL: it fails because C4::Auth_with_ldap doesn't match the expected behaviour
- Apply the bugfix from Martin
- Run
$ prove t/db_dependent/Auth_with_ldap.t
=> SUCCESS: tests now pass.
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
This followup provides some unit tests for C4::Auth_with_ldap. It implements
regression tests for this bug's changes too.
The aim was to cover all use cases for checkpw_ldap from the first line up to
those touched by this bug's patches. It should serve as a start for having
unit tests for the LDAP thing.
Having said that: no excuses for not providing regression tests for LDAP from now on :-/
Regards
To+
Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Also added more feedback and the ability to override KOHA_CONF
"useldapserver" value with ENV variable KOHA_USELDAPSERVER.
This patch should be considered for inclusion to 3.0 stable branch
because it prevents unnecessary failure of test battery.
Signed-off-by: Galen Charlton <galen.charlton@liblime.com>
Moved test cases that depend on the DBMS and having
an initialized Koha database to a subdirectory
of t so that they will not be swept up into
the default 'make test'. Goal is to have
these DB-dependent tests runnable either via
a special make target or perhaps from the
web installer.
Signed-off-by: Chris Cormack <crc@liblime.com>
Signed-off-by: Joshua Ferraro <jmf@liblime.com>
