Bug 34182

Summary:	AddBiblio shouldn't set biblio.serial based on biblio.seriestitle
Product:	Koha	Reporter:	Phil Ringnalda <phil>
Component:	Cataloging	Assignee:	Phil Ringnalda <phil>
Status:	RESOLVED FIXED	QA Contact:	Martin Renvoize (ashimema) <martin.renvoize>
Severity:	normal
Priority:	P5 - low	CC:	lucas, m.de.rooy, martin.renvoize
Version:	Main
Hardware:	All
OS:	All
See Also:	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34128
Change sponsored?:	---	Patch complexity:	Trivial patch
Documentation contact:		Documentation submission:
Text to go in the release notes:		Version(s) released in:	23.11.00,23.05.02,22.11.08,22.05.16
Circulation function:
Attachments:	Bug 34182 - AddBiblio shouldn't set biblio.serial based on biblio.seriestitle Bug 34182 - AddBiblio shouldn't set biblio.serial based on biblio.seriestitle Bug 34182: Don't set biblio.serial based on biblio.seriestitle in AddBiblio

Description Phil Ringnalda 2023-07-01 05:32:07 UTC

Back in the 3.0 days, in https://git.koha-community.org/Koha-community/Koha/commit/af466ca41ab02150f4f6c60bf6262b079b2e6e85 someone looked at the dead code in https://git.koha-community.org/Koha-community/Koha/blame/commit/c6013f3fd09ac5bd02f31e86bcc932c265c58502/C4/Biblio.pm#L3288

my $series       = 0;

if ( $biblio->{'seriestitle'} ) { $series = 1 }

which did nothing because there wasn't any biblio.series for $series to be setting (maybe there had been at one time, maybe it was always dead code, dunno), and decided to "fix" it by changing it to

   # set the series flag
my $serial = 0;
if ( $biblio->{'seriestitle'} ) { $serial = 1 };

which turned it into actively wrong code, overriding someone actively setting serial with whether or not it had a series title. A few years later, someone at least let setting something mapped to biblio.serial cause biblio.serial to be set, while leaving the bogus setting of it based on seriestitle still there if serial wasn't already set.

The added comment "# set the series flag" makes it clear that it wasn't done out of any belief that seriestitle implies serial, only thinking that serial was how you spell "has a series."

As bug 34128 will tell you, it isn't. And since CheckPrevCheckout uses biblio.serial to only warn people if they have previously checked out a particular copy of People Weekly rather than warning them that they checked out last week's issue so they don't want this week's issue, had it noticed that as a result of seriestitle -> serial it doesn't warn people who've checked out copy 1 of the 62nd Stone Barrington novel that they probably don't want to check out copy 2, CheckPrevCheckout would also tell you that it isn't.

For comic relief, AddBiblio only sets biblio.serial the first time that you save a biblio which has a seriestitle. If you edit the biblio again, ModBiblio unsets it, so it's both wrong and incapable of making its wrongness stick.

Comment 1 Phil Ringnalda 2023-07-01 16:30:18 UTC

Created attachment 152921 [details] [review]
Bug 34182 - AddBiblio shouldn't set biblio.serial based on biblio.seriestitle

