Patch applies and works correctly based on test plan. QA Tools fails (see below). Once these are corrected I'd be happy to sign off!

Processing files before patches
|========================>| 4 / 4 (100.00%)
Processing files after patches
|========================>| 4 / 4 (100.00%)

 OK     cataloguing/editor.pl
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   OK     spelling
   OK     valid

 OK     koha-tmpl/intranet-tmpl/prog/en/includes/cateditor-ui.inc
   OK     forbidden patterns
   OK     git manipulation
   OK     spelling
   OK     tt_valid
   OK     valid_template

 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/authorities/blinddetail-biblio-search.tt
   FAIL   forbidden patterns
   OK     git manipulation
   OK     spelling
   OK     tt_valid
   OK     valid_template

 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/editor.tt
   FAIL   forbidden patterns
   OK     git manipulation
   OK     spelling
   OK     tt_valid
   OK     valid_template

Love it! One day there might be an advanced editor for authorities too, but for now this is a big missing piece to make the new editor useful for more libraries.

Found 2 problems:

1) RTL languages

If the heading is in an RTL language, the field order will be messed up after linking it. Is that something we could fix? Entering the data manually and without linking seems to work ok.

To test:
- Enter something Arabic or Hebrew in 100$a
- Ctrl+L to link to an authority record
- Either select the right one or add a new one with heading in RTL language
- Link
- Verify display in 100$a is not looking right
- Try to save record, receive error message

2) Sequence of subfields is 'random'

This is actually a quite important one, as the sequence really matters.

To test:
- Add an authority records using $a $b $c $d
- Link record
- Subfields will appear out of order (if not, try again, might be random)

In my first test, I ended up with: 
100 _ _ ‡d1983-‡bIII‡aDuck, Donald‡cPope‡94

Created attachment 65144 [details] [review]
Bug 18904 - QA Followup

Use subfields in the order they are stored.

This patch removes use of an intermediate hash for storing values. Order
of subfields as obtained from authority is now preserved.

Also removes useless code that was intended to set $2 subfields

(In reply to Katrin Fischer from comment #4)
> 1) RTL languages
Yes, this is an issue, but it is an issue with the editor in general. RTL fields end up making the whole line RTL  - this has consequences since the editor mostly assumes LTR for field/subfield info. I think this shoudl be a separate bug. On a positive note - while linking to an RTL authority looks wrong, the information is saved correctly (view in the standard editor to see fields are read correctly after save)

> 2) Sequence of subfields is 'random'
This should now be fixed - we were using a hash to store subfield info and losing order - it should now be preserved as saved in the record

QA: Looking here now

Added a 600, opened blinddetail with Ctrl+L, chose an authority, and got:
Internal server error: Can't call method "authtypecode" on an undefined value at /usr/share/koha/masterclone/authorities/blinddetail-biblio-search.pl line 77.

Not giving up yet. This probably was bad data. But as said before on other reports if nobody checks what ->find returns, you get this.

Interesting feature! Rancor absolutely needs it.
I have the impression that you fix one or two bugs here in blinddetail that should go on their own. Please split.
Also I would rather not change blinddetail for Rancor, but replace what you need on the receiving side. (experimental etc.)

+            index = 'rancor';//+field.line+'|'+field.tag;
Debugging leftover?

+        [%- FOREACH authtag = authtags -%]
Hmm. Doesnt look nice.

+           var cur_field = opener.opener.jQuery(".CodeMirror")[0].CodeMirror.marceditor.getCurrentField();
What are you doing here exactly ? Why opener.opener ?

(In reply to Marcel de Rooy from comment #14)
> I have the impression that you fix one or two bugs here in blinddetail that
> should go on their own. Please split.
Created 19537 - the subfield order patch makes no sense on its own as it has no effect outside of rancor

> Also I would rather not change blinddetail for Rancor, but replace what you
> need on the receiving side. (experimental etc.)
I looked, but it doesn't make sense to me to move it - blinddetail does the replacement of the text in the record and we do similar fixing of text for traditional editor

> +            index = 'rancor';//+field.line+'|'+field.tag;
> Debugging leftover?
Fixed

 
> +        [%- FOREACH authtag = authtags -%]
> Hmm. Doesnt look nice.
Not sure what part you don't like here? Makes sense to me

> +           var cur_field =
> opener.opener.jQuery(".CodeMirror")[0].CodeMirror.marceditor.
> getCurrentField();
> What are you doing here exactly ? Why opener.opener ?
Same as traditional - editor opens auth-finder which opens blinddetail - opener.opener gets us back to the place we called from (the editor)

 FAIL   authorities/blinddetail-biblio-search.pl
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   SKIP   spelling
   FAIL   valid
                Global symbol "%subfields" requires explicit package name
                authorities/blinddetail-biblio-search.pl had compilation errors.

Missed adding dependency on 19537, that should clear up the error

Just tested now and have some issues/comments:

1) The indicators are not taken from authority to biblio, if this can add indicators, it'll take advantage of bug 14769 once it is pushed

2) cataloguing.js link in the template cataloguing/editor.tt should use [% theme %] instead of hadcoded "prog" in path. And should have attached _[% KOHA_VERSION %]

Could you have a look please at bug 14769 ?
We have a conflict re blinddetail*
Bug 14769 solves some issues for authority controlled indicators (including $2 subfields), working with the regular editor. I may be biased, but Rancor is still a lower priority?
Could you follow the changes there ?

