Bug 21098 - Add missing budget optgroups to parcels.tt and newemptyorder.tt
Summary: Add missing budget optgroups to parcels.tt and newemptyorder.tt
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-20 08:02 UTC by David Cook
Modified: 2023-06-02 02:05 UTC (History)
3 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 21098 - Add missing budget optgroups to parcels.tt and newemptyorder.tt (10.79 KB, patch)
2018-07-20 08:42 UTC, David Cook
Details | Diff | Splinter Review
Bug 21098 - Add missing budget optgroups to parcels.tt and newemptyorder.tt (11.01 KB, patch)
2018-11-20 05:39 UTC, David Cook
Details | Diff | Splinter Review
Bug 21098: Add missing budget optgroups to parcels.tt and newemptyorder.tt (11.05 KB, patch)
2018-12-05 16:38 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21098: Add missing budget optgroups to parcels.tt and newemptyorder.tt (11.11 KB, patch)
2019-01-11 19:54 UTC, José-Mario Monteiro-Santos
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2018-07-20 08:02:23 UTC
orderreceive.tt uses optgroup tags to organise funds into budgets, but this same functionality is missing on parcels.tt and newemptyorder.tt.

If you have funds in different budgets with the same or similar descriptions, it becomes impossible to differentiate funds.
Comment 1 David Cook 2018-07-20 08:42:19 UTC Comment hidden (obsolete)
Comment 2 John Sterbenz 2018-09-18 16:17:44 UTC
20180918:  Failed testing

Patch applied to master on (new) Bywater sandboxes.

Verify fail:

Apply patch to master

As necessary, create and appropriate a budget
As necessary, create and appropriate a fund within that budget
As necessary, create a vendor
As necessary, create a basket
Select "Add to basket"
Select "From a new (empty) record:
ERROR:  Template process failed: file error - parse error - /kohadevbox/koha/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/neworderempty.tt line 783: unexpected end of input at /kohadevbox/koha/C4/Templates.pm line 122

This also fails for Z39.50 cataloging record selection--perform search, pick a record from results list, click "Order"--same error.

Untested for other "Add order to basket" methods.

See https://drive.google.com/open?id=1jWM6wIoUTcu5CSQYeGJaYj_V2uIwZLjM (anyone with link can view)--files include a longer version that first replicated the problem.  The steps above skip the replication on a system with the patch applied.
Comment 3 David Cook 2018-11-20 05:39:46 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2018-12-05 16:38:33 UTC
Created attachment 82878 [details] [review]
Bug 21098: Add missing budget optgroups to parcels.tt and newemptyorder.tt

I noticed that orderreceive.tt organizes funds by budget, but neworderempty.tt
and parcels.tt do not. That is to say, when placing an order there is no
organisation of funds in the drop-down menu, but there is when you recive an order,
but not when you receive a shipment (ie choose a Shipping fund).

This patch seeks to harmonize the user experience, so that users are always
seeing funds organised by budget.

