Jonathan,

I'm not sure if there's a consensus re constructors in the Koha namespace and use of Class::Accessor. I believe it is rarely necessary to provide new() in those cases.

I suggest:
* remove new() constructors all together, Koha::AuthorisedValue->new({av => $av}) is unnecessary cause it is equivalent to Koha::AuthorisedValue->new($av}
* upgrade fetch(key) to constructors, passing the key values; this way Koha::AuthorisedValues->new() becomes Koha::AuthorisedValues->fetch()
* if you need to reload(), make those methods, and make reload() and fetch() call _fetch(key)

In general, has there been a discussion on using DBIx::Class or similar?

(In reply to comment #4)

Srdjan,

> I'm not sure if there's a consensus re constructors in the Koha namespace
> and use of Class::Accessor. I believe it is rarely necessary to provide
> new() in those cases.

Class:Accessor is already used in many places in the Koha namespace.
 
> I suggest:
> * remove new() constructors all together, Koha::AuthorisedValue->new({av =>
> $av}) is unnecessary cause it is equivalent to
> Koha::AuthorisedValue->new($av}

The Koha::AuthorisedValue->new can be called directly with the $av (which contains all values) or just with an id or category/value. These different ways to build an object forced me to have a "new" routine. But maybe I don't see concretely what you say :-/

> * upgrade fetch(key) to constructors, passing the key values; this way
> Koha::AuthorisedValues->new() becomes Koha::AuthorisedValues->fetch()

Yes but, like as before, sometimes we don't want to do a select in DB (when the object is called from Koha::AuthorisedValues).

> * if you need to reload(), make those methods, and make reload() and fetch()
> call _fetch(key)

I think I don't have the need to reload.

> In general, has there been a discussion on using DBIx::Class or similar?

Yes, but it seems that nobody strongly supports the integration into Koha.

Hm, as there is some progress on DBIX integration now, I think it might be worth to put some more thought into this. Putting this "In Discussion" to get some more opinions.

(In reply to Katrin Fischer from comment #9)
> Hm, as there is some progress on DBIX integration now, I think it might be
> worth to put some more thought into this. Putting this "In Discussion" to
> get some more opinions.

Katrin,
Even if someone uses DBIx for authorised values, I think this patch is useful, it refactores dirty code in admin/authorised_values.pl and adds unit tests.

Galen, could you give your opinion on this patch please (see comment 9 and comment 10)?

(In reply to Katrin Fischer from comment #9)
> Hm, as there is some progress on DBIX integration now, I think it might be
> worth to put some more thought into this. Putting this "In Discussion" to
> get some more opinions.

It appears that no opinion comes, so must this patch be lost in the hole of the discussion status? I would prefer to change the status in order to others developers see it on the bugs list.
I worked some hours on this refactoring and, as I already said, even if someone wants to write a package using DBIx for authorised values this patch will help him/her!
I added unit tests to the new package, I clarified a lot of dirty code: I don't understand the goal to block this patch in discussion for a long time.

Hi Jonathan, I didn't intend to block it for a long time - just seeking out opinions. Maybe 'adding new modules' could be discussed next Tuesday? I have filed a bug for categorie.pl yesterday - which also needs a new module.

I take the liberty of switching back the status to Needs Signoff.

Patch applied cleanly, go forth and signoff

Just a trivial rebase (updatedatabase)

[Preliminary] QA Comment:
First, compliments to Jonathan for his work in getting the code more organized. This report has been pending for some time already in the queue. Several people (including myself) may have been somewhat hesistant to give comments here. I hope to trigger some discussion again in order to get this report further.

Somehow I am not completely happy with the proposed design: two objects for 1) one authorized value and 2) several ones. It got me thinking: Do we really need the singular object? Should we focus to category or collection?
And referring to an earlier comment, if we would have DBIC in-between, what still is the additional value of what we have now?

Again referring to above (and as a general remark), it is confusing that we already have three or more ways to define objects in the new Koha namespace. You choose for Class:Accessor, but with a lot of additional handwork (blessing, SUPER:: calls, etc.) Somehow the current design makes the impression that the choice for OO here was not so obvious. See also earlier discussion on the new constructor. Why should your object have a fetch method, etc.? (Similar questions for e.g. a filter or all method.)

Glancing at the branches_limitation related code in class and script, I wonder if your object(s) should expose that as is done currently. Just stimulating your thoughts about improving design again :)

As a preliminary conclusion, I think this needs some more discussion and I would not recommend to move these modules to the Koha folder in the current form (design and DBIC argument-wise). But I would not oppose to move them to C4 (as a compromise) and use them as base for improvement from there. What do others think?

Changing status to trigger some discussion. (Jonathan: Feel free to ask the dev list for feedback.) Will try to attract the attention from Galen too..

Hello Marcel,

