Bug 8643 - Add ability to mark some MARC tags and subfields as important and alert on saving the record if they are found to be empty
Summary: Add ability to mark some MARC tags and subfields as important and alert on sa...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Charles Farmer
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-14 19:16 UTC by Maxime Pelletier
Modified: 2021-06-14 21:28 UTC (History)
20 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This feature allows tags and subfields in bibliographic frameworks to be marked as important. The important attribute will trigger a confirmation message on saving the record, but will allow you to save the record without filling the fields.
Version(s) released in:
20.05.00


Attachments
proposed enhancement patch (28.99 KB, patch)
2012-08-14 19:17 UTC, Maxime Pelletier
Details | Diff | Splinter Review
patch with fixed updatedatabase (28.98 KB, patch)
2012-08-14 19:54 UTC, Maxime Pelletier
Details | Diff | Splinter Review
Bug 8643 - Add important constraint to marc subfields (32.73 KB, patch)
2014-08-08 19:12 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 8643 - Add important constraint to marc subfields (32.99 KB, patch)
2014-08-20 17:41 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 8643 - Add important constraint to marc subfields (31.19 KB, patch)
2015-06-17 12:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 8643 - Add important constraint to marc subfields (31.22 KB, patch)
2015-06-17 12:49 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug8643 - Removed the use of CGI::checkbox for "Important" subfield and duplicate authorized tag (4.66 KB, patch)
2015-07-31 12:15 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug8643 - Removed the use of CGI::checkbox for "Important" subfield and duplicate authorized tag (4.66 KB, patch)
2015-07-31 12:27 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Did I just make everyone happy =D? (7.66 KB, patch)
2015-07-31 12:51 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug8643 - Removed all uses of CGI::Checkbox in marc_subfields_structure.pl (7.66 KB, patch)
2015-07-31 12:52 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8643 - Add important constraint to marc subfields (31.33 KB, patch)
2015-12-18 14:52 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8643: Removed the use of CGI::checkbox for "Important" subfield and duplicate authorized tag (4.75 KB, patch)
2015-12-18 14:52 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8643: Removed all uses of CGI::Checkbox in marc_subfields_structure.pl (7.77 KB, patch)
2015-12-18 14:52 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 8643: (follow-up) atomic update (2.43 KB, patch)
2015-12-18 14:53 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 8643 - Add important constraint to marc subfields (30.27 KB, patch)
2017-02-22 13:32 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 8643: (follow-up) atomic update (1.45 KB, patch)
2017-02-22 13:34 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 17639 - Remove white filling inside of Koha logo (33.75 KB, patch)
2017-07-20 13:38 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 8643 - Added hint for important fields and implemented it for item fields (6.49 KB, patch)
2017-07-20 14:48 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 8643 - Add important constraint to marc subfields (30.26 KB, patch)
2017-08-08 17:26 UTC, Philippe Audet-Fortin
Details | Diff | Splinter Review
Bug 8643: (follow-up) atomic update (1.45 KB, patch)
2018-02-23 20:04 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643 - Added hint for important fields and implemented it for item fields (6.49 KB, patch)
2018-02-23 20:04 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643 - Add important constraint to marc subfields (28.79 KB, patch)
2018-02-23 20:04 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643 - QA Follow-up (2.91 KB, patch)
2018-02-23 20:04 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643 - Paler yellow and better column order (3.74 KB, patch)
2018-04-03 21:02 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643: Add hint for important fields and implemented it for item fields (6.51 KB, patch)
2018-04-07 08:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (28.83 KB, patch)
2018-04-07 08:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8643: (QA follow-up) Fix alert, clean up patch, improve db update (2.97 KB, patch)
2018-04-07 08:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8643: (QA follow-up) Use paler yellow and better column order (3.78 KB, patch)
2018-04-07 08:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8643: (QA follow-up) Move input field up below mandatory (2.28 KB, patch)
2018-04-07 08:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8643: (follow-up) Atomic update (1.51 KB, patch)
2018-04-07 08:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8643: Add hint for important fields and implemented it for item fields (6.56 KB, patch)
2018-04-07 08:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (28.89 KB, patch)
2018-04-07 08:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8643: (QA follow-up) Fix alert, clean up patch, improve db update (3.03 KB, patch)
2018-04-07 08:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8643: (QA follow-up) Use paler yellow and better column order (3.84 KB, patch)
2018-04-07 08:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8643: (follow-up) Move input field up below mandatory (2.27 KB, patch)
2018-04-07 08:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 8643: (follow-up) Atomic update (1.51 KB, patch)
2018-04-16 20:15 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643: Add hint for important fields and implemented it for item fields (6.54 KB, patch)
2018-04-16 20:15 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (29.84 KB, patch)
2018-04-16 20:15 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643: (QA follow-up) Fix alert, clean up patch, improve db update (3.01 KB, patch)
2018-04-16 20:15 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643: (QA follow-up) Use paler yellow and better column order (3.92 KB, patch)
2018-04-16 20:16 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643: (QA follow-up) Use paler yellow and better column order (3.92 KB, patch)
2018-04-16 20:17 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643: (follow-up) Move input field up below mandatory (1.49 KB, patch)
2018-04-16 20:17 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (38.32 KB, patch)
2018-10-23 19:31 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (36.90 KB, patch)
2019-03-04 13:41 UTC, Pierre-Marc Thibault
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (133.85 KB, patch)
2019-07-15 16:12 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (38.15 KB, patch)
2019-08-08 15:01 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (37.27 KB, patch)
2019-08-20 15:11 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (38.27 KB, patch)
2019-12-24 17:53 UTC, Arthur Bousquet
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (38.38 KB, patch)
2019-12-25 14:35 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (38.36 KB, patch)
2020-01-13 16:44 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 8643: code cleanup (12.71 KB, patch)
2020-01-13 16:44 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (38.52 KB, patch)
2020-02-08 05:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 8643: code cleanup (12.78 KB, patch)
2020-02-08 05:19 UTC, David Nind
Details | Diff | Splinter Review
Bug 8643: Add important constraint to marc subfields (37.75 KB, patch)
2020-02-11 12:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8643: code cleanup (12.85 KB, patch)
2020-02-11 12:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 8643: Fix IsMarcStructureInternal tests (1.96 KB, patch)
2020-02-11 15:44 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 Maxime Pelletier 2012-08-14 19:16:21 UTC
Add an "Important" field to the marc structure pages. It works the same way as "Mandatory", but it shows a non blocking alert if the field is not filled.

