Bug 26055

Summary: Advanced Editor Rancor should use "visibility" and not "mandatory" for drawing new records
Product: Koha Reporter: David Cook <dcook>
Component: CatalogingAssignee: Bugs List <koha-bugs>
Status: NEW --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrewfh, m.de.rooy
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25962
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:

Description David Cook 2020-07-23 23:50:16 UTC
The Advanced Editor (Rancor) uses a tag's mandatory flag to decide whether to show in the UI when showing an editor for a new record.

This doesn't make 100% sense, since often a cataloguer will also want to see optional fields (like 500) by default to serve as a prompt. 

In systems like Horizon, one refers to these as "templates". You have a separate set of typical fields for books, journals, DVDs, etc. You don't have to fill in all those fields, but they're the typical fields that you would use for them.
Comment 1 David Cook 2020-07-23 23:54:25 UTC
I could work on this one, but I wonder about the best way to handle this.

In Koha, we typically do not change existing behaviour, so we add system preferences. 

I'm undecided whether the current behaviour is a "bug" or if it's just a system design with which I disagree. 

Perhaps the solution is to create a new option in the editor and use a syspref to change the button.

Note the following:

1) /cgi-bin/koha/cataloguing/editor.pl#new/
This only shows fields that are marked as "Mandatory"

2) /cgi-bin/koha/cataloguing/editor.pl#new-full/
This shows all fields in the framework regardless of "Mandatory" or "visibility" states
Comment 2 David Cook 2020-07-23 23:54:42 UTC
Relevant code:

./koha-tmpl/intranet-tmpl/prog/en/includes/cateditor-ui.inc
'new': {
            titleForRecord: _("Editing new record"),
            get: function( id, callback ) {
                record = new MARC.Record();
                KohaBackend.FillRecord( '', record );

                callback( record );
            },
        },

"koha-tmpl/intranet-tmpl/lib/koha/cateditor/koha-backend.js"
FillRecord: function( frameworkcode, record, allTags ) {
            $.each( _frameworks[frameworkcode], function( undef, tag ) {
                var tagnum = tag[0], taginfo = tag[1];

                if ( taginfo.mandatory != "1" && !allTags ) return;

                var fields = record.fields(tagnum);

                if ( fields.length == 0 ) {
                    var newField = new MARC.Field( tagnum, ' ', ' ', [] );
                    fields.push( newField );
                    record.addFieldGrouped( newField );

                    if ( tagnum < '010' ) {
                        newField.addSubfield( [ '@', (taginfo.subfields[0] ? taginfo.subfields[0][1].defaultvalue : null ) || '' ] );
                        return;
                    }
                }

                $.each( taginfo.subfields, function( undef, subfield ) {
                    var subfieldcode = subfield[0], subfieldinfo = subfield[1];

                    if ( subfieldinfo.mandatory != "1" && !subfieldinfo.defaultvalue && !allTags ) return;

                    $.each( fields, function( undef, field ) {
                        if ( !field.hasSubfield(subfieldcode) ) {
                            field.addSubfieldGrouped( [ subfieldcode, subfieldinfo.defaultvalue || '' ] );
                        } else if ( subfieldinfo.defaultvalue && field.subfield( subfieldcode ) === '' ) {
                            field.subfield( subfieldcode, subfieldinfo.defaultvalue );
                        }
                    } );
                } );
            } );
        },