Thank you very much for your attention on these patches.
The 2 packages are certainly not perfect but they group the code related to AV and add unit tests.
And even if DBIx::Class is coming, this refactoring will make easier its utilisation for AV.
When a patch improves current code (especially when it's refactoring) and does not introduce regression, I think it is enough to let it go ahead.

As I already said, I developped them on my spare time. I don't plan to spend more time for a rewrite.
If a further need on this refactoring is small, I will, of course, provide follow-ups, but I won't rewrite them.

That's why, I remove myself from the assignee.

Please don't see any offense in this answer, I really appreciated your constructive comment!

See also bug 9967

I've applied the patches against master 3.17.00.013

The patch "DB Changes: ..." needs to be rebased.
So I pass the patch to "Patch doesn't apply" status.

Going on, a category created without AV is managed differently against before.

About the new category "NEWCAT" I've just created, koha lists 1 "empty" value to "Edit" or "Delete".

So, in order to set an AV for the new category (as the 5/ step of the plan requires), I'd better "Edit" this empty value, rather than adding a new AV.

Moreover, if I pretty delete the "empty" value, the new category disappears from the list "Show category:".

On the contrary, about the old category "Asort1" without AV koha still says like before applying:

Authorized values for category Asort1:
There are no authorized values defined for Asort1

(In reply to Paola Rossi from comment #32)
> I've applied the patches against master 3.17.00.013
> 
> The patch "DB Changes: ..." needs to be rebased.
> So I pass the patch to "Patch doesn't apply" status.

Not necessarily, it's a common conflict, easy to solve.
And is the only one

> On the contrary, about the old category "Asort1" without AV koha still says
> like before applying:
> 
> Authorized values for category Asort1:
> There are no authorized values defined for Asort1

But that is the result of trick :)
AV categories are extracted from DB, but some hardcoded values are appended to the list and are not into the DB, search form example Asort1 on authorised_values table, there is none.

So it's normal that when you delete the last AV then category is also deleted,
except for that hardcoded values.

I have refactored the work of Jonathan in order to adapt it to the different comments I have read on this bug report.
I have tried to find the best compromise between organization, heaviness and simplicity to finally integrate this improvement in Koha.

Do either of these account for the caching that ought to be taking place with authorised values?

The current method in C4::Koha uses memoise, new stuff should use Koha::Cache.

Just another try: https://git.biblibre.com/biblibre/kohac/commits/ft/bug_10363 wip

(In reply to Kyle M Hall from comment #43)
> Created attachment 32634 [details] [review] [review]

I can't apply your patch on a clean Git repo. I get:

fatal: sha1 information is lacking or useless (Koha/Object.pm).

Hi Frederic, did you try to apply bug 13019 first? We are looking for a sign-off on that one, as it's blocking a few things now.

(In reply to Katrin Fischer from comment #45)
> Hi Frederic, did you try to apply bug 13019 first? We are looking for a
> sign-off on that one, as it's blocking a few things now.

Sorry. My fault. I hadn't noted this bug depends on bug 13019.

(In reply to Jonathan Druart from comment #50)
> Created attachment 33423 [details] [review] [review]
> Bug 10363: Use Koha::AuthorisedValue[s] in the admin page

Kyle,
Can I get you signoff on this patch please?

Comment on attachment 33862 [details] [review]
Bug 10363 [QA Followup]

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

Hi Kyle,
Thanks for the followup!

::: admin/authorised_values.pl
@@ +113,4 @@
>          $av->imageurl( $imageurl );
>          eval{
>              $av->store;
> +            $av->replace_branch_limitations( \@branches );

Could you explain what was wrong here?

Comment on attachment 33860 [details] [review]
[SIGNED-OFF] Bug 10363 - There is no package for authorised values.

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

::: Koha/AuthorisedValue.pm
@@ +144,5 @@
> +
> +    return $self->lib_opac() || $self->lib();
> +}
> +
> +=head3 Koha::Objects->_resultset

Copy/paste error?

::: Koha/AuthorisedValues.pm
@@ +22,5 @@
> +use Carp;
> +
> +use Koha::Database;
> +
> +use Koha::Borrower;

Copy/paste error?

@@ +28,5 @@
> +use base qw(Koha::Objects);
> +
> +=head1 NAME
> +
> +Koha::Borrower - Koha Borrower Object class

Copy/paste error?

@@ +68,5 @@
> +}
> +
> +=head3 type
> +
> +=cut

Why do we need type() and object_class() methods?

Ahh, I see now that type() is used in the Koha::Object constructor...

(In reply to David Cook from comment #56)
> Comment on attachment 33860 [details] [review] [review]
> [SIGNED-OFF] Bug 10363 - There is no package for authorised values.
> 
> Review of attachment 33860 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/AuthorisedValue.pm
> @@ +144,5 @@
> > +
> > +    return $self->lib_opac() || $self->lib();
> > +}
> > +
> > +=head3 Koha::Objects->_resultset
> 
> Copy/paste error?

Fixed.

> ::: Koha/AuthorisedValues.pm
> @@ +22,5 @@
> > +use Carp;
> > +
> > +use Koha::Database;
> > +
> > +use Koha::Borrower;
> 
> Copy/paste error?

Fixed.

> @@ +28,5 @@
> > +use base qw(Koha::Objects);
> > +
> > +=head1 NAME
> > +
> > +Koha::Borrower - Koha Borrower Object class
> 
> Copy/paste error?

Already fixed in a previous patch.

Fix pod?

-Koha::Borrower - Koha Borrower Object class
+Koha::AuthorisedValues - Koha Borrower Object set class
 
:-)

(In reply to M. de Rooy from comment #60)
> Fix pod?

At least tried...

Comment on attachment 35838 [details] [review]
Bug 10363 - There is no package for authorised values.

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

::: installer/data/mysql/updatedatabase.pl
@@ +9766,5 @@
> +            CHANGE av_id av_id INT( 11 ) NOT NULL,
> +            CHANGE branchcode branchcode VARCHAR( 10 ) NOT NULL
> +    });
> +    print "Upgrade to $DBversion done ( Bug 10363 - There is no package for authorised values. )\n";
> +    SetVersion($DBversion);

Shouldn't this be an atomicupdate now?

We just changed the guidelines, I think for old patches already in the QA the old database updates are not a problem at all. Note: this is the form the RM will convert them to anyway.

There is a bug in there:

1) Create or edit an authorized value
2) Limit to 2+ branches (I had 5)
3) Try to edit the authorized value

