[Bug 11433] New: Remove attachbasket op value in basket.pl and rename it in basketgroup.pl
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 Bug ID: 11433 Summary: Remove attachbasket op value in basket.pl and rename it in basketgroup.pl Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Acquisitions Assignee: koha-bugs@lists.koha-community.org Reporter: mathieu.saby@univ-rennes2.fr QA Contact: testopia@bugs.koha-community.org 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 --- Comment #1 from mathieu saby <mathieu.saby@univ-rennes2.fr> --- Created attachment 23740 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23740&action=edit [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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 mathieu saby <mathieu.saby@univ-rennes2.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |mathieu.saby@univ-rennes2.f |ity.org |r -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 mathieu saby <mathieu.saby@univ-rennes2.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 mathieu saby <mathieu.saby@univ-rennes2.fr> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 --- Comment #2 from mathieu saby <mathieu.saby@univ-rennes2.fr> --- See also bug 11056 for code cleaning on basketgroup.pl -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |veron@veron.ch --- Comment #3 from Marc Véron <veron@veron.ch> --- Wanted to test, got "Merge conflict in acqui/basketgroup.pl" -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 mathieu saby <mathsabypro@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23740|0 |1 is obsolete| | --- Comment #4 from mathieu saby <mathsabypro@gmail.com> --- Created attachment 25367 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25367&action=edit Bug 11433 - Remove attachbasket op value in basket.pl and rename it in basketgroup Patch rebased -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 mathieu saby <mathsabypro@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #25367|0 |1 is obsolete| | --- Comment #5 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 27037 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=27037&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |m.de.rooy@rijksmuseum.nl Patch complexity|Trivial patch |Small patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m --- Comment #6 from Jonathan Druart <jonathan.druart@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 --- Comment #7 from mathieu saby <mathsabypro@gmail.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 --- Comment #8 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 --- Comment #9 from mathieu saby <mathsabypro@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #10 from M. de Rooy <m.de.rooy@rijksmuseum.nl> ---
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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 --- Comment #11 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 --- Comment #12 from mathieu saby <mathsabypro@gmail.com> --- OK for me. I rewrite the patch. mathieu -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 mathieu saby <mathsabypro@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #27037|0 |1 is obsolete| | --- Comment #13 from mathieu saby <mathsabypro@gmail.com> --- Created attachment 27648 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=27648&action=edit [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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 mathieu saby <mathsabypro@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 mathieu saby <mathsabypro@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Remove attachbasket op |Remove attachbasket op |value in basket.pl and |value in basket.pl |rename it in basketgroup.pl | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 --- Comment #14 from mathieu saby <mathsabypro@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #27648|0 |1 is obsolete| | --- Comment #15 from Galen Charlton <gmcharlt@gmail.com> --- Created attachment 27990 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=27990&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #27990|0 |1 is obsolete| | --- Comment #16 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 28024 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=28024&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11433 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |gmcharlt@gmail.com --- Comment #17 from Galen Charlton <gmcharlt@gmail.com> --- Pushed to master. Thanks, Mathieu! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org