Bug 17703 - Always export ISSN in serials claims issues CSV
Summary: Always export ISSN in serials claims issues CSV
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin Somers
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on: 11861
Blocks:
  Show dependency treegraph
 
Reported: 2016-11-30 13:32 UTC by Fridolin Somers
Modified: 2022-02-19 19:00 UTC (History)
4 users (show)

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


Attachments
Bug 17703 - Add ISSN column in serials claims table - fix (1.79 KB, patch)
2016-11-30 13:37 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 17703 - Add ISSN column in serials claims table - fix (1.87 KB, patch)
2017-08-03 08:44 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 17703 - Add ISSN column in serials claims table - fix (6.12 KB, patch)
2017-08-03 09:57 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED OFF] Bug 17703 - Add ISSN column in serials claims table - fix (6.19 KB, patch)
2017-08-17 20:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17703: Always add ISSN in serials claims issues (6.13 KB, patch)
2020-03-27 12:36 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 17703: add unit test (2.34 KB, patch)
2020-03-27 12:48 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 17703: Always add ISSN in serials claims issues (6.13 KB, patch)
2020-03-27 12:49 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 17703: Add unit tests (2.34 KB, patch)
2020-03-27 12:52 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 17703: Always add ISSN in serials claims issues (6.13 KB, patch)
2020-03-27 12:52 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 17703: Add unit tests (2.39 KB, patch)
2020-04-01 09:49 UTC, David Nind
Details | Diff | Splinter Review
Bug 17703: Always add ISSN in serials claims issues (6.19 KB, patch)
2020-04-01 09:49 UTC, David Nind
Details | Diff | Splinter Review
Bug 17703: Add unit tests (2.29 KB, patch)
2021-04-12 09:41 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 17703: Add unit tests (2.29 KB, patch)
2021-04-12 09:43 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 17703: Always add ISSN in serials claims issues (6.18 KB, patch)
2021-04-12 09:43 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2016-11-30 13:32:56 UTC
Bug 11861 added ISSN column in serials claims table, in a SQL query in C4::Serials::GetLateOrMissingIssues().
But this SQL query is in a if condition, the same change must be done in the else case.
Comment 1 Fridolin Somers 2016-11-30 13:37:30 UTC
Created attachment 57832 [details] [review]
Bug 17703 - Add ISSN column in serials claims table - fix

Bug 11861 added ISSN column in serials claims table, in a SQL query in C4::Serials::GetLateOrMissingIssues().
But this SQL query is in a if condition, the same change must be done in the else case.

Test plan:
1/ Go to serials claims page
2/ Do not choose a vendor and submit
3/ You should see a ISSN column, filled with subscription ISSN
Comment 2 Katrin Fischer 2017-01-07 22:43:50 UTC
Hi Fridolin,

can you check your test plan?

I have tested, but didn't manage to not have the ISSN column filled on claim serials page. In your test plan you say:

2/ Do not choose a vendor and submit

But this is not possible int he GUI - there is no empyt entry in the pull down and an entry there only appears for a vendor with late serial issues.
Comment 3 Fridolin Somers 2017-08-03 08:44:11 UTC
Created attachment 65454 [details] [review]
Bug 17703 - Add ISSN column in serials claims table - fix

Bug 11861 added ISSN column in serials claims table, in a SQL query in C4::Serials::GetLateOrMissingIssues().
But this SQL query is in a if condition, the same change must be done in the else case.

