Bug 14957 - Write protecting MARC fields based on source of import
Summary: Write protecting MARC fields based on source of import
Status: ASSIGNED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: David Gustafsson
QA Contact: Tomás Cohen Arazi
URL: https://gitlab.com/gnucifer/koha-manu...
Keywords: additional_work_needed, Manual, rel_21_11_candidate
: 11519 11780 13213 18210 22354 26372 (view as bug list)
Depends on: 24788
Blocks: 25539
  Show dependency treegraph
 
Reported: 2015-10-05 14:07 UTC by Martin Stenberg
Modified: 2021-06-08 12:02 UTC (History)
35 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Text to go in the release notes:
This enhancement enables the use of rules for merging MARC records. For example, it can be used to prevent field data from being overwritten. It is enabled using the new system preference "MARCOverlayRules". Rules are added, edited and deleted in the staff interface from Home > Koha administration > Catalog > MARC overlay rules. NOTE: A follow-up bug is being worked upon to add compatibility with bulkmarcimport.
Version(s) released in:


Attachments
Bug 14957: Rule based permission for MARC fields (81.46 KB, patch)
2015-10-27 11:31 UTC, Martin Stenberg
Details | Diff | Splinter Review
Bug 14957: Database changes (3.56 KB, patch)
2015-10-27 11:31 UTC, Martin Stenberg
Details | Diff | Splinter Review
Bug 14957: Add filters to relevant scripts (5.84 KB, patch)
2015-10-27 11:31 UTC, Martin Stenberg
Details | Diff | Splinter Review
Bug 14957: Rule based permission for MARC fields (82.59 KB, patch)
2016-05-23 20:53 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 14957: Correct parameters for TransformMarcToKoha (940 bytes, patch)
2016-05-23 20:59 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 14957: Key fixes to patch (19.48 KB, patch)
2016-10-19 04:23 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14957: Key fixes to patch (19.48 KB, patch)
2016-10-19 04:28 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14957: Database changes (3.56 KB, patch)
2016-10-19 09:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14957: Add filters to relevant scripts (6.07 KB, patch)
2016-10-19 09:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14957: Rule based permission for MARC fields (81.96 KB, patch)
2016-10-19 09:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14957: Correct parameters for TransformMarcToKoha (940 bytes, patch)
2016-10-19 09:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14957: Key fixes to patch (19.48 KB, patch)
2016-10-19 09:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14957: Followup - Pod, import_lexile.pl, template and test fixes (3.45 KB, patch)
2016-10-19 09:23 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14957: Database changes (3.56 KB, patch)
2016-11-03 07:06 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14957: Add filters to relevant scripts (6.07 KB, patch)
2016-11-03 07:06 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14957: Rule based permission for MARC fields (81.60 KB, patch)
2016-11-03 07:06 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14957: Correct parameters for TransformMarcToKoha (940 bytes, patch)
2016-11-03 07:06 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14957: Key fixes to patch (19.48 KB, patch)
2016-11-03 07:07 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14957: Followup - Pod, import_lexile.pl, template and test fixes (3.45 KB, patch)
2016-11-03 07:07 UTC, Josef Moravec
Details | Diff | Splinter Review
Rule based permission for MARC fields (87.02 KB, patch)
2017-02-02 12:44 UTC, David Gustafsson
Details | Diff | Splinter Review
Rule based permission for MARC fields (86.66 KB, patch)
2017-02-02 12:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (90.52 KB, patch)
2017-02-03 14:09 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (90.64 KB, patch)
2017-02-27 13:09 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (90.64 KB, patch)
2017-03-06 09:59 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (90.60 KB, patch)
2017-03-06 10:04 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (90.88 KB, patch)
2017-03-14 13:25 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (90.77 KB, patch)
2017-03-27 12:00 UTC, Johan Larsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (91.35 KB, patch)
2017-03-30 12:24 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (92.35 KB, patch)
2017-07-04 14:15 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (68.58 KB, patch)
2018-03-07 10:16 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (68.40 KB, patch)
2018-04-25 19:22 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (67.12 KB, patch)
2018-05-07 12:27 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (122.93 KB, patch)
2018-06-20 12:07 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (66.56 KB, patch)
2018-06-20 12:10 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (122.93 KB, patch)
2018-06-20 12:10 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (66.56 KB, patch)
2018-06-20 12:11 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Refactor and fix tests and documention (122.93 KB, patch)
2018-06-20 12:11 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (66.56 KB, patch)
2018-06-20 12:12 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Refactor and fix tests and documentation (122.93 KB, patch)
2018-06-20 12:12 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (66.56 KB, patch)
2018-06-20 12:15 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Refactor and fix tests and documentation (122.93 KB, patch)
2018-06-20 12:15 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Remove exports of removed subroutines (733 bytes, patch)
2018-06-20 12:23 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (66.75 KB, patch)
2018-09-14 13:00 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Refactor and fix tests and documentation (123.17 KB, patch)
2018-09-14 13:00 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Remove exports of removed subroutines (733 bytes, patch)
2018-09-14 13:00 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Remove exports of removed subroutines (733 bytes, patch)
2018-09-25 12:37 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (66.75 KB, patch)
2018-09-25 12:39 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Refactor and fix tests and documentation (123.17 KB, patch)
2018-09-25 12:39 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Remove exports of removed subroutines (733 bytes, patch)
2018-09-25 12:39 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Add missing use in Biblio.pm (594 bytes, patch)
2018-10-03 13:52 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (66.95 KB, patch)
2018-10-22 11:57 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Refactor and fix tests and documentation (123.19 KB, patch)
2018-10-22 11:58 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Remove exports of removed subroutines (770 bytes, patch)
2018-10-22 11:58 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Add missing use in Biblio.pm (570 bytes, patch)
2018-10-22 11:58 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Fix control field rules regression and add rule validation (9.10 KB, patch)
2018-10-25 15:17 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Fix control field rules regression and add rule validation (11.17 KB, patch)
2018-10-25 15:18 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Fix control field rules regression and add rule validation (11.13 KB, patch)
2018-10-25 15:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Fix control field rules regression and add rule validation (11.26 KB, patch)
2018-10-25 15:23 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: DO NOT PUSH Schema changes (4.89 KB, patch)
2018-10-30 13:50 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957 - Rule based permission for MARC fields (80.65 KB, patch)
2018-10-31 10:59 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: DO NOT PUSH Schema changes (4.89 KB, patch)
2018-10-31 11:03 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (80.70 KB, patch)
2018-10-31 11:17 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: DO NOT PUSH Schema changes (4.89 KB, patch)
2018-10-31 11:17 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (80.70 KB, patch)
2018-10-31 11:18 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: DO NOT PUSH Schema changes (4.89 KB, patch)
2018-10-31 11:18 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 21596: (follow-up) Handle updated_on part 2 (1.31 KB, patch)
2018-11-05 15:50 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (80.87 KB, patch)
2018-11-05 15:52 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: DO NOT PUSH Schema changes (4.89 KB, patch)
2018-11-05 15:52 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (80.87 KB, patch)
2018-11-07 14:35 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: DO NOT PUSH Schema changes (4.89 KB, patch)
2018-11-07 14:36 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (83.17 KB, patch)
2018-11-30 10:35 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (83.13 KB, patch)
2019-04-10 13:11 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (83.13 KB, patch)
2019-04-10 13:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Fix most Koha QA test tool errors (12.74 KB, patch)
2019-04-11 14:27 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Put database updates in perl script (6.08 KB, patch)
2019-04-12 17:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Put database updates in perl script (8.96 KB, patch)
2019-04-17 15:37 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Add tests and pod for new options param to ModBiblio (6.38 KB, patch)
2019-04-17 15:38 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Fix categorycode module filter value and rename some presets (4.66 KB, patch)
2019-05-22 14:43 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (91.10 KB, patch)
2019-06-17 14:16 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: DO NOT PUSH Schema changes (2.46 KB, patch)
2019-06-17 14:17 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: DO NOT PUSH Schema changes (2.46 KB, patch)
2019-06-17 14:27 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (90.45 KB, patch)
2019-06-17 14:27 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (88.92 KB, patch)
2019-06-18 11:04 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Make preset labels translatable (2.96 KB, patch)
2019-06-18 12:39 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Make preset labels translatable (3.61 KB, patch)
2019-06-18 13:00 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Make preset labels translatable (3.83 KB, patch)
2019-06-18 13:17 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: DO NOT PUSH Schema changes (2.46 KB, patch)
2019-09-18 12:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (88.98 KB, patch)
2019-09-18 12:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Make preset labels translatable (3.83 KB, patch)
2019-09-18 12:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Adjust paging option for new version of datatables (997 bytes, patch)
2019-09-18 12:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Fix rule javascript validation issue (1.73 KB, patch)
2019-09-18 12:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Change YUI layout to to Boostrap (1.36 KB, patch)
2019-09-18 12:49 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Remove prefetch of removed relationship (843 bytes, patch)
2019-09-20 14:14 UTC, David Gustafsson
Details | Diff | Splinter Review
Buttons difference (4.91 KB, image/png)
2019-10-02 15:35 UTC, Séverine Queune
Details
Bug 14957: Add icons for confirmation dialog and set preset based on actions configuration (5.26 KB, patch)
2019-10-31 12:20 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Replace filter value with label if one exists (2.09 KB, patch)
2019-10-31 12:42 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Remove usage of deleted Koha::MarcMergeRulesModules (12.81 KB, patch)
2020-03-03 13:15 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: DO NOT PUSH Schema changes (2.51 KB, patch)
2020-03-03 19:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (89.03 KB, patch)
2020-03-03 19:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 14957: Make preset labels translatable (3.88 KB, patch)
2020-03-03 19:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 14957: Adjust paging option for new version of datatables (1.02 KB, patch)
2020-03-03 19:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 14957: Fix rule javascript validation issue (1.78 KB, patch)
2020-03-03 19:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 14957: Change YUI layout to to Boostrap (1.41 KB, patch)
2020-03-03 19:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 14957: Remove prefetch of removed relationship (891 bytes, patch)
2020-03-03 19:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 14957: Add icons for confirmation dialog and set preset based on actions configuration (5.31 KB, patch)
2020-03-03 19:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 14957: Replace filter value with label if one exists (2.13 KB, patch)
2020-03-03 19:37 UTC, David Nind
Details | Diff | Splinter Review
Bug 14957: Remove usage of deleted Koha::MarcMergeRulesModules (12.86 KB, patch)
2020-03-03 19:38 UTC, David Nind
Details | Diff | Splinter Review
Screenshot of option placement in OPAC (99.09 KB, image/png)
2020-03-24 09:05 UTC, Katrin Fischer
Details
Bug 14957: Merge rules system for merging of MARC records (88.91 KB, patch)
2020-09-11 15:32 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Make preset labels translatable (3.87 KB, patch)
2020-09-11 15:32 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Adjust paging option for new version of datatables (1.02 KB, patch)
2020-09-11 15:32 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Fix rule javascript validation issue (1.77 KB, patch)
2020-09-11 15:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Change YUI layout to to Boostrap (1.40 KB, patch)
2020-09-11 15:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Remove prefetch of removed relationship (891 bytes, patch)
2020-09-11 15:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Add icons for confirmation dialog and set preset based on actions configuration (5.30 KB, patch)
2020-09-11 15:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Replace filter value with label if one exists (2.12 KB, patch)
2020-09-11 15:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Remove usage of deleted Koha::MarcMergeRulesModules (12.85 KB, patch)
2020-09-11 15:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (90.88 KB, patch)
2021-02-10 03:50 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14957: Updated schema files (2.46 KB, patch)
2021-02-10 03:50 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (90.88 KB, patch)
2021-02-10 03:50 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14957: Updated schema files (2.46 KB, patch)
2021-02-10 03:51 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (90.86 KB, patch)
2021-03-05 12:47 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Updated schema files (2.45 KB, patch)
2021-03-05 12:47 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (91.08 KB, patch)
2021-03-05 13:27 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 14957: Updated schema files (2.62 KB, patch)
2021-03-05 13:27 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 14957: fix context for batchmod and batchimport (3.98 KB, patch)
2021-03-05 13:39 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: fix context for batchmod and batchimport (4.42 KB, patch)
2021-03-05 14:43 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (91.14 KB, patch)
2021-03-09 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: Updated schema files (2.73 KB, patch)
2021-03-09 12:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: fix context for batchmod and batchimport (4.54 KB, patch)
2021-03-09 12:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (91.24 KB, patch)
2021-03-28 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14957: Updated schema files (2.79 KB, patch)
2021-03-28 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14957: fix context for batchmod and batchimport (4.60 KB, patch)
2021-03-28 13:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Button styling (3.36 KB, patch)
2021-03-28 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Add tooltips (2.14 KB, patch)
2021-03-28 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Rename 'merge' => 'overlay' (49.81 KB, patch)
2021-03-28 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14957: Feature hidden in menues if no other 'cagaloguing' permissions (3.49 KB, patch)
2021-03-28 13:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14957: DBIC changes (7.45 KB, patch)
2021-03-28 13:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14957: (follow-up) marc_merge_rules_modules removed (3.24 KB, patch)
2021-03-28 13:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Clarify 'context' param (8.22 KB, patch)
2021-03-28 13:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (91.14 KB, patch)
2021-03-29 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: Updated schema files (2.73 KB, patch)
2021-03-29 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: fix context for batchmod and batchimport (4.54 KB, patch)
2021-03-29 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Button styling (3.42 KB, patch)
2021-03-29 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Add tooltips (2.21 KB, patch)
2021-03-29 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Rename 'merge' => 'overlay' (49.84 KB, patch)
2021-03-29 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: Feature hidden in menues if no other 'cagaloguing' permissions (3.54 KB, patch)
2021-03-29 09:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: DBIC changes (7.50 KB, patch)
2021-03-29 09:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (follow-up) marc_merge_rules_modules removed (3.30 KB, patch)
2021-03-29 09:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Clarify 'context' param (8.27 KB, patch)
2021-03-29 09:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Fix syntax error in atomicupdate (1.45 KB, patch)
2021-03-29 09:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Fix translation issues (3.68 KB, patch)
2021-03-29 09:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Rename and move admin link (2.26 KB, patch)
2021-03-29 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Comment 'bulkmarcimport' temporarily (1.39 KB, patch)
2021-03-29 12:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (91.14 KB, patch)
2021-05-05 15:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: Updated schema files (2.73 KB, patch)
2021-05-05 15:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: fix context for batchmod and batchimport (4.54 KB, patch)
2021-05-05 15:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Button styling (3.42 KB, patch)
2021-05-05 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Add tooltips (2.21 KB, patch)
2021-05-05 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Rename 'merge' => 'overlay' (49.84 KB, patch)
2021-05-05 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: Feature hidden in menues if no other 'cagaloguing' permissions (3.54 KB, patch)
2021-05-05 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: DBIC changes (7.50 KB, patch)
2021-05-05 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (follow-up) marc_merge_rules_modules removed (3.30 KB, patch)
2021-05-05 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Clarify 'context' param (8.27 KB, patch)
2021-05-05 15:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Fix syntax error in atomicupdate (1.45 KB, patch)
2021-05-05 15:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Fix translation issues (3.68 KB, patch)
2021-05-05 15:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Rename and move admin link (2.26 KB, patch)
2021-05-05 15:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Comment 'bulkmarcimport' temporarily (1.39 KB, patch)
2021-05-05 15:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Add missing filters (3.46 KB, patch)
2021-05-05 15:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 26703 (1.02 KB, patch)
2021-05-05 15:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 20054 (885 bytes, patch)
2021-05-05 15:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 20053 (865 bytes, patch)
2021-05-05 15:37 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 17858 (18.66 KB, patch)
2021-05-05 15:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 27846 (1.40 KB, patch)
2021-05-05 15:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Fix template indentation (28.21 KB, patch)
2021-05-05 15:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Fix template indentation (28.13 KB, patch)
2021-05-05 15:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 23271 (1.27 KB, patch)
2021-05-06 07:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: Merge rules system for merging of MARC records (91.14 KB, patch)
2021-05-06 07:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: Updated schema files (2.73 KB, patch)
2021-05-06 07:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: fix context for batchmod and batchimport (4.54 KB, patch)
2021-05-06 07:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Button styling (3.42 KB, patch)
2021-05-06 07:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Add tooltips (2.21 KB, patch)
2021-05-06 07:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Rename 'merge' => 'overlay' (49.84 KB, patch)
2021-05-06 07:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: Feature hidden in menues if no other 'cagaloguing' permissions (3.54 KB, patch)
2021-05-06 07:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: DBIC changes (7.50 KB, patch)
2021-05-06 07:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (follow-up) marc_merge_rules_modules removed (3.30 KB, patch)
2021-05-06 07:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Clarify 'context' param (8.27 KB, patch)
2021-05-06 07:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Fix syntax error in atomicupdate (1.45 KB, patch)
2021-05-06 07:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Fix translation issues (3.68 KB, patch)
2021-05-06 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Rename and move admin link (2.26 KB, patch)
2021-05-06 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Comment 'bulkmarcimport' temporarily (1.39 KB, patch)
2021-05-06 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Add missing filters (3.46 KB, patch)
2021-05-06 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 26703 (1.02 KB, patch)
2021-05-06 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 20054 (885 bytes, patch)
2021-05-06 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 20053 (865 bytes, patch)
2021-05-06 07:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 17858 (18.66 KB, patch)
2021-05-06 07:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 27846 (1.40 KB, patch)
2021-05-06 07:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Fix template indentation (28.13 KB, patch)
2021-05-06 07:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Update for bug 23271 (1.27 KB, patch)
2021-05-06 07:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Fix script name (2.70 KB, patch)
2021-05-06 07:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: (QA follow-up) Fix for jQuery undefined (3.54 KB, patch)
2021-05-06 07:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14957: Clean controller (3.10 KB, patch)
2021-05-07 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14957: Clean controller (3.14 KB, patch)
2021-05-07 10:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14957: Improve readbility of tests (40.53 KB, patch)
2021-05-07 16:22 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Stenberg 2015-10-05 14:07:53 UTC
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
Comment 1 Katrin Fischer 2015-10-05 17:34:58 UTC
+100!
Comment 2 Martin Stenberg 2015-10-27 11:31:38 UTC Comment hidden (obsolete)
Comment 3 Martin Stenberg 2015-10-27 11:31:47 UTC Comment hidden (obsolete)
Comment 4 Martin Stenberg 2015-10-27 11:31:58 UTC Comment hidden (obsolete)
Comment 5 Magnus Enger 2016-04-21 19:36:23 UTC
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
Comment 6 Eivin Giske Skaaren 2016-05-23 20:53:46 UTC Comment hidden (obsolete)
Comment 7 Eivin Giske Skaaren 2016-05-23 20:59:31 UTC Comment hidden (obsolete)
Comment 8 Eivin Giske Skaaren 2016-05-23 21:01:05 UTC
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.
Comment 9 David Cook 2016-05-24 23:38:09 UTC
Oh I had forgotten about this one... might have to look at this sometime...
Comment 10 Magnus Enger 2016-06-02 09:39:03 UTC
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!
Comment 11 Magnus Enger 2016-06-02 10:13:34 UTC
Never mind me, I was applying the patches on the 3.22.x branch.
Comment 12 Magnus Enger 2016-06-02 11:56:29 UTC
When you are on the "MARC field permissions" page, "MARC field permissions" is not shown in the left hand menu.
Comment 13 Andreas Hedström Mace 2016-07-19 20:47:21 UTC
Failed to apply for me (merge conflicts). Needs a rebase again?
Comment 14 delaye 2016-10-12 12:28:50 UTC
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 .
Comment 15 Aleisha Amohia 2016-10-19 04:23:46 UTC Comment hidden (obsolete)
Comment 16 Aleisha Amohia 2016-10-19 04:28:15 UTC Comment hidden (obsolete)
Comment 17 Josef Moravec 2016-10-19 09:23:03 UTC Comment hidden (obsolete)
Comment 18 Josef Moravec 2016-10-19 09:23:11 UTC Comment hidden (obsolete)
Comment 19 Josef Moravec 2016-10-19 09:23:18 UTC Comment hidden (obsolete)
Comment 20 Josef Moravec 2016-10-19 09:23:29 UTC Comment hidden (obsolete)
Comment 21 Josef Moravec 2016-10-19 09:23:34 UTC Comment hidden (obsolete)
Comment 22 Josef Moravec 2016-10-19 09:23:42 UTC Comment hidden (obsolete)
Comment 23 Josef Moravec 2016-10-19 09:24:34 UTC
Rebased on master

