14629 – Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher

Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher

Summary: Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher

Status:	CLOSED FIXED

Alias:	None

Product:	Koha
Classification:	Unclassified
Component:	Cataloging (show other bugs)
Version:	Main
Hardware:	All All

Importance:	P5 - low enhancement
Assignee:	Nick Clemens (kidclamp)
QA Contact:	Marcel de Rooy

URL:
Keywords:	dependency

Depends on:
Blocks:

Reported:	2015-07-31 18:45 UTC by Kyle M Hall
Modified:	2018-06-04 20:10 UTC (History)
CC List:	7 users (show)

See Also:
Change sponsored?:	Sponsored
Patch complexity:	Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:	This patch adds a syspref "AggressiveMatchOnISSN" allowing for a match of ISSNs with or without hyphens. It uses Business::ISSN in order to follow the use of Business::ISBN and allow for validation of ISSNs. It is equivalent to the aggressive ISBN matcher.
Version(s) released in:
Circulation function:

Attachments
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher (9.01 KB, patch) 2015-12-30 23:22 UTC, Nick Clemens (kidclamp)	Details \| Diff \| Splinter Review
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN (2.39 KB, patch) 2015-12-31 16:07 UTC, Nick Clemens (kidclamp)	Details \| Diff \| Splinter Review
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher (9.02 KB, patch) 2016-01-08 15:58 UTC, Nick Clemens (kidclamp)	Details \| Diff \| Splinter Review
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher (9.62 KB, patch) 2016-03-01 13:53 UTC, Nick Clemens (kidclamp)	Details \| Diff \| Splinter Review
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN (3.56 KB, patch) 2016-03-01 13:53 UTC, Nick Clemens (kidclamp)	Details \| Diff \| Splinter Review
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher (9.62 KB, patch) 2016-03-01 14:24 UTC, Nick Clemens (kidclamp)	Details \| Diff \| Splinter Review
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN (3.30 KB, patch) 2016-03-01 14:24 UTC, Nick Clemens (kidclamp)	Details \| Diff \| Splinter Review
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher (8.78 KB, patch) 2016-03-08 16:08 UTC, Nick Clemens (kidclamp)	Details \| Diff \| Splinter Review
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN (2.39 KB, patch) 2016-03-08 16:08 UTC, Nick Clemens (kidclamp)	Details \| Diff \| Splinter Review
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher (8.95 KB, patch) 2016-04-29 17:38 UTC, Nicole C. Engard	Details \| Diff \| Splinter Review
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN (2.44 KB, patch) 2016-04-29 17:38 UTC, Nicole C. Engard	Details \| Diff \| Splinter Review
Bug 14629 - (QA Followup) sysprefs.sql changes (1.64 KB, patch) 2016-08-10 18:54 UTC, Nick Clemens (kidclamp)	Details \| Diff \| Splinter Review
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher (9.05 KB, patch) 2016-10-28 10:35 UTC, Marcel de Rooy	Details \| Diff \| Splinter Review
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN (2.54 KB, patch) 2016-10-28 10:35 UTC, Marcel de Rooy	Details \| Diff \| Splinter Review
Bug 14629 - (QA Followup) sysprefs.sql changes (1.74 KB, patch) 2016-10-28 10:36 UTC, Marcel de Rooy	Details \| Diff \| Splinter Review
Bug 14629: [QA Follow-up] Small tidy up (1.30 KB, patch) 2016-10-28 10:36 UTC, Marcel de Rooy	Details \| Diff \| Splinter Review
Show Obsolete (12) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Kyle M Hall 2015-07-31 18:45:12 UTC

There should be an AggressiveMatchOnISSN feature just like there is already AggressiveMatchOnISBN.

Comment 1 Nick Clemens (kidclamp) 2015-12-30 23:22:42 UTC Comment hidden (obsolete)

Created attachment 46102 [details] [review]
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher

This patch adds a syspref "AggressiveMatchOnISSN" allowing for a match
of ISSNs with or without hyphens. It uses Business::ISSN in order to
follow the use of Business::ISBN and allow for validation of ISSNs

To test:
1 - Find a record in your system with an ISSN (or add one)
2 - Stage a record containing the same ISSN but lacking a hyphen
3 - Matching on ISSN should find 0 matches
4 - Repeat with no hyphen ISSN in system and hyphen ISSN in import
5 - Matching should find 0
6 - Apply patch
7 - Update datbase and install Business::ISSN
8 - Leave AggressiveMatchOnISSN as don't and repeat original tests- no
change
9 - Set AggressiveMatchOnISSN as do and repeat original test
10 - You should find a match
11 - prove t/Koha.t - all tests pass

Comment 2 Nick Clemens (kidclamp) 2015-12-31 16:07:58 UTC Comment hidden (obsolete)