I hope it can be useful to someone.
You can test by checking the new Important checkbox for a field or a subfield and then testing that you have a warning saying that the field or it's subfield should be filled.
Comment 1 Maxime Pelletier 2012-08-14 19:17:12 UTC Comment hidden (obsolete)
Comment 2 Jared Camins-Esakov 2012-08-14 19:39:46 UTC
Unfortunately the database update does not work. I got the following error message:

DBD::mysql::db do failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'ALTER TABLE `marc_tag_structure` ADD COLUMN `important` TINYINT(4) NOT NULL DEFA' at line 2 at installer/data/mysql/updatedatabase.pl line 5640.
Upgrade to XXX done (Add mandatory to marc frameworks)

Also, please do not use backticks in the updatedatabase.pl script. Although it is a MySQL-only file, Marc Balmer is in the process of making it database-agnostic, so that it will work with Postgres as well.

I am very excited to use this feature!
Comment 3 Maxime Pelletier 2012-08-14 19:54:02 UTC Comment hidden (obsolete)
Comment 4 Maxime Pelletier 2013-03-13 16:42:14 UTC
This clearly doesn't apply in master. Will do a new patch someday.
Comment 5 simith.doliveira 2014-08-08 19:12:01 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2014-08-13 19:17:41 UTC
Applying: Bug 8643
/home/oleonard/kohaclone/.git/rebase-apply/patch:388: trailing whitespace.
                    <p>	Tab:[% subfield.tab %] | $[% subfield.tagsubfield %]                        
warning: 1 line adds whitespace errors.
Using index info to reconstruct a base tree...
<stdin>:388: trailing whitespace.
                    <p>	Tab:[% subfield.tab %] | $[% subfield.tagsubfield %]                        
warning: 1 line applied after fixing whitespace errors.
Falling back to patching base and 3-way merge...
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 koha-tmpl/intranet-tmpl/prog/en/modules/admin/marctagstructure.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/marctagstructure.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc_subfields_structure.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc_subfields_structure.tt
Auto-merging installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging cataloguing/addbiblio.pl
Auto-merging admin/marctagstructure.pl
CONFLICT (content): Merge conflict in admin/marctagstructure.pl
Auto-merging admin/marc_subfields_structure.pl
Auto-merging C4/Biblio.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8643
Comment 7 simith.doliveira 2014-08-20 17:41:39 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2015-02-19 19:46:48 UTC
Applying: Bug 8643 - Add important constraint to marc subfields
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/marctagstructure.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc_subfields_structure.tt
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Auto-merging cataloguing/addbiblio.pl
Auto-merging admin/marctagstructure.pl
Auto-merging admin/marc_subfields_structure.pl
Auto-merging C4/Biblio.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 8643 - Add important constraint to marc subfields
Comment 9 Mark Tompsett 2015-06-17 12:33:35 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2015-06-17 12:49:32 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2015-06-17 12:56:51 UTC
Sorry, the Authorized Dropdown is HASH now. I'll attempt to fix later, unless simith.doliveira@inlibro.com wants to post a rebase. That would make everyone happy. I dislike "Patch doesn't apply". :)
Comment 12 Mark Tompsett 2015-06-17 12:59:45 UTC
Oh, and don't added CGI::checkbox! HTML code should be in the template, so it is easier to translate.
Comment 13 Rémi Mayrand-Provencher 2015-07-30 15:41:21 UTC
The patch applied properly for me, so I do not think that it needs a rebase.

Also, CGI::checkbox is used for other subfields too. I don't think the aim of this patch is to remove CGI::checkbox from marc_subfields_structure.pl.

