| Summary: | Simplify merge_ajax.pl | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
| Component: | Cataloging | Assignee: | Marcel de Rooy <m.de.rooy> |
| Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
| Severity: | minor | ||
| Priority: | P5 - low | CC: | jonathan.druart, m.de.rooy, mtompset, tomascohen |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: | http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14589 | ||
| Change sponsored?: | --- | Patch complexity: | Trivial patch |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: |
Bug 14588: Simplify merge_ajax.pl
Checked the code, looks good to me. I didn't notice any Bug 14588: Simplify merge_ajax.pl Bug 14588: Simplify merge_ajax.pl Bug 14588: Simplify merge_ajax.pl Bug 14588: Simplify merge_ajax.pl |
||
|
Description
Marcel de Rooy
2015-07-22 12:18:53 UTC
Created attachment 41126 [details] [review] Bug 14588: Simplify merge_ajax.pl This script does not need a few modules: IO::File; CGI::Session; C4::UploadedFile. Warnings can be enabled (with Modern::Perl). Indirect object syntax replaced for CGI and CGI::Cookie. (Note that Class::->method is preferred above Class->method. See perlobj.) Moved the $reply line upwards (not needed twice anymore). Test plan: [1] Log in as staff user. Run the URL cataloguing/merge_ajax.pl. You should see the JSON reponse in the browser. [2] Logout. Run the URL again. Blank screen. [3] Go to addbooks.pl (Cataloging Search). Search something, select two biblios and click Merge selected, etc. (In reply to Marcel de Rooy from comment #1) > ... (Note that > Class::->method is preferred above Class->method. See perlobj.) http://perldoc.perl.org/perlobj.html#Invoking-Class-Methods Interesting. I did not know this. Created attachment 41462 [details] [review] Checked the code, looks good to me. I didn't notice any changes in the behaviour of the merge functionality. Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> (In reply to Joonas Kylmälä from comment #3) > Signed-off-by: Joonas Kylmälä <j.kylmala@gmail.com> Thx Joonas (In reply to Marcel de Rooy from comment #1) >(Note that Class::->method is preferred above Class->method. See perlobj.) I did not find where it's written it's better. I personally think you are adding a third way where we already have new CGI and CGI->new... (In reply to Jonathan Druart from comment #5) > (In reply to Marcel de Rooy from comment #1) > >(Note that Class::->method is preferred above Class->method. See perlobj.) > > I did not find where it's written it's better. > I personally think you are adding a third way where we already have new CGI > and CGI->new... ThereIsMoreThanOneWayToDoIt The reference is few comments up, but for the rest it is completely unimportant. (In reply to Jonathan Druart from comment #5) > (In reply to Marcel de Rooy from comment #1) > >(Note that Class::->method is preferred above Class->method. See perlobj.) > > I did not find where it's written it's better. > I personally think you are adding a third way where we already have new CGI > and CGI->new... I agree that "better" is not the word used. However, it is expressly explicit in how Perl should interpret the call. There is something to be said positively for explicitness. "Because Perl allows you to use barewords for package names and subroutine names, it sometimes interprets a bareword's meaning incorrectly." http://perldoc.perl.org/perlobj.html#Invoking-Class-Methods Though, I do share Jonathan's view that CGI->new is sufficient. This explicit syntax is an example of confusing ugliness that I personally would prefer to avoid. If there is widespread concensus for removing this *ugly* construction, I will be happy to remove it quickly :) Created attachment 41670 [details] [review] Bug 14588: Simplify merge_ajax.pl This script does not need a few modules: IO::File; CGI::Session; C4::UploadedFile. Warnings can be enabled (with Modern::Perl). Indirect object syntax replaced for CGI and CGI::Cookie. (Note that Class::->method is preferred above Class->method. See perlobj.) Moved the $reply line upwards (not needed twice anymore). Test plan: [1] Log in as staff user. Run the URL cataloguing/merge_ajax.pl. You should see the JSON reponse in the browser. [2] Logout. Run the URL again. Blank screen. [3] Go to addbooks.pl (Cataloging Search). Search something, select two biblios and click Merge selected, etc. Signed-off-by: Joonas Kylmala <j.kylmala@gmail.com> Created attachment 41676 [details] [review] Bug 14588: Simplify merge_ajax.pl This script does not need a few modules: IO::File; CGI::Session; C4::UploadedFile. Warnings can be enabled (with Modern::Perl). Indirect object syntax replaced for CGI and CGI::Cookie. Moved the $reply line upwards (not needed twice anymore). Test plan: [1] Log in as staff user. Run the URL cataloguing/merge_ajax.pl. You should see the JSON reponse in the browser. [2] Logout. Run the URL again. Blank screen. [3] Go to addbooks.pl (Cataloging Search). Search something, select two biblios and click Merge selected, etc. Signed-off-by: Joonas Kylmala <j.kylmala@gmail.com> Created attachment 41683 [details] [review] Bug 14588: Simplify merge_ajax.pl This script does not need a few modules: IO::File; CGI::Session; C4::UploadedFile. Warnings can be enabled (with Modern::Perl). Indirect object syntax replaced for CGI and CGI::Cookie. Moved the $reply line upwards (not needed twice anymore). Test plan: [1] Log in as staff user. Run the URL cataloguing/merge_ajax.pl. You should see the JSON reponse in the browser. [2] Logout. Run the URL again. Blank screen. [3] Go to addbooks.pl (Cataloging Search). Search something, select two biblios and click Merge selected, etc. Signed-off-by: Joonas Kylmala <j.kylmala@gmail.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Created attachment 41707 [details] [review] Bug 14588: Simplify merge_ajax.pl This script does not need a few modules: IO::File; CGI::Session; C4::UploadedFile. Warnings can be enabled (with Modern::Perl). Indirect object syntax replaced for CGI and CGI::Cookie. Moved the $reply line upwards (not needed twice anymore). Test plan: [1] Log in as staff user. Run the URL cataloguing/merge_ajax.pl. You should see the JSON reponse in the browser. [2] Logout. Run the URL again. Blank screen. [3] Go to addbooks.pl (Cataloging Search). Search something, select two biblios and click Merge selected, etc. Signed-off-by: Joonas Kylmala <j.kylmala@gmail.com> Signed-off-by: Mark Tompsett <mtompset@hotmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Patch pushed to master. Thanks Marcel! |