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!

This clearly doesn't apply in master. Will do a new patch someday.

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

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

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". :)

Oh, and don't added CGI::checkbox! HTML code should be in the template, so it is easier to translate.

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?

(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

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

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>

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

Created attachment 45837 [details] [review]
Bug 8643: (follow-up) atomic update

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

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?

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.

Hm, the feature is not working (see bug 2280) - but would be better to fix or remove in a separate bug.

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

Created attachment 60555 [details] [review]
Bug 8643: (follow-up) atomic update

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

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.

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!

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.

Hi Philippe, I think you attached the wrong patch!

Created attachment 65149 [details] [review]
Bug 8643 - Added hint for important fields and implemented it for item fields

Sorry for this.

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?

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

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).

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?

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");

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?

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.

(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 :)

Sorry, does not apply on current master...

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>

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>

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>

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>

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>

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>

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.

Rebased on current master. Should apply cleanly now.

When you rebase you can usually set back to the status it had before. In this case it was signed off.

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.

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 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.

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?

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 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 ...");
}

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 )

The patch has been fixed and can be applied on master.

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 )

Rebased on current master.

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

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?

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 )

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 )

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 )

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.

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

(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!

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.

(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.

(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

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.

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.

(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.

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>

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>

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

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>

Nice work everyone!

Pushed to master for 20.05

Created attachment 98712 [details] [review]
Bug 8643: Fix IsMarcStructureInternal tests

Nice work everyone!

Pushed to master for 20.05

featurehancement not pushed to 19.11.x