Could you be clearer on what are the changes you would like to see in this patch?
Comment 14 Tomás Cohen Arazi 2015-07-30 16:10:49 UTC
(In reply to Rémi Mayrand-Provencher from comment #13)
> Also, CGI::checkbox is used for other subfields too. I don't think the aim
> of this patch is to remove CGI::checkbox from marc_subfields_structure.pl.

I discourage patches adding HTML building stuff on the .pl files. It is true that you don't need to remove the existing ones, but please don't add new :-D

A followup removing the already present would make people happy too :-D
Comment 15 Rémi Mayrand-Provencher 2015-07-31 12:15:11 UTC Comment hidden (obsolete)
Comment 16 Rémi Mayrand-Provencher 2015-07-31 12:27:17 UTC Comment hidden (obsolete)
Comment 17 Rémi Mayrand-Provencher 2015-07-31 12:51:19 UTC Comment hidden (obsolete)
Comment 18 Rémi Mayrand-Provencher 2015-07-31 12:52:07 UTC Comment hidden (obsolete)
Comment 19 Bernardo Gonzalez Kriegel 2015-12-18 14:52:16 UTC
Created attachment 45834 [details] [review]
[SIGNED-OFF] Bug 8643 - Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

I Apply the patch
II Run updatedatabase.pl

0) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
1) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
2) Edit a record to clear the field 100
3) Save the record.
4) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

sponsored by the CCSR ( http://www.ccsr.qc.ca )

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described. Fixed small conflict
Comment 20 Bernardo Gonzalez Kriegel 2015-12-18 14:52:25 UTC
Created attachment 45835 [details] [review]
[SIGNED-OFF] Bug 8643: Removed the use of CGI::checkbox for "Important" subfield and duplicate authorized tag

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 21 Bernardo Gonzalez Kriegel 2015-12-18 14:52:33 UTC
Created attachment 45836 [details] [review]
[SIGNED-OFF] Bug 8643: Removed all uses of CGI::Checkbox in marc_subfields_structure.pl

To test this patch :

1) Apply all patches
2) Run ./installer/data/mysql/updatedatabase.pl
3) Go to More > Administration > MARC bibliographic framework > default framework ("MARC structure")
4) Search for tag 100 > Click subfields
5) Edit the "a" field and check the "reapeatable","mandatory","important" and "Is a URL" checkboxes
6) Click save an validate that the constraints for field "a" were modified properly.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

All checkboxes work, verified on db
No errors
Comment 22 Bernardo Gonzalez Kriegel 2015-12-18 14:53:04 UTC
Created attachment 45837 [details] [review]
Bug 8643: (follow-up) atomic update

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 23 Jonathan Druart 2015-12-28 10:41:19 UTC
Comment on attachment 45835 [details] [review]
[SIGNED-OFF] Bug 8643: Removed the use of CGI::checkbox for "Important" subfield and duplicate authorized tag

Review of attachment 45835 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/marctagstructure.tt
@@ -111,4 @@
>              <input type="checkbox" name="important" id="important" value="1" />
>          [% END %]
>      </li>
> -    <li><label for="authorised_value">Authorized value: </label>[% authorised_value %] (if you select a value here, the indicators will be limited to the authorized value list)</li>

Why do you remove this line?
Comment 24 Jonathan Druart 2015-12-28 10:44:43 UTC
QA comments:
1/ You have to deal with the items' fields too. I can mark a 995/952 field as important, but the behavior is not implemented when adding items.
2/ If a field is marked as important and mandatory, there are 2 stars (1 red and 1 black), is it the expected behavior? Maybe the importance of a field can be ignored if it is also marked as mandatory.
Comment 25 Katrin Fischer 2015-12-29 22:41:42 UTC
Hm, the feature is not working (see bug 2280) - but would be better to fix or remove in a separate bug.
Comment 27 Mehdi Hamidi 2017-02-22 13:32:43 UTC
Created attachment 60554 [details] [review]
Bug 8643 - Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

I Apply the patch
II Run updatedatabase.pl

0) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
1) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
2) Edit a record to clear the field 100
3) Save the record.
4) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

sponsored by the CCSR ( http://www.ccsr.qc.ca )

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described. Fixed small conflict
Comment 28 Mehdi Hamidi 2017-02-22 13:34:51 UTC
Created attachment 60555 [details] [review]
Bug 8643: (follow-up) atomic update

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 29 Mehdi Hamidi 2017-02-22 13:48:37 UTC
Hello everyone,
I rebased the patch and removed two patches that were not necessary.
I also addressed some of the the comments :

(In reply to Jonathan Druart from comment #23)

> ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/marctagstructure.tt
> @@ -111,4 @@
> >              <input type="checkbox" name="important" id="important" value="1" />
> >          [% END %]
> >      </li>
> > -    <li><label for="authorised_value">Authorized value: </label>[% authorised_value %] (if you select a value here, the indicators will be limited to the authorized value list)</li>
> 
> Why do you remove this line?

I put back the line, as for why it was removed, I'm not sure.

(In reply to Jonathan Druart from comment #24)
> QA comments:
> 1/ You have to deal with the items' fields too. I can mark a 995/952 field
> as important, but the behavior is not implemented when adding items.
> 2/ If a field is marked as important and mandatory, there are 2 stars (1 red
> and 1 black), is it the expected behavior? Maybe the importance of a field
> can be ignored if it is also marked as mandatory.

I did not fix the first point as it requires more development, but for your second point I fixed it and it won't show important if the field is also mandatory.
Comment 30 Katrin Fischer 2017-05-01 22:10:12 UTC
Tested this a bit and it appears to work ok. 

Some things found:
1) Doesn't work on item fields, would be nice to see it implemented there as well. Maybe for now we could add a hint to the setting to avoid confusion -"Currently not implemented for items"?
2) I marked 001 as important but it didn't trigger the message. I had to mark the subfield @ important to make it work. Would be nice if this could be fixed.
3) It would be nice to have some visual hint on the important fields and subfields, atm it seems only visible when saving a record.
4) At the moment it will first tell you about the mandatory missing and then after fixing those about the important ones. It would be nicer if the checks were done altogether, to avoid the repeated action.