Created attachment 46159 [details] [review]
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN

This is an optional follow up depending on community opinion.

While copying over the code form the ISBN portion I noticed that with
aggressive matching enabled invalid ISBNs (and hence ISSNs) were being
stripped from the record.

I think in the case of a library exporting records, making changes, and
reimporting they would expect to get a match on ISSN or ISBN whether or
not the number is valid.

This patch changes the subroutine to return the original ISSN in the
case of it being invalid.

To test:
With first patch only export a record with an invalid ISSN and reimport
with AggressiveMatchOnISSN enabled and match on ISSN - you should not get a match
Apply this patch
reimport the file and you should find a match.

Comment 3 Nick Clemens (kidclamp) 2016-01-08 15:58:30 UTC Comment hidden (obsolete)

Created attachment 46440 [details] [review]
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher

This patch adds a syspref "AggressiveMatchOnISSN" allowing for a match
of ISSNs with or without hyphens. It uses Business::ISSN in order to
follow the use of Business::ISBN and allow for validation of ISSNs

To test:
1 - Find a record in your system with an ISSN (or add one)
2 - Stage a record containing the same ISSN but lacking a hyphen
3 - Matching on ISSN should find 0 matches
4 - Repeat with no hyphen ISSN in system and hyphen ISSN in import
5 - Matching should find 0
6 - Apply patch
7 - Update datbase and install Business::ISSN
8 - Leave AggressiveMatchOnISSN as don't and repeat original tests- no
change
9 - Set AggressiveMatchOnISSN as do and repeat original test
10 - You should find a match
11 - prove t/Koha.t - all tests pass

Sponsored by North Central Regional Library System (NCRL) www.ncrl.org

Comment 4 Nick Clemens (kidclamp) 2016-03-01 13:53:27 UTC Comment hidden (obsolete)

Created attachment 48509 [details] [review]
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher

This patch adds a syspref "AggressiveMatchOnISSN" allowing for a match
of ISSNs with or without hyphens. It uses Business::ISSN in order to
follow the use of Business::ISBN and allow for validation of ISSNs

To test:
1 - Find a record in your system with an ISSN (or add one)
2 - Stage a record containing the same ISSN but lacking a hyphen
3 - Matching on ISSN should find 0 matches
4 - Repeat with no hyphen ISSN in system and hyphen ISSN in import
5 - Matching should find 0
6 - Apply patch
7 - Update datbase and install Business::ISSN
8 - Leave AggressiveMatchOnISSN as don't and repeat original tests- no
change
9 - Set AggressiveMatchOnISSN as do and repeat original test
10 - You should find a match
11 - prove t/Koha.t - all tests pass

Sponsored by North Central Regional Library System (NCRL) www.ncrl.org

Comment 5 Nick Clemens (kidclamp) 2016-03-01 13:53:31 UTC Comment hidden (obsolete)

Created attachment 48510 [details] [review]
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN

This is an optional follow up depending on community opinion.

While copying over the code form the ISBN portion I noticed that with
aggressive matching enabled invalid ISBNs (and hence ISSNs) were being
stripped from the record.

I think in the case of a library exporting records, making changes, and
reimporting they would expect to get a match on ISSN or ISBN whether or
not the number is valid.

This patch changes the subroutine to return the original ISSN in the
case of it being invalid.

To test:
With first patch only export a record with an invalid ISSN and reimport
with AggressiveMatchOnISSN enabled and match on ISSN - you should not get a match
Apply this patch
reimport the file and you should find a match.

Comment 6 Nick Clemens (kidclamp) 2016-03-01 14:24:50 UTC Comment hidden (obsolete)

Created attachment 48513 [details] [review]
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher

This patch adds a syspref "AggressiveMatchOnISSN" allowing for a match
of ISSNs with or without hyphens. It uses Business::ISSN in order to
follow the use of Business::ISBN and allow for validation of ISSNs

To test:
1 - Find a record in your system with an ISSN (or add one)
2 - Stage a record containing the same ISSN but lacking a hyphen
3 - Matching on ISSN should find 0 matches
4 - Repeat with no hyphen ISSN in system and hyphen ISSN in import
5 - Matching should find 0
6 - Apply patch
7 - Update datbase and install Business::ISSN
8 - Leave AggressiveMatchOnISSN as don't and repeat original tests- no
change
9 - Set AggressiveMatchOnISSN as do and repeat original test
10 - You should find a match
11 - prove t/Koha.t - all tests pass

Sponsored by North Central Regional Library System (NCRL) www.ncrl.org

Comment 7 Nick Clemens (kidclamp) 2016-03-01 14:24:55 UTC Comment hidden (obsolete)

Created attachment 48514 [details] [review]
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN

This is an optional follow up depending on community opinion.