A misunderstanding of the intention of some dead code that probably wanted
to set biblio.series (which doesn't exist) left us setting biblio.serial
if biblio.seriestitle was set. The only thing series and serial have in
common is the first four letters. We shouldn't set serial on something
with a series (unless someone also sets serial on it, of course).

Test plan:
 1. Administration - MARC bibliographic framework - Actions button next to
    Default framework - MARC structure
 2. In the Search for tag input type 942 and click search
 3. Actions button next to 942 - Edit subfields
 4. Tab s - check the checkbox for Editor, uncheck the checkbox for
    Collapsed - Save changes
 5. Cataloging - New record
 6. Click in the input for 000 and hold down Tab until you get past 008
    to fill in mandatory default values, then type any character in 040
    subfield c
 7. Tab 2 - In 245 subfield a type Series not serial
 8. Tab 4 - In 490 subfield a type any character
 9. Tab 9 - Set the value of subfield c to Books
10. Click save and leave the tab open to keep the biblionumber
11. Cataloging - New record - repeat step 6
12. Tab 2 - In 245 subfield a type Serial not series
13. Tab 9 - Set the value of subfield c to Books - Type 1 in subfield s
14. Click save, the biblionumber should be one higher than the first one
15. Reports - Create from SQL
16. Type something in Report name, paste in the SQL
SELECT biblio.serial, biblio.seriestitle, biblio.title FROM biblio WHERE
biblionumber IN ("","")
    and put your first biblionumber in the first "", your second in the
    second.
17. Save report - Run report
18. Series not serial should have a blank in the serial column and the
   character you typed in the seriestitle column; Serial not series
   should have a 1 in the serial column and a blank in the seriestitle
   column.

Comment 2 Martin Renvoize (ashimema) 2023-07-03 09:23:10 UTC

Created attachment 152923 [details] [review]
Bug 34182 - AddBiblio shouldn't set biblio.serial based on biblio.seriestitle

A misunderstanding of the intention of some dead code that probably wanted
to set biblio.series (which doesn't exist) left us setting biblio.serial
if biblio.seriestitle was set. The only thing series and serial have in
common is the first four letters. We shouldn't set serial on something
with a series (unless someone also sets serial on it, of course).

Test plan:
 1. Administration - MARC bibliographic framework - Actions button next to
    Default framework - MARC structure
 2. In the Search for tag input type 942 and click search
 3. Actions button next to 942 - Edit subfields
 4. Tab s - check the checkbox for Editor, uncheck the checkbox for
    Collapsed - Save changes
 5. Cataloging - New record
 6. Click in the input for 000 and hold down Tab until you get past 008
    to fill in mandatory default values, then type any character in 040
    subfield c
 7. Tab 2 - In 245 subfield a type Series not serial
 8. Tab 4 - In 490 subfield a type any character
 9. Tab 9 - Set the value of subfield c to Books
10. Click save and leave the tab open to keep the biblionumber
11. Cataloging - New record - repeat step 6
12. Tab 2 - In 245 subfield a type Serial not series
13. Tab 9 - Set the value of subfield c to Books - Type 1 in subfield s
14. Click save, the biblionumber should be one higher than the first one
15. Reports - Create from SQL
16. Type something in Report name, paste in the SQL
SELECT biblio.serial, biblio.seriestitle, biblio.title FROM biblio WHERE
biblionumber IN ("","")
    and put your first biblionumber in the first "", your second in the
    second.
17. Save report - Run report
18. Series not serial should have a blank in the serial column and the
   character you typed in the seriestitle column; Serial not series
   should have a 1 in the serial column and a blank in the seriestitle
   column.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Comment 3 Martin Renvoize (ashimema) 2023-07-03 09:25:28 UTC

Thanks so much for the digging here Phil, great bit of coding forensics there.

This makes eminent sense now giveny our explainations and the challenges we've been having over on bug 34128.

Signing off.

Comment 4 Katrin Fischer 2023-07-09 14:55:45 UTC

Thanks from me too, Phil! I had a hunch something was wrong, but you did the hard part of proving and fixing!

Comment 5 Katrin Fischer 2023-07-09 14:59:42 UTC

Created attachment 153225 [details] [review]
Bug 34182: Don't set biblio.serial based on biblio.seriestitle in AddBiblio

A misunderstanding of the intention of some dead code that probably wanted
to set biblio.series (which doesn't exist) left us setting biblio.serial
if biblio.seriestitle was set. The only thing series and serial have in
common is the first four letters. We shouldn't set serial on something
with a series (unless someone also sets serial on it, of course).

Test plan:
 1. Administration - MARC bibliographic framework - Actions button next to
    Default framework - MARC structure
 2. In the Search for tag input type 942 and click search
 3. Actions button next to 942 - Edit subfields
 4. Tab s - check the checkbox for Editor, uncheck the checkbox for
    Collapsed - Save changes
 5. Cataloging - New record
 6. Click in the input for 000 and hold down Tab until you get past 008
    to fill in mandatory default values, then type any character in 040
    subfield c
 7. Tab 2 - In 245 subfield a type Series not serial
 8. Tab 4 - In 490 subfield a type any character
 9. Tab 9 - Set the value of subfield c to Books
10. Click save and leave the tab open to keep the biblionumber
11. Cataloging - New record - repeat step 6
12. Tab 2 - In 245 subfield a type Serial not series
13. Tab 9 - Set the value of subfield c to Books - Type 1 in subfield s
14. Click save, the biblionumber should be one higher than the first one
15. Reports - Create from SQL
16. Type something in Report name, paste in the SQL
SELECT biblio.serial, biblio.seriestitle, biblio.title FROM biblio WHERE
biblionumber IN ("","")
    and put your first biblionumber in the first "", your second in the
    second.
17. Save report - Run report
18. Series not serial should have a blank in the serial column and the
   character you typed in the seriestitle column; Serial not series
   should have a 1 in the serial column and a blank in the seriestitle
   column.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Comment 6 Katrin Fischer 2023-07-09 14:59:51 UTC

It would be nice if we could fix data with wrongly set info now somehow, but we might as well do that on a different bug and get this in ASAP.

Maybe one of the existing scripts (touch.../rebuild...) could do the trick even?

Comment 7 Tomás Cohen Arazi (tcohen) 2023-07-14 18:24:13 UTC

Pushed to master for 23.11.

Nice work everyone, thanks!

Comment 8 Martin Renvoize (ashimema) 2023-07-18 11:52:20 UTC

Thanks for all the hard work!

Pushed to 23.05.x for the next release

Comment 9 Matt Blenkinsop 2023-07-18 15:28:59 UTC

Nice work everyone!

Pushed to oldstable for 22.11.x

Comment 10 Lucas Gass (lukeg) 2023-09-13 20:37:52 UTC

Backported to 22.05.x for upcoming 22.05.16