| Summary: | Auth_with_ldap check if categorycode is valid | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Dobrica Pavlinusic <dpavlin> |
| Component: | Authentication | Assignee: | Dobrica Pavlinusic <dpavlin> |
| Status: | CLOSED FIXED | QA Contact: | Bugs List <koha-bugs> |
| Severity: | major | ||
| Priority: | PATCH-Sent (DO NOT USE) | CC: | chris, dpavlin, mglavica, paul.poulain |
| Version: | 3.6 | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | Sponsored | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: |
check validity of categorycode when importing from ldap
Bug 6022: Auth_with_ldap check if categorycode is valid Bug 6022: Auth_with_ldap check if categorycode is valid Bug 6022: Auth_with_ldap check if categorycode is valid Bug 6022: Auth_with_ldap check if categorycode is valid |
||
|
Description
Dobrica Pavlinusic
2011-03-31 12:18:53 UTC
Created attachment 3572 [details] [review] check validity of categorycode when importing from ldap This bug is mentioned in: Bug 6022: Auth_with_ldap check if categorycode is valid http://lists.koha-community.org/pipermail/koha-patches/2011-March/014397.html Created attachment 4140 [details] [review] Bug 6022: Auth_with_ldap check if categorycode is valid When importing users from LDAP, Auth_with_ldap.pm doesn't check if value for categorycode is present in categories table in Koha resulting in referential integrity error instead of using default value from koha-conf.xml Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> QA comment
I have some comments, 2 of them being blockers :
+ # XXX check if categorycode exists, if not, fallback to default from koha-conf.xml
1 why XXX at the beginning of the comment ? (not blocking, i'm just surprised. ++ for the comment though
+ my $dbh = C4::Context->dbh;
+ my $sth = $dbh->prepare("SELECT categorycode FROM categories WHERE upper(categorycode) = upper(?)");
2 Why upped in SQL ? It's useless, as SQL don't care with uc/lc or diacritics.
categorycodes are automatically UCed, so I think it would be better to :
* UC the $borrower{'categorycode'}
* do SQL query without upped()
+ $sth->execute( $borrower{'categorycode'} );
+ if ( my $row = $sth->fetchrow_hashref ) {
3 Why an empty IF then ELSE ? UNLESS would have done the job !
+
+ } else {
+ my $default = $mapping{'categorycode'}->{content};
+ warn "Can't find ", $borrower{'categorycode'}, " default to: $default for ", $borrower{userid};
4 UNCONDITIONAL WARN detected. If you want to add a warn (which is OK), please use
$DEBUG && warn "blabla...";
Thus the warn will be issued only if you're in DEBUG mode (set in httpd.conf)
+ $borrower{'categorycode'} = $default
+ }
Sorry, but failed QA for reasons 2 and 4 (and 3 should be fixed as well, but I wouldn't fail QA just for reason 3)
I see it's written "ENH" => nope, it's a bug. And a major one according to me. So upgrading severity Created attachment 4985 [details] [review] Bug 6022: Auth_with_ldap check if categorycode is valid Cleanup required by QA Created attachment 6391 [details] [review] Bug 6022: Auth_with_ldap check if categorycode is valid added test scenario. Test scenario: 1. enable LDAP in koha-conf.xml using <useldapserver>1</useldapserver> and add <ldapserver> configuration with <categorycode is="SomeLDAPField">DefaultCategoryCode</categorycode> 2. select/create LDAP user with category in SomeLDAPField which isn't in Koha 3. try logging in and ensure that assigned category to new user is DefaultCategoryCode Created attachment 6392 [details] [review] Bug 6022: Auth_with_ldap check if categorycode is valid Signed-off-by: Marijana Glavica <mglavica@ffzg.hr> QA comment: * code is OK, clean, commented * I've tested that without LDAP, nothing has changed Patch pushed, please test Included in 3.6 prior to 3.6.5. |