Software error:

Can't use string ("CPL") as a HASH ref while "strict refs" in use at /home/katrin/kohaclone/admin/authorised_values.pl line 63.

For help, please send mail to the webmaster (webmaster@bumblebee), giving this error message and the time and date of the error.

Hm also: the duplicate check is not working.

1) Enter an entry to any authorized value category with value new
2) Try to enter another with value new for the same category
3) on master: an error message is shown 
   with the patches: authorized value saved successfully...

FWIW, it works perfectly on my local branch, but it is ... 1440 commits behind master...

Maybe something got lost in a rebase? I think the needed fix is probably something small, but I didn't understand the code enough to make it.

Sorry, but I still see the duplicate bug - last patch doesn't seem to work?

Try changing an existing values description, you will receive:
"This authorized value already exists."
The change is not saved.

Some small note for someone returning to this: I like 'added' better than 'inserted' in the error messages, as it matches the other terminology in Koha better.

Hm, I think we are also missing a kohastructure.sql change here?

(In reply to Katrin Fischer from comment #82)
> Sorry, but I still see the duplicate bug - last patch doesn't seem to work?
> 
> Try changing an existing values description, you will receive:
> "This authorized value already exists."
> The change is not saved.

It was not exactly the same issue, good catch.
Should be fixed now.

> Some small note for someone returning to this: I like 'added' better than
> 'inserted' in the error messages, as it matches the other terminology in
> Koha better.

Fixed.

(In reply to Katrin Fischer from comment #83)
> Hm, I think we are also missing a kohastructure.sql change here?

Added.

Created attachment 43550 [details] [review]
[PASSED QA] Bug 10363: There is no package for authorised values.

Test Plan:
1) Apply this patch
2) run updatedatabase.pl
3) prove t/db_dependent/AuthorisedValues.t

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Created attachment 43551 [details] [review]
[PASSED QA] Bug 10363: Use Koha::AuthorisedValue[s] in the admin page

Now we have packages, we need use them in the pl script.

Test plan:
Verify there are no regression on addind/editing/deleting authorised
values.
Done forget to test the branch limitation.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Created attachment 43552 [details] [review]
[PASSED QA] Bug 10363: [QA Followup]

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Created attachment 43553 [details] [review]
[PASSED QA] Bug 10363: Fix pod

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Created attachment 43554 [details] [review]
[PASSED QA] Bug 10363: (QA followup) DB update / atomic update

This patch just moves the DB update code into an atomicupdate file
to avoid nasty merge conflicts.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Created attachment 43555 [details] [review]
[PASSED QA] Bug 10363: QA Follow-up - fixing POD and spelling

We should stick with either AE (authorized) or
BE (authorised) spelling.

Corrected error messages to use authorized.

Also fixed tiny copy & paste error in POD.

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

Created attachment 43556 [details] [review]
[PASSED QA] Bug 10363: FIX rebase conflict error

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

Created attachment 43557 [details] [review]
[PASSED QA] Bug 10363: FIX regression - prevent duplicate

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

Created attachment 43558 [details] [review]
[PASSED QA] Bug 13636: Change wording added vs inserted

http://bugs.koha-community.org/show_bug.cgi?id=10363

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

Created attachment 43559 [details] [review]
[PASSED QA] Bug 10363: (follow-up) DB update - change in kohastructure.sql

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

Over 2 years... but finally!

Patches pushed to master.

Great job Jonathan!

Created attachment 43696 [details] [review]
Bug 10363: Fix tests

The delete op is now "delete" and the confirmation is a JS dialog box.

Note that now this patch removes the categories the tests will add (to
avoid error on inserting duplicate).

Created attachment 43827 [details] [review]
Bug 10363: Fix Updatedb entry on MariaDB

With MariaDB, I get the following error:

ERROR 1832 (HY000): Cannot change column 'av_id': used in a foreign key
constraint 'authorised_values_branches_ibfk_1'

The solution would be to remove the constraints, modify the columns and
finally reintroduce the foreign keys.

Followup pushed to master.

Thanks for taking care, Jonathan!