While copying over the code form the ISBN portion I noticed that with
aggressive matching enabled invalid ISBNs (and hence ISSNs) were being
stripped from the record.

I think in the case of a library exporting records, making changes, and
reimporting they would expect to get a match on ISSN or ISBN whether or
not the number is valid.

This patch changes the subroutine to return the original ISSN in the
case of it being invalid.

To test:
With first patch only export a record with an invalid ISSN and reimport
with AggressiveMatchOnISSN enabled and match on ISSN - you should not get a match
Apply this patch
reimport the file and you should find a match.

Comment 8 Nick Clemens (kidclamp) 2016-03-08 16:08:27 UTC Comment hidden (obsolete)

Created attachment 48793 [details] [review]
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher

This patch adds a syspref "AggressiveMatchOnISSN" allowing for a match
of ISSNs with or without hyphens. It uses Business::ISSN in order to
follow the use of Business::ISBN and allow for validation of ISSNs

To test:
1 - Find a record in your system with an ISSN (or add one)
2 - Stage a record containing the same ISSN but lacking a hyphen
3 - Matching on ISSN should find 0 matches
4 - Repeat with no hyphen ISSN in system and hyphen ISSN in import
5 - Matching should find 0
6 - Apply patch
7 - Update datbase and install Business::ISSN
8 - Leave AggressiveMatchOnISSN as don't and repeat original tests- no
change
9 - Set AggressiveMatchOnISSN as do and repeat original test
10 - You should find a match
11 - prove t/Koha.t - all tests pass

Sponsored by North Central Regional Library System (NCRL) www.ncrl.org

Comment 9 Nick Clemens (kidclamp) 2016-03-08 16:08:32 UTC Comment hidden (obsolete)

Created attachment 48794 [details] [review]
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN

This is an optional follow up depending on community opinion.

While copying over the code form the ISBN portion I noticed that with
aggressive matching enabled invalid ISBNs (and hence ISSNs) were being
stripped from the record.

I think in the case of a library exporting records, making changes, and
reimporting they would expect to get a match on ISSN or ISBN whether or
not the number is valid.

This patch changes the subroutine to return the original ISSN in the
case of it being invalid.

To test:
With first patch only export a record with an invalid ISSN and reimport
with AggressiveMatchOnISSN enabled and match on ISSN - you should not get a match
Apply this patch
reimport the file and you should find a match.

Comment 10 Nicole C. Engard 2016-04-29 17:38:49 UTC

Created attachment 51028 [details] [review]
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher

This patch adds a syspref "AggressiveMatchOnISSN" allowing for a match
of ISSNs with or without hyphens. It uses Business::ISSN in order to
follow the use of Business::ISBN and allow for validation of ISSNs

To test:
1 - Find a record in your system with an ISSN (or add one)
2 - Stage a record containing the same ISSN but lacking a hyphen
3 - Matching on ISSN should find 0 matches
4 - Repeat with no hyphen ISSN in system and hyphen ISSN in import
5 - Matching should find 0
6 - Apply patch
7 - Update datbase and install Business::ISSN
8 - Leave AggressiveMatchOnISSN as don't and repeat original tests- no
change
9 - Set AggressiveMatchOnISSN as do and repeat original test
10 - You should find a match
11 - prove t/Koha.t - all tests pass

Sponsored by North Central Regional Library System (NCRL) www.ncrl.org

Signed-off-by: Chad Roseburg <croseburg@ncrl.org>

Comment 11 Nicole C. Engard 2016-04-29 17:38:55 UTC

Created attachment 51029 [details] [review]
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN

This is an optional follow up depending on community opinion.

While copying over the code form the ISBN portion I noticed that with
aggressive matching enabled invalid ISBNs (and hence ISSNs) were being
stripped from the record.

I think in the case of a library exporting records, making changes, and
reimporting they would expect to get a match on ISSN or ISBN whether or
not the number is valid.

This patch changes the subroutine to return the original ISSN in the
case of it being invalid.

To test:
With first patch only export a record with an invalid ISSN and reimport
with AggressiveMatchOnISSN enabled and match on ISSN - you should not get a match
Apply this patch
reimport the file and you should find a match.

Signed-off-by: Chad Roseburg <croseburg@ncrl.org>

Comment 12 Jonathan Druart 2016-08-01 15:46:58 UTC

Changes to sysprefs.sql are missing.

Comment 13 Nick Clemens (kidclamp) 2016-08-10 18:54:51 UTC

Created attachment 54280 [details] [review]
Bug 14629 - (QA Followup) sysprefs.sql changes

Comment 14 Jonathan Druart 2016-08-11 10:08:01 UTC

libbusiness-issn-perl is only packaged for jessie, adding keyword nowheezy.

Comment 15 Jonathan Druart 2016-08-11 10:11:37 UTC

