Bug 18613 - Deleting a Letter from a library as superlibrarian deletes the "All libraries" rule
Summary: Deleting a Letter from a library as superlibrarian deletes the "All libraries...
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Baptiste
QA Contact: Testopia
URL:
Whiteboard:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2017-05-16 13:46 UTC by Baptiste
Modified: 2017-07-19 21:34 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18613: Correct behaviour (2.68 KB, patch)
2017-05-16 13:52 UTC, Baptiste
Details | Diff | Splinter Review
Bug 18613: Correct behaviour (2.68 KB, patch)
2017-05-16 13:55 UTC, Baptiste
Details | Diff | Splinter Review
Bug 18613: Correct behaviour (2.67 KB, patch)
2017-05-16 13:56 UTC, Baptiste
Details | Diff | Splinter Review
Bug 18613: Correct the behaviour (1.31 KB, patch)
2017-05-19 10:33 UTC, Baptiste
Details | Diff | Splinter Review
Bug 18613: Correct the behaviour (1.35 KB, patch)
2017-06-25 16:37 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18613: Remove letter rule correctly as superlibrarian (1.38 KB, patch)
2017-07-07 13:34 UTC, Baptiste
Details | Diff | Splinter Review
Bug 18613: Remove letter rule correctly as superlibrarian (1.45 KB, patch)
2017-07-07 13:46 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 18613: Add unit tests for getletter (4.00 KB, patch)
2017-07-11 09:13 UTC, Baptiste
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Baptiste 2017-05-16 13:46:33 UTC Comment hidden (obsolete)
Comment 1 Baptiste 2017-05-16 13:52:28 UTC Comment hidden (obsolete)
Comment 2 Baptiste 2017-05-16 13:55:12 UTC Comment hidden (obsolete)
Comment 3 Baptiste 2017-05-16 13:56:29 UTC Comment hidden (obsolete)
Comment 4 Baptiste 2017-05-19 10:33:18 UTC
Created attachment 63578 [details] [review]
Bug 18613: Correct the behaviour

TEST PLAN

(reproducing bug)
1 - Set 'IndependantBranches' to enable
2 - Log in as Superlibrarian.
3 - Create 3 rules
    - 1 for all librairies (1)
    - 1 for the library of the superlibrarian (2)
    - 1 for another library (3)
4 - Try to delete them and see them and see that (3) isn't the good one

(checking patch)
5 - Apply patch
6 - Try again 3-
7 - Connect as not superlibrarian
8 - Try again 3-
Comment 5 Alex Buckley 2017-06-25 16:37:07 UTC
Created attachment 64616 [details] [review]
Bug 18613: Correct the behaviour

TEST PLAN

(reproducing bug)
1 - Set 'IndependantBranches' to enable
2 - Log in as Superlibrarian.
3 - Create 3 rules
    - 1 for all librairies (1)
    - 1 for the library of the superlibrarian (2)
    - 1 for another library (3)
4 - Try to delete them and see them and see that (3) isn't the good one

(checking patch)
5 - Apply patch
6 - Try again 3-
7 - Connect as not superlibrarian
8 - Try again 3-

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 6 Julian Maurice 2017-07-07 09:57:45 UTC
Comment on attachment 64616 [details] [review]
Bug 18613: Correct the behaviour

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

::: C4/Letters.pm
@@ +204,5 @@
>  
>  
> +    my $only_my_library = C4::Context->only_my_library;
> +    if ( $only_my_library and $branchcode ) {
> +        $branchcode = $only_my_library;

C4::Context->only_my_library is supposed to return a boolean value.
You should use C4::Context::mybranch() here
Comment 7 Julian Maurice 2017-07-07 09:59:43 UTC
Also, if you could change the patch subject to a more meaningful one, that would be great! :)
Comment 8 Baptiste 2017-07-07 13:34:15 UTC
Created attachment 64912 [details] [review]
Bug 18613: Remove letter rule correctly as superlibrarian

TEST PLAN

(reproducing bug)
1 - Set 'IndependantBranches' to enable
2 - Log in as Superlibrarian.
3 - Create 3 rules
    - 1 for all librairies (1)
    - 1 for the library of the superlibrarian (2)
    - 1 for another library (3)
4 - Try to delete them and see them and see that (3) isn't the good one

(checking patch)
5 - Apply patch
6 - Try again 3-
7 - Connect as not superlibrarian
8 - Try again 3-

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 9 Julian Maurice 2017-07-07 13:46:15 UTC
Created attachment 64914 [details] [review]
Bug 18613: Remove letter rule correctly as superlibrarian

TEST PLAN

(reproducing bug)
1 - Set 'IndependantBranches' to enable
2 - Log in as Superlibrarian.
3 - Create 3 rules
    - 1 for all librairies (1)
    - 1 for the library of the superlibrarian (2)
    - 1 for another library (3)
4 - Try to delete them and see them and see that (3) isn't the good one

(checking patch)
5 - Apply patch
6 - Try again 3-
7 - Connect as not superlibrarian
8 - Try again 3-

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 10 Jonathan Druart 2017-07-10 20:54:49 UTC
Please provide tests.
Comment 11 Baptiste 2017-07-11 09:13:25 UTC
Created attachment 64987 [details] [review]
Bug 18613: Add unit tests for getletter
Comment 12 Jonathan Druart 2017-07-17 19:12:38 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 13 Fridolin SOMERS 2017-07-18 14:53:34 UTC
Pushed to 17.05.x, will be in 17.05.02
Comment 14 Katrin Fischer 2017-07-19 21:34:25 UTC
These patches have been pushed to 16.11.x and will be in 16.11.10.

Tests pass, but there is an additional warn about userenv:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Letters.t 
t/db_dependent/Letters.t .. 1/74 C4::Context->userenv not defined! at /home/vagrant/kohaclone/C4/Letters.pm line 206.
Fake sendmail at t/db_dependent/Letters.t line 32.
t/db_dependent/Letters.t .. ok 
All tests successful.
Files=1, Tests=74,  1 wallclock secs ( 0.02 usr  0.00 sys +  1.17 cusr  0.36 csys =  1.55 CPU)
Result: PASS