Bug 23045 - Advanced cataloging editor (rancor) throws a JS error on incomplete/blank lines
Summary: Advanced cataloging editor (rancor) throws a JS error on incomplete/blank lines
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-04 13:41 UTC by Kelly McElligott
Modified: 2019-07-26 15:12 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.03


Attachments
Bug 23045: TextMarc errors on blank lines (2.27 KB, patch)
2019-06-04 14:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23045: TextMarc errors on blank lines (2.31 KB, patch)
2019-06-07 06:15 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 23045: Return record along with errors (1.19 KB, patch)
2019-06-07 10:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23045: TextMarc errors on blank lines (2.37 KB, patch)
2019-06-10 21:21 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23045: Return record along with errors (1.23 KB, patch)
2019-06-10 21:22 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 23045: TextMarc errors on blank lines (2.42 KB, patch)
2019-07-01 09:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23045: Return record along with errors (1.30 KB, patch)
2019-07-01 09:15 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kelly McElligott 2019-06-04 13:41:55 UTC
When running this macro (see below) with the helpers turned the JS is breaking.  Error: record.leader is not a function.  

It is possible the helper is calling the leader (possibly when it is deleted since part of the macro is delete the leader) and therefore the get material is breaking. 

delete 000
delete 005
delete 008
delete 245
new 000=      cam a22     Ii 4500
new 005=  
new 008=190320s2017    nyua     c    000 0 eng d

In 18.05.10 this is not performing this same way, however, bug 16251 was fixed and pushed to the next 18.05 version, and therefore this might be the cause of the breakage.
Comment 1 Nick Clemens 2019-06-04 14:37:01 UTC
Created attachment 90320 [details] [review]
Bug 23045: TextMarc errors on blank lines

Since bug 16251 we use TextMarc to get some record fields, however, it throws an error when
processing blank lines.

We should just pass on these rather then erroring

To test:
1 - Define a new macro that delete more fields than it adds
  delete 245
2 - Run it
3 - Note an error in JS console, and helpers will not load
4 - Apply patch
5 - Reload, try again
6 - No error
Comment 2 Claire Gravely 2019-06-07 06:15:39 UTC
Created attachment 90405 [details] [review]
Bug 23045: TextMarc errors on blank lines

Since bug 16251 we use TextMarc to get some record fields, however, it throws an error when
processing blank lines.

We should just pass on these rather then erroring

To test:
1 - Define a new macro that delete more fields than it adds
  delete 245
2 - Run it
3 - Note an error in JS console, and helpers will not load
4 - Apply patch
5 - Reload, try again
6 - No error

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Comment 3 Nick Clemens 2019-06-07 10:21:27 UTC
Created attachment 90411 [details] [review]
Bug 23045: Return record along with errors

We don't seem to directly handle the errors in most cases, let's always return the record

To test:
1 - Add some blank lines to editor, previous patch is okay with this
2 - Add a line with just 'e' or some other incomplete field
3 - Try to save
4 - Note JS leader error
5 - Apply this patch
6 - No errors
Comment 4 Liz Rea 2019-06-10 21:21:59 UTC
Created attachment 90466 [details] [review]
Bug 23045: TextMarc errors on blank lines

Since bug 16251 we use TextMarc to get some record fields, however, it throws an error when
processing blank lines.

We should just pass on these rather then erroring

To test:
1 - Define a new macro that delete more fields than it adds
  delete 245
2 - Run it
3 - Note an error in JS console, and helpers will not load
4 - Apply patch
5 - Reload, try again
6 - No error

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 5 Liz Rea 2019-06-10 21:22:07 UTC
Created attachment 90467 [details] [review]
Bug 23045: Return record along with errors

We don't seem to directly handle the errors in most cases, let's always return the record

To test:
1 - Add some blank lines to editor, previous patch is okay with this
2 - Add a line with just 'e' or some other incomplete field
3 - Try to save
4 - Note JS leader error
5 - Apply this patch
6 - No errors

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 6 Martin Renvoize 2019-07-01 09:15:43 UTC
Created attachment 91136 [details] [review]
Bug 23045: TextMarc errors on blank lines

Since bug 16251 we use TextMarc to get some record fields, however, it throws an error when
processing blank lines.

We should just pass on these rather then erroring

To test:
1 - Define a new macro that delete more fields than it adds
  delete 245
2 - Run it
3 - Note an error in JS console, and helpers will not load
4 - Apply patch
5 - Reload, try again
6 - No error

Signed-off-by: Claire Gravely <claire.gravely@bsz-bw.de>
Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2019-07-01 09:15:46 UTC
Created attachment 91137 [details] [review]
Bug 23045: Return record along with errors

We don't seem to directly handle the errors in most cases, let's always return the record

To test:
1 - Add some blank lines to editor, previous patch is okay with this
2 - Add a line with just 'e' or some other incomplete field
3 - Try to save
4 - Note JS leader error
5 - Apply this patch
6 - No errors

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2019-07-01 09:17:33 UTC
This works well and meets current QA requirements.. so I'm Passing QA..

but.. it really worries me that I can't find any tests for Rancor.. I'd really like to have this mini-app tested to lower the likelihood or regressions.  I'll open another bug to highlight this issue of missing tests.
Comment 9 Martin Renvoize 2019-07-01 15:19:36 UTC
Nice work!

Pushed to master for 19.11.00
Comment 10 Fridolin SOMERS 2019-07-25 12:04:28 UTC
Pushed to 19.05.x for 19.05.03
Comment 11 Lucas Gass 2019-07-26 15:12:40 UTC
backported to 18.11.x for 18.11.09