Marking failed QA for 1) and 2), but hopefully those should not be hard to fix!
Comment 31 Philippe Audet-Fortin 2017-07-20 13:38:26 UTC Comment hidden (obsolete)
Comment 32 Philippe Audet-Fortin 2017-07-20 13:47:36 UTC
Hi Katrin,

I implemented the important constraint to item fields and added the hint.
However, for the point number 2, I wasn't sure what to do because it has the same behaviour has the mandatory constraint. Maybe an other ticket should be open for this.
Comment 33 Katrin Fischer 2017-07-20 13:48:38 UTC
Hi Philippe, I think you attached the wrong patch!
Comment 34 Philippe Audet-Fortin 2017-07-20 14:48:27 UTC
Created attachment 65149 [details] [review]
Bug 8643 - Added hint for important fields and implemented it for item fields
Comment 35 Philippe Audet-Fortin 2017-07-20 14:50:00 UTC
Sorry for this.
Comment 36 Katrin Fischer 2017-08-08 15:07:51 UTC
Hm, there are changes in kohastructure.sql to the table serialitems causing a conflict when applying the patch:

Bug 8643 - Add important constraint to marc subfields

60554 - Bug 8643 - Add important constraint to marc subfields
60555 - Bug 8643: (follow-up) atomic update
65149 - Bug 8643 - Added hint for important fields and implemented it for item fields

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 8643 - Add important constraint to marc subfields
.git/rebase-apply/patch:22: trailing whitespace.
        FROM marc_tag_structure 
.git/rebase-apply/patch:23: trailing whitespace.
        WHERE frameworkcode=? 
.git/rebase-apply/patch:42: trailing whitespace.
         FROM   marc_subfield_structure 
.git/rebase-apply/patch:43: trailing whitespace.
         WHERE  frameworkcode=? 
.git/rebase-apply/patch:194: trailing whitespace.
                FROM marc_tag_structure 
warning: squelched 10 whitespace errors
warning: 15 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	C4/Biblio.pm
M	admin/marc_subfields_structure.pl
M	cataloguing/addbiblio.pl
M	installer/data/mysql/kohastructure.sql
.git/rebase-apply/patch:22: trailing whitespace.
        FROM marc_tag_structure 
.git/rebase-apply/patch:23: trailing whitespace.
        WHERE frameworkcode=? 
.git/rebase-apply/patch:42: trailing whitespace.
         FROM   marc_subfield_structure 
.git/rebase-apply/patch:43: trailing whitespace.
         WHERE  frameworkcode=? 
.git/rebase-apply/patch:194: trailing whitespace.
                FROM marc_tag_structure 
warning: squelched 10 whitespace errors
warning: 15 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Auto-merging cataloguing/addbiblio.pl
Auto-merging admin/marc_subfields_structure.pl
Auto-merging C4/Biblio.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 8643 - Add important constraint to marc subfields
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-8643---Add-important-constraint-to-marc-subfie-FP83NQ.patch

Are those changes related to the patch?
Comment 37 Philippe Audet-Fortin 2017-08-08 17:26:27 UTC
Created attachment 65682 [details] [review]
Bug 8643 - Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

I Apply the patch
II Run updatedatabase.pl

0) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
1) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
2) Edit a record to clear the field 100
3) Save the record.
4) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

sponsored by the CCSR ( http://www.ccsr.qc.ca )

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described. Fixed small conflict
Comment 38 Katrin Fischer 2017-08-08 21:35:55 UTC
There are some things odd about this patch set:

1) In the database update the new columns are added in different spots:

+ALTER TABLE marc_subfield_structure ADD COLUMN important TINYINT(4) NOT NULL DEFAULT 0  AFTER mandatory;
+ALTER TABLE marc_tag_structure ADD COLUMN important TINYINT(4) NOT NULL DEFAULT 0  AFTER frameworkcode;

Why not have marc_tag_structure.important after mandatory too? This is mostly cosmetic, but I was wondering about the inconsistency.

2) There is a change on the serialitems table in kohastructure.sql, that needs to be removed from this patch set:

@@ -2456,7 +2458,7 @@ DROP TABLE IF EXISTS `serialitems`;
 CREATE TABLE `serialitems` (
     `itemnumber` int(11) NOT NULL,
     `serialid` int(11) NOT NULL,
-    PRIMARY KEY (`itemnumber`),
+    UNIQUE KEY `serialitemsidx` (`itemnumber`),

3) In marctagstructure.tt the following line is added, I think also accidentally?

+    <li><label for="authorised_value">Authorized value: </label>[% authorised_value %] (if you select a value here, the indicators will be limited to the authorized value list)</li>

Also, when testing I noticed that the important field shows up below the authorised value. I think it should be right below mandatory here too, to be consistent with the subfield configuration.

