Bug 7882 - Add ability to move and reorder fields in MARC editor
Summary: Add ability to move and reorder fields in MARC editor
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact: Jonathan Druart
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-02 21:55 UTC by Melia Meggs
Modified: 2018-04-04 10:36 UTC (History)
19 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Adds jQueryUI sortable features to fields. Converts list items from div's to ul's Added Image files to be used for Drag and drop in frameworks added CSS changes for images (9.59 KB, patch)
2012-11-01 21:46 UTC, Elliott Davis
Details | Diff | Splinter Review
Rebase for master -- DO NOT USE (23.22 KB, patch)
2012-11-20 14:57 UTC, Elliott Davis
Details | Diff | Splinter Review
Bug 7882: Add ability to sort fields and subfields (24.52 KB, patch)
2012-11-29 21:22 UTC, Elliott Davis
Details | Diff | Splinter Review
Bug 7882: Add ability to sort fields and subfields (24.43 KB, patch)
2012-12-19 21:13 UTC, Elliott Davis
Details | Diff | Splinter Review
Bug 7882 [fixed] (7.90 KB, patch)
2013-01-29 06:24 UTC, Elliott Davis
Details | Diff | Splinter Review
Bug 7882 - move or reorder 6xx and 7xx fields (8.32 KB, patch)
2013-05-08 13:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7882 - move or reorder 6xx and 7xx fields (8.37 KB, patch)
2013-05-08 13:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7882 - move or reorder 6xx and 7xx fields - Followup - Remove placeholder highlighting (1.29 KB, patch)
2013-05-08 13:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7882 - move or reorder 6xx and 7xx fields - Followup - Remove placeholder highlighting (1.51 KB, patch)
2013-05-08 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7882 - move or reorder 6xx and 7xx fields (7.07 KB, patch)
2013-11-08 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7882 - move or reorder 6xx and 7xx fields - Followup - Remove placeholder highlighting (1.51 KB, patch)
2013-11-08 16:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7882 - move or reorder 6xx and 7xx fields (6.98 KB, patch)
2013-12-13 15:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7882 - move or reorder 6xx and 7xx fields (7.04 KB, patch)
2013-12-13 16:36 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 7882: Inputs should be editable (1.27 KB, patch)
2014-01-07 13:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7882 - Add ability to move and reorder fields in MARC editor (7.06 KB, patch)
2014-01-13 12:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7882: Inputs should be editable (1.31 KB, patch)
2014-01-13 12:50 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Melia Meggs 2012-04-02 21:55:08 UTC
Would like to be able to move or reorder 6xx and 7xx fields.  The only way to change the order of 700s is to copy or cut /paste. We would like to be able to easily move production people on DVDs to below the actors.  Or change the order of 6xx fields to more accurately describe the subject of an item.
Comment 1 Elliott Davis 2012-11-01 21:46:49 UTC Comment hidden (obsolete)
Comment 2 Elliott Davis 2012-11-15 12:47:12 UTC
oops forgot to set the correct flag
Comment 3 Elliott Davis 2012-11-20 14:57:13 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2012-11-21 13:55:41 UTC
The reordering works great, but the clone and delete tag links appear to be broken.
Comment 5 Nicole C. Engard 2012-11-26 13:51:48 UTC
I wanted to add myself to the CC list, but couldn't without setting a patch complexity - please update the complexity if I guessed wrong.

Nicole
Comment 6 Elliott Davis 2012-11-29 21:22:33 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2012-12-04 10:30:55 UTC
Hi Elliott,

Some remarks:

1/ Could you provide a patch with yours modifications and 1 patch for the indentation please ? It is not easy at all to read the patch.

2/ I just have fields on the first tab, the others tabs are empty.

3/ The title for the cursor_split image is wrong ("move up")

4/ the following if statement is not graceful:
     if(flag){
         return StrAlert;
     } else {
         return flag;
     }
It could be replaced with if ( flag ) return StrAlert; return 0;

