The 'hidden' value is a serialization of several possible visualizatio combinations (http://manual.koha-community.org/3.8/en/catadmin.html#marcbibframeworks). This way of handling is troublesome for: - The code (difficult to read, difficult to find bugs) - The users (instead of simple checkboxes they need to read the docs, looking for a weird number. Several bugs about this 'hidden' option are related to some math on the 'hidden' value. I propose to change that field for 'vis_opac' |' vis_intranet' | 'vis_editor' | 'collapsed', all of them YesNo values. This will require a lot of rewrite in code like this: next if ( ($sf_def->{hidden}||0) > 0 ); or this: next if ( $tagslib->{ $fields[$x_i]->tag() }->{'@'}->{hidden} =~ /-7|-4|-3|-2|2|3|5|8/);
I'd like to add that I wrote some serialization/de-serialization javascript functions in the meantime that could fit 3.12, but it depends on the RM right now.
Created attachment 16876 [details] [review] Bug 9894 - (3.12) de-serialize the 'hidden' value for subfields in frameworks This patch aims to provide a non-intrusive way (with respect to Koha's code, so it could have got into 3.12) for the users to easily choose the visibility settings for each subfield. This patch is also known to have arrived late for 3.12. I've been struggling with other stuff at work, but I just wanted to put this somewhere just in case it can be reused or something else. The patch adds two JavaScript functions that deal with (de)serialization of the 'hidden' value for subfields. One each way. Listeners are added to the checkboxes so the (actually, heh) 'hidden' value gets correctly updated on each checkbox change. There's a costly function to update all checkboxes on every tab to match the 'hidden' value on page load. To test: - open the docs to have at hand the list of codes/visibility options combinations and try changing several subfields from a single tagfield. Save, and reopen the "Edit subfields" page. - I had a MySQL shell opened to repeatedly run this query (i tested against field 886 on the default framework): > SELECT tagfield,tagsubfield,hidden FROM marc_subfield_structure WHERE frameworkcode='' AND tagfield=886; - Compare what was saved on the DB with the docs for that visibility selection. Regards To+ PS: I promise I'll work on the proper patch for 3.14, which would involve DB structure changes and (more than I expected) rewriten code.
Created attachment 16877 [details] [review] Bug 9894 - (3.12) de-serialize the 'hidden' value for subfields in frameworks This patch aims to provide a non-intrusive way (with respect to Koha's code, so it could have got into 3.12) for the users to easily choose the visibility settings for each subfield. This patch is also known to have arrived late for 3.12. I've been struggling with other stuff at work, but I just wanted to put this somewhere just in case it can be reused or something else. The patch adds two JavaScript functions that deal with (de)serialization of the 'hidden' value for subfields. One each way. Listeners are added to the checkboxes so the (actually, heh) 'hidden' value gets correctly updated on each checkbox change. There's a costly function to update all checkboxes on every tab to match the 'hidden' value on page load. To test: - open the docs to have at hand the list of codes/visibility options combinations and try changing several subfields from a single tagfield. Save, and reopen the "Edit subfields" page. - I had a MySQL shell opened to repeatedly run this query (i tested against field 886 on the default framework): > SELECT tagfield,tagsubfield,hidden FROM marc_subfield_structure WHERE frameworkcode='' AND tagfield=886; - Compare what was saved on the DB with the docs for that visibility selection. Regards To+ PS: I promise I'll work on the proper patch for 3.14, which would involve DB structure changes and (more than I expected) rewriten code. Sponsored-by: Universidad Nacional de Córdoba
The current patch is a reasonable bugfix and therefore could be included in Koha 3.12 if it gets tested and so forth before the string freeze. A broader overhaul for is also needed, but for 3.14 not 3.12.
Created attachment 16878 [details] [review] Bug 9894 - (3.12) de-serialize the 'hidden' value for subfields in frameworks This patch aims to provide a non-intrusive way (with respect to Koha's code, so it could have got into 3.12) for the users to easily choose the visibility settings for each subfield. This patch is also known to have arrived late for 3.12. I've been struggling with other stuff at work, but I just wanted to put this somewhere just in case it can be reused or something else. The patch adds two JavaScript functions that deal with (de)serialization of the 'hidden' value for subfields. One each way. Listeners are added to the checkboxes so the (actually, heh) 'hidden' value gets correctly updated on each checkbox change. There's a costly function to update all checkboxes on every tab to match the 'hidden' value on page load. To test: - open the docs to have at hand the list of codes/visibility options combinations and try changing several subfields from a single tagfield. Save, and reopen the "Edit subfields" page. - I had a MySQL shell opened to repeatedly run this query (i tested against field 886 on the default framework): > SELECT tagfield,tagsubfield,hidden FROM marc_subfield_structure WHERE frameworkcode='' AND tagfield=886; - Compare what was saved on the DB with the docs for that visibility selection. Regards To+ PS: I promise I'll work on the proper patch for 3.14, which would involve DB structure changes and (more than I expected) rewriten code. Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Liz Rea <liz@catalyst.net.nz> I promise to hug you the next time we meet. This works, and is a reasonable work around for a very longstanding awkward interface.
QA comment: Looks good. But I have 2 questions: 1/ What about the -8 value? 2/ What about "bad values"? Since it is a free field, the value can be anything. Your patch set "5" for these cases, is it intended? Maybe it could be great to update the help page. Please clarify 1 and 2.
I've been looking through the documentation, I can't even see what the "Flag" value does - maybe someone out there knows? I have certainly never seen it used in practice anywhere. Regarding number 2, as the help states - there are only 17 implemented view options - putting anything else but one of those (which this patch doesn't allow) is invalid input anyway, and nothing will happen. The original feature was poorly designed, this is a HUGE improvement. I'd be happy to add the condition for flag as a follow up, and update the help pages to get this patch moving. Liz
Created attachment 17019 [details] [review] Bug 9894 - Followup: Add support for "flagged" values Even though we don't know what they mean or do. To test: - open the docs to have at hand the list of codes/visibility options combinations and try changing several subfields from a single tagfield. Save, and reopen the "Edit subfields" page. - Click the "Flagged" checkbox - it should disable the other checkboxes as this setting is (apparently) exclusive to the others. - Step through several tabs and set your visibilities as you like. - I had a MySQL shell opened to repeatedly run this query (i tested against field 886 on the default framework): > SELECT tagfield,tagsubfield,hidden FROM marc_subfield_structure WHERE frameworkcode='' AND tagfield=886; - Compare what was saved on the DB with the docs for that visibility selection. Bug 9894 - Help file updates To test: Click the Help link in the upper right hand corner of the MARC subfield editor Verify that the epic list of values is gone Verify that the help page overall looks alright Verify that the help makes sense and is understandable.
Created attachment 17176 [details] [review] Bug 9894 - Followup: Add support for "flagged" values Even though we don't know what they mean or do. To test: - open the docs to have at hand the list of codes/visibility options combinations and try changing several subfields from a single tagfield. Save, and reopen the "Edit subfields" page. - Click the "Flagged" checkbox - it should disable the other checkboxes as this setting is (apparently) exclusive to the others. - Step through several tabs and set your visibilities as you like. - I had a MySQL shell opened to repeatedly run this query (i tested against field 886 on the default framework): > SELECT tagfield,tagsubfield,hidden FROM marc_subfield_structure WHERE frameworkcode='' AND tagfield=886; - Compare what was saved on the DB with the docs for that visibility selection. Bug 9894 - Help file updates To test: Click the Help link in the upper right hand corner of the MARC subfield editor Verify that the epic list of values is gone Verify that the help page overall looks alright Verify that the help makes sense and is understandable. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Created attachment 17177 [details] [review] Bug 9894 - (3.12) Small followup Liz's patch adds the 'flagged' value back, but there's a problem with its logic that prevents it to work on startup (works fine for changing values) Regards To+ Sponsored-by: Universidad Nacional de Córdoba
Created attachment 17181 [details] [review] Bug 9894: Add labels for checkboxes
Created attachment 17259 [details] [review] [SIGNED-OFF] Bug 9894 - (3.12) Small followup Liz's patch adds the 'flagged' value back, but there's a problem with its logic that prevents it to work on startup (works fine for changing values) Regards To+ Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Created attachment 17260 [details] [review] [SIGNED-OFF] Bug 9894: Add labels for checkboxes Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Great ergonomic improvement. All good to me. Marked as Passed QA.
Created attachment 17278 [details] [review] Bug 9894 - (3.12) de-serialize the 'hidden' value for subfields in frameworks This patch aims to provide a non-intrusive way (with respect to Koha's code, so it could have got into 3.12) for the users to easily choose the visibility settings for each subfield. This patch is also known to have arrived late for 3.12. I've been struggling with other stuff at work, but I just wanted to put this somewhere just in case it can be reused or something else. The patch adds two JavaScript functions that deal with (de)serialization of the 'hidden' value for subfields. One each way. Listeners are added to the checkboxes so the (actually, heh) 'hidden' value gets correctly updated on each checkbox change. There's a costly function to update all checkboxes on every tab to match the 'hidden' value on page load. To test: - open the docs to have at hand the list of codes/visibility options combinations and try changing several subfields from a single tagfield. Save, and reopen the "Edit subfields" page. - I had a MySQL shell opened to repeatedly run this query (i tested against field 886 on the default framework): > SELECT tagfield,tagsubfield,hidden FROM marc_subfield_structure WHERE frameworkcode='' AND tagfield=886; - Compare what was saved on the DB with the docs for that visibility selection. Regards To+ PS: I promise I'll work on the proper patch for 3.14, which would involve DB structure changes and (more than I expected) rewriten code. Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Liz Rea <liz@catalyst.net.nz> I promise to hug you the next time we meet. This works, and is a reasonable work around for a very longstanding awkward interface. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 17279 [details] [review] Bug 9894 - Followup: Add support for "flagged" values Even though we don't know what they mean or do. To test: - open the docs to have at hand the list of codes/visibility options combinations and try changing several subfields from a single tagfield. Save, and reopen the "Edit subfields" page. - Click the "Flagged" checkbox - it should disable the other checkboxes as this setting is (apparently) exclusive to the others. - Step through several tabs and set your visibilities as you like. - I had a MySQL shell opened to repeatedly run this query (i tested against field 886 on the default framework): > SELECT tagfield,tagsubfield,hidden FROM marc_subfield_structure WHERE frameworkcode='' AND tagfield=886; - Compare what was saved on the DB with the docs for that visibility selection. Bug 9894 - Help file updates To test: Click the Help link in the upper right hand corner of the MARC subfield editor Verify that the epic list of values is gone Verify that the help page overall looks alright Verify that the help makes sense and is understandable. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 17280 [details] [review] Bug 9894 - (3.12) Small followup Liz's patch adds the 'flagged' value back, but there's a problem with its logic that prevents it to work on startup (works fine for changing values) Regards To+ Sponsored-by: Universidad Nacional de Córdoba Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 17281 [details] [review] Bug 9894: Add labels for checkboxes Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
This patch has been pushed to master and 3.12.x.