Test plan:
- Make sure you have a subscription with some late issues
- Go to serials claims page /cgi-bin/koha/serials/claims.pl
- Choose a vendor and click OK
=> You should see a ISSN column, filled with subscription ISSN
Comment 4 Fridolin Somers 2017-08-03 08:57:41 UTC
Ahhhhh oups no this change is needed for CSV export.
I rewrite test plan
Comment 5 Fridolin Somers 2017-08-03 09:56:17 UTC
(In reply to Fridolin SOMERS from comment #4)
> Ahhhhh oups no this change is needed for CSV export.
> I rewrite test plan

I even rewrite method to use common SQL query with only condition on WHERE.
Comment 6 Fridolin Somers 2017-08-03 09:57:06 UTC
Created attachment 65455 [details] [review]
Bug 17703 - Add ISSN column in serials claims table - fix

Bug 11861 added ISSN column in serials claims table and a SQL query in C4::Serials::GetLateOrMissingIssues().
But this SQL query is in a if $supplierid condition, the same change must be done if $serialid to allow ISSN in CVS export of claims.

This patch rewrites the method to have a common SQL query with conditions only on WHERE.
Removes the order input arg since it is never used, order in TT is managed by DataTable and CSV export is generated with the order of selected rows.

Test plan:
- Make sure you have a subscription with some late or missing issues
- Go to Tool > CSV profils
- Create a profile :
  Profile name = Issues to claim
  Profile type = SQL
  Usage = Late serial issues claims
  Profile SQL fields = SUPPLIER=aqbooksellers.name|TITLE=subscription.title|ISSN=subscription.issn|ISSUE NUMBER=serial.serialseq|LATE SINCE=serial.planneddate
- Go to serials claims page : /cgi-bin/koha/serials/claims.pl
- Choose a vendor and click OK
- You see serials to claim, with ISSN column filled with subscription's ISSN
- Check all lines
- Select "Issues to claim" CSV profile
- Click on "Download selected claims"
=> With patch you have ISSN value
- Run UT t/db_dependent/Serials/Claims.t
Comment 7 Katrin Fischer 2017-08-17 20:07:57 UTC
Created attachment 66150 [details] [review]
[SIGNED OFF] Bug 17703 - Add ISSN column in serials claims table - fix

Bug 11861 added ISSN column in serials claims table and a SQL query in C4::Serials::GetLateOrMissingIssues().
But this SQL query is in a if $supplierid condition, the same change must be done if $serialid to allow ISSN in CVS export of claims.

This patch rewrites the method to have a common SQL query with conditions only on WHERE.
Removes the order input arg since it is never used, order in TT is managed by DataTable and CSV export is generated with the order of selected rows.

Test plan:
- Make sure you have a subscription with some late or missing issues
- Go to Tool > CSV profils
- Create a profile :
  Profile name = Issues to claim
  Profile type = SQL
  Usage = Late serial issues claims
  Profile SQL fields = SUPPLIER=aqbooksellers.name|TITLE=subscription.title|ISSN=subscription.issn|ISSUE NUMBER=serial.serialseq|LATE SINCE=serial.planneddate
- Go to serials claims page : /cgi-bin/koha/serials/claims.pl
- Choose a vendor and click OK
- You see serials to claim, with ISSN column filled with subscription's ISSN
- Check all lines
- Select "Issues to claim" CSV profile
- Click on "Download selected claims"
=> With patch you have ISSN value
- Run UT t/db_dependent/Serials/Claims.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 8 Jonathan Druart 2017-08-30 17:52:06 UTC
please provide a test.
Comment 9 Fridolin Somers 2020-03-27 12:36:14 UTC
Created attachment 101994 [details] [review]
Bug 17703: Always add ISSN in serials claims issues

Bug 11861 added ISSN column in serials claims table and a SQL query in C4::Serials::GetLateOrMissingIssues().
But this SQL query is in a if $supplierid condition, the same change must be done if $serialid to allow ISSN in CVS export of claims.

This patch rewrites the method to have a common SQL query with conditions only on WHERE.
Removes the order input arg since it is never used, order in TT is managed by DataTable and CSV export is generated with the order of selected rows.

Test plan:
- Make sure you have a subscription with some late or missing issues
- Go to Tool > CSV profils
- Create a profile :
  Profile name = Issues to claim
  Profile type = SQL
  Usage = Late serial issues claims
  Profile SQL fields = SUPPLIER=aqbooksellers.name|TITLE=subscription.title|ISSN=subscription.issn|ISSUE NUMBER=serial.serialseq|LATE SINCE=serial.planneddate
- Go to serials claims page : /cgi-bin/koha/serials/claims.pl
- Choose a vendor and click OK
- You see serials to claim, with ISSN column filled with subscription's ISSN
- Check all lines
- Select "Issues to claim" CSV profile
- Click on "Download selected claims"
=> With patch you have ISSN value

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Fridolin Somers 2020-03-27 12:48:17 UTC
Created attachment 101996 [details] [review]
Bug 17703: add unit test

Run : prove t/db_dependent/Serials/Claims.t
Comment 11 Fridolin Somers 2020-03-27 12:49:10 UTC
Created attachment 101997 [details] [review]
Bug 17703: Always add ISSN in serials claims issues

Bug 11861 added ISSN column in serials claims table and a SQL query in C4::Serials::GetLateOrMissingIssues().
But this SQL query is in a if $supplierid condition, the same change must be done if $serialid to allow ISSN in CVS export of claims.

This patch rewrites the method to have a common SQL query with conditions only on WHERE.
Removes the order input arg since it is never used, order in TT is managed by DataTable and CSV export is generated with the order of selected rows.

Test plan:
- Make sure you have a subscription with some late or missing issues
- Go to Tool > CSV profils
- Create a profile :
  Profile name = Issues to claim
  Profile type = SQL
  Usage = Late serial issues claims
  Profile SQL fields = SUPPLIER=aqbooksellers.name|TITLE=subscription.title|ISSN=subscription.issn|ISSUE NUMBER=serial.serialseq|LATE SINCE=serial.planneddate
- Go to serials claims page : /cgi-bin/koha/serials/claims.pl
- Choose a vendor and click OK
- You see serials to claim, with ISSN column filled with subscription's ISSN
- Check all lines
- Select "Issues to claim" CSV profile
- Click on "Download selected claims"
=> With patch you have ISSN value

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Fridolin Somers 2020-03-27 12:52:38 UTC
Created attachment 102000 [details] [review]
Bug 17703: Add unit tests

Run : prove t/db_dependent/Serials/Claims.t
Comment 13 Fridolin Somers 2020-03-27 12:52:44 UTC
Created attachment 102001 [details] [review]
Bug 17703: Always add ISSN in serials claims issues

Bug 11861 added ISSN column in serials claims table and a SQL query in C4::Serials::GetLateOrMissingIssues().
But this SQL query is in a if $supplierid condition, the same change must be done if $serialid to allow ISSN in CVS export of claims.

This patch rewrites the method to have a common SQL query with conditions only on WHERE.
Removes the order input arg since it is never used, order in TT is managed by DataTable and CSV export is generated with the order of selected rows.

Test plan:
- Make sure you have a subscription with some late or missing issues
- Go to Tool > CSV profils
- Create a profile :
  Profile name = Issues to claim
  Profile type = SQL
  Usage = Late serial issues claims
  Profile SQL fields = SUPPLIER=aqbooksellers.name|TITLE=subscription.title|ISSN=subscription.issn|ISSUE NUMBER=serial.serialseq|LATE SINCE=serial.planneddate
- Go to serials claims page : /cgi-bin/koha/serials/claims.pl
- Choose a vendor and click OK
- You see serials to claim, with ISSN column filled with subscription's ISSN
- Check all lines
- Select "Issues to claim" CSV profile
- Click on "Download selected claims"
=> With patch you have ISSN value

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Fridolin Somers 2020-03-27 12:54:25 UTC
(In reply to Jonathan Druart from comment #8)
> please provide a test.

Here it is.
Comment 15 David Nind 2020-04-01 09:49:11 UTC
Created attachment 102197 [details] [review]
Bug 17703: Add unit tests

Run : prove t/db_dependent/Serials/Claims.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 16 David Nind 2020-04-01 09:49:15 UTC
Created attachment 102199 [details] [review]
Bug 17703: Always add ISSN in serials claims issues

Bug 11861 added ISSN column in serials claims table and a SQL query in C4::Serials::GetLateOrMissingIssues().
But this SQL query is in a if $supplierid condition, the same change must be done if $serialid to allow ISSN in CVS export of claims.

This patch rewrites the method to have a common SQL query with conditions only on WHERE.
Removes the order input arg since it is never used, order in TT is managed by DataTable and CSV export is generated with the order of selected rows.

Test plan:
- Make sure you have a subscription with some late or missing issues
- Go to Tool > CSV profils
- Create a profile :
  Profile name = Issues to claim
  Profile type = SQL
  Usage = Late serial issues claims
  Profile SQL fields = SUPPLIER=aqbooksellers.name|TITLE=subscription.title|ISSN=subscription.issn|ISSUE NUMBER=serial.serialseq|LATE SINCE=serial.planneddate
- Go to serials claims page : /cgi-bin/koha/serials/claims.pl
- Choose a vendor and click OK
- You see serials to claim, with ISSN column filled with subscription's ISSN
- Check all lines
- Select "Issues to claim" CSV profile
- Click on "Download selected claims"
=> With patch you have ISSN value

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Jonathan Druart 2020-04-08 07:52:38 UTC
Tests are failing for me.

t/db_dependent/Serials/Claims.t .. 2/20
#   Failed test 'subscription 1 should have 1 issue in late'
#   at t/db_dependent/Serials/Claims.t line 172.
#          got: '0'
#     expected: '1'

#   Failed test 'return of GetLateOrMissingIssues() contains biblioitems.issn'
#   at t/db_dependent/Serials/Claims.t line 173.
#          got: undef
#     expected: '123456789'
# Looks like you failed 2 tests of 20.
Comment 18 Fridolin Somers 2021-04-12 09:41:42 UTC
Created attachment 119444 [details] [review]
Bug 17703: Add unit tests

Run : prove t/db_dependent/Serials/Claims.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 Fridolin Somers 2021-04-12 09:43:18 UTC
Created attachment 119445 [details] [review]
Bug 17703: Add unit tests

Run : prove t/db_dependent/Serials/Claims.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 Fridolin Somers 2021-04-12 09:43:51 UTC
Created attachment 119446 [details] [review]
Bug 17703: Always add ISSN in serials claims issues

Bug 11861 added ISSN column in serials claims table and a SQL query in C4::Serials::GetLateOrMissingIssues().
But this SQL query is in a if $supplierid condition, the same change must be done if $serialid to allow ISSN in CVS export of claims.

This patch rewrites the method to have a common SQL query with conditions only on WHERE.
Removes the order input arg since it is never used, order in TT is managed by DataTable and CSV export is generated with the order of selected rows.

Test plan:
- Make sure you have a subscription with some late or missing issues
- Go to Tool > CSV profils
- Create a profile :
  Profile name = Issues to claim
  Profile type = SQL
  Usage = Late serial issues claims
  Profile SQL fields = SUPPLIER=aqbooksellers.name|TITLE=subscription.title|ISSN=subscription.issn|ISSUE NUMBER=serial.serialseq|LATE SINCE=serial.planneddate
- Go to serials claims page : /cgi-bin/koha/serials/claims.pl
- Choose a vendor and click OK
- You see serials to claim, with ISSN column filled with subscription's ISSN
- Check all lines
- Select "Issues to claim" CSV profile
- Click on "Download selected claims"
=> With patch you have ISSN value

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 Fridolin Somers 2021-04-12 09:46:27 UTC
I've removed from unit test :
+use C4::Serials;

This is already managed by :
 use_ok('C4::Serials');
Comment 22 Fridolin Somers 2021-04-12 09:46:58 UTC
(In reply to Jonathan Druart from comment #17)
> Tests are failing for me.
> 
> t/db_dependent/Serials/Claims.t .. 2/20
> #   Failed test 'subscription 1 should have 1 issue in late'
> #   at t/db_dependent/Serials/Claims.t line 172.
> #          got: '0'
> #     expected: '1'
> 
> #   Failed test 'return of GetLateOrMissingIssues() contains
> biblioitems.issn'
> #   at t/db_dependent/Serials/Claims.t line 173.
> #          got: undef
> #     expected: '123456789'
> # Looks like you failed 2 tests of 20.

Works for me, can you retest please ?
Comment 23 Jonathan Druart 2021-04-12 10:25:42 UTC
Still failing, same errors.
Comment 24 Fridolin Somers 2021-04-12 10:32:47 UTC
> Failed test 'subscription 1 should have 1 issue in late'
Stange, my patch does not impact this test o_O
Comment 25 Jonathan Druart 2021-04-13 09:09:46 UTC
Hum, retested today and the tests pass on a fresh install.
Comment 26 Kyle M Hall 2021-04-23 13:22:15 UTC
root@kohadevbox:koha(bug17703-qa)$ prove t/db_dependent/Serials/Claims.t
t/db_dependent/Serials/Claims.t .. 16/20
#   Failed test 'subscription 1 should have 1 issue in late'
#   at t/db_dependent/Serials/Claims.t line 171.
#          got: '0'
#     expected: '1'

#   Failed test 'return of GetLateOrMissingIssues() contains biblioitems.issn'
#   at t/db_dependent/Serials/Claims.t line 172.
#          got: undef
#     expected: '123456789'
# Looks like you failed 2 tests of 20.