There should be a way to show the content of a basketgroup without reopening it.
I am working on that Mathieu
Created attachment 22021 [details] [review] [PATCH] Bug 9807 : Make possible to view a basketgroup without reopening it This patch make possible to view an individual closed basketgroup without reopening it. - It adds a new "View" button on closed basketgroup list - It creates a view for closed basketgroups, with 3 buttons (reopen, print, export) - It adds a "delete" button on standard "edit" view (for open basketgroup) To test : 1/ regression test : - create some empty basketgroups - create some basketgroups by closing baskets - in the list of basketgroups closed and opened, check you can use the buttons that existed before the patch (close and print, delete, export, print, reopen) - click on "Edit" to edit a opened basketgroup : check everything is like before : -- change the billing and delivery places, -- add a note, -- put some new baskets in the bg, -- remove baskets from it -- save it without checking "close" box => it must be save but kept open -- edit it again, and make other some changes (define a free delivery place for example) -- save it with checking "close" => it must be saved but closed 2/ new feature test - click on "view" button on top right column of some closed basketgroup - check all the displayed informations are correct (places, free place, note, list of baskets) - check you can not change anything - click on "print" button => check a pdf is created - click on "export" button => check a csv is created - click on "reopen" button => you must stay on the same basketgroup, but it is now open and you can make some changes - go back to the basketgroup list of the vendor. Check the reopened bg is in "open" tab - click on "edit" - click on new "delete" button => the bg must be deleted, and you are redirected to the bg list of the vendor.
Next step for improving basketgroup management will be Bug 10919 M. Saby
(In reply to mathieu saby from comment #3) > Next step for improving basketgroup management will be Bug 10919 > > M. Saby And make links from other pages (orders, baskets...) to a specific basket group when it is needed (it was not possible before this patch, because it reopened automatically the basketgroup...) M. Saby
Patch tested with a sandbox, by cedric.vita@dracenie.com <cedric.vita@dracenie.com>
Created attachment 22023 [details] [review] Bug 9807 : Make possible to view a basketgroup without reopening it This patch make possible to view an individual closed basketgroup without reopening it. - It adds a new "View" button on closed basketgroup list - It creates a view for closed basketgroups, with 3 buttons (reopen, print, export) - It adds a "delete" button on standard "edit" view (for open basketgroup) To test : 1/ regression test : - create some empty basketgroups - create some basketgroups by closing baskets - in the list of basketgroups closed and opened, check you can use the buttons that existed before the patch (close and print, delete, export, print, reopen) - click on "Edit" to edit a opened basketgroup : check everything is like before : -- change the billing and delivery places, -- add a note, -- put some new baskets in the bg, -- remove baskets from it -- save it without checking "close" box => it must be save but kept open -- edit it again, and make other some changes (define a free delivery place for example) -- save it with checking "close" => it must be saved but closed 2/ new feature test - click on "view" button on top right column of some closed basketgroup - check all the displayed informations are correct (places, free place, note, list of baskets) - check you can not change anything - click on "print" button => check a pdf is created - click on "export" button => check a csv is created - click on "reopen" button => you must stay on the same basketgroup, but it is now open and you can make some changes - go back to the basketgroup list of the vendor. Check the reopened bg is in "open" tab - click on "edit" - click on new "delete" button => the bg must be deleted, and you are redirected to the bg list of the vendor. Signed-off-by: cedric.vita@dracenie.com <cedric.vita@dracenie.com>
Thank you Pierre and Cedric! Mathieu
I am a little bit disconcerted. The aqbasketgroup.tt is really dirty and it is not easy to do something in it. But this patch is not really clear: 1/ I don't know if to add a lot of simple comment is useful (in the pl file). 2/ The view "show" is not clear in the code. Personally, I don't understand anything. 3/ There is some useless loop: [% FOREACH billingplaceloo IN billingplaceloop %] [% IF ( billingplaceloo.selected ) %] <input name="billingplace" id="billingplace" type ="hidden" value="[% billingplaceloo.value %]" />[% billingplaceloo.branchname %] [% END %] [% END %] It is not a logic way to display an input. 4/ There is a mix between new code and indentation fixes (use the -w option for git show). 5/ There is no "show" view for the opened bg. And for the closed bd, the "show" view is accessible on click on a button. Why there is no a simple "show" op and a link on the bg name? I let someone else from the QA team to give his/her opinion on this patch.
(In reply to Jonathan Druart from comment #8) > I am a little bit disconcerted. > The aqbasketgroup.tt is really dirty and it is not easy to do something in > it. => so true. it's horrible. And very hard to understand without reindenting... > But this patch is not really clear: > 1/ I don't know if to add a lot of simple comment is useful (in the pl file). It was usefull for me, to understand how the file was structured. But I can suppress comments if they are unneeded. > 4/ There is a mix between new code and indentation fixes (use the -w option > for git show). For me reindentation were logical, because I add a new condition in the template (l. 147: [% IF (closedbg) %] ), so I felt free to reindent For the other points, I cannot answer quickly, and must watch it closely. Mathieu
> 2/ The view "show" is not clear in the code. Personally, I don't understand > anything. Could you specify what you call the view "show" ? I don't find the "show" word in my patch. > 3/ There is some useless loop: > [% FOREACH billingplaceloo IN billingplaceloop %] > [% IF ( billingplaceloo.selected ) %] > <input name="billingplace" id="billingplace" type ="hidden" value="[% > billingplaceloo.value %]" />[% billingplaceloo.branchname %] > [% END %] > [% END %] > It is not a logic way to display an input. This unless loop was here before my patch. So I kept it here. It was not my goal to remove it. But it can be fixed later. Mathieu
The closedbg variable passed to template is used to decide if the bg must be only displayed, or make editable. By the way, I've remarked a typo : "closedgb" for "closedbg". I'm writing a followup. Mathieu
Created attachment 22781 [details] [review] [PATCH] Bug 9807 : Make possible to view a basketgroup without reopening it Main patch rebased (little conflict)
Created attachment 22782 [details] [review] [PATCH 2] Bug 9807 : Followup fixing variable name Fixing closedbg name in template
Jonathan, I will rewrite the patch without any reindenting. I think it will be more easy to understand for QA team. As reindenting is needed for code readabiliy, it will add a followup for it. Mathieu
Created attachment 22792 [details] [review] [PATCH ] Bug 9807 : Make possible to view a basketgroup without reopening it Patch rewritten to eliminate reindenting Should be more easy to understand. Mathieu
Created attachment 22793 [details] [review] [PATCH 2] Bug 9807 : Followup fixing variable name
Created attachment 22812 [details] [review] Bug 9807 : Make possible to view a basketgroup without reopening it Patch rewritten to eliminate reindenting This patch make possible to view an individual closed basketgroup without reopening it. - It adds a new "View" button on closed basketgroup list - It creates a view for closed basketgroups, with 3 buttons (reopen, print, export) - It adds a "delete" button on standard "edit" view (for open basketgroup) To test : 1/ regression test : - create some empty basketgroups - create some basketgroups by closing baskets - in the list of basketgroups closed and opened, check you can use the buttons that existed before the patch (close and print, delete, export, print, reopen) - click on "Edit" to edit a opened basketgroup : check everything is like before : -- change the billing and delivery places, -- add a note, -- put some new baskets in the bg, -- remove baskets from it -- save it without checking "close" box => it must be save but kept open -- edit it again, and make other some changes (define a free delivery place for example) -- save it with checking "close" => it must be saved but closed 2/ new feature test - click on "view" button on top right column of some closed basketgroup - check all the displayed informations are correct (places, free place, note, list of baskets) - check you can not change anything - click on "print" button => check a pdf is created - click on "export" button => check a csv is created - click on "reopen" button => you must stay on the same basketgroup, but it is now open and you can make some changes - go back to the basketgroup list of the vendor. Check the reopened bg is in "open" tab - click on "edit" - click on new "delete" button => the bg must be deleted, and you are redirected to the bg list of the vendor. Signed-off-by: cedric.vita@dracenie.com <cedric.vita@dracenie.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Passes koha-qa.pl, t and xt. Works as advertised.
Created attachment 22813 [details] [review] Bug 9807 : Followup fixing variable name Fixing closedbg name in template Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I would like to note that the pdf print view is a "interesting" but certainly no reason to not pass qa. I think a long term future goal should be to enable Template Toolkit for slips and notices, and then perhaps switch print views such as this to being html views configurable via slips and notices.
(In reply to Kyle M Hall from comment #19) > I would like to note that the pdf print view is a "interesting" but > certainly no reason to not pass qa. I think a long term future goal should > be to enable Template Toolkit for slips and notices, and then perhaps switch > print views such as this to being html views configurable via slips and > notices. Hi Not sure to understand your message. You mean generate the pdf from a HTML file, not with Perl PDF library? If so, yes it would be a good idea ;-) But my patch did not concern this part of code... Mathieu
Created attachment 22827 [details] [review] [PATCH 3] Bug 9807 : Reindent basketgroup template Subject: [PATCH 3/3] reindent basketgroup.tt Content-Type: text/plain; charset="utf-8" Basketgroup.tt is very hard to read This patch reindents it No visiable changes expected
Regression test : - check the list of opened and closed basketgroups for a vendor is displayed properly - check an individual basketgroup (closed and opened) is displayed properly - check you can edit a basketgroup and save changes - check you can print and export a basketgroup - check you can close and reopen a basketgroup The last attachment Needs signoff. The 2 firsts have passed QA M. Saby
Created attachment 23233 [details] [review] Bug 9807, followup: Reindent basketgroup.tt Basketgroup.tt is very hard to read. This patch simply reindents it. No visiable changes expected Regression test : - check the list of opened and closed basketgroups for a vendor is displayed properly - check an individual basketgroup (closed and opened) is displayed properly - check you can edit a basketgroup and save changes - check you can print and export a basketgroup - check you can close and reopen a basketgroup http://bugs.koha-community.org/show_bug.cgi?id=9807 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected.. the final patch really is just a indenting clean up. Patch still works, patches all still pass qa script and code looks much nicer.
Passing QA for that final followup. It's a template tidy up which doesn't affect operation. It'll affect blame as it change whitespace (which can be ignored by -w) but also breaks lines. I'm not sure that discussion ever came to an end, but I don't see why the initial two patches can't get pushed?
(In reply to Martin Renvoize from comment #25) > Passing QA for that final followup. It's a template tidy up which doesn't > affect operation. > > It'll affect blame as it change whitespace (which can be ignored by -w) but > also breaks lines. > > I'm not sure that discussion ever came to an end, but I don't see why the > initial two patches can't get pushed? Hello Discussion with whom? In fact, if someone really does not want this followup, of course I can suppress the patch. But I think the reindenting on this file was really needed and will make further changes easier ;-) Mathieu
Hi Mathieu, There were a few IRC converstaions and a thread on the mailing list about tidying the whole koha codebase at some point.. I think the final decision was to just do it as we go along instead of a massive change.. which is exactly what you've done here so is good.. I'de leave the patch in, now it's marked passed QA it'll pass by Galen attention and I'm sure he'll make a sensible decision either way. Martin
(In reply to Martin Renvoize from comment #27) > Hi Mathieu, > > There were a few IRC converstaions and a thread on the mailing list about > tidying the whole koha codebase at some point.. I think the final decision > was to just do it as we go along instead of a massive change.. which is > exactly what you've done here so is good.. > > I'de leave the patch in, now it's marked passed QA it'll pass by Galen > attention and I'm sure he'll make a sensible decision either way. > > Martin OK I remember having read - not to mix reindenting and new code in the same commit - not to do a patch only for reindenting So, as this bug changes some behavior in a template file, I think it is legitimate to add a followup for reindenting the file. Mathieu
Pushed to master. Thanks, Mathieu!