Bug 35383 - Dragging and dropping subfield of repeated tags doesn't work
Summary: Dragging and dropping subfield of repeated tags doesn't work
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Slava Shishkin
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 35425
  Show dependency treegraph
 
Reported: 2023-11-22 11:57 UTC by Slava Shishkin
Modified: 2024-03-22 02:17 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.01


Attachments
Bug 35383: Make subfields of repeated tags draggable (1.34 KB, patch)
2023-11-22 15:05 UTC, Slava Shishkin
Details | Diff | Splinter Review
Bug 35383: Make subfields of repeated tags draggable (1.39 KB, patch)
2023-11-23 01:23 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 35383: Make subfields of repeated tags draggable (1.49 KB, patch)
2023-11-24 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Slava Shishkin 2023-11-22 11:57:43 UTC
This does not work when editing an existing record or when creating a new record in basic editor mode.

To test:
1. Open any Biblio and edit that record. 
   (select "Edit record" from the "Edit" drop-down menu).
2. Repeat any tag that may be repeated and has more than one subfield
   (e.g. 050 or 082) (click "Repeat this tag")
3. that when you try dragging a subfield of the new repeated tag, 
   the entire new tag moves.
Comment 1 Slava Shishkin 2023-11-22 15:05:57 UTC
Created attachment 159196 [details] [review]
Bug 35383: Make subfields of repeated tags draggable

To reproduce:
1. Open any biblio and edit that record. 
   (select "Edit record" from the "Edit" drop-down menu).
2. Repeat any tag that may be repeated and has more than one subfield
   (e.g. 050 or 082) (click "Repeat this tag")
3. Try dragging a subfield of the new repeated tag

Apply the patch and make sure dragging a subfield works well.
Comment 2 Phil Ringnalda 2023-11-23 01:23:04 UTC
Created attachment 159202 [details] [review]
Bug 35383: Make subfields of repeated tags draggable

To reproduce:
1. Open any biblio and edit that record.
   (select "Edit record" from the "Edit" drop-down menu).
2. Repeat any tag that may be repeated and has more than one subfield
   (e.g. 050 or 082) (click "Repeat this tag")
3. Try dragging a subfield of the new repeated tag

Apply the patch and make sure dragging a subfield works well.

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>
Comment 3 Marcel de Rooy 2023-11-24 09:54:11 UTC
Created attachment 159233 [details] [review]
Bug 35383: Make subfields of repeated tags draggable

To reproduce:
1. Open any biblio and edit that record.
   (select "Edit record" from the "Edit" drop-down menu).
2. Repeat any tag that may be repeated and has more than one subfield
   (e.g. 050 or 082) (click "Repeat this tag")
3. Try dragging a subfield of the new repeated tag

Apply the patch and make sure dragging a subfield works well.

Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2023-11-24 09:55:26 UTC
Thanks for your patch, Slava.
My only concern would be the assumption that Sortable is defined. In the current codebase this seems to be the case (called from authorities.tt or addbiblio).
Comment 5 Slava Shishkin 2023-11-28 17:38:52 UTC
(In reply to Marcel de Rooy from comment #4)

thanks, that makes a sense,

I can wrap up it in "if",

    if (typeof Sortable !== 'undefined') {
        $(clone).find("ul.sortable_subfield").each((i, e) => {
            Sortable.create(e, {
                animation: 150,
            });
        });
    }

but the best then is to make a callback option for CloneSubfield and leave Sortable.create code in files where it resides,

but that also leads to refactoring of onclick="CloneSubfield... to modern way how we doing that in our codebase by moving that listener to JS document ready block

Marcel, what do you think?
Comment 6 Marcel de Rooy 2023-12-01 07:18:13 UTC
(In reply to Slava Shishkin from comment #5)
> (In reply to Marcel de Rooy from comment #4)
> 
> thanks, that makes a sense,
> 
> I can wrap up it in "if",
> 
>     if (typeof Sortable !== 'undefined') {
>         $(clone).find("ul.sortable_subfield").each((i, e) => {
>             Sortable.create(e, {
>                 animation: 150,
>             });
>         });
>     }
> 
> but the best then is to make a callback option for CloneSubfield and leave
> Sortable.create code in files where it resides,
> 
> but that also leads to refactoring of onclick="CloneSubfield... to modern
> way how we doing that in our codebase by moving that listener to JS document
> ready block
> 
> Marcel, what do you think?

You are welcome to improve it. Please do so on a new report. I think that this patch can go in now in its current form. Your commitment is appreciated.
Comment 7 Slava Shishkin 2023-12-01 16:10:22 UTC
(In reply to Marcel de Rooy from comment #6)
> You are welcome to improve it. Please do so on a new report. I think that
> this patch can go in now in its current form. Your commitment is appreciated.

Ok, then I'll leave it in its current state and start working on refactoring. I will create a new ticket for that.
Comment 8 Katrin Fischer 2023-12-07 11:05:30 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 9 Fridolin Somers 2023-12-07 21:25:43 UTC
Pushed to 23.11.x for 23.11.01
Comment 10 Lucas Gass 2023-12-14 21:02:59 UTC
Cannot reproduce the problem in 23.05.x, no backport.