Fixed few issues in the follow-up
Comment 24 Andreas Hedström Mace 2016-11-02 21:32:04 UTC
Doesn't apply for me. Needs yet another rebase?
Comment 25 Josef Moravec 2016-11-03 07:06:07 UTC Comment hidden (obsolete)
Comment 26 Josef Moravec 2016-11-03 07:06:17 UTC
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.
Comment 27 Josef Moravec 2016-11-03 07:06:51 UTC Comment hidden (obsolete)
Comment 28 Josef Moravec 2016-11-03 07:06:59 UTC Comment hidden (obsolete)
Comment 29 Josef Moravec 2016-11-03 07:07:05 UTC Comment hidden (obsolete)
Comment 30 Josef Moravec 2016-11-03 07:07:11 UTC Comment hidden (obsolete)
Comment 31 Josef Moravec 2016-11-03 07:08:17 UTC
It really needed rebasing again. Done.
Comment 32 Andreas Hedström Mace 2016-12-13 22:42:34 UTC
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?
Comment 33 David Gustafsson 2017-02-02 12:44:21 UTC Comment hidden (obsolete)
Comment 34 David Gustafsson 2017-02-02 12:49:03 UTC Comment hidden (obsolete)
Comment 35 David Gustafsson 2017-02-02 13:10:51 UTC
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.
Comment 36 Andreas Hedström Mace 2017-02-03 09:38:14 UTC
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>
Comment 37 Magnus Enger 2017-02-03 09:49:45 UTC
(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?
Comment 38 David Gustafsson 2017-02-03 12:39:06 UTC
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).
Comment 39 Magnus Enger 2017-02-03 12:48:25 UTC
(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. :-)
Comment 40 Andreas Hedström Mace 2017-02-03 13:42:16 UTC
(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!
Comment 41 David Gustafsson 2017-02-03 14:09:20 UTC Comment hidden (obsolete)
Comment 42 David Gustafsson 2017-02-03 14:13:50 UTC
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).
Comment 43 David Gustafsson 2017-02-27 13:09:39 UTC Comment hidden (obsolete)
Comment 44 David Gustafsson 2017-02-27 13:10:37 UTC
Rebased against master.
Comment 45 David Gustafsson 2017-03-06 09:59:24 UTC Comment hidden (obsolete)
Comment 46 David Gustafsson 2017-03-06 10:00:30 UTC
Screwed up the last merge. Should be fixed now.
Comment 47 David Gustafsson 2017-03-06 10:03:43 UTC
Nope, still managed to screw it up. Should be ok now.
Comment 48 David Gustafsson 2017-03-06 10:04:35 UTC Comment hidden (obsolete)
Comment 49 David Gustafsson 2017-03-09 14:06:35 UTC
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.
Comment 50 David Gustafsson 2017-03-14 13:25:23 UTC Comment hidden (obsolete)
Comment 51 David Gustafsson 2017-03-14 13:32:02 UTC
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).
Comment 52 Johan Larsson 2017-03-27 12:00:29 UTC Comment hidden (obsolete)
Comment 53 Johan Larsson 2017-03-27 12:01:57 UTC
corrected spelling error in commit message.
Comment 54 David Gustafsson 2017-03-30 12:24:11 UTC Comment hidden (obsolete)
Comment 55 David Gustafsson 2017-03-30 12:30:45 UTC Comment hidden (obsolete)
Comment 56 David Gustafsson 2017-03-30 12:33:01 UTC
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.
Comment 57 David Gustafsson 2017-07-04 14:15:50 UTC Comment hidden (obsolete)
Comment 58 David Gustafsson 2017-07-04 15:04:03 UTC
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).
Comment 59 Andreas Hedström Mace 2017-08-25 10:30:32 UTC
Wanted to to try and test this again, but patch doesn't apply for me.
Comment 60 David Gustafsson 2018-01-25 17:31:25 UTC
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.
Comment 61 Katrin Fischer 2018-01-30 07:20:41 UTC
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.
Comment 62 David Gustafsson 2018-01-30 10:31:53 UTC
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.
Comment 63 Katrin Fischer 2018-02-22 21:52:39 UTC
Hi David, thx for the feedback. Moving this to Failed QA as it seems more appropriate for now.
Comment 64 David Gustafsson 2018-03-07 10:16:36 UTC Comment hidden (obsolete)
Comment 65 paxed 2018-03-07 10:25:09 UTC
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));
Comment 66 David Gustafsson 2018-03-07 10:26:36 UTC
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.
Comment 67 Katrin Fischer 2018-04-01 14:00:55 UTC
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.
Comment 68 David Gustafsson 2018-04-25 19:22:47 UTC Comment hidden (obsolete)
Comment 69 David Gustafsson 2018-04-25 19:23:59 UTC
Removed String::Similarity dependency and fixed an issue with the field data hashing function.
Comment 70 David Gustafsson 2018-04-25 19:25:53 UTC
@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.
Comment 71 David Gustafsson 2018-05-07 12:27:03 UTC Comment hidden (obsolete)
Comment 72 David Gustafsson 2018-05-07 12:30:30 UTC
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.
Comment 73 David Gustafsson 2018-05-07 12:33:04 UTC
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.
Comment 74 David Gustafsson 2018-06-14 14:11:10 UTC
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.
Comment 75 David Gustafsson 2018-06-20 12:07:34 UTC Comment hidden (obsolete)
Comment 76 David Gustafsson 2018-06-20 12:10:24 UTC Comment hidden (obsolete)
Comment 77 David Gustafsson 2018-06-20 12:10:30 UTC Comment hidden (obsolete)
Comment 78 David Gustafsson 2018-06-20 12:11:38 UTC Comment hidden (obsolete)
Comment 79 David Gustafsson 2018-06-20 12:11:43 UTC Comment hidden (obsolete)
Comment 80 David Gustafsson 2018-06-20 12:12:31 UTC Comment hidden (obsolete)
Comment 81 David Gustafsson 2018-06-20 12:12:36 UTC Comment hidden (obsolete)
Comment 82 David Gustafsson 2018-06-20 12:15:07 UTC Comment hidden (obsolete)
Comment 83 David Gustafsson 2018-06-20 12:15:16 UTC Comment hidden (obsolete)
Comment 84 David Gustafsson 2018-06-20 12:16:07 UTC
Sorry about the many attachments, had some issues adding the latest patch. Now everything should be in order.
Comment 85 David Gustafsson 2018-06-20 12:23:18 UTC Comment hidden (obsolete)
Comment 86 Katrin Fischer 2018-06-22 05:38:32 UTC
Hi David, thank you for your work on this!
Comment 87 David Gustafsson 2018-06-26 16:05:26 UTC
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.
Comment 88 Cab Vinton 2018-08-09 13:02:42 UTC
Unable to apply patch in a number of Biblibre sandboxes even after several attempts.
Comment 89 Cab Vinton 2018-08-09 14:33:53 UTC
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 .
Comment 90 David Gustafsson 2018-09-14 13:00:42 UTC Comment hidden (obsolete)
Comment 91 David Gustafsson 2018-09-14 13:00:48 UTC Comment hidden (obsolete)
Comment 92 David Gustafsson 2018-09-14 13:00:53 UTC Comment hidden (obsolete)
Comment 93 David Gustafsson 2018-09-14 13:03:07 UTC
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.
Comment 94 Claire Gravely 2018-09-14 18:49:12 UTC
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.
Comment 95 Tomás Cohen Arazi 2018-09-19 14:40:13 UTC
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.
Comment 96 David Gustafsson 2018-09-25 12:37:49 UTC Comment hidden (obsolete)
Comment 97 David Gustafsson 2018-09-25 12:39:03 UTC Comment hidden (obsolete)
Comment 98 David Gustafsson 2018-09-25 12:39:08 UTC Comment hidden (obsolete)
Comment 99 David Gustafsson 2018-09-25 12:39:15 UTC Comment hidden (obsolete)
Comment 100 David Gustafsson 2018-09-25 12:46:41 UTC
(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).
Comment 101 Tomás Cohen Arazi 2018-09-25 12:56:00 UTC
(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
Comment 102 David Gustafsson 2018-10-03 13:52:01 UTC Comment hidden (obsolete)
Comment 103 David Gustafsson 2018-10-03 13:54:02 UTC
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.
Comment 104 Cori Lynn Arnold 2018-10-17 13:15:12 UTC
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
Comment 105 David Gustafsson 2018-10-22 11:57:55 UTC Comment hidden (obsolete)
Comment 106 David Gustafsson 2018-10-22 11:58:02 UTC Comment hidden (obsolete)
Comment 107 David Gustafsson 2018-10-22 11:58:08 UTC Comment hidden (obsolete)
Comment 108 David Gustafsson 2018-10-22 11:58:14 UTC Comment hidden (obsolete)
Comment 109 David Gustafsson 2018-10-22 12:01:24 UTC
Rebased against master. I think I was responsible for the conflict this time (bug 19687) :)
Comment 110 David Gustafsson 2018-10-25 15:17:41 UTC Comment hidden (obsolete)
Comment 111 David Gustafsson 2018-10-25 15:18:42 UTC Comment hidden (obsolete)
Comment 112 David Gustafsson 2018-10-25 15:20:46 UTC Comment hidden (obsolete)
Comment 113 David Gustafsson 2018-10-25 15:23:02 UTC
Created attachment 81179 [details] [review]
Bug 14957: Fix control field rules regression and add rule validation
Comment 114 Cori Lynn Arnold 2018-10-30 12:54:42 UTC
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
Comment 115 David Gustafsson 2018-10-30 13:44:36 UTC
(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.
Comment 116 David Gustafsson 2018-10-30 13:50:05 UTC Comment hidden (obsolete)
Comment 117 David Gustafsson 2018-10-30 13:50:49 UTC
Pushed the schema changes and followed the convention in Bug 19482.
Comment 118 David Gustafsson 2018-10-30 13:56:41 UTC
I could also squash all the changes and add a new updated "To test", anyone has some input on this?
Comment 119 Cori Lynn Arnold 2018-10-30 15:15:21 UTC
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.
Comment 120 David Gustafsson 2018-10-30 15:40:42 UTC
Ok, help pages will have to moved to online manual as previously noted.
Comment 121 David Gustafsson 2018-10-31 10:59:53 UTC Comment hidden (obsolete)
Comment 122 David Gustafsson 2018-10-31 11:03:30 UTC Comment hidden (obsolete)
Comment 123 David Gustafsson 2018-10-31 11:17:37 UTC Comment hidden (obsolete)
Comment 124 David Gustafsson 2018-10-31 11:17:42 UTC Comment hidden (obsolete)
Comment 125 David Gustafsson 2018-10-31 11:18:29 UTC Comment hidden (obsolete)
Comment 126 David Gustafsson 2018-10-31 11:18:43 UTC Comment hidden (obsolete)
Comment 127 Cori Lynn Arnold 2018-11-01 19:46:57 UTC

    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"?
Comment 128 David Gustafsson 2018-11-05 15:50:40 UTC Comment hidden (obsolete)
Comment 129 David Gustafsson 2018-11-05 15:52:05 UTC Comment hidden (obsolete)
Comment 130 David Gustafsson 2018-11-05 15:52:12 UTC Comment hidden (obsolete)
Comment 131 David Gustafsson 2018-11-05 15:58:05 UTC
(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.
Comment 132 David Gustafsson 2018-11-07 14:35:53 UTC Comment hidden (obsolete)
Comment 133 David Gustafsson 2018-11-07 14:36:02 UTC Comment hidden (obsolete)
Comment 134 David Gustafsson 2018-11-07 14:36:47 UTC
Fixed too long foreign key length causing troubles with MariaDB.
Comment 135 David Gustafsson 2018-11-30 10:35:32 UTC Comment hidden (obsolete)
Comment 136 David Gustafsson 2018-11-30 10:36:58 UTC
Rebased against master and added granular permission in accordance with Bug 14391.
Comment 137 Katrin Fischer 2019-02-17 12:05:35 UTC
*** Bug 22354 has been marked as a duplicate of this bug. ***
Comment 138 Frank Hansen 2019-03-12 13:24:42 UTC
The patch is tested and works fine. Nice work!
Comment 139 Katrin Fischer 2019-03-12 13:26:29 UTC
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.
Comment 140 Frank Hansen 2019-03-12 14:20:16 UTC
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.
Comment 141 Katrin Fischer 2019-03-12 14:34:35 UTC
(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.
Comment 142 David Gustafsson 2019-04-10 13:11:33 UTC Comment hidden (obsolete)
Comment 143 David Gustafsson 2019-04-10 13:14:12 UTC Comment hidden (obsolete)
Comment 144 David Gustafsson 2019-04-10 13:14:32 UTC
Rebased against master.
Comment 145 Katrin Fischer 2019-04-10 23:58:34 UTC
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 146 Katrin Fischer 2019-04-11 00:08:15 UTC
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.
Comment 147 Katrin Fischer 2019-04-11 00:15:17 UTC
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.
Comment 148 David Gustafsson 2019-04-11 14:27:29 UTC Comment hidden (obsolete)
Comment 149 David Gustafsson 2019-04-12 17:49:01 UTC Comment hidden (obsolete)
Comment 150 David Gustafsson 2019-04-17 15:37:58 UTC Comment hidden (obsolete)
Comment 151 David Gustafsson 2019-04-17 15:38:03 UTC Comment hidden (obsolete)
Comment 152 David Cook 2019-05-14 05:33:47 UTC
(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.
Comment 153 David Gustafsson 2019-05-22 14:43:19 UTC Comment hidden (obsolete)
Comment 154 David Gustafsson 2019-05-22 14:45:17 UTC
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.
Comment 155 Katrin Fischer 2019-06-02 08:16:50 UTC
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.
Comment 156 David Gustafsson 2019-06-12 13:43:30 UTC
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.
Comment 157 Katrin Fischer 2019-06-12 13:50:32 UTC
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.
Comment 158 David Gustafsson 2019-06-17 14:16:43 UTC Comment hidden (obsolete)
Comment 159 David Gustafsson 2019-06-17 14:17:18 UTC Comment hidden (obsolete)
Comment 160 David Gustafsson 2019-06-17 14:27:34 UTC Comment hidden (obsolete)
Comment 161 David Gustafsson 2019-06-17 14:27:42 UTC Comment hidden (obsolete)
Comment 162 David Gustafsson 2019-06-17 14:33:42 UTC
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.
Comment 163 David Gustafsson 2019-06-17 14:36:45 UTC
Also did a pod change in C4/Biblio.pm without the output so need also to do that.
Comment 164 David Gustafsson 2019-06-18 11:04:54 UTC Comment hidden (obsolete)
Comment 165 David Gustafsson 2019-06-18 12:39:32 UTC Comment hidden (obsolete)
Comment 166 David Gustafsson 2019-06-18 12:43:27 UTC
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'
     ]
  %]
Comment 167 David Gustafsson 2019-06-18 13:00:46 UTC Comment hidden (obsolete)
Comment 168 David Gustafsson 2019-06-18 13:17:34 UTC Comment hidden (obsolete)
Comment 169 David Gustafsson 2019-06-18 14:05:39 UTC
Fixed documentation (borrower changed to userid).

https://gitlab.com/gnucifer/koha-manual/tree/bug-14957-marc-merge-rules
Comment 170 David Gustafsson 2019-09-18 12:49:18 UTC Comment hidden (obsolete)
Comment 171 David Gustafsson 2019-09-18 12:49:23 UTC Comment hidden (obsolete)
Comment 172 David Gustafsson 2019-09-18 12:49:28 UTC Comment hidden (obsolete)
Comment 173 David Gustafsson 2019-09-18 12:49:34 UTC Comment hidden (obsolete)
Comment 174 David Gustafsson 2019-09-18 12:49:39 UTC Comment hidden (obsolete)
Comment 175 David Gustafsson 2019-09-18 12:49:45 UTC Comment hidden (obsolete)
Comment 176 David Gustafsson 2019-09-18 12:52:28 UTC
Rebased against current master and fixed some issues.
Comment 177 David Gustafsson 2019-09-20 14:14:38 UTC Comment hidden (obsolete)
Comment 178 Katrin Fischer 2019-09-22 09:30:47 UTC
Thx for the rebase David, I have a little hope to tackle this at the Marseille Hackfest.
Comment 179 Séverine Queune 2019-10-02 15:31:35 UTC
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 !
Comment 180 Séverine Queune 2019-10-02 15:35:31 UTC Comment hidden (obsolete)
Comment 181 Katrin Fischer 2019-10-28 06:58:10 UTC
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.
Comment 182 David Gustafsson 2019-10-29 15:53:55 UTC
Sorry about the late reply, I'm looking into it.
Comment 183 David Gustafsson 2019-10-31 12:20:18 UTC Comment hidden (obsolete)
Comment 184 David Gustafsson 2019-10-31 12:42:28 UTC Comment hidden (obsolete)
Comment 185 David Gustafsson 2019-10-31 12:43:17 UTC
@Séverine Queune I think most points should now be addressed. Thank you for your feedback!
Comment 186 Katrin Fischer 2020-01-05 12:07:13 UTC
*** Bug 11780 has been marked as a duplicate of this bug. ***
Comment 187 Katrin Fischer 2020-01-05 12:07:56 UTC
*** Bug 11519 has been marked as a duplicate of this bug. ***
Comment 188 Katrin Fischer 2020-01-06 12:55:41 UTC
*** Bug 13213 has been marked as a duplicate of this bug. ***
Comment 189 Katrin Fischer 2020-01-11 23:24:55 UTC
*** Bug 18210 has been marked as a duplicate of this bug. ***
Comment 190 David Nind 2020-02-17 09:48:47 UTC
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.
Comment 191 David Gustafsson 2020-03-02 16:09:27 UTC
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.
Comment 192 David Gustafsson 2020-03-03 13:15:05 UTC Comment hidden (obsolete)
Comment 193 David Gustafsson 2020-03-03 13:40:28 UTC
I now found the source of the bug and created a new issue: Bug 24788. This problem is currently blocking this patch.
Comment 194 David Nind 2020-03-03 19:37:04 UTC Comment hidden (obsolete)
Comment 195 David Nind 2020-03-03 19:37:11 UTC Comment hidden (obsolete)
Comment 196 David Nind 2020-03-03 19:37:18 UTC Comment hidden (obsolete)
Comment 197 David Nind 2020-03-03 19:37:24 UTC Comment hidden (obsolete)
Comment 198 David Nind 2020-03-03 19:37:30 UTC Comment hidden (obsolete)
Comment 199 David Nind 2020-03-03 19:37:37 UTC Comment hidden (obsolete)
Comment 200 David Nind 2020-03-03 19:37:43 UTC Comment hidden (obsolete)
Comment 201 David Nind 2020-03-03 19:37:49 UTC Comment hidden (obsolete)
Comment 202 David Nind 2020-03-03 19:37:56 UTC Comment hidden (obsolete)
Comment 203 David Nind 2020-03-03 19:38:02 UTC Comment hidden (obsolete)
Comment 204 David Nind 2020-03-03 19:51:27 UTC
(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)
Comment 205 Katrin Fischer 2020-03-24 09:05:52 UTC Comment hidden (obsolete)
Comment 206 Katrin Fischer 2020-03-24 09:10:15 UTC Comment hidden (obsolete)
Comment 207 Katrin Fischer 2020-06-21 16:15:55 UTC
At the moment this is blocked by bug 25539 and also needs a little rebase!
Comment 208 Katrin Fischer 2020-09-03 20:11:36 UTC
*** Bug 26372 has been marked as a duplicate of this bug. ***
Comment 209 Martin Renvoize 2020-09-09 11:05:02 UTC
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.
Comment 210 David Gustafsson 2020-09-11 15:32:32 UTC Comment hidden (obsolete)
Comment 211 David Gustafsson 2020-09-11 15:32:45 UTC Comment hidden (obsolete)
Comment 212 David Gustafsson 2020-09-11 15:32:57 UTC Comment hidden (obsolete)
Comment 213 David Gustafsson 2020-09-11 15:33:06 UTC Comment hidden (obsolete)
Comment 214 David Gustafsson 2020-09-11 15:33:13 UTC Comment hidden (obsolete)
Comment 215 David Gustafsson 2020-09-11 15:33:22 UTC Comment hidden (obsolete)
Comment 216 David Gustafsson 2020-09-11 15:33:30 UTC Comment hidden (obsolete)
Comment 217 David Gustafsson 2020-09-11 15:33:39 UTC Comment hidden (obsolete)
Comment 218 David Gustafsson 2020-09-11 15:33:47 UTC Comment hidden (obsolete)
Comment 219 David Gustafsson 2020-09-11 15:36:53 UTC
A small conflict in installer/data/mysql/sysprefs.sql, should now be rebased against master.
Comment 220 David Nind 2020-09-18 23:30:57 UTC
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
Comment 221 Katrin Fischer 2020-12-13 13:42:44 UTC
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
Comment 222 Aleisha Amohia 2021-02-10 03:50:08 UTC Comment hidden (obsolete)
Comment 223 Aleisha Amohia 2021-02-10 03:50:15 UTC Comment hidden (obsolete)
Comment 224 Aleisha Amohia 2021-02-10 03:50:59 UTC Comment hidden (obsolete)
Comment 225 Aleisha Amohia 2021-02-10 03:51:07 UTC Comment hidden (obsolete)
Comment 226 Andrew Fuerste-Henry 2021-02-10 13:42:20 UTC
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.
Comment 227 Martin Renvoize 2021-03-02 16:41:45 UTC
What's the current status of this one..?
Comment 228 Christian Stelzenmüller 2021-03-05 07:32:34 UTC
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
Comment 229 David Gustafsson 2021-03-05 12:34:36 UTC
Thats probably because of a conflict in that file. I will see if can rebase and fix this.
Comment 230 David Gustafsson 2021-03-05 12:47:32 UTC Comment hidden (obsolete)
Comment 231 David Gustafsson 2021-03-05 12:47:53 UTC Comment hidden (obsolete)
Comment 232 PTFS Europe Sandboxes 2021-03-05 13:27:21 UTC Comment hidden (obsolete)
Comment 233 PTFS Europe Sandboxes 2021-03-05 13:27:27 UTC Comment hidden (obsolete)
Comment 234 David Gustafsson 2021-03-05 13:39:18 UTC Comment hidden (obsolete)
Comment 235 David Gustafsson 2021-03-05 14:00:07 UTC
(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.
Comment 236 David Gustafsson 2021-03-05 14:43:15 UTC Comment hidden (obsolete)
Comment 237 David Gustafsson 2021-03-05 14:45:29 UTC
Have now verified that batchmod rules should works again.
Comment 238 Martin Renvoize 2021-03-09 08:57:29 UTC
(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.
Comment 239 Martin Renvoize 2021-03-09 12:40:53 UTC Comment hidden (obsolete)
Comment 240 Martin Renvoize 2021-03-09 12:41:00 UTC Comment hidden (obsolete)
Comment 241 Martin Renvoize 2021-03-09 12:41:06 UTC Comment hidden (obsolete)
Comment 242 Tomás Cohen Arazi 2021-03-26 16:25:38 UTC
Martin and I are working heavily on this one for QA. Marking as assigned just in case, to avoid overlapping with others.
Comment 243 Tomás Cohen Arazi 2021-03-28 13:39:07 UTC Comment hidden (obsolete)
Comment 244 Tomás Cohen Arazi 2021-03-28 13:39:31 UTC Comment hidden (obsolete)
Comment 245 Tomás Cohen Arazi 2021-03-28 13:39:51 UTC Comment hidden (obsolete)
Comment 246 Tomás Cohen Arazi 2021-03-28 13:40:09 UTC Comment hidden (obsolete)
Comment 247 Tomás Cohen Arazi 2021-03-28 13:40:21 UTC Comment hidden (obsolete)
Comment 248 Tomás Cohen Arazi 2021-03-28 13:40:31 UTC Comment hidden (obsolete)
Comment 249 Tomás Cohen Arazi 2021-03-28 13:40:43 UTC Comment hidden (obsolete)
Comment 250 Tomás Cohen Arazi 2021-03-28 13:41:13 UTC Comment hidden (obsolete)
Comment 251 Tomás Cohen Arazi 2021-03-28 13:41:30 UTC Comment hidden (obsolete)
Comment 252 Tomás Cohen Arazi 2021-03-28 13:41:43 UTC Comment hidden (obsolete)
Comment 253 Tomás Cohen Arazi 2021-03-28 13:50:32 UTC
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
Comment 254 Tomás Cohen Arazi 2021-03-28 13:51:49 UTC
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.
Comment 255 Martin Renvoize 2021-03-29 09:28:15 UTC Comment hidden (obsolete)
Comment 256 Martin Renvoize 2021-03-29 09:28:22 UTC Comment hidden (obsolete)
Comment 257 Martin Renvoize 2021-03-29 09:28:29 UTC Comment hidden (obsolete)
Comment 258 Martin Renvoize 2021-03-29 09:28:35 UTC Comment hidden (obsolete)
Comment 259 Martin Renvoize 2021-03-29 09:28:42 UTC Comment hidden (obsolete)
Comment 260 Martin Renvoize 2021-03-29 09:28:50 UTC Comment hidden (obsolete)
Comment 261 Martin Renvoize 2021-03-29 09:28:57 UTC Comment hidden (obsolete)
Comment 262 Martin Renvoize 2021-03-29 09:29:04 UTC Comment hidden (obsolete)
Comment 263 Martin Renvoize 2021-03-29 09:29:11 UTC Comment hidden (obsolete)
Comment 264 Martin Renvoize 2021-03-29 09:29:18 UTC Comment hidden (obsolete)
Comment 265 Martin Renvoize 2021-03-29 09:29:25 UTC Comment hidden (obsolete)
Comment 266 Martin Renvoize 2021-03-29 09:29:32 UTC Comment hidden (obsolete)
Comment 267 Martin Renvoize 2021-03-29 09:33:47 UTC Comment hidden (obsolete)
Comment 268 Martin Renvoize 2021-03-29 12:33:49 UTC Comment hidden (obsolete)
Comment 269 Jonathan Druart 2021-04-08 13:24:49 UTC
The QA script raises several errors, please double-check.
Comment 270 Katrin Fischer 2021-04-14 11:43:22 UTC
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.
Comment 271 Martin Renvoize 2021-05-05 15:35:39 UTC Comment hidden (obsolete)
Comment 272 Martin Renvoize 2021-05-05 15:35:47 UTC Comment hidden (obsolete)
Comment 273 Martin Renvoize 2021-05-05 15:35:55 UTC Comment hidden (obsolete)
Comment 274 Martin Renvoize 2021-05-05 15:36:03 UTC Comment hidden (obsolete)
Comment 275 Martin Renvoize 2021-05-05 15:36:12 UTC Comment hidden (obsolete)
Comment 276 Martin Renvoize 2021-05-05 15:36:20 UTC Comment hidden (obsolete)
Comment 277 Martin Renvoize 2021-05-05 15:36:28 UTC Comment hidden (obsolete)
Comment 278 Martin Renvoize 2021-05-05 15:36:36 UTC Comment hidden (obsolete)
Comment 279 Martin Renvoize 2021-05-05 15:36:45 UTC Comment hidden (obsolete)
Comment 280 Martin Renvoize 2021-05-05 15:36:54 UTC Comment hidden (obsolete)
Comment 281 Martin Renvoize 2021-05-05 15:37:01 UTC Comment hidden (obsolete)
Comment 282 Martin Renvoize 2021-05-05 15:37:10 UTC Comment hidden (obsolete)
Comment 283 Martin Renvoize 2021-05-05 15:37:17 UTC Comment hidden (obsolete)
Comment 284 Martin Renvoize 2021-05-05 15:37:25 UTC Comment hidden (obsolete)
Comment 285 Martin Renvoize 2021-05-05 15:37:33 UTC Comment hidden (obsolete)
Comment 286 Martin Renvoize 2021-05-05 15:37:41 UTC Comment hidden (obsolete)
Comment 287 Martin Renvoize 2021-05-05 15:37:50 UTC Comment hidden (obsolete)
Comment 288 Martin Renvoize 2021-05-05 15:37:58 UTC Comment hidden (obsolete)
Comment 289 Martin Renvoize 2021-05-05 15:38:07 UTC Comment hidden (obsolete)
Comment 290 Martin Renvoize 2021-05-05 15:40:56 UTC
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.
Comment 291 Martin Renvoize 2021-05-05 15:44:14 UTC Comment hidden (obsolete)
Comment 292 Martin Renvoize 2021-05-05 15:49:48 UTC Comment hidden (obsolete)
Comment 293 Martin Renvoize 2021-05-05 15:55:10 UTC Comment hidden (obsolete)
Comment 294 Katrin Fischer 2021-05-05 21:00:25 UTC
Seeing the last status change - what happened?
Comment 295 Martin Renvoize 2021-05-06 07:01:10 UTC Comment hidden (obsolete)
Comment 296 Martin Renvoize 2021-05-06 07:27:34 UTC Comment hidden (obsolete)
Comment 297 Martin Renvoize 2021-05-06 07:27:42 UTC Comment hidden (obsolete)
Comment 298 Martin Renvoize 2021-05-06 07:27:50 UTC Comment hidden (obsolete)
Comment 299 Martin Renvoize 2021-05-06 07:27:58 UTC Comment hidden (obsolete)
Comment 300 Martin Renvoize 2021-05-06 07:28:07 UTC Comment hidden (obsolete)
Comment 301 Martin Renvoize 2021-05-06 07:28:15 UTC Comment hidden (obsolete)
Comment 302 Martin Renvoize 2021-05-06 07:28:23 UTC Comment hidden (obsolete)
Comment 303 Martin Renvoize 2021-05-06 07:28:31 UTC Comment hidden (obsolete)
Comment 304 Martin Renvoize 2021-05-06 07:28:41 UTC Comment hidden (obsolete)
Comment 305 Martin Renvoize 2021-05-06 07:28:49 UTC Comment hidden (obsolete)
Comment 306 Martin Renvoize 2021-05-06 07:28:58 UTC Comment hidden (obsolete)
Comment 307 Martin Renvoize 2021-05-06 07:29:05 UTC Comment hidden (obsolete)
Comment 308 Martin Renvoize 2021-05-06 07:29:13 UTC Comment hidden (obsolete)
Comment 309 Martin Renvoize 2021-05-06 07:29:22 UTC Comment hidden (obsolete)
Comment 310 Martin Renvoize 2021-05-06 07:29:30 UTC Comment hidden (obsolete)
Comment 311 Martin Renvoize 2021-05-06 07:29:38 UTC Comment hidden (obsolete)
Comment 312 Martin Renvoize 2021-05-06 07:29:46 UTC Comment hidden (obsolete)
Comment 313 Martin Renvoize 2021-05-06 07:29:54 UTC Comment hidden (obsolete)
Comment 314 Martin Renvoize 2021-05-06 07:30:02 UTC Comment hidden (obsolete)
Comment 315 Martin Renvoize 2021-05-06 07:30:11 UTC Comment hidden (obsolete)
Comment 316 Martin Renvoize 2021-05-06 07:30:20 UTC Comment hidden (obsolete)
Comment 317 Martin Renvoize 2021-05-06 07:30:28 UTC Comment hidden (obsolete)
Comment 318 Martin Renvoize 2021-05-06 07:30:36 UTC Comment hidden (obsolete)
Comment 319 Martin Renvoize 2021-05-06 07:30:45 UTC Comment hidden (obsolete)
Comment 320 Martin Renvoize 2021-05-06 07:36:08 UTC
(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.
Comment 321 Jonathan Druart 2021-05-07 09:42:32 UTC
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?
Comment 322 Jonathan Druart 2021-05-07 10:08:21 UTC
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.
Comment 323 Jonathan Druart 2021-05-07 10:09:34 UTC
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.
Comment 324 Jonathan Druart 2021-05-07 10:31:03 UTC
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?
Comment 325 Jonathan Druart 2021-05-07 10:44:30 UTC
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?
Comment 326 Martin Renvoize 2021-05-07 11:43:39 UTC
(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.
Comment 327 Martin Renvoize 2021-05-07 11:52:36 UTC
(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 ! :(
Comment 328 Martin Renvoize 2021-05-07 11:58:59 UTC
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.
Comment 329 Jonathan Druart 2021-05-07 12:55:55 UTC
(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.
Comment 330 Jonathan Druart 2021-05-07 14:04:42 UTC
(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.
Comment 331 Jonathan Druart 2021-05-07 15:03:52 UTC
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).
Comment 332 Jonathan Druart 2021-05-07 16:22:07 UTC
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.
Comment 333 Jonathan Druart 2021-05-07 16:25:48 UTC
(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.
Comment 334 Jonathan Druart 2021-05-11 10:47:10 UTC
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.
Comment 335 Jonathan Druart 2021-06-04 09:04:23 UTC
Nobody interested in getting this done?
Comment 336 Katrin Fischer 2021-06-04 09:13:10 UTC
(In reply to Jonathan Druart from comment #335)
> Nobody interested in getting this done?

Martin and I are, but more hands would be useful.