Description
Nick Clemens (kidclamp)
2018-09-16 21:45:16 UTC
Created attachment 79015 [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 Created attachment 79019 [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 Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> 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 !! Nick, can you please reindent this block correctly as well as no splitting the html nodes on several lines Created attachment 79069 [details] [review] Bug 21364: (follow-up) Tidy indentation and remove copy/paste newlines 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." 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 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> (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? (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 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> 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. 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) 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 :( Good catch Severine, bit annoyed at myself for not spotting it myself. Seeing Failed QA pending a follow-up to resolve that oversight. 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 Created attachment 88393 [details] [review] Bug 21364: (follow-up) Check for standing status and uncertain prices (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 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! Created attachment 88787 [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 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 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> 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> 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. 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. 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> 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> Awesome work all! Pushed to master for 19.05 Enhancement will not be backported to 18.11.x series. Please, we need the "Add to basket" link accessible in one click! Owen, could you take a look at the discussion here? 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." I've moved the discussion about the buttons onto a new bug 23031. |