Add options for write protecting individual MARC fields based on source of import (Z39.50, stage for import, etc) to avoid overwriting local changes. This feature is a superset of http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11780
+100!
Created attachment 44056 [details] [review] Bug 14957: Rule based permission for MARC fields This patch adds a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Apply related database patch 2. Apply related scripts update patch 3. Run updatedatabase.pl 5. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
Created attachment 44057 [details] [review] Bug 14957: Database changes
Created attachment 44058 [details] [review] Bug 14957: Add filters to relevant scripts Add MARCPermissions filters in scripts which modifies MARC records to enable the permission system for these scripts.
Just confirming the patches do not apply. Here is the error: $ git bz apply 14957 Bug 14957 - Write protecting MARC fields based on source of import 44056 - Bug 14957: Rule based permission for MARC fields 44057 - Bug 14957: Database changes 44058 - Bug 14957: Add filters to relevant scripts Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 14957: Rule based permission for MARC fields Using index info to reconstruct a base tree... M C4/Biblio.pm M koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt M koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt Auto-merging C4/Biblio.pm CONFLICT (content): Merge conflict in C4/Biblio.pm Failed to merge in the changes. Patch failed at 0001 Bug 14957: Rule based permission for MARC fields The copy of the patch that failed is found in: /home/magnus/scripts/kohaclone/.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-14957-Rule-based-permission-for-MARC-fields-IZUxzW.patch
Created attachment 51723 [details] [review] Bug 14957: Rule based permission for MARC fields This patch adds a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Apply related database patch 2. Apply related scripts update patch 3. Run updatedatabase.pl 5. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
Created attachment 51724 [details] [review] Bug 14957: Correct parameters for TransformMarcToKoha
I have rebased and merged this and it seems to work. I dont have any MARC experience so please test that this works in some real use case.
Oh I had forgotten about this one... might have to look at this sometime...
I am trying to test this, but after applying the patches (and fixing some conflicts), I get this error when I try to do a search, both in the OPAC and the intranet: Can't locate object method "fields" via package "DBI::db" at /home/kohaadmin/kohaclone/C4/Biblio.pm line 2673. Line 2673 looks like this: MARCFIELD: foreach my $field ( $record->fields() ) { I'll try to figure out the problem, but if anyone got any hunches I'm all ears!
Never mind me, I was applying the patches on the 3.22.x branch.
When you are on the "MARC field permissions" page, "MARC field permissions" is not shown in the left hand menu.
Failed to apply for me (merge conflicts). Needs a rebase again?
i can't test the patch with a sandbox biblibre i have this message The sandbox you've requested is not ready. Some problems occurred applying patches from bug 14957: <h1>Something went wrong !</h1>Applying: Bug 14957: Database changes Applying: Bug 14957: Add filters to relevant scripts .git/rebase-apply/patch:14: trailing whitespace. } .git/rebase-apply/patch:16: trailing whitespace. ModBiblioframework( , ); warning: 2 lines add whitespace errors. Using index info to reconstruct a base tree... M cataloguing/addbiblio.pl M koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt M misc/migration_tools/bulkmarcimport.pl M tools/batch_record_modification.pl .git/rebase-apply/patch:14: trailing whitespace. } .git/rebase-apply/patch:16: trailing whitespace. ModBiblioframework( , ); warning: 2 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging tools/batch_record_modification.pl CONFLICT (content): Merge conflict in tools/batch_record_modification.pl Auto-merging misc/migration_tools/bulkmarcimport.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt Auto-merging cataloguing/addbiblio.pl error: Failed to merge in the changes. Patch failed at 0001 Bug 14957: Add filters to relevant scripts 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. Bug 14957 - Write protecting MARC fields based on source of import 44057 - Bug 14957: Database changes 44058 - Bug 14957: Add filters to relevant scripts 51723 - Bug 14957: Rule based permission for MARC fields 51724 - Bug 14957: Correct parameters for TransformMarcToKoha Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-14957-Add-filters-to-relevant-scripts-7vvvqA.patch .
Created attachment 56637 [details] [review] Bug 14957: Key fixes to patch - Fixing merge conflicts - adding String::Similariry to list of dependencies - font-awesome icons and adding consistency to formatting - removing original use of onclick - moving help text out of main page and into a new help file When testing, follow above test plan but: 1) Remember to install String::Similarity before testing (apt-get install libstring-similarity-perl) 2) Instead of doing step 7, click Help in top-right corner instead. Confirm this works and shows as expected Sponsored-by: Catalyst IT
Created attachment 56638 [details] [review] Bug 14957: Key fixes to patch - Fixing merge conflicts - adding String::Similariry to list of dependencies - font-awesome icons and adding consistency to formatting - removing original use of onclick - moving help text out of main page and into a new help file When testing, follow above test plan but: 1) Remember to install String::Similarity before testing (apt-get install libstring-similarity-perl) 2) Instead of doing step 7, click Help in top-right corner instead. Confirm this works and shows as expected Sponsored-by: Catalyst IT
Created attachment 56643 [details] [review] Bug 14957: Database changes
Created attachment 56644 [details] [review] Bug 14957: Add filters to relevant scripts Add MARCPermissions filters in scripts which modifies MARC records to enable the permission system for these scripts.
Created attachment 56645 [details] [review] Bug 14957: Rule based permission for MARC fields This patch adds a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Apply related database patch 2. Apply related scripts update patch 3. Run updatedatabase.pl 5. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
Created attachment 56646 [details] [review] Bug 14957: Correct parameters for TransformMarcToKoha
Created attachment 56647 [details] [review] Bug 14957: Key fixes to patch - Fixing merge conflicts - adding String::Similariry to list of dependencies - font-awesome icons and adding consistency to formatting - removing original use of onclick - moving help text out of main page and into a new help file When testing, follow above test plan but: 1) Remember to install String::Similarity before testing (apt-get install libstring-similarity-perl) 2) Instead of doing step 7, click Help in top-right corner instead. Confirm this works and shows as expected Sponsored-by: Catalyst IT
Created attachment 56648 [details] [review] Bug 14957: Followup - Pod, import_lexile.pl, template and test fixes
Rebased on master Fixed few issues in the follow-up
Doesn't apply for me. Needs yet another rebase?
Created attachment 57114 [details] [review] Bug 14957: Database changes
Created attachment 57115 [details] [review] Bug 14957: Add filters to relevant scripts Add MARCPermissions filters in scripts which modifies MARC records to enable the permission system for these scripts.
Created attachment 57116 [details] [review] Bug 14957: Rule based permission for MARC fields This patch adds a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Apply related database patch 2. Apply related scripts update patch 3. Run updatedatabase.pl 5. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
Created attachment 57117 [details] [review] Bug 14957: Correct parameters for TransformMarcToKoha
Created attachment 57118 [details] [review] Bug 14957: Key fixes to patch - Fixing merge conflicts - adding String::Similariry to list of dependencies - font-awesome icons and adding consistency to formatting - removing original use of onclick - moving help text out of main page and into a new help file When testing, follow above test plan but: 1) Remember to install String::Similarity before testing (apt-get install libstring-similarity-perl) 2) Instead of doing step 7, click Help in top-right corner instead. Confirm this works and shows as expected Sponsored-by: Catalyst IT
Created attachment 57119 [details] [review] Bug 14957: Followup - Pod, import_lexile.pl, template and test fixes
It really needed rebasing again. Done.
I started testing this today and after some initial confusion about the rules system (that could use some more documentation!) it seems mostly to work as intended. But am I correct in understanding that Marc field permissions does not currently work with Staged MARC Imports?
Created attachment 59811 [details] [review] Rule based permission for MARC fields Rebased against master as single patch. Behaviour of filter modules slightly tweaked.
Created attachment 59812 [details] [review] Rule based permission for MARC fields Oops, forgot to remove some temporary comments.
The previously posted version of this patch at least for me did not apply cleanly. Rebased against current HEAD of master, and also slightly altered the behavior of "GetMarcPermissionsAction". The previous behaviour struck me as odd since a module with a higher specificity will always override modules with lower, even for the case when a filter argument for a module with lower specificity has an exact match. So for example, if we have a rule with filter 'source' => 'bulkmarcimport', and another for 'borrower' => '*', the later rule will always have precedence, even when source is matching 'bulkmarcimport'. I seems more flexible to have the option to override on a more specific filter match. Generally I think this patch is great, and almost exactly what we need, but the whole filter-system I think is in need of some revamping. It would be nice if the modules where more like separate namespaces, so that one set of rules (belonging to a certain module) was applied at once. Right now rules from several modules might be mixed in, and field-specificity has precedence in rules selection. The code could be simplified and optimized if rules where grouped by module instead (and also easier to admin from a user perspective I would believe, since rules precedence would be more obvious. But have made no such changes in the current patch, as wanted to bring this up for discussion before make any large breaking changes.
Patch doesn't apply for my (using Kohadevbox). I get the following error: Traceback (most recent call last): File "/usr/local/bin/git-bz", line 2630, in <module> do_apply(*args) File "/usr/local/bin/git-bz", line 1755, in do_apply users = bug.server.get_xmlrpc_proxy().User.get({ 'names': [patch.attacher] })['users'] File "/usr/lib/python2.7/xmlrpclib.py", line 1233, in __call__ return self.__send(self.__name, args) File "/usr/lib/python2.7/xmlrpclib.py", line 1591, in __request verbose=self.__verbose File "/usr/lib/python2.7/xmlrpclib.py", line 1273, in request return self.single_request(host, handler, request_body, verbose) File "/usr/lib/python2.7/xmlrpclib.py", line 1321, in single_request response.msg, xmlrpclib.ProtocolError: <ProtocolError for bugs.koha-community.org/xmlrpc.cgi: 404 Not Found>
(In reply to Andreas Hedström Mace from comment #36) > Patch doesn't apply for my (using Kohadevbox). I get the following error: ... > xmlrpclib.ProtocolError: <ProtocolError for > bugs.koha-community.org/xmlrpc.cgi: 404 Not Found> That looks more like an error from git bz than from git itself. 404 Not Found - are you sure git bz is setup properly and has all the configs it needs?
I think I might not have followed the guidelines for submitting patches properly. Will try to see if I can generate a patch using the recommended tools (git bz etc).
(In reply to David Gustafsson from comment #38) > I think I might not have followed the guidelines for submitting patches > properly. Will try to see if I can generate a patch using the recommended > tools (git bz etc). Indeed, it looks more like a regular diff than a git patch. It seems odd that this should trigger the error that Andreas got, though. But I tested, an I get the same error on this patch while other patches work as they should. A new patch looks like a good place to start, though. :-)
(In reply to Magnus Enger from comment #39) > Indeed, it looks more like a regular diff than a git patch. It seems odd > that this should trigger the error that Andreas got, though. But I tested, > an I get the same error on this patch while other patches work as they > should. A new patch looks like a good place to start, though. :-) Yeah, as I have never seen such an error either I made sure to check that other patches applied as intended, which they did. Sounds great if you can provide a follow-up David!
Created attachment 59885 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
Ok! One more try. A little bit unsure how to handle attribution, right now looks like the patch is attributed to me which is wrong. But hope it will at least apply now (also hope I did not make any mistake during the rebase, when through the patch several times to check everything adds up, but still might have botched something up).
Created attachment 60711 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
Rebased against master.
Created attachment 60824 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
Screwed up the last merge. Should be fixed now.
Nope, still managed to screw it up. Should be ok now.
Created attachment 60825 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
Have discovered a lot of weirdness in how permissions are applied, not sure if this is something I have introduces or present in the original patch, but don't merge in this quite yet. Will update the patch some time in the coming days.
Created attachment 61065 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
Made some small adjustments: - Removed $options-{filter} arguments from ModBiblioMarc in AddBiblio since ApplyMarcPermissions is never run on adding new biblios anyways (since no caller uses it, and marc-xml for current biblionumber will always be empty on save and ApplyMarcPermissions will bail out anyway), and to provide that arguments makes things even more confusing. - Added source => 'batchimport' for staged import batches. So now ApplyMarcPermissions runs also on staged imports. One thing I did not realize before is that marc permissions are never enforced adding new biblios, this is also the case for staged imports. Only if a match is found permissions will be applied. It would probably be more appropriate to call the current system "Marc merge rules" or something like that, since it is really a framework for defining what happens when two marc records are to be merged (and not "permissions" in the strict sense of the word).
Created attachment 61641 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administration". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
corrected spelling error in commit message.
Created attachment 61736 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
As another criticism, some of the things that should be easy are not, in particular, Server-Side Rendering. Ember’s implementation of SSR requires two simple entries on the command-line. Vue’s requires a simple wrapper around standard objects. AngularJS’ implementation is honestly kind of awful but at least well documented with a whole dedicated sub-site. This is Meteor’s implementation: ¯\_(ツ)_/¯
Ops, wrong copy paste. Fixed missing "String::Similarity" include in Biblio.pm, and changed $filter argument in ModBiblio to more flexible $options to avoid future merge conflicts and provide a more stable API.
Created attachment 64792 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
The last change (adding the $options parameter) contained several mistakes, (hopefully) fixed those now. Have run the tests and did some manual testing (which I apparently did not do last time).
Wanted to to try and test this again, but patch doesn't apply for me.
Have discovered several serious issues with the current patch in this issue, so do not recommend that any one uses it or re-roll it against master. My personal view is that the current approach is based on some invalid assumptions (that positions of fields in marc is stable and carry significance for example). Have rewritten most of the core logic, and could perhaps submit it here when cleaned up. But for our needs (Gothenburg University Library) I'm not sure we want to get this in to Koha, but instead refactor and use as a plugin, but because of time-constraints I'm not sure will have time to submit the code for at least a couple of weeks.
Hi David, we've always been keen on this one. If you have something to submit, I'd be interested in testing. As we work in a union catalog setting (union catalog is 'master'), Koha's missing ability to protect fields and subfields from being overwritten is sometimes a problem for us.
Hi! The code can be found here, but there are some parts (help section, tests, user UI) that has not really been adjusted for the new code. Have performed some limited manual testing and seems to work, but it will probably be confusing to configure for someone who has not looked through the code first: https://github.com/ub-digit/Koha/tree/bug_14957 I will try to clean up, make ui more intuitive, and document the functionality, but right now as previously noted there is not enough time for me to do this.
Hi David, thx for the feedback. Moving this to Failed QA as it seems more appropriate for now.
Created attachment 72498 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library
Comment on attachment 72498 [details] [review] Bug 14957 - Rule based permission for MARC fields $(this).attr('title', 'Invalid regular expression: ' + e.message); should be $(this).attr('title', _("Invalid regular expression: %s").format(e.message));
Attaching the patch with latest changes even though not yet ready for sign off to not block any further contributions by someone else. Since the model for how rules work has changed since the "old" version of the patch tests and help pages needs updating to reflect these changes, which has not been done. I will have time to work some more on this in about a month or so, but since the patch is functional for our library in it's current state it will not get much more love from us until then.
Hi David, thx for submitting. Peeking at the code I am not quite sure how it works, but notice it adds a lot of code to C4. Would probably need quite a bit of work.
Created attachment 74855 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Removed String::Similarity dependency and fixed an issue with the field data hashing function.
@Katrin Fischer Yes it could use some work, still have not gotten around to updating the help-pages and tests. We are currently running this in production though and has been doing so for a couple of weeks. It could take some time though before have time to fix the above.
Created attachment 75118 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Rebase against master and remove (probably last of) String::Similarity related code. Have not had time to work on the other issues. We have been running this in production for a couple of weeks now, and have not experienced any issues.
Which I now saw I already wrote about in previous comment. Still need to fix tests and documentation as the bare minimum before ready for sign-off.
I'm actively working on fixing the remaining issues and moving relevant things from C4 into the Koha namespace. Will probably submit changes tomorrow or beginning of next week.
Created attachment 76201 [details] [review] Bug 14957 - Rule based permission for MARC fields Change "MARC permissions" to "MARC merge rules" since this is more descriptive of the feature provided. Update help page to reflect changes since large refactorization. Replace outdated tests with up to date tests. Improve rules admin UI adding "preset" setting. Move large part of added C4::Biblio code into Koha:: namespace.
Created attachment 76202 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 76203 [details] [review] Bug 14957 - Rule based permission for MARC fields Change "MARC permissions" to "MARC merge rules" since this is more descriptive of the feature provided. Update help page to reflect changes since large refactorization. Replace outdated tests with up to date tests. Improve rules admin UI adding "preset" setting. Move large part of added C4::Biblio code into Koha:: namespace.
Created attachment 76204 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 76205 [details] [review] Bug 14957 - Refactor and fix tests and documention Change "MARC permissions" to "MARC merge rules" since this is more descriptive of the feature provided. Update help page to reflect changes since large refactorization. Replace outdated tests with up to date tests. Improve rules admin UI adding "preset" setting. Move large part of added C4::Biblio code into Koha:: namespace.
Created attachment 76206 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 76207 [details] [review] Bug 14957 - Refactor and fix tests and documentation Change "MARC permissions" to "MARC merge rules" since this is more descriptive of the feature provided. Update help page to reflect changes since large refactorization. Replace outdated tests with up to date tests. Improve rules admin UI adding "preset" setting. Move large part of added C4::Biblio code into Koha:: namespace.
Created attachment 76208 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 76209 [details] [review] Bug 14957 - Refactor and fix tests and documentation Change "MARC permissions" to "MARC merge rules" since this is more descriptive of the feature provided. Update help page to reflect changes since large refactorization. Replace outdated tests with up to date tests. Improve rules admin UI adding "preset" setting. Move large part of added C4::Biblio code into Koha:: namespace.
Sorry about the many attachments, had some issues adding the latest patch. Now everything should be in order.
Created attachment 76210 [details] [review] Bug 14957 - Remove exports of removed subroutines Remove exports of subroutines previously removed
Hi David, thank you for your work on this!
Thanks. Since this is a rather big change there probably are some minor undiscovered issues with the current patch and we are still running with the prior version in production. The tests are pretty exhaustive though, so I would be surprised if there are any major undiscovered problems. Usability and documentation is probably where there is still most room for improvement and some feedback would be useful to improve on those. But should more or less be ready for sign-off.
Unable to apply patch in a number of Biblibre sandboxes even after several attempts.
The sandbox you've requested is not ready. Some problems occurred applying patches from bug 14957: <h1>Something went wrong !</h1>Applying: Bug 14957 - Rule based permission for MARC fields .git/rebase-apply/patch:871: trailing whitespace. } warning: 1 line adds whitespace errors. Using index info to reconstruct a base tree... M C4/Biblio.pm M cataloguing/addbiblio.pl M koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc M koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt M misc/migration_tools/bulkmarcimport.pl .git/rebase-apply/patch:871: trailing whitespace. } warning: 1 line adds whitespace errors. Falling back to patching base and 3-way merge... Auto-merging misc/migration_tools/bulkmarcimport.pl CONFLICT (content): Merge conflict in misc/migration_tools/bulkmarcimport.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/admin-home.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/admin-menu.inc Auto-merging cataloguing/addbiblio.pl Auto-merging C4/Biblio.pm Patch failed at 0001 Bug 14957 - Rule based permission for MARC fields 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. error: Failed to merge in the changes. Bug 14957 - Write protecting MARC fields based on source of import 76208 - Bug 14957 - Rule based permission for MARC fields 76209 - Bug 14957 - Refactor and fix tests and documentation 76210 - Bug 14957 - Remove exports of removed subroutines Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-14957---Rule-based-permission-for-MARC-fields-eC50px.patch .
Created attachment 78651 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 78652 [details] [review] Bug 14957 - Refactor and fix tests and documentation Change "MARC permissions" to "MARC merge rules" since this is more descriptive of the feature provided. Update help page to reflect changes since large refactorization. Replace outdated tests with up to date tests. Improve rules admin UI adding "preset" setting. Move large part of added C4::Biblio code into Koha:: namespace.
Created attachment 78653 [details] [review] Bug 14957 - Remove exports of removed subroutines Remove exports of subroutines previously removed
Rebased against master. Patches should now apply. Have not tested that still works though. I will report back once have had the time to test, or if someone else does before me.
Hi David, sorry the patch doesn't apply : Applying: Bug 14957 - Rule based permission for MARC fields Using index info to reconstruct a base tree... M help.pl Falling back to patching base and 3-way merge... Auto-merging help.pl CONFLICT (content): Merge conflict in help.pl Failed to merge in the changes.
I think I might be repeating myself, probably on a different bug. But I think we should add both whitelisting and blacklisting 'MARC record overlay configurations', and then hook them into whatever place we want (i.e. for z39.50, for OAI harvsting, etc). It seems to me that we are duplicating efforts in some places, and solutions don't look generic enough. 'Protecting' is just a way to say you are blacklisting some fields on overlaying.
Created attachment 79367 [details] [review] Bug 14957 - Remove exports of removed subroutines Remove exports of subroutines previously removed
Created attachment 79368 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 79369 [details] [review] Bug 14957 - Refactor and fix tests and documentation Change "MARC permissions" to "MARC merge rules" since this is more descriptive of the feature provided. Update help page to reflect changes since large refactorization. Replace outdated tests with up to date tests. Improve rules admin UI adding "preset" setting. Move large part of added C4::Biblio code into Koha:: namespace.
Created attachment 79370 [details] [review] Bug 14957 - Remove exports of removed subroutines Remove exports of subroutines previously removed
(In reply to Tomás Cohen Arazi from comment #95) > I think I might be repeating myself, probably on a different bug. But I > think we should add both whitelisting and blacklisting 'MARC record overlay > configurations', and then hook them into whatever place we want (i.e. for > z39.50, for OAI harvsting, etc). It seems to me that we are duplicating > efforts in some places, and solutions don't look generic enough. > > 'Protecting' is just a way to say you are blacklisting some fields on > overlaying. Now rebased again, so should work with latest master. "Protecting" is probably a little bit misleading to reflect what the patch currently does, it has been largely refactored since the first version (which I did not write). I have renamed it to MARC merge rules instead, since it is more of a framework for auto-merging records according to a set or rules. My experience is that is is really tricky to get this right, and if trying to fit it into another abstraction that might not be suited for this exact purpose it could be more trouble than it's worth (https://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction).
(In reply to David Gustafsson from comment #100) > (In reply to Tomás Cohen Arazi from comment #95) > > I think I might be repeating myself, probably on a different bug. But I > > think we should add both whitelisting and blacklisting 'MARC record overlay > > configurations', and then hook them into whatever place we want (i.e. for > > z39.50, for OAI harvsting, etc). It seems to me that we are duplicating > > efforts in some places, and solutions don't look generic enough. > > > > 'Protecting' is just a way to say you are blacklisting some fields on > > overlaying. > > Now rebased again, so should work with latest master. "Protecting" is > probably a little bit misleading to reflect what the patch currently does, > it has been largely refactored since the first version (which I did not > write). I have renamed it to MARC merge rules instead, since it is more of a > framework for auto-merging records according to a set or rules. I hadn't noticed that. We should change the bug name to reflect what this actually does. > My experience is that is is really tricky to get this right, and if trying to > fit it into another abstraction that might not be suited for this exact > purpose it could be more trouble than it's worth > (https://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction). While I agree with the assertion, I don't think it is the case. What I'm saying is that we could just add a boolean (whitelist/blacklist) to specify a different behaviour, using the same code and rules. That's something that would belong to a separate bug, of course. I just mentioned here because I thought 'Protect' could be avoided, which you already did :-D
Created attachment 79890 [details] [review] Bug 14957 - Add missing use in Biblio.pm
Ok :) Fixed an issue where Biblio.pm was missing "use Koha::MarcMergeRules". I missed this since the test works (where file was incluced), but obviously had not tested the code in the staff client.
Patch doesn't apply Bug 14957 - Write protecting MARC fields based on source of import 79368 - Bug 14957 - Rule based permission for MARC fields 79369 - Bug 14957 - Refactor and fix tests and documentation 79370 - Bug 14957 - Remove exports of removed subroutines 79890 - Bug 14957 - Add missing use in Biblio.pm Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 14957 - Rule based permission for MARC fields .git/rebase-apply/patch:871: trailing whitespace. } warning: 1 line adds whitespace errors. Using index info to reconstruct a base tree... M C4/Biblio.pm M cataloguing/addbiblio.pl M koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref .git/rebase-apply/patch:871: trailing whitespace. } warning: 1 line adds whitespace errors. Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref Auto-merging cataloguing/addbiblio.pl Auto-merging C4/Biblio.pm CONFLICT (content): Merge conflict in C4/Biblio.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 14957 - Rule based permission for MARC fields 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-14957---Rule-based-permission-for-MARC-fields-WiTsgs.patch
Created attachment 80972 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch 2. Run updatedatabase.pl 3. Log in to staff client 4. Enable new syspref MARCPermissions 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". 6. Go to the "MARC field permissions" page 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this for more information about the rule system. 8. Add desired rules to the rule table, press the +-button to add. 9. Clicking the pen-icon (edit) should allow you to edit corresponding rule. 10. Clicking the x-icon should remove corresponding rule after confirmation. 11. Selecting one or more rules followed by clicking the trash-icon should remove all selected rules after confirmation. 12. To test your rules, import or in other way modify a record. 13. Check that the record was modified as defined by your rules. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 80973 [details] [review] Bug 14957 - Refactor and fix tests and documentation Change "MARC permissions" to "MARC merge rules" since this is more descriptive of the feature provided. Update help page to reflect changes since large refactorization. Replace outdated tests with up to date tests. Improve rules admin UI adding "preset" setting. Move large part of added C4::Biblio code into Koha:: namespace.
Created attachment 80974 [details] [review] Bug 14957 - Remove exports of removed subroutines Remove exports of subroutines previously removed
Created attachment 80975 [details] [review] Bug 14957 - Add missing use in Biblio.pm
Rebased against master. I think I was responsible for the conflict this time (bug 19687) :)
Created attachment 81176 [details] [review] Bug 14957: Fix control field rules regression and add rule validation
Created attachment 81177 [details] [review] Bug 14957: Fix control field rules regression and add rule validation
Created attachment 81178 [details] [review] Bug 14957: Fix control field rules regression and add rule validation
Created attachment 81179 [details] [review] Bug 14957: Fix control field rules regression and add rule validation
Applied fine, but there were runtime errors: 5. You should see a new link "MARC field permissions" in the "Catalog" section under "Koha administrtion". --> Link Changed to "MARCMergeRules" 7. Next to the page heading ("Manage MARC field permissions") there is a button with a question mark, click this r more information about the rule system. --> Link labeled "MARC merge rules" --> Page throws error "DBIx::Class::Schema::source(): Can't find source for MarcMergeRule at /home/vagrant/kohaclone/Koha/Objects.pm line 341 at /usr/share/perl5/DBIx/Class/Exception.pm line 77" --cori lynn
(In reply to Cori Lynn Arnold from comment #114) > Applied fine, but there were runtime errors: > > 5. You should see a new link "MARC field permissions" in the "Catalog" > section under "Koha administrtion". > --> Link Changed to "MARCMergeRules" > > 7. Next to the page heading ("Manage MARC field permissions") there is a > button with a question mark, click this r more information about the > rule system. > --> Link labeled "MARC merge rules" > --> Page throws error > "DBIx::Class::Schema::source(): Can't find source for MarcMergeRule at > /home/vagrant/kohaclone/Koha/Objects.pm line 341 > at /usr/share/perl5/DBIx/Class/Exception.pm line 77" > > --cori lynn Regarding the commit message I'm a little bit confused about how to handle. If I edit the commit message for the first commit it no longer reflect the changes for that commit, on the other hand the "To test" is partly outdated, so perhaps it should have been rewritten in the "Bug 14957 - Refactor and fix tests and documentation"-patch. It seems the local help feature has been removed in Bug 19817, so work is needed to move it into the manual instead? Regarding the "DBIx::Class::Schema::source()" I think you need to run the "misc/devel/update_dbix_class_files.pl" script to regenerate the Schema-files, as those are not to be included in the patch. Perhaps there is a convention to include them an attachment anyway, see if I can find an example of such a bug.
Created attachment 81621 [details] [review] Bug 14957: DO NOT PUSH Schema changes
Pushed the schema changes and followed the convention in Bug 19482.
I could also squash all the changes and add a new updated "To test", anyone has some input on this?
Thoughts in IRC are that squishing would be fine and to please re-write the tests to make them clearer and easier to follow with correct information.
Ok, help pages will have to moved to online manual as previously noted.
Created attachment 81699 [details] [review] Bug 14957 - Rule based permission for MARC fields Add a rule based permission system for MARC field modifications. Test plan: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245a, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245a rule in step 5 in place, edit a bibliographic record, change 245a (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245a, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 81700 [details] [review] Bug 14957: DO NOT PUSH Schema changes
Created attachment 81705 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245a, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245a rule in step 5 in place, edit a bibliographic record, change 245a (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245a, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 81706 [details] [review] Bug 14957: DO NOT PUSH Schema changes
Created attachment 81707 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245a, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245a rule in step 5 in place, edit a bibliographic record, change 245a (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245a, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 81708 [details] [review] Bug 14957: DO NOT PUSH Schema changes
10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. ==Error is generated, but rule still is created. User has to delete the invalid rule. Seems like it shouldn't be created as with the other error conditions. 11. With the 245a rule in step 5 in place, edit a bibliographic record, change 245a (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. ==Changes *are* saved. Attempted to edit a record already in the catalog, and creating a new one via Z39 search. Can you clarify how these rules should be applied for "edit record"?
Created attachment 81969 [details] [review] Bug 21596: (follow-up) Handle updated_on part 2 It seems that I have over complicated things here, now t/db_dependent/Koha/Object.t is failing Apparently everything is ok with this change. Test plan: prove -r prove -r t/db_dependent/Koha/Object.t t/Auth_with_shibboleth.t t/db_dependent/Patrons.t t/db_dependent/Koha/Patron* Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 81970 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 81971 [details] [review] Bug 14957: DO NOT PUSH Schema changes
(In reply to Cori Lynn Arnold from comment #127) > > 10. Try creating a rule with tag set to "008" (or other control > field) and set Appended: Append and Removed: Skip, the other > options does not matter. Verify that saving this rule produces > an error message complaining about invalid combination of actions > for control field. > ==Error is generated, but rule still is created. User has to delete the > invalid rule. Seems like it shouldn't be created as with the other error > conditions. > > > 11. With the 245a rule in step 5 in place, edit a bibliographic record, > change 245a (which should be Title for MARC21) and save. > 12. Verify that the changes has not been saved. > ==Changes *are* saved. Attempted to edit a record already in the catalog, > and creating a new one via Z39 search. Can you clarify how these rules > should be applied for "edit record"? I accidentally attached another patch, so that comment can be ignored. The issue where rules are saved also when validation fails should be fixed. The "To test"-instruction regarding 245a was wrong, as only tag expressions are allowed (not subfields). Since even I forgot about that perhaps that should be clarified, or [a-z] not allowed in field patterns.
Created attachment 82030 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 82031 [details] [review] Bug 14957: DO NOT PUSH Schema changes
Fixed too long foreign key length causing troubles with MariaDB.
Created attachment 82775 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Rebased against master and added granular permission in accordance with Bug 14391.
*** Bug 22354 has been marked as a duplicate of this bug. ***
The patch is tested and works fine. Nice work!
Hi Frank! Can you please add a sign-off line to the patch too? This can be done for example by using the sandboxes: https://wiki.koha-community.org/wiki/Sandboxes If you can't that's ok, we can also fix it in the QA step.
Katrin, I have already signed off the patches from a sandbox. I'm getting: koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc: needs merge Guess, I don't have a change to do that. The sandbox is here: http://sb1.sandboxes.bywatersolutions.com if somebody want to try.
(In reply to Frank Hansen from comment #140) > Katrin, I have already signed off the patches from a sandbox. I'm getting: > koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc: needs merge > > Guess, I don't have a change to do that. > > The sandbox is here: http://sb1.sandboxes.bywatersolutions.com if somebody > want to try. Hi Frank, looks like we might need a rebase - master is changing fast and sometimes there are conflicts in files changed by different patches. We will fix it.
Created attachment 87709 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenbug University Library
Created attachment 87711 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library
Taking a first view here, starting with the QA script: 1) QA script failures: FAIL koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-merge-rules.tt FAIL filters missing_filter at line 376 ( <input type="checkbox" name="batchremove" value="[% rule.id %]"[% IF rule.removemarked %] checked="checked"[% END %]/>) missing_filter at line 364 ( <td>[% rule.filter %]</td>) missing_filter at line 372 ( <a href="?op=remove&id=[% rule.id %]" title="Delete" class="btn btn-mini"><i class="fa fa-trash"></i> Delete</a>) missing_filter at line 317 ( <option value="[% module.id %]">[% module.name %]</option>) missing_filter at line 373 ( <a href="?op=edit&id=[% rule.id %]" title="Edit" class="btn btn-mini"><i class="fa fa-pencil"></i> Edit</a>) missing_filter at line 363 ( <td>[% rule.module %]</td>) missing_filter at line 308 ( <tr id="[% rule.id %]" class="rule[% IF rule.edit %]-edit[% END %]">) missing_filter at line 315 ( <option value="[% module.id %]" selected="selected">[% module.name %]</option>) missing_filter at line 234 (<form action="[% script_name %]" method="GET">) missing_filter at line 328 ( <option value="[% preset %]">[% preset %]</option>) missing_filter at line 357 ( <button class="btn btn-mini" title="Save" id="doedit" value="[% rule.id %]"><i class="fa fa-check"></i> Save</button>) missing_filter at line 323 ( <td><input type="text" size="3" name="tag" value="[% rule.tag %]"/></td>) wrong_html_filter at line 373 ( <a href="?op=edit&id=[% rule.id %]" title="Edit" class="btn btn-mini"><i class="fa fa-pencil"></i> Edit</a>) missing_filter at line 5 (<link rel="stylesheet" type="text/css" href="[% interface %]/[% theme %]/css/datatables.css" />) missing_filter at line 310 ( <td>[% rule.id %]</td>) missing_filter at line 385 (<form action="[% script_name %]" method="post">) missing_filter at line 365 ( <td>[% rule.tag %]</td>) missing_filter at line 241 (<form action="[% script_name %]" method="POST" id="marc-merge-rules-form">) wrong_html_filter at line 372 ( <a href="?op=remove&id=[% rule.id %]" title="Delete" class="btn btn-mini"><i class="fa fa-trash"></i> Delete</a>) missing_filter at line 362 ( <td>[% rule.id %]</td>) missing_filter at line 263 ( <option value="[% module.id %]">[% module.name %]</option>) missing_filter at line 273 ( <option value="[% preset %]">[% preset %]</option>) missing_filter at line 322 ( <td><input type="text" size="5" name="filter" value="[% rule.filter %]"/></td>) FAIL forbidden patterns forbidden pattern: Do *not* include full path to css files, use the Asset TT plugin (bug 20538) (line 5) forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 372) forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 373) forbidden pattern: Attribute type should not be used for script tags (bug 20053) (line 14) forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 357) forbidden pattern: Attribute type should not be used for style tags (bug 20054) (line 8) forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 302) forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 358) forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 301) OK git manipulation FAIL js_in_body A <script> tag found inside head, must be moved to the body (see bug 17858) OK spelling FAIL tt_valid lines 334, 335, 340, 341, 346, 347, 352, 353, 376 OK valid_template FAIL Koha/MarcMergeRule.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for 'store' OK spelling OK valid FAIL Koha/MarcMergeRules.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for delete POD is missing for find_or_create POD is missing for object_class POD is missing for operations POD is missing for update POD is missing for validate OK spelling OK valid FAIL Koha/MarcMergeRule.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for 'store' OK spelling OK valid FAIL Koha/MarcMergeRules.pm OK critic OK forbidden patterns OK git manipulation OK pod FAIL pod coverage POD is missing for delete POD is missing for find_or_create POD is missing for object_class POD is missing for operations POD is missing for update POD is missing for validate OK spelling OK valid 2) Please merge your 3 atomic update SQL files into one .perl file. That will make the RM happy. 3) There is no sign-off line on the patch?
Comment on attachment 87711 [details] [review] Bug 14957: Merge rules system for merging of MARC records Review of attachment 87711 [details] [review]: ----------------------------------------------------------------- ::: C4/Biblio.pm @@ +262,3 @@ > =cut > > sub ModBiblio { 4) Changes to ModBiblio should be covered by unit tests. @@ +3615,4 @@ > return $record; > } > > +=head2 ApplyMarcMergeRules 5) New method needs to be covered by unit tests. ::: admin/marc-merge-rules.pl @@ +15,5 @@ > +# You should have received a copy of the GNU General Public License > +# along with Koha; if not, see <http://www.gnu.org/licenses>. > + > +use strict; > +use warnings; 6) Should be: Modern::Perl 7) Changes to kohastructure.sql are missing.
8) Changes to syspref.sql are missing 9) There are some default values set in the database updat that can't go into kohastructure.sql, might have to go into a new SQL file in the installers. 10) Looking at the setup table I'll also need documentation, pretty please! Right now, I have no clue on how to use this. Good stuff too: Unit tests pass, new permission is good too.
Created attachment 87771 [details] [review] Bug 14957: Fix most Koha QA test tool errors
Created attachment 87929 [details] [review] Bug 14957: Put database updates in perl script Also add schema changes to kohastructure.sql and syspref to sysprefs.sql
Created attachment 88205 [details] [review] Bug 14957: Put database updates in perl script Also add schema changes to kohastructure.sql and syspref to sysprefs.sql https://bugs.koha-community.org/show_bug.cgi?id=14956
Created attachment 88206 [details] [review] Bug 14957: Add tests and pod for new options param to ModBiblio https://bugs.koha-community.org/show_bug.cgi?id=14956
(In reply to David Gustafsson from comment #151) > https://bugs.koha-community.org/show_bug.cgi?id=14956 I think that URL should probably be https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14957, although I'm not sure why it's there either? In any case, intrigued to see how these patches progress.
Created attachment 89961 [details] [review] Bug 14957: Fix categorycode module filter value and rename some presets
Added some documentation here: https://gitlab.com/gnucifer/koha-manual/commits/bug-14957-marc-merge-rules I could create a pull request, but perhaps better to await some feedback before doing that.
Hi David, 1) Documentation https://gitlab.com/gnucifer/koha-manual/commit/593c5da008dbc4b074cac9dee167cce9a6dfc250#c5e2c75dc923f21c5b95185b2f350df00fec661f Thanks a lot, this documentation is SO helpful. We really couldn't have continued without as the feature is quite complex. 2) GUI - Terminology and translatability One thing that bothers me is that the GUI is a big 'rough'. I'd much prefer if it was using better terms/descriptions like in the help instead of 'codes'. bachmod = Bach record modification intranet = Cataloging module (unles it inlcudes batchmod?) batchimport = Staged MARC import etc. batchmarcimport = Cronjob batchmarcimport.pl (I hope I got them right) Experience tells that those are likely to get translated strangely or might not be translatable at all. 3) Over-granularity? Another thing that bothers me is a bit is the granularity. Being able to limit to specific patrons seems a little overkill? Especially since the interface expects borrowernumbers to be entered? (borrower = patron) Is there a use case for this? 4) marc_merge_rules_modules I am still quite suspicious of this one. We should not add descriptions to the database ever (translatability) - and this currently won't work for new installations. Can you please consider moving the modules to the templates instead and checking if this table is really needed as is? Or explain to me? At the moment the patch set doesn't apply. - I am really interested in the feature, just feel right now that it's an old development that still needs a bit of love.
Thanks for the feedback. I more or less agree with all of pointed out issues and not that it matters but those design choices, including the somewhat cryptic naming of modules, were not made by me but comes from the original version of the patch. I have not addressed them since just wanted to spend the minimal effort for the patch to get into Koha and have focused on the issues brought up thus far. But at least 4) has been brought up before and I should perhaps have fixed that in the current iteration of the patch since the modules are added in an atomic update and I saw no other instance where data is inserted in kohastructure.sql. Thus if making a fresh Koha installation this data will be missing which is not acceptable. Since the modules are not invoked dynamically in the code, but are hard coded, it makes more sense for them to be defined as constants, which would resolve the database population issue. I think this should be pretty uncomplicated to refactor, by hard coding the modules in the template (to make the label translatable) making that the authoritative source. In my opinion there is a rather urgent need for having translatable strings in Koha without (ab)using the template system, but that is out of the scope of this bug. About 4) I agree that is to use borrowernumber is not pretty, I guess the reason it is used instead of userid is it is available in scope without having to look up the userid in a database call, but userid (username) would be better. I also agree that it is probably a very rare use-case to need to specify rules per user, but on the other hand, since the cost of providing this context is relatively low, it might also be a little bit presumptuous to remove the option. For example, it is a simple way to allow certain super-users to override all rules by creating a wildcard "allow all" rule without having to reserve a user category for this purpose.
3) was more an observation at this point. If you could rebase and take care of 4) I'd love to continue testing. This is a bit "too big" overall... and shoudl have been split into smaller bits and pieces...(too late now, I know, but we've seen this with a lot of things) But I hope we can make it move.
Created attachment 90668 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library
Created attachment 90669 [details] [review] Bug 14957: DO NOT PUSH Schema changes
Created attachment 90671 [details] [review] Bug 14957: DO NOT PUSH Schema changes
Created attachment 90672 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library
Fixed most of the issues, rebased and squashed into one commit as things were getting a little bit messy. There are still some things that might need fixing for example I'm not sure if the marc rule presets are translatable as they are defined as js-data. I haven't used the translation tools myself but will try to figure this out.
Also did a pod change in C4/Biblio.pm without the output so need also to do that.
Created attachment 90722 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library
Created attachment 90723 [details] [review] Bug 14957: Make preset labels translatable
I managed to find documentation on how to make javascript strings translatable here: https://wiki.koha-community.org/wiki/Coding_Guidelines#JS2:_Enabling_translation_of_language_strings_in_embedded_JavaScript I'm a little bit unsure if the inline template toolkit strings are translatable though and don't know how to test if they are. For example: [% FOR preset IN [ 'Protect', 'Overwrite', 'Add new', 'Add and append', 'Protect from deletion' ] %]
Created attachment 90740 [details] [review] Bug 14957: Make preset labels translatable
Created attachment 90741 [details] [review] Bug 14957: Make preset labels translatable
Fixed documentation (borrower changed to userid). https://gitlab.com/gnucifer/koha-manual/tree/bug-14957-marc-merge-rules
Created attachment 92933 [details] [review] Bug 14957: DO NOT PUSH Schema changes
Created attachment 92934 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library
Created attachment 92935 [details] [review] Bug 14957: Make preset labels translatable
Created attachment 92936 [details] [review] Bug 14957: Adjust paging option for new version of datatables
Created attachment 92937 [details] [review] Bug 14957: Fix rule javascript validation issue
Created attachment 92938 [details] [review] Bug 14957: Change YUI layout to to Boostrap
Rebased against current master and fixed some issues.
Created attachment 92999 [details] [review] Bug 14957: Remove prefetch of removed relationship
Thx for the rebase David, I have a little hope to tackle this at the Marseille Hackfest.
On step 6 : when I edit the rule, the "Preset" field comes back to the first value "Custom" : I think it's a disturbing and the value registred should be kept. On step 7 : there are gaps into the buttons, like if icons where missing, and they don't seem to have the same css (can send a screenshot if needed). Behaviours of both buttons are correct. On step 13 : once the rule is saved, the filter displays "intranet" but when creating the rule, it's "Staff client MARC editor". Maybe it should be uniformised ? Tested using a sandbox so can't run tests on step 15. Hope this feedback will be helpfull ! Except these points, the feature works fine and will be very useful !
Created attachment 93499 [details] Buttons difference
I think it's too late now for 19.11 (feature slush this Wednesday), but was so happy to see progress here and would love to see it early on in 20.05. David, can you provide follow-ups for the problems Severine noted? I think we are close to a sign-off.
Sorry about the late reply, I'm looking into it.
Created attachment 94909 [details] [review] Bug 14957: Add icons for confirmation dialog and set preset based on actions configuration
Created attachment 94911 [details] [review] Bug 14957: Replace filter value with label if one exists
@Séverine Queune I think most points should now be addressed. Thank you for your feedback!
*** Bug 11780 has been marked as a duplicate of this bug. ***
*** Bug 11519 has been marked as a duplicate of this bug. ***
*** Bug 13213 has been marked as a duplicate of this bug. ***
*** Bug 18210 has been marked as a duplicate of this bug. ***
Had a go at testing but get an error when adding a rule (step 5 of the test plan): DBIx::Class::Row::delete(): Not in database at /kohadevbox/koha/Koha/Object.pm line 219 at /usr/share/perl5/DBIx/Class/Exception.pm line 77 in DBIx::Class::Exception::throw at /usr/share/perl5/DBIx/Class/Exception.pm line 77 74: my $self = { msg => $msg }; 75: bless $self => $class; 76: 77: die $self; 78: } 79: 80: =head2 rethrow Show function arguments Can post the full error trace if required. Notes: The patch applied fine, I ran the database update, and enabled the new system preference.
Ok, thanks! A regression seems to been introduced in https://github.com/Koha-Community/Koha/commit/abb51797d8d5ebb274856e912e207a921ace478b causing this bug. Very difficult to debug since "delete" is somehow called without appearing in the code, and a stack trace points out a line (where delete is called) that does not exist. But I'm looking into it.
Created attachment 99998 [details] [review] Bug 14957: Remove usage of deleted Koha::MarcMergeRulesModules
I now found the source of the bug and created a new issue: Bug 24788. This problem is currently blocking this patch.
Created attachment 100052 [details] [review] Bug 14957: DO NOT PUSH Schema changes Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100053 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100054 [details] [review] Bug 14957: Make preset labels translatable Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100055 [details] [review] Bug 14957: Adjust paging option for new version of datatables Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100056 [details] [review] Bug 14957: Fix rule javascript validation issue Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100057 [details] [review] Bug 14957: Change YUI layout to to Boostrap Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100058 [details] [review] Bug 14957: Remove prefetch of removed relationship Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100059 [details] [review] Bug 14957: Add icons for confirmation dialog and set preset based on actions configuration Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100060 [details] [review] Bug 14957: Replace filter value with label if one exists Signed-off-by: David Nind <david@davidnind.com>
Created attachment 100061 [details] [review] Bug 14957: Remove usage of deleted Koha::MarcMergeRulesModules Signed-off-by: David Nind <david@davidnind.com>
(In reply to David Gustafsson from comment #193) > I now found the source of the bug and created a new issue: Bug 24788. This > problem is currently blocking this patch. Have signed off - everything seems to work as per the test plan the tests! One note for test plan: need to update the database (updatedatabase on koha-testing-docker)
Created attachment 101510 [details] Screenshot of option placement in OPAC I think the placement of the new option in the OPAC is not ideal, it appears in a very prominent spot, even atop of our warnings and other messages. I'd suggest to move it lower, even below the checkouts table maybe, but below the warnings.
Comment on attachment 101510 [details] Screenshot of option placement in OPAC Wrong bug...
At the moment this is blocked by bug 25539 and also needs a little rebase!
*** Bug 26372 has been marked as a duplicate of this bug. ***
I've unblocked this by working through and signing off the dependency bug. Setting to Patch doesn't apply to get attention for the rebase and hopefully we can get this one moving forward again.
Created attachment 109955 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com>
Created attachment 109956 [details] [review] Bug 14957: Make preset labels translatable Signed-off-by: David Nind <david@davidnind.com>
Created attachment 109957 [details] [review] Bug 14957: Adjust paging option for new version of datatables Signed-off-by: David Nind <david@davidnind.com>
Created attachment 109958 [details] [review] Bug 14957: Fix rule javascript validation issue Signed-off-by: David Nind <david@davidnind.com>
Created attachment 109959 [details] [review] Bug 14957: Change YUI layout to to Boostrap Signed-off-by: David Nind <david@davidnind.com>
Created attachment 109960 [details] [review] Bug 14957: Remove prefetch of removed relationship Signed-off-by: David Nind <david@davidnind.com>
Created attachment 109961 [details] [review] Bug 14957: Add icons for confirmation dialog and set preset based on actions configuration Signed-off-by: David Nind <david@davidnind.com>
Created attachment 109962 [details] [review] Bug 14957: Replace filter value with label if one exists Signed-off-by: David Nind <david@davidnind.com>
Created attachment 109963 [details] [review] Bug 14957: Remove usage of deleted Koha::MarcMergeRulesModules Signed-off-by: David Nind <david@davidnind.com>
A small conflict in installer/data/mysql/sysprefs.sql, should now be rebased against master.
Patch(es) no longer apply on latest master 8-( : root@kohadevbox:koha(bz14957)$ git bz apply 14957 Bug 14957 - Write protecting MARC fields based on source of import 100052 - Bug 14957: DO NOT PUSH Schema changes 109955 - Bug 14957: Merge rules system for merging of MARC records 109956 - Bug 14957: Make preset labels translatable 109957 - Bug 14957: Adjust paging option for new version of datatables 109958 - Bug 14957: Fix rule javascript validation issue 109959 - Bug 14957: Change YUI layout to to Boostrap 109960 - Bug 14957: Remove prefetch of removed relationship 109961 - Bug 14957: Add icons for confirmation dialog and set preset based on actions configuration 109962 - Bug 14957: Replace filter value with label if one exists 109963 - Bug 14957: Remove usage of deleted Koha::MarcMergeRulesModules Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 14957: DO NOT PUSH Schema changes Applying: Bug 14957: Merge rules system for merging of MARC records Using index info to reconstruct a base tree... M cataloguing/addbiblio.pl M installer/data/mysql/kohastructure.sql M installer/data/mysql/sysprefs.sql M koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc M koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref M koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt M misc/migration_tools/bulkmarcimport.pl M tools/batch_record_modification.pl Falling back to patching base and 3-way merge... Auto-merging tools/batch_record_modification.pl Auto-merging misc/migration_tools/bulkmarcimport.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc Auto-merging installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/kohastructure.sql Auto-merging cataloguing/addbiblio.pl error: Failed to merge in the changes. Patch failed at 0001 Bug 14957: Merge rules system for merging of MARC records 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-14957-Merge-rules-system-for-merging-of-MARC-r-KugWDA.patch
I've added a suggestion for this on the Koha 21.05 Road Map - please sign up if you are interested to finally get this one in :) https://annuel.framapad.org/p/koha_21.05_roadmap
Created attachment 116622 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com>
Created attachment 116623 [details] [review] Bug 14957: Updated schema files
Created attachment 116624 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com>
Created attachment 116625 [details] [review] Bug 14957: Updated schema files
I made this rule: Module: source Filter: * Tag: 500 Preset: Protect When I go to edit a record, there's nothing in the edit interface to suggest my edits won't be saved. I can type in a new 500, click to delete an existing 500, etc. When I save my edits, there's nothing to indicate that my changes were discarded or why. The edits simply aren't saved. This lack of feedback seems really confusing. Meanwhile, if I take a record that has a 500 and put it through batch record modification to remove the 500$a my rule does not prevent that edit as intended.
What's the current status of this one..?
I got an error at testing on step 4: Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. Error trace: Could not compile /kohadevbox/koha/admin/marc-merge-rules.pl: Global symbol "$bib_items_added" requires explicit package name (did you forget to declare "my $bib_items_added"?) at /kohadevbox/koha/C4/ImportBatch.pm line 676. Global symbol "$bib_items_replaced" requires explicit package name (did you forget to declare "my $bib_items_replaced"?) at /kohadevbox/koha/C4/ImportBatch.pm line 676. Global symbol "$bib_items_errored" requires explicit package name (did you forget to declare "my $bib_items_errored"?) at /kohadevbox/koha/C4/ImportBatch.pm line 676. Global symbol "$bib_items_added" requires explicit package name (did you forget to declare "my $bib_items_added"?) at /kohadevbox/koha/C4/ImportBatch.pm line 677. Global symbol "$bib_items_replaced" requires explicit package name (did you forget to declare "my $bib_items_replaced"?) at /kohadevbox/koha/C4/ImportBatch.pm line 678. Global symbol "$bib_items_errored" requires explicit package name (did you forget to declare "my $bib_items_errored"?) at /kohadevbox/koha/C4/ImportBatch.pm line 679. syntax error at /kohadevbox/koha/C4/ImportBatch.pm line 684, near "HEAD ModAuthority" Compilation failed in require at /kohadevbox/koha/admin/marc-merge-rules.pl line 33. BEGIN failed--compilation aborted at /kohadevbox/koha/admin/marc-merge-rules.pl line 33. at /usr/share/perl5/CGI/Compile.pm line 132
Thats probably because of a conflict in that file. I will see if can rebase and fix this.
Created attachment 117844 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com>
Created attachment 117845 [details] [review] Bug 14957: Updated schema files
Created attachment 117849 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Created attachment 117850 [details] [review] Bug 14957: Updated schema files Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de>
Created attachment 117853 [details] [review] Bug 14957: fix context for batchmod and batchimport
(In reply to Andrew Fuerste-Henry from comment #226) > I made this rule: > Module: source > Filter: * > Tag: 500 > Preset: Protect > > When I go to edit a record, there's nothing in the edit interface to suggest > my edits won't be saved. I can type in a new 500, click to delete an > existing 500, etc. When I save my edits, there's nothing to indicate that my > changes were discarded or why. The edits simply aren't saved. > > This lack of feedback seems really confusing. > > Meanwhile, if I take a record that has a 500 and put it through batch record > modification to remove the 500$a my rule does not prevent that edit as > intended. Had a look at this and ModBiblio context argument for batch-modification was incorrect, an error introduced in one or more of the rebases of this patch. Fixed this and was going to test, but looks like ElasticSearch is broken in latest master, but will try to get around that. Regarding the lack of user notification I would regard that as a feature request rather than a bug. It's not obvious without being acquainted with the code, but it would actually be quite a lot of work to implement, and since this is not a feature that is prioritize by our library I don't have the resources to do this.
Created attachment 117859 [details] [review] Bug 14957: fix context for batchmod and batchimport
Have now verified that batchmod rules should works again.
(In reply to Andrew Fuerste-Henry from comment #226) > > This lack of feedback seems really confusing. > I agree feedback is important.. however perhaps we can do that in a followup bug to keep this one moving.
Created attachment 117964 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> https://bugs.koha-community.org/show_bug.cgi?id=24957
Created attachment 117965 [details] [review] Bug 14957: Updated schema files Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> https://bugs.koha-community.org/show_bug.cgi?id=24957
Created attachment 117966 [details] [review] Bug 14957: fix context for batchmod and batchimport Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> https://bugs.koha-community.org/show_bug.cgi?id=24957
Martin and I are working heavily on this one for QA. Marking as assigned just in case, to avoid overlapping with others.
Created attachment 118918 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> https://bugs.koha-community.org/show_bug.cgi?id=24957 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118919 [details] [review] Bug 14957: Updated schema files Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> https://bugs.koha-community.org/show_bug.cgi?id=24957 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118920 [details] [review] Bug 14957: fix context for batchmod and batchimport Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> https://bugs.koha-community.org/show_bug.cgi?id=24957 Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118921 [details] [review] Bug 14957: (QA follow-up) Button styling This patch updates the action buttons to include btn-default and switched from btn-sm to btn-xs to match other similar UI's found in datatable based admin pages. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118922 [details] [review] Bug 14957: (QA follow-up) Add tooltips This patch adds an info-tooltip to the field rules in the table header to expand on their meaning to reduce the need to read the documentation each time you come to use the configuration tool Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118923 [details] [review] Bug 14957: (QA follow-up) Rename 'merge' => 'overlay' This patch acknowledges the fact that 'merging' has a different meaning in Koha than the behavior this great patchset introduces. The more idiomatic way of describing the behavior is to talk about 'overlay rules'. This patch also: - Fixes kohastructure.sql missing table encoding a collation - Fixes the atomic update completely missing a table - Moves the syspref entry to 'Importing', probably a rebase issue Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118924 [details] [review] Bug 14957: Feature hidden in menues if no other 'cagaloguing' permissions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118925 [details] [review] Bug 14957: DBIC changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118926 [details] [review] Bug 14957: (follow-up) marc_merge_rules_modules removed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118927 [details] [review] Bug 14957: (QA follow-up) Clarify 'context' param This patch renames the (passed through) 'context' param for 'overlay_context'. I propose doing so, because in Koha-land 'context' has a special meaning, related to C4::Context and it reads ambigous. The patch itself is pretty trivial. Tests should pass: 1. Run: $ kshell k$ prove t/db_dependent/Biblio/MarcOverlayRules.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This patchset has been hanging for too long, and my follow-up patches are my attempt to make it pushable while making it adaptable for future refactorings without major changes. Missing for my PQA stamp: - Translatability in the marc-overlay-rules.tt
Ok, I submitted what we have so far so others can add here. The patchset introduces several 'features' in my opinion, and we could do better with the DB and classes. In my opinion we would need: 1. A table for rules (done-ish) 2. A table for defining named rule sets 3. A table for assigning 'globally enforced' rule sets to specific places, categories, etc (this is what the patchset does, only, in my opinion). It is a pity we cannot choose rulesets explicitly when importing, but I know we can do it later.
Created attachment 118934 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118935 [details] [review] Bug 14957: Updated schema files Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118936 [details] [review] Bug 14957: fix context for batchmod and batchimport Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 118937 [details] [review] Bug 14957: (QA follow-up) Button styling This patch updates the action buttons to include btn-default and switched from btn-sm to btn-xs to match other similar UI's found in datatable based admin pages. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118938 [details] [review] Bug 14957: (QA follow-up) Add tooltips This patch adds an info-tooltip to the field rules in the table header to expand on their meaning to reduce the need to read the documentation each time you come to use the configuration tool Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118939 [details] [review] Bug 14957: (QA follow-up) Rename 'merge' => 'overlay' This patch acknowledges the fact that 'merging' has a different meaning in Koha than the behavior this great patchset introduces. The more idiomatic way of describing the behavior is to talk about 'overlay rules'. This patch also: - Fixes kohastructure.sql missing table encoding a collation - Fixes the atomic update completely missing a table - Moves the syspref entry to 'Importing', probably a rebase issue Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118940 [details] [review] Bug 14957: Feature hidden in menues if no other 'cagaloguing' permissions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118941 [details] [review] Bug 14957: DBIC changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118942 [details] [review] Bug 14957: (follow-up) marc_merge_rules_modules removed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118943 [details] [review] Bug 14957: (QA follow-up) Clarify 'context' param This patch renames the (passed through) 'context' param for 'overlay_context'. I propose doing so, because in Koha-land 'context' has a special meaning, related to C4::Context and it reads ambigous. The patch itself is pretty trivial. Tests should pass: 1. Run: $ kshell k$ prove t/db_dependent/Biblio/MarcOverlayRules.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118944 [details] [review] Bug 14957: (QA follow-up) Fix syntax error in atomicupdate Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118945 [details] [review] Bug 14957: (QA follow-up) Fix translation issues This patch fixes all the translation issues I could spot in the marc-overlay-rules template. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 118946 [details] [review] Bug 14957: (QA follow-up) Rename and move admin link This patch renames 'MARC overlay rules' to 'Record overlay rules' and moves it up the list to just below 'Record matching rules' to make it more contextually ordered.
Created attachment 118957 [details] [review] Bug 14957: (QA follow-up) Comment 'bulkmarcimport' temporarily It appears this patchset doesn't fully support bulkmarcimport (see bug 25539 for details). At this time, to get this bug moving I think we should comment out the ability to define rules that affect bulkmarcimport and then we can fix and re-enable that feature at a later date. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
The QA script raises several errors, please double-check.
If yo can, please have a look here - we are getting close to the critical timeframe before release and it would be so great to have this included.
Created attachment 120533 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 120534 [details] [review] Bug 14957: Updated schema files Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 120535 [details] [review] Bug 14957: fix context for batchmod and batchimport Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 120536 [details] [review] Bug 14957: (QA follow-up) Button styling This patch updates the action buttons to include btn-default and switched from btn-sm to btn-xs to match other similar UI's found in datatable based admin pages. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120537 [details] [review] Bug 14957: (QA follow-up) Add tooltips This patch adds an info-tooltip to the field rules in the table header to expand on their meaning to reduce the need to read the documentation each time you come to use the configuration tool Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120538 [details] [review] Bug 14957: (QA follow-up) Rename 'merge' => 'overlay' This patch acknowledges the fact that 'merging' has a different meaning in Koha than the behavior this great patchset introduces. The more idiomatic way of describing the behavior is to talk about 'overlay rules'. This patch also: - Fixes kohastructure.sql missing table encoding a collation - Fixes the atomic update completely missing a table - Moves the syspref entry to 'Importing', probably a rebase issue Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120539 [details] [review] Bug 14957: Feature hidden in menues if no other 'cagaloguing' permissions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120540 [details] [review] Bug 14957: DBIC changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120541 [details] [review] Bug 14957: (follow-up) marc_merge_rules_modules removed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120542 [details] [review] Bug 14957: (QA follow-up) Clarify 'context' param This patch renames the (passed through) 'context' param for 'overlay_context'. I propose doing so, because in Koha-land 'context' has a special meaning, related to C4::Context and it reads ambigous. The patch itself is pretty trivial. Tests should pass: 1. Run: $ kshell k$ prove t/db_dependent/Biblio/MarcOverlayRules.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120543 [details] [review] Bug 14957: (QA follow-up) Fix syntax error in atomicupdate Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120544 [details] [review] Bug 14957: (QA follow-up) Fix translation issues This patch fixes all the translation issues I could spot in the marc-overlay-rules template. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120545 [details] [review] Bug 14957: (QA follow-up) Rename and move admin link This patch renames 'MARC overlay rules' to 'Record overlay rules' and moves it up the list to just below 'Record matching rules' to make it more contextually ordered.
Created attachment 120546 [details] [review] Bug 14957: (QA follow-up) Comment 'bulkmarcimport' temporarily It appears this patchset doesn't fully support bulkmarcimport (see bug 25539 for details). At this time, to get this bug moving I think we should comment out the ability to define rules that affect bulkmarcimport and then we can fix and re-enable that feature at a later date. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120547 [details] [review] Bug 14957: (QA follow-up) Add missing filters
Created attachment 120548 [details] [review] Bug 14957: (QA follow-up) Update for bug 26703
Created attachment 120550 [details] [review] Bug 14957: (QA follow-up) Update for bug 20054
Created attachment 120553 [details] [review] Bug 14957: (QA follow-up) Update for bug 20053
Created attachment 120554 [details] [review] Bug 14957: (QA follow-up) Update for bug 17858
I fixed the QA issues with follow-ups.. the only remaining ones flagged up are git manipulation errors (and some executable bit errors that are actually resolved by the manipulations anyway).. I did not squash commits as I didn't want to lose author attribution.. there's been a few people working on this one over the duration of its development life. I feel this is ready for push myself.. yes it has a few sharp edges and I'd love to see some of the templates indented more nicely for example.. if you agree I could do that indent fix at the least as a follow-up here. Hoping this could be one of the last ones for this release.
Created attachment 120555 [details] [review] Bug 14957: (QA follow-up) Update for bug 27846
Created attachment 120556 [details] [review] Bug 14957: (QA follow-up) Fix template indentation
Created attachment 120557 [details] [review] Bug 14957: (QA follow-up) Fix template indentation
Seeing the last status change - what happened?
Created attachment 120581 [details] [review] Bug 14957: (QA follow-up) Update for bug 23271
Created attachment 120582 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 120583 [details] [review] Bug 14957: Updated schema files Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 120584 [details] [review] Bug 14957: fix context for batchmod and batchimport Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 120585 [details] [review] Bug 14957: (QA follow-up) Button styling This patch updates the action buttons to include btn-default and switched from btn-sm to btn-xs to match other similar UI's found in datatable based admin pages. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120586 [details] [review] Bug 14957: (QA follow-up) Add tooltips This patch adds an info-tooltip to the field rules in the table header to expand on their meaning to reduce the need to read the documentation each time you come to use the configuration tool Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120587 [details] [review] Bug 14957: (QA follow-up) Rename 'merge' => 'overlay' This patch acknowledges the fact that 'merging' has a different meaning in Koha than the behavior this great patchset introduces. The more idiomatic way of describing the behavior is to talk about 'overlay rules'. This patch also: - Fixes kohastructure.sql missing table encoding a collation - Fixes the atomic update completely missing a table - Moves the syspref entry to 'Importing', probably a rebase issue Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120588 [details] [review] Bug 14957: Feature hidden in menues if no other 'cagaloguing' permissions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120589 [details] [review] Bug 14957: DBIC changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120590 [details] [review] Bug 14957: (follow-up) marc_merge_rules_modules removed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120591 [details] [review] Bug 14957: (QA follow-up) Clarify 'context' param This patch renames the (passed through) 'context' param for 'overlay_context'. I propose doing so, because in Koha-land 'context' has a special meaning, related to C4::Context and it reads ambigous. The patch itself is pretty trivial. Tests should pass: 1. Run: $ kshell k$ prove t/db_dependent/Biblio/MarcOverlayRules.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120592 [details] [review] Bug 14957: (QA follow-up) Fix syntax error in atomicupdate Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120593 [details] [review] Bug 14957: (QA follow-up) Fix translation issues This patch fixes all the translation issues I could spot in the marc-overlay-rules template. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120594 [details] [review] Bug 14957: (QA follow-up) Rename and move admin link This patch renames 'MARC overlay rules' to 'Record overlay rules' and moves it up the list to just below 'Record matching rules' to make it more contextually ordered.
Created attachment 120595 [details] [review] Bug 14957: (QA follow-up) Comment 'bulkmarcimport' temporarily It appears this patchset doesn't fully support bulkmarcimport (see bug 25539 for details). At this time, to get this bug moving I think we should comment out the ability to define rules that affect bulkmarcimport and then we can fix and re-enable that feature at a later date. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 120596 [details] [review] Bug 14957: (QA follow-up) Add missing filters
Created attachment 120597 [details] [review] Bug 14957: (QA follow-up) Update for bug 26703
Created attachment 120598 [details] [review] Bug 14957: (QA follow-up) Update for bug 20054
Created attachment 120599 [details] [review] Bug 14957: (QA follow-up) Update for bug 20053
Created attachment 120600 [details] [review] Bug 14957: (QA follow-up) Update for bug 17858
Created attachment 120601 [details] [review] Bug 14957: (QA follow-up) Update for bug 27846
Created attachment 120602 [details] [review] Bug 14957: (QA follow-up) Fix template indentation
Created attachment 120603 [details] [review] Bug 14957: (QA follow-up) Update for bug 23271
Created attachment 120604 [details] [review] Bug 14957: (QA follow-up) Fix script name
Created attachment 120605 [details] [review] Bug 14957: (QA follow-up) Fix for jQuery undefined
(In reply to Katrin Fischer from comment #294) > Seeing the last status change - what happened? I broke it with the moving the JS to the bottom of the page.. so whilst I was working to fix that I reset the status to FQA to stop anyone wasting time testing. I've now fixed it again.. and run through the whole test plan to check I didn't break anything else.
Running `dbic` generates a change to Koha/Schema/Result/MarcOverlayRule.pm. It removes a FK to Koha::Schema::Result::MarcOverlayRulesModule. However this table does not exist. Should I drop the schema changes and regenerate it properly or here is something else missing?
Created attachment 120670 [details] [review] Bug 14957: Clean controller Too many imports and unecessary complicated init of $rules There are certainly other things to improve, like the "op" handling, standardize names of "op", etc.
Created attachment 120671 [details] [review] Bug 14957: Clean controller Too many imports and unecessary complicated init of $rules There are certainly other things to improve, like the "op" handling, standardize names of "op", etc.
1. See comment 321 2. We don't hide the admin page if the pref is off, is that expected? 3. Should we link the pref to the admin page? 4. We need a wiki page, or at least more info/examples for the release notes. There is only 2 very succinct example in the test plan but the feature allows much more. 5. Can you explain me what this rule means? | id | tag | module | filter | add | append | remove | delete | | 1 | 12 | source | batchmod | 1 | 0 | 0 | 0 | 6. How does it interface with the marc modification templates when batch editing bibliographic records?
7. (In reply to Andrew Fuerste-Henry from comment #226) > I made this rule: > Module: source > Filter: * > Tag: 500 > Preset: Protect > > When I go to edit a record, there's nothing in the edit interface to suggest > my edits won't be saved. I can type in a new 500, click to delete an > existing 500, etc. When I save my edits, there's nothing to indicate that my > changes were discarded or why. The edits simply aren't saved. > > This lack of feedback seems really confusing. Still valid. However a new record can be created with a 500, is that expected?
(In reply to Jonathan Druart from comment #321) > Running `dbic` generates a change to Koha/Schema/Result/MarcOverlayRule.pm. > It removes a FK to Koha::Schema::Result::MarcOverlayRulesModule. > However this table does not exist. Should I drop the schema changes and > regenerate it properly or here is something else missing? I think drop the dbic patch here and use the regenerated one.. I think the Module bit got removed during the history of this bug but the schema didn't get regenerated.
(In reply to Jonathan Druart from comment #324) > 1. See comment 321 Replied above > 2. We don't hide the admin page if the pref is off, is that expected? Whilst I agree with what you're saying here, I think if memory serves this was discussed earlier in the bug and the authors wanted the ability to create their rules and then enable them.. so they wanted to display the page regardless or whether the pref was on or off (the page does warn you at the top if the pref is off).. having said that.. I'd be happy to follow-up to change that.. or, in my opinion in a later bug we could drop the pref and instead have an 'enabled' flag at the rule level. > 3. Should we link the pref to the admin page? Not sure what you're saying here? > 4. We need a wiki page, or at least more info/examples for the release notes. > There is only 2 very succinct example in the test plan but the feature > allows much more. I agree, it took me a fair while to wrap my head around the rules.. I think a wiki page would be great and would be happy to start one. > 5. Can you explain me what this rule means? > > | id | tag | module | filter | add | append | remove | delete | > | 1 | 12 | source | batchmod | 1 | 0 | 0 | 0 | With 'batchmod', only add tag 12 is if exists only in the incoming record. > 6. How does it interface with the marc modification templates when batch > editing bibliographic records? Investigating.. I have a feeling his patchset predates it and I missed this case ! :(
OK, I did test this actually.. The modification templates are applied prior to save.. so the changes are then blocked by the rules here. The main thing I don't like about this bug is exactly what Andrew is highlighting.. at the moment it's all rather silent. It would be great as a followup early next cycle to enhance each of the area's it touched with feedback either during save or prior to it to let the end user know that their changes will not be applied due to the rules in place here. I felt that the base bug was already large and complicated and shouldn't be blocked any long as a bug in itself.. I'm really hoping that follow-up bugs will be acted on in a timely fashion.. even if it means me lending a hand.
(In reply to Martin Renvoize from comment #327) > (In reply to Jonathan Druart from comment #324) > > 2. We don't hide the admin page if the pref is off, is that expected? > so they wanted to display the page regardless or whether the pref was on or off Make sense > (the page does warn you at the top if the pref is off) That's wrong, there is no warning. > > 3. Should we link the pref to the admin page? > > Not sure what you're saying here? Have a link to the admin page, next to the pref.
(In reply to Martin Renvoize from comment #327) > (In reply to Jonathan Druart from comment #324) > > 5. Can you explain me what this rule means? > > > > | id | tag | module | filter | add | append | remove | delete | > > | 1 | 12 | source | batchmod | 1 | 0 | 0 | 0 | > > With 'batchmod', only add tag 12 is if exists only in the incoming record. I still don't understand, but I now see the manual stuff, will read it (haven't seen it before, sorry!) Note for myself: One another thing to check, how is working the "preview" for the batchmod.
I know librarians can have weird needs but, should we really offer the "custom" preset? I am reading the tests right now and some combinations are doing funky stuffs. Maybe we could simplify the UI if we stick to the only 1 "action" column with the predefined ones? (not considering this question a blocker, but I would be happy to get an answer).
Created attachment 120705 [details] [review] Bug 14957: Improve readbility of tests Not saying that they were not readable before, but I had to do some changes to compare the before/after records and it was easier that way. Feel free to obsolete if you disagree.
(In reply to Jonathan Druart from comment #331) > I know librarians can have weird needs but, should we really offer the > "custom" preset? I am reading the tests right now and some combinations are > doing funky stuffs. Maybe we could simplify the UI if we stick to the only 1 > "action" column with the predefined ones? (not considering this question a > blocker, but I would be happy to get an answer). See the "warning"s and "fixme"s from the last patch please.
I am sorry, I am not feeling confident enough with this one for 21.05. I would like to get answer to my questions, and improve the user experience. Keep the work going, it's almost ready! And we push it early in the next cycle.
Nobody interested in getting this done?
(In reply to Jonathan Druart from comment #335) > Nobody interested in getting this done? Martin and I are, but more hands would be useful.
Still interested but just haven't been able to commit any more time to it. Honestly it's a complicated enough patch that I get lost in it for days every time I start to look at it.
Regarding the level of confidence in this patch regarding stability we have been running this for a pretty large University Library (Gothenburg University Library) in production for a couple of years, about three I think, and not encountered any major issues. We have not merged in the latest upstream changes from this issue, but I don't think much have changed when it comes to the core logic/functionality.
There are several questions waiting for answers, see comments 321-333.
Created attachment 125514 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125515 [details] [review] Bug 14957: Updated schema files Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125516 [details] [review] Bug 14957: fix context for batchmod and batchimport Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 125517 [details] [review] Bug 14957: (QA follow-up) Button styling This patch updates the action buttons to include btn-default and switched from btn-sm to btn-xs to match other similar UI's found in datatable based admin pages. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125518 [details] [review] Bug 14957: (QA follow-up) Add tooltips This patch adds an info-tooltip to the field rules in the table header to expand on their meaning to reduce the need to read the documentation each time you come to use the configuration tool Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125519 [details] [review] Bug 14957: (QA follow-up) Rename 'merge' => 'overlay' This patch acknowledges the fact that 'merging' has a different meaning in Koha than the behavior this great patchset introduces. The more idiomatic way of describing the behavior is to talk about 'overlay rules'. This patch also: - Fixes kohastructure.sql missing table encoding a collation - Fixes the atomic update completely missing a table - Moves the syspref entry to 'Importing', probably a rebase issue Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125520 [details] [review] Bug 14957: Feature hidden in menues if no other 'cagaloguing' permissions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125521 [details] [review] Bug 14957: DBIC changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125522 [details] [review] Bug 14957: (follow-up) marc_merge_rules_modules removed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125523 [details] [review] Bug 14957: (QA follow-up) Clarify 'context' param This patch renames the (passed through) 'context' param for 'overlay_context'. I propose doing so, because in Koha-land 'context' has a special meaning, related to C4::Context and it reads ambigous. The patch itself is pretty trivial. Tests should pass: 1. Run: $ kshell k$ prove t/db_dependent/Biblio/MarcOverlayRules.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125524 [details] [review] Bug 14957: (QA follow-up) Fix syntax error in atomicupdate Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125525 [details] [review] Bug 14957: (QA follow-up) Fix translation issues This patch fixes all the translation issues I could spot in the marc-overlay-rules template. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125526 [details] [review] Bug 14957: (QA follow-up) Rename and move admin link This patch renames 'MARC overlay rules' to 'Record overlay rules' and moves it up the list to just below 'Record matching rules' to make it more contextually ordered.
Created attachment 125527 [details] [review] Bug 14957: (QA follow-up) Comment 'bulkmarcimport' temporarily It appears this patchset doesn't fully support bulkmarcimport (see bug 25539 for details). At this time, to get this bug moving I think we should comment out the ability to define rules that affect bulkmarcimport and then we can fix and re-enable that feature at a later date. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 125528 [details] [review] Bug 14957: (QA follow-up) Add missing filters
Created attachment 125529 [details] [review] Bug 14957: (QA follow-up) Update for bug 26703
Created attachment 125530 [details] [review] Bug 14957: (QA follow-up) Update for bug 20054
Created attachment 125531 [details] [review] Bug 14957: (QA follow-up) Update for bug 20053
Created attachment 125532 [details] [review] Bug 14957: (QA follow-up) Update for bug 17858
Created attachment 125533 [details] [review] Bug 14957: (QA follow-up) Update for bug 27846
Created attachment 125534 [details] [review] Bug 14957: (QA follow-up) Fix template indentation
Created attachment 125535 [details] [review] Bug 14957: (QA follow-up) Update for bug 23271
Created attachment 125536 [details] [review] Bug 14957: (QA follow-up) Fix script name
Created attachment 125537 [details] [review] Bug 14957: (QA follow-up) Fix for jQuery undefined
Created attachment 125538 [details] [review] Bug 14957: Clean controller Too many imports and unecessary complicated init of $rules There are certainly other things to improve, like the "op" handling, standardize names of "op", etc.
Created attachment 125539 [details] [review] Bug 14957: Improve readbility of tests Not saying that they were not readable before, but I had to do some changes to compare the before/after records and it was easier that way. Feel free to obsolete if you disagree.
Created attachment 125540 [details] [review] Bug 14957: Fix imports in tests
Another rebase.. another follow-up to fix things that have moved on. I'm not sure what more I can do... 6 years is too long for an important feature like this to languish.
(In reply to Jonathan Druart from comment #339) > There are several questions waiting for answers, see comments 321-333. I read through those comments again, and the questions where not addressing stability issues but feature requests and refactoring suggestions. Probably one of the reasons this has taken so long is that instead of concentrating on minimal viable functionality there are continuous suggestions for new features and improvements. Some of which are actually pretty complicated to implement (like user feedback). As I said we have been running this in production for years, and it more or less covers our needs (as pretty decently sized university library with 200+ employees. I did forget about one thing though, that actually is a show-stopper, and that is if this bug is merged without the fix in bug 25539, this will trigger a bug with this current patch. I discovered that bug 25539 was set as depending on this bug when it should have been the other way around which I now fixed. The reason everything works for us is that we use the fix in bug 25539. We also have large and unruly patch for bulkmarcimport.pl (a which I believe is now deactivated in the current iteration of this patch) so that also source works. Can't remember at the top of my head what the issues in bulkmarcimport.pl was that caused us to disable it, but the fix in bug 25539 will not solve this issue I think (but is still needed to avoid the bug causing rules to be ignored on updates. So I think focusing on sorting out the issue addressed in bug 25539, and possibly address the problems with bulkmarcimport.pl would be reasonable thing to prioritize.
I've spent a lost of time on this patch set to make it part of 21.05. Added follow-ups and written test. Then I had some questions, and didn't get the answers I needed to write the missing bit. I was not asking anybody else to write anything. I wish I had people asking me to write follow-up for my patches..
> I did forget about one thing though, that actually is a show-stopper, and > that is if this bug is merged without the fix in bug 25539, this will > trigger a bug with this current patch. I discovered that bug 25539 was set > as depending on this bug when it should have been the other way around which > I now fixed. Pretty sure we reversed the dependencies deliberately to unblock this.. we disable this feature for use with bulkmarcimport in one of the patches.
(In reply to Martin Renvoize from comment #370) > > I did forget about one thing though, that actually is a show-stopper, and > > that is if this bug is merged without the fix in bug 25539, this will > > trigger a bug with this current patch. I discovered that bug 25539 was set > > as depending on this bug when it should have been the other way around which > > I now fixed. > > Pretty sure we reversed the dependencies deliberately to unblock this.. we > disable this feature for use with bulkmarcimport in one of the patches. Yes, now I remember. I mistakenly misremembered the bug in bug 25539 appearing when saving using the staff interface. In that case there should be no blockers for this patch (with bulkmarcimport.pl disabled, which would be a show-stopper for us, but we would in case this got merged address with a local patch).
(In reply to Jonathan Druart from comment #369) > I've spent a lost of time on this patch set to make it part of 21.05. Added > follow-ups and written test. > > Then I had some questions, and didn't get the answers I needed to write the > missing bit. I was not asking anybody else to write anything. > > I wish I had people asking me to write follow-up for my patches.. Sorry I missed some of those questions (i for some reason read comment 221-333 so I was not referring to you regarding requesting new features). Regarding the question in comment 324: > 4. We need a wiki page, or at least more info/examples for the release notes. There is only 2 very succinct example in the test plan but the feature allows much more. I have written a manual entry, it might now be partly out dated since it was 2 years ago, there are some links to it further up in the thread: https://gitlab.com/gnucifer/koha-manual/-/commit/eb07dd1abcb357de27d294cee49c287c6f5bfe6c The reason I pointed out that we have been running this in production for a long time is that there appears to be great hesitancy in merging this which is understandable due to the large surface area of changes and and relative high complexity. That serious bugs could lurk is not impossible, but is highly unlikely as we would probably have encountered them by now. There are for example certain contexts that we do not use, like batchmod, z3950 and import_lexile, but it can be good to know that the core functionality appears to be pretty solid. The scariest part of this by far is the potential for unexpected data loss or corruption when records are taken apart and put back together again, and that part appears to be solid. As a side note I don't really get why MergeRules were renamed to OverlayRules, it's not that much worse than merge, but is it really better? To me merge is more intuitive as we basically have to revisions of a record and use certain rules to resolve the diff, merging the two.
Created attachment 125632 [details] [review] Bug 14957: Fix regression, rules must be refetched on add/edit/deletion
Created attachment 125633 [details] [review] Bug 14957: Fix imports in marc-overlay-rules.pl
Fixed a regression introduced in commit, "Bug 14957: Clean controller". There was reason for that kludgy code even though it could be somewhat simplified. Also some imports that needed to be fixed after some subs no longer exported by default.
Appears to be some failing tests. I think I know why as encountered the same issue with another patch. Part of the leader field is now modified in Koha in ModBiblio, which was not the case before, so records appears to have changed when the data has not as the whole record is now checked, when only specific fields where before. Will have a look at this at monday.
Created attachment 125636 [details] [review] Bug 14957: Exclude leader from saved/updated record comparison in tests
Realized was very easy to fix, now tests should be working.
Thanks David for your follow-ups. Martin and I are ready to help you here. We have enough time before the 21.11 freeze to make this new feature part of the next release :) Can you reply to the other open questions please? See below. I am volunteering to write the patches needed. (In reply to Martin Renvoize from comment #327) > (In reply to Jonathan Druart from comment #324) > > 2. We don't hide the admin page if the pref is off, is that expected? > > Whilst I agree with what you're saying here, I think if memory serves this > was discussed earlier in the bug and the authors wanted the ability to > create their rules and then enable them.. so they wanted to display the page > regardless or whether the pref was on or off (the page does warn you at the > top if the pref is off).. having said that.. I'd be happy to follow-up to > change that.. or, in my opinion in a later bug we could drop the pref and > instead have an 'enabled' flag at the rule level. It seems weird to have the pref off but the link to the admin page visible. > > 3. Should we link the pref to the admin page? > > Not sure what you're saying here? I meant, should we have a link to the admin page of the feature in the syspref's description. I can do it. > > 6. How does it interface with the marc modification templates when batch > > editing bibliographic records? > > Investigating.. I have a feeling his patchset predates it and I missed this > case ! :( Need to be investigated. (In reply to Jonathan Druart from comment #331) > I know librarians can have weird needs but, should we really offer the > "custom" preset? I am reading the tests right now and some combinations are > doing funky stuffs. Maybe we could simplify the UI if we stick to the only 1 > "action" column with the predefined ones? (not considering this question a > blocker, but I would be happy to get an answer). Still waiting for an answer. See also the "warning"s and "fixme"s from the patch "Improve readbility of tests".
(And I think it's probably much better to compare the whole record in the tests, you don't get as detailed feedback on failure, but on the other hand catches if there are unexpected modifications of other parts of the record which is more important).
Hi again! > It seems weird to have the pref off but the link to the admin page visible. I have not made any active decision regarding this, it's been like this since before I got involved and have never given it much thought. Don't have any strong opinions either way. Elasticsearch settings seems to work the same way, that is if SearchEngine is Zebra, you still have a "Search engine configuration (Elasticsearch)" link in Administration with a "Elasticsearch is currently disabled" notice at the top. So personally I would probably keep it as it is just to avoid the extra work (even though should be pretty easy to change). > > 6. How does it interface with the marc modification templates when batch > > editing bibliographic records? > > Investigating.. I have a feeling his patchset predates it and I missed this > case ! :( I don't have time to look at this today but can do on Monday. > I know librarians can have weird needs but, should we really offer the > "custom" preset? I am reading the tests right now and some combinations are > doing funky stuffs. Maybe we could simplify the UI if we stick to the only 1 > "action" column with the predefined ones? (not considering this question a > blocker, but I would be happy to get an answer). I see no reason to remove this, I just don't see the harm of having the option if there is some weird scenario where it could be useful. I don't see the "custom" option as complicating the UI, it's just an extra choice in a dropdown.
And I think there are pedagogical reasons to keep the columns with the action choices and being able to see which ones are selected for each preset. Even though I understand it will be challenging for must librarians to understand the current rules concept, if we where to remove the separate actions (which now also have pretty good explanations), I think it would be much harder for people to understand exactly how rules are applied, and a higher risk for assuming work in some different way.
> > 6. How does it interface with the marc modification templates when batch > > editing bibliographic records? > > Investigating.. I have a feeling his patchset predates it and I missed this > case ! :( Sorry, never got around to have a look at this yesterday, but now did. From what I can gather from the code I'm pretty sure that if no rules have been defined for the batchmod context, the incoming record simply overwrites the existing record. If there are rules, these are applied on the record after the record has been modified using the specified template. So if the modification template modifies some protected field for example, these changes will not be applied. This is expected behavior as far as I can tell?
(In reply to David Gustafsson from comment #383) > > > 6. How does it interface with the marc modification templates when batch > > > editing bibliographic records? > > > > Investigating.. I have a feeling his patchset predates it and I missed this > > case ! :( > > Sorry, never got around to have a look at this yesterday, but now did. From > what I can gather from the code I'm pretty sure that if no rules have been > defined for the batchmod context, the incoming record simply overwrites the > existing record. If there are rules, these are applied on the record after > the record has been modified using the specified template. So if the > modification template modifies some protected field for example, these > changes will not be applied. This is expected behavior as far as I can tell? Tested right now: - Protect 500 - Edit bibliographic record, add new 500 field => Change not saved (OK) - Use the batch record modification tool (previously created a MARC modification template that add a 500$a => The preview show the 500$a (KO) - Save => The new field is created (KO)
Created attachment 126424 [details] [review] Bug 14957: (follow-up) Clarify 'context' param
Created attachment 126425 [details] [review] Bug 14957: Fix record preview
Created attachment 126431 [details] [review] Bug 14957: Fix syspref's values It didn't switch off when set to "don't use"
Regression found, the order of the subfields (introduced by bug 7882) is not kept. To recreate: Turn on MARCOverlayRules Edit a record Change the order of subfields Save Edit again => The order hasn't been saved.
(In reply to Jonathan Druart from comment #388) > Regression found, the order of the subfields (introduced by bug 7882) is not > kept. > > To recreate: > Turn on MARCOverlayRules > Edit a record > Change the order of subfields > Save > Edit again > => The order hasn't been saved. diff --git a/Koha/MarcOverlayRules.pm b/Koha/MarcOverlayRules.pm index 11b33d1928b..70500d00d2f 100644 --- a/Koha/MarcOverlayRules.pm +++ b/Koha/MarcOverlayRules.pm @@ -149,7 +149,7 @@ sub merge_records { my $hash_field_data = sub { my ($field) = @_; my $indicators = join("\x1E", map { $field->indicator($_) } (1, 2)); - return $indicators . "\x1E" . join("\x1E", sort map { join "\x1E", @{$_} } $field->subfields()); + return $indicators . "\x1E" . join("\x1E", map { join "\x1E", @{$_} } $field->subfields()); }; my $diff_by_key = sub { I think it's because of this 'sort', but there are 2 other sort calls in this method, not sure how/why they are useful.
> I think it's because of this 'sort', but there are 2 other sort calls in > this method, not sure how/why they are useful. There might be some other issue that causes this issue, but it's not those sorts if I'm not completely misguided. That's a hash function for comparing field data, and the data sorted will not be written back to the record.
(In reply to David Gustafsson from comment #390) > > I think it's because of this 'sort', but there are 2 other sort calls in > > this method, not sure how/why they are useful. > > There might be some other issue that causes this issue, but it's not those > sorts if I'm not completely misguided. That's a hash function for comparing > field data, and the data sorted will not be written back to the record. Just realized it might actually be the reason, but in a round-about way. If just changing the order of fields without modifying the data, they will still be considered identical so the old value will be kept. This is most likely the cause. If we remove the sort we risk writing an extra entry if for example the order has been changed for a record in Koha, and an external record with the same data in a different order is imported, as it will be considered a new field entry. Will think about how to handle this, hopefully without not complicating the merge algorithm too much.
Now that looked at this some more, the subfield ordering is one thing, and field ordering another, and has to be handled differently. I think I know how to address the subfield order issue. Will see if can find a way to handle field order correctly.
Created attachment 126592 [details] [review] Bug 14957: Fix field order issue
The fix for subfield ordering actually resolved them both. The change might be a little bit hard to understand so please let me know if have any questions.
One could also argue that should be able to change order also for non-overwritable field, or to introduce an extra "change order" action.
We mostly use "In discussion" only if there is an argument between parties - I don't see that happen here yet, so will switch back to 'needs sign-off' so that this will show for the testers.
Created attachment 126666 [details] [review] Bug 14957: Add tests for subfield order
Created attachment 126667 [details] [review] Bug 14957: Tell the cataloguer that a change has been rejected
(In reply to Jonathan Druart from comment #398) > Created attachment 126667 [details] [review] [review] > Bug 14957: Tell the cataloguer that a change has been rejected This patch is pretty ugly but I wanted to know the shortest route to display feedback to the cataloger. That must be tidied to be ready. Obsoleting and letting who may need it moving it to its own bug report.
Comment on attachment 126667 [details] [review] Bug 14957: Tell the cataloguer that a change has been rejected This patch needs to be improved and moved to its own bug report.
Created attachment 126752 [details] [review] Bug 14957: Merge rules system for merging of MARC records Add a rule based system for merging MARC records to for example prevent field data from being overwritten. To test: 1. Apply this patch. 2. Log in to staff client. 3. Enable new syspref MARCMergeRules. 4. Click the new link "MARC merge rules" in the "Catalog" section of the Koha administration page. 5. Create a new rule: Module: source, Filter: *, Tag: 245, Preset: Protect. 6. Clicking "Edit" should allow you to edit corresponding rule. 7. Clicking "Delete" should remove corresponding rule after confirmation. 8. Selecting one or more rules followed by clicking "Delete selected" should remove all selected rules after confirmation. 9. Try creating a rule with tag set to "**", the other options does not matter. Verify that saving this rule produces an error message complaining about invalid tag regular expression. 10. Try creating a rule with tag set to "008" (or other control field) and set Appended: Append and Removed: Skip, the other options does not matter. Verify that saving this rule produces an error message complaining about invalid combination of actions for control field. 11. With the 245 rule in step 5 in place, edit a bibliographic record, change 245a for example (which should be Title for MARC21) and save. 12. Verify that the changes has not been saved. 13. Create a new rule: Module: source, Filter: intranet, Tag: 245, Preset: Overwrite. 14. Repeat step 12, and verify that the changes has now been saved. 15. Run tests in t/db_dependent/Biblio/MarcMergeRules.t and very that all tests pass. Sponsored-by: Halland County Library Sponsored-by: Catalyst IT Sponsored-by: Gothenburg University Library Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126753 [details] [review] Bug 14957: Updated schema files Signed-off-by: Christian Stelzenmüller <christian.stelzenmueller@bsz-bw.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126754 [details] [review] Bug 14957: fix context for batchmod and batchimport Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126755 [details] [review] Bug 14957: (QA follow-up) Button styling This patch updates the action buttons to include btn-default and switched from btn-sm to btn-xs to match other similar UI's found in datatable based admin pages. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126756 [details] [review] Bug 14957: (QA follow-up) Add tooltips This patch adds an info-tooltip to the field rules in the table header to expand on their meaning to reduce the need to read the documentation each time you come to use the configuration tool Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126757 [details] [review] Bug 14957: (QA follow-up) Rename 'merge' => 'overlay' This patch acknowledges the fact that 'merging' has a different meaning in Koha than the behavior this great patchset introduces. The more idiomatic way of describing the behavior is to talk about 'overlay rules'. This patch also: - Fixes kohastructure.sql missing table encoding a collation - Fixes the atomic update completely missing a table - Moves the syspref entry to 'Importing', probably a rebase issue Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126758 [details] [review] Bug 14957: Feature hidden in menues if no other 'cagaloguing' permissions Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126759 [details] [review] Bug 14957: DBIC changes Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126760 [details] [review] Bug 14957: (follow-up) marc_merge_rules_modules removed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126761 [details] [review] Bug 14957: (QA follow-up) Clarify 'context' param This patch renames the (passed through) 'context' param for 'overlay_context'. I propose doing so, because in Koha-land 'context' has a special meaning, related to C4::Context and it reads ambigous. The patch itself is pretty trivial. Tests should pass: 1. Run: $ kshell k$ prove t/db_dependent/Biblio/MarcOverlayRules.t => SUCCESS: Tests pass 2. Apply this patch 3. Repeat 1 => SUCCESS: Tests still pass! 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126762 [details] [review] Bug 14957: (QA follow-up) Fix syntax error in atomicupdate Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126763 [details] [review] Bug 14957: (QA follow-up) Fix translation issues This patch fixes all the translation issues I could spot in the marc-overlay-rules template. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126764 [details] [review] Bug 14957: (QA follow-up) Rename and move admin link This patch renames 'MARC overlay rules' to 'Record overlay rules' and moves it up the list to just below 'Record matching rules' to make it more contextually ordered.
Created attachment 126765 [details] [review] Bug 14957: (QA follow-up) Comment 'bulkmarcimport' temporarily It appears this patchset doesn't fully support bulkmarcimport (see bug 25539 for details). At this time, to get this bug moving I think we should comment out the ability to define rules that affect bulkmarcimport and then we can fix and re-enable that feature at a later date. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126766 [details] [review] Bug 14957: (QA follow-up) Add missing filters Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126767 [details] [review] Bug 14957: (QA follow-up) Update for bug 26703 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126768 [details] [review] Bug 14957: (QA follow-up) Update for bug 20054 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126769 [details] [review] Bug 14957: (QA follow-up) Update for bug 20053 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126770 [details] [review] Bug 14957: (QA follow-up) Update for bug 17858 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126771 [details] [review] Bug 14957: (QA follow-up) Update for bug 27846 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126772 [details] [review] Bug 14957: (QA follow-up) Fix template indentation Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126773 [details] [review] Bug 14957: (QA follow-up) Update for bug 23271 Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126774 [details] [review] Bug 14957: (QA follow-up) Fix script name Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126775 [details] [review] Bug 14957: (QA follow-up) Fix for jQuery undefined Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126776 [details] [review] Bug 14957: Clean controller Too many imports and unecessary complicated init of $rules There are certainly other things to improve, like the "op" handling, standardize names of "op", etc. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126777 [details] [review] Bug 14957: Improve readbility of tests Not saying that they were not readable before, but I had to do some changes to compare the before/after records and it was easier that way. Feel free to obsolete if you disagree. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126778 [details] [review] Bug 14957: Fix imports in tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126779 [details] [review] Bug 14957: Fix regression, rules must be refetched on add/edit/deletion Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126780 [details] [review] Bug 14957: Fix imports in marc-overlay-rules.pl Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126781 [details] [review] Bug 14957: Exclude leader from saved/updated record comparison in tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126782 [details] [review] Bug 14957: (follow-up) Clarify 'context' param Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126783 [details] [review] Bug 14957: Fix record preview Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126784 [details] [review] Bug 14957: Fix syspref's values It didn't switch off when set to "don't use" Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126785 [details] [review] Bug 14957: Fix field order issue Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126786 [details] [review] Bug 14957: Add tests for subfield order Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I worked through the whole set one last time.. must admit brain is hazy on the details with so much water under the bridge again.. but it all appears to be working as expected. QA scripts spit out some noise, but I'm confident it's all noise due to git manipulations and things. Passing QA. Yes, we need the feedback stuff handled in a follow-up bug.
Some patches will be squashed when pushed.
Pushed to master for 21.11, thanks to everybody involved!
It would be great to have something ready for the manual before 21.11 is released. By the way, the release notes are saying "NOTE: A follow-up bug is being worked upon to add compatibility with bulkmarcimport.". Which bug is that?
Great :) Thanks everyone. There is a patch for the manual: https://gitlab.com/gnucifer/koha-manual/commit/593c5da008dbc4b074cac9dee167cce9a6dfc250#c5e2c75dc923f21c5b95185b2f350df00fec661f But some of the terminology need to change (marc overlay rules instaed of merge rules for example).
(In reply to Jonathan Druart from comment #439) > By the way, the release notes are saying "NOTE: A follow-up bug is being > worked upon to add compatibility with bulkmarcimport.". Which bug is that? Probably bug 25539
Created attachment 126950 [details] [review] Bug 14957: Fix perlcritic error on admin/marc-overlay-rules.pl Subroutine "new" called using indirect syntax at line 30, column 13. See page 349 of PBP. (Severity: 4)
Created attachment 126951 [details] [review] Bug 14957: DBIC changes - Remove MarcOverlayRulesModule.pm Where is that coming from??
Last two patches pushed to master.
(In reply to David Gustafsson from comment #440) > Great :) Thanks everyone. There is a patch for the manual: > https://gitlab.com/gnucifer/koha-manual/commit/ > 593c5da008dbc4b074cac9dee167cce9a6dfc250#c5e2c75dc923f21c5b95185b2f350df00fec > 661f > > But some of the terminology need to change (marc overlay rules instaed of > merge rules for example). Could you submit that as a merge request on gitlab now please.. I can do the tidying up if you like.
Ok, thanks! https://gitlab.com/gnucifer/koha-manual/-/merge_requests/1
As someone who periodically uses MarcEdit's integration w/ Koha to download, change, & upload records & I'm curious whether this is one of the included update methods for this enhancement (batchimport, zv39.50, intranet, bulkmarcimport (?), import_lexile, batchmod). I believe the MarcEdit integration relies on an API, so I'm thinking probably not? (https://blog.reeset.net/archives/2133; also 1123).
Good point. There is a ModBiblio in svc/bib, there should absolutely be a context module for that, would be easy to add. But I don't know what API MarcEdit is using. There is a swagger-api that I'm totally unfamiliar with, but from what I can gather from there definitions there don't seem to be any support for updating biblios, but it's pretty impenetrable to me as now nothing about swagger and/or that part of Koha. It's also hard to figure out what MarcEdit actually uses since there is no actual code in that post.