Bug 20866 - ArticleRequests.t fails on existing requests
Summary: ArticleRequests.t fails on existing requests
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 18403
Blocks:
  Show dependency treegraph
 
Reported: 2018-06-04 08:25 UTC by Marcel de Rooy
Modified: 2018-06-13 06:30 UTC (History)
4 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 20866: Fix ArticleRequests.t when table contains records (1.21 KB, patch)
2018-06-04 08:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[ALTERNATIVE PATH] Bug 20866: Make ArticleRequests.t pass even if table is not empty (3.68 KB, patch)
2018-06-04 18:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20866: Make ArticleRequests.t pass even if table is not empty (3.73 KB, patch)
2018-06-04 19:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20866: Make ArticleRequests.t pass even if table is not empty (3.82 KB, patch)
2018-06-05 11:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20866: (QA follow-up) Use build_object and remove two tests (3.00 KB, patch)
2018-06-05 11:49 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 Marcel de Rooy 2018-06-04 08:25:25 UTC
Subtest search_limited fails.
# Subtest: search_limited
    1..4
    ok 1 - Koha::ArticleRequests should return all article requests
    not ok 2 - Koha::ArticleRequests->search_limited should return all article requests for superlibrarian
    #   Failed test 'Koha::ArticleRequests->search_limited should return all article requests for superlibrarian'
    #   at t/db_dependent/ArticleRequests.t line 216.
    #          got: '1'
    #     expected: '3'
    ok 3 - Koha::ArticleRequests should return all article requests
    not ok 4 - Koha::ArticleRequests->search_limited should not return all article requests for restricted patron
    #   Failed test 'Koha::ArticleRequests->search_limited should not return all article requests for restricted patron'
    #   at t/db_dependent/ArticleRequests.t line 219.
    #          got: '0'
    #     expected: '2'
    # Looks like you failed 2 tests of 4.
not ok 55 - search_limited

Can be fixed by clearing the table at the start.
Comment 1 Marcel de Rooy 2018-06-04 08:30:17 UTC
Created attachment 75750 [details] [review]
Bug 20866: Fix ArticleRequests.t when table contains records

Subtest search_limited fails. Assumptions in that code may not be true.
Simple fix for now: clear the table at the start.

Test plan:
[1] Create an article request.
[2] Run t/db_dependent/ArticleRequests.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2018-06-04 08:33:37 UTC
Signing off too: trivial fix
Comment 3 Jonathan Druart 2018-06-04 18:23:34 UTC
There are some problems hidden your patch actually.
Comment 4 Jonathan Druart 2018-06-04 18:32:41 UTC
Created attachment 75784 [details] [review]
[ALTERNATIVE PATH] Bug 20866: Make ArticleRequests.t pass even if table is not empty

Previous patch from Marcel removed the data from article_requests, I do
not think it is a good idea to remove existing data, it could hide bugs.

This patch moves the count inside the subtest block in order to have the
correct value when these tests are executed.
Previous code was broken: Koha::Patron->store does not generate a
userid, and ->search_limited did not work correctly
(Koha::Patron->has_permission returned early because of the non-existing
userid, 'return unless $self->userid;');

Test plan:
[1] Create an article request.
[2] Run t/db_dependent/ArticleRequests.t
Comment 5 Katrin Fischer 2018-06-04 19:33:36 UTC
Created attachment 75791 [details] [review]
Bug 20866: Make ArticleRequests.t pass even if table is not empty

Previous patch from Marcel removed the data from article_requests, I do
not think it is a good idea to remove existing data, it could hide bugs.

This patch moves the count inside the subtest block in order to have the
correct value when these tests are executed.
Previous code was broken: Koha::Patron->store does not generate a
userid, and ->search_limited did not work correctly
(Koha::Patron->has_permission returned early because of the non-existing
userid, 'return unless $self->userid;');

Test plan:
[1] Create an article request.
[2] Run t/db_dependent/ArticleRequests.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 6 Marcel de Rooy 2018-06-05 11:49:48 UTC
Created attachment 75813 [details] [review]
Bug 20866: Make ArticleRequests.t pass even if table is not empty

Previous patch from Marcel removed the data from article_requests, I do
not think it is a good idea to remove existing data, it could hide bugs.

This patch moves the count inside the subtest block in order to have the
correct value when these tests are executed.
Previous code was broken: Koha::Patron->store does not generate a
userid, and ->search_limited did not work correctly
(Koha::Patron->has_permission returned early because of the non-existing
userid, 'return unless $self->userid;');

Test plan:
[1] Create an article request.
[2] Run t/db_dependent/ArticleRequests.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2018-06-05 11:49:55 UTC
Created attachment 75814 [details] [review]
Bug 20866: (QA follow-up) Use build_object and remove two tests

The unique constraint on userid is handled in TestBuilder. So let's use it.
The two tests if count==$count do not make much sense anymore when we
call ->count a few lines before. The check with search_limited is enough.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Nick Clemens 2018-06-08 13:54:42 UTC
Pushed to master for 18.11, awesome work all!
Comment 9 Martin Renvoize 2018-06-11 07:51:35 UTC
This patch has been pushed to 18.05.x and will be in 18.05.01
Comment 10 Fridolin SOMERS 2018-06-13 06:30:34 UTC
Depends on Bug 18403 not in 17.11.x