Bug 17970 - GetPreparedLetter does not warn when expected
Summary: GetPreparedLetter does not warn when expected
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 17958
Blocks: 17965 17971
  Show dependency treegraph
 
Reported: 2017-01-23 14:26 UTC by Jonathan Druart
Modified: 2017-12-07 22:15 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 17970: Add tests to highlight the problem (2.04 KB, patch)
2017-01-23 14:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17970: Fix GetPreparedLetter behavior if nothing to substitute (1.90 KB, patch)
2017-01-23 14:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17970: Add tests to highlight the problem (2.04 KB, patch)
2017-01-23 14:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17970: Add tests to highlight the problem (2.25 KB, patch)
2017-01-23 14:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17970: Add tests to highlight the problem (2.30 KB, patch)
2017-01-24 03:32 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17970: Fix GetPreparedLetter behavior if nothing to substitute (1.95 KB, patch)
2017-01-24 03:32 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 17970: Add tests to highlight the problem (2.40 KB, patch)
2017-03-10 08:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17970: Fix GetPreparedLetter behavior if nothing to substitute (2.05 KB, patch)
2017-03-10 08:38 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 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.