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
Created attachment 23740 [details] [review] [PATCH] Bug 11433 - Remove attachbasket op value in basket.pl and rename it in basketgroup.pl This patch cleans code in basket.pl, basketgroup.pl and basketgroup.tt Not absolutely needed, but makes further improvements easier '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. This patch - removes code related to 'attachbasket' in basket.pl - rename 'attachbasket' to 'save' in basketgroup.pl and basketgroup.tt - fixes some comments related to 'attachbasket' and 'mod_basket' in basketgroup.pl 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
See also bug 11056 for code cleaning on basketgroup.pl
Wanted to test, got "Merge conflict in acqui/basketgroup.pl"
Created attachment 25367 [details] [review] Bug 11433 - Remove attachbasket op value in basket.pl and rename it in basketgroup Patch rebased
Created attachment 27037 [details] [review] Bug 11433 - Remove attachbasket op value in basket.pl and rename it in basketgroup.pl This patch cleans code in basket.pl, basketgroup.pl and basketgroup.tt Not absolutely needed, but makes further improvements easier '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. This patch - removes code related to 'attachbasket' in basket.pl - rename 'attachbasket' to 'save' in basketgroup.pl and basketgroup.tt - fixes some comments related to 'attachbasket' and 'mod_basket' in basketgroup.pl 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: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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.
(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
(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.
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
> 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.
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.
OK for me. I rewrite the patch. mathieu
Created attachment 27648 [details] [review] [PATCH] Bug 11433 - Remove attachbasket op value in basket.pl and rename it in basketgroup.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 must be removed. This patch - removes code related to 'attachbasket' in basket.pl 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
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
Created attachment 27990 [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>
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>
Pushed to master. Thanks, Mathieu!