Bug 9894 - 'hidden' value is hard to understand in frameworks
Summary: 'hidden' value is hard to understand in frameworks
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 3.12
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-21 18:07 UTC by Tomás Cohen Arazi
Modified: 2014-05-26 21:04 UTC (History)
3 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9894 - (3.12) de-serialize the 'hidden' value for subfields in frameworks (9.09 KB, patch)
2013-03-26 02:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9894 - (3.12) de-serialize the 'hidden' value for subfields in frameworks (9.22 KB, patch)
2013-03-26 02:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9894 - (3.12) de-serialize the 'hidden' value for subfields in frameworks (9.39 KB, patch)
2013-03-26 03:05 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 9894 - Followup: Add support for "flagged" values (9.15 KB, patch)
2013-03-29 09:55 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 9894 - Followup: Add support for "flagged" values (9.16 KB, patch)
2013-04-03 18:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9894 - (3.12) Small followup (3.02 KB, patch)
2013-04-03 18:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 9894: Add labels for checkboxes (4.42 KB, patch)
2013-04-04 07:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9894 - (3.12) Small followup (3.09 KB, patch)
2013-04-07 15:03 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9894: Add labels for checkboxes (4.49 KB, patch)
2013-04-07 15:03 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 9894 - (3.12) de-serialize the 'hidden' value for subfields in frameworks (9.46 KB, patch)
2013-04-08 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9894 - Followup: Add support for "flagged" values (9.23 KB, patch)
2013-04-08 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9894 - (3.12) Small followup (3.14 KB, patch)
2013-04-08 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9894: Add labels for checkboxes (4.54 KB, patch)
2013-04-08 10:41 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 Tomás Cohen Arazi 2013-03-21 18:07:44 UTC
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/);
Comment 1 Tomás Cohen Arazi 2013-03-21 18:13:25 UTC
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.
Comment 2 Tomás Cohen Arazi 2013-03-26 02:07:37 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2013-03-26 02:35:17 UTC Comment hidden (obsolete)
Comment 4 Jared Camins-Esakov 2013-03-26 02:52:50 UTC
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.
Comment 5 Liz Rea 2013-03-26 03:05:52 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2013-03-28 16:23:25 UTC
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.
Comment 7 Liz Rea 2013-03-28 23:58:27 UTC
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
Comment 8 Liz Rea 2013-03-29 09:55:21 UTC Comment hidden (obsolete)
Comment 9 Tomás Cohen Arazi 2013-04-03 18:54:46 UTC Comment hidden (obsolete)
Comment 10 Tomás Cohen Arazi 2013-04-03 18:56:31 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2013-04-04 07:53:19 UTC Comment hidden (obsolete)
Comment 12 Bernardo Gonzalez Kriegel 2013-04-07 15:03:45 UTC Comment hidden (obsolete)
Comment 13 Bernardo Gonzalez Kriegel 2013-04-07 15:03:54 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2013-04-08 10:41:15 UTC
Great ergonomic improvement.
All good to me.

Marked as Passed QA.
Comment 15 Jonathan Druart 2013-04-08 10:41:36 UTC
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>
Comment 16 Jonathan Druart 2013-04-08 10:41:41 UTC
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>
Comment 17 Jonathan Druart 2013-04-08 10:41:44 UTC
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>
Comment 18 Jonathan Druart 2013-04-08 10:41:47 UTC
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>
Comment 19 Jared Camins-Esakov 2013-04-18 12:03:48 UTC
This patch has been pushed to master and 3.12.x.