Bug 21364 - Allow closing basket from vendor search/view
Summary: Allow closing basket from vendor search/view
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 23031
  Show dependency treegraph
 
Reported: 2018-09-16 21:45 UTC by Nick Clemens
Modified: 2019-06-02 08:39 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
This enhancement adds 'Close basket' to the available actions in the vendor search results view.
Version(s) released in:
19.05.00


Attachments
Bug 21364: Allow closing basket from vendor view/search (6.25 KB, patch)
2018-09-16 21:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21364: Allow closing basket from vendor view/search (6.41 KB, patch)
2018-09-16 22:09 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 21364: (follow-up) Tidy indentation and remove copy/paste newlines (5.89 KB, patch)
2018-09-19 00:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21364: Allow closing basket from vendor view/search (6.97 KB, patch)
2018-09-19 12:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21364: Allow closing basket from vendor view/search (7.02 KB, patch)
2018-09-19 18:36 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 21364: Allow closing basket from vendor view/search (7.08 KB, patch)
2018-09-27 08:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21364: (follow-up) Check for standing status and uncertain prices (4.36 KB, patch)
2019-04-22 20:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21364: (follow-up) Check for standing status and uncertain prices (6.50 KB, patch)
2019-04-25 15:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21364: (follow-up) Check for standing status and uncertain prices (5.50 KB, patch)
2019-04-25 15:27 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 21364: Allow closing basket from vendor view/search (7.15 KB, patch)
2019-04-26 06:25 UTC, Claire Gravely
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 21364: (follow-up) Check for standing status and uncertain prices (5.55 KB, patch)
2019-04-26 06:25 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 21364: (follow-up) Check for standing status and uncertain prices (5.62 KB, patch)
2019-04-27 13:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21364: (QA follow-up) Fix QA script complaints (tabs and filters) (6.93 KB, patch)
2019-04-27 13:58 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-09-16 21:45:16 UTC

    
Comment 1 Nick Clemens 2018-09-16 21:48:20 UTC Comment hidden (obsolete)
Comment 2 ByWater Sandboxes 2018-09-16 22:09:25 UTC Comment hidden (obsolete)
Comment 3 Séverine Queune 2018-09-16 22:09:52 UTC
Step 4 is slightly different as described but I thing behaviour in Koha is the one expected :
  a - empty basket show "View" button and dropdown menu with only "Add to basket" option
  b - no-empty basket show "View" button and dropdown menu with "Add" and "Close" options
  c - close basket show only "View" button

Everything behaves as expected so thanks a lot Nick !!
Comment 4 Jonathan Druart 2018-09-16 22:42:13 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2018-09-19 00:44:02 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2018-09-19 12:43:19 UTC
Maybe this is out of scope, I don't know... I've never thought about it before, but having a "view" button doesn't fit common practices at all. The name of the basket should be a link to the view.

What about this: 

 - Link the basket name to the view.
 - Keep the split button, but making "Add to basket" the main action.
 - In the menu, put "edit," "close," and "delete."
Comment 7 Nick Clemens 2018-09-19 12:56:56 UTC
Created attachment 79106 [details] [review]
Bug 21364: Allow closing basket from vendor view/search

To test:
1 - Have a vendor
2 - Add some empty baskets
3 - Add some non-empty baskets
4 - Search for vendor and note you now
    a - Can only 'View' empty or closed baskets
    b - Have a dropup menu to 'Add to ' or 'Close' open baskets
5 - Confirm things look correct and buttons work
6 - Confirm that basket name is now a link to view basket
Comment 8 Owen Leonard 2018-09-19 18:36:00 UTC
Created attachment 79121 [details] [review]
Bug 21364: Allow closing basket from vendor view/search

To test:
1 - Have a vendor
2 - Add some empty baskets
3 - Add some non-empty baskets
4 - Search for vendor and note you now
    a - Can only 'View' empty or closed baskets
    b - Have a dropup menu to 'Add to ' or 'Close' open baskets