Marked as Failed QA.
Comment 8 Elliott Davis 2012-12-19 21:13:47 UTC Comment hidden (obsolete)
Comment 9 Elliott Davis 2012-12-20 15:03:32 UTC
(In reply to comment #7)
> Hi Elliott,
> 
> Some remarks:
> 
> 1/ Could you provide a patch with yours modifications and 1 patch for the
> indentation please ? It is not easy at all to read the patch.
> 
> 2/ I just have fields on the first tab, the others tabs are empty.
> 
I have corrected this issue

> 3/ The title for the cursor_split image is wrong ("move up")
> 
I corrected this to say move
> 4/ the following if statement is not graceful:
>      if(flag){
>          return StrAlert;
>      } else {
>          return flag;
>      }
> It could be replaced with if ( flag ) return StrAlert; return 0;
> 
This was added with the tab fix.  It appears that the file had tabs in it before and when I replaced them with spaces I got more diff than I bargained for.
> Marked as Failed QA.

I don't suppose there is any way to let the changes in whitespace slide.  I think to correct them I'd have to re-write the patch.
Comment 10 Jonathan Druart 2012-12-21 12:52:09 UTC
(In reply to comment #9)
> > 2/ I just have fields on the first tab, the others tabs are empty.
> > 
> I have corrected this issue
> 
> > 3/ The title for the cursor_split image is wrong ("move up")
> > 
> I corrected this to say move

Thanks!


> This was added with the tab fix.  It appears that the file had tabs in it
> before and when I replaced them with spaces I got more diff than I bargained
> for.

You mean your editor automatically replaces tab with 4 spaces when editing a file?

> I don't suppose there is any way to let the changes in whitespace slide.  I
> think to correct them I'd have to re-write the patch.

Yes I know it is not easy for this patch. But try to think about that for your next patch :)

There is no SO for your patch so I change the status to "Needs Signoff".
Comment 11 Elliott Davis 2012-12-21 14:28:13 UTC
not automatically, but before I found the git whitespace fix from rangi's blog I had it replace tabs with spaces.  Thanks for letting it slide this once and sorry for setting it to the wrong status.  I assumed that since it was you (Jonathan) it was going through QA.  Thats what I get for not looking at the backlog ;)

Elliott Davis
Comment 12 Owen Leonard 2012-12-28 17:01:20 UTC
This appears to be working at first glance, but I see that there is a problem somewhere with the markup, causing the language-switcher menu not to "stick" to the bottom of the screen. This is probably caused by an unclosed <div> somewhere.

I hate to say it, but I really think this patch needs to be submitted without all the whitespace changes. addbiblio.pl is a complex page, and not being able to carefully review the relevant changes makes me nervous.
Comment 13 Elliott Davis 2013-01-29 06:24:56 UTC Comment hidden (obsolete)
Comment 14 Bernardo Gonzalez Kriegel 2013-01-29 22:57:00 UTC
Work as described. Tags and subtags could be reordered.
But the result is a bit ugly, and there is too much indentation on subtags.
That's cosmetic.

The trouble I found is that can't collapse or expand tags.
Comment 15 Elliott Davis 2013-02-26 13:12:00 UTC
(In reply to comment #14)

> The trouble I found is that can't collapse or expand tags.

Was this existing functionality?  I wasn't aware that you could collapse or expand tags before.
Comment 16 Nicole C. Engard 2013-02-26 14:07:22 UTC
Elliott,

Clicking on the description of the tag let you expand or collapse it.

Nicole
Comment 17 Frederic Durand 2013-03-20 13:25:29 UTC
I ve tried it on a sandbox, it did not work, here s the error message :
Applying: Bug 7882 [fixed] 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/css/addbiblio.css Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
Comment 18 Jonathan Druart 2013-03-20 14:17:21 UTC
(In reply to comment #17)
> I ve tried it on a sandbox, it did not work, here s the error message :
> Applying: Bug 7882 [fixed] 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/css/addbiblio.css Auto-merging
> koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt

It is not blocker for testing. The patch was correctly applied.
Comment 19 Frederic Durand 2013-03-20 16:16:12 UTC
I confirm that expand/collapse is off.
When you want to modify you can t do it just clicking on it, you need to click the tag or subtag title or on the left of it
Remark/question : when you want to drag and move your field you got a yellow area, i suppose it s done to show you where you are going to move? In this case it misses its goal cause it tends to stay on the top.
Hope i m not too blurry in my explanations :)
Comment 20 Kyle M Hall 2013-05-08 13:39:53 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2013-05-08 13:40:49 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2013-05-08 13:40:58 UTC Comment hidden (obsolete)
Comment 23 Kyle M Hall 2013-05-08 13:42:25 UTC Comment hidden (obsolete)
Comment 24 Bernardo Gonzalez Kriegel 2013-05-08 17:50:26 UTC
Kyle,
I could move subfields but not fields
(ej a <-> b, but not 610 <-> 611)

Is that the expected behavior?
Comment 25 Bernardo Gonzalez Kriegel 2013-05-10 00:18:45 UTC
(In reply to comment #24)
> Kyle,
> I could move subfields but not fields
> (ej a <-> b, but not 610 <-> 611)
> 
That ^ was my mistake.

Works as described, but still could not expand or collapse fields.
Comment 26 Bernardo Gonzalez Kriegel 2013-05-14 00:19:40 UTC
Also another side effect is that load time increases:
with this patch ~36 sec 
without ~9 sec
Comment 27 I'm just a bot 2013-09-18 09:58:14 UTC
Applying: Bug 7882 - move or reorder 6xx and 7xx fields
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/en/css/addbiblio.css
M	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
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/css/addbiblio.css
Patch failed at 0001 Bug 7882 - move or reorder 6xx and 7xx fields
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 28 Kyle M Hall 2013-11-08 16:50:18 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2013-11-08 16:50:34 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2013-12-13 15:00:39 UTC Comment hidden (obsolete)
Comment 31 sandboxes@biblibre.com 2013-12-13 16:35:22 UTC
Patch tested with a sandbox, by Joel Sasse <jsasse@plumcreeklibrary.net>
Comment 32 sandboxes@biblibre.com 2013-12-13 16:36:15 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2013-12-24 11:24:36 UTC
QA comment:

After applying the patch, I am not able to add value in input text (as if they were readonly, but they are not...).

Moreover the commit message indicates that 6xx and 7xx fields will be sortable, but this patch impacts all fields .

Note that the page loading increases from 1.76s (onload: 5.19s) to 2.37s (onload: 7.15s) with this patch.

Marked as Failed QA.
Comment 34 Kyle M Hall 2014-01-07 12:36:19 UTC
> After applying the patch, I am not able to add value in input text (as if
> they were readonly, but they are not...).

Can you post some screenshots, or better yet, a video of this issue? I'm not able to reproduce it, but I'm not sure if I'm looking in the right place either.

> Moreover the commit message indicates that 6xx and 7xx fields will be
> sortable, but this patch impacts all fields .

Is this a real issue? That is, I propose we rename the bug to "move and reorder marc fields". I see this as only adding more flexibility to the marc editor.
Comment 35 Jonathan Druart 2014-01-07 13:36:58 UTC Comment hidden (obsolete)
Comment 36 Jonathan Druart 2014-01-07 13:39:16 UTC
(In reply to Kyle M Hall from comment #34)
> > After applying the patch, I am not able to add value in input text (as if
> > they were readonly, but they are not...).
> 
> Can you post some screenshots, or better yet, a video of this issue? I'm not
> able to reproduce it, but I'm not sure if I'm looking in the right place
> either.

I did nothing in particular, just open the editor and try to edit an input.
Why do you use disableSelection? My patch fixes my issue.

> > Moreover the commit message indicates that 6xx and 7xx fields will be
> > sortable, but this patch impacts all fields .
> 
> Is this a real issue? That is, I propose we rename the bug to "move and
> reorder marc fields". I see this as only adding more flexibility to the marc
> editor.

It is not an issue, of course. But it is not what it is written in the commit message.
Comment 37 Kyle M Hall 2014-01-13 12:50:05 UTC
Created attachment 24227 [details] [review]
Bug 7882 - Add ability to move and reorder fields in MARC editor

Adds jQueryUI sortable features to fields. Converts list items from
div's to ul's Added Image files to be used for Drag and drop in
frameworks added CSS changes for images

Primary authorship by Elliott Davis.

To Test:

Go into catalogging and choose to add a new item with your favorite
framework.
You should be able to move the fields and the subfields around
After making your desired number of changes choose to save
You should notice the marc record saved in the order that you arranged
the fields

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Joel Sasse <jsasse@plumcreeklibrary.net>
Comment 38 Kyle M Hall 2014-01-13 12:50:26 UTC
Created attachment 24228 [details] [review]
Bug 7882: Inputs should be editable

disableSelection is deprecated and cause an issue on editing an input
(input, select, textarea, etc.)
see http://api.jqueryui.com/disableselection/

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 39 Jonathan Druart 2014-01-13 13:15:13 UTC
Works as expected.

My only concern is about the page loading time. Tested now:

Before the patch:
1.12s (onload: 2.62s)
983ms (onload: 2.35s)
1.14s (onload: 2.5s)

After the patch:
1.6s (onload: 2.69s)
1.87s (onload: 3.04s)
1.59s (onload: 2.78s)

tested with firebug.

Maybe we should add a new pref here in order not to increase load time for low/old computers.

I let the RM takes the decision.

Marked as Passed QA.
Comment 40 Galen Charlton 2014-01-13 23:42:49 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #25)
> Works as described, but still could not expand or collapse fields.

This is still a problem -- ExpandField() cataloging.js, and possibly other routines, are expecting that subfields are contained in a <div>, not <li>, and would need to be updated.

Another thing that gets broken by the patch is the ability to client on the little triangle icons next to each subfield to move them up or down.  That should either be made to work, or it should be removed (on account that the subfields become sortable with this patch).

I'm failing the patch for those reasons, but they won't be difficult to deal with.

There's a more serious issue, however, which isn't caused by this patch but which would become more obvious with it -- if you open the record again in the editor, field order isn't maintained.  For example, if you construct your record so that subject headings are in the following order:

650 $aElizabethan theatre
600 $aShakespeare, William

then open it up again in the editor, the 600 will be displayed before the 650, even though the 650 field is stored first in the record.  This has the potential to be seriously confusing to users, and I feel that a patch that allows sorting of the fields in a user-specified order needs to be accompanied by work that makes the editor display existing fields in the order that they appear in the record.

The increase in the time (as measured by Jonathan) it takes to render the editor is unfortunate, but IMO does not quite rise to the level that calls for a new syspref.  Now if Bernardo's experience (36 seconds!) were the norm, that would be unacceptably slow.
Comment 41 Marjorie Barry-Vila 2015-02-12 19:17:54 UTC
Hi,
Someone is still working on it?

Regards,
Marjorie
Comment 42 Jacek Ablewicz 2015-03-30 08:51:59 UTC
(In reply to Marjorie Barry-Vila from comment #41)

> Someone is still working on it?

I wonder the same thing.. turns out that such feature would really come in handy, especially as C4::AuthoritiesMarc::merge() does currently have uncanny tendency to mess up field order in biblio records pretty badly, when authority records are being modified.
Comment 43 Marjorie Barry-Vila 2017-11-06 14:08:32 UTC
(In reply to Kyle M Hall from comment #38)
> Created attachment 24228 [details] [review] [review]
> Bug 7882: Inputs should be editable
> 
> disableSelection is deprecated and cause an issue on editing an input
> (input, select, textarea, etc.)
> see http://api.jqueryui.com/disableselection/
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Hi Kyle,

Do you think you can rebase the patch for the master version?

Regards,
Marjorie
Comment 44 David Bourgault 2018-02-28 14:50:14 UTC
>There's a more serious issue, however, which isn't caused by this patch but which would become more obvious with it -- if you open the record again in the editor, field order isn't maintained.  For example, if you construct your record so that subject headings are in the following order:
>
>650 $aElizabethan theatre
>600 $aShakespeare, William
>
>Then open it up again in the editor, the 600 will be displayed before the 650, even though the 650 field is stored first in the record.  This has the potential to be seriously confusing to users, and I feel that a patch that allows sorting of the fields in a user-specified order needs to be accompanied by work that makes the editor display existing fields in the order that they appear in the record.

This raises another question as to how to order the fields when loading a Marc record.

Let [1,2,3,4] be the framework structure
Let [1,4,2] be the saved record

Currently we load on the framework structure order, giving us
1, 2, 3 (empty), 4
This does not respect the saved record order. But if we load according do the saved record order, where should the empty tag (3) go?  Should we place it before the saved record's fields or after? Or do some weird half-sorted behavior?

before : [3 (empty), 1, 4, 2]
after  : [1, 4, 2, 3 (empty)]
weird  : [1, 3 (empty), 4, 2]

Let me know if my explanation isn't clear enough.
Comment 45 Katrin Fischer 2018-03-31 21:12:40 UTC
> 
> Let [1,2,3,4] be the framework structure
> Let [1,4,2] be the saved record
> 
> Currently we load on the framework structure order, giving us
> 1, 2, 3 (empty), 4
> This does not respect the saved record order. But if we load according do
> the saved record order, where should the empty tag (3) go?  Should we place
> it before the saved record's fields or after? Or do some weird half-sorted
> behavior?
> 
> before : [3 (empty), 1, 4, 2]
> after  : [1, 4, 2, 3 (empty)]
> weird  : [1, 3 (empty), 4, 2]

I'd put the empty ones at the bottom of the list, in alphabetic sequence.
Comment 46 Josef Moravec 2018-04-04 10:36:30 UTC
(In reply to Katrin Fischer from comment #45)
> > 
> > Let [1,2,3,4] be the framework structure
> > Let [1,4,2] be the saved record
> > 
> > Currently we load on the framework structure order, giving us
> > 1, 2, 3 (empty), 4
> > This does not respect the saved record order. But if we load according do
> > the saved record order, where should the empty tag (3) go?  Should we place
> > it before the saved record's fields or after? Or do some weird half-sorted
> > behavior?
> > 
> > before : [3 (empty), 1, 4, 2]
> > after  : [1, 4, 2, 3 (empty)]
> > weird  : [1, 3 (empty), 4, 2]
> 
> I'd put the empty ones at the bottom of the list, in alphabetic sequence.

+1