Bug 9807 - There should be a way to show the content of a basketgroup without reopening it
Summary: There should be a way to show the content of a basketgroup without reopening it
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Mathieu Saby
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 9654
  Show dependency treegraph
 
Reported: 2013-03-13 13:08 UTC by Mathieu Saby
Modified: 2014-12-07 20:07 UTC (History)
5 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
[PATCH] Bug 9807 : Make possible to view a basketgroup without reopening it (29.92 KB, patch)
2013-10-16 22:40 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 9807 : Make possible to view a basketgroup without reopening it (29.94 KB, patch)
2013-10-17 08:53 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
[PATCH] Bug 9807 : Make possible to view a basketgroup without reopening it (29.79 KB, patch)
2013-11-07 14:48 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PATCH 2] Bug 9807 : Followup fixing variable name (1.44 KB, patch)
2013-11-07 14:49 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PATCH ] Bug 9807 : Make possible to view a basketgroup without reopening it (24.84 KB, patch)
2013-11-07 19:57 UTC, Mathieu Saby
Details | Diff | Splinter Review
[PATCH 2] Bug 9807 : Followup fixing variable name (1.44 KB, patch)
2013-11-07 20:03 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 9807 : Make possible to view a basketgroup without reopening it (24.95 KB, patch)
2013-11-08 12:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9807 : Followup fixing variable name (1.46 KB, patch)
2013-11-08 12:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PATCH 3] Bug 9807 : Reindent basketgroup template (37.29 KB, patch)
2013-11-08 15:20 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 9807, followup: Reindent basketgroup.tt (37.38 KB, patch)
2013-12-02 12:09 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mathieu Saby 2013-03-13 13:08:30 UTC
There should be a way to show the content of a basketgroup without reopening it.
Comment 1 Mathieu Saby 2013-10-10 12:37:50 UTC
I am working on that
Mathieu
Comment 2 Mathieu Saby 2013-10-16 22:40:21 UTC Comment hidden (obsolete)
Comment 3 Mathieu Saby 2013-10-17 07:38:20 UTC
Next step for improving basketgroup management will be Bug 10919

M. Saby
Comment 4 Mathieu Saby 2013-10-17 07:40:09 UTC
(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
Comment 5 Biblibre Sandboxes 2013-10-17 08:53:13 UTC
Patch tested with a sandbox, by cedric.vita@dracenie.com <cedric.vita@dracenie.com>
Comment 6 Biblibre Sandboxes 2013-10-17 08:53:32 UTC Comment hidden (obsolete)
Comment 7 Mathieu Saby 2013-10-17 09:25:41 UTC
Thank you Pierre and Cedric!

Mathieu
Comment 8 Jonathan Druart 2013-11-07 14:10:14 UTC
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.
Comment 9 Mathieu Saby 2013-11-07 14:30:38 UTC
(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
Comment 10 Mathieu Saby 2013-11-07 14:34:32 UTC
> 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
Comment 11 Mathieu Saby 2013-11-07 14:44:29 UTC
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
Comment 12 Mathieu Saby 2013-11-07 14:48:14 UTC Comment hidden (obsolete)
Comment 13 Mathieu Saby 2013-11-07 14:49:51 UTC Comment hidden (obsolete)
Comment 14 Mathieu Saby 2013-11-07 19:06:59 UTC
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
Comment 15 Mathieu Saby 2013-11-07 19:57:16 UTC Comment hidden (obsolete)
Comment 16 Mathieu Saby 2013-11-07 20:03:11 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2013-11-08 12:42:41 UTC
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.
Comment 18 Kyle M Hall 2013-11-08 12:42:54 UTC
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>
Comment 19 Kyle M Hall 2013-11-08 12:44:59 UTC
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.
Comment 20 Mathieu Saby 2013-11-08 12:50:47 UTC
(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
Comment 21 Mathieu Saby 2013-11-08 15:20:04 UTC Comment hidden (obsolete)
Comment 22 Mathieu Saby 2013-11-08 15:20:49 UTC
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
Comment 23 Martin Renvoize 2013-12-02 12:09:33 UTC
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>
Comment 24 Martin Renvoize 2013-12-02 12:11:17 UTC
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.
Comment 25 Martin Renvoize 2013-12-02 12:58:08 UTC
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?
Comment 26 Mathieu Saby 2013-12-02 13:10:17 UTC
(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
Comment 27 Martin Renvoize 2013-12-02 13:17:14 UTC
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
Comment 28 Mathieu Saby 2013-12-02 13:20:09 UTC
(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
Comment 29 Galen Charlton 2013-12-17 17:41:01 UTC
Pushed to master.  Thanks, Mathieu!