Bug 17836 - (ILSDI) 'charges' always '1'
Summary: (ILSDI) 'charges' always '1'
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-03 16:09 UTC by Tomás Cohen Arazi
Modified: 2017-12-07 22:15 UTC (History)
5 users (show)

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


Attachments
Bug 17836: (ILSDI) Regression test (1.68 KB, patch)
2017-01-03 16:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17836: (ILSDI) Make GetPatronInfo fill 'charges' correctly (1.36 KB, patch)
2017-01-03 16:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17836: (ILSDI) Regression test (1.73 KB, patch)
2017-01-03 17:36 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17836: (ILSDI) Make GetPatronInfo fill 'charges' correctly (1.41 KB, patch)
2017-01-03 17:37 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17836: (ILSDI) Make GetPatronInfo fill 'charges' correctly (1.46 KB, patch)
2017-01-13 20:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17836: (ILSDI) Regression test (1.79 KB, patch)
2017-01-13 20:23 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2017-01-03 16:09:10 UTC
Due to a typo, the 'charges' attribute will always return 1.
Comment 1 Tomás Cohen Arazi 2017-01-03 16:27:30 UTC
Created attachment 58568 [details] [review]
Bug 17836: (ILSDI) Regression test

Sponsored-by: ByWater Solutions
Comment 2 Tomás Cohen Arazi 2017-01-03 16:28:33 UTC
Created attachment 58569 [details] [review]
Bug 17836: (ILSDI) Make GetPatronInfo fill 'charges' correctly

This trivial fix corrects a typo on C4/ILSDI/Services.pm.
It was hidden because the tests for ILSDI only cover the 'attributes'
portion of the response. I added regression tests for this.

To test:
- Have the regression test patch applied
- Run:
  $ prove t/db_dependent/ILSDI_Services.t
=> FAIL: Tests fail because 'charges' is always set to 1
- Apply the patch
- Run:
  $ prove t/db_dependent/ILSDI_Services.t
=> SUCCESS: Tests pass
- Sign off :-D

Sponsored-by: ByWater Solutions
Comment 3 Mark Tompsett 2017-01-03 17:34:37 UTC
Comment on attachment 58569 [details] [review]
Bug 17836: (ILSDI) Make GetPatronInfo fill 'charges' correctly

Review of attachment 58569 [details] [review]:
-----------------------------------------------------------------

::: C4/ILSDI/Services.pm
@@ -376,4 @@
>  
>      # Cleaning the borrower hashref
>      my $flags = C4::Members::patronflags( $borrower );
> -    $borrower->{'charges'}    = $flags>{'CHARGES'}->{'amount'};

OH MY! What a horrid typo.
Comment 4 Mark Tompsett 2017-01-03 17:36:50 UTC
Created attachment 58574 [details] [review]
Bug 17836: (ILSDI) Regression test

Sponsored-by: ByWater Solutions

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 5 Mark Tompsett 2017-01-03 17:37:02 UTC
Created attachment 58575 [details] [review]
Bug 17836: (ILSDI) Make GetPatronInfo fill 'charges' correctly

This trivial fix corrects a typo on C4/ILSDI/Services.pm.
It was hidden because the tests for ILSDI only cover the 'attributes'
portion of the response. I added regression tests for this.

To test:
- Have the regression test patch applied
- Run:
  $ prove t/db_dependent/ILSDI_Services.t
=> FAIL: Tests fail because 'charges' is always set to 1
- Apply the patch
- Run:
  $ prove t/db_dependent/ILSDI_Services.t
=> SUCCESS: Tests pass
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 6 Mark Tompsett 2017-01-03 17:38:11 UTC
Followed test plan in comment #5 and also ran koha-qa test tools.
Comment 7 Tomás Cohen Arazi 2017-01-03 18:03:16 UTC
(In reply to M. Tompsett from comment #3)
> Comment on attachment 58569 [details] [review] [review]
> Bug 17836: (ILSDI) Make GetPatronInfo fill 'charges' correctly
> 
> Review of attachment 58569 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/ILSDI/Services.pm
> @@ -376,4 @@
> >  
> >      # Cleaning the borrower hashref
> >      my $flags = C4::Members::patronflags( $borrower );
> > -    $borrower->{'charges'}    = $flags>{'CHARGES'}->{'amount'};
> 
> OH MY! What a horrid typo.

This should have been cough by our test suite, but we haven't reached ILSID yet in our attempt to improve coverage.
Comment 8 Nick Clemens 2017-01-13 20:23:08 UTC
Created attachment 58981 [details] [review]
Bug 17836: (ILSDI) Make GetPatronInfo fill 'charges' correctly

This trivial fix corrects a typo on C4/ILSDI/Services.pm.
It was hidden because the tests for ILSDI only cover the 'attributes'
portion of the response. I added regression tests for this.

To test:
- Have the regression test patch applied
- Run:
  $ prove t/db_dependent/ILSDI_Services.t
=> FAIL: Tests fail because 'charges' is always set to 1
- Apply the patch
- Run:
  $ prove t/db_dependent/ILSDI_Services.t
=> SUCCESS: Tests pass
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2017-01-13 20:23:28 UTC
Created attachment 58982 [details] [review]
Bug 17836: (ILSDI) Regression test

Sponsored-by: ByWater Solutions

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Kyle M Hall 2017-01-20 13:44:19 UTC
Pushed to master for 17.05, thanks Tomas!
Comment 11 Katrin Fischer 2017-01-22 19:53:17 UTC
Error doesn't exist in 16.11.x - it looks like it has been introduced by 
http://git.koha-community.org/gitweb/?p=koha.git;a=commitdiff;h=41493004f69caf471e8d73b7db2a64e433f57465#patch2
which was after the release of 16.11