Bug 10363 - Move authorised value related code into its own package
Summary: Move authorised value related code into its own package
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 13019
Blocks: 10198 15799
  Show dependency treegraph
 
Reported: 2013-05-28 13:44 UTC by Jonathan Druart
Modified: 2017-06-14 22:11 UTC (History)
16 users (show)

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


Attachments
Bug 10363: Add 2 packages for authorised values (37.43 KB, patch)
2013-05-28 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363: DB Changes: Adds a unique key for the authorised values table. (1.72 KB, patch)
2013-05-28 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363: Adds unit tests for Koha::AuthorisedValue[s] packages (6.11 KB, patch)
2013-05-28 14:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363: Add 2 packages for authorised values (37.49 KB, patch)
2013-06-07 13:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10363: DB Changes: Adds a unique key for the authorised values table. (1.79 KB, patch)
2013-06-07 13:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10363: Adds unit tests for Koha::AuthorisedValue[s] packages (6.18 KB, patch)
2013-06-07 13:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10363: UT: execute tests into a transaction (1.22 KB, patch)
2013-08-30 07:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10363: Add 2 packages for authorised values (37.84 KB, patch)
2013-10-06 17:17 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10363: DB Changes: Adds a unique key for the authorised values table. (1.83 KB, patch)
2013-10-06 17:17 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10363: Adds unit tests for Koha::AuthorisedValue[s] packages (6.24 KB, patch)
2013-10-06 17:17 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10363: UT: execute tests into a transaction (1.29 KB, patch)
2013-10-06 17:17 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 10363: Add 2 packages for authorised values (37.87 KB, patch)
2014-02-14 17:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10363: DB Changes: Adds a unique key for the authorised values table. (1.90 KB, patch)
2014-02-14 17:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10363: Adds unit tests for Koha::AuthorisedValue[s] packages (6.28 KB, patch)
2014-02-14 17:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10363: UT: execute tests into a transaction (1.32 KB, patch)
2014-02-14 17:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10363: Adds unit tests for Koha::AuthorisedValue package (5.85 KB, patch)
2014-08-07 14:42 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 10363: DB Changes: Adds a unique key for the authorised values table. (2.74 KB, patch)
2014-08-07 14:43 UTC, Yohann Dufour
Details | Diff | Splinter Review
Bug 10363: Add a package for authorised values (31.83 KB, patch)
2014-08-07 14:43 UTC, Yohann Dufour
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10363: Adds unit tests for Koha::AuthorisedValue package (5.98 KB, patch)
2014-08-11 16:22 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10363: DB Changes: Adds a unique key for the authorised values table. (2.89 KB, patch)
2014-08-11 16:22 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10363: Add a package for authorised values (32.04 KB, patch)
2014-08-11 16:22 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[ALTERNATE POC] Bug 10363 - There is no package for authorised values. (9.45 KB, patch)
2014-09-12 17:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10363 - There is no package for authorised values. (11.07 KB, patch)
2014-10-23 16:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10363 - There is no package for authorised values. (13.32 KB, patch)
2014-10-23 16:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10363 - There is no package for authorised values. (13.31 KB, patch)
2014-10-23 17:06 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13019 - Add base classes on which to build Koha objects (23.05 KB, patch)
2014-11-10 13:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13019: (follow-up) Remove smartmatch operator (4.16 KB, patch)
2014-11-10 13:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363 - There is no package for authorised values. (13.37 KB, patch)
2014-11-10 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363: Use Koha::AuthorisedValue[s] in the admin page (27.95 KB, patch)
2014-11-10 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10363 - There is no package for authorised values. (13.44 KB, patch)
2014-11-24 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10363: Use Koha::AuthorisedValue[s] in the admin page (28.14 KB, patch)
2014-11-24 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10363 [QA Followup] (2.61 KB, patch)
2014-11-24 14:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10363: Fix pod (1.13 KB, patch)
2014-11-25 08:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363: Fix pod (1.14 KB, patch)
2014-11-26 13:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363 - There is no package for authorised values. (13.55 KB, patch)
2015-02-11 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10363: Use Koha::AuthorisedValue[s] in the admin page (28.20 KB, patch)
2015-02-11 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10363 [QA Followup] (2.08 KB, patch)
2015-02-11 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10363: Fix pod (1.14 KB, patch)
2015-02-11 15:29 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10363: There is no package for authorised values. (12.48 KB, patch)
2015-07-31 18:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10363: Use Koha::AuthorisedValue[s] in the admin page (28.26 KB, patch)
2015-07-31 18:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10363: [QA Followup] (2.14 KB, patch)
2015-07-31 18:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10363: Fix pod (1.20 KB, patch)
2015-07-31 18:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED OFF] Bug 10363: (QA followup) DB update / atomic update (1.39 KB, patch)
2015-07-31 18:31 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10363: QA Follow-up - fixing POD and spelling (2.83 KB, patch)
2015-08-02 18:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10363: FIX rebase conflict error (1005 bytes, patch)
2015-08-04 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363: FIX prevent duplicate (2.48 KB, patch)
2015-08-04 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363: FIX regression - prevent duplicate (2.49 KB, patch)
2015-08-04 13:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363: FIX regression - prevent duplicate (2.53 KB, patch)
2015-10-05 08:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13636: Change wording added vs inserted (1.14 KB, patch)
2015-10-05 08:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363: (follow-up) DB update - change in kohastructure.sql (1.14 KB, patch)
2015-10-05 08:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 10363: There is no package for authorised values. (12.54 KB, patch)
2015-10-18 09:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10363: Use Koha::AuthorisedValue[s] in the admin page (28.32 KB, patch)
2015-10-18 09:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10363: [QA Followup] (2.20 KB, patch)
2015-10-18 09:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10363: Fix pod (1.26 KB, patch)
2015-10-18 09:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10363: (QA followup) DB update / atomic update (1.44 KB, patch)
2015-10-18 09:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10363: QA Follow-up - fixing POD and spelling (2.90 KB, patch)
2015-10-18 09:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10363: FIX rebase conflict error (1.05 KB, patch)
2015-10-18 09:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10363: FIX regression - prevent duplicate (2.60 KB, patch)
2015-10-18 09:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 13636: Change wording added vs inserted (1.20 KB, patch)
2015-10-18 09:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10363: (follow-up) DB update - change in kohastructure.sql (1.21 KB, patch)
2015-10-18 09:03 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10363: Fix tests (4.79 KB, patch)
2015-10-21 14:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10363: Fix Updatedb entry on MariaDB (1.75 KB, patch)
2015-10-23 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-05-28 13:44:07 UTC
We should have a package to manage authorised values.
Comment 1 Jonathan Druart 2013-05-28 14:11:24 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2013-05-28 14:11:27 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-05-28 14:11:30 UTC Comment hidden (obsolete)
Comment 4 Srdjan Jankovic 2013-06-04 01:23:49 UTC
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?
Comment 5 Jonathan Druart 2013-06-07 09:30:19 UTC
(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.
Comment 6 Kyle M Hall 2013-06-07 13:50:56 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2013-06-07 13:51:06 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2013-06-07 13:51:12 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2013-07-12 22:08:57 UTC
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.
Comment 10 Jonathan Druart 2013-07-15 10:19:04 UTC
(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.
Comment 11 Jonathan Druart 2013-07-18 12:28:00 UTC
Galen, could you give your opinion on this patch please (see comment 9 and comment 10)?
Comment 12 Jonathan Druart 2013-08-30 07:42:09 UTC
(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.
Comment 13 Jonathan Druart 2013-08-30 07:47:57 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2013-08-30 07:48:43 UTC
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.
Comment 15 Jonathan Druart 2013-10-04 12:07:30 UTC
I take the liberty of switching back the status to Needs Signoff.
Comment 16 I'm just a bot 2013-10-05 01:21:24 UTC
Patch applied cleanly, go forth and signoff
Comment 17 Bernardo Gonzalez Kriegel 2013-10-06 17:17:06 UTC Comment hidden (obsolete)
Comment 18 Bernardo Gonzalez Kriegel 2013-10-06 17:17:17 UTC Comment hidden (obsolete)
Comment 19 Bernardo Gonzalez Kriegel 2013-10-06 17:17:25 UTC Comment hidden (obsolete)
Comment 20 Bernardo Gonzalez Kriegel 2013-10-06 17:17:40 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2014-02-14 17:49:37 UTC Comment hidden (obsolete)
Comment 22 Marcel de Rooy 2014-02-14 17:49:43 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2014-02-14 17:49:47 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2014-02-14 17:49:52 UTC Comment hidden (obsolete)
Comment 25 Marcel de Rooy 2014-02-14 17:50:17 UTC
Just a trivial rebase (updatedatabase)
Comment 26 Marcel de Rooy 2014-02-14 19:48:32 UTC
[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..
Comment 27 Jonathan Druart 2014-02-15 11:40:53 UTC
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!
Comment 28 Robin Sheat 2014-04-09 04:35:00 UTC
See also bug 9967
Comment 29 Yohann Dufour 2014-08-07 14:42:05 UTC Comment hidden (obsolete)
Comment 30 Yohann Dufour 2014-08-07 14:43:00 UTC Comment hidden (obsolete)
Comment 31 Yohann Dufour 2014-08-07 14:43:34 UTC Comment hidden (obsolete)
Comment 32 Paola Rossi 2014-08-11 12:27:27 UTC
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
Comment 33 Bernardo Gonzalez Kriegel 2014-08-11 16:22:17 UTC Comment hidden (obsolete)
Comment 34 Bernardo Gonzalez Kriegel 2014-08-11 16:22:24 UTC Comment hidden (obsolete)
Comment 35 Bernardo Gonzalez Kriegel 2014-08-11 16:22:38 UTC Comment hidden (obsolete)
Comment 36 Bernardo Gonzalez Kriegel 2014-08-11 16:30:28 UTC
(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.
Comment 37 Yohann Dufour 2014-08-18 14:09:17 UTC
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.
Comment 38 Kyle M Hall 2014-09-12 17:06:07 UTC Comment hidden (obsolete)
Comment 39 Robin Sheat 2014-09-14 23:09:14 UTC
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.
Comment 40 Jonathan Druart 2014-10-10 15:12:14 UTC
Just another try: https://git.biblibre.com/biblibre/kohac/commits/ft/bug_10363 wip
Comment 41 Kyle M Hall 2014-10-23 16:45:27 UTC Comment hidden (obsolete)
Comment 42 Kyle M Hall 2014-10-23 16:46:31 UTC Comment hidden (obsolete)
Comment 43 Kyle M Hall 2014-10-23 17:06:00 UTC Comment hidden (obsolete)
Comment 44 Frédéric Demians 2014-10-27 08:35:16 UTC
(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).
Comment 45 Katrin Fischer 2014-10-27 08:38:03 UTC
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.
Comment 46 Frédéric Demians 2014-10-27 09:21:14 UTC
(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.
Comment 47 Jonathan Druart 2014-11-10 13:15:51 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2014-11-10 13:15:58 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2014-11-10 13:22:06 UTC Comment hidden (obsolete)
Comment 50 Jonathan Druart 2014-11-10 13:22:24 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2014-11-10 13:22:53 UTC
(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 52 Kyle M Hall 2014-11-24 14:46:29 UTC Comment hidden (obsolete)
Comment 53 Kyle M Hall 2014-11-24 14:46:39 UTC Comment hidden (obsolete)
Comment 54 Kyle M Hall 2014-11-24 14:46:42 UTC Comment hidden (obsolete)
Comment 55 Jonathan Druart 2014-11-24 14:55:36 UTC
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 56 David Cook 2014-11-25 05:30:29 UTC
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?
Comment 57 David Cook 2014-11-25 05:35:21 UTC
Ahh, I see now that type() is used in the Koha::Object constructor...
Comment 58 Jonathan Druart 2014-11-25 08:11:31 UTC Comment hidden (obsolete)
Comment 59 Jonathan Druart 2014-11-25 08:12:24 UTC
(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.
Comment 60 Marcel de Rooy 2014-11-26 12:57:27 UTC
Fix pod?

-Koha::Borrower - Koha Borrower Object class
+Koha::AuthorisedValues - Koha Borrower Object set class
 
:-)
Comment 61 Jonathan Druart 2014-11-26 13:05:37 UTC Comment hidden (obsolete)
Comment 62 Jonathan Druart 2014-11-26 13:06:01 UTC
(In reply to M. de Rooy from comment #60)
> Fix pod?

At least tried...
Comment 63 Kyle M Hall 2015-02-11 15:29:27 UTC Comment hidden (obsolete)
Comment 64 Kyle M Hall 2015-02-11 15:29:36 UTC Comment hidden (obsolete)
Comment 65 Kyle M Hall 2015-02-11 15:29:40 UTC Comment hidden (obsolete)
Comment 66 Kyle M Hall 2015-02-11 15:29:44 UTC Comment hidden (obsolete)
Comment 67 Mark Tompsett 2015-04-14 05:07:03 UTC
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?
Comment 68 Katrin Fischer 2015-04-14 07:19:58 UTC
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.
Comment 69 Tomás Cohen Arazi 2015-07-31 18:31:01 UTC Comment hidden (obsolete)
Comment 70 Tomás Cohen Arazi 2015-07-31 18:31:14 UTC Comment hidden (obsolete)
Comment 71 Tomás Cohen Arazi 2015-07-31 18:31:27 UTC Comment hidden (obsolete)
Comment 72 Tomás Cohen Arazi 2015-07-31 18:31:40 UTC Comment hidden (obsolete)
Comment 73 Tomás Cohen Arazi 2015-07-31 18:31:50 UTC Comment hidden (obsolete)
Comment 74 Katrin Fischer 2015-08-02 18:42:26 UTC Comment hidden (obsolete)
Comment 75 Katrin Fischer 2015-08-02 18:43:10 UTC
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.
Comment 76 Katrin Fischer 2015-08-02 18:56:02 UTC
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...
Comment 77 Jonathan Druart 2015-08-04 13:16:32 UTC
FWIW, it works perfectly on my local branch, but it is ... 1440 commits behind master...
Comment 78 Katrin Fischer 2015-08-04 13:25:30 UTC
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.
Comment 79 Jonathan Druart 2015-08-04 13:46:19 UTC Comment hidden (obsolete)
Comment 80 Jonathan Druart 2015-08-04 13:46:46 UTC Comment hidden (obsolete)
Comment 81 Jonathan Druart 2015-08-04 13:50:25 UTC Comment hidden (obsolete)
Comment 82 Katrin Fischer 2015-10-04 20:19:51 UTC
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.
Comment 83 Katrin Fischer 2015-10-04 20:22:49 UTC
Hm, I think we are also missing a kohastructure.sql change here?
Comment 84 Jonathan Druart 2015-10-05 08:43:05 UTC Comment hidden (obsolete)
Comment 85 Jonathan Druart 2015-10-05 08:43:09 UTC Comment hidden (obsolete)
Comment 86 Jonathan Druart 2015-10-05 08:43:14 UTC Comment hidden (obsolete)
Comment 87 Jonathan Druart 2015-10-05 08:45:55 UTC
(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.
Comment 88 Katrin Fischer 2015-10-18 09:02:30 UTC
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>
Comment 89 Katrin Fischer 2015-10-18 09:02:35 UTC
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>
Comment 90 Katrin Fischer 2015-10-18 09:02:40 UTC
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>
Comment 91 Katrin Fischer 2015-10-18 09:02:44 UTC
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>
Comment 92 Katrin Fischer 2015-10-18 09:02:49 UTC
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>
Comment 93 Katrin Fischer 2015-10-18 09:02:53 UTC
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>
Comment 94 Katrin Fischer 2015-10-18 09:02:58 UTC
Created attachment 43556 [details] [review]
[PASSED QA] Bug 10363: FIX rebase conflict error

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 95 Katrin Fischer 2015-10-18 09:03:02 UTC
Created attachment 43557 [details] [review]
[PASSED QA] Bug 10363: FIX regression - prevent duplicate

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 96 Katrin Fischer 2015-10-18 09:03:12 UTC
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>
Comment 97 Katrin Fischer 2015-10-18 09:03:16 UTC
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>
Comment 98 Katrin Fischer 2015-10-18 09:05:25 UTC
Over 2 years... but finally!
Comment 99 Tomás Cohen Arazi 2015-10-19 15:55:22 UTC
Patches pushed to master.

Great job Jonathan!
Comment 100 Jonathan Druart 2015-10-21 14:46:00 UTC
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).
Comment 101 Jonathan Druart 2015-10-23 13:21:38 UTC
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.
Comment 102 Tomás Cohen Arazi 2015-10-23 13:55:03 UTC
Followup pushed to master.

Thanks for taking care, Jonathan!