Suppression should be a biblio table field instead of in a marc field. Running reports with it to extract that field each time is not optimal.
+1 I like this idea.
Created attachment 175648 [details] [review] Bug 38330: DB changes
Created attachment 175649 [details] [review] Bug 38330: DBIC updates
Created attachment 175650 [details] [review] Bug 38330: Map to biblio.opac_suppressed on saving record
Created attachment 175651 [details] [review] Bug 38330: Make sure biblio.opac_suppressed is populated/updated on save This patchset adds a new column to biblio/deletedbiblio for storing the information about OPAC suppression for the record. It works as any other framework-mapped attribute. It defaults to 0 (not suppressed) and some sanitization is done on storing, because the DB should only contain 0 or 1. To test: 0. Have a couple records with 942$n set to suppress them. 1. Apply this patches 2. Run: $ ktd --shell k$ updatedatabase => SUCCESS: All good 3. Check the `opac_suppressed` column was added to both tables 4. Notice it tells you need to run `touch_all_biblios.pl` 5. Run: k$ koha-mysql kohadev > SELECT opac_suppressed,COUNT(opac_suppressed) FROM biblio GROUP BY opac_suppressed; => FAIL: There are no records with the opac_suppressed flag set to 1 6. Run: k$ perl misc/maintenance/touch_all_biblios.pl -v => SUCCESS: No failures 7. Repeat 5 => SUCCESS: Your couple records have the flag set. This means ModBiblio is doing the right thing 8. Play with adding new records (suppressed and not) => SUCCESS: Suppression status is set correctly 9. Change the status to select records => SUCCESS: The DB column and the MARC are in sync! 10. Sign off :-D
My two cents towards all this.
Created attachment 175767 [details] [review] Bug 38330: DB changes Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Created attachment 175768 [details] [review] Bug 38330: DBIC updates Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Created attachment 175769 [details] [review] Bug 38330: Map to biblio.opac_suppressed on saving record Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Created attachment 175770 [details] [review] Bug 38330: Make sure biblio.opac_suppressed is populated/updated on save This patchset adds a new column to biblio/deletedbiblio for storing the information about OPAC suppression for the record. It works as any other framework-mapped attribute. It defaults to 0 (not suppressed) and some sanitization is done on storing, because the DB should only contain 0 or 1. To test: 0. Have a couple records with 942$n set to suppress them. 1. Apply this patches 2. Run: $ ktd --shell k$ updatedatabase => SUCCESS: All good 3. Check the `opac_suppressed` column was added to both tables 4. Notice it tells you need to run `touch_all_biblios.pl` 5. Run: k$ koha-mysql kohadev > SELECT opac_suppressed,COUNT(opac_suppressed) FROM biblio GROUP BY opac_suppressed; => FAIL: There are no records with the opac_suppressed flag set to 1 6. Run: k$ perl misc/maintenance/touch_all_biblios.pl -v => SUCCESS: No failures 7. Repeat 5 => SUCCESS: Your couple records have the flag set. This means ModBiblio is doing the right thing 8. Play with adding new records (suppressed and not) => SUCCESS: Suppression status is set correctly 9. Change the status to select records => SUCCESS: The DB column and the MARC are in sync! 10. Sign off :-D Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com>
Created attachment 179335 [details] [review] Bug 38330: DB changes Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 179336 [details] [review] Bug 38330: DBIC updates Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 179337 [details] [review] Bug 38330: Map to biblio.opac_suppressed on saving record Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 179338 [details] [review] Bug 38330: Make sure biblio.opac_suppressed is populated/updated on save This patchset adds a new column to biblio/deletedbiblio for storing the information about OPAC suppression for the record. It works as any other framework-mapped attribute. It defaults to 0 (not suppressed) and some sanitization is done on storing, because the DB should only contain 0 or 1. To test: 0. Have a couple records with 942$n set to suppress them. 1. Apply this patches 2. Run: $ ktd --shell k$ updatedatabase => SUCCESS: All good 3. Check the `opac_suppressed` column was added to both tables 4. Notice it tells you need to run `touch_all_biblios.pl` 5. Run: k$ koha-mysql kohadev > SELECT opac_suppressed,COUNT(opac_suppressed) FROM biblio GROUP BY opac_suppressed; => FAIL: There are no records with the opac_suppressed flag set to 1 6. Run: k$ perl misc/maintenance/touch_all_biblios.pl -v => SUCCESS: No failures 7. Repeat 5 => SUCCESS: Your couple records have the flag set. This means ModBiblio is doing the right thing 8. Play with adding new records (suppressed and not) => SUCCESS: Suppression status is set correctly 9. Change the status to select records => SUCCESS: The DB column and the MARC are in sync! 10. Sign off :-D Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This is a great move in a positive direction.. it highlights a few API oversights though.. which I think should be corrected in a follow-up bug. Passing QA as it is.
Created attachment 179339 [details] [review] Bug 38330: (QA follow-up) Remove use of metadata_extractor We had a local method for getting opac_suppressed in Koha::Biblio which relied on extracting from the MARC record. Now that we have an opac_suppressed field in the Biblio table, we no longer need this wrapper method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 179340 [details] [review] Bug 38330: (QA follow-up) Remove use of metadata_extractor We had a local method for getting opac_suppressed in Koha::Biblio which relied on extracting from the MARC record. Now that we have an opac_suppressed field in the Biblio table, we no longer need this wrapper method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 179341 [details] [review] Bug 38330: (QA follow-up) Add population of opac_suppressed from biblio_metadata This patch updates the database update to populate the new opac_suppressed field in the bilios table using the data in the biblio_metadata table. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Wouldn't be a bad idea for someone to have a final look at those follow-ups.
This is super exciting to see, but we've got a little problem in the QA follow-up, as it's using 952$n instead of 942$n. Marking Failed QA but hopefully will be in this status only for a short time. I'll try to take a look at this again shortly (once I've gotten through the rest of my emails...)
(In reply to Martin Renvoize (ashimema) from comment #19) > Wouldn't be a bad idea for someone to have a final look at those follow-ups. I'll fix up the 952$n to 942$n... and I might do a different UPDATE since historically any true 942$n value was valid to trigger suppression. I'll have a think about that one.
(In reply to David Cook from comment #21) > (In reply to Martin Renvoize (ashimema) from comment #19) > > Wouldn't be a bad idea for someone to have a final look at those follow-ups. > > I'll fix up the 952$n to 942$n... and I might do a different UPDATE since > historically any true 942$n value was valid to trigger suppression. I'll > have a think about that one. I'm thinking about using boolean() in the Xpath. The following returns 1 for any true value and should return 0 for any false value, so it'll return 1 for new value of 1 or old values like "YES" or "yes" or whatever. 0 will be returned for 0 or empty or non-existent. IF(ExtractValue(metadata,'//datafield[@tag="942"]/subfield[@code="n" and boolean(text())]/text()'),1,0)
Keeping in mind bug 39337 it might be worthwhile adding an index on the opac_suppressed column. That said, it might not be used by the query planner. I suppose that's something that we could explore later.
Created attachment 182094 [details] [review] Bug 38330: DB changes Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 182095 [details] [review] Bug 38330: DBIC updates Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 182096 [details] [review] Bug 38330: Map to biblio.opac_suppressed on saving record Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 182097 [details] [review] Bug 38330: Make sure biblio.opac_suppressed is populated/updated on save This patchset adds a new column to biblio/deletedbiblio for storing the information about OPAC suppression for the record. It works as any other framework-mapped attribute. It defaults to 0 (not suppressed) and some sanitization is done on storing, because the DB should only contain 0 or 1. To test: 0. Have a couple records with 942$n set to suppress them. 1. Apply this patches 2. Run: $ ktd --shell k$ updatedatabase => SUCCESS: All good 3. Check the `opac_suppressed` column was added to both tables 4. Notice it tells you need to run `touch_all_biblios.pl` 5. Run: k$ koha-mysql kohadev > SELECT opac_suppressed,COUNT(opac_suppressed) FROM biblio GROUP BY opac_suppressed; => FAIL: There are no records with the opac_suppressed flag set to 1 6. Run: k$ perl misc/maintenance/touch_all_biblios.pl -v => SUCCESS: No failures 7. Repeat 5 => SUCCESS: Your couple records have the flag set. This means ModBiblio is doing the right thing 8. Play with adding new records (suppressed and not) => SUCCESS: Suppression status is set correctly 9. Change the status to select records => SUCCESS: The DB column and the MARC are in sync! 10. Sign off :-D Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 182098 [details] [review] Bug 38330: (QA follow-up) Remove use of metadata_extractor We had a local method for getting opac_suppressed in Koha::Biblio which relied on extracting from the MARC record. Now that we have an opac_suppressed field in the Biblio table, we no longer need this wrapper method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 182099 [details] [review] Bug 38330: (QA follow-up) Add population of opac_suppressed from biblio_metadata This patch updates the database update to populate the new opac_suppressed field in the bilios table using the data in the biblio_metadata table. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 182100 [details] [review] Bug 38330: (QA follow-up) Correction to field extaction As pointed out by David.. it's the 942 not the 952 we have this data in. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk>
Created attachment 182101 [details] [review] Bug 38330: (QA follow-up) Improve resilience of DB update David considered using boolean, but this would catch 'no' and 0 as truthy. I instead look for common values here more explicitly.
Created attachment 182102 [details] [review] Bug 38330: (QA follow-up) Add index to opac_suppressed As suggested by David, this one adds an index on the new column
Any chance of a final QA run on this David
(In reply to Martin Renvoize (ashimema) from comment #33) > Any chance of a final QA run on this David Sure thing, mon ami.
The /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_38330.pl has a syntax error in the SQL because of the ADD INDEX but I'll add a follow-up for that.
The test plan is missing a "koha-plack --restart kohadev", so the test actually uses the pre-patch behaviour it seems.
So /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_38330.pl was bugging me... Why would it try to SET b.opac_suppressed AND say "You need to run the `maintenance/touch_all_biblios.pl` script"? And the reason is that the WHERE clause in the UPDATE SQL will never match. Currently it is this: '%<datafield tag="942">%' But it should be this: '%<datafield tag="942" %' There should be no reason to run maintenance/touch_all_biblios.pl, since we're just updating the database and not changing the indexing.
Funny enough... in ktd biblionumber 232 and 234 are already marked suppressed. The things we learn when all you need to find suppressed records is: select biblionumber,opac_suppressed from biblio where opac_suppressed <> 0
After applying the patch, and running running "reset_all", "opac_suppressed" is 0 for KTD test data (e.g. biblionumber 232 and 234), but it should be 1. I think this is a problem with the KTD tools though. They must be doing direct SQL inserts instead of using Koha's internal functions... (yeah see /koha-misc4dev/-/tree/main/data/sql/marc21/2412/after_26684). That means there will be some buggy OpacSuppression behaviour in KTD for a transitional time. I've raised issue #88 in koha-miscdev to address this. Fortunately, the MARC bibliographic frameworks are created correctly out of the box, which is good.
Worth noting that the "Suppressed in OPAC" in the search results still comes from the MARC rather than the DB.
(In reply to David Cook from comment #40) > Worth noting that the "Suppressed in OPAC" in the search results still comes > from the MARC rather than the DB. Same with the detail page actually. In future, we should probably move that out of the XSLT and into its own spot. Not a dealbreaker at this point though... There are references to 942$n in Koha/ILL/Backend/Standard.pm and Koha/Filter/MARC/ExpandCodedFields.pm but they both make sense and they're irrelevant for our work here.
(In reply to David Cook from comment #37) > There should be no reason to run maintenance/touch_all_biblios.pl, since > we're just updating the database and not changing the indexing. Theoretically, this is true. However, practically/technically it is possible for there to be MARCXML in the database which isn't formed exactly as we expect. For instance, it could be '<datafield ind1="" ind2="" tag="942">'. From memory, I think that Koha will still work with that, and that would fail our WHERE clause test. It's totally an edge case... but it's technically possible. So... I'm changing the warning to the following: "The biblio.opac_suppressed column should be updated, but you can run the `maintenance/touch_all_biblios.pl` script to be certain" That lets people know that the data should be updated, but that if they have issues... just run touch_all_biblios.pl.
Created attachment 182135 [details] [review] Bug 38330: DB changes Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 182136 [details] [review] Bug 38330: DBIC updates Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 182137 [details] [review] Bug 38330: Map to biblio.opac_suppressed on saving record Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 182138 [details] [review] Bug 38330: Make sure biblio.opac_suppressed is populated/updated on save This patchset adds a new column to biblio/deletedbiblio for storing the information about OPAC suppression for the record. It works as any other framework-mapped attribute. It defaults to 0 (not suppressed) and some sanitization is done on storing, because the DB should only contain 0 or 1. To test: 0. Have a couple records with 942$n set to suppress them. 1. Apply this patches 2. Run: $ ktd --shell k$ updatedatabase => SUCCESS: All good 3. Check the `opac_suppressed` column was added to both tables 4. Notice it tells you need to run `touch_all_biblios.pl` 5. Run: k$ koha-mysql kohadev > SELECT opac_suppressed,COUNT(opac_suppressed) FROM biblio GROUP BY opac_suppressed; => FAIL: There are no records with the opac_suppressed flag set to 1 6. Run: k$ perl misc/maintenance/touch_all_biblios.pl -v => SUCCESS: No failures 7. Repeat 5 => SUCCESS: Your couple records have the flag set. This means ModBiblio is doing the right thing 8. Play with adding new records (suppressed and not) => SUCCESS: Suppression status is set correctly 9. Change the status to select records => SUCCESS: The DB column and the MARC are in sync! 10. Sign off :-D Signed-off-by: Andrew Fuerste Henry <andrew@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 182139 [details] [review] Bug 38330: (QA follow-up) Remove use of metadata_extractor We had a local method for getting opac_suppressed in Koha::Biblio which relied on extracting from the MARC record. Now that we have an opac_suppressed field in the Biblio table, we no longer need this wrapper method. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 182140 [details] [review] Bug 38330: (QA follow-up) Add population of opac_suppressed from biblio_metadata This patch updates the database update to populate the new opac_suppressed field in the bilios table using the data in the biblio_metadata table. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 182141 [details] [review] Bug 38330: (QA follow-up) Correction to field extaction As pointed out by David.. it's the 942 not the 952 we have this data in. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 182142 [details] [review] Bug 38330: (QA follow-up) Improve resilience of DB update David considered using boolean, but this would catch 'no' and 0 as truthy. I instead look for common values here more explicitly. Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 182143 [details] [review] Bug 38330: (QA follow-up) Add index to opac_suppressed As suggested by David, this one adds an index on the new column Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 182144 [details] [review] Bug 38330: (QA follow-up) Fix SQL syntax error Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 182145 [details] [review] Bug 38330: (QA follow-up) Fix WHERE clause in SQL update and re-word warning Signed-off-by: David Cook <dcook@prosentient.com.au>
I need to ponder this one. It feels like it might be too big and risky for 25.05 at this point in time (being after hard feature freeze).
(In reply to Katrin Fischer from comment #54) > I need to ponder this one. It feels like it might be too big and risky for > 25.05 at this point in time (being after hard feature freeze). No-one has argued against this, so I am setting this rel_25_11_candidate. Let's get this in early (but you'll have to talk to Lucas ;))
(In reply to Katrin Fischer from comment #54) > I need to ponder this one. It feels like it might be too big and risky for > 25.05 at this point in time (being after hard feature freeze). I meant to reply to this before. I think that's a good decision.
[FAIL] installer/data/mysql/kohastructure.sql FAIL tinyint_has_boolean_flag The new column biblio.opac_suppressed is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/Biblio.pm), see the SQL12 coding guideline The new column deletedbiblio.opac_suppressed is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/Deletedbiblio.pm), see the SQL12 coding guideline