Bug 11742 - Overdue notice/status triggers displaying the wrong notice for default
Summary: Overdue notice/status triggers displaying the wrong notice for default
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 13215
  Show dependency treegraph
 
Reported: 2014-02-11 17:47 UTC by Kyle M Hall
Modified: 2015-06-04 23:33 UTC (History)
5 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 11742: A letter code should be unique. (12.36 KB, patch)
2014-03-06 12:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11742: Change prototype for GetLetters (14.13 KB, patch)
2014-05-07 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11742: A letter code should be unique. (19.46 KB, patch)
2014-05-07 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11742: Change return type for GetLetters (14.18 KB, patch)
2014-06-05 20:24 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11742: A letter code should be unique. (19.51 KB, patch)
2014-06-05 20:25 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 11742: FIX the code parameter was not take into account (1.97 KB, patch)
2014-06-09 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11742: FIX the code parameter was not take into account (2.02 KB, patch)
2014-06-18 20:15 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 11742: Change return type for GetLetters (14.58 KB, patch)
2014-06-19 15:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11742: A letter code should be unique. (19.58 KB, patch)
2014-06-19 15:24 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 11742: FIX the code parameter was not take into account (2.09 KB, patch)
2014-06-19 15:24 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 11742: (QA followup) wrong number of tests (1.66 KB, patch)
2014-06-23 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-02-11 17:47:49 UTC
If you copy the notice set for an overdue trigger, overduerules.pl will display the description for the copied notice instead of the actual notice! This is because the copied notice shares the same letter code as the notice that was copied.

Video example: http://screencast.com/t/WeEG3qlw
Comment 1 Jonathan Druart 2014-03-06 12:53:28 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-03-06 14:51:17 UTC
I am not really fond of this patch (especially as it will break the bug 9016 integration). Feel free to provide something else.
Comment 3 Galen Charlton 2014-05-06 15:34:03 UTC
Now that the patch for bug 9016 has been pushed, this needs a fresh look.
Comment 4 Jonathan Druart 2014-05-07 14:21:39 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-05-07 14:21:48 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2014-06-05 20:24:52 UTC Comment hidden (obsolete)
Comment 7 Chris Cormack 2014-06-05 20:25:11 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2014-06-06 18:35:54 UTC
Problems:
1) If you create a new notice with the same module/code/branch as an existing letter  it will overwrite the existing letter without notice

2) If you choose to edit a notice, and change it's module/code/branch to match another letter, it will overwrite the other letter, while leaving the one you edited unmodified!

I think letters is in serious need of an id column.
Comment 9 Jonathan Druart 2014-06-09 10:24:14 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2014-06-09 10:25:34 UTC
(In reply to Kyle M Hall from comment #8)
> Problems:

Good catch Kyle, thanks!

> 1) If you create a new notice with the same module/code/branch as an
> existing letter  it will overwrite the existing letter without notice

Should be fixed.

> 2) If you choose to edit a notice, and change it's module/code/branch to
> match another letter, it will overwrite the other letter, while leaving the
> one you edited unmodified!

Should be fixed too.

> I think letters is in serious need of an id column.

Yes definitely, but it is a big change!
Comment 11 Chris Cormack 2014-06-18 20:15:06 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2014-06-19 15:23:25 UTC
Created attachment 29049 [details] [review]
[PASSED QA] Bug 11742: Change return type for GetLetters

The GetLetters subroutine should return an arrayref with different
letters for a module.

Test plan:
0/ Delete your notices with module=claimacquisition, claimissues,
serial
1/ Go on the late orders page (acqui/lateorders.pl) and verify you
cannot choose a notice for claiming
2/ Create a notice with module=claimacquisition
3/ Go on the late orders page (acqui/lateorders.pl) and verify you
can choose the notice for claiming
4/ Go on the Claim serials page (serials/claims.pl) and repeat the same
thing with the a "claimissues" notice
5/ Create a new subscription (serials/subscription-add.pl) and verify
you cannot choose a notification for patrons.
6/ Create a notice with module "serial" and verify you can.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script. Additional tests done:

- copy notice ODUE, on saving you are now prompted to choose
  a new CODE for the notice
- edit new notice, try to set code back to ODUE. You are
  prompted that the code is already in use.

This will prevent people from accidentally overwriting a letter
with the same letter code.
Comment 13 Katrin Fischer 2014-06-19 15:24:00 UTC
Created attachment 29050 [details] [review]
[PASSED QA] Bug 11742: A letter code should be unique.

This patch is a dirty way to fix a design issue on notices.
Currently the code assumes that a letter code is unique. Which is wrong,
the primary key is module, code, branchcode.

Maybe we should add a primary key (id) for the letter table in order to
pass the id to the template and correctly manage the letter code
duplication.

Test plan:
Try to duplicate a letter code using edit, add and copy actions.
If you manage to do it, please describe how you did.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 14 Katrin Fischer 2014-06-19 15:24:11 UTC
Created attachment 29051 [details] [review]
[PASSED QA] Bug 11742: FIX the code parameter was not take into account

In C4::Letters::GetLetters, the code filter was not used as a query
parameter.

Moreover, the JS code was buggy. We only need to check the letter code,
except if it is an edit and the letter code has not been changed.

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 15 Tomás Cohen Arazi 2014-06-23 18:29:43 UTC
Patches pushed to master (along with a trivial followup fixing the tests count).

Thanks Jonathan!
Comment 16 Tomás Cohen Arazi 2014-06-23 19:44:49 UTC
Created attachment 29180 [details] [review]
Bug 11742: (QA followup) wrong number of tests

The number of tests was wrong.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 17 Katrin Fischer 2014-11-03 22:35:52 UTC
I feel like we made a mistake here. Now it's not possible to add notices that have a hardcoded notice code like 'HOLD' for multiple branches.

Maybe I am wrong...?
Comment 18 Jonathan Druart 2014-11-06 15:37:04 UTC
(In reply to Katrin Fischer from comment #17)
> I feel like we made a mistake here. Now it's not possible to add notices
> that have a hardcoded notice code like 'HOLD' for multiple branches.
> 
> Maybe I am wrong...?

Good catch.
This behavior will be problematic quickly and should be fix before the release.
I am going to submit a patch.