Bug 17970

Summary: GetPreparedLetter does not warn when expected
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, kyle, m.de.rooy, mtompset
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17958    
Bug Blocks: 17965, 17971    
Attachments: Bug 17970: Add tests to highlight the problem
Bug 17970: Fix GetPreparedLetter behavior if nothing to substitute
Bug 17970: Add tests to highlight the problem
Bug 17970: Add tests to highlight the problem
Bug 17970: Add tests to highlight the problem
Bug 17970: Fix GetPreparedLetter behavior if nothing to substitute
Bug 17970: Add tests to highlight the problem
Bug 17970: Fix GetPreparedLetter behavior if nothing to substitute

Description Jonathan Druart 2017-01-23 14:26:10 UTC
From C4::Letters::GetPreparedLetter:

    my $tables = $params{tables};
    my $substitute = $params{substitute};

    $tables || $substitute || $repeat
       or carp( "ERROR: nothing to substitute - both 'tables' and 'substitute' are empty" ),
          return;

So if the parameter tables or substitute is passed but does not contain anything, it will not warn as expected.
Comment 1 Jonathan Druart 2017-01-23 14:28:12 UTC
Created attachment 59422 [details] [review]
Bug 17970: Add tests to highlight the problem
Comment 2 Jonathan Druart 2017-01-23 14:28:19 UTC
Created attachment 59423 [details] [review]
Bug 17970: Fix GetPreparedLetter behavior if nothing to substitute

From C4::Letters::GetPreparedLetter:

    my $tables = $params{tables};
    my $substitute = $params{substitute};

    $tables || $substitute || $repeat
       or carp( "ERROR: nothing to substitute - both 'tables' and 'substitute' are empty" ),
          return;

So if the parameter tables or substitute is passed but does not contain anything, it will not warn as expected.

Test plan:
1/ Apply the patch with tests
2/ Confirm that they do not pass
3/ Apply this patch
4/ Confirm that the tests now pass
Comment 3 Jonathan Druart 2017-01-23 14:42:28 UTC
Created attachment 59424 [details] [review]
Bug 17970: Add tests to highlight the problem
Comment 4 Jonathan Druart 2017-01-23 14:43:31 UTC
Created attachment 59425 [details] [review]
Bug 17970: Add tests to highlight the problem
Comment 5 Mark Tompsett 2017-01-24 03:32:03 UTC
Created attachment 59481 [details] [review]
Bug 17970: Add tests to highlight the problem

Depends on 17958

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 6 Mark Tompsett 2017-01-24 03:32:09 UTC
Created attachment 59482 [details] [review]
Bug 17970: Fix GetPreparedLetter behavior if nothing to substitute

From C4::Letters::GetPreparedLetter:

    my $tables = $params{tables};
    my $substitute = $params{substitute};

    $tables || $substitute || $repeat
       or carp( "ERROR: nothing to substitute - both 'tables' and 'substitute' are empty" ),
          return;

So if the parameter tables or substitute is passed but does not contain anything, it will not warn as expected.

Test plan:
1/ Apply the patch with tests
2/ Confirm that they do not pass
3/ Apply this patch
4/ Confirm that the tests now pass

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 7 Marcel de Rooy 2017-03-10 08:38:54 UTC
Created attachment 60976 [details] [review]
Bug 17970: Add tests to highlight the problem

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2017-03-10 08:38:57 UTC
Created attachment 60977 [details] [review]
Bug 17970: Fix GetPreparedLetter behavior if nothing to substitute

From C4::Letters::GetPreparedLetter:

    my $tables = $params{tables};
    my $substitute = $params{substitute};

    $tables || $substitute || $repeat
       or carp( "ERROR: nothing to substitute - both 'tables' and 'substitute' are empty" ),
          return;

So if the parameter tables or substitute is passed but does not contain anything, it will not warn as expected.

Test plan:
1/ Apply the patch with tests
2/ Confirm that they do not pass
3/ Apply this patch
4/ Confirm that the tests now pass

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2017-03-10 08:44:39 UTC
Not really sure if we need this. But no blocker.
Note that you also have wantlibrarian, which triggers an action but will not be allowed on itself now. And we still replace OPACBaseURL and call _process_tt if we do not pass table/substitute/repeat..
I diagonally checked all calls of GetPreparedLetter and they all use one or more of the parameters in the condition. So no problems expected.
The error message does not mention repeat (part of the condition).
The POD does not tell me which parameters are mandatory.
Comment 10 Kyle M Hall 2017-03-31 11:51:46 UTC
Pushed to master for 17.05, thanks Jonathan!
Comment 11 Katrin Fischer 2017-04-02 16:22:08 UTC
Dependency is not in 16.11.x, so this should not be needed there.