oops forgot to set the correct flag

The reordering works great, but the clone and delete tag links appear to be broken.

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

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.

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

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

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

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.

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.

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

Elliott,

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

Nicole

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

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

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

Kyle,
I could move subfields but not fields
(ej a <-> b, but not 610 <-> 611)

Is that the expected behavior?

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

Also another side effect is that load time increases:
with this patch ~36 sec 
without ~9 sec

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

Patch tested with a sandbox, by Joel Sasse <jsasse@plumcreeklibrary.net>

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.

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

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

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.

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

Hi,
Someone is still working on it?

Regards,
Marjorie

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

(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

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

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

(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

This is working great!

2 smalls things:
- required fields (the subfield's lib) are red
https://snipboard.io/CMuDYp.jpg
Is that expected?

- Do we really need the sort icon for each subfield?

And, not blocker for this bug report (regarding its age), but we could remove the "sortable" option for fields that cannot be duplicated for instance.

(In reply to Jonathan Druart from comment #49)
> This is working great!
> 
> 2 smalls things:
> - required fields (the subfield's lib) are red
> https://snipboard.io/CMuDYp.jpg
> Is that expected?

Sorry, that is coming from bug 24090.

Created attachment 96344 [details] [review]
Bug 7882: Add ability to move and reorder fields in MARC editor

Use jQueryUI sortable to make fields and subfields moveable in the
MARC editor for both records and authorities.

This patch convert items from div's to ul's and consequently alter css
and js to match. It also replace the up arrow with a more adapted icon
(from font awesome).

Primary authorship by Elliott Davis.

To Test:

1. Add or edit a record in cataloguing module.
2. You should be able to move the fields and subfields around.
    - You can click on any part of the element to drag it, the move
    icon to the left of the item is a good place to do it.
    => You can only change the order of fields of the same tag.
3. Make sure all of the javascript driven fonctionnality still work :
    - Duplicate fields/subfields
    - Remove fields/subfields
    - Using the tag editor for control fields or to link authorities
    - etc
4. Reorder some fields/subfields and save the record.
5. Edit the record again.
6. The order in the editor should match the changes which were saved.
    - Empty subfields should appear after the ones with content.
7. Repeat steps 1-6 with the authority editor.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

*** Bug 2124 has been marked as a duplicate of this bug. ***

When I test this I am able to drag a tag but when I drop it it returns to its original position. Dragging subfields works to re-order them, but after I save the record the order is back to where it was before. Tested in Firefox and Chrome on Windows 10.

Created attachment 96352 [details] [review]
Bug 7882: (follow-up) Update sorting interaction

This follow-up modifies the CSS for the sortable fields so that a
different icon is used. Elsewhere in Koha (stock rotation stage sorting)
we use the "bars" icon to indicate a sortable element. I think it would
be good to continue to use this. The cursor is now styled as a "Move"
cursor.

The configuration of the sortable function in the script has been
updated in two ways: 1. The movement of the element is constrained along
the y-axis, and 2. The movement of the element is contstrained to the
container within which it can be moved (for the tag, the tab, and for
the subfield, the tag).

To test, apply the patch and edit a MARC record in the basic cataloging
editor.

Confirm that the new icon is used, and that the "Move" cursor shows when
you hover over the icon.

Confirm that a tag can be dragged up and down within the constraints of
the visible section. A subfield should be only movable within its parent
tag.

I've added a minor follow-up even though this isn't working correctly for me (comment 53).

(In reply to Owen Leonard from comment #53)
> When I test this I am able to drag a tag but when I drop it it returns to
> its original position. Dragging subfields works to re-order them, but after
> I save the record the order is back to where it was before. Tested in
> Firefox and Chrome on Windows 10.

Hi Owen,
The important bit is "    => You can only change the order of fields of the same tag."
That is always why I suggested to remove the sortable thing for those fields. For instance unique fields cannot be moved.

> Dragging subfields works to re-order them, but after I save the record the order is back to where it was befor

I do not see this issue. I can sort subfields, save, edit again => the subfields order is kept.

(In reply to Owen Leonard from comment #55)
> even though this isn't working correctly for me

You're right Jonathan, tester error.

Created attachment 96384 [details] [review]
Bug 7882: (follow-up) Only show sortable icon for sortable tags

Only tags which can be repeated can be sorted, so the "bars" icon should
not display by these tags.

Maryse, Séverine, could you signoff Owen's patch?

Created attachment 96541 [details] [review]
Bug 7882: (follow-up) Update sorting interaction

This follow-up modifies the CSS for the sortable fields so that a
different icon is used. Elsewhere in Koha (stock rotation stage sorting)
we use the "bars" icon to indicate a sortable element. I think it would
be good to continue to use this. The cursor is now styled as a "Move"
cursor.

The configuration of the sortable function in the script has been
updated in two ways: 1. The movement of the element is constrained along
the y-axis, and 2. The movement of the element is contstrained to the
container within which it can be moved (for the tag, the tab, and for
the subfield, the tag).

To test, apply the patch and edit a MARC record in the basic cataloging
editor.

Confirm that the new icon is used, and that the "Move" cursor shows when
you hover over the icon.

Confirm that a tag can be dragged up and down within the constraints of
the visible section. A subfield should be only movable within its parent
tag.

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Created attachment 96542 [details] [review]
Bug 7882: (follow-up) Only show sortable icon for sortable tags

Only tags which can be repeated can be sorted, so the "bars" icon should
not display by these tags.

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Created attachment 96633 [details] [review]
Bug 7882: Add ability to move and reorder fields in MARC editor

Use jQueryUI sortable to make fields and subfields moveable in the
MARC editor for both records and authorities.

This patch convert items from div's to ul's and consequently alter css
and js to match. It also replace the up arrow with a more adapted icon
(from font awesome).

Primary authorship by Elliott Davis.

To Test:

1. Add or edit a record in cataloguing module.
2. You should be able to move the fields and subfields around.
    - You can click on any part of the element to drag it, the move
    icon to the left of the item is a good place to do it.
    => You can only change the order of fields of the same tag.
3. Make sure all of the javascript driven fonctionnality still work :
    - Duplicate fields/subfields
    - Remove fields/subfields
    - Using the tag editor for control fields or to link authorities
    - etc
4. Reorder some fields/subfields and save the record.
5. Edit the record again.
6. The order in the editor should match the changes which were saved.
    - Empty subfields should appear after the ones with content.
7. Repeat steps 1-6 with the authority editor.

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 96634 [details] [review]
Bug 7882: (follow-up) Update sorting interaction

This follow-up modifies the CSS for the sortable fields so that a
different icon is used. Elsewhere in Koha (stock rotation stage sorting)
we use the "bars" icon to indicate a sortable element. I think it would
be good to continue to use this. The cursor is now styled as a "Move"
cursor.

The configuration of the sortable function in the script has been
updated in two ways: 1. The movement of the element is constrained along
the y-axis, and 2. The movement of the element is contstrained to the
container within which it can be moved (for the tag, the tab, and for
the subfield, the tag).

To test, apply the patch and edit a MARC record in the basic cataloging
editor.

Confirm that the new icon is used, and that the "Move" cursor shows when
you hover over the icon.

Confirm that a tag can be dragged up and down within the constraints of
the visible section. A subfield should be only movable within its parent
tag.

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Created attachment 96635 [details] [review]
Bug 7882: (follow-up) Only show sortable icon for sortable tags

Only tags which can be repeated can be sorted, so the "bars" icon should
not display by these tags.

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Nice work everyone!

Pushed to master for 20.05

not backported to 19.11.x

*** Bug 11073 has been marked as a duplicate of this bug. ***

Was asked to reconsider backporting.  This is now backported to 19.11.x branch for 19.11.05

choosing to backport this very cool enhancement. 

backported to 19.05.x for 19.05.10

Enhancement will not be backported to 18.11.x series

This feature does not work in Safari (tested with version 14). Perhaps we could have the older arrow system for browsers that do not support this otherwise some users are without a method to reorder subfields. Thank you!

We also still have the problem that this feature doesn't work in Safari, which makes it difficult for those working from home using this browser.
Is it possible to do what Father Vlasie proposes?
Thank you in advance!