This change is being sponsored by Melbourne Athenaeum Library.

Colin Campbell wrote:
I've had this reported recently but havent been able to look at it. I
think there might be a change of behaviour I think screen message used to
get a default value in cases like this rather than the cryptic "1". It
may be the initial flag value for screen_msg in ILS::Patron is not
getting replaced by a default?

This is running in production for the sponsor, using 3.16.07.  It works nicely.
I'll sign off, but it may need another sign-off against Master?

Hi Mason, 

can the first patch be obsoleted?
I think this is not a problem specific to OverDrive, but to the error messages from the SIP server? Shoudl we adjust the title?
To have the library show up in the release notes as a sponsor, you can add Sponsored-by: ... to the commit message (http://wiki.koha-community.org/wiki/Commit_messages)

(In reply to Katrin Fischer from comment #6)
> Hi Mason, 
> 
> can the first patch be obsoleted?

hmm, no - it's a perltidy patch, that the feature depends on
as you can see from the tidy patch, the sub() desperately needed tidying

> I think this is not a problem specific to OverDrive, but to the error
> messages from the SIP server? Shoudl we adjust the title?

sure, thats fine with me

i really was thinking about calling the bug/patch something a bit more general. but i couldn't really think of anything suitable


> To have the library show up in the release notes as a sponsor, you can add
> Sponsored-by: ... to the commit message
> (http://wiki.koha-community.org/wiki/Commit_messages)

thanks, i'll do that :0)

Pardon my being late to reply to this, but could this be solved with a a screen message regex?

<screen_msg_regex find="(1) " replace="Invalid patron or patron password" />

If not, please reset the status of this bug.

(In reply to Bob Birchall from comment #0)
> When a Koha user offers correct credentials, Overdrive redirects to the
> users'
> Overdrive account page and no messages from Koha are displayed.
> 
> However:
> 1.  Where a correct cardnumber is entered with an incorrect password, Koha's
> SIP server responds with '(1) Greetings from Koha.'
> 2.  Where an incorrect cardnumber is entered (with any password), the SIP
> server responds with a blank message: '(1) '
> 
> These are displayed on an Overdrive page which includes a link: 'Click here
> to return to the sign in form' but there is nothing to draw the user's
> attention to their error and the need to follow that link back to try again.
> 
> We propose to replace these messages with 'You have entered an incorrect
> cardnumber or password.  Please try again.'   This will appear just above
> the 'Click here to return ...' link and should make the process obvious for
> the user.

Hm, not sure this would work?

3/ enter a correct username and correct password in overdrive
see overdrive display '(1) Greetings from Koha…' (good)

4/ enter a correct username and *incorrect* password in overdrive
see overdrive display '(1) Greetings from Koha…' (bad)

5/ enter an incorrect username in overdrive
see overdrive display '(1)' (badder)

... and 1 being the cardnumber.

I see. I hadn't looked deeply enough to realize 1 was the card number. In that case my solution won't work.

(In reply to Katrin Fischer from comment #10)
> ... and 1 being the cardnumber.

This is looking really good, but I think if we are going to change the behavior for the patron status response, I think we should have the same behavior for the patron information response as well. Mason, can you implement that?

sorry, i missed some questions here...

(In reply to Kyle M Hall from comment #8)
> Pardon my being late to reply to this, but could this be solved with a a
> screen message regex?
> 
> <screen_msg_regex find="(1) " replace="Invalid patron or patron password" />

^ no that wont work, the glitchy '(1)' string is created within the Overdrive app, and external to Koha

(In reply to Kyle M Hall from comment #11)
> I see. I hadn't looked deeply enough to realize 1 was the card number. In
> that case my solution won't work.
> 
> (In reply to Katrin Fischer from comment #10)
> > ... and 1 being the cardnumber.

just to confirm, (1) appears to be random string from within the Overdrive app
..it's not related to any 'id' within Koha

Given Mason's comments, does this still need to be in Failed QA?

(In reply to Mason James from comment #19)
> (In reply to Kyle M Hall from comment #11)
> > I see. I hadn't looked deeply enough to realize 1 was the card number. In
> > that case my solution won't work.
> > 
> > (In reply to Katrin Fischer from comment #10)
> > > ... and 1 being the cardnumber.
> 
> just to confirm, (1) appears to be random string from within the Overdrive
> app
> ..it's not related to any 'id' within Koha

So for the patron status response, Koha is sending back the "(1)" but not for the patron information response?

(In reply to Kyle M Hall from comment #21)
> (In reply to Mason James from comment #19)
> > (In reply to Kyle M Hall from comment #11)
> > > I see. I hadn't looked deeply enough to realize 1 was the card number. In
> > > that case my solution won't work.
> > > 
> > > (In reply to Katrin Fischer from comment #10)
> > > > ... and 1 being the cardnumber.
> > 
> > just to confirm, (1) appears to be random string from within the Overdrive
> > app
> > ..it's not related to any 'id' within Koha
> 
> So for the patron status response, Koha is sending back the "(1)" but not
> for the patron information response?

Nope, afaik, the '(1)' string is created and displayed completely within the Overdrive web app
..therefore, it is impossible to correct/remove this string within Koha

(In reply to Bob Birchall from comment #20)
> Given Mason's comments, does this still need to be in Failed QA?

any update Kyle?

> Nope, afaik, the '(1)' string is created and displayed completely within the
> Overdrive web app
> ..therefore, it is impossible to correct/remove this string within Koha

That seems like quite a coincidence, but I've no reason to doubt you! Back to needs signoff!

Some preliminary QA comment on the first patch only:

Smart perl tidy: Removing Sip:: and adding $server. If I only had such a smart perl tidy :)
Seriously, we should not do ANY changes if we say that a patch is a perltidy patch. Changes should be in another patch!
< $resp .= $lang . Sip::timestamp();
> $resp .= $lang . timestamp();
< $resp .= maybe_add( FID_SCREEN_MSG, $patron->screen_msg );
< $resp .= maybe_add( FID_SCREEN_MSG, $patron->{branchcode} )
> $resp .= maybe_add( FID_SCREEN_MSG, $patron->screen_msg, $server );
> $resp .= maybe_add( FID_SCREEN_MSG, $patron->{branchcode}, $server )
< $resp .= 'YYYY' . ( ' ' x 10 ) . $lang . Sip::timestamp();
> $resp .= 'YYYY' . (' ' x 10) . $lang . timestamp();

Interestingly, this cuddled else disappears too. But note that xt/perltidyrc contains a --cuddled-else. Note that I do not like cuddled else, but somewhere in time the community decided that we should use it. (Nothing better than discussion about tabs or curly braces..)
-    } else {
+    }
+    else {
Your tidy patch also contains constructs like (variable) instead of the preferred ( variable ). Two simple examples:
< foreach my $i ( keys(%handlers) ) {
> foreach my $i (keys(%handlers)) {
<     my ( $class, $msg, $seqno ) = @_;
>     my ($class, $msg, $seqno) = @_;
So, it appears that you did not use the default Koha perltidyrc but some other one.
By conclusion, I will add a new perl tidy patch and extract your real changes in a separate patch.

I rebased this patch set on the new perltidy.
The second patch includes comments on the changes from Mason.
This effectively obsoletes the last follow-up from Kyle to fix the namespace; the namespace is not needed.
Still have to look at patch 2 and forth.

And some advanced wrestling with utf8 characters in the commit message (and the git version on Debian 7)

Will continue QA session here later.

(In reply to Marcel de Rooy from comment #41)
> Will continue QA session here later.

Submitting some follow-ups to get this thru QA later this week.

Created attachment 48667 [details] [review]
Bug 13871: Adjusted perl tidy on MsgType.pm

Run perltidy pro=xt/perltidyrc on the file.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 48668 [details] [review]
Bug 13871: [TITLE_AMENDED] Additional changes

The original perltidy patch from Mason has been amended.

The perltidy itself has been moved to a separate patch with the current
perltidyrc applied.

As noted on Bugzilla, the original perltidy patch included some extra
changes:
[1] You prefix timestamp with Sip
    This is not actually needed (it is imported), but if we should prefix it,
    we should prefix now with C4::SIP::Sip. But you only changed two
    occurrences (out of 26). So I remove these two changes.
[2] You remove the $server parameter from two calls of maybe_add:
    A closer look at the remaining code tells me that $server is always
    passed to maybe_add for FID_SCREEN_MSG. So this only left me the
    current whitespace change.
But at least we documented what we did or did not, and why..

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 48669 [details] [review]
Bug 13871 - OverDrive message when user authentication fails

NOTE: apply this patch after the additional perltidy patch

this patch is basically a small work-around to fix some confusing login text,
when users enter incorrect auth details via Overdrive's website

with this option disabled (default) there is no change to SIP's behaviour

to test...

1/ configure your overdrive account to talk to your Koha's SIP service

2/ start Koha's SIP

3/ enter a correct username and correct password in overdrive
see overdrive display '(1) Greetings from Koha' (good)

4/ enter a correct username and *incorrect* password in overdrive
see overdrive display '(1) Greetings from Koha' (bad)

5/ enter an incorrect username in overdrive
see overdrive display '(1)' (badder)

6/ apply patch, enable 'overdrive-mode' in Koha's SIPConfig.xml

example...
---------------------
<accounts>
<login id="kohasip" password="xxxxx" delimiter="|"
error-detect="enabled" institution="YYY" overdrive-mode="1" />
</accounts>
---------------------

7/ restart SIP

8/ enter a correct username and correct password
see overdrive display '(1) Greetings from Koha'

9/ enter a correct username and *incorrect* password
see overdrive display '(1) Invalid patron or patron password'

10/ enter an incorrect username and incorrect password
see overdrive display '(1) Invalid patron or patron password'

http://bugs.koha-community.org/show_bug.cgi?id=1387

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 48670 [details] [review]
Bug 13871: [QA Follow-up] Adjust Patron Status Request

Conform QA comment on Bugzilla, we do this:

[1] Attribute for overdrive mode/invalid credentials is not really needed.
    We can always pass a screen message that card or password is invalid.
[2] If the cardnumber is correct and the password is wrong, we should
    still honour the request. The bad password is recognized by BLN and
    an additional message in AF.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Verified by telnetting SIP server.
And tested additionally with the new unit test of bug 15956.

Created attachment 48671 [details] [review]
Bug 13871: [QA Follow-up] Adjust Patron Info Request

Conform QA comment, Patron Info request is slightly adjusted to be
consistent with changes to Patron Status request.

If the cardnumber is ok and the password is wrong, BL=N is reported but
also add 'Invalid password' in AF.
Additionally, an invalid card number is reported in AF.

Test plan:
[1] Send patron info request for invalid card.
[2] Idem for valid card, no password.
[3] Idem for valid card, good password.
[4] Idem for valid card, wrong password.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Verified by telnetting SIP server.
And tested additionally with the new unit test of bug 15956.

QA Comment:
Since this patch waited much too long, I have been working somewhat more on it to get it thru QA.
My main concerns were:
[1] Change of behavior when sending correct card number but invalid password. We should get patron info with BL=Y and CQ=N.
Your patch changed the if-structure and handled this situation similar to wrong card number. So no patron information. 
Handled in a followup.
[2] Adding a SIPconfig attribute like overdrivemode or msg_invalid_credentials is not really needed. If we can prevent adding such "prefs", the code will be simpler.
Although the response contains BL and CQ to inform about the checked credentials, we could just send "Invalid card number" if there is no card (add AF here).
If the card is OK and the pw is not, we could add Invalid pw to the existing AF. This does not harm anyone.
Handled in a followup.
[3] We should have a look at Patron Info too. Added a followup.
Added a new report for additional unit tests for changes in Patron Status/Patron Info.

Passed QA

Created attachment 48672 [details] [review]
Bug 13871: [QA Follow-up] Add $server for FID_SCREEN_MSG

When you are ready, you still see that small detail.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Pushed to Master - Should be in the May 2016 release. Thanks!

Patches pushed to 3.22.x, will be in 3.22.6

This patch has altered the way that the patron branch is sent.  Previously it was in a separate AF field, now the branch is concatenated onto the other messages with a ' -- '

I am not sure if this is intentional but it is causing problems for one of our libraries where this field was captured for statistical purposes. Can we adjust this?

Hi Nick, can you please open a new bug linked to this one?

(In reply to Nick Clemens from comment #52)
> This patch has altered the way that the patron branch is sent.  Previously
> it was in a separate AF field, now the branch is concatenated onto the other
> messages with a ' -- '
> 
> I am not sure if this is intentional but it is causing problems for one of
> our libraries where this field was captured for statistical purposes. Can we
> adjust this?

hi Nick, no, it's not an intentional change, and can be changed back

(i think the AF change may have happened in a follow-up patch?)

(In reply to Nick Clemens from comment #52)
> This patch has altered the way that the patron branch is sent.  Previously
> it was in a separate AF field, now the branch is concatenated onto the other
> messages with a ' -- '
> 
> I am not sure if this is intentional but it is causing problems for one of
> our libraries where this field was captured for statistical purposes. Can we
> adjust this?

Hi Nick,
Your problem should not be that hard to resolve.
I think we should not rely on the fact that AF only includes the patron branch code. It is just a screen message field. And I am not sure if SIP2 officially supports multiple AF fields btw.
Could you use field AO that only contains the institution id? But not usable for another branch.
Or - as workaround - add a prefix to the AF home branch part and capture only that part with a simple regex or so?

> Hi Nick,
> Your problem should not be that hard to resolve.
> I think we should not rely on the fact that AF only includes the patron
> branch code. It is just a screen message field. And I am not sure if SIP2
> officially supports multiple AF fields btw.
> Could you use field AO that only contains the institution id? But not usable
> for another branch.
> Or - as workaround - add a prefix to the AF home branch part and capture
> only that part with a simple regex or so?

Yes, SIP2 can have multiple AF fields. The branchcode in AF field feature was requested specifically by Overdrive to be able to track use by home library. The AO does not transmit the patron home branch, it transmits the branch of the user used to authenticate as for using SIP. It is not practical or correct to send the patron branch on the AO field. This is definitely a regression and needs to be fixed or we may have to revert this bug in master before the release.

(In reply to Kyle M Hall from comment #56)
> Yes, SIP2 can have multiple AF fields. 
In that case that is the best route.

(In reply to Kyle M Hall from comment #56)
> This is
> definitely a regression and needs to be fixed or we may have to revert this
> bug in master before the release.

Note that I have seen more serious regressions than this where the word "revert" is not used rightaway. Just saying.

The problem I see here is that it's already in a stable release. So a revert is  not as easy as just getting it out of master.

(In reply to Katrin Fischer from comment #59)
> The problem I see here is that it's already in a stable release. So a revert
> is  not as easy as just getting it out of master.

It is a trivial fix. So why talk revert?

@RM: Handled as QA follow-up. Please check last patch and push that one only.

I agree - fixing it is better.

Created attachment 51422 [details] [review]
Bug 13871: [QA Follow-up] Fix small regression on AF with branchcode in Patron Info request

If you enable send_patron_home_library_in_af in your sip account, you
want a separate AF field for the home branch.

Test plan:
Send a 63 (Patron Info) and verify that you have an extra AF.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested Patron Status and Patron Info.

Thanks Marcel! Follow-up has been pushed to master for 16.05.

*** Bug 16479 has been marked as a duplicate of this bug. ***

Follow-up pushed to 3.22.x, will be in 3.22.7