Testing scenario (Creating an authority record for a failed automatic link) :

* In you system preferences, deactivate the "AutoCreateAuthorities" preference.
* Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
* Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
* Add random informations in field 600$a of the biblio record.
* Click the "Link authorities automatically" button. The message box should now show "600 - No matching authority found." if no matching authority was found. For this scenario, we want the authority match to have failed.
* The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
* A popup should appear, containing a "Modify authority" form. The form should be pre-filled, a the informations in the 600$a field of the biblio should be in the 100$a field of this new authority record.
* Click the "Save" button
* The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
* Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

This patch apply correctly.
I create an authorithy following all these steps without saving the record (with 600$a and 700$a).
Works great !
But...
When the authority already exists, the message box still indicates "No matching authority found." (when the record is not saved, and when it is)
 
Even if we can check with the regular "Tag Editor" button, it can be really confusing to have both "authority checking system"...

The bug you encountered seems to be an indexation problem.
If the zebra index wasn't rebuild between the authority creation and the linkage attempt, the new authority record won't be found by the linker.

On our instances we use a deamon which automatically rebuilds the Zebra index when a new record is added to the queue.

Could anyone test this feature again and rebuild the zebra index once the new authority is added?

Also, the automatic linking should work on existing authorities that were already indexed.

Changing status to "Needs Signoff" so another round of QA can be done with zebra reindexations.

Could not test this patch due to a problem with the sandbox setup:

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 11299:
<h1>Something went wrong !</h1>Applying: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Biblio.pm
Auto-merging C4/Linker.pm
Auto-merging authorities/authorities.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/js/cataloging.js
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt
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
Failed to merge in the changes.
Patch failed at 0001 Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

I don't see the "Link authorities automatically" button.

Does this patch still apply ?

I "fixed" Frederik/Simith's patch by standardizing the title and adding the testing scenario to it.

Added a second patch with the lost button.

QA comments:

1/ run the qa script, there are some failures
2/ perltidy the new script (cataloguing/automatic_linker.pl)
3/ use Modern::Perl;
4/ Not sure about the permission {catalogue => 1}
Since authorities are created, I'd say editauthorities is required.
5/     * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."

No, I got
    Automatic authority link results:
    700 - No matching authority found.
    ...
6/ Tests are needed for changes done in subroutines.

Comment on attachment 39477 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

Review of attachment 39477 [details] [review]:
-----------------------------------------------------------------

::: cataloguing/automatic_linker.pl
@@ +29,5 @@
> +my $input        = new CGI;
> +print $input->header('application/json');
> +
> +# Check the user's permissions
> +my %cookies = fetch CGI::Cookie;

Bug 14589 is attempting to standardize to CGI::Cookie->fetch;
Please change your syntax.

Created attachment 45218 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

CONFLICT (content): Merge conflict in C4/Biblio.pm

Could you rebase?

The addbiblio.tt conflict is easy enough to resolve, though you need to look into fontawesome classes instead of icon-ok.

The C4/Biblio.pm conflict is harder to resolve, as the logic which is ripped out and replaced with a function has changed. This means the logic in the new function needed to be rechecked.

Please rebase.

Created attachment 56440 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

 add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

Created attachment 56509 [details] [review]
Bug 11299 small fix because TransformHtmlToMarc parameters it's changed

small fix and rebase all patches

I tested these patches and found some issues:

When editing biblio (cataloguing/addbiblio.pl), the button "Z39.50/SRU Search" is here twice - probably due to rebase

Button "Link authorities automatically" doesn't have icon - please use font awesome icons instead of obsoleted icon-* classes

When trying to link on blank record i got "undefined - undefined" response instead of "No authority link was changed."

Icons next to authority fields  (approve.gif / deny.gif) are not showing.

The matching itself looks OK

Created attachment 58307 [details] [review]
Bug 11299: fix problem show undefined when submit blank data and other small fix

   - Show icon of button automaticlinker and search
   - Add icons deny and approve
   - Fix problem button 'Z39.50/SRU Search' it's show twice

Created attachment 60998 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

Hi all,
I rebase all patches and squash all in one good patch.
Thanks.

Hi Bouzid,

I've tested the linking from the normal cataloguing editor and I really love it.

So, some first feedback:

There are 2 complaints from the QA script:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
   FAIL	  forbidden patterns
		forbidden pattern: Contains old bootstrap style for buttons (see bug 16239) (line 710)

 FAIL	authorities/authorities.pl
   FAIL	  spelling
		 autority  ==> authority

Testing:

When a match was found, it works nicely. Problems appear, when a new authority is to be added.

- When I click on the plugin, instead of the normal search form, the add authority form opens, but the window is too small for the big form with the normal header on top, the navigation on top is 'messy' in the small screen.
- It opened the wrong framework. I had 655 - so it should open Genre/Form, but instead it was Personal name.

Maybe it would be worth to split this big patch. For now, have the automatic linking from cataloguing and leave the authority plugin untouched and deal with that later on? But maybe it's an easy fix.

Created attachment 63312 [details] [review]
Bug 11299 - QA fix

Created attachment 63313 [details] [review]
Bug11299 - Fix problems when authority is not found and i want add it

Hi Katrin,

Thanks for the feedback.

I fix all problems from the QA script and all problems when you open window for add authority.

Now, if the authority is not found, when you click add authority, it's open the  window with the correct authority type.

Thanks.

Hi Bouzid, one step further!

- QA test tools pass now
- Adding a new authority entry after checking works nicely.
- Adding a new authority that was checked negative works much better too, but fields are not prepopulated as noted in the test plan, could you check? If this is not intended for now, we could also change the test plan and solve this later on, as long as we don't leave broken code.

Created attachment 64075 [details] [review]
Bug11299 - Prepopulate the fields of new authority by the values writed in biblio

I am sorry, but the workflow for adding a new record is still not quite right:

- Download a record from LOC
- Link authorities
- Check fields listed
- First problem: The same icon appears next to $9 and $a, but with different meaning. $a has the normal search, $9 the Create authority. We should use different icons at least.
- Second problem: Using Create authority the fields are prefilled now (good!), but after saving the window doesn't close and the $9 in the record is not populated.

I will try to keep an eye on this one and come back to it sooner next time!

Argh, sorry, I think I forgot to switch the status last time I worked on this.

Created attachment 67162 [details] [review]
Bug 11299 - Change icon for create authority

Hi katrin,
 - For the first problem, I changed icon for $9 when create authority
 - For the second problem, the workflow is the same in cataloguing authority plugin  
          - Add MARC record
          - Write any information in for example 700$a and click Tag editor
          - You have cataloguing authority plugin window
          - If you click Create new authority, you have the same page when you  click create authority added in this patch
          - Import authority form LOC and save
          - After saving, the window doesn't close and the information is not reported in the cataloguing authority plugin.
          - If you need found this authority, you should reindex zebra and click search 

   - The workflow in this patch is the same, when create authority, you should reindex and reclick Link authorities automatickly and you have the authid in $9.

