Bug 11433 - Remove attachbasket op value in basket.pl
Summary: Remove attachbasket op value in basket.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Mathieu Saby
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-20 22:27 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 11433 - Remove attachbasket op value in basket.pl and rename it in basketgroup.pl (6.55 KB, patch)
2013-12-20 22:36 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 11433 - Remove attachbasket op value in basket.pl and rename it in basketgroup (6.52 KB, patch)
2014-02-18 11:49 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 11433 - Remove attachbasket op value in basket.pl and rename it in basketgroup.pl (6.57 KB, patch)
2014-04-11 17:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PATCH] Bug 11433 - Remove attachbasket op value in basket.pl and rename it in basketgroup.pl (1.83 KB, patch)
2014-04-26 12:27 UTC, Mathieu Saby
Details | Diff | Splinter Review
Bug 11433: (code cleanup) remove unused 'attachbasket' op value in basket.pl (1.81 KB, patch)
2014-05-06 16:37 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 11433: (code cleanup) remove unused 'attachbasket' op value in basket.pl (1.92 KB, patch)
2014-05-07 14:57 UTC, Marcel de Rooy
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-12-20 22:27:29 UTC
Code cleaning:

'attachbasket' is used in basket.pl and basketgroup.pl as a value of $op

In basket.pl, some code is supposed to be executed if $op eq 'attachbasket'. But it is never the case (grep attachbasket * -r), so this condition must be removed.

In basketgroup.pl, the description of 'attachbasket' value in comments is wrong, and the name is misleading (this value of $op is ONLY used when someone clic on 'Save' button on a basketgroup page). So 'attachbasket' should be renamed.

M. Saby
Comment 1 Mathieu Saby 2013-12-20 22:36:43 UTC Comment hidden (obsolete)
Comment 2 Mathieu Saby 2013-12-20 22:38:21 UTC
See also bug 11056 for code cleaning on basketgroup.pl
Comment 3 Marc Véron 2014-02-17 18:10:31 UTC
Wanted to test, got "Merge conflict in acqui/basketgroup.pl"
Comment 4 Mathieu Saby 2014-02-18 11:49:08 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2014-04-11 17:10:48 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2014-04-16 11:06:19 UTC
I'm not sure "save" is more understandable than "attachbasket". This script does a lot of things ($op can be "delete", "reopen", "mod_basket", etc.).
In my opinion, "save" is not really meaningful.
Comment 7 Mathieu Saby 2014-04-18 18:13:48 UTC
(In reply to Jonathan Druart from comment #6)
> I'm not sure "save" is more understandable than "attachbasket". This script
> does a lot of things ($op can be "delete", "reopen", "mod_basket", etc.).
> In my opinion, "save" is not really meaningful.

To be sure I understand your comment : do you agree that "attachbasket" is not a very good name, as it does not trigger the attachment of a basket?
Maybe "save_bg" could be more precise than just "save"?

Mathieu
Comment 8 Marcel de Rooy 2014-04-18 19:49:12 UTC
(In reply to mathieu saby from comment #7)
> (In reply to Jonathan Druart from comment #6)
> > I'm not sure "save" is more understandable than "attachbasket". This script
> > does a lot of things ($op can be "delete", "reopen", "mod_basket", etc.).
> > In my opinion, "save" is not really meaningful.
> 
> To be sure I understand your comment : do you agree that "attachbasket" is
> not a very good name, as it does not trigger the attachment of a basket?
> Maybe "save_bg" could be more precise than just "save"?
> 
> Mathieu

The problem is this op does two different things depending on basketgroupid. We have actually two operations: 1) save basketgroup, 2) create new basketgroup for closed basket.
attachbasket refers to point 2: attaching a basket to a new group. Your new name refers to point 1.
I would not recommend save_bg.
Comment 9 Mathieu Saby 2014-04-19 07:40:22 UTC
Well, I suppose you are both right.

But the the other point of the patch was to suppress "attachbasket" code from basket.pl, which was never executed.
Do you agree with that?
If so, I can make a followup to limit the patch to this 1st point.


Mathieu
Comment 10 Marcel de Rooy 2014-04-21 14:47:12 UTC
> But the the other point of the patch was to suppress "attachbasket" code from 
> basket.pl, which was never executed.
> Do you agree with that?
> If so, I can make a followup to limit the patch to this 1st point.

I signed off. So I agreed with the patch in its current state. The name of the operation does not seem so important that we should still put more effort in this dev (at least imo).
I add Katrin to the CC list. Katrin: Could you decide if this can pass qa in its current form? Thanks.
Comment 11 Katrin Fischer 2014-04-23 10:22:16 UTC
Maybe the best for now would be removing the unused code and keep this bug very specific. Discussion about how to improve the remaining code and which ops we should have, could then continue on a different bug.
Comment 12 Mathieu Saby 2014-04-26 12:11:48 UTC
OK for me. I rewrite the patch.
mathieu
Comment 13 Mathieu Saby 2014-04-26 12:27:27 UTC Comment hidden (obsolete)
Comment 14 Mathieu Saby 2014-04-26 12:29:39 UTC
I rewrote the patch. It now only removes useless code.
As it is no more the same patch, I changed its name, and removed Marcel's sign off (maybe I could have kept it?).

Mathieu
Comment 15 Galen Charlton 2014-05-06 16:37:52 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2014-05-07 14:57:42 UTC
Created attachment 28024 [details] [review]
Bug 11433: (code cleanup) remove unused 'attachbasket' op value in basket.pl

This patch cleans code in basket.pl

In basket.pl, some code is supposed to be executed if
$op eq 'attachbasket'. But it is never the case
(grep attachbasket * -r), so this condition can be removed.

No functional change expected.

Regression test only :
* Make a complete acquisition process, from the creation of a basket
 to the closure of a basketgroup, and check everything is OK
* On a basket page, try to change the basketgroup it belongs to, and
  check everything is OK
* On a basketgroup page, try to edit the content of a basketgroup (put
  a new basket in it, change the deliverybranch...), and check everything
  is OK
* On a basketgroup page, try to reopen a closed basketgroup, and close an
  open basketgroup, and check everything is OK

Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Galen Charlton 2014-05-07 16:05:04 UTC
Pushed to master.  Thanks, Mathieu!