Description
Dani Elder
2024-11-01 20:08:14 UTC
+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. |