(In reply to Jonathan Druart from comment #14)
> libbusiness-issn-perl is only packaged for jessie, adding keyword nowheezy.

Forget that, it is also packaged for wheezy.

Comment 16 Jonathan Druart 2016-08-11 10:23:12 UTC

(In reply to Nicole C. Engard from comment #11)
> Created attachment 51029 [details] [review] [review]
> Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN

Should not we do that for GetVariationsOfISBN as well?

Comment 17 Nick Clemens (kidclamp) 2016-08-11 15:02:15 UTC

(In reply to Jonathan Druart from comment #16)
> (In reply to Nicole C. Engard from comment #11)
> > Created attachment 51029 [details] [review] [review] [review]
> > Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN
> 
> Should not we do that for GetVariationsOfISBN as well?

If we do think this is the right way to go I will file a new bug for ISBN, I just wanted to see if there was any opposing opinion here before doing that.

Comment 18 Katrin Fischer 2016-08-28 23:19:47 UTC

Adding Mirko as this adds a new dependency.

Comment 19 Mirko Tietgen 2016-08-30 18:16:54 UTC

Dependency is in Wheezy and Jessie, we're good to go from my side. I'll update the control file once this is pushed.

Comment 20 Marcel de Rooy 2016-10-28 08:26:53 UTC

QA: Looking here now

Comment 21 Marcel de Rooy 2016-10-28 10:35:54 UTC

Created attachment 56954 [details] [review]
Bug 14629 - Add aggressive ISSN matching feature equivalent to the aggressive ISBN matcher

This patch adds a syspref "AggressiveMatchOnISSN" allowing for a match
of ISSNs with or without hyphens. It uses Business::ISSN in order to
follow the use of Business::ISBN and allow for validation of ISSNs

To test:
1 - Find a record in your system with an ISSN (or add one)
2 - Stage a record containing the same ISSN but lacking a hyphen
3 - Matching on ISSN should find 0 matches
4 - Repeat with no hyphen ISSN in system and hyphen ISSN in import
5 - Matching should find 0
6 - Apply patch
7 - Update datbase and install Business::ISSN
8 - Leave AggressiveMatchOnISSN as don't and repeat original tests- no
change
9 - Set AggressiveMatchOnISSN as do and repeat original test
10 - You should find a match
11 - prove t/Koha.t - all tests pass

Sponsored by North Central Regional Library System (NCRL) www.ncrl.org

Signed-off-by: Chad Roseburg <croseburg@ncrl.org>

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

Comment 22 Marcel de Rooy 2016-10-28 10:35:59 UTC

Created attachment 56955 [details] [review]
Bug 14629 - [QA Followup] Pass invalid ISSN through GetVariationsOfISSN

This is an optional follow up depending on community opinion.

While copying over the code form the ISBN portion I noticed that with
aggressive matching enabled invalid ISBNs (and hence ISSNs) were being
stripped from the record.

I think in the case of a library exporting records, making changes, and
reimporting they would expect to get a match on ISSN or ISBN whether or
not the number is valid.

This patch changes the subroutine to return the original ISSN in the
case of it being invalid.

To test:
With first patch only export a record with an invalid ISSN and reimport
with AggressiveMatchOnISSN enabled and match on ISSN - you should not get a match
Apply this patch
reimport the file and you should find a match.

Signed-off-by: Chad Roseburg <croseburg@ncrl.org>

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

Comment 23 Marcel de Rooy 2016-10-28 10:36:04 UTC

Created attachment 56956 [details] [review]
Bug 14629 - (QA Followup) sysprefs.sql changes

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

Comment 24 Marcel de Rooy 2016-10-28 10:36:08 UTC

Created attachment 56957 [details] [review]
Bug 14629: [QA Follow-up] Small tidy up

Some cosmetic changes as to whitespace and one call less to Normalize.

Test plan:
Run t/Koha.t

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

Comment 25 Marcel de Rooy 2016-10-28 10:44:04 UTC

Looks good to me.
Few small remarks.

As mentioned, would be good to keep consistency between ISBN -- ISSN stuff.
Returning the invalid one for ISSN will not lead to much discussion, since this is just new. I prefer it. ISBN side is not new, might raise discussion..

Using the private routine _issn is not recommended.

Too bad this has a relation with the unfinished QueryParser..

I also tested with 1234-5679 (some text)
You could also imagine importing 1234-5679 (text) while only having 1234-5679 in the database? Should this match or not? Depends on your length set in the rule too.

Passed QA

Comment 26 Kyle M Hall (khall) 2016-10-28 12:00:59 UTC

Pushed to master for 16.11, thanks Nick, Marcel!

Comment 27 Mirko Tietgen 2016-10-29 03:34:28 UTC

Bug 17084 now has the control file update for the dependency.