4) The number of the tab is missing from the alert:

A few important fields are not filled:

	* tag 245 subfield b Zusatz zum Titel in tab 

Are you sure you want to save?

5) The hightlighting seems to only work for subfields, not for fields, is that correct?

I think it would be nice if the important fields highlighted in a different color to the mandatory ones (totally not blocker).
Comment 39 Charles Farmer 2018-02-23 20:04:42 UTC Comment hidden (obsolete)
Comment 40 Charles Farmer 2018-02-23 20:04:47 UTC Comment hidden (obsolete)
Comment 41 Charles Farmer 2018-02-23 20:04:53 UTC Comment hidden (obsolete)
Comment 42 Charles Farmer 2018-02-23 20:04:58 UTC Comment hidden (obsolete)
Comment 43 Charles Farmer 2018-02-23 20:28:14 UTC
Hi Katrin,

Thank you once again for commenting!

I've tackled points 1 to 4 of comment#38. It'd be great if this version could once again be tested.

For point #5, from what I see in the code, you're correct: highlighting is only applied to subfields.

On the issue of changing the CSS class to highlight important fields differently from mandatory fields, it sounds like an idea I can get behind. Do you have a color suggestion for those fields? Clearer yellow?
Comment 44 Katrin Fischer 2018-03-31 21:37:05 UTC
Hi Charles,

a lighter yellow would work for me.

I retested, but I can't get the message to show up now, neither for an important field or subfield. Can you please check?

Tiny thing: marc_tag_structure - maybe move the new field after mandatory?

+--     $dbh->do("ALTER TABLE marc_tag_structure ADD COLUMN important TINYINT(4) NOT NULL DEFAULT 0  AFTER frameworkcode");
Comment 45 Charles Farmer 2018-04-03 21:02:19 UTC Comment hidden (obsolete)
Comment 46 Charles Farmer 2018-04-03 21:44:27 UTC
Tried again with a fresh database: I could see the 'important' field dialog box ONLY after correcting every mandatory field warnings.

I don't have much cues to give otherwise. It could either be a javascript problem, or a forgotten atomicupdate?
Comment 47 Katrin Fischer 2018-04-07 08:42:48 UTC Comment hidden (obsolete)
Comment 48 Katrin Fischer 2018-04-07 08:42:53 UTC Comment hidden (obsolete)
Comment 49 Katrin Fischer 2018-04-07 08:42:58 UTC Comment hidden (obsolete)
Comment 50 Katrin Fischer 2018-04-07 08:43:02 UTC Comment hidden (obsolete)
Comment 51 Katrin Fischer 2018-04-07 08:43:07 UTC Comment hidden (obsolete)
Comment 52 Katrin Fischer 2018-04-07 08:45:54 UTC Comment hidden (obsolete)
Comment 53 Katrin Fischer 2018-04-07 08:45:59 UTC Comment hidden (obsolete)
Comment 54 Katrin Fischer 2018-04-07 08:46:04 UTC Comment hidden (obsolete)
Comment 55 Katrin Fischer 2018-04-07 08:46:09 UTC Comment hidden (obsolete)
Comment 56 Katrin Fischer 2018-04-07 08:46:13 UTC Comment hidden (obsolete)
Comment 57 Katrin Fischer 2018-04-07 08:46:18 UTC Comment hidden (obsolete)
Comment 58 Katrin Fischer 2018-04-07 08:49:55 UTC
One thing for next time :) Please take care of always writing good commit messages and the format of the subject line spefically:
https://wiki.koha-community.org/wiki/Commit_messages

These patches have a lot of history, being started in 2012. It works alright now in my eyes.
Comment 59 Charles Farmer 2018-04-09 13:57:49 UTC
(In reply to Katrin Fischer from comment #58)
> One thing for next time :) Please take care of always writing good commit
> messages and the format of the subject line spefically:
> https://wiki.koha-community.org/wiki/Commit_messages
> 
> These patches have a lot of history, being started in 2012. It works alright
> now in my eyes.

You're right, I'll keep it in mind next time I rebase/submit a bug :)
Comment 60 Josef Moravec 2018-04-16 14:40:48 UTC
Sorry, does not apply on current master...
Comment 61 Charles Farmer 2018-04-16 20:15:39 UTC
Created attachment 74267 [details] [review]
Bug 8643: (follow-up) Atomic update

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 62 Charles Farmer 2018-04-16 20:15:44 UTC
Created attachment 74268 [details] [review]
Bug 8643: Add hint for important fields and implemented it for item fields

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 63 Charles Farmer 2018-04-16 20:15:50 UTC
Created attachment 74269 [details] [review]
Bug 8643: Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

I Apply the patch
II Run updatedatabase.pl

0) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
1) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
2) Edit a record to clear the field 100
3) Save the record.
4) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

