Description
Kyle M Hall (khall)
2014-01-13 14:46:58 UTC
I want to add some additional information to this bug report -- the checkout and holds summaries now display the entire 245 MARC field -- not just subtitle (245$b). Information like the 245$h [gmd], 245$n (number of work) and 245$p part of work, also display -- which are very useful pieces of information, especially with graphic novels. But, loading this entire field in large systems does appear to cause page loading. I wonder if all 4 subfields need to be added to the biblio table, not just the 245$b subfield. *** Bug 7715 has been marked as a duplicate of this bug. *** Adding a single field to the biblio table seems much too limiting. Would you combine all the 245 subfields > 'a' into one string and store it that way? Is anyone thinking about a better way to do this? I agree, a single subfield is not enough here. I think at least having hbnp would make sense. I suppose the only problem is that this needs to be driven by the bibliographic framework, and we can't assume that the subfields include punctuation. While it's possible to map multiple subfields to a single Koha field in the framework, there's no way to define punctuation, and the rules for that might become a bit complicated too. One option would be to actually add several columns such as: title_remainder = b (in MARC21) medium = h part_number = n part_name = p Then the presentation could be left to whatever module uses the data. This would require more work from the presentation code, but it would also allow way more versatile display. What do you think? I think we can map multiple, but they are not all added to the db (I think). I like the idea of separate fields. I think these days multiple values are concatenated, but with spaces and a pipe character, so it's not very friendly for display purposes. I'll see if I can come up with a patch. I thought this was only for repeatable fields, but might be mistaken! Created attachment 81930 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 81931 [details] [review] Bug 11529: Use new biblio fields whenever possible This is pretty much work in progress but I posted so that it's not lost in any catastrophic event. :) In any case, the first patch is pretty much done and the second is done but completely untested. For the record, I ended up naming the field for "remainder of title" subtitle for back-compatibility, as subtitle is the predominant term in the current index. Adding the fields increases the space requirement for the biblio table, but the increase is pretty minor in the grand scheme. The only controversial thing, as far as I can see, is that retrieval of subtitle with this mechanism doesn't use the fieldmapping table ("Keyword to MARC mapping" in the UI) anymore. I don't see it as an issue since, as far as I can see, it's a mechanism that's only been used for the subtitle. So we can probably get rid of it completely here. I'll just need to finish the transition to get rid of the GetRecordValue method or make it work with the framework data. Created attachment 82024 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 82025 [details] [review] Bug 11529: Use new biblio fields whenever possible Created attachment 82026 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 82027 [details] [review] Bug 11529: Use new biblio fields whenever possible Created attachment 82028 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 82029 [details] [review] Bug 11529: Use new biblio fields whenever possible The latest version might actually work, at least to some extent. I still need to go through all the paths that have been changed. Created attachment 82088 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 82089 [details] [review] Bug 11529: Use new biblio fields whenever possible Created attachment 82090 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl With the latest patches e.g. holds reports in circulation display the information properly. Created attachment 82428 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 82429 [details] [review] Bug 11529: Use new biblio fields whenever possible Created attachment 82430 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Created attachment 82431 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Do we really want to add "medium" considering the 245$h [gmd] has a rival with the 336 and 338 fields? $h is obsolete with RDA, but I don't think it hurts to have the column as RDA is not used everywhere. Created attachment 82538 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 82539 [details] [review] Bug 11529: Use new biblio fields whenever possible Created attachment 82540 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Created attachment 82541 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Created attachment 82542 [details] [review] Bug 11529: Remove field mapping table and admin page I believe this is now ready to be tested. I Initially attempted to keep the templates unchanged as much as possible, but there were multiple issues with the old subtitle field handling, so I ended up unifying the handling a bit too. Part name and number have been added to the hold specific lists where I know they're needed, but they can easily be added to other screens too if deemed necessary. The addition medium field was based on a comment on the mailing list about the old functionality being used to include that one too. I don't have a strong opinion on it, but since the content is based on the framework, it could also be configured to be taken from the RDA fields. Test plan: 1. Apply the patches 2. Run "perl misc/batchRebuildBiblioTables.pl -c" to populate the columns in biblio table 3. Verify that at least subtitle is properly displayed in Holds queue, Holds to pull and Holds awaiting pickup list as well as patron's checkouts list. 4. Verify that tests still pass. (In reply to Katrin Fischer from comment #28) > $h is obsolete with RDA, but I don't think it hurts to have the column as > RDA is not used everywhere. Since RDA isn't used everywhere, I was thinking maybe something more generic would be good to take it into account though. But maybe that's just a premature optimization. Hi Ere, it looks almost good just few thinks: Holds to pull does not show data in table Hold ratios does not work (see bellow) Note, that changes in Koha::Schema files should be in its own commit - to avoid conflicts on hash Also this is report from qa tools: FAIL circ/reserveratios.pl FAIL valid "my" variable @reservedata masks earlier declaration in same scope Global symbol "$data" requires explicit package name (did you forget to declare "my $data"?) "my" variable $rd masks earlier declaration in same statement circ/reserveratios.pl has too many errors. Global symbol "$thisratio" requires explicit package name (did you forget to declare "my $thisratio"?) syntax error FAIL koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt FAIL forbidden patterns forbidden pattern: tab char (line 156) FAIL opac/opac-showreviews.pl FAIL forbidden patterns forbidden pattern: tab char (line 100) forbidden pattern: tab char (line 101) forbidden pattern: tab char (line 99) forbidden pattern: tab char (line 102) Created attachment 83103 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 83104 [details] [review] Bug 11529: Use new biblio fields whenever possible Created attachment 83105 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Created attachment 83106 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Created attachment 83107 [details] [review] Bug 11529: Remove field mapping table and admin page Created attachment 83108 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Comment on attachment 83106 [details] [review] Bug 11529: Clean up subtitle usage Review of attachment 83106 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/js/checkouts.js @@ +244,3 @@ > }); > > + title += " " + oObj.part_number + " " + oObj.part_name; If there is no part_number or part_name, there is "null" string shown in UI @@ +601,3 @@ > }); > > + title += " " + oObj.part_number + " " + oObj.part_name; If there is no part_number or part_name, there is "null" string shown in UI Created attachment 83961 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 83962 [details] [review] Bug 11529: Use new biblio fields whenever possible Created attachment 83963 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Created attachment 83964 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Created attachment 83965 [details] [review] Bug 11529: Remove field mapping table and admin page Created attachment 83966 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Thanks Josef, fixed and rebased. *** Bug 18408 has been marked as a duplicate of this bug. *** *** Bug 20403 has been marked as a duplicate of this bug. *** Created attachment 85587 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 85588 [details] [review] Bug 11529: Use new biblio fields whenever possible Created attachment 85589 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Created attachment 85590 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Created attachment 85591 [details] [review] Bug 11529: Remove field mapping table and admin page Created attachment 85592 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Rebased and made tests more robust. Hi Ere, i tested patches i get this error: Falling back to patching base and 3-way merge... Removing Koha/Schema/Result/Fieldmapping.pm Auto-merging Koha/Schema/Result/Biblio.pm CONFLICT (content): Merge conflict in Koha/Schema/Result/Biblio.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 11529: DBIx schema changes Can you rebase? Created attachment 86677 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 86678 [details] [review] Bug 11529: Use new biblio fields whenever possible Created attachment 86679 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Created attachment 86680 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Created attachment 86681 [details] [review] Bug 11529: Remove field mapping table and admin page Created attachment 86682 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Thanks Michal, now rebased. Hi Ere, I'm not sure about these results: Holds at Borrower detail: see "null" http://prntscr.com/n56znx Holds to Pull: http://prntscr.com/n56xmj Maybe something is wrong with my catalogue template or records. Michal Created attachment 87248 [details] [review] Bug 11529: Add new fields to biblio table Created attachment 87249 [details] [review] Bug 11529: Use new biblio fields whenever possible Created attachment 87250 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Created attachment 87251 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Created attachment 87252 [details] [review] Bug 11529: Remove field mapping table and admin page Created attachment 87253 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Thanks Michal. Both issues should now be fixed. The displaying of null happened if part_number and part_name in database were null, which is possible if there's no mapping for them or the batch rebuild has not been run. Now the service guards against that. Created attachment 87256 [details] [review] Bug 11529: Add new fields to biblio table Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87257 [details] [review] Bug 11529: Use new biblio fields whenever possible Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87258 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87259 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87260 [details] [review] Bug 11529: Remove field mapping table and admin page Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87261 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Leaving some first notes here. Ere, can you please have a look? Nothing that keeps me from testing more for now, but it will help to have follow-ups and answers :) I Code review Good: - QA test tools pass - Clean database update - New tests pass Bad (blocker): - git grep bibliosubtitle.subtitle still shows several occurrences to be fixed - Can you explain the use of $raw on the subtitle in koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/sendshelf.tt and koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-sendshelf.tt Non-blocker notes: - Need to leave a note on Taiga/bug here that the Koha-to-keyword content needs to be removed from the manual - Will need extensive release notes notes. Pre-existing: - GetHoldsQueueItems: biblioitems.publicationsyear is listed twice in the SQL query - Why so many occurrences of [% subtitl %]? - The ',' between subtitles will in opac-results.pl, opac-showreviews-rss.tt will look not work for MARC21 libraries using proper ISBD punctuation in their records. II Questions: - In the database update we don't check if libraries had already mapped the fields to other database columns. The remapping might leave data and reports orphaned. Should we check for this and at least print a warning? - What is our recommendation on how to populate the new db fields? Can we imagine something that works faster than touch_all_biblios.pl? SQL updates? What's the difference between touch_all_biblios.pl and batchRebuildBiblioTables.pl? - Why was the -t test option removed from batchRebuildBiblioTables.pl? - AFAIK UNIMARC doesn't have ISBD punctuation in the records. Do we need to introduce a separator for UNIMARC between title and subtitle? There is one place where we already seem to deal with this in opac-detail.pl: <title>[% IF ( LibraryNameTitle ) %][% LibraryNameTitle | html %][% ELSE %]Koha online[% END %] catalog › Details for: [% title | html %][% FOREACH subtitl IN subtitle %][% IF Koha.Preference('marcflavour')=='UNIMARC' %],[% END %] [% subtitl | html %][% END %]</title> Created attachment 87911 [details] [review] Bug 11529: Add new fields to biblio table Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87912 [details] [review] Bug 11529: Use new biblio fields whenever possible Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87913 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87914 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87915 [details] [review] Bug 11529: Remove field mapping table and admin page Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87916 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 87917 [details] [review] Bgu 11529: Update kohafield in default framework only if not already set. Created attachment 87918 [details] [review] Bug 11529: Replace bibliosubtitle with biblio in all framework sql's. Created attachment 87919 [details] [review] Bug 11529: Remove duplicate column name from select query (In reply to Katrin Fischer from comment #82) > Bad (blocker): > - git grep bibliosubtitle.subtitle still shows several occurrences to be > fixed Fixed. > - Can you explain the use of $raw on the subtitle in > koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/sendshelf.tt and > koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-sendshelf.tt I suppose it's because of the html2text filter, but I just kept what there already was. > Non-blocker notes: > - Need to leave a note on Taiga/bug here that the Koha-to-keyword content > needs to be removed from the manual Can I access Taiga somewhere or is it better to just file a bug? > - Will need extensive release notes notes. I'll try to come up with something. > Pre-existing: > - GetHoldsQueueItems: biblioitems.publicationsyear is listed twice in the > SQL query Fixed here. > - Why so many occurrences of [% subtitl %]? I've kept as much of the code intact as possible. Since the old code used that, I've kept it intact. Also, we concatenate multiple values in the database with ' | ', so splitting and handling them one by one allows one to customize the display more. > - The ',' between subtitles will in opac-results.pl, opac-showreviews-rss.tt > will look not work for MARC21 libraries using proper ISBD punctuation in > their records. I agree, but I believe that hasn't changed here. It could be handled as a separate issue. > II Questions: > > - In the database update we don't check if libraries had already mapped the > fields to other database columns. The remapping might leave data and reports > orphaned. Should we check for this and at least print a warning? Good point, check added. > - What is our recommendation on how to populate the new db fields? Can we > imagine something that works faster than touch_all_biblios.pl? SQL updates? > What's the difference between touch_all_biblios.pl and > batchRebuildBiblioTables.pl? batchRebuildBiblioTables.pl is recommended. touch_all_biblios.pl also indexes all changes, which isn't necessary here. > - Why was the -t test option removed from batchRebuildBiblioTables.pl? It served no purpose. There was no useful output or anything that I could think of to use it for. > - AFAIK UNIMARC doesn't have ISBD punctuation in the records. Do we need to > introduce a separator for UNIMARC between title and subtitle? That'd probably be nice, but I believe that could be done in a separate bug. Since most of the existing templates didn't have that, I don't think it's vital. Also, I believe the code in opac-detail could be improved if comma is not the best separator. Hi Ere, I am sorry, I waited to long to continue here. Could you rebase it please? I will try to come back to it as fast ass possible after (faster than last time!) Managed the rebase myself - the change was caused by the non-XSLT view being removed. Working here... I'll keep testing, but we need another patch: - Remove manage_keywords2koha_mappings from display and patron accounts Right, I didn't notice the permission before. I'll add a follow-up. I have another big issue here... In our installations we have mapped subtitle to 245cnp for ages now, so all those subfields show in a lot of places. With this patch set, I am afraid we will see a major regression that won't be fixable by configuration. In a lot of places, only the subtitle c will be displayed. I think the thinking error here is that subtitle was only used for displaying c before, but I think this is not the case. Examples are: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt | 2 +- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-readingrecord.tt | 6 +-- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt | 2 +- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-sendshelf.tt | 2 +- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt | 2 +- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-showreviews-rss.tt | 2 +- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-showreviews.tt | 2 +- koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt Created attachment 88247 [details] [review] Bug 11529: Add new fields to biblio table Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88248 [details] [review] Bug 11529: Use new biblio fields whenever possible Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88249 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88250 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88251 [details] [review] Bug 11529: Remove field mapping table and admin page Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88252 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88253 [details] [review] Bgu 11529: Update kohafield in default framework only if not already set. Created attachment 88254 [details] [review] Bug 11529: Replace bibliosubtitle with biblio in all framework sql's. Created attachment 88255 [details] [review] Bug 11529: Remove duplicate column name from select query Created attachment 88256 [details] [review] Bug 11529: (follow-up) Remove manage_keywords2koha_mappings permission. See comment #98 :( Rebased and added a follow-up to remove the permission. Katrin, needing subfields n and p was anticipated, but I didn't realize they were needed in so many places. There's nothing preventing from using the new subtitle for all of them like before, but it probably makes more sense to include part_number and part_name explicitly in the templates you mentioned. I'll try to do that now. (In reply to Ere Maijala from comment #110) > Rebased and added a follow-up to remove the permission. > > Katrin, needing subfields n and p was anticipated, but I didn't realize they > were needed in so many places. There's nothing preventing from using the new > subtitle for all of them like before, but it probably makes more sense to > include part_number and part_name explicitly in the templates you mentioned. > I'll try to do that now. Sorry, not sure I understand the first part "using the new subtitle"? I mean you could map 245n and 245p to the biblio.subtitle field in the MARC framework, but since these changes introduce the biblio.part_name and biblio.part_number fields, it makes sense to actually bring them into the templates. (In reply to Ere Maijala from comment #112) > I mean you could map 245n and 245p to the biblio.subtitle field in the MARC > framework, but since these changes introduce the biblio.part_name and > biblio.part_number fields, it makes sense to actually bring them into the > templates. I am not sure that would work in the expected way - it would not make all the information go into the field, but I think only one of it. I haven't tested, but that's how I remember behaviour for mapping both 260/264. (In reply to Katrin Fischer from comment #113) > (In reply to Ere Maijala from comment #112) > > I mean you could map 245n and 245p to the biblio.subtitle field in the MARC > > framework, but since these changes introduce the biblio.part_name and > > biblio.part_number fields, it makes sense to actually bring them into the > > templates. > > I am not sure that would work in the expected way - it would not make all > the information go into the field, but I think only one of it. I haven't > tested, but that's how I remember behaviour for mapping both 260/264. It used to be that way, but nowadays it will put everything into the field separated by ' | '. And since the splitting is done for the subtitle field, it's displayed one by one just like before. But that's still a not-so-great hack, so I'll make sure the part fields are displayed as well. Maybe some include template file could help to improve and standardize the way we are displaying titles and links to record in UI. We have similar one for patrons - patron-title.inc Yes, that makes sense. (In reply to Josef Moravec from comment #115) > Maybe some include template file could help to improve and standardize the > way we are displaying titles and links to record in UI. We have similar one > for patrons - patron-title.inc +1 Was thinking the same, it would also make our life much easier if we want to add punctuation for UNIMARC later on. Ok, I'm going for a bit more cleanup. My original approach was to minimize template changes, but since they're needed anyway, there's no point trying to hack it. Created attachment 88406 [details] [review] Bug 11529: Add new fields to biblio table Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88407 [details] [review] Bug 11529: Use new biblio fields whenever possible Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88408 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88409 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88410 [details] [review] Bug 11529: Remove field mapping table and admin page Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88411 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88412 [details] [review] Bug 11529: Update kohafield in default framework only if not already set. Created attachment 88413 [details] [review] Bug 11529: Replace bibliosubtitle with biblio in all framework sql's. Created attachment 88414 [details] [review] Bug 11529: Remove duplicate column name from select query Created attachment 88415 [details] [review] Bug 11529: (follow-up) Remove manage_keywords2koha_mappings permission. Created attachment 88416 [details] [review] Bug 11529: Add templates for biblio title display. Unify display. Okay, so this became a bit bigger set of changes and I'm not sure how to ever get this in since I can imagine the last patch rotting away like no other. Anyway, this is an attempt to use an include file for any time a biblio title is to be displayed. There are separate includes for the title field in head and any other stuff in html. There are also classes added to each part. This allows one to style them independently and to e.g. easily hide part information from breadcrumbs if that's necessary. I've tried to find all occasions where the title is displayed, and I've also tried to test them, but there may well be something that I've missed. Hi Ere, I am willing to test - even if it looks scary. Changing everywhere title shows... you mean changed everywhere subtitle was used, I hope? (just aiming for no regression right now) One small change to make before I start: staff-global.css should not be in there. Can you please kick it out? Created attachment 88433 [details] [review] Bug 11529: Add new fields to biblio table Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88434 [details] [review] Bug 11529: Use new biblio fields whenever possible Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88435 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88436 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88437 [details] [review] Bug 11529: Remove field mapping table and admin page Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88438 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88439 [details] [review] Bug 11529: Update kohafield in default framework only if not already set. Created attachment 88440 [details] [review] Bug 11529: Replace bibliosubtitle with biblio in all framework sql's. Created attachment 88441 [details] [review] Bug 11529: Remove duplicate column name from select query Created attachment 88442 [details] [review] Bug 11529: (follow-up) Remove manage_keywords2koha_mappings permission. Created attachment 88443 [details] [review] Bug 11529: Add templates for biblio title display. Unify display. Removed staff-global.css changes. Katrin, I've also added the biblio-title include to places where it's logical to use it and wasn't before. E.g. it makes no sense to display subtitle for checkouts but not for article requests, so I've done my best to unify the related displays. Same with e.g. the sub-pages of catalog record (holds, article requests, checkout history etc.). Hi Ere, I get error: Applying: Bgu 11529: Update kohafield in default framework only if not already set. Using index info to reconstruct a base tree... A installer/data/mysql/atomicupdate/bug_11529.perl Falling back to patching base and 3-way merge... CONFLICT (modify/delete): installer/data/mysql/atomicupdate/bug_11529.perl deleted in HEAD and modified in Bgu 11529: Update kohafield in default framework only if not already set.. Version Bgu 11529: Update kohafield in default framework only if not already set. of installer/data/mysql/atomicupdate/bug_11529.perl left in tree. error: Failed to merge in the changes. Please, can you rebase? Oops, sorry. There was an older version of one of the patches left over because of a typo in the title. I obsoleted it so it should be better now. Created attachment 88762 [details] [review] Bug 11529: Add new fields to biblio table Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88763 [details] [review] Bug 11529: Use new biblio fields whenever possible Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88764 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88765 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88766 [details] [review] Bug 11529: Remove field mapping table and admin page Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88767 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Created attachment 88768 [details] [review] Bug 11529: Update kohafield in default framework only if not already set. Created attachment 88769 [details] [review] Bug 11529: Replace bibliosubtitle with biblio in all framework sql's. Created attachment 88770 [details] [review] Bug 11529: Remove duplicate column name from select query Created attachment 88771 [details] [review] Bug 11529: (follow-up) Remove manage_keywords2koha_mappings permission. Created attachment 88772 [details] [review] Bug 11529: Add templates for biblio title display. Unify display. Rebased again. Created attachment 88851 [details] [review] Bug 11529: Add new fields to biblio table Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 88852 [details] [review] Bug 11529: Use new biblio fields whenever possible Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 88853 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 88854 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 88855 [details] [review] Bug 11529: Remove field mapping table and admin page Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 88856 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 88857 [details] [review] Bug 11529: Update kohafield in default framework only if not already set. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 88858 [details] [review] Bug 11529: Replace bibliosubtitle with biblio in all framework sql's. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 88859 [details] [review] Bug 11529: Remove duplicate column name from select query Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 88860 [details] [review] Bug 11529: (follow-up) Remove manage_keywords2koha_mappings permission. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 88861 [details] [review] Bug 11529: Add templates for biblio title display. Unify display. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 88862 [details] [review] Bug 11529: (follow-up) Fix link on hold request page Signed-off-by: Josef Moravec <josef.moravec@gmail.com> I am back on this, but sadly it doesn't apply :( Please fix, I would have time tomorrow for testing. Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11529: Add new fields to biblio table Applying: Bug 11529: Use new biblio fields whenever possible .git/rebase-apply/patch:390: trailing whitespace. count(DISTINCT reserves.borrowernumber) as reservecount, error: sha1 information is lacking or useless (C4/Biblio.pm). error: could not build fake ancestor Patch failed at 0001 Bug 11529: Use new biblio fields whenever possible The copy of the patch that failed is found in: .git/rebase-apply/patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-11529-Use-new-biblio-fields-whenever-possible-TtxGN1.patch Created attachment 89096 [details] [review] Bug 11529: Add new fields to biblio table Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89097 [details] [review] Bug 11529: Use new biblio fields whenever possible Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89098 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89099 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89100 [details] [review] Bug 11529: Remove field mapping table and admin page Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89101 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89102 [details] [review] Bug 11529: Update kohafield in default framework only if not already set. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89103 [details] [review] Bug 11529: Replace bibliosubtitle with biblio in all framework sql's. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89104 [details] [review] Bug 11529: Remove duplicate column name from select query Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89105 [details] [review] Bug 11529: (follow-up) Remove manage_keywords2koha_mappings permission. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89106 [details] [review] Bug 11529: Add templates for biblio title display. Unify display. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89107 [details] [review] Bug 11529: (follow-up) Fix link on hold request page Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Rebased. Thank you! Embarking on my testing journey here... wish me luck! Patches apply, QA tests pass... more later. 1) RELEASE NOTES - Removes the Koha to Keyword mappings - Might cause change in behaviour for people having mappign fields outside of 245bhnp or UNIMARC equivalents - Requires you to run misc/batchRebuildBiblioTables.pl to have 245bhnp displayed 2) Template Toolkit Stash I noticed some of the files use Stash. I haven't come across this so far. Can you explain? koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-detail.tt +[% USE Stash %] Details for <i>[% INCLUDE 'biblio-title.inc' biblio=Stash.stash() %]</i> This is a problem, because at the moment this is only used for debugging and not marked as required in the Dependencies. Adding dependency tag to the bug. 3) SplitKohaField I wonder if the use case warrants its own subroutine, it's only used in a few places and for splitting a pipe separeted string into an array: C4/Biblio.pm:sub SplitKohaField { svc/checkouts: subtitle => C4::Biblio::SplitKohaField($c->{'subtitle'}), svc/holds:use C4::Biblio qw(SplitKohaField); svc/holds: subtitle => C4::Biblio::SplitKohaField($biblio->subtitle()), 4) Template variables Right now the changes require a lot of new variables passed to the templates and some older SQL methods to be extended to SELECT additional fields. I wonder if instead of working with an INCLUDE working with a TT plugin taking the biblionumber and querying would hurt performance much. It would allow us to remove quite a bit of code I think. Also I guess in the future working with a Record object might make things easier? 5) misc/batchRebuildBiblioTables.pl I noticed that you removed the table locking. Is this safe? 6) Unrelated changes Small one: koha-tmpl/intranet-tmpl/prog/en/includes/biblio-view-menu.inc And: t/db_dependent/Biblio.t - my $authorities_mod = Test::MockModule->new( 'C4::AuthoritiesMarc' ); + my $authorities_mod = Test::MockModule->new( 'C4::Heading' ); 7) Repeatable subfields np Testing with: 245abhnpnp Multiple np show as: The C programming language / Zusatz Number | Number2 Part | Part2 Actually this is a deeper rabbit hole... as we don't keep the sequence we might lose meaning here, but that would not have worked before. Maybe this would make things a little easier? http://template-toolkit.org/docs/manual/VMethods.html#method_split 8) Good news - Removal of permission complete - Removal of Koha to keywords feature complete - Removal of GetRecordValue and Biblio:subtitles complete I am sorry, but after testing I feel we probably should aim for early 19.11 instead of 19.05. While the basic change is not that complicated, in the sum the changes are pretty heavy. Created attachment 89234 [details] [review] Bug 11529: Add new fields to biblio table Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89235 [details] [review] Bug 11529: Use new biblio fields whenever possible Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89236 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89237 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89238 [details] [review] Bug 11529: Remove field mapping table and admin page Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89239 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89240 [details] [review] Bug 11529: Update kohafield in default framework only if not already set. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89241 [details] [review] Bug 11529: Replace bibliosubtitle with biblio in all framework sql's. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89242 [details] [review] Bug 11529: Remove duplicate column name from select query Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89243 [details] [review] Bug 11529: (follow-up) Remove manage_keywords2koha_mappings permission. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89244 [details] [review] Bug 11529: Add templates for biblio title display. Unify display. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89245 [details] [review] Bug 11529: (follow-up) Fix link on hold request page Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 89246 [details] [review] Bug 11529: (follow-up) Fix QA issues - Remove SplitKohaField - Avoid using Stash in templates - Improved display of part fields (In reply to Katrin Fischer from comment #187) Thanks, Katrin. Comments below. > 1) RELEASE NOTES > > - Removes the Koha to Keyword mappings > - Might cause change in behaviour for people having mappign fields outside > of 245bhnp or UNIMARC equivalents > - Requires you to run misc/batchRebuildBiblioTables.pl to have 245bhnp > displayed Do you want me to clarify these in the proposed release notes? > 2) Template Toolkit Stash > > I noticed some of the files use Stash. I haven't come across this so far. > Can you explain? Ah, I didn't notice it's only for development. I just used it to collect all the template params to a hash, but changed the code to pass biblio to the template in one case (the other actually already had it). > 3) SplitKohaField > > I wonder if the use case warrants its own subroutine, it's only used in > a few places and for splitting a pipe separeted string into an array: You're right. Now removed. > 4) Template variables > > Right now the changes require a lot of new variables passed to the templates > and some older SQL methods to be extended to SELECT additional fields. > I wonder if instead of working with an INCLUDE working with a TT plugin > taking the biblionumber and querying would hurt performance much. It would > allow > us to remove quite a bit of code I think. Also I guess in the future working > with > a Record object might make things easier? It's true, but the primary aim was to improve performance, and while fetching the data for a single record is not that bad, it's not optimal either, and when there's a list of records of any kind it can be significant. In some cases we could just pass the biblio object instead of select pieces of it, but I feel that's something that could be addressed in a follow-up issue, since this is usually just extending the existing practice. > 5) misc/batchRebuildBiblioTables.pl > > I noticed that you removed the table locking. Is this safe? It was only commented-out code that I cleaned up. > 6) Unrelated changes Fixed. > 7) Repeatable subfields np > > Testing with: 245abhnpnp > Multiple np show as: > The C programming language / Zusatz Number | Number2 Part | Part2 > > Actually this is a deeper rabbit hole... as we don't keep the sequence we > might lose meaning here, but that would not have worked before. I made the templates handle repeated fields a bit better. At least I think they would normally be interleaved, and that's how they're now displayed. It's perhaps still not perfect, but at least it works properly with ISBD punctuation, and further tweaks are easy to add later as necessary. Please note that qa-test-tools is not happy about the WHILE statement in the biblio-title* templates, but that seems to be a spurious error. (In reply to Katrin Fischer from comment #188) > I am sorry, but after testing I feel we probably should aim for early 19.11 > instead of 19.05. While the basic change is not that complicated, in the sum > the changes are pretty heavy. I'm fine with that. If we can get it in early in the cycle, it's of course easier and any rough edges can be found. Patch doesn't apply: Applying: Bug 11529: Add new fields to biblio table Applying: Bug 11529: Use new biblio fields whenever possible Applying: Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Applying: Bug 11529: Clean up subtitle usage error: sha1 information is lacking or useless (C4/Biblio.pm). error: could not build fake ancestor Patch failed at 0001 Bug 11529: Clean up subtitle usage Created attachment 91680 [details] [review] Bug 11529: Add new fields to biblio table Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91681 [details] [review] Bug 11529: Use new biblio fields whenever possible Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91682 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91683 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91684 [details] [review] Bug 11529: Remove field mapping table and admin page Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91685 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91686 [details] [review] Bug 11529: Update kohafield in default framework only if not already set. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91687 [details] [review] Bug 11529: Replace bibliosubtitle with biblio in all framework sql's. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91688 [details] [review] Bug 11529: Remove duplicate column name from select query Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91689 [details] [review] Bug 11529: (follow-up) Remove manage_keywords2koha_mappings permission. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91690 [details] [review] Bug 11529: Add templates for biblio title display. Unify display. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91691 [details] [review] Bug 11529: (follow-up) Fix link on hold request page Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 91692 [details] [review] Bug 11529: (follow-up) Fix QA issues - Remove SplitKohaField - Avoid using Stash in templates - Improved display of part fields Rebased. Created attachment 91824 [details] [review] Bug 11529: Add new fields to biblio table Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91825 [details] [review] Bug 11529: Use new biblio fields whenever possible Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91826 [details] [review] Bug 11529: Simplify and optimize batchRebuildBiblioTables.pl Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91827 [details] [review] Bug 11529: Clean up subtitle usage Unify and clean up subtitle usage so that it's always used as a simple array and not the old hash structure. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91828 [details] [review] Bug 11529: Remove field mapping table and admin page Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91829 [details] [review] Bug 11529: DBIx schema changes Updates to Biblio schema and removal of FieldMapping schema. Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91830 [details] [review] Bug 11529: Update kohafield in default framework only if not already set. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91831 [details] [review] Bug 11529: Replace bibliosubtitle with biblio in all framework sql's. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91832 [details] [review] Bug 11529: Remove duplicate column name from select query Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91833 [details] [review] Bug 11529: (follow-up) Remove manage_keywords2koha_mappings permission. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91834 [details] [review] Bug 11529: Add templates for biblio title display. Unify display. Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91835 [details] [review] Bug 11529: (follow-up) Fix link on hold request page Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91836 [details] [review] Bug 11529: (follow-up) Fix QA issues - Remove SplitKohaField - Avoid using Stash in templates - Improved display of part fields Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 91837 [details] [review] Bug 11529: (QA follow-up) Copy existing subtitle mappings and warn others Signed-off-by: Nick Clemens <nick@bywatersolutions.com> This works well, I think we should get this soon to be 'early' for 19.11 and catch any misses. I added to DB update in order to keep any non-standard mappings for the default, and warn of any that will be lost, letting for RM to approve ere++ This is a great improvement and cleanup, thanks Ere and all who tested it. PS, I moved sponsorship attribution into the git commit message.. should appear in release notes nicely still (and also get recorded to go into the about page at a later date) Nice work! Pushed to master for 19.11.00 It caused the misc4dev scripts to fail on inserting new bibliographic records. Fixed, update your repo! https://gitlab.com/koha-community/koha-misc4dev/merge_requests/31 Created attachment 92016 [details] [review] Bug 11529: (RM follow-up) Fix missing comma in query Looks like we introduced an error during a rebase somewhere on bug 11529. This patch siply replaces a missng comma in the SQL query for C4::HoldsQueue::GetHoldsQueueItems. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 92019 [details] [review] Bug 11529: Prevent too many spaces/break lines in the title tag Caught by t/db_dependent/www/batch.t Before this patch: """ <title>Koha › Catalog › Details for Bleak House / </title> """ After this patch: """ <title>Koha › Catalog › Details for Bleak House / </title> """ Created attachment 92020 [details] [review] Bug 11529: (RM follow-up) Add WHILE to directive list The QA tools don't current recognise the WHILE directive used in this template update. This patch adds the keyword to the list of recognised directives and thus allows through unfiltered variables during QA runs. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thanks for everything, including the follow-up fixes! Enhancement not pushed to 19.05.x Nice job, thanks all ;) Turns out the database update for this change causes unexpected changes to the "Koha to MARC mapping" (via the MARC frameworks) if you had additional non-standard "Koha to MARC mapping" entries. Personally, I wouldn't have updated the marc_subfield_structure automatically. Here's the query used: UPDATE marc_subfield_structure JOIN fieldmapping ON tagfield = fieldcode AND subfieldcode=tagsubfield SET kohafield='biblio.subtitle' WHERE fieldmapping.frameworkcode=''" This would be a better query: UPDATE marc_subfield_structure JOIN fieldmapping ON tagfield = fieldcode AND subfieldcode=tagsubfield SET kohafield='biblio.subtitle' WHERE fieldmapping.frameworkcode=''" and field='subtitle' Now I have a lot of different MARC fields mapped to biblio.subtitle [U+1F605] Of course, even the modified query wouldn't be great, because in my case 245$h would get mapped to biblio.title too which isn't what we'd want. We have seen this exact thing, and it's rather a huge problem for upgrading libraries going to 19.11 (In reply to David Cook from comment #243) > Turns out the database update for this change causes unexpected changes to > the "Koha to MARC mapping" (via the MARC frameworks) if you had additional > non-standard "Koha to MARC mapping" entries. > > Personally, I wouldn't have updated the marc_subfield_structure > automatically. > > Here's the query used: > UPDATE marc_subfield_structure JOIN fieldmapping ON tagfield = fieldcode AND > subfieldcode=tagsubfield SET kohafield='biblio.subtitle' WHERE > fieldmapping.frameworkcode=''" > > This would be a better query: > UPDATE marc_subfield_structure JOIN fieldmapping ON tagfield = fieldcode AND > subfieldcode=tagsubfield SET kohafield='biblio.subtitle' WHERE > fieldmapping.frameworkcode=''" and field='subtitle' > > Now I have a lot of different MARC fields mapped to biblio.subtitle [U+1F605] > > Of course, even the modified query wouldn't be great, because in my case > 245$h would get mapped to biblio.title too which isn't what we'd want. Created attachment 105667 [details] [review] Bug 11529: Do not update marc mappings, print old mappings to upgrade log (In reply to Kyle M Hall from comment #245) > Created attachment 105667 [details] [review] [review] > Bug 11529: Do not update marc mappings, print old mappings to upgrade log Nick and I have been looking into this and this patch seems to be the only reasonable solution. In a nutshell, what Nick was trying to accomplish with that update query just isn't possible anymore. (In reply to Kyle M Hall from comment #246) > (In reply to Kyle M Hall from comment #245) > > Created attachment 105667 [details] [review] [review] [review] > > Bug 11529: Do not update marc mappings, print old mappings to upgrade log > > Nick and I have been looking into this and this patch seems to be the only > reasonable solution. In a nutshell, what Nick was trying to accomplish with > that update query just isn't possible anymore. Agreed. Your patch is the same thing I've already implemented locally, and I haven't had any issues since. Created attachment 105672 [details] [review] Bug 11529: Do not update marc mappings, print old mappings to upgrade log Signed-off-by: David Cook <dcook@prosentient.com.au> Not sure if this needed a sign off but did one anyway. (In reply to David Cook from comment #249) > Not sure if this needed a sign off but did one anyway. A sign off is great, but we'll also need this to be moved to a separate bug - this bug has been long released and we want the fix to show up with a nice description in the release notes. |