_TEST PLAN_
1. Add two budgets
2. Add one fund to each budget
3. Add a vendor
4. Add a basket
5. Add an order
5b. Note that you can now see funds grouped by budget
6. Close the basket
7. Receive the shipment
7b. Note that you can now see funds groups by budget
8. Receive the order
8b. Note that the organisation of funds is the same as the
    other two areas (even though the patch doesn't touch this one).

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 5 David Cook 2018-12-06 23:57:11 UTC
Thanks, Owen :)
Comment 6 Jonathan Druart 2018-12-11 15:47:18 UTC
The "Show inactive" does not seem to work on parcels.pl
Comment 7 David Cook 2018-12-12 00:44:42 UTC
(In reply to Jonathan Druart from comment #6)
> The "Show inactive" does not seem to work on parcels.pl

Thanks, Jonathan. I'll take a look at that when I have a moment.
Comment 8 Jonathan Druart 2018-12-12 15:13:27 UTC
(In reply to Jonathan Druart from comment #6)
> The "Show inactive" does not seem to work on parcels.pl

This is wrong, it works. I had budget_amount==0
The behavior between the 2 views is different, because of:

neworderemtpy.pl
324     if (!defined $r->{budget_amount} || $r->{budget_amount} <0) {

and parcels.pl
186     if (!defined $r->{budget_amount} || $r->{budget_amount} == 0) {

You can forget my comment.
Comment 9 José-Mario Monteiro-Santos 2019-01-11 19:54:28 UTC
Created attachment 83824 [details] [review]
Bug 21098: Add missing budget optgroups to parcels.tt and newemptyorder.tt

I noticed that orderreceive.tt organizes funds by budget, but neworderempty.tt
and parcels.tt do not. That is to say, when placing an order there is no
organisation of funds in the drop-down menu, but there is when you recive an order,
but not when you receive a shipment (ie choose a Shipping fund).

This patch seeks to harmonize the user experience, so that users are always
seeing funds organised by budget.

_TEST PLAN_
1. Add two budgets
2. Add one fund to each budget
3. Add a vendor
4. Add a basket
5. Add an order
5b. Note that you can now see funds grouped by budget
6. Close the basket
7. Receive the shipment
7b. Note that you can now see funds groups by budget
8. Receive the order
8b. Note that the organisation of funds is the same as the
    other two areas (even though the patch doesn't touch this one).

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Jose-Mario <jose-mario.monteiro-santos@inLibro.com>
Comment 10 Katrin Fischer 2019-02-02 20:34:57 UTC
Hi, 

there is one noticeable and big difference: With the patch you see the optgroups of budgets, even if there are no funds listed below. That makes the list grow long, especially as it also includes the inactive ones by default.

Combinations I tested:
- Active budget with multi-level funds - ok
- Inactive budget with fund - not ok, budget is listed
- Locked budget with fund - not ok, budget is listed
- Active budget with funds that are not accessible by permission 
  (restricted to owner) - not ok
  This would for example be used to separate budgets and funds of different
  libraries in a system and could also make the list quite long.

I really like the idea of this improvement, but I think we are not there yet.
Comment 11 David Cook 2019-02-03 23:27:10 UTC
(In reply to Katrin Fischer from comment #10)
> Hi, 
> 
> there is one noticeable and big difference: With the patch you see the
> optgroups of budgets, even if there are no funds listed below. That makes
> the list grow long, especially as it also includes the inactive ones by
> default.
> 
> Combinations I tested:
> - Active budget with multi-level funds - ok
> - Inactive budget with fund - not ok, budget is listed
> - Locked budget with fund - not ok, budget is listed
> - Active budget with funds that are not accessible by permission 
>   (restricted to owner) - not ok
>   This would for example be used to separate budgets and funds of different
>   libraries in a system and could also make the list quite long.
> 
> I really like the idea of this improvement, but I think we are not there yet.

Do you know if this is comparable to orderreceive.tt? My original plan was just to bring parcels.tt and newemptyorder.tt in line with that template.

Did inactive and locked budgets not show before the patch?

I'm not sure I understand the "accessible by permission" part. I think the code for that is identical before and after the patch?

If the list were long, I would think having optgroup tags would be essential for organising them? Otherwise the select list would be difficult to navigate. I'm not sure that I understand this part of the feedback.
Comment 12 David Cook 2019-02-03 23:30:06 UTC
This is a change that I already manage locally so I'm probably unlikely to revise this patch any time soon. I'll remove myself as Assignee, but re-add myself if I do find myself with some time to try again.
Comment 13 Katrin Fischer 2019-02-04 06:46:01 UTC
I wrote difference because the behaviour is different to the already existing drop down on receive. This one works correctly.
Comment 14 Katrin Fischer 2019-02-04 06:47:13 UTC
"By permission" would for example be a fund managed by another library in the system that is restricted to users, owners and staff patrons from this library. With you patch I seee the budget, even if I can't order from any fund below it.
Comment 15 David Cook 2023-06-02 02:05:11 UTC
Still relevant but I think the failure is still relevant too.