sponsored by the CCSR ( http://www.ccsr.qc.ca )

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Work as described. Fixed small conflict

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 64 Charles Farmer 2018-04-16 20:15:55 UTC
Created attachment 74270 [details] [review]
Bug 8643: (QA follow-up) Fix alert, clean up patch, improve db update

* Field 'important' now added after 'mandatory' in table marc_tag_structure
* Removed modification on table serialitems in kohastructure.sql
* Removed bogus line in marctagstructure.tt
* Fixed the missing tag number in the alert message

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 65 Charles Farmer 2018-04-16 20:16:01 UTC
Created attachment 74271 [details] [review]
Bug 8643: (QA follow-up) Use paler yellow and better column order

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 66 Charles Farmer 2018-04-16 20:17:26 UTC
Created attachment 74272 [details] [review]
Bug 8643: (QA follow-up) Use paler yellow and better column order

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 67 Charles Farmer 2018-04-16 20:17:33 UTC
Created attachment 74273 [details] [review]
Bug 8643: (follow-up) Move input field up below mandatory

Changes sequence of input fields to be more logical by
grouping repeatable, mandatory, important, like it's already
done for the subfield configuration.
Comment 68 Charles Farmer 2018-04-16 20:22:08 UTC
Rebased on current master. Should apply cleanly now.
Comment 69 Katrin Fischer 2018-04-30 15:16:39 UTC
When you rebase you can usually set back to the status it had before. In this case it was signed off.
Comment 70 Josef Moravec 2018-05-30 15:33:53 UTC
Just started QA and found some issues:

1) qa test tools report:
FAIL	admin/marctagstructure.pl
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 125)

2) Sometimes you use attribute "style" for css - should be done using class

koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
588    <span style="color:red" title="This field is mandatory">*</span>

koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
348[% ELSIF ( ite.important ) %] <span class="important" style="font-style: italic; margin-left: 

3) Marking whole control field as important does not work - there is no confirmation dialog when I left it empty. On the subfield level, it works

4) The tag edit page is a bit odd - the "Important" setting should be after "Mandatory". Authorized values setting is duplicated.
Comment 71 Josef Moravec 2018-05-30 15:41:22 UTC
Sent by accident before finished ;)

ad 2)
koha-tmpl/intranet-tmpl/prog/en/modules/admin/marctagstructure.tt
230    <p style="text-indent: 5em">Tab:[% subfield.tab %] | $[% subfield.tagsubfield %]

koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
791    <span style="color:black" title="This field is important">*</span>
Comment 72 Pierre-Marc Thibault 2018-10-23 17:00:17 UTC
Comment on attachment 74273 [details] [review]
Bug 8643: (follow-up) Move input field up below mandatory

This patch is set at obsolete since it contains no usable code and duplicated code.
Comment 73 Pierre-Marc Thibault 2018-10-23 19:31:07 UTC
Created attachment 81032 [details] [review]
Bug 8643: Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

1) Apply the patch
2) Run updatedatabase.pl
3) Regenerate CSS
4) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
5) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
6) Edit a record to clear the field 100 (subfields are all blank)
7) Save the record.
8) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?
Comment 74 Pierre-Marc Thibault 2018-10-23 19:44:36 UTC
All the old patches have been merged to make tests easier. The new patch is rebased and solves the last problems mentioned. The tab char is gone, the style attributes too and the inputs order is now correct. It seems there is no problem with the alert message, it will be to confirme with other tests.
Comment 75 Josef Moravec 2019-02-04 07:48:20 UTC
Comment on attachment 81032 [details] [review]
Bug 8643: Add important constraint to marc subfields

Review of attachment 81032 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/atomicupdate/bug-8643-add_important_constraint.sql
@@ +6,5 @@
> +--     $dbh->do("ALTER TABLE marc_subfield_structure ADD COLUMN important TINYINT(4) NOT NULL DEFAULT 0  AFTER mandatory");
> +--     $dbh->do("ALTER TABLE marc_tag_structure ADD COLUMN important TINYINT(4) NOT NULL DEFAULT 0  AFTER mandatory");
> +--     print "Upgrade to $DBversion done (Bug 8643 - Add important constraint to marc subfields)\n";
> +--     SetVersion($DBversion);
> +-- }

The database update should be idempotent, make it all in perl and use column_exists helper function, like this:

if( !column_exists( 'marc_subfield_structure', 'important' ) ) {
    $dbh->do("ALTER TABLE ... ADD COLUMN ...");
}
Comment 76 Pierre-Marc Thibault 2019-03-04 13:41:56 UTC
Created attachment 85982 [details] [review]
Bug 8643: Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

1) Apply the patch
2) Run updatedatabase.pl
3) Regenerate CSS
4) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
5) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
6) Edit a record to clear the field 100 (subfields are all blank)
7) Save the record.
8) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

Sponsored by the CCSR ( http://www.ccsr.qc.ca )
Comment 77 Pierre-Marc Thibault 2019-03-04 13:43:38 UTC
The patch has been fixed and can be applied on master.
Comment 78 Maryse Simard 2019-07-15 16:12:44 UTC
Created attachment 91537 [details] [review]
Bug 8643: Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

1) Apply the patch
2) Run updatedatabase.pl
3) Regenerate CSS
4) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
5) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
6) Edit a record to clear the field 100 (subfields are all blank)
7) Save the record.
8) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

