21.11 line 356 $op eq "edititem" * $current_item = Koha::Items->find($itemnumber)->unblessed; Master L465
Created attachment 144571 [details] [review] Add fallback redirect to additem op if \$current_item is undefined
Just nagging: Assignee not filled. QA tools warn: * Commit title does not start with 'Bug XXXXX: ' - 672a0dd335
+ $current_item = Koha::Items->find($itemnumber)->unblessed or undef; This does not work. The find will not return an opbject. So the unblessed still crashes.
+ if (!$current_item) { $nextop = "additem"; last; } What does this last do here? You dont exit a loop here..
Yeah, sorry. Fixin' it now.
Created attachment 144639 [details] [review] Bug 32418 - Can't call method "unblessed" on an undefined value at cataloguing/additem.pl Check whether result of Koha::Items->find is defined before invoking unblessed. If undef, redirect to additem op.
Created attachment 144640 [details] [review] Bug 32418: Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether result of Koha::Items->find is defined before invoking unblessed. If undef, redirect to additem op.
This time I actually tested it. Should work now. But still open for ideas.
Created attachment 144641 [details] Bug 32420: Set focus for cursor to barcode input box in batch item modification This should do it. Don't know whether it's compatible with the JS guidelines in the wiki. But I don't know whether it makes that much sense to move this block to one of the included scripts. To test: 1. Go to Cataloging > Batch item modification --> The barcodelist input field is now selected by default in the 'batch item modification' as well as the 'batch item deletion' view.
(In reply to Paul Derscheid from comment #9) > Created attachment 144641 [details] [review] > Bug 32420: Set focus for cursor to barcode input box in batch item > modification > > This should do it. Don't know whether it's compatible with the JS guidelines > in the wiki. > But I don't know whether it makes that much sense to move this block to > one of the included scripts. > > To test: > 1. Go to Cataloging > Batch item modification > --> The barcodelist input field is now selected by default in > the 'batch item modification' as well as the 'batch item deletion' view. Wrong bug !
+ if (!$current_item) { $nextop = "additem"; return; } Return ?
Created attachment 144643 [details] [review] Bug 32418 - Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether result of Koha::Items->find is defined before invoking unblessed. If undef, redirect to additem op. This uses an unlabeled block so we can add a guard clause to the 'dupeitem' case and exit early with the 'last' keyword.
This needs a test plan. I can reproduce by editing URL with a unkown itemnumber, ie : /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 (with KTD). But I think we should test the item existence at the beginning of the page. And display a message in TT, like in addbiblio.pl : https://git.koha-community.org/Koha-community/Koha/src/commit/0a934194ca25e2614912f9248f55822aebf38fb8/cataloguing/addbiblio.pl#L771
Created attachment 144794 [details] [review] Bug 32418 - Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether the current op is edititem or dupeitem and if so check whether the itemnumber supplied as a query param actually exists. If it doesn't, redirect to the additem op and hide all UI elements except for a dialog that gives options to add a new item to the record or to view the records holdings. This behaviour was adapted from the addbiblio view, as suggested by Fridolin. To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off
Great code Paul ;) If I may : I didnt saw there is a method output_and_exit_if_error() that manages incorrect patron + biblio. Maybe it would be nice to move you check on item in it ? Is uses include blocking_errors.inc that already contains "unknown_item" code, see : https://git.koha-community.org/Koha-community/Koha/src/commit/9d6d641d1f8b77271800f43bc027b651f9aea52b/circ/bookcount.pl#L50
Created attachment 144812 [details] [review] Bug 32418 - Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether the current op is edititem or dupeitem and if so check whether the itemnumber supplied as a query param actually exists. If the item is not defined we output_and_exit and save the time to execute the rest of the controller code. Additionally we now don't have to process the whole template anymore. The only caveat is that we now have the suggested actions above the actual error message. Still an improvement. though. Again, thanks Frido. Hope, that this is what you had in mind. To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off
I'm getting a sha1 error when trying to apply the patch to test (I was using interactive and only applying the latest patch). I think the first patch needs obsoleting. Message when trying to apply the patch: git bz apply 32418 Bug 32418 - CRASH: Can't call method "unblessed" on an undefined value at cataloguing/additem.pl 144794 - Bug 32418 - Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl 144812 - Bug 32418 - Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Apply? [(y)es, (n)o, (i)nteractive] i Applying: Bug 32418 - Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl error: sha1 information is lacking or useless (cataloguing/additem.pl). error: could not build fake ancestor Patch failed at 0001 Bug 32418 - Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl
The second patch relies on changes in the first. Works correctly if you apply both.
(In reply to Paul Derscheid from comment #18) > The second patch relies on changes in the first. > Works correctly if you apply both. Thanks Paul. I have done that now and will sign off. I was a bit confused as the patch titles and test plan were the same/almost identical, and I thought (without actually looking at the patches) that they were the same. You may want to amend the patches for the QA Team so that the subject line is in the format Bug XXXXX: Bug title, as per https://wiki.koha-community.org/wiki/Commit_messages
Created attachment 145306 [details] [review] Bug 32418 - Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether the current op is edititem or dupeitem and if so check whether the itemnumber supplied as a query param actually exists. If it doesn't, redirect to the additem op and hide all UI elements except for a dialog that gives options to add a new item to the record or to view the records holdings. This behaviour was adapted from the addbiblio view, as suggested by Fridolin. To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off Signed-off-by: David Nind <david@davidnind.com>
Created attachment 145307 [details] [review] Bug 32418 - Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether the current op is edititem or dupeitem and if so check whether the itemnumber supplied as a query param actually exists. If the item is not defined we output_and_exit and save the time to execute the rest of the controller code. Additionally we now don't have to process the whole template anymore. The only caveat is that we now have the suggested actions above the actual error message. Still an improvement. though. Again, thanks Frido. Hope, that this is what you had in mind. To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off Signed-off-by: David Nind <david@davidnind.com>
Two patches with the same commit name. Will squash them.
Created attachment 145505 [details] [review] Bug 32418 - Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether the current op is edititem or dupeitem and if so check whether the itemnumber supplied as a query param actually exists. If it doesn't, redirect to the additem op and hide all UI elements except for a dialog that gives options to add a new item to the record or to view the records holdings. This behaviour was adapted from the addbiblio view, as suggested by Fridolin. To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off Signed-off-by: David Nind <david@davidnind.com> Bug 32418 - Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether the current op is edititem or dupeitem and if so check whether the itemnumber supplied as a query param actually exists. If the item is not defined we output_and_exit and save the time to execute the rest of the controller code. Additionally we now don't have to process the whole template anymore. The only caveat is that we now have the suggested actions above the actual error message. Still an improvement. though. Again, thanks Frido. Hope, that this is what you had in mind. To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off Signed-off-by: David Nind <david@davidnind.com>
FAIL koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt FAIL filters wrong_html_filter at line 71 ( <a href="/cgi-bin/koha/cataloguing/additem.pl?biblionumber=[% biblio.biblionumber %]">Add a new item</a> or <a href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% biblio.biblionumber %]#holdings">go to the records holdings</a>.) missing_filter at line 71 ( <a href="/cgi-bin/koha/cataloguing/additem.pl?biblionumber=[% biblio.biblionumber %]">Add a new item</a> or <a href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% biblio.biblionumber %]#holdings">go to the records holdings</a>.) missing_filter at line 71 ( <a href="/cgi-bin/koha/cataloguing/additem.pl?biblionumber=[% biblio.biblionumber %]">Add a new item</a> or <a href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% biblio.biblionumber %]#holdings">go to the records holdings</a>.)
Processing additional checks * Commit title does not start with 'Bug XXXXX: ' - 94c83e2a74
(In reply to Paul Derscheid from comment #18) > The second patch relies on changes in the first. > Works correctly if you apply both. In this cas you need to add "(follow-up)" in second patch. See for example Bug 32283 More there https://wiki.koha-community.org/wiki/Commit_messages With 1rst rule being : Bug XXX - blabla => Bug XXX: blabla
(In reply to Marcel de Rooy from comment #24) > FAIL koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt > FAIL filters > wrong_html_filter at line 71 ( <a > href="/cgi-bin/koha/cataloguing/additem.pl?biblionumber=[% > biblio.biblionumber %]">Add a new item</a> or <a > href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% biblio.biblionumber > %]#holdings">go to the records holdings</a>.) > missing_filter at line 71 ( <a > href="/cgi-bin/koha/cataloguing/additem.pl?biblionumber=[% > biblio.biblionumber %]">Add a new item</a> or <a > href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% biblio.biblionumber > %]#holdings">go to the records holdings</a>.) > missing_filter at line 71 ( <a > href="/cgi-bin/koha/cataloguing/additem.pl?biblionumber=[% > biblio.biblionumber %]">Add a new item</a> or <a > href="/cgi-bin/koha/catalogue/detail.pl?biblionumber=[% biblio.biblionumber > %]#holdings">go to the records holdings</a>.) You need to add Template Toolkit filter for URI : /cgi-bin/koha/catalogue/detail.pl?biblionumber=[% biblio.biblionumber | uri %]
(In reply to Fridolin Somers from comment #15) > Great code Paul ;) > > If I may : > > I didnt saw there is a method output_and_exit_if_error() that manages > incorrect patron + biblio. > Maybe it would be nice to move you check on item in it ? > > Is uses include blocking_errors.inc that already contains "unknown_item" > code, see : > > https://git.koha-community.org/Koha-community/Koha/src/commit/ > 9d6d641d1f8b77271800f43bc027b651f9aea52b/circ/bookcount.pl#L50 Forget about that ;) The actual code is close to the code in addbiblio.tt that's perfect.
Created attachment 146944 [details] [review] Bug 32418: Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether the current op is edititem or dupeitem and if so check whether the itemnumber supplied as a query param actually exists. If it doesn't, redirect to the additem op and hide all UI elements except for a dialog that gives options to add a new item to the record or to view the records holdings. This behaviour was adapted from the addbiblio view, as suggested by Fridolin. To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off
Is this ready for retesting?
Hi Katrin, this is ready for retesting.
Created attachment 149092 [details] [review] Bug 32418: Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether the current op is edititem or dupeitem and if so check whether the itemnumber supplied as a query param actually exists. If it doesn't, redirect to the additem op and hide all UI elements except for a dialog that gives options to add a new item to the record or to view the records holdings. This behaviour was adapted from the addbiblio view, as suggested by Fridolin. To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off Signed-off-by: David Nind <david@davidnind.com>
* We don't need to hide nodes in the template * The links we display in the template does not work (we do not send the biblio to the template as we exit early)
Created attachment 149591 [details] [review] Bug 32418: (follow-up) Remove jQuery, Fix typos in url, add biblio as template param To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off
Created attachment 149626 [details] [review] Bug 32418: Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether the current op is edititem or dupeitem and if so check whether the itemnumber supplied as a query param actually exists. If it doesn't, redirect to the additem op and hide all UI elements except for a dialog that gives options to add a new item to the record or to view the records holdings. This behaviour was adapted from the addbiblio view, as suggested by Fridolin. To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off Signed-off-by: David Nind <david@davidnind.com>
Created attachment 149627 [details] [review] Bug 32418: (follow-up) Remove jQuery, Fix typos in url, add biblio as template param To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off Signed-off-by: David Nind <david@davidnind.com>
Created attachment 149635 [details] [review] Bug 32418: Can't call method 'unblessed' on an undefined value at cataloguing/additem.pl Check whether the current op is edititem or dupeitem and if so check whether the itemnumber supplied as a query param actually exists. If it doesn't, redirect to the additem op and hide all UI elements except for a dialog that gives options to add a new item to the record or to view the records holdings. This behaviour was adapted from the addbiblio view, as suggested by Fridolin. To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 149636 [details] [review] Bug 32418: (follow-up) Remove jQuery, Fix typos in url, add biblio as template param To test: 1. Go to any record 2. Hit the edit button on an item in the holdings table 3. Modify the URL so that the query param for the itemnumber is either 3.1 empty: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber= 3.2 an itemnumber that doesn't exist: /cgi-bin/koha/cataloguing/additem.pl?op=edititem&biblionumber=384&itemnumber=9999999999 3.3 whatever else you come up with.. 4. Check the same thing for the dupe option (op=dupeitem) 5. Sign off Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 149637 [details] [review] Bug 32418: Remove useless statement Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work everyone! Pushed to stable for 22.11.x
Doesn't apply cleanly to 22.05.x, no backport.