(In reply to Marcel de Rooy from comment #34)
> Could you have a look please at bug 14769 ?
> We have a conflict re blinddetail*
> Bug 14769 solves some issues for authority controlled indicators (including
> $2 subfields), working with the regular editor. I may be biased, but Rancor
> is still a lower priority?
> Could you follow the changes there ?

I don't think we should prioritize here, but I am biased too. Since Rancor is no longer experimental and this is just the feature missing that keeps a lot of people from using it. I view this one as a very important candidate for 18.05.

First patch here: 6.7.2017, Sign off: 10.8.2017
Review here has already been done a few times with prompt follow-ups and never mentioning a possible conflict. 

For bug 14769
Current patch set: 26.1.2018, Sign off: 5.3.2018

That said, please talk to each other to figure out how to resolve this conflict best - if we work together, it will hopefully be possible to have both patches go in before the deadline.

I'll help with testing too, just tell me what you come up with. 
I can also test this again, if there is a follow-up. I am just out for QA since I did the initial sign-off.

(In reply to Katrin Fischer from comment #35)
> Since Rancor is no longer experimental
Bug 19538 is still in discussion?

I think we don't need to keep discussing - let's try to find a way out of the maze. :)

(In reply to Marcel de Rooy from comment #34)
> but Rancor is still a lower priority?

Just for completeness: Do not take this too strongly :)

QA: Looking here now

Created attachment 73851 [details] [review]
Bug 18904: Advanced editor - Rancor - Add authority support

This patchset adds the ability to use the authorities search to select
or clear fields in the advanced editor

To test:
 1 - Open a record in the advanced cataloging editor
 2 - Press 'Ctrl+L' while in a field that shuold not be linked to
authorities (300 for instance)
 3 - Nothing should happen
 4 - Try it in a field that should be linked
 5 - You should get the authorities pop-up
 6 - Values in pop-up should be populated from values in record (as
        appropriate for authority type)
 7 - Correct authority type should be selected ( PERSO_NAME for 100,
        TOPIC_TERM for 650, etc.)
 8 - Press 'Clear', field should be blanked
 9 - Search again and select an authority
10 - Field should be correctly populated

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 73852 [details] [review]
Bug 18904: (follow-up) Fix whitespace and don't use hardcoded path

Fix whitespace/tabs
use [% interface %] not intranet-tmpl

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 73853 [details] [review]
Bug 18904: (follow-up) Retrieve subfield in the order they are stored

Use subfields in the order they are stored.

This patch removes use of an intermediate hash for storing values. Order
of subfields as obtained from authority is now preserved.

Also removes useless code that was intended to set $2 subfields

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 73854 [details] [review]
Bug 18904: Remove debugging code

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 73855 [details] [review]
Bug 18904: (follow-up) Set indicators and correct js path

TO test:
1 - Have an authority with some indicators
2 - Link a field in rancor to that authority
3 - See that you get some indicators (same in unimarc, diff in marc21)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 73856 [details] [review]
Bug 18904: (follow-up) Rebase onto 14769

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: simplified if-then-else around cur_field.setIndicator[12].

Created attachment 73857 [details] [review]
Bug 18904: (QA follow-up) Do not put authid in searchbox of blinddetail

Adding trivial test if subfield equals 9.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Tiny remark:

+my $authtags = Koha::MarcSubfieldStructures->search({ authtypecode => { '!=' => '' }, 'frameworkcode' => '' });

Does Rancor only look at default framework ?
(Actually, this is quite fundamental, but this report is not about the relation between Rancor and the frameworks.)

(In reply to Marcel de Rooy from comment #54)
> Tiny remark:
> 
> +my $authtags = Koha::MarcSubfieldStructures->search({ authtypecode => {
> '!=' => '' }, 'frameworkcode' => '' });
> 
> Does Rancor only look at default framework ?
> (Actually, this is quite fundamental, but this report is not about the
> relation between Rancor and the frameworks.)

(13:20:46) ashimema: I do think Rancor entirely ignores frameworks.. always been somewhat surprised by that too marcelr ;)
(13:20:56) kidclamp: yes, it does only use default framework, anything edited becomes defauilt framework
(13:22:43) cait: marcelr: it's true, it only looks at default - there is no need for most of the settings if you jsut type on a blank screen

But... why CTRL+L?
It's used by the browser the select the address bar, so if I CTRL+L the popup opens, but the address bar is selected when I close it.

(In reply to Jonathan Druart from comment #56)
> But... why CTRL+L?
> It's used by the browser the select the address bar, so if I CTRL+L the
> popup opens, but the address bar is selected when I close it.

CTRL+L made sense logically in English L = 'Link'

I tried my best but can't seem to prevent the default action in the browser

I am happy to change it if you have a suggestion

It's weird that nobody else raised this problem before. Do not you think the behavior is very bad?

(In reply to Jonathan Druart from comment #59)
> It's weird that nobody else raised this problem before. Do not you think the
> behavior is very bad?

I remember commenting on problems with some shortcuts in Firefox on another report. Whether the key worked, depended on the field having focus iirc..

Why not use Ctrl-Shift-L?

Created attachment 74023 [details] [review]
Bug 18904: (follow-up) Use Ctrl+Shift+L

Created attachment 74055 [details] [review]
Bug 18904: (follow-up) Use Ctrl+Shift+L

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Retested with the new shortcut - works great for me. Docs were adjusted.

Pushed to master for 18.05, thanks to everybody involved!

Enhancement, not backported for 17.11.x