Bug 32154 - Missing primary key on erm_user_roles table
Summary: Missing primary key on erm_user_roles table
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Pedro Amorim
QA Contact: Testopia
URL:
Keywords:
Depends on: 32030
Blocks: 32163 32267
  Show dependency treegraph
 
Reported: 2022-11-09 17:42 UTC by Pedro Amorim
Modified: 2023-06-08 22:29 UTC (History)
6 users (show)

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


Attachments
Bug 32154: Missing primary key on erm_user_roles table (3.69 KB, patch)
2022-11-09 17:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32154: Missing primary key on erm_user_roles table (3.94 KB, patch)
2022-11-10 06:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32154: Missing primary key on erm_user_roles table (4.01 KB, patch)
2022-11-11 11:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32154: Fix Agreements.t (1.22 KB, patch)
2022-11-18 13:02 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 Pedro Amorim 2022-11-09 17:42:47 UTC
This is an outstanding issue related to the ERM module mentioned at:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32030#c31

Jonathan Druart:
TestBuilder.t is failing on ERM::User::Roles, it's missing a primary key on erm_user_roles I think
Can be tested with:

use t::lib::TestBuilder;
my $builder = t::lib::TestBuilder->new;
my $m = 'Koha::ERM::UserRoles';
eval "require $m";
my $object = $builder->build_object( { class => $m } ); 
say ref($object);

Should display "Koha::ERM::UserRole", not be empty
Comment 1 Pedro Amorim 2022-11-09 17:56:10 UTC
Created attachment 143565 [details] [review]
Bug 32154: Missing primary key on erm_user_roles table

The aforementioned TestBuilder should display "Koha::ERM::UserRole", not be empty. This patch adds a primary key to the erm_user_roles table and fixes that.
Also includes atomicupdate file to update database schema as well as updates to kohastructure.sql provided by Jonathan Druart.
Looked into the vue files but didn't find anywhere or a need to include the new primary key in any of the requests as there is no request of users by role_user_id being made.
Erm users are being created/updated as a relationship through licenceses/agreements. Furthermore, when editing a role of an existing user, this new primary key is already being supplied embeded in the license/agreement.
Comment 2 Jonathan Druart 2022-11-10 06:35:14 UTC
Created attachment 143620 [details] [review]
Bug 32154: Missing primary key on erm_user_roles table

The aforementioned TestBuilder should display "Koha::ERM::UserRole", not be empty. This patch adds a primary key to the erm_user_roles table and fixes that.
Also includes atomicupdate file to update database schema as well as updates to kohastructure.sql provided by Jonathan Druart.
Looked into the vue files but didn't find anywhere or a need to include the new primary key in any of the requests as there is no request of users by role_user_id being made.
Erm users are being created/updated as a relationship through licenceses/agreements. Furthermore, when editing a role of an existing user, this new primary key is already being supplied embeded in the license/agreement.

Test plan:
Add users to license and agreement
Run the cypress tests
Confirm that the change fix the test that was failing in TestBuilder.t
(another one may still fail however)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 3 Martin Renvoize 2022-11-10 12:06:38 UTC
Just thinking about this one, it feels to me like it should be a multi-field primary key rather than a new field entirely.

Having said that, it looks like we allow a user + licence + agreement combination to have multiple roles?  That would mean having all four fields in the primary key... food for thought.
Comment 4 Jonathan Druart 2022-11-11 08:01:58 UTC
(In reply to Martin Renvoize from comment #3)
> Just thinking about this one, it feels to me like it should be a multi-field
> primary key rather than a new field entirely.
> 
> Having said that, it looks like we allow a user + licence + agreement
> combination to have multiple roles?  That would mean having all four fields
> in the primary key... food for thought.

It's actually not possible, license_id and agreement_id are nullable and thus cannot be part of the primary key
https://mariadb.com/kb/en/primary-keys-with-nullable-columns/
Comment 5 Martin Renvoize 2022-11-11 11:16:52 UTC
Created attachment 143773 [details] [review]
Bug 32154: Missing primary key on erm_user_roles table

The aforementioned TestBuilder should display "Koha::ERM::UserRole", not be empty.

This patch adds a primary key to the erm_user_roles table and fixes that.

Also includes atomicupdate file to update database schema as well as updates to
kohastructure.sql provided by Jonathan Druart.

Looked into the vue files but didn't find anywhere or a need to include the new
primary key in any of the requests as there is no request of users by role_user_id
being made.

Erm users are being created/updated as a relationship through licenceses/agreements.

Furthermore, when editing a role of an existing user, this new primary key is already
being supplied embeded in the license/agreement.

Test plan:
Add users to license and agreement
Run the cypress tests
Confirm that the change fix the test that was failing in TestBuilder.t
(another one may still fail however)

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2022-11-11 11:17:36 UTC
Ah yes, your right.. I'd completely missed the NULLable.

Working as desired then..

Passing QA
Comment 7 Tomás Cohen Arazi 2022-11-11 12:03:04 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 8 Jonathan Druart 2022-11-16 09:00:56 UTC
This is missing the "DBIC changes" commit in master!
Comment 9 Jonathan Druart 2022-11-18 13:02:55 UTC
Created attachment 144061 [details] [review]
Bug 32154: Fix Agreements.t

#   Failed test at t/db_dependent/Koha/ERM/Agreements.t line 111.
    #     Structures begin differing at:
    #          $got->[0]{user_role_id} = '2'
    #     $expected->[0]{user_role_id} = Does not exist
    # Looks like you failed 1 test of 4.
Comment 10 Jonathan Druart 2022-11-18 13:17:07 UTC
(In reply to Jonathan Druart from comment #9)
> Created attachment 144061 [details] [review] [review]
> Bug 32154: Fix Agreements.t
> 
> #   Failed test at t/db_dependent/Koha/ERM/Agreements.t line 111.
>     #     Structures begin differing at:
>     #          $got->[0]{user_role_id} = '2'
>     #     $expected->[0]{user_role_id} = Does not exist
>     # Looks like you failed 1 test of 4.

Moved to bug 32267.