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
Created attachment 25892 [details] [review] 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. But the C4::Letters::GetLetters routine returns a hashref with letter code in keys => only 1 per letter code is returned. I tried to fix the API but it is more complicated than I thought. 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.
I am not really fond of this patch (especially as it will break the bug 9016 integration). Feel free to provide something else.
Now that the patch for bug 9016 has been pushed, this needs a fresh look.
Created attachment 28020 [details] [review] Bug 11742: Change prototype 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.
Created attachment 28021 [details] [review] 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.
Created attachment 28687 [details] [review] 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>
Created attachment 28688 [details] [review] 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>
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.
Created attachment 28729 [details] [review] 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.
(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!
Created attachment 28956 [details] [review] 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>
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.
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>
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>
Patches pushed to master (along with a trivial followup fixing the tests count). Thanks Jonathan!
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>
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...?
(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.