|
Description
Tomás Cohen Arazi (tcohen)
2019-04-11 11:11:33 UTC
Created attachment 87764 [details] [review] Bug 22688: Unit tests Created attachment 87765 [details] [review] Bug 22688: TT plugin for pickup locations code wrong Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Libraries.t Created attachment 87766 [details] [review] Bug 22688: Unit tests Created attachment 87768 [details] [review] Bug 22688: Unit tests Created attachment 87772 [details] [review] Bug 22688: TT plugin for pickup locations code wrong Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Libraries.t Signed-off-by: Agustín Moyano <agustinmoyano@theke.io> Created attachment 87773 [details] [review] Bug 22688: Unit tests Signed-off-by: Agustín Moyano <agustinmoyano@theke.io> Kyle, it seems to me that the test case you picked is the one that doesn't test pickup limitations (compared to all()).
I think we should test:
- Passing a selected => branchcode, then if selected => 1 is added to the hashref
- Using mock_userenv({ branchcode => $library->branchcode }) to test the case in which C4::Context->userenv is used to pick the 'selected' library.
I confess I spend a good amount of time today trying to implement a fully mocked test, only to learn that there's a problem with the 'selected' param because it would be passed all the way down to Koha::Libraries->search...
This got into a rabbit hole (writing this tests).
Created attachment 87817 [details] [review] Bug 22688: Unit tests Signed-off-by: Agustín Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 87818 [details] [review] Bug 22688: (follow-up) Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 87819 [details] [review] Bug 22688: TT plugin for pickup locations code wrong Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Libraries.t Signed-off-by: Agustín Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 87820 [details] [review] Bug 22688: (QA follow-up) Use search_params as all() does It looks like selected would be passed all the way down to Koha::Libraries->search and this would fail all the time. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> I decided to submit what I had done. But it needs your review, Kyle, and maybe the mocked tests are not enough. It is such a tiny thing, used in a single place... Created attachment 87884 [details] [review] Bug 22688: Unit tests Signed-off-by: Agustín Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 87885 [details] [review] Bug 22688: Unit tests Signed-off-by: Agustín Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 87886 [details] [review] Bug 22688: (follow-up) Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 87887 [details] [review] Bug 22688: TT plugin for pickup locations code wrong Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Libraries.t Signed-off-by: Agustín Moyano <agustinmoyano@theke.io> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 87888 [details] [review] Bug 22688: (QA follow-up) Use search_params as all() does It looks like selected would be passed all the way down to Koha::Libraries->search and this would fail all the time. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Awesome work all! Pushed to master for 19.05 Pushed to 18.11.x for 18.11.05 missing dependencies for 18.05.x, not backporting |