[Koha-patches] [Bug 5379] Pull Request

[Chris Nighswonger]

 Hey Galen,

On Tue, Nov 9, 2010 at 11:32 PM, Galen Charlton <gmcharlt at gmail.com> wrote:

> Hi,
>
> I have the following comments on the patch:
>
> [1] To bail out of SetBorrowerAttributes, just do a return, not return
> 0 - at present, nothing is looking for a return value for that
> function, so a plain 'return' would do what the consumers expect and
> return undef.
>

I disagree since an undef return is a de facto indicator of success at
present, doing a return only on a db error and then throwing an error to the
interface based on that value results in an inconsistency with some of the
consumers reading the undef return as success and others as failure. Grep
seems to think that no present consumer looks at the return value of this
sub, so setting it to something to differentiate an error would not appear
to hurt any present calls to it and would give a positive indication of an
error.



> [2] Bailing out of AddMember should be done before invoking
> manualinvoice.  I also suggest using a plain return; having AddMember
> return undef (but note, I don't mean doing 'return undef;', I mean
> 'return;') would both signify the error condition *and* force
> consumers to not blithely charge along trying to work with a patron
> whose borrowernumber is 0.  I'm thinking of memberentry.pl in
> particular.  Also, changing the return of AddMember is definitely
> cause to update the POD.
>

I agree with moving where we bail so that no further code is executed after
the db error.

I disagree, however, with an undef return. Again, grep shows that returning
0 on a db error will not cause any worse problems than are presently cause
then an undef is returned. It will, however, permit positive confirmation of
an error for the consumer. And in any case, current calls to this sub should
be fixed up to handle an undef return. There are not many.

Kind Regards,
Chris



> On Tue, Nov 9, 2010 at 10:42 AM, Chris Nighswonger
> <cnighswonger at foundations.edu> wrote:
> > The following changes since commit
> 1aef81c354eb17c2cf2d557cac78d4e37c30a046:
> >  Chris Cormack (1):
> >        Ukranian and Russian syspref language updates
> >
> > are available in the git repository at:
> >
> >  git://git.koha-community.org/wip/koha-fbc.git k_bug_5379
> >
> > Chris Nighswonger (1):
> >      Bug 5379 - import_borrowers.pl fails with db insert/update errors
> >
> >  C4/Members.pm                                      |    5 +++++
> >  C4/Members/Attributes.pm                           |    6 ++++++
> >  .../prog/en/modules/tools/import_borrowers.tmpl    |    6 ++++--
> >  tools/import_borrowers.pl                          |   16
> ++++++++++++++--
> >  4 files changed, 29 insertions(+), 4 deletions(-)
> > _______________________________________________
> > Koha-patches mailing list
> > Koha-patches at lists.koha-community.org
> > http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-patches
> > website : http://www.koha-community.org/
> > git : http://git.koha-community.org/
> > bugs : http://bugs.koha-community.org/
> >
>
>
>
> --
> Galen Charlton
> gmcharlt at gmail.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/koha-patches/attachments/20101111/e83eceae/attachment.htm>