Bug 13871 - OverDrive message when user authentication fails
Summary: OverDrive message when user authentication fails
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Mason James
QA Contact: Marcel de Rooy
URL:
Keywords:
: 16479 (view as bug list)
Depends on:
Blocks: 15956
  Show dependency treegraph
 
Reported: 2015-03-19 00:08 UTC by Bob Birchall
Modified: 2016-12-05 21:25 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 13871 - OverDrive message when user authentication fails (4.02 KB, patch)
2015-04-07 09:39 UTC, Mason James
Details | Diff | Splinter Review
Bug 13871 - OverDrive message when user authentication fails (4.78 KB, patch)
2015-04-07 10:18 UTC, Mason James
Details | Diff | Splinter Review
Bug 13871 - perltidy build_patron_status (3.96 KB, patch)
2015-04-23 18:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13871 - perltidy build_patron_status (3.96 KB, patch)
2015-04-23 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13871 - OverDrive message when user authentication fails (4.74 KB, patch)
2015-04-23 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13871 [QA Followup] - Rename parameter to send_msg_on_invalid_credentials (2.42 KB, patch)
2015-04-23 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13871 [QA Followup] - Fix namespaces (1.22 KB, patch)
2015-04-23 18:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13871 - perltidy build_patron_status (4.02 KB, patch)
2015-12-14 13:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13871 - perltidy build_patron_status (4.02 KB, patch)
2015-12-14 13:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13871 - OverDrive message when user authentication fails (4.79 KB, patch)
2015-12-14 13:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13871 [QA Followup] - Rename parameter to send_msg_on_invalid_credentials (2.48 KB, patch)
2015-12-14 13:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13871 [QA Followup] - Fix namespaces (1.28 KB, patch)
2015-12-14 13:12 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13871: Adjusted perl tidy on MsgType.pm (101.37 KB, patch)
2016-02-26 09:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871: [TITLE_AMENDED] Additional changes (1.72 KB, patch)
2016-02-26 09:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871 - OverDrive message when user authentication fails (4.62 KB, patch)
2016-02-26 09:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871 [QA Followup] - Rename parameter to send_msg_on_invalid_credentials (2.58 KB, patch)
2016-02-26 09:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871 - OverDrive message when user authentication fails (4.61 KB, patch)
2016-02-26 09:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871 [QA Followup] - Rename parameter to send_msg_on_invalid_credentials (2.58 KB, patch)
2016-02-26 09:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871 - OverDrive message when user authentication fails (4.52 KB, patch)
2016-02-26 09:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871 [QA Followup] - Rename parameter to send_msg_on_invalid_credentials (2.58 KB, patch)
2016-02-26 09:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871: Adjusted perl tidy on MsgType.pm (101.37 KB, patch)
2016-03-04 10:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871: [TITLE_AMENDED] Additional changes (1.72 KB, patch)
2016-03-04 10:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871 - OverDrive message when user authentication fails (4.58 KB, patch)
2016-03-04 10:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871: [QA Follow-up] Adjust Patron Status Request (3.40 KB, patch)
2016-03-04 10:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871: [QA Follow-up] Adjust Patron Info Request (2.88 KB, patch)
2016-03-04 10:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871: [QA Follow-up] Add $server for FID_SCREEN_MSG (1.35 KB, patch)
2016-03-04 10:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871: [QA Follow-up] Fix small regression on AF with branchcode in Patron Info request (1.30 KB, patch)
2016-05-11 12:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13871: [QA Follow-up] Fix small regression on AF with branchcode in Patron Info request (1.39 KB, patch)
2016-05-11 12:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bob Birchall 2015-03-19 00:08:52 UTC
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.
Comment 1 Bob Birchall 2015-03-19 00:10:47 UTC
This change is being sponsored by Melbourne Athenaeum Library.
Comment 2 Bob Birchall 2015-03-19 00:11:39 UTC
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?
Comment 3 Mason James 2015-04-07 09:39:11 UTC Comment hidden (obsolete)
Comment 4 Mason James 2015-04-07 10:18:41 UTC Comment hidden (obsolete)
Comment 5 Bob Birchall 2015-04-08 02:08:47 UTC
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?
Comment 6 Katrin Fischer 2015-04-13 08:36:19 UTC
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)
Comment 7 Mason James 2015-04-14 09:15:20 UTC
(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)
Comment 8 Kyle M Hall 2015-04-23 13:03:09 UTC
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.
Comment 9 Katrin Fischer 2015-04-23 13:06:00 UTC
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)
Comment 10 Katrin Fischer 2015-04-23 13:06:21 UTC
... and 1 being the cardnumber.
Comment 11 Kyle M Hall 2015-04-23 18:09:51 UTC
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.
Comment 12 Kyle M Hall 2015-04-23 18:16:41 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2015-04-23 18:17:33 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-04-23 18:17:40 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2015-04-23 18:17:43 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2015-04-23 18:40:05 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2015-04-23 18:49:19 UTC
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?
Comment 18 Mason James 2015-09-03 02:23:10 UTC
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
Comment 19 Mason James 2015-09-03 09:42:43 UTC
(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
Comment 20 Bob Birchall 2015-11-27 04:29:19 UTC
Given Mason's comments, does this still need to be in Failed QA?
Comment 21 Kyle M Hall 2015-12-01 16:02:52 UTC
(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?
Comment 22 Mason James 2015-12-14 03:07:57 UTC
(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
Comment 23 Mason James 2015-12-14 03:11:03 UTC
(In reply to Bob Birchall from comment #20)
> Given Mason's comments, does this still need to be in Failed QA?

any update Kyle?
Comment 24 Kyle M Hall 2015-12-14 13:09:24 UTC
> 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!
Comment 25 Kyle M Hall 2015-12-14 13:11:47 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2015-12-14 13:12:18 UTC Comment hidden (obsolete)
Comment 27 Kyle M Hall 2015-12-14 13:12:25 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2015-12-14 13:12:27 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2015-12-14 13:12:30 UTC Comment hidden (obsolete)
Comment 30 Marcel de Rooy 2016-02-26 08:38:28 UTC
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.
Comment 31 Marcel de Rooy 2016-02-26 09:38:25 UTC Comment hidden (obsolete)
Comment 32 Marcel de Rooy 2016-02-26 09:38:30 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2016-02-26 09:38:34 UTC Comment hidden (obsolete)
Comment 34 Marcel de Rooy 2016-02-26 09:38:38 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2016-02-26 09:41:04 UTC
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.
Comment 36 Marcel de Rooy 2016-02-26 09:47:32 UTC Comment hidden (obsolete)
Comment 37 Marcel de Rooy 2016-02-26 09:47:36 UTC Comment hidden (obsolete)
Comment 38 Marcel de Rooy 2016-02-26 09:49:28 UTC Comment hidden (obsolete)
Comment 39 Marcel de Rooy 2016-02-26 09:49:32 UTC Comment hidden (obsolete)
Comment 40 Marcel de Rooy 2016-02-26 09:50:31 UTC
And some advanced wrestling with utf8 characters in the commit message (and the git version on Debian 7)
Comment 41 Marcel de Rooy 2016-02-26 10:58:04 UTC
Will continue QA session here later.
Comment 42 Marcel de Rooy 2016-02-29 08:08:01 UTC
(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.
Comment 43 Marcel de Rooy 2016-03-04 10:16:43 UTC
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>
Comment 44 Marcel de Rooy 2016-03-04 10:16:49 UTC
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>
Comment 45 Marcel de Rooy 2016-03-04 10:16:55 UTC
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>
Comment 46 Marcel de Rooy 2016-03-04 10:17:01 UTC
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.
Comment 47 Marcel de Rooy 2016-03-04 10:17:06 UTC
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.
Comment 48 Marcel de Rooy 2016-03-04 10:20:38 UTC
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
Comment 49 Marcel de Rooy 2016-03-04 10:31:00 UTC
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>
Comment 50 Brendan Gallagher 2016-03-07 17:28:14 UTC
Pushed to Master - Should be in the May 2016 release. Thanks!
Comment 51 Julian Maurice 2016-04-08 07:56:10 UTC
Patches pushed to 3.22.x, will be in 3.22.6
Comment 52 Nick Clemens 2016-05-09 15:00:15 UTC
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?
Comment 53 Katrin Fischer 2016-05-09 17:09:51 UTC
Hi Nick, can you please open a new bug linked to this one?
Comment 54 Mason James 2016-05-10 20:14:57 UTC
(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?)
Comment 55 Marcel de Rooy 2016-05-11 11:35:35 UTC
(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?
Comment 56 Kyle M Hall 2016-05-11 12:16:18 UTC
> 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.
Comment 57 Marcel de Rooy 2016-05-11 12:27:53 UTC
(In reply to Kyle M Hall from comment #56)
> Yes, SIP2 can have multiple AF fields. 
In that case that is the best route.
Comment 58 Marcel de Rooy 2016-05-11 12:40:13 UTC
(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.
Comment 59 Katrin Fischer 2016-05-11 12:43:04 UTC
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.
Comment 60 Marcel de Rooy 2016-05-11 12:49:28 UTC Comment hidden (obsolete)
Comment 61 Marcel de Rooy 2016-05-11 12:50:14 UTC
(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?
Comment 62 Marcel de Rooy 2016-05-11 12:51:35 UTC
@RM: Handled as QA follow-up. Please check last patch and push that one only.
Comment 63 Katrin Fischer 2016-05-11 12:53:44 UTC
I agree - fixing it is better.
Comment 64 Marcel de Rooy 2016-05-11 12:55:35 UTC
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.
Comment 65 Kyle M Hall 2016-05-11 13:04:27 UTC
Thanks Marcel! Follow-up has been pushed to master for 16.05.
Comment 66 Nick Clemens 2016-05-11 15:38:36 UTC
*** Bug 16479 has been marked as a duplicate of this bug. ***
Comment 67 Julian Maurice 2016-05-16 12:58:00 UTC
Follow-up pushed to 3.22.x, will be in 3.22.7