5 - Confirm things look correct and buttons work
6 - Confirm that basket name is now a link to view basket

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Jonathan Druart 2018-09-21 21:23:22 UTC
(In reply to Owen Leonard from comment #8)
> 4 - Search for vendor and note you now
>     a - Can only 'View' empty or closed baskets

Not true, but behavior is correct.
Can only "view" if basket is closed. If basket is empty we can "view" and "add to"

(In reply to Owen Leonard from comment #6)
>  - Keep the split button, but making "Add to basket" the main action.

Finally this has not been done, any good reasons?
Comment 10 Nick Clemens 2018-09-24 10:57:37 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to Owen Leonard from comment #8)
> > 4 - Search for vendor and note you now
> >     a - Can only 'View' empty or closed baskets
> 
> Not true, but behavior is correct.
> Can only "view" if basket is closed. If basket is empty we can "view" and
> "add to"

Yes, was incorrect language from earlier draft of plans, behaviour is correct

> 
> (In reply to Owen Leonard from comment #6)
> >  - Keep the split button, but making "Add to basket" the main action.
> 
> Finally this has not been done, any good reasons?

Owen and I talked this over, adding the other options to the dropdown was a bigger change and making 'add to basket' the default would be an issue since some baskets cannot be added to. This can be improved on future bug reports
Comment 11 Martin Renvoize 2018-09-27 08:20:09 UTC
Created attachment 79470 [details] [review]
Bug 21364: Allow closing basket from vendor view/search

To test:
1 - Have a vendor
2 - Add some empty baskets
3 - Add some non-empty baskets
4 - Search for vendor and note you now
    a - Can only 'View' empty or closed baskets
    b - Have a dropup menu to 'Add to ' or 'Close' open baskets
5 - Confirm things look correct and buttons work
6 - Confirm that basket name is now a link to view basket

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2018-09-27 08:21:09 UTC
Works well for me and I agree that, although it could be further enhanced down the line, it works reasonably as is and those further enhancements can be left for another day.. baby steps.

Passing QA.
Comment 13 Katrin Fischer 2018-09-27 10:21:47 UTC
Making Add to basket a 2-click action might be debatable - maybe sometimes more buttons are better than menus? 
What happens if you don't have order_manage* permissions? (I can't test this right now because of bug 21423)
Comment 14 Séverine Queune 2018-10-03 07:19:57 UTC
I've just realized this new option allows to close standing orders baskets.
This option is normally not available, so maybe the behaviour should be modified.
Sorry to catch that point so late :(
Comment 15 Martin Renvoize 2018-10-03 07:46:28 UTC
Good catch Severine, bit annoyed at myself for not spotting it myself. Seeing Failed QA pending a follow-up to resolve that oversight.
Comment 16 Katrin Fischer 2018-10-03 13:45:40 UTC
Looking at the basket summary page code, I think we need to check for other things as well:
-  existing uncertain prices (uncerteinparices)
-  standing order (unclosable  => @orders ? $basket->{is_standing} : 1,)
-  order_manage permission
Comment 17 Nick Clemens 2019-04-22 20:01:11 UTC
Created attachment 88393 [details] [review]
Bug 21364: (follow-up) Check for standing status and uncertain prices
Comment 18 Nick Clemens 2019-04-22 20:01:52 UTC
(In reply to Katrin Fischer from comment #16)
> Looking at the basket summary page code, I think we need to check for other
> things as well:
> -  existing uncertain prices (uncertainprices)
 Added

> -  standing order (unclosable  => @orders ? $basket->{is_standing} : 1,)
 Added
> -  order_manage permission
 I believe this is done by the call to CanUserManageBasket
Comment 19 Claire Gravely 2019-04-25 13:47:56 UTC
Baskets with uncertain pricing still get a close basket option on the basket summary page. Instead they are marked bold and have dark red text. 

Everything else looks good!
Comment 20 Nick Clemens 2019-04-25 15:22:29 UTC Comment hidden (obsolete)
Comment 21 Nick Clemens 2019-04-25 15:27:59 UTC
Created attachment 88788 [details] [review]
Bug 21364: (follow-up) Check for standing status and uncertain prices

Fetch 'uncertainprices' when getting basket info. A side effect is that styling on baskets with uncertain prices is restored by this bug. They will appear in red

To test:
1 - Add an order with an uncertainprice to the basket
2 - Return to vendor baskets view
3 - Note the actions now include "Uncertain prices"
4 - Click that, resolve the uncertain prcie
5 - Return to vendor basket view
6 - Note you can now close the basket, but don't
7 - Edit the basket, mark it as standing
8 - Return to vendor basket view
9 - Note you cannot close the basket
Comment 22 Claire Gravely 2019-04-26 06:25:23 UTC
Created attachment 88847 [details] [review]
[SIGNED-OFF] Bug 21364: Allow closing basket from vendor view/search

To test:
1 - Have a vendor
2 - Add some empty baskets
3 - Add some non-empty baskets
4 - Search for vendor and note you now
    a - Can only 'View' empty or closed baskets
    b - Have a dropup menu to 'Add to ' or 'Close' open baskets
5 - Confirm things look correct and buttons work
6 - Confirm that basket name is now a link to view basket

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 23 Claire Gravely 2019-04-26 06:25:26 UTC
Created attachment 88848 [details] [review]
[SIGNED-OFF] Bug 21364: (follow-up) Check for standing status and uncertain prices

Fetch 'uncertainprices' when getting basket info. A side effect is that styling on baskets with uncertain prices is restored by this bug. They will appear in red

To test:
1 - Add an order with an uncertainprice to the basket
2 - Return to vendor baskets view
3 - Note the actions now include "Uncertain prices"
4 - Click that, resolve the uncertain prcie
5 - Return to vendor basket view
6 - Note you can now close the basket, but don't
7 - Edit the basket, mark it as standing
8 - Return to vendor basket view
9 - Note you cannot close the basket

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 24 Katrin Fischer 2019-04-27 13:45:19 UTC
 FAIL	C4/Acquisition.pm
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 688)
   OK	  git manipulation
   OK	  pod
   OK	  pod coverage
   OK	  spelling
   OK	  valid

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/acqui/booksellers.tt
   FAIL	  filters
		wrong_html_filter at line 105 (                                        <td><a href="/cgi-bin/koha/acqui/basket.pl?basketno=[% basket.basketno | html %]">[% basket.basketname | html %]<a></td>)
		wrong_html_filter at line 154 (                                                        <li><a href="/cgi-bin/koha/acqui/uncertainprice.pl?booksellerid=[% basket.booksellerid | html %]&owner=1"><i class="fa fa-usd"></i> Uncertain prices</a></li>)
		wrong_html_filter at line 156 (                                                        <li><a href="/cgi-bin/koha/acqui/basket.pl?op=close&basketno=[% basket.basketno | html %]&booksellerid=[% basket.booksellerid | html %]"><i class="fa fa-times-circle"></i> Close this basket</a></li>)
		wrong_html_filter at line 156 (                                                        <li><a href="/cgi-bin/koha/acqui/basket.pl?op=close&basketno=[% basket.basketno | html %]&booksellerid=[% basket.booksellerid | html %]"><i class="fa fa-times-circle"></i> Close this basket</a></li>)

Trying to fix in a follow-up.
Comment 25 Katrin Fischer 2019-04-27 13:57:54 UTC
I am not super happy about the 'add to basket' option being hidden now in a menu in all cases. I would have preferred to keep it visible and easy to access. It kind of contradicts the original purpose of being a fast action now. No longer saving any clicks compared to viewing and adding.

I do like the 'uncertain prices' and 'close basket' actions.
Comment 26 Katrin Fischer 2019-04-27 13:58:39 UTC
Created attachment 88982 [details] [review]
Bug 21364: (follow-up) Check for standing status and uncertain prices

Fetch 'uncertainprices' when getting basket info. A side effect is that styling on baskets with uncertain prices is restored by this bug. They will appear in red

To test:
1 - Add an order with an uncertainprice to the basket
2 - Return to vendor baskets view
3 - Note the actions now include "Uncertain prices"
4 - Click that, resolve the uncertain prcie
5 - Return to vendor basket view
6 - Note you can now close the basket, but don't
7 - Edit the basket, mark it as standing
8 - Return to vendor basket view
9 - Note you cannot close the basket

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2019-04-27 13:58:44 UTC
Created attachment 88983 [details] [review]
Bug 21364: (QA follow-up) Fix QA script complaints (tabs and filters)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 28 Nick Clemens 2019-04-30 13:41:53 UTC
Awesome work all!

Pushed to master for 19.05
Comment 29 Martin Renvoize 2019-05-08 14:12:01 UTC
Enhancement will not be backported to 18.11.x series.
Comment 30 Jonathan Druart 2019-05-09 17:11:38 UTC
Please, we need the "Add to basket" link accessible in one click!
Comment 31 Katrin Fischer 2019-05-11 23:10:49 UTC
Owen, could you take a look at the discussion here?
Comment 32 Owen Leonard 2019-05-13 14:27:01 UTC
The "View" part of the split button is an unnecessary duplication of the linked basket name earlier in the table. I think the actions column should have two possible buttons: "Add to basket" and "Close basket."
Comment 33 Katrin Fischer 2019-06-02 08:39:15 UTC
I've moved the discussion about the buttons onto a new bug 23031.