Sponsored by the CCSR ( http://www.ccsr.qc.ca )
Comment 79 Maryse Simard 2019-07-15 16:14:49 UTC
Rebased on current master.
Comment 80 Alex Buckley 2019-07-26 02:00:11 UTC
Hi Maryse,

Thank you for your contribution. Unfortunately when testing this I am not getting a confirmation message when I set 100 tag as 'Important' and try to submit the Edit item with all 100 tag subfields empty. 

Similarly, I am not getting a confirmation message when I have set 100a as important and set try to submit the 'Edit item' form when 100a is blank. 

There does not appear to be JS errors in the console. 

Cheers,
Alex
Comment 81 Maryse Simard 2019-07-30 18:38:38 UTC
When I test myself, the message does appear. Though, it only appears once all the mandatory fields have been filled. Could it be what happened to you?
Comment 82 Maryse Simard 2019-08-08 15:01:46 UTC
Created attachment 92081 [details] [review]
Bug 8643: Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

1) Apply the patch
2) Run updatedatabase.pl
3) Regenerate CSS
4) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
5) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
6) Edit a record to clear the field 100 (subfields are all blank)
7) Save the record.
8) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

Sponsored by the CCSR ( http://www.ccsr.qc.ca )
Comment 83 Maryse Simard 2019-08-20 15:11:03 UTC
Created attachment 92384 [details] [review]
Bug 8643: Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

1) Apply the patch
2) Run updatedatabase.pl
3) Regenerate CSS
4) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
5) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
6) Edit a record to clear the field 100 (subfields are all blank)
7) Save the record.
8) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

Sponsored by the CCSR ( http://www.ccsr.qc.ca )
Comment 84 Arthur Bousquet 2019-12-24 17:53:40 UTC
Created attachment 96625 [details] [review]
Bug 8643: Add important constraint to marc subfields

I modified the MarcSubfieldStructure.pm file to match the new database shema (with the "important" column)

This fix permits to add an "Important" option to the marc structure pages.

Testing:

1) Apply the patch
2) Run updatedatabase.pl
3) Regenerate CSS
4) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
5) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
6) Edit a record to clear the field 100 (subfields are all blank)
7) Save the record.
8) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

Sponsored by the CCSR ( http://www.ccsr.qc.ca )
Comment 85 Bernardo Gonzalez Kriegel 2019-12-25 14:35:53 UTC
Created attachment 96627 [details] [review]
Bug 8643: Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

1) Apply the patch
2) Run updatedatabase.pl
3) Regenerate CSS
4) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
5) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
6) Edit a record to clear the field 100 (subfields are all blank)
7) Save the record.
8) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

Sponsored by the CCSR ( http://www.ccsr.qc.ca )

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested on update and new install, works as described.
No errors.
Comment 86 Jonathan Druart 2019-12-26 09:03:34 UTC
Does not work for me:

Can't use an undefined value as an ARRAY reference at /usr/lib/x86_64-linux-gnu/perl5/5.24/DBI.pm line 2088
        DBD::_::st::fetchall_arrayref('DBI::st=HASH(0x5570f6dbc278)', 'HASH(0x5570f6dbc440)', undef) called at /kohadevbox/koha/admin/marctagstructure.pl line 330
        CGI::Compile::ROOT::kohadevbox_koha_admin_marctagstructure_2epl::StringSearch(0, 'ACQ') called at /kohadevbox/koha/admin/marctagstructure.pl line 284
        eval {...} at /kohadevbox/koha/admin/marctagstructure.pl line 349
Comment 87 Jonathan Druart 2019-12-26 09:05:20 UTC
(In reply to Jonathan Druart from comment #86)
> Does not work for me:
> 
> Can't use an undefined value as an ARRAY reference at
> /usr/lib/x86_64-linux-gnu/perl5/5.24/DBI.pm line 2088
>         DBD::_::st::fetchall_arrayref('DBI::st=HASH(0x5570f6dbc278)',
> 'HASH(0x5570f6dbc440)', undef) called at
> /kohadevbox/koha/admin/marctagstructure.pl line 330
>        
> CGI::Compile::ROOT::kohadevbox_koha_admin_marctagstructure_2epl::
> StringSearch(0, 'ACQ') called at /kohadevbox/koha/admin/marctagstructure.pl
> line 284
>         eval {...} at /kohadevbox/koha/admin/marctagstructure.pl line 349

Sorry, I forgot the new DB column!
Comment 88 Jonathan Druart 2019-12-26 09:23:12 UTC
I am quite ok with this patch, just a couple of things:

1. Who is the original author of this patch? Bernardo? Regarding the history I do not think so.

2. Not blocker but, when a code is not nice, better refactore and improve it than duplicate it ;)
I am referring to the JS code to test if a mandatory field is filled, it would have been easier to make the function accepts an argument to check for mandatories or importants.
Comment 89 Bernardo Gonzalez Kriegel 2019-12-26 09:34:57 UTC
(In reply to Jonathan Druart from comment #88)
> 1. Who is the original author of this patch? Bernardo? Regarding the history
> I do not think so.
I just signed, ~2015 first time, and now.
Comment 90 Bernardo Gonzalez Kriegel 2019-12-26 13:51:46 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #89)
> (In reply to Jonathan Druart from comment #88)
> > 1. Who is the original author of this patch? Bernardo? Regarding the history
> > I do not think so.
> I just signed, ~2015 first time, and now.

All these people, from InLibro, seems to have worked on this bug

Maxime Pelletier 
simith.doliveira@inlibro
Rémi Mayrand-Provencher 
Mehdi Hamidi 
Philippe Audet-Fortin 
Charles Farmer 
Pierre-Marc Thibault
Maryse Simard 
Arthur Bousquet
Comment 91 Maryse Simard 2020-01-13 16:44:42 UTC
Created attachment 97325 [details] [review]
Bug 8643: Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

1) Apply the patch
2) Run updatedatabase.pl
3) Regenerate CSS
4) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
5) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
6) Edit a record to clear the field 100 (subfields are all blank)
7) Save the record.
8) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