Comment 3 Andrew Fuerste-Henry 2020-07-24 12:30:03 UTC
I don't like the idea of making Visibility apply to Rancor. In the regular editor, visibility is determining whether or not a tag is available to put a value into at all. That really doesn't apply in Rancor, where one can manually key in any tag one wants. I suppose we could add a new checkbox for Advanced Editor to the visibility options.
This highlights a broader issue of how Rancor's relationship to frameworks is confusingly vague compared to the regular editor's relationship to frameworks.
Comment 4 Katrin Fischer 2020-07-24 12:39:33 UTC
(In reply to Andrew Fuerste-Henry from comment #3)
> I don't like the idea of making Visibility apply to Rancor. In the regular
> editor, visibility is determining whether or not a tag is available to put a
> value into at all. That really doesn't apply in Rancor, where one can
> manually key in any tag one wants. I suppose we could add a new checkbox for
> Advanced Editor to the visibility options.
> This highlights a broader issue of how Rancor's relationship to frameworks
> is confusingly vague compared to the regular editor's relationship to
> frameworks.

+1 

I agree that I would not like seeing the editor visibility used for both editors. Libraries might use both and while you HAVE to check visibility to be able to enter it at all in one editor, it would get in the way in the other. You will have to delete empty fields before saving, so you will want to keep it to an absolute minimum set = different use case. I agree a new checkbox would be a good option and relabelling "editor" to make clear what applies to which.
Comment 5 David Cook 2020-07-27 00:37:12 UTC
(In reply to Andrew Fuerste-Henry from comment #3)
> This highlights a broader issue of how Rancor's relationship to frameworks
> is confusingly vague compared to the regular editor's relationship to
> frameworks.

Agreed

(In reply to Katrin Fischer from comment #4)
> I agree a new
> checkbox would be a good option and relabelling "editor" to make clear what
> applies to which.

While a new checkbox sounds like a good idea, it is easier said than done. 

Here's some comments on the current mapping from ./Koha/Filter/MARC/ViewPolicy.pm:

# marc_subfields_structure.hidden
# allows you to select from 19 possible visibility conditions, 17 of which are implemented. They are the following:
# -9 => Future use
# -8 => Flag
# -7 => OPAC !Intranet !Editor Collapsed
# -6 => OPAC Intranet !Editor !Collapsed
# -5 => OPAC Intranet !Editor Collapsed
# -4 => OPAC !Intranet !Editor !Collapsed
# -3 => OPAC !Intranet Editor Collapsed
# -2 => OPAC !Intranet Editor !Collapsed
# -1 => OPAC Intranet Editor Collapsed
# 0 => OPAC Intranet Editor !Collapsed
# 1 => !OPAC Intranet Editor Collapsed
# 2 => !OPAC !Intranet Editor !Collapsed
# 3 => !OPAC !Intranet Editor Collapsed
# 4 => !OPAC Intranet Editor !Collapsed
# 5 => !OPAC !Intranet !Editor Collapsed
# 6 => !OPAC Intranet !Editor !Collapsed
# 7 => !OPAC Intranet !Editor Collapsed
# 8 => !OPAC !Intranet !Editor !Collapsed
# 9 => Future use
# ( ! means 'not visible' or in the case of Collapsed 'not Collapsed')

Firstly, the premise that there are 19 possible visibility conditions is false. While the "hidden" field is TINYINT(1), the 1 apparently refers to column padding rather than allowed characters. TINYINT(1) actually allows for values from -128 to +127. 

Secondly, in theory, 4 options means 16 permutations and 5 options means 25 permutations, but I doubt we want to include the new Rancor option in any of the existing 16 permutations, so I think we end up with 32 permutations like below? (Flag not being included in these counts obviously.)

# -16 => OPAC Intranet Editor Collapsed Rancor
# -15 => OPAC Intranet Editor !Collapsed Rancor
# -14 => OPAC Intranet !Editor Collapsed Rancor
# -13 => OPAC Intranet !Editor !Collapsed Rancor
# -12 => OPAC !Intranet Editor Collapsed Rancor
# -11 => OPAC !Intranet Editor !Collapsed Rancor
# -10 => OPAC !Intranet !Editor Collapsed Rancor
# -9 =>  OPAC !Intranet !Editor !Collapsed Rancor

# -8 =>  Flag
# -7 =>  OPAC !Intranet !Editor Collapsed !Rancor
# -6 =>  OPAC Intranet !Editor !Collapsed !Rancor
# -5 =>  OPAC Intranet !Editor Collapsed !Rancor
# -4 =>  OPAC !Intranet !Editor !Collapsed !Rancor
# -3 =>  OPAC !Intranet Editor Collapsed !Rancor
# -2 =>  OPAC !Intranet Editor !Collapsed !Rancor
# -1 =>  OPAC Intranet Editor Collapsed !Rancor
# 0 =>   OPAC Intranet Editor !Collapsed !Rancor
# 1 =>  !OPAC Intranet Editor Collapsed !Rancor
# 2 =>  !OPAC !Intranet Editor !Collapsed !Rancor
# 3 =>  !OPAC !Intranet Editor Collapsed !Rancor
# 4 =>  !OPAC Intranet Editor !Collapsed !Rancor
# 5 =>  !OPAC !Intranet !Editor Collapsed !Rancor
# 6 =>  !OPAC Intranet !Editor !Collapsed !Rancor
# 7 =>  !OPAC Intranet !Editor Collapsed !Rancor
# 8 =>  !OPAC !Intranet !Editor !Collapsed !Rancor

# 9 =>  !OPAC !Intranet !Editor !Collapsed Rancor
# 10 => !OPAC !Intranet !Editor Collapsed Rancor
# 11 => !OPAC !Intranet Editor !Collapsed Rancor
# 12 => !OPAC !Intranet Editor Collapsed Rancor
# 13 => !OPAC Intranet !Editor !Collapsed Rancor
# 14 => !OPAC Intranet !Editor Collapsed Rancor
# 15 => !OPAC Intranet Editor !Collapsed Rancor
# 16 => !OPAC Intranet Editor Collapsed Rancor

It's a shame we didn't go with bitpacking for the "hidden" field. That would've been so much easier. Or just had separate database fields. 

Does that look right to other people?
Comment 6 David Cook 2020-07-27 00:39:01 UTC
Alternatively... it looks like we no longer support SQL export/import... so maybe just adding a separate column for "show_advanced_editor" might be better.
Comment 7 Katrin Fischer 2020-07-27 05:41:58 UTC
I'd actually use a new database column instead of adding to the hard to understand number scheme.
Comment 8 David Cook 2020-07-28 23:12:35 UTC
(In reply to Katrin Fischer from comment #7)
> I'd actually use a new database column instead of adding to the hard to
> understand number scheme.

I was concerned about this breaking import/export of frameworks, but I think it might be OK. I'll have to look into it a bit further at some point.