I think this is the workflow of koha.

Thanks.
Bouzid.

(In reply to Bouzid from comment #43)
> Hi katrin,
>  - For the first problem, I changed icon for $9 when create authority
>  - For the second problem, the workflow is the same in cataloguing authority
> plugin  
>           - Add MARC record
>           - Write any information in for example 700$a and click Tag editor
>           - You have cataloguing authority plugin window
>           - If you click Create new authority, you have the same page when
> you  click create authority added in this patch
>           - Import authority form LOC and save
>           - After saving, the window doesn't close and the information is
> not reported in the cataloguing authority plugin.
>           - If you need found this authority, you should reindex zebra and
> click search 
> 
>    - The workflow in this patch is the same, when create authority, you
> should reindex and reclick Link authorities automatickly and you have the
> authid in $9.

I think if this is the case (could not verify on master yet) - it's a regression/bug. I just tested on 16.11.10 as this was available to me:

- Open default framework
- Click on plugin on 100$a
- Use create authority from plugin window
- Enter 100$a with a name
- Save authority
- Window closes and data is filled in

If there is a bug, we will need to fix that first.

Created attachment 67708 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

Created attachment 67709 [details] [review]
Bug 11299 - QA fix

Created attachment 67710 [details] [review]
Bug11299 - Fix problems when authority is not found and i want add it

Created attachment 67712 [details] [review]
Bug11299 - Prepopulate the fields of new authority by the values writed in biblio

Created attachment 67713 [details] [review]
Bug 11299 - Change icon for create authority

Created attachment 67714 [details] [review]
Bug11299 - Populated field for record when import authority in z3950

Hi Katrin,
I fixed problem populated field of record when import from z3950 server.
The last patch, fix also problem when click tag editor in cataloguig module
    - Choose create authority
    - z39.50 search
    - In result of search, choose any authority and click import.
    - When save authority, the fileds of record is populate with value of import authority

without the patch, when save authority, the field is not populate.

Thanks.
Bouzid.

Created attachment 67824 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

 add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

Created attachment 67825 [details] [review]
Bug 11299 - QA fix

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

Created attachment 67826 [details] [review]
Bug11299 - Fix problems when authority is not found and i want add it

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

Created attachment 67827 [details] [review]
Bug11299 - Prepopulate the fields of new authority by the values writed in biblio

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

Created attachment 67828 [details] [review]
Bug 11299 - Change icon for create authority

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

Created attachment 67829 [details] [review]
Bug11299 - Populated field for record when import authority in z3950

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

Thank you for your perseverance!

+eval { eval "require $linker_module"; };

Does not look good.

Bouzid: I will still continue on this one. But how do you make sure that you do not save a personal name into say field 610 that should only include corporate names ?

(In reply to Marcel de Rooy from comment #60)
> Bouzid: I will still continue on this one. But how do you make sure that you
> do not save a personal name into say field 610 that should only include
> corporate names ?

QA: Looking here again little bit.
Bouzid: A response would have been helpful to expedite QA now..

Thx for submitting your work. But I am still having serious doubts about this development.

In the editor we normally work with the authority popup. You completely go around that here, but I still need to undo the plugin assignments before I can really get your function working.
If I disconnect 600, you still assume that it is a personal name. But I may not want to work with your approach? If I enter an incorrect authid into $9, it accepts the name in $a and does not create an authority.
But in another field (650), I see one is not accepted with an invalid authid, and another one with invalid authid is accepted. This is not stable.

To get short, I would recommend to remove the link button, move your function to a plugin and minimize the changes to existing code to get it working. In that way we can choose which plugin we want to use. It might provide evidence for being stable and making a transition.

tests for LinkBibHeadingsToAuthorities ?
new sub AddAuthorityFromHeading
Please note that the RM does no longer want to add C4 routines.
What about UNIMARC btw?
And adding an authority should not be in C4::Biblio!

+    # Get the first heading found in the biblio informations
+    foreach my $field ( $record->fields() ) {
+        my $heading = C4::Heading->new_from_bib_field($field);
+        next unless defined $heading;
Isn't there a smarter way than just passing each field to Heading ?

+is(C4::Biblio::AddAuthorityFromHeading(), 0, 'AddAuthorityFromHeading returns zero if not passed heading, field or bib');
This is not enough to test a new sub.

Double eval (comment 59)

There are enough reasons listed to postpone further QA for now. I am sorry for the amount of work here, but imo it needs larger adjustments.

Please squash the patches and correct the bug titles. See guidelines. We need them to be of the form Bug 12345: What does this patch do?

Hi Marcel,

> In the editor we normally work with the authority popup. You completely go
> around that here, but I still need to undo the plugin assignments before I
> can really get your function working.

I didn't do that in my testing. I still could use the normal plugin with the link on $a IIRC. I know I didn't change the frameworks.

> If I disconnect 600, you still assume that it is a personal name. But I may
> not want to work with your approach? If I enter an incorrect authid into $9,
> it accepts the name in $a and does not create an authority.
> But in another field (650), I see one is not accepted with an invalid
> authid, and another one with invalid authid is accepted. This is not stable.
> 
> To get short, I would recommend to remove the link button, move your
> function to a plugin and minimize the changes to existing code to get it
> working. In that way we can choose which plugin we want to use. It might
> provide evidence for being stable and making a transition.

I think you can't achieve the same functionality with a plugin, a button to check all fields at the same time and give you a summary report of the ones linked/unlinked. 

And you shouldn't have to choose - both should always be possible, manual (with search) and automatic linking.

I think it should work like this: Check the framework for the authority type linked to a field and check for authorities of that type to link it with automatically. Fields not linked to an authority type, should not be auto-linked.

(In reply to Katrin Fischer from comment #65)
> I think it should work like this: Check the framework for the authority type
> linked to a field and check for authorities of that type to link it with
> automatically. Fields not linked to an authority type, should not be
> auto-linked.

Currently this is hardcoded in Heading somewhere.

(In reply to Marcel de Rooy from comment #66)
> (In reply to Katrin Fischer from comment #65)
> > I think it should work like this: Check the framework for the authority type
> > linked to a field and check for authorities of that type to link it with
> > automatically. Fields not linked to an authority type, should not be
> > auto-linked.
> 
> Currently this is hardcoded in Heading somewhere.

Hm, yeah, I seem to remember that this was used for importing authorities in order to assign the correct framework. Maybe this could be seen as a separate bug?

(In reply to Marcel de Rooy from comment #60)
> Bouzid: I will still continue on this one. But how do you make sure that you
> do not save a personal name into say field 610 that should only include
> corporate names ?

Hi Marcel,
Bouzid worked for our community to implement this development.
We use it as it has been here for 7 years now.
When using  "Link authorities automatically" the authorities are linked according to the type of authority.
If you enter a person name in a 610, the automatic link will not be made to a PERSO_NAME authority in 610 because it must be a CORPO_NAME. 
Can you describe test plan that led you to this result?

Regards,
Marjorie

(In reply to Marjorie Barry-Vila from comment #68)
> Hi Marcel,
> Bouzid worked for our community to implement this development.
> We use it as it has been here for 7 years now.
> When using  "Link authorities automatically" the authorities are linked
> according to the type of authority.
> If you enter a person name in a 610, the automatic link will not be made to
> a PERSO_NAME authority in 610 because it must be a CORPO_NAME. 
> Can you describe test plan that led you to this result?

Hi Marjorie,
I raised a question and hoped for a response from Bouzid, but he did not and I continued QA until I felt that it made no sense to continue further..
So I do not have a test plan for you.
The comments before gave a series of points that need attention and/or feedback.

Marcel

Bouzid did not respond because I asked him not to, waiting for the user (Marjorie) to comment.  Fixing bugs or architecture issues is one thing.  Defending the usefulness of a feature is another, and a much bigger one in my opinion.

If the value of the feature can't pass your smell test, I can't recommend that the user throw good money after bad into the re-engineering of the feature, indefinitely, to follow the latest architectural whims.  They coded this in 2010ish, so you can bet it's always playing catch-up, code-wise.

And if it could be put into a plugin, I would have recommended that avenue long ago.  My rebase at each version would be much simpler, and the client would have saved money overall.  But this is a major cataloging feature  -- essential, as commented by a cataloging power-user -- and needs to be seamlessly integrated.

If the user wants, Bouzid will start with the rework you asked.  Not sure about Unimarc, honestly, and if it's a deal breaker, well...  

Let's put this under the vise of slow death, for now: I'll put it In Discussion.

(In reply to Blou from comment #70)
> Bouzid did not respond because I asked him not to, waiting for the user
> (Marjorie) to comment.  
Which could lead to misunderstandings about why the author does not respond..

> If the value of the feature can't pass your smell test
'Your smell test' does not sound good to me. Smells like an offense ;) No native speaker btw. But I understand your feelings when code is not approved.

> If the user wants, Bouzid will start with the rework you asked.  Not sure
> about Unimarc, honestly, and if it's a deal breaker, well...  
> Let's put this under the vise of slow death, for now: I'll put it In
> Discussion.

I will send a mail to the QA team. We could see if other QAers support the feature in its current form.

(In reply to Blou from comment #70)
> If the value of the feature can't pass your smell test, I can't recommend
> that the user throw good money after bad into the re-engineering of the
> feature, indefinitely, to follow the latest architectural whims.  They coded
> this in 2010ish, so you can bet it's always playing catch-up, code-wise.

Coded in 2010 and submitted to the community in November 2013.

I do not need to read the test plan or the content of the patch when I see that:
 C4/Biblio.pm                                       | 138 +++++++++++++-
 C4/Linker.pm                                       |   5 +-
 C4/Linker/Default.pm                               |   2 +-
 t/Biblio.t                                         |   2 +

This definitely is code smell, tests for changes in C4 and Koha are required for years now.

Several questions have been asked by Marcel 4 months ago, it would be good to get  an answer first. That should not take much time to just... answer, right?

1) Did you read my explanation on the matter of answering?  The user/owner has many other priorities, and the delay should not be a consideration.  Who's waiting on who here?

2) In all those exchanges, I do not see the issue of Tests being raised until you did.  Not that it's not valid, mind you, but the testers are working on the Functionalities/usage (Kathleen) or questioning the validity (Marcel).  I prefer that, personally.  No point in working and rebasing and working again on tests for a feature that might not get accepted as deemed useless.  Tests are easy, Bouzid/team has written plenty of those for all the other patches.

I have no personal attachment to the feature.  Didn't code it, do not maintain it.  And knowing what code it touches, it matters to me that it's done well.  The process is important and must be followed.  But the feature, per the actual cataloging users, is a must have for a serious tool. 

Must. Have.

Philippe
PS You get a piece of meat, you smell it, your experience/intuition tells you it's shit, you won't eat it.  No need to call a biolab to test the meat.  QAs have seen enough features and pieces of code to do that with patches.

I do that with all _new_ features or bugs we talk about submitting. Let me tell you I have a picky nose myself, and I'm good at math/finance.  The paying users appreciate.

A smell test is not an insult.

Hi all,

as I have commented before I see a very valid use case for the feature as is and I think we should work on including it into Koha properly.

I am sorry for having missed the missing tests - I _should_ have noticed and noted that early on.

I agree that we need to do 2 things here:
- Agree on the feature itself.
- Agree on a clear list of things TBD to get this into Koha.

This won't make it into 18.05 now which is unfortunate.

I'd like to offer to go through the comments and see if I can create a summary first of what has been asked/unanswered and then we could try moving from there?
I could do this within the next 2 weeks I think.

That would be great, Kathleen.  Follwing that, I'll make sure we give it quick response.

Thanks

Created attachment 79385 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

Created attachment 79386 [details] [review]
Bug 11299 - QA fix

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

Created attachment 79387 [details] [review]
Bug11299 - Fix problems when authority is not found and i want add it

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

Created attachment 79388 [details] [review]
Bug11299 - Prepopulate the fields of new authority by the values writed in biblio

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

Created attachment 79389 [details] [review]
Bug 11299 - Change icon for create authority

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

Created attachment 79390 [details] [review]
Bug11299 - Populated field for record when import authority in z3950

Created attachment 79399 [details] [review]
Bug11299 - Populated field for record when import authority in z3950

Created attachment 79447 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

Created attachment 79448 [details] [review]
Bug 11299 - QA fix

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

Created attachment 79449 [details] [review]
Bug11299 - Fix problems when authority is not found and i want add it

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

Created attachment 79450 [details] [review]
Bug11299 - Prepopulate the fields of new authority by the values writed in biblio

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

Created attachment 79451 [details] [review]
Bug 11299 - Change icon for create authority

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

Created attachment 79452 [details] [review]
Bug11299 - Populated field for record when import authority in z3950

Created attachment 79545 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

Comment on attachment 79545 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

Review of attachment 79545 [details] [review]:
-----------------------------------------------------------------

::: C4/Biblio.pm
@@ +475,4 @@
>  
>  =head2 LinkBibHeadingsToAuthorities
>  
> +  my $num_headings_changed, %results = LinkBibHeadingsToAuthorities($linker, $marc, $frameworkcode, [$allowrelink, $verbose]);

This is getting too long, consider a parameter refactor.

Please look at: https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL16:_Hashrefs_should_be_used_as_arguments

Hi,
any progress on this bug?

Created attachment 94818 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

This patch works but broke me import via z39.50. Without them import working, with do nothing. Can you fix this?

Thank you.

Created attachment 95126 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

https://bugs.koha-community.org/show_bug.cgi?id=12299

Hi Michal,
I fixed the problem for import z3950.
Thanks.
Bouzid.

Created attachment 95152 [details] [review]
Bug 11299 - Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

1. I see a new sub added to C4 without test coverage. Moreover it's c/ping a lot of code from LinkBibHeadingsToAuthorities (refactoring needed)

2. New images must be replaced by bootstrap icons

3. In authorities.pl, "require C4::Heading;" why require instead of use?

4. addbiblio needs permission editcatalogue and displays the "linker button", but 
cataloguing/automatic_linker.pl requires editauthorities. I think the button must be hidden if permission is not sufficient.

5. Hazardous indentation in some file, for instance:

+function confirmnotdup(redirect){
+       $("#confirm_not_duplicate").attr("value","1");
+    $("#redirect").attr("value",redirect);
+       Check();
+}

Regarding history I am removing the FQA status, letting QA step for someone else.

Created attachment 98571 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

 add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 98572 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Hi,
 
 1. Refactoring  AddAuthorityFromHeading

  2. Deleting New images and use bootstrap icons

  3. Replace require by use In authorities.pl

  4. Add permission editcatalogue in cataloguing/automatic_linker.pl.

  5. Indentation  in all files
  
  6. When running qa tools, all it's ok

 Thanks.

Bouzid.

I just realized I gave this the first sign-off - so I am out for QA. 

But also, patch doesn't apply:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.
Using index info to reconstruct a base tree...
M	C4/Biblio.pm
M	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
M	koha-tmpl/intranet-tmpl/prog/js/cataloging.js
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/js/cataloging.js
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 C4/Biblio.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-11299-Add-a-button-to-the-biblio-edition-page--LuP6BI.patch

Created attachment 99249 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Hi Kat,
The patch it's apply correctly now.
Thanks.
Bouzid.

I think it would have been beneficial to split this patch (as cait also suggested in the bug report) since there seems to be at least two logical changes. This would also increase the chances people have the time to sign-off and QA the patches.

This is not a comprehensive review but I noticed this introduces new subroutine GetRecordValue which is a) not used at all b) doesn't appear to be functional since the DB table it uses doesn't exists. Also MARC::Field module is used without importing it.

It also appears that the new sub AddAuthorityFromHeading() should go at least to C4/AuthoritiesMarc.pm because it has nothing to do with biblio(?). It would be even better if it was in a Koha object since C4 namespace is being deprecated.

Overall this works and I agree, it is a much desired functionality.

Test plan missing step: Must enable BiblioAddsAuthorities

Blockers:

When creating a new record the popup does not close / links not added automatically
The record is saved an picked up if I relink

sub AddAuthorityFromHeading
  - can this just be an option for AddAuthority? I don't understand why it is in C4/Biblio.pm
  - it checks the heading for things like 'series_added_entry' - i cannot identify where in the code these are being set

sub GetRecordValue - added but not used?

Change to POD in Linker.pm doesn't seem to have corresponding code change?

Why a change to default linker behavior?
-if ( $behavior eq 'default' && $#{$authorities} == 0 ) {
+if ( $behavior eq 'default' && $#{$authorities} >= 0 ) {

automatic_linker.pl
  - duplicates a lot of code from C4/Biblio->BiblioAutoLink - we should rather pass verbose option through that sub
  - should this be in svc?
  
I am a bit confused about new param biblioindex vs index, we only ever seem to pass index, unless I missed something here

Without edit authorities permission I get a warning that I cannot use the 'Link authorities automatically' button - I shouldn't see it if I can't use it

As has been noted, test coverage is needed for changes to modules

Other improvement, could be on new bug:
1 - This doesn't work with the advanced cataloging editor - code should be made flexible enough that can be done
2 - If links are changed they become green - if button clicked again they are not green - may not be obvious that I still need to save changes - i think a second run should preserve what was changed since the record was opened/created.
3 - The notice about links should show the headings that were linked/not-linked otherwise it can be confusing which individual 650 had an issue - colors are nice but we should no rely only on colors for accessibility
4 - The js uses hardcoded styles for colors - can this be made css so the js just adds a class, would allow easier override by users

Another two months without change, this is too late for 20.05 now.. updating target to 20.11

Hi Bouzid,
any progress with this bug? We're ready for testing.

Hi Michal,
Thanks, next week I will working for fixing all issues.
Bouzid

Created attachment 108091 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

 add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Hi all,
I started working on this feature to resolve any issues that were raised, hoping it will be accepted this time around.

Step 1: Rebase the patch so that it applies correctly on the master   ---OK

Step 2 : I fix all issues  -- in progress

Thanks.
Bouzid

Created attachment 108309 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

 add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Hi all,
 - I have finished resolving the issues that Nick raised.
 - Now, when we create a new record, all the open windows close correctly and the links are done automatically, I even added the deletion of the authorities  creation link in case the link is made correctly.
 - Move AddAuthorityFromHeading to C4/AuthoritiesMarc.pm
 - Delete GetRecordValue
 - Restore default linker behavior
 - Deleting param biblioindex and using index
 - Make the Link authorities automatically button invisible if the user does not have editauthorities permission 
 
 I believe I have resolved all the issues raised, hoping they will be accepted.
 This feature is very late and its maintenance is becoming more and more complex.
 Thanks in advance to those who will pay attention to this feature and allow it to move forward.
 
 Bouzid

Created attachment 108310 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Hi Bouzid,

good work, it works for me. SO again after rebase.

Tiny critique from the QA script:

 FAIL	cataloguing/automatic_linker.pl
   OK	  critic
   FAIL	  forbidden patterns
		forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545) (line 19)

Nick, would you mind continuing here as you did the last review?

(In reply to Katrin Fischer from comment #126)
> Nick, would you mind continuing here as you did the last review?

C4/AuthoritiesMarc.pm:
sub AddAuthorityFromHeading - added but never used - can remove test as well

C4/Biblio.pm:
sub LinkBibsHeadingsToAuthorities
please add verbose tests to t/db_dependent/Biblio.t

C4/Linker.pm - remove POD changes (no changes to code here)

authorities/authorities.pl - I do not think we need the line below
+  use C4::Heading;

cataloguing/automatic_linker.pl:
- should be in svc I think
- This dupicats the sub "BiblioAutoLink" - instead that sub should be updated to pass through the 'verbose' switch and pass back results and this script can simply call that sub

koha-tmpl/intranet-tmpl/prog/en/modules/authorities/blinddetail-biblio-search.tt:
please use a CSS class rather than hardcoded color value

koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:
please use a CSS class rather than hardcoded color values

Other improvement, could be on new bug:
1 - This doesn't work with the advanced cataloging editor - code should be made flexible enough that can be done
2 - If links are changed they become green - if button clicked again they are not green - may not be obvious that I still need to save changes - i think a second run should preserve what was changed since the record was opened/created.
3 - The notice about links should show the headings that were linked/not-linked otherwise it can be confusing which individual 650 had an issue - colors are nice but we should not rely only on colors for accessibility

Created attachment 108396 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

 add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 108552 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

 add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Hi all,
 I have fixed all the problems now.
  1- adding verbose tests to t/db_dependent/Biblio.t
  2- Remove line use C4::Heading in authorities/authorities.pl
  3- Move cataloguing/automatic_linker.pl  to svc and call  BiblioAutoLink  with verbose
  4- Using Css class in all .tt
  5- Changing the message <tag> No matching authority found when the authority is created or imported
 
  Thanks.
  Bouzid.

Created attachment 108834 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 * In you system preferences, deactivate the "AutoCreateAuthorities" preference.
 * Go to the biblio record creation form to create a new biblio record. (koha/cataloguing/addbiblio.pl)
 * Click the "Link authorities automatically" button. A message should appear, telling the user "No authority link was changed."
 * Add random informations in field 600$a of the biblio record.
 * Click the "Link authorities automatically" button.
   * if matching authority was found,  The message box should now show "600 - A matching authority was found in the local database".
   * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
     * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.
     * A popup will appear, containing a form to create the missing  authority. The form should be pre-filled, a the informations in the 600$a field of the biblio should    be in the 100$a field of this new authority record.
 * Fill in the form and click the "Save" button
 * The 600$9 field in the biblio record should now be back to normal and have the authid of the authority record that was just created.
 * Click the "Link authorities automatically" button again. The message box should now show "No authority link was changed.".

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 108835 [details] [review]
Bug 11299: (QA follow-up) Fix tests, spaces, and CSS update

Removed my from variables in test

Undid stray line deletions

Undid adding 1; to test file

Fix CSS replace command

Update license

Fix spelling

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

1. 
+function confirmnotdup(redirect){

+function Dopop(link,i) {

+function PopupZ3950() {

Those 3 functions already exist in the file!

2. 
-    <input type="hidden" name="authtypecode" value="[% authtypecode | html %]" />
+    <input type="hidden" name="authtypecode" value="[% authtypecode  | html%]" />
     <input type="hidden" name="authid" value="[% authid | html %]" />
-    <input type="hidden" name="index" value="[% index | html %]" />
+    <input type="hidden" name="index" value="[% index | html  %]" />

Really, this is plain useless.

3. +            action: "../authorities/authorities.pl",
Why not /cgi-bin/...?

4. 
+            form.append($('<input>').attr({
+                type: 'hidden',
+                name: 'authtypecode',
+                value: heading.auth_type
+            }));

Yes, if you want, but you can also write the input as "usual" I think
form.append($('<input type="hidden" name="authtypecode" value="'+heading.auth_type+'" />'));
Not blocker.

5. Test plan says "Add random informations in field 600$a of the biblio record."
But 600$a has a "Thesaurus" and so input is disabled. I removed it.
Then I entered "a" or "harry", but "loading please wait" and JS error: TypeError: subfield[0] is undefined
Please double check, this is not working.

6. svc script, we usually use output_with_http_headers
I would like to test potential encoding issues. Has this been tested?

7. dialog "No authority link was changed." must be styled (class alert or message)

Created attachment 109062 [details] [review]
Bug 11299: (follow-up) Correct rebase errors and add message class to dialog

(In reply to Jonathan Druart from comment #133)
> 6. svc script, we usually use output_with_http_headers
> I would like to test potential encoding issues. Has this been tested?

There shouldn't be encoding, we are returning only diagnostic info on what was matched, not any actual authority terms from the DB.

Created attachment 109063 [details] [review]
Bug 11299: (follow-up) Skip fields with no 9 subfields

To test:
1 - Disable the 'thesaurus' for a controlled field like 600a or 650a
2 - Edit a record and put random data in this field
3 - Try to link, get a JS error
4 - Apply patch
5 - Repeat
6 - No more error, field is not linked (as it has no $9 subfield)

I am still getting the JS error "TypeError: subfield[0] is undefined"
* edit 600$a for the default framework, set thesaurus blank
* edit a new biblio, default framework, go to 600$a fill in with "a", click the magic button
= it's loading endlessly

Created attachment 109069 [details] [review]
Bug 11299: (follow-up) Skip fields with no 9 subfields

To test:
1 - Disable the 'thesaurus' for a controlled field like 600a or 650a
2 - Edit a record and put random data in this field
3 - Try to link, get a JS error
4 - Apply patch
5 - Repeat
6 - No more error, field is not linked (as it has no $9 subfield)

Same as comment 137.

Created attachment 109181 [details] [review]
Bug 11299: (follow-up) Skip fields with no 9 subfields

To test:
1 - Disable the 'thesaurus' for a controlled field like 600a or 650a
2 - Edit a record and put random data in this field
3 - Try to link, get a JS error
4 - Apply patch
5 - Repeat
6 - No more error, field is not linked (as it has no $9 subfield)

(In reply to Jonathan Druart from comment #139)
> Same as comment 137.

Yes, it quite helps if I commit my changes

Can I get a better and full test plan please?

I always get "No authority link was changed."

Btw the test plan says

       * if no matching authority was found, the message box should now show  "600 - No matching authority found.".  For this scenario, we want the authority match to have failed.
         * The 600$9 field should now be red and icons should have appeared next to it. Click on the "create authority" icon.

I had to make 600$9 visible for editor, and still not see the red, icons or "create authority" icon

Created attachment 110553 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 1 - In your system preferences set:
    AutoCreateAuthorities: don't generate
    BiblioAddsAuthorities: don't allow
 2 - Go to the Cataloging -> New record (koha/cataloguing/addbiblio.pl)
    Ensure you are using the basic editor
 3 - Click the "Link authorities automatically" button.
    A message should appear, telling the user "No authority link was changed."
 4 - Add random informations in field 600$a of the biblio record.
 5 - Click the "Link authorities automatically" button.
   the message box should now show  "600 - No matching authority found.".
   the 9 subfield is red
   Above the 9 subfield is a red X with a blue plus next to it
   Hover on the plus, see it is titled 'Create authority'
 6 - Click the 'Create authority' link
 7 - A new authroity form pops up, the info from the cataloging editor is prefilled
     Click the 100 field heading to expand and confirm info is transferred
 8 - Fill in necessary fields and save the new authority
 9 - The cataloging screen now has the 9 subfield populated and is green
10 - Click "Link authorities automatically" again
     Dialog says "No authority link was changed"
11 - In another tab go to System preferences and set AutoCreateAuthorities to 'Allow'
12 - Add random information to the 650 field
13 - Click 'Link authorities' button
14 - Dialog says:650 - No matching authority found. A new authority was created automatically.
15 - The subfield 9 is green and has the id of the new authority record
16 - In another tab search authorities and find an existing subject heading
17 - Add a new 650 with the info from the existing record
18 - Click 'Link authorities'
19 - The new field is correctly linked to existing authority

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 110554 [details] [review]
Bug 11299: (QA follow-up) Fix tests, spaces, and CSS update

Removed my from variables in test

Undid stray line deletions

Undid adding 1; to test file

Fix CSS replace command

Update license

Fix spelling

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 110555 [details] [review]
Bug 11299: (follow-up) Correct rebase errors and add message class to dialog

Created attachment 110556 [details] [review]
Bug 11299: (follow-up) Skip fields with no 9 subfields

To test:
1 - Disable the 'thesaurus' for a controlled field like 600a or 650a
2 - Edit a record and put random data in this field
3 - Try to link, get a JS error
4 - Apply patch
5 - Repeat
6 - No more error, field is not linked (as it has no $9 subfield)

Created attachment 110557 [details] [review]
Bug 11299: (follow-up) correct link

Created attachment 110568 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 1 - In your system preferences set:
    AutoCreateAuthorities: don't generate
    BiblioAddsAuthorities: don't allow
 2 - Go to the Cataloging -> New record (koha/cataloguing/addbiblio.pl)
    Ensure you are using the basic editor
 3 - Click the "Link authorities automatically" button.
    A message should appear, telling the user "No authority link was changed."
 4 - Add random informations in field 600$a of the biblio record.
 5 - Click the "Link authorities automatically" button.
   the message box should now show  "600 - No matching authority found.".
   the 9 subfield is red
   Above the 9 subfield is a red X with a blue plus next to it
   Hover on the plus, see it is titled 'Create authority'
 6 - Click the 'Create authority' link
 7 - A new authroity form pops up, the info from the cataloging editor is prefilled
     Click the 100 field heading to expand and confirm info is transferred
 8 - Fill in necessary fields and save the new authority
 9 - The cataloging screen now has the 9 subfield populated and is green
10 - Click "Link authorities automatically" again
     Dialog says "No authority link was changed"
11 - In another tab go to System preferences and set AutoCreateAuthorities to 'Allow'
12 - Add random information to the 650 field
13 - Click 'Link authorities' button
14 - Dialog says:650 - No matching authority found. A new authority was created automatically.
15 - The subfield 9 is green and has the id of the new authority record
16 - In another tab search authorities and find an existing subject heading
17 - Add a new 650 with the info from the existing record
18 - Click 'Link authorities'
19 - The new field is correctly linked to existing authority

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 110569 [details] [review]
Bug 11299: (QA follow-up) Fix tests, spaces, and CSS update

Removed my from variables in test

Undid stray line deletions

Undid adding 1; to test file

Fix CSS replace command

Update license

Fix spelling

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 110570 [details] [review]
Bug 11299: (follow-up) Correct rebase errors and add message class to dialog

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 110571 [details] [review]
Bug 11299: (follow-up) Skip fields with no 9 subfields

To test:
1 - Disable the 'thesaurus' for a controlled field like 600a or 650a
2 - Edit a record and put random data in this field
3 - Try to link, get a JS error
4 - Apply patch
5 - Repeat
6 - No more error, field is not linked (as it has no $9 subfield)

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 110572 [details] [review]
Bug 11299: (follow-up) correct link

Signed-off-by: Michal Denar <black23@gmail.com>

Hi Nick,
awesome work. Patches working if I follow test plan. 
But sometimes I can't make match with local authority and link if:
- $x (650) or 
- $v 

is filled in biblio record. 

If I'll clear $v and try to autolink, link wil be created without problem. Is it correct behaviour?

(In reply to Michal Denar from comment #153)
> Hi Nick,
> awesome work. Patches working if I follow test plan. 
> But sometimes I can't make match with local authority and link if:
> - $x (650) or 
> - $v 
> 
> is filled in biblio record. 
> 
> If I'll clear $v and try to autolink, link wil be created without problem.
> Is it correct behaviour?

Sorry Michal, I don't think I follow, can you illustrate with some screen shots?

Hi Nick,

maybe this video helps:
https://recordit.co/HZnnGOsfHD

Thank you

(In reply to Michal Denar from comment #155)
> Hi Nick,
> 
> maybe this video helps:
> https://recordit.co/HZnnGOsfHD
> 
> Thank you

It looks like other headings linked, do you confirm that you have a record for totalinarianism without subheading fiction? Otherwise I believe things are functioning as expected

Michal, do you have time to retest? 

I'd really love to see this finally make it over the finish line and we are getting awfully close to feature freeze again.

Hi Katrin,
I tested it and this feature working for me. I think taha we can move to QA.

Created attachment 111706 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 1 - In your system preferences set:
    AutoCreateAuthorities: Don't generate
    BiblioAddsAuthorities: Allow
 2 - Go to the Cataloging -> New record (koha/cataloguing/addbiblio.pl)
    Ensure you are using the basic editor
 3 - Click the "Link authorities automatically" button.
    A message should appear, telling the user "No authority link was changed."
 4 - Add random informations in field 600$a of the biblio record.
 5 - Click the "Link authorities automatically" button.
   the message box should now show  "600 - No matching authority found.".
   the 9 subfield is red
   Above the 9 subfield is a red X with a blue plus next to it
   Hover on the plus, see it is titled 'Create authority'
 6 - Click the 'Create authority' link
 7 - A new authroity form pops up, the info from the cataloging editor is prefilled
     Click the 100 field heading to expand and confirm info is transferred
 8 - Fill in necessary fields and save the new authority
 9 - The cataloging screen now has the 9 subfield populated and is green
10 - Click "Link authorities automatically" again
     Dialog says "No authority link was changed"
11 - In another tab go to System preferences and set AutoCreateAuthorities to 'Generate'
12 - Add random information to the 650 field
13 - Click 'Link authorities' button
14 - Dialog says:650 - No matching authority found. A new authority was created automatically.
15 - The subfield 9 is green and has the id of the new authority record
16 - In another tab search authorities and find an existing subject heading
17 - Add a new 650 with the info from the existing record
18 - Click 'Link authorities'
19 - The new field is correctly linked to existing authority

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 111707 [details] [review]
Bug 11299: (QA follow-up) Fix tests, spaces, and CSS update

Removed my from variables in test

Undid stray line deletions

Undid adding 1; to test file

Fix CSS replace command

Update license

Fix spelling

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 111708 [details] [review]
Bug 11299: (follow-up) Correct rebase errors and add message class to dialog

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 111709 [details] [review]
Bug 11299: (follow-up) Skip fields with no 9 subfields

To test:
1 - Disable the 'thesaurus' for a controlled field like 600a or 650a
2 - Edit a record and put random data in this field
3 - Try to link, get a JS error
4 - Apply patch
5 - Repeat
6 - No more error, field is not linked (as it has no $9 subfield)

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 111710 [details] [review]
Bug 11299: (follow-up) correct link

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 112125 [details] [review]
Bug 11299: (QA follow-up) Fix tests, spaces, and CSS update

Removed my from variables in test

Undid stray line deletions

Undid adding 1; to test file

Fix CSS replace command

Update license

Fix spelling

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 112126 [details] [review]
Bug 11299: (follow-up) Correct rebase errors and add message class to dialog

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 112127 [details] [review]
Bug 11299: (follow-up) Skip fields with no 9 subfields

To test:
1 - Disable the 'thesaurus' for a controlled field like 600a or 650a
2 - Edit a record and put random data in this field
3 - Try to link, get a JS error
4 - Apply patch
5 - Repeat
6 - No more error, field is not linked (as it has no $9 subfield)

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 112128 [details] [review]
Bug 11299: (follow-up) correct link

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 112129 [details] [review]
Bug 11299: Fix indirect object notation

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

Created attachment 112130 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 1 - In your system preferences set:
    AutoCreateAuthorities: Don't generate
    BiblioAddsAuthorities: Allow
 2 - Go to the Cataloging -> New record (koha/cataloguing/addbiblio.pl)
    Ensure you are using the basic editor
 3 - Click the "Link authorities automatically" button.
    A message should appear, telling the user "No authority link was changed."
 4 - Add random informations in field 600$a of the biblio record.
 5 - Click the "Link authorities automatically" button.
   the message box should now show  "600 - No matching authority found.".
   the 9 subfield is red
   Above the 9 subfield is a red X with a blue plus next to it
   Hover on the plus, see it is titled 'Create authority'
 6 - Click the 'Create authority' link
 7 - A new authroity form pops up, the info from the cataloging editor is prefilled
     Click the 100 field heading to expand and confirm info is transferred
 8 - Fill in necessary fields and save the new authority
 9 - The cataloging screen now has the 9 subfield populated and is green
10 - Click "Link authorities automatically" again
     Dialog says "No authority link was changed"
11 - In another tab go to System preferences and set AutoCreateAuthorities to 'Generate'
12 - Add random information to the 650 field
13 - Click 'Link authorities' button
14 - Dialog says:650 - No matching authority found. A new authority was created automatically.
15 - The subfield 9 is green and has the id of the new authority record
16 - In another tab search authorities and find an existing subject heading
17 - Add a new 650 with the info from the existing record
18 - Click 'Link authorities'
19 - The new field is correctly linked to existing authority

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 112131 [details] [review]
Bug 11299: (QA follow-up) Fix tests, spaces, and CSS update

Removed my from variables in test

Undid stray line deletions

Undid adding 1; to test file

Fix CSS replace command

Update license

Fix spelling

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 112132 [details] [review]
Bug 11299: (follow-up) Correct rebase errors and add message class to dialog

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 112133 [details] [review]
Bug 11299: (follow-up) Skip fields with no 9 subfields

To test:
1 - Disable the 'thesaurus' for a controlled field like 600a or 650a
2 - Edit a record and put random data in this field
3 - Try to link, get a JS error
4 - Apply patch
5 - Repeat
6 - No more error, field is not linked (as it has no $9 subfield)

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 112134 [details] [review]
Bug 11299: (follow-up) correct link

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 112135 [details] [review]
Bug 11299: Fix indirect object notation

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

I retested and added a little follow-up for the indirect object notation :)

There are several conflicts, please rebase.

Note the LinkBibHeadingsToAuthorities new parameter $tagtolink added by bug 26641.

I tried to rebase with the new parameter $tagtolink.
I just put the new parameter $tagtolink to undef when calling the function LinkBibHeadingsToAuthorities in BiblioAutoLink.
With my rebase and when I'am following the test plan on comment #169, I have this error :
<pre>
Uncaught TypeError: subfield[0] is undefined
</pre>
And I am not able to fix it.

Nick, could you maybe have another look here?

Created attachment 113562 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 1 - In your system preferences set:
    AutoCreateAuthorities: Don't generate
    BiblioAddsAuthorities: Allow
 2 - Go to the Cataloging -> New record (koha/cataloguing/addbiblio.pl)
    Ensure you are using the basic editor
 3 - Click the "Link authorities automatically" button.
    A message should appear, telling the user "No authority link was changed."
 4 - Add random informations in field 600$a of the biblio record.
 5 - Click the "Link authorities automatically" button.
   the message box should now show  "600 - No matching authority found.".
   the 9 subfield is red
   Above the 9 subfield is a red X with a blue plus next to it
   Hover on the plus, see it is titled 'Create authority'
 6 - Click the 'Create authority' link
 7 - A new authroity form pops up, the info from the cataloging editor is prefilled
     Click the 100 field heading to expand and confirm info is transferred
 8 - Fill in necessary fields and save the new authority
 9 - The cataloging screen now has the 9 subfield populated and is green
10 - Click "Link authorities automatically" again
     Dialog says "No authority link was changed"
11 - In another tab go to System preferences and set AutoCreateAuthorities to 'Generate'
12 - Add random information to the 650 field
13 - Click 'Link authorities' button
14 - Dialog says:650 - No matching authority found. A new authority was created automatically.
15 - The subfield 9 is green and has the id of the new authority record
16 - In another tab search authorities and find an existing subject heading
17 - Add a new 650 with the info from the existing record
18 - Click 'Link authorities'
19 - The new field is correctly linked to existing authority

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 113563 [details] [review]
Bug 11299: (QA follow-up) Fix tests, spaces, and CSS update

Removed my from variables in test

Undid stray line deletions

Undid adding 1; to test file

Fix CSS replace command

Update license

Fix spelling

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 113564 [details] [review]
Bug 11299: (follow-up) Correct rebase errors and add message class to dialog

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 113565 [details] [review]
Bug 11299: (follow-up) Skip fields with no 9 subfields

To test:
1 - Disable the 'thesaurus' for a controlled field like 600a or 650a
2 - Edit a record and put random data in this field
3 - Try to link, get a JS error
4 - Apply patch
5 - Repeat
6 - No more error, field is not linked (as it has no $9 subfield)

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 113566 [details] [review]
Bug 11299: (follow-up) correct link

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 113567 [details] [review]
Bug 11299: Fix indirect object notation

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

Created attachment 113568 [details] [review]
Bug 11299: (follow-up) Rebase on recent changes

Sounds quite useful!

1.
The green/red color switch does not seem to work.

https://snipboard.io/P6BZCx.jpg

To recreate:
xxx does not exist
❤❤❤ does exist

insert "xxx" in $a, click the "Link" button
=> red
modify with "❤❤❤" in $a, click the "Link" button
=> still red but $9 if filled in with the authid

2.
 8 - Fill in necessary fields and save the new authority
 9 - The cataloging screen now has the 9 subfield populated and is green
Here I don't have $9 prefilled. Note that the popup is still opened.
I have to click "Link" again and then it's filled in, but it's not what the test plan said. (Note that background is still red)

3. What about the following patch I am about to submit?

Created attachment 114803 [details] [review]
Bug 11299: Adjust call to check_cookie_auth

Why were we using CGI::Cookie?

(In reply to Jonathan Druart from comment #188)
> Created attachment 114803 [details] [review] [review]
> Bug 11299: Adjust call to check_cookie_auth
> 
> Why were we using CGI::Cookie?

That's a good question. My guess would be copy/paste from an existing script. 

It looks like CGI::Cookie is still used explicitly in 17 files. No idea why since https://metacpan.org/pod/CGI::Cookie itself says that CGI is already using it internally anyway.

I've opened bug 27337 for the existing calls.

Created attachment 114924 [details] [review]
Bug 11299: Add a button to the biblio edition page to automatically add authority links in the current biblio record via AJAX. Also adds a button to easily create missing authority records.

add authority type in the form to create the missing  authority.
 when authority was found, the 600$9 field have the authid.

Testing scenario (Creating an authority record for a failed automatic link) :

 1 - In your system preferences set:
    AutoCreateAuthorities: Don't generate
    BiblioAddsAuthorities: Allow
 2 - Go to the Cataloging -> New record (koha/cataloguing/addbiblio.pl)
    Ensure you are using the basic editor
 3 - Click the "Link authorities automatically" button.
    A message should appear, telling the user "No authority link was changed."
 4 - Add random informations in field 600$a of the biblio record.
 5 - Click the "Link authorities automatically" button.
   the message box should now show  "600 - No matching authority found.".
   the 9 subfield is red
   Above the 9 subfield is a red X with a blue plus next to it
   Hover on the plus, see it is titled 'Create authority'
 6 - Click the 'Create authority' link
 7 - A new authroity form pops up, the info from the cataloging editor is prefilled
     Click the 100 field heading to expand and confirm info is transferred
 8 - Fill in necessary fields and save the new authority
 9 - The cataloging screen now has the 9 subfield populated and is green
10 - Click "Link authorities automatically" again
     Dialog says "No authority link was changed"
11 - In another tab go to System preferences and set AutoCreateAuthorities to 'Generate'
12 - Add random information to the 650 field
13 - Click 'Link authorities' button
14 - Dialog says:650 - No matching authority found. A new authority was created automatically.
15 - The subfield 9 is green and has the id of the new authority record
16 - In another tab search authorities and find an existing subject heading
17 - Add a new 650 with the info from the existing record
18 - Click 'Link authorities'
19 - The new field is correctly linked to existing authority

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

https://bugs.koha-community.org/show_bug.cgi?id=12299
Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Michal Denar <black23@gmail.com>

Created attachment 114925 [details] [review]
Bug 11299: (QA follow-up) Fix tests, spaces, and CSS update

Removed my from variables in test

Undid stray line deletions

Undid adding 1; to test file

Fix CSS replace command

Update license

Fix spelling

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 114926 [details] [review]
Bug 11299: (follow-up) Correct rebase errors and add message class to dialog

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 114927 [details] [review]
Bug 11299: (follow-up) Skip fields with no 9 subfields

To test:
1 - Disable the 'thesaurus' for a controlled field like 600a or 650a
2 - Edit a record and put random data in this field
3 - Try to link, get a JS error
4 - Apply patch
5 - Repeat
6 - No more error, field is not linked (as it has no $9 subfield)

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 114928 [details] [review]
Bug 11299: (follow-up) correct link

Signed-off-by: Michal Denar <black23@gmail.com>

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

Created attachment 114929 [details] [review]
Bug 11299: Fix indirect object notation

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

Created attachment 114930 [details] [review]
Bug 11299: (follow-up) Rebase on recent changes

Created attachment 114931 [details] [review]
Bug 11299: Adjust call to check_cookie_auth

Why were we using CGI::Cookie?

Created attachment 114932 [details] [review]
Bug 11299: (follow-up) adjust tests for bug 26641, fix styling, fix population of new record

This corrects parameters in tests to adjust for new parameter in LinkBibHeadings

We also add lines to remove classes before adding the correct class while linking and restore
a missing class to the icons

Lastly we correct fetching the paramters to pass to authority creation popup to ensure
values are populated on save

(In reply to Jonathan Druart from comment #187)
> 1.
> The green/red color switch does not seem to work.
> 
> https://snipboard.io/P6BZCx.jpg
> 
> To recreate:
> xxx does not exist
> ❤❤❤ does exist
> 
> insert "xxx" in $a, click the "Link" button
> => red
> modify with "❤❤❤" in $a, click the "Link" button
> => still red but $9 if filled in with the authid
> 
> 2.
>  8 - Fill in necessary fields and save the new authority
>  9 - The cataloging screen now has the 9 subfield populated and is green
> Here I don't have $9 prefilled. Note that the popup is still opened.
> I have to click "Link" again and then it's filled in, but it's not what the
> test plan said. (Note that background is still red)
> 
> 3. What about the following patch I am about to submit?

1 - fixed! We forgot to remove old classes
2 - fixed! needed 'find' instead of 'children'
3 - Ok!

Two more things (sorry!)

1. Shouldn't we show/hide the "link auth automatically" button depending on AutoCreateAuthorities and BiblioAddsAuthorities?
2. 
Add a new bib:
600$a=xxx, click link, create the authority, save
Edit the bib
600$a$9 are correct, but then edit $a with yyy, click link
=> Automatic authority link results:
    No authority link was changed.

That seems wrong.

(In reply to Jonathan Druart from comment #201)
> Two more things (sorry!)
> 
> 1. Shouldn't we show/hide the "link auth automatically" button depending on
> AutoCreateAuthorities and BiblioAddsAuthorities?
> 2. 
> Add a new bib:
> 600$a=xxx, click link, create the authority, save
> Edit the bib
> 600$a$9 are correct, but then edit $a with yyy, click link
> => Automatic authority link results:
>     No authority link was changed.
> 
> That seems wrong.

1 - No, we should always have the button - the settings will just determine whether the tool creates new records or not

2 - Try adjusting 'CatalogModuleRelink' - if set to Don't then the auth isn't updated. If set to 'Do' then it should

Pushed to master for 21.05, thanks to everybody involved!

(In reply to Jonathan Druart from comment #203)
> Pushed to master for 21.05, thanks to everybody involved!

Awesome!

Enhancement not pushed to 20.11.x