Sponsored by the CCSR ( http://www.ccsr.qc.ca )

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested on update and new install, works as described.
No errors.
Comment 92 Maryse Simard 2020-01-13 16:44:47 UTC
Created attachment 97326 [details] [review]
Bug 8643: code cleanup

Merges "AreMandatoriesNotOk" and new "AreImportantsNotOk" into one
function "AreFieldsNotOk". It can search for mandatory or important
fields depending on the value of it's boolean "mandatory"
parameter.

To test:
This patch should not change current behavior.
Follow the test plan from previous patch and check that saving is
still prevented for unfilled mandatory fields.
Comment 93 Maryse Simard 2020-01-13 16:45:16 UTC
(In reply to Jonathan Druart from comment #88)
> 1. Who is the original author of this patch? Bernardo? Regarding the history
> I do not think so.

The author was lost when the patches were squashed. I changed it back to simith.

> 2. Not blocker but, when a code is not nice, better refactore and improve it
> than duplicate it ;)
> I am referring to the JS code to test if a mandatory field is filled, it
> would have been easier to make the function accepts an argument to check for
> mandatories or importants.

It's true this duplication is not very nice. I added a patch to try and reuse the current function instead of duplicating it. I followed your suggestion and used an argument to indicate whether we want a mandatory or important check.

I've also set the bug back to Needs Signoff because of this change.
Comment 94 David Nind 2020-02-08 05:19:34 UTC
Created attachment 98607 [details] [review]
Bug 8643: Add important constraint to marc subfields

This enhancement adds an "Important" option to the marc structure pages.

Testing:

1) Apply the patch
2) Update the database
   (perl installer/data/mysql/updatedatabase.pl)
3) Regenerate the staff interface CSS
   (https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)
4) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure (Default framework) » Edit )
5) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default framework) » Subfield » tag a)
6) Edit a record to clear the field 100 (subfields are all blank)
7) Save the record
8) Confirm that this message is displayed:

    A few important fields are not filled:

      * tag 100 subfield a Personal name in tab 1
      * Field 100 is important, at least one of its subfields must be filled.

    Are you sure you want to save?

Sponsored by the CCSR ( http://www.ccsr.qc.ca )

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested on update and new install, works as described.
No errors.

Signed-off-by: David Nind <david@davidnind.com>
Comment 95 David Nind 2020-02-08 05:19:39 UTC
Created attachment 98608 [details] [review]
Bug 8643: code cleanup

Merges "AreMandatoriesNotOk" and new "AreImportantsNotOk" into one
function "AreFieldsNotOk". It can search for mandatory or important
fields depending on the value of it's boolean "mandatory"
parameter.

To test:
This patch should not change current behavior.
Follow the test plan from previous patch and check that saving is
still prevented for unfilled mandatory fields.

Signed-off-by: David Nind <david@davidnind.com>
Comment 96 Jonathan Druart 2020-02-11 12:19:31 UTC
Created attachment 98702 [details] [review]
Bug 8643: Add important constraint to marc subfields

This fix permits to add an "Important" option to the marc structure pages.

Testing:

1) Apply the patch
2) Run updatedatabase.pl
3) Regenerate CSS
4) Define 100 as an "important" field ( Administration » MARC bibliographic framework » MARC structure ( Default Frameword) » Edit )
5) Define 100$a as an "important" subfield (Administration » MARC bibliographic framework » MARC structure (Default Frameword) » Subfield » Onglet a)
6) Edit a record to clear the field 100 (subfields are all blank)
7) Save the record.
8) Validate the following message:

A few important fields are not filled:

    * tag 100 subfield a Nom de personne in tab
    * Field 100 is important, at least one of its subfields should be filled.

Are you sure you want to save?

Sponsored by the CCSR ( http://www.ccsr.qc.ca )

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested on update and new install, works as described.
No errors.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Amended-patch: Remove the schema change
Comment 97 Jonathan Druart 2020-02-11 12:19:37 UTC
Created attachment 98703 [details] [review]
Bug 8643: code cleanup

Merges "AreMandatoriesNotOk" and new "AreImportantsNotOk" into one
function "AreFieldsNotOk". It can search for mandatory or important
fields depending on the value of it's boolean "mandatory"
parameter.

To test:
This patch should not change current behavior.
Follow the test plan from previous patch and check that saving is
still prevented for unfilled mandatory fields.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 98 Martin Renvoize 2020-02-11 14:38:51 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 99 Jonathan Druart 2020-02-11 15:44:54 UTC
Created attachment 98712 [details] [review]
Bug 8643: Fix IsMarcStructureInternal tests
Comment 100 Martin Renvoize 2020-02-17 11:31:51 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 101 Joy Nelson 2020-03-05 00:50:08 UTC
featurehancement not pushed to 19.11.x