Bug 35287 - Add additional fields support to ERM licenses
Summary: Add additional fields support to ERM licenses
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: Main
Hardware: All All
: P5 - low new feature
Assignee: Pedro Amorim
QA Contact: Paul Derscheid
URL:
Keywords: additional_work_needed, release-notes-needed
Depends on: 30645 35044 37389
Blocks: 32751 34324 37576 38262 38272
  Show dependency treegraph
 
Reported: 2023-11-08 12:35 UTC by Pedro Amorim
Modified: 2024-10-25 12:23 UTC (History)
13 users (show)

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


Attachments
Bug 35287: Add cypress tests (955.08 KB, patch)
2023-11-09 14:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Add unit tests (4.25 KB, patch)
2023-11-09 14:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Preparation: Extend AdditionalFields Mixin (3.41 KB, patch)
2023-11-09 14:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Preparation: AdditionalFields License Mixin (1.38 KB, patch)
2023-11-09 14:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Admin (1.71 KB, patch)
2023-11-09 14:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: REST API - Add additional fields support to ERM Licenses (6.45 KB, patch)
2023-11-09 14:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Vue preparation - AdditionalFields (15.85 KB, patch)
2023-11-09 14:42 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Vue preparation - Extend KohaTable to consider searchable additional fields (3.85 KB, patch)
2023-11-09 14:42 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Vue - Add additional fields support to ERM Licenses (10.50 KB, patch)
2023-11-09 14:42 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: perltidy for green QA script (16.70 KB, patch)
2023-11-09 14:42 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Add cypress tests (955.14 KB, patch)
2023-12-07 12:28 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35287: Add unit tests (4.31 KB, patch)
2023-12-07 12:28 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35287: Preparation: Extend AdditionalFields Mixin (3.47 KB, patch)
2023-12-07 12:28 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35287: Preparation: AdditionalFields License Mixin (1.44 KB, patch)
2023-12-07 12:28 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35287: Admin (1.77 KB, patch)
2023-12-07 12:28 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35287: REST API - Add additional fields support to ERM Licenses (6.51 KB, patch)
2023-12-07 12:29 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35287: Vue preparation - AdditionalFields (15.91 KB, patch)
2023-12-07 12:29 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35287: Vue preparation - Extend KohaTable to consider searchable additional fields (3.91 KB, patch)
2023-12-07 12:29 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35287: Vue - Add additional fields support to ERM Licenses (10.56 KB, patch)
2023-12-07 12:29 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35287: perltidy for green QA script (16.75 KB, patch)
2023-12-07 12:29 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 35287: Rebase after follow-up to 35197 (11.79 KB, patch)
2024-04-24 10:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Add cypress tests (955.20 KB, patch)
2024-04-26 10:31 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Add unit tests (4.36 KB, patch)
2024-04-26 10:31 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Preparation: Extend AdditionalFields Mixin (3.53 KB, patch)
2024-04-26 10:31 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Preparation: AdditionalFields License Mixin (1.50 KB, patch)
2024-04-26 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Admin (1.83 KB, patch)
2024-04-26 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: REST API - Add additional fields support to ERM Licenses (6.58 KB, patch)
2024-04-26 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Vue preparation - AdditionalFields (15.97 KB, patch)
2024-04-26 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Vue preparation - Extend KohaTable to consider searchable additional fields (3.97 KB, patch)
2024-04-26 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Vue - Add additional fields support to ERM Licenses (10.62 KB, patch)
2024-04-26 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: perltidy for green QA script (16.84 KB, patch)
2024-04-26 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Rebase after follow-up to 35197 (11.85 KB, patch)
2024-04-26 10:32 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Another follow-up to 35197 fix (1.11 KB, patch)
2024-06-13 09:20 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Add cypress tests (22.88 KB, patch)
2024-07-26 14:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Add unit tests (4.48 KB, patch)
2024-07-26 14:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Preparation: Extend AdditionalFields Mixin (3.53 KB, patch)
2024-07-26 14:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Preparation: AdditionalFields License Mixin (1.50 KB, patch)
2024-07-26 14:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Admin (1.83 KB, patch)
2024-07-26 14:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: REST API - Add additional fields support to ERM Licenses (8.00 KB, patch)
2024-07-26 14:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Vue preparation - AdditionalFields (17.08 KB, patch)
2024-07-26 14:30 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Vue preparation - Extend KohaTable to consider searchable additional fields (4.12 KB, patch)
2024-07-26 14:31 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Vue - Add additional fields support to ERM Licenses (10.72 KB, patch)
2024-07-26 14:31 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Follow-up 37389: ExtendedAttributes (2.34 KB, patch)
2024-07-26 14:31 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: (QA follow-up): Squash (8.30 KB, patch)
2024-08-16 11:17 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: (QA follow-up): Squash (9.38 KB, patch)
2024-08-27 10:15 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Add cypress tests (22.94 KB, patch)
2024-10-17 10:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35287: Add unit tests (4.42 KB, patch)
2024-10-17 10:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35287: Preparation: Extend AdditionalFields Mixin (3.58 KB, patch)
2024-10-17 10:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35287: Preparation: AdditionalFields License Mixin (1.55 KB, patch)
2024-10-17 10:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35287: Admin (1.88 KB, patch)
2024-10-17 10:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35287: REST API - Add additional fields support to ERM Licenses (8.06 KB, patch)
2024-10-17 10:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35287: Vue preparation - AdditionalFields (17.13 KB, patch)
2024-10-17 10:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35287: Vue preparation - Extend KohaTable to consider searchable additional fields (4.17 KB, patch)
2024-10-17 10:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35287: Vue - Add additional fields support to ERM Licenses (10.78 KB, patch)
2024-10-17 10:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35287: Follow-up 37389: ExtendedAttributes (2.40 KB, patch)
2024-10-17 10:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35287: (QA follow-up): Squash (9.43 KB, patch)
2024-10-17 10:12 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 35287: (QA follow-up): Add v-bind:key (2.69 KB, patch)
2024-10-21 10:31 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 35287: Add cypress tests (23.00 KB, patch)
2024-10-22 01:46 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: Add unit tests (4.48 KB, patch)
2024-10-22 01:46 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: Preparation: Extend AdditionalFields Mixin (3.64 KB, patch)
2024-10-22 01:46 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: Preparation: AdditionalFields License Mixin (1.61 KB, patch)
2024-10-22 01:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: Admin (1.94 KB, patch)
2024-10-22 01:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: REST API - Add additional fields support to ERM Licenses (8.12 KB, patch)
2024-10-22 01:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: Vue preparation - AdditionalFields (17.19 KB, patch)
2024-10-22 01:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: Vue preparation - Extend KohaTable to consider searchable additional fields (4.23 KB, patch)
2024-10-22 01:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: Vue - Add additional fields support to ERM Licenses (10.83 KB, patch)
2024-10-22 01:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: Follow-up 37389: ExtendedAttributes (2.45 KB, patch)
2024-10-22 01:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: (QA follow-up): Squash (9.49 KB, patch)
2024-10-22 01:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: (QA follow-up): Add v-bind:key (2.75 KB, patch)
2024-10-22 01:47 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35287: Fix tests (10.28 KB, patch)
2024-10-24 16:00 UTC, Pedro Amorim
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-11-08 12:35:26 UTC

    
Comment 1 Pedro Amorim 2023-11-09 14:41:45 UTC Comment hidden (obsolete)
Comment 2 Pedro Amorim 2023-11-09 14:41:48 UTC Comment hidden (obsolete)
Comment 3 Pedro Amorim 2023-11-09 14:41:51 UTC Comment hidden (obsolete)
Comment 4 Pedro Amorim 2023-11-09 14:41:53 UTC Comment hidden (obsolete)
Comment 5 Pedro Amorim 2023-11-09 14:41:56 UTC Comment hidden (obsolete)
Comment 6 Pedro Amorim 2023-11-09 14:41:58 UTC Comment hidden (obsolete)
Comment 7 Pedro Amorim 2023-11-09 14:42:01 UTC Comment hidden (obsolete)
Comment 8 Pedro Amorim 2023-11-09 14:42:03 UTC Comment hidden (obsolete)
Comment 9 Pedro Amorim 2023-11-09 14:42:06 UTC Comment hidden (obsolete)
Comment 10 Pedro Amorim 2023-11-09 14:42:09 UTC Comment hidden (obsolete)
Comment 11 PTFS Europe Sandboxes 2023-12-07 12:28:48 UTC
Created attachment 159634 [details] [review]
Bug 35287: Add cypress tests

Test plan, k-t-d:
cd /kohadevbox/koha
cypress run --spec "t/cypress/integration/AdditionalFields_spec.ts"

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Comment 12 PTFS Europe Sandboxes 2023-12-07 12:28:51 UTC
Created attachment 159635 [details] [review]
Bug 35287: Add unit tests

Test plan, k-t-d:
prove t/db_dependent/Koha/Object/Mixin/AdditionalFields.t

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Comment 13 PTFS Europe Sandboxes 2023-12-07 12:28:53 UTC
Created attachment 159636 [details] [review]
Bug 35287: Preparation: Extend AdditionalFields Mixin

The mixin now implements the extended_attributes method to be utilized by any Koha class that implements the AdditionalFields Mixin and is exposed through the REST API
It also implements a strings_map to be utilized in the same fashion as described above. This is useful because additional fields may be an authorised value

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Comment 14 PTFS Europe Sandboxes 2023-12-07 12:28:55 UTC
Created attachment 159637 [details] [review]
Bug 35287: Preparation: AdditionalFields License Mixin

Koha::Object::Mixin::AdditionalFields needs to come before Koha::Object
in the inheritance so that the strings_map method that is called is
the one from Mixin and not from Koha::Object.
This is only required for Koha classes implementing the AdditionalFields Mixin and expose data through the REST API, because strings_map is implement in both the aforementioned classes

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Comment 15 PTFS Europe Sandboxes 2023-12-07 12:28:58 UTC
Created attachment 159638 [details] [review]
Bug 35287: Admin

ERM licenses is now a table option in the additional fields admin panel
ERM licenses allows for the 'searchable' option (as well as 'repeatable')

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Comment 16 PTFS Europe Sandboxes 2023-12-07 12:29:00 UTC
Created attachment 159639 [details] [review]
Bug 35287: REST API - Add additional fields support to ERM Licenses

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Comment 17 PTFS Europe Sandboxes 2023-12-07 12:29:03 UTC
Created attachment 159640 [details] [review]
Bug 35287: Vue preparation - AdditionalFields

New api client: additional-fields-api-client.js to interact with the REST endpoint introduced by bug 35197
2 new components:
- AdditionalFieldsEntry: Component used to extend the FormAdd component including the corresponding additional fields
- AdditionalFieldsDisplay: Component used to display the additional fields associated to a data record implementing it (makes use of strings_map)

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Comment 18 PTFS Europe Sandboxes 2023-12-07 12:29:05 UTC
Created attachment 159641 [details] [review]
Bug 35287: Vue preparation - Extend KohaTable to consider searchable additional fields

This follows the same pattern as in subscription serials searchable additional fields, i.e. if an additional field is searchable, it is added to the list table - with the corresponding search inputs and functionality

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Comment 19 PTFS Europe Sandboxes 2023-12-07 12:29:08 UTC
Created attachment 159642 [details] [review]
Bug 35287: Vue - Add additional fields support to ERM Licenses

Test plan, k-t-d:
1) Add a new additional field for the newly supported 'erm_licenses', visit:
/cgi-bin/koha/admin/additional-fields.pl?tablename=erm_licenses
2) Create 4 fields by clicking on '+ New field'
2.1) 'text non-repeatable'
2.2) 'text repeatable', check the 'repeatable' box
2.3) 'av non-repeatable', pick an authorised value category
2.4) 'av repeatable', pick an authorised value category and check the 'repeatable' box
3) Add a new ERM license, visit:
/cgi-bin/koha/erm/licenses/add
4) Notice there is now a 'Additional fields' section at the bottom listing the fields we created
5) Fill in the mandatory regular license fields and play around with the additional fields
5.1) Put in some text in the text fields, test out the 'clear' and '+new' buttons
5.2) Select some AV options from the AV fields, deselect them, notice the repeatable one allows for multiple selection, the non-repeatable one does not
6) Save the license. On the list table, click on the license name (to navigate to the show page), or if license id=1, visit:
/cgi-bin/koha/erm/licenses/1
7) Notice the additional fields are listed there, AV fields show their human readable description, not the AV code. Repeatable fields are shown comma separated
8) Edit the license, visit:
/cgi-bin/koha/erm/licenses/edit/1
9) Play around again, do some more inserting and deletion of additional fields. Save. Notice everything is as expected.

Searchable testing:
10) Go back to the additional fields admin panel, visit:
/cgi-bin/koha/admin/additional-fields.pl?tablename=erm_licenses
11) Edit one field (AV or not) that you have inserted some data in, in the previous steps, and tick the 'searchable' box
12) Go back to licenses, visit:
/cgi-bin/koha/erm/licenses
13) Notice the searchable additional field now has its own column in the licenses list table
14) If the searchable field is AV, it has a dropdown with the AV values of the same AV category
15) If the searchable field not AV, it has a text input that allows for normal text search
16) If the searchable field is repeatable and has multiple values, its displayed in comma separated

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Comment 20 PTFS Europe Sandboxes 2023-12-07 12:29:11 UTC
Created attachment 159643 [details] [review]
Bug 35287: perltidy for green QA script

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Comment 21 Edith Speller 2023-12-07 12:31:18 UTC
Great work, thanks very much Pedro!
Comment 22 Martin Renvoize (ashimema) 2023-12-20 16:43:21 UTC
Comment on attachment 159639 [details] [review]
Bug 35287: REST API - Add additional fields support to ERM Licenses

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

::: Koha/REST/V1/ERM/Licenses.pm
@@ +183,5 @@
>                  $license->user_roles($user_roles);
>                  $license->documents($documents);
>  
> +                my @extended_attributes = map { {'id' => $_->{field_id}, 'value' => $_->{value}} } @{$extended_attributes};
> +                $license->extended_attributes( \@extended_attributes );

Won't you need to check here that @extended_attributes is indeed populated.. else you may wipe them away without meaning to?
Comment 23 Pedro Amorim 2024-01-09 17:18:12 UTC
(In reply to Martin Renvoize from comment #22)
> Comment on attachment 159639 [details] [review] [review]
> Bug 35287: REST API - Add additional fields support to ERM Licenses
> 
> Review of attachment 159639 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/REST/V1/ERM/Licenses.pm
> @@ +183,5 @@
> >                  $license->user_roles($user_roles);
> >                  $license->documents($documents);
> >  
> > +                my @extended_attributes = map { {'id' => $_->{field_id}, 'value' => $_->{value}} } @{$extended_attributes};
> > +                $license->extended_attributes( \@extended_attributes );
> 
> Won't you need to check here that @extended_attributes is indeed populated..
> else you may wipe them away without meaning to?

Good point. It's working like that by design. The scenario of wiping additional attributes without meaning to should not be possible through the Koha form, as existing data is always pre-populated on the form when editing.

Having said that, it is still possible with a third-party request to the REST API but I believe that to be a wider topic. I've opened bug 35747 where I provide a test plan to discuss it.
Comment 24 Pedro Amorim 2024-04-24 10:23:45 UTC
Created attachment 165451 [details] [review]
Bug 35287: Rebase after follow-up to 35197
Comment 25 Michaela Sieber 2024-04-26 07:47:01 UTC
(In reply to Pedro Amorim from comment #24)
> Created attachment 165451 [details] [review] [review]
> Bug 35287: Rebase after follow-up to 35197


Thanks Pedro!
We tested the additional fields for licenses and we wanted to sign off but have some problems with ktd and also with public sandboxes.



Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 26 Pedro Amorim 2024-04-26 10:31:52 UTC
Created attachment 165637 [details] [review]
Bug 35287: Add cypress tests

Test plan, k-t-d:
cd /kohadevbox/koha
cypress run --spec "t/cypress/integration/AdditionalFields_spec.ts"

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 27 Pedro Amorim 2024-04-26 10:31:55 UTC
Created attachment 165638 [details] [review]
Bug 35287: Add unit tests

Test plan, k-t-d:
prove t/db_dependent/Koha/Object/Mixin/AdditionalFields.t

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 28 Pedro Amorim 2024-04-26 10:31:58 UTC
Created attachment 165639 [details] [review]
Bug 35287: Preparation: Extend AdditionalFields Mixin

The mixin now implements the extended_attributes method to be utilized by any Koha class that implements the AdditionalFields Mixin and is exposed through the REST API
It also implements a strings_map to be utilized in the same fashion as described above. This is useful because additional fields may be an authorised value

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 29 Pedro Amorim 2024-04-26 10:32:01 UTC
Created attachment 165640 [details] [review]
Bug 35287: Preparation: AdditionalFields License Mixin

Koha::Object::Mixin::AdditionalFields needs to come before Koha::Object
in the inheritance so that the strings_map method that is called is
the one from Mixin and not from Koha::Object.
This is only required for Koha classes implementing the AdditionalFields Mixin and expose data through the REST API, because strings_map is implement in both the aforementioned classes

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 30 Pedro Amorim 2024-04-26 10:32:04 UTC
Created attachment 165641 [details] [review]
Bug 35287: Admin

ERM licenses is now a table option in the additional fields admin panel
ERM licenses allows for the 'searchable' option (as well as 'repeatable')

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 31 Pedro Amorim 2024-04-26 10:32:07 UTC
Created attachment 165642 [details] [review]
Bug 35287: REST API - Add additional fields support to ERM Licenses

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 32 Pedro Amorim 2024-04-26 10:32:10 UTC
Created attachment 165643 [details] [review]
Bug 35287: Vue preparation - AdditionalFields

New api client: additional-fields-api-client.js to interact with the REST endpoint introduced by bug 35197
2 new components:
- AdditionalFieldsEntry: Component used to extend the FormAdd component including the corresponding additional fields
- AdditionalFieldsDisplay: Component used to display the additional fields associated to a data record implementing it (makes use of strings_map)

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 33 Pedro Amorim 2024-04-26 10:32:13 UTC
Created attachment 165644 [details] [review]
Bug 35287: Vue preparation - Extend KohaTable to consider searchable additional fields

This follows the same pattern as in subscription serials searchable additional fields, i.e. if an additional field is searchable, it is added to the list table - with the corresponding search inputs and functionality

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 34 Pedro Amorim 2024-04-26 10:32:16 UTC
Created attachment 165645 [details] [review]
Bug 35287: Vue - Add additional fields support to ERM Licenses

Test plan, k-t-d:
1) Add a new additional field for the newly supported 'erm_licenses', visit:
/cgi-bin/koha/admin/additional-fields.pl?tablename=erm_licenses
2) Create 4 fields by clicking on '+ New field'
2.1) 'text non-repeatable'
2.2) 'text repeatable', check the 'repeatable' box
2.3) 'av non-repeatable', pick an authorised value category
2.4) 'av repeatable', pick an authorised value category and check the 'repeatable' box
3) Add a new ERM license, visit:
/cgi-bin/koha/erm/licenses/add
4) Notice there is now a 'Additional fields' section at the bottom listing the fields we created
5) Fill in the mandatory regular license fields and play around with the additional fields
5.1) Put in some text in the text fields, test out the 'clear' and '+new' buttons
5.2) Select some AV options from the AV fields, deselect them, notice the repeatable one allows for multiple selection, the non-repeatable one does not
6) Save the license. On the list table, click on the license name (to navigate to the show page), or if license id=1, visit:
/cgi-bin/koha/erm/licenses/1
7) Notice the additional fields are listed there, AV fields show their human readable description, not the AV code. Repeatable fields are shown comma separated
8) Edit the license, visit:
/cgi-bin/koha/erm/licenses/edit/1
9) Play around again, do some more inserting and deletion of additional fields. Save. Notice everything is as expected.

Searchable testing:
10) Go back to the additional fields admin panel, visit:
/cgi-bin/koha/admin/additional-fields.pl?tablename=erm_licenses
11) Edit one field (AV or not) that you have inserted some data in, in the previous steps, and tick the 'searchable' box
12) Go back to licenses, visit:
/cgi-bin/koha/erm/licenses
13) Notice the searchable additional field now has its own column in the licenses list table
14) If the searchable field is AV, it has a dropdown with the AV values of the same AV category
15) If the searchable field not AV, it has a text input that allows for normal text search
16) If the searchable field is repeatable and has multiple values, its displayed in comma separated

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 35 Pedro Amorim 2024-04-26 10:32:19 UTC
Created attachment 165646 [details] [review]
Bug 35287: perltidy for green QA script

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 36 Pedro Amorim 2024-04-26 10:32:22 UTC
Created attachment 165647 [details] [review]
Bug 35287: Rebase after follow-up to 35197

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 37 Pedro Amorim 2024-06-13 09:20:24 UTC
Created attachment 167681 [details] [review]
Bug 35287: Another follow-up to 35197 fix

Leaving this here for review.
This is following up the changes submitted to 35197 on 2024-04-29.
Squash later.
Comment 38 Pedro Amorim 2024-07-26 14:30:39 UTC
Created attachment 169728 [details] [review]
Bug 35287: Add cypress tests

Test plan, k-t-d:
cd /kohadevbox/koha
cypress run --spec "t/cypress/integration/AdditionalFields_spec.ts"

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 39 Pedro Amorim 2024-07-26 14:30:42 UTC
Created attachment 169729 [details] [review]
Bug 35287: Add unit tests

Test plan, k-t-d:
prove t/db_dependent/Koha/Object/Mixin/AdditionalFields.t

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 40 Pedro Amorim 2024-07-26 14:30:45 UTC
Created attachment 169730 [details] [review]
Bug 35287: Preparation: Extend AdditionalFields Mixin

The mixin now implements the extended_attributes method to be utilized by any Koha class that implements the AdditionalFields Mixin and is exposed through the REST API
It also implements a strings_map to be utilized in the same fashion as described above. This is useful because additional fields may be an authorised value

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 41 Pedro Amorim 2024-07-26 14:30:49 UTC
Created attachment 169731 [details] [review]
Bug 35287: Preparation: AdditionalFields License Mixin

Koha::Object::Mixin::AdditionalFields needs to come before Koha::Object
in the inheritance so that the strings_map method that is called is
the one from Mixin and not from Koha::Object.
This is only required for Koha classes implementing the AdditionalFields Mixin and expose data through the REST API, because strings_map is implement in both the aforementioned classes

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 42 Pedro Amorim 2024-07-26 14:30:52 UTC
Created attachment 169732 [details] [review]
Bug 35287: Admin

ERM licenses is now a table option in the additional fields admin panel
ERM licenses allows for the 'searchable' option (as well as 'repeatable')

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 43 Pedro Amorim 2024-07-26 14:30:55 UTC
Created attachment 169733 [details] [review]
Bug 35287: REST API - Add additional fields support to ERM Licenses

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 44 Pedro Amorim 2024-07-26 14:30:59 UTC
Created attachment 169734 [details] [review]
Bug 35287: Vue preparation - AdditionalFields

New api client: additional-fields-api-client.js to interact with the REST endpoint introduced by bug 35197
2 new components:
- AdditionalFieldsEntry: Component used to extend the FormAdd component including the corresponding additional fields
- AdditionalFieldsDisplay: Component used to display the additional fields associated to a data record implementing it (makes use of strings_map)

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 45 Pedro Amorim 2024-07-26 14:31:02 UTC
Created attachment 169735 [details] [review]
Bug 35287: Vue preparation - Extend KohaTable to consider searchable additional fields

This follows the same pattern as in subscription serials searchable additional fields, i.e. if an additional field is searchable, it is added to the list table - with the corresponding search inputs and functionality

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 46 Pedro Amorim 2024-07-26 14:31:06 UTC
Created attachment 169736 [details] [review]
Bug 35287: Vue - Add additional fields support to ERM Licenses

Test plan, k-t-d:
1) Add a new additional field for the newly supported 'erm_licenses', visit:
/cgi-bin/koha/admin/additional-fields.pl?tablename=erm_licenses
2) Create 4 fields by clicking on '+ New field'
2.1) 'text non-repeatable'
2.2) 'text repeatable', check the 'repeatable' box
2.3) 'av non-repeatable', pick an authorised value category
2.4) 'av repeatable', pick an authorised value category and check the 'repeatable' box
3) Add a new ERM license, visit:
/cgi-bin/koha/erm/licenses/add
4) Notice there is now a 'Additional fields' section at the bottom listing the fields we created
5) Fill in the mandatory regular license fields and play around with the additional fields
5.1) Put in some text in the text fields, test out the 'clear' and '+new' buttons
5.2) Select some AV options from the AV fields, deselect them, notice the repeatable one allows for multiple selection, the non-repeatable one does not
6) Save the license. On the list table, click on the license name (to navigate to the show page), or if license id=1, visit:
/cgi-bin/koha/erm/licenses/1
7) Notice the additional fields are listed there, AV fields show their human readable description, not the AV code. Repeatable fields are shown comma separated
8) Edit the license, visit:
/cgi-bin/koha/erm/licenses/edit/1
9) Play around again, do some more inserting and deletion of additional fields. Save. Notice everything is as expected.

Searchable testing:
10) Go back to the additional fields admin panel, visit:
/cgi-bin/koha/admin/additional-fields.pl?tablename=erm_licenses
11) Edit one field (AV or not) that you have inserted some data in, in the previous steps, and tick the 'searchable' box
12) Go back to licenses, visit:
/cgi-bin/koha/erm/licenses
13) Notice the searchable additional field now has its own column in the licenses list table
14) If the searchable field is AV, it has a dropdown with the AV values of the same AV category
15) If the searchable field not AV, it has a text input that allows for normal text search
16) If the searchable field is repeatable and has multiple values, its displayed in comma separated

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 47 Pedro Amorim 2024-07-26 14:31:10 UTC
Created attachment 169737 [details] [review]
Bug 35287: Follow-up 37389: ExtendedAttributes

Apply ExtendedAttributes Mixin to AdditionalFields

To test the ExtendedAttributes mixin search is working:
1) Create 2 or more searchable licenses additional fields
2) Create a new license and enter values in these searchable fields
3) Go to the licenses list, use the column filters to search on more
than one column, verify that the search work as intended (AND operator)
Comment 48 Pedro Amorim 2024-07-26 14:33:16 UTC
This is now fully rebased on the pushed version of bug 35197.
This is now also adding a light follow-up to bug 37389 to make use of the search performance and consistency improvements made there.

I'm keeping the sign-off status as all the functionality is the same, even though the code has changed quite a bit. Happy either way. Please let me know.
Comment 49 Michaela Sieber 2024-08-06 07:19:11 UTC
Thanks Pedro, I've tested once again and everything worked fine.
Comment 50 Victor Grousset/tuxayo 2024-08-09 22:28:20 UTC
(In reply to Pedro Amorim from comment #48)
> I'm keeping the sign-off status as all the functionality is the same, even
> though the code has changed quite a bit. Happy either way. Please let me
> know.

It's a case where retesting is needed. Because signing off isn't about vetting the functional change. (function approval or issues found are welcome)
But rather to confirm that there is indeed a functional change, whatever it might be. It's like automated tests but manual, so (non trivial) code changes warrant rerunning them.

Thanks Michaela for retesting :)
Comment 51 David Cook 2024-08-13 00:29:27 UTC
I had some QA tool failings, but it looks like I hadn't properly applied the dependencies. I'll try to run again soon, but have some urgent things at the moment to deal with.
Comment 52 Victor Grousset/tuxayo 2024-08-14 01:26:15 UTC
> I had some QA tool failings, but it looks like I hadn't properly applied the dependencies.

There is at least this:

 WARN	Koha/Object/Mixin/AdditionalFields.pm
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 17, now: 19)

 FAIL	Koha/Objects/Mixin/AdditionalFields.pm
   FAIL	  pod coverage
		POD is missing for 'extended_attributes_tablename_query'
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 3, now: 6)

 WARN	Koha/REST/V1/ERM/Licenses.pm
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 46, now: 50)

 WARN	t/db_dependent/Koha/Object/Mixin/AdditionalFields.t
   WARN	  tidiness
		The file is less tidy than before (bad/messy lines before: 20, now: 35)
Comment 53 Victor Grousset/tuxayo 2024-08-16 03:10:12 UTC
Looks like there is an unused variable:

+sub extended_attributes_config {
+    my ($self) = @_;
+
+    my $tablename_query = $self->extended_attributes_tablename_query;
Comment 54 Pedro Amorim 2024-08-16 11:17:31 UTC
Created attachment 170448 [details] [review]
Bug 35287: (QA follow-up): Squash

Remove leftover unused code
Add missing pod
Tidy

Run:
qa -c 11 -v 2

Verify everything is green
Comment 55 Victor Grousset/tuxayo 2024-08-26 19:01:17 UTC
Thanks for the follow-up :D


----

Any sponsor of this quite long enhancement?

---

QA notes:
prettier ok (JS14) on all vue, js and ts files touched.
- koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/LicensesList.vue
- koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/LicensesFormAdd.vue
- koha-tmpl/intranet-tmpl/prog/js/vue/components/AdditionalFieldsEntry.vue
- koha-tmpl/intranet-tmpl/prog/js/vue/components/AdditionalFieldsDisplay.vue
- koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/LicensesShow.vue
- koha-tmpl/intranet-tmpl/prog/js/vue/components/KohaTable.vue
- t/cypress/integration/AdditionalFields_spec.ts


This new file needs a /* keep tidy */ comment on top:
koha-tmpl/intranet-tmpl/prog/js/vue/fetch/additional-fields-api-client.js

It's also a good opportunity to add a /* keep tidy */ comment on top of this file:
koha-tmpl/intranet-tmpl/prog/js/vue/fetch/erm-api-client.js
To avoid having to manually check it with prettier.

----

there are two
cy.get("#licenses_list table tbody tr:first td:first a").click();

IMHO either they should get a comment to easily know what's clicked when in the future there will be some change to the structure of the table and the test will break.
Or to prevent breakage and documenting at the same time, an id or a class could be added to what's being clicked (or it's parent) so the selector speaks for itself and would be much less likely to break with future changes.


---


found two instances of forgotten
// cy.wait("@avcategories");


---

tip: (not a QA worry to be addressed)
I think it's okay to add a class or id from time to time facilitate selection to make the tests more concise by avoiding the need for sibling circumvention, such as .parent().children("input")
And also be able to handle cases without an easily selectable sibling. Like in the previous case of "#licenses_list table tbody tr:first td:first a"
Comment 56 Pedro Amorim 2024-08-27 10:15:14 UTC
Created attachment 170753 [details] [review]
Bug 35287: (QA follow-up): Squash

Remove leftover unused code
Add missing pod
Tidy
Removed redundant commented in AdditionalFields_spec.ts

Run:
qa -c 11 -v 2

Verify everything is green

Sponsored-by: UKHSA - UK Health Security Agency
Sponsored-by: PTFS Europe Ltd
Comment 57 Pedro Amorim 2024-08-27 10:18:48 UTC
Thank you, Victor.

I've squashed the removal of commented cy.wait("@avcategories"); to the squash qa follow-up patch.
I've also added sponsored-by lines.

(In reply to Victor Grousset/tuxayo from comment #55)
> This new file needs a /* keep tidy */ comment on top:
> koha-tmpl/intranet-tmpl/prog/js/vue/fetch/additional-fields-api-client.js
> 
> It's also a good opportunity to add a /* keep tidy */ comment on top of this
> file:
> koha-tmpl/intranet-tmpl/prog/js/vue/fetch/erm-api-client.js
> To avoid having to manually check it with prettier.

No api.js files under js/vue/fetch have this. If this is reason to FQA it should at least be flagged by the QA script first then.

> there are two
> cy.get("#licenses_list table tbody tr:first td:first a").click();
> 
> IMHO either they should get a comment to easily know what's clicked when in
> the future there will be some change to the structure of the table and the
> test will break.
> Or to prevent breakage and documenting at the same time, an id or a class
> could be added to what's being clicked (or it's parent) so the selector
> speaks for itself and would be much less likely to break with future changes.

This is an established pattern utilized in all cypress spec files to click on the first element of a table (to navigate to it). I can see your point but table rows don't usually have an id. I'm on board for filing a new bug titled "Add id to table rows to be more easily selected by cypress tests" but I don't see how this should be in the scope of this patchset.
Comment 58 Victor Grousset/tuxayo 2024-08-28 05:17:17 UTC
(In reply to Pedro Amorim from comment #57)
> I've squashed the removal of commented cy.wait("@avcategories"); to the
> squash qa follow-up patch.
> I've also added sponsored-by lines.

Thanks :)


> (In reply to Victor Grousset/tuxayo from comment #55)
> > This new file needs a /* keep tidy */ comment on top:
> > koha-tmpl/intranet-tmpl/prog/js/vue/fetch/additional-fields-api-client.js
> > 
> > It's also a good opportunity to add a /* keep tidy */ comment on top of this
> > file:
> > koha-tmpl/intranet-tmpl/prog/js/vue/fetch/erm-api-client.js
> > To avoid having to manually check it with prettier.
> 
> No api.js files under js/vue/fetch have this. 

JS14 guideline is more recent (march) than these. So it's case of dealing with legacy JS/TS/vue files. «Older JS files should also strive to become tidier and eventually all end up with the keep tidy header line too.»
It's just that in the meantime they haven't been touched or this guideline was forgotten.

> If this is reason to FQA it
> should at least be flagged by the QA script first then.

The point of /* keep tidy */ is to opt in the QA script enforcement of JS14. That's how to deal with the old files that are totally not compliant. (so not checked by default)
So until they are pretty and /* keep tidy */ is added, they will never show up in the QA script. At least not for this check.


> > there are two
> > cy.get("#licenses_list table tbody tr:first td:first a").click();
> > 
> > IMHO either they should get a comment to easily know what's clicked when in
> > the future there will be some change to the structure of the table and the
> > test will break.
> > Or to prevent breakage and documenting at the same time, an id or a class
> > could be added to what's being clicked (or it's parent) so the selector
> > speaks for itself and would be much less likely to break with future changes.
> 
> This is an established pattern utilized in all cypress spec files to click
> on the first element of a table (to navigate to it). 

Oops, sorry, the current selector is actually fine. I guess after reading too much in a row from these large patches my brain got fried ﷐[U+1F635]﷑‍﷐[U+1F4AB]﷑

> I'm on board for filing a new bug
> titled "Add id to table rows to be more easily selected by cypress tests"
> but I don't see how this should be in the scope of this patchset.

Even if not a relevant case here, about the general idea:
No need to fill a separate bug. It can totally be part of adding a test. I know it can feel wrong to even change minimally the markup to accommodate a test. But I think it's only because tests can still feel like second class citizens. We can think of it like when extracting a method to be able to unit test it. There it's okay to do so at the same time as adding tests for changes; make the code more easily testable.
So it should be ok (when the change is easy to make at least) to make the markup more testable. Both to get more reliable tests for if the page structure change. And have more readable tests and the markup itself.

As a fallback, having a comment to not have an undocumented "magic selector" (cf. magic number)

(again not at all needed for this case, it's fine)
Comment 59 Jonathan Druart 2024-08-28 08:04:51 UTC
(In reply to Victor Grousset/tuxayo from comment #58)
> (In reply to Pedro Amorim from comment #57)
> > (In reply to Victor Grousset/tuxayo from comment #55)
> > > This new file needs a /* keep tidy */ comment on top:
> > > koha-tmpl/intranet-tmpl/prog/js/vue/fetch/additional-fields-api-client.js
> > > 
> > > It's also a good opportunity to add a /* keep tidy */ comment on top of this
> > > file:
> > > koha-tmpl/intranet-tmpl/prog/js/vue/fetch/erm-api-client.js
> > > To avoid having to manually check it with prettier.
> > 
> > No api.js files under js/vue/fetch have this. 
> 
> JS14 guideline is more recent (march) than these. So it's case of dealing
> with legacy JS/TS/vue files. «Older JS files should also strive to become
> tidier and eventually all end up with the keep tidy header line too.»
> It's just that in the meantime they haven't been touched or this guideline
> was forgotten.
> 
> > If this is reason to FQA it
> > should at least be flagged by the QA script first then.
> 
> The point of /* keep tidy */ is to opt in the QA script enforcement of JS14.
> That's how to deal with the old files that are totally not compliant. (so
> not checked by default)
> So until they are pretty and /* keep tidy */ is added, they will never show
> up in the QA script. At least not for this check.

Not sure to follow the discussion, just want to confirm that .js files under koha-tmpl/intranet-tmpl/prog/js/vue do not need the 'keep tidy' flag, but are checked by both the QA script and unit test (xt/vue_tidy.t)
Comment 60 Victor Grousset/tuxayo 2024-08-30 21:18:45 UTC
Thanks Jonathan for the info, I updated the coding guidelines to list the places that are checked without needing the flag.
https://wiki.koha-community.org/wiki/Coding_Guidelines#JS14%3A_Prettier_JavaScript
Comment 61 Paul Derscheid 2024-10-17 10:12:12 UTC
Created attachment 172850 [details] [review]
Bug 35287: Add cypress tests

Test plan, k-t-d:
cd /kohadevbox/koha
cypress run --spec "t/cypress/integration/AdditionalFields_spec.ts"

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 62 Paul Derscheid 2024-10-17 10:12:15 UTC
Created attachment 172851 [details] [review]
Bug 35287: Add unit tests

Test plan, k-t-d:
prove t/db_dependent/Koha/Object/Mixin/AdditionalFields.t

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 63 Paul Derscheid 2024-10-17 10:12:19 UTC
Created attachment 172852 [details] [review]
Bug 35287: Preparation: Extend AdditionalFields Mixin

The mixin now implements the extended_attributes method to be utilized by any Koha class that implements the AdditionalFields Mixin and is exposed through the REST API
It also implements a strings_map to be utilized in the same fashion as described above. This is useful because additional fields may be an authorised value

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 64 Paul Derscheid 2024-10-17 10:12:22 UTC
Created attachment 172853 [details] [review]
Bug 35287: Preparation: AdditionalFields License Mixin

Koha::Object::Mixin::AdditionalFields needs to come before Koha::Object
in the inheritance so that the strings_map method that is called is
the one from Mixin and not from Koha::Object.
This is only required for Koha classes implementing the AdditionalFields Mixin and expose data through the REST API, because strings_map is implement in both the aforementioned classes

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 65 Paul Derscheid 2024-10-17 10:12:25 UTC
Created attachment 172854 [details] [review]
Bug 35287: Admin

ERM licenses is now a table option in the additional fields admin panel
ERM licenses allows for the 'searchable' option (as well as 'repeatable')

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 66 Paul Derscheid 2024-10-17 10:12:28 UTC
Created attachment 172855 [details] [review]
Bug 35287: REST API - Add additional fields support to ERM Licenses

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 67 Paul Derscheid 2024-10-17 10:12:32 UTC
Created attachment 172856 [details] [review]
Bug 35287: Vue preparation - AdditionalFields

New api client: additional-fields-api-client.js to interact with the REST endpoint introduced by bug 35197
2 new components:
- AdditionalFieldsEntry: Component used to extend the FormAdd component including the corresponding additional fields
- AdditionalFieldsDisplay: Component used to display the additional fields associated to a data record implementing it (makes use of strings_map)

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 68 Paul Derscheid 2024-10-17 10:12:35 UTC
Created attachment 172857 [details] [review]
Bug 35287: Vue preparation - Extend KohaTable to consider searchable additional fields

This follows the same pattern as in subscription serials searchable additional fields, i.e. if an additional field is searchable, it is added to the list table - with the corresponding search inputs and functionality

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 69 Paul Derscheid 2024-10-17 10:12:39 UTC
Created attachment 172858 [details] [review]
Bug 35287: Vue - Add additional fields support to ERM Licenses

Test plan, k-t-d:
1) Add a new additional field for the newly supported 'erm_licenses', visit:
/cgi-bin/koha/admin/additional-fields.pl?tablename=erm_licenses
2) Create 4 fields by clicking on '+ New field'
2.1) 'text non-repeatable'
2.2) 'text repeatable', check the 'repeatable' box
2.3) 'av non-repeatable', pick an authorised value category
2.4) 'av repeatable', pick an authorised value category and check the 'repeatable' box
3) Add a new ERM license, visit:
/cgi-bin/koha/erm/licenses/add
4) Notice there is now a 'Additional fields' section at the bottom listing the fields we created
5) Fill in the mandatory regular license fields and play around with the additional fields
5.1) Put in some text in the text fields, test out the 'clear' and '+new' buttons
5.2) Select some AV options from the AV fields, deselect them, notice the repeatable one allows for multiple selection, the non-repeatable one does not
6) Save the license. On the list table, click on the license name (to navigate to the show page), or if license id=1, visit:
/cgi-bin/koha/erm/licenses/1
7) Notice the additional fields are listed there, AV fields show their human readable description, not the AV code. Repeatable fields are shown comma separated
8) Edit the license, visit:
/cgi-bin/koha/erm/licenses/edit/1
9) Play around again, do some more inserting and deletion of additional fields. Save. Notice everything is as expected.

Searchable testing:
10) Go back to the additional fields admin panel, visit:
/cgi-bin/koha/admin/additional-fields.pl?tablename=erm_licenses
11) Edit one field (AV or not) that you have inserted some data in, in the previous steps, and tick the 'searchable' box
12) Go back to licenses, visit:
/cgi-bin/koha/erm/licenses
13) Notice the searchable additional field now has its own column in the licenses list table
14) If the searchable field is AV, it has a dropdown with the AV values of the same AV category
15) If the searchable field not AV, it has a text input that allows for normal text search
16) If the searchable field is repeatable and has multiple values, its displayed in comma separated

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 70 Paul Derscheid 2024-10-17 10:12:42 UTC
Created attachment 172860 [details] [review]
Bug 35287: Follow-up 37389: ExtendedAttributes

Apply ExtendedAttributes Mixin to AdditionalFields

To test the ExtendedAttributes mixin search is working:
1) Create 2 or more searchable licenses additional fields
2) Create a new license and enter values in these searchable fields
3) Go to the licenses list, use the column filters to search on more
than one column, verify that the search work as intended (AND operator)

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 71 Paul Derscheid 2024-10-17 10:12:45 UTC
Created attachment 172861 [details] [review]
Bug 35287: (QA follow-up): Squash

Remove leftover unused code
Add missing pod
Tidy
Removed redundant commented in AdditionalFields_spec.ts

Run:
qa -c 11 -v 2

Verify everything is green

Sponsored-by: UKHSA - UK Health Security Agency
Sponsored-by: PTFS Europe Ltd
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 72 Paul Derscheid 2024-10-17 10:15:41 UTC
Awesome work Pedro. I just rushed through the test plan and almost set this to FQA, but all of it is working nicely and the additional fields mixin is 🔥
Comment 73 Victor Grousset/tuxayo 2024-10-21 05:52:29 UTC
Thanks Paul for the rest of the QA :D

From my code review in august, everything looked compliant with the coding guidelines ☑️

Except for JS8 (ESLint compliance) for which the tooling isn't working to effectively look at it. ¯\_ (ツ)_/¯

-----

Still, reliably catching stuff like unused variables, unreachable code and other things hard to manually look for would be nice.
After fighting tooth and nail with node packaging and ESLint config, I got something running to experiment here and help with the almost 1000 lines of .vue .ts.

And the result is that the changes look pretty good 👍

Config needs for tuning to our practices so there is a lot of noise that I'm filtering plus now I don't bother with minor stuff like using const on every not reassigned variable and order of attributes in Vue markup.


Here are some quick suspicious things that remain:
t/cypress/integration/AdditionalFields_spec.ts
    2:15  error  A `require()` style import is forbidden                               @typescript-eslint/no-require-imports
Is that really bad? (For this ticket at least. Knowing if this rule should be disabled is for another day ^^, unless it's obvious if it's relevant for the future or not)


koha-tmpl/intranet-tmpl/prog/js/vue/components/AdditionalFieldsEntry.vue
   16:21  error    Elements in iteration expect to have 'v-bind:key' directives                                 vue/require-v-for-key
   50:21  error    Elements in iteration expect to have 'v-bind:key' directives                                 vue/require-v-for-key
   65:29  warning  Attribute "v-model" should go before ":multiple"                                             vue/attributes-order
   83:21  error    Elements in iteration expect to have 'v-bind:key' directives                                 vue/require-v-for-key
   83:21  error    Elements in iteration expect to have 'v-bind:key' directives                                 vue/require-v-for-key
Is that really bad?

-----


I think that's all, so good news :)
Comment 74 Pedro Amorim 2024-10-21 10:31:56 UTC
Created attachment 173022 [details] [review]
Bug 35287: (QA follow-up): Add v-bind:key

Remove more unused imports
Comment 75 Pedro Amorim 2024-10-21 10:34:08 UTC
Thank you Paul and thank you Victor.
@Victor I've submitted an additional patch that should address the v-bind:key warning.
I suppose for the 'require' warning in AdditionalFields_spec.ts we could instead use:
import * as dayjs from 'dayjs';

But probably a conversation for a different bug as other spec.ts files would have to be updated.
Comment 76 Victor Grousset/tuxayo 2024-10-22 01:46:43 UTC
Created attachment 173102 [details] [review]
Bug 35287: Add cypress tests

Test plan, k-t-d:
cd /kohadevbox/koha
cypress run --spec "t/cypress/integration/AdditionalFields_spec.ts"

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 77 Victor Grousset/tuxayo 2024-10-22 01:46:51 UTC
Created attachment 173103 [details] [review]
Bug 35287: Add unit tests

Test plan, k-t-d:
prove t/db_dependent/Koha/Object/Mixin/AdditionalFields.t

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 78 Victor Grousset/tuxayo 2024-10-22 01:46:56 UTC
Created attachment 173104 [details] [review]
Bug 35287: Preparation: Extend AdditionalFields Mixin

The mixin now implements the extended_attributes method to be utilized by any Koha class that implements the AdditionalFields Mixin and is exposed through the REST API
It also implements a strings_map to be utilized in the same fashion as described above. This is useful because additional fields may be an authorised value

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 79 Victor Grousset/tuxayo 2024-10-22 01:47:00 UTC
Created attachment 173105 [details] [review]
Bug 35287: Preparation: AdditionalFields License Mixin

Koha::Object::Mixin::AdditionalFields needs to come before Koha::Object
in the inheritance so that the strings_map method that is called is
the one from Mixin and not from Koha::Object.
This is only required for Koha classes implementing the AdditionalFields Mixin and expose data through the REST API, because strings_map is implement in both the aforementioned classes

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 80 Victor Grousset/tuxayo 2024-10-22 01:47:04 UTC
Created attachment 173106 [details] [review]
Bug 35287: Admin

ERM licenses is now a table option in the additional fields admin panel
ERM licenses allows for the 'searchable' option (as well as 'repeatable')

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 81 Victor Grousset/tuxayo 2024-10-22 01:47:08 UTC
Created attachment 173107 [details] [review]
Bug 35287: REST API - Add additional fields support to ERM Licenses

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 82 Victor Grousset/tuxayo 2024-10-22 01:47:11 UTC
Created attachment 173108 [details] [review]
Bug 35287: Vue preparation - AdditionalFields

New api client: additional-fields-api-client.js to interact with the REST endpoint introduced by bug 35197
2 new components:
- AdditionalFieldsEntry: Component used to extend the FormAdd component including the corresponding additional fields
- AdditionalFieldsDisplay: Component used to display the additional fields associated to a data record implementing it (makes use of strings_map)

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 83 Victor Grousset/tuxayo 2024-10-22 01:47:15 UTC
Created attachment 173109 [details] [review]
Bug 35287: Vue preparation - Extend KohaTable to consider searchable additional fields

This follows the same pattern as in subscription serials searchable additional fields, i.e. if an additional field is searchable, it is added to the list table - with the corresponding search inputs and functionality

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 84 Victor Grousset/tuxayo 2024-10-22 01:47:19 UTC
Created attachment 173110 [details] [review]
Bug 35287: Vue - Add additional fields support to ERM Licenses

Test plan, k-t-d:
1) Add a new additional field for the newly supported 'erm_licenses', visit:
/cgi-bin/koha/admin/additional-fields.pl?tablename=erm_licenses
2) Create 4 fields by clicking on '+ New field'
2.1) 'text non-repeatable'
2.2) 'text repeatable', check the 'repeatable' box
2.3) 'av non-repeatable', pick an authorised value category
2.4) 'av repeatable', pick an authorised value category and check the 'repeatable' box
3) Add a new ERM license, visit:
/cgi-bin/koha/erm/licenses/add
4) Notice there is now a 'Additional fields' section at the bottom listing the fields we created
5) Fill in the mandatory regular license fields and play around with the additional fields
5.1) Put in some text in the text fields, test out the 'clear' and '+new' buttons
5.2) Select some AV options from the AV fields, deselect them, notice the repeatable one allows for multiple selection, the non-repeatable one does not
6) Save the license. On the list table, click on the license name (to navigate to the show page), or if license id=1, visit:
/cgi-bin/koha/erm/licenses/1
7) Notice the additional fields are listed there, AV fields show their human readable description, not the AV code. Repeatable fields are shown comma separated
8) Edit the license, visit:
/cgi-bin/koha/erm/licenses/edit/1
9) Play around again, do some more inserting and deletion of additional fields. Save. Notice everything is as expected.

Searchable testing:
10) Go back to the additional fields admin panel, visit:
/cgi-bin/koha/admin/additional-fields.pl?tablename=erm_licenses
11) Edit one field (AV or not) that you have inserted some data in, in the previous steps, and tick the 'searchable' box
12) Go back to licenses, visit:
/cgi-bin/koha/erm/licenses
13) Notice the searchable additional field now has its own column in the licenses list table
14) If the searchable field is AV, it has a dropdown with the AV values of the same AV category
15) If the searchable field not AV, it has a text input that allows for normal text search
16) If the searchable field is repeatable and has multiple values, its displayed in comma separated

Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 85 Victor Grousset/tuxayo 2024-10-22 01:47:22 UTC
Created attachment 173111 [details] [review]
Bug 35287: Follow-up 37389: ExtendedAttributes

Apply ExtendedAttributes Mixin to AdditionalFields

To test the ExtendedAttributes mixin search is working:
1) Create 2 or more searchable licenses additional fields
2) Create a new license and enter values in these searchable fields
3) Go to the licenses list, use the column filters to search on more
than one column, verify that the search work as intended (AND operator)

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 86 Victor Grousset/tuxayo 2024-10-22 01:47:26 UTC
Created attachment 173112 [details] [review]
Bug 35287: (QA follow-up): Squash

Remove leftover unused code
Add missing pod
Tidy
Removed redundant commented in AdditionalFields_spec.ts

Run:
qa -c 11 -v 2

Verify everything is green

Sponsored-by: UKHSA - UK Health Security Agency
Sponsored-by: PTFS Europe Ltd
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 87 Victor Grousset/tuxayo 2024-10-22 01:47:29 UTC
Created attachment 173113 [details] [review]
Bug 35287: (QA follow-up): Add v-bind:key

Remove more unused imports

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 88 Victor Grousset/tuxayo 2024-10-22 01:55:18 UTC
Thanks again Pedro :)

Here is another signoff about not seeing any issue following the test plan. Plus not seeing any issue either relating to the coding guidelines. 👍

And well done, the UI was nice to use :)

----

API testing question: After applying the patches what should be done besides this:
rspack build
restart_all

I had 400 errors on /api/v1/extended_attribute_types?resource_type=license
Until doing reset_all.

What was missing? Something with that redocly thing?
Comment 89 Pedro Amorim 2024-10-22 08:23:19 UTC
(In reply to Victor Grousset/tuxayo from comment #88) 
> What was missing? Something with that redocly thing?

You were possibly missing:
$ yarn api:bundle

The above is part of "reset_all" and that is usually what I go for instead of "restart_all".
Comment 90 Victor Grousset/tuxayo 2024-10-22 16:23:27 UTC
Thanks, that was it :D

Documented here (assuming there isn't a better place) for anyone needing to find the info again in the future.
https://wiki.koha-community.org/wiki/Rest_Api_HowTo#Testing_API_patches
Comment 91 Katrin Fischer 2024-10-24 11:38:59 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 92 Katrin Fischer 2024-10-24 14:34:27 UTC
Unfortunately this broke the existing cypress tests for licenses. 
Please fix in a follow-up.
Comment 93 Jonathan Druart 2024-10-24 14:45:45 UTC
(In reply to Katrin Fischer from comment #92)
> Unfortunately this broke the existing cypress tests for licenses. 
> Please fix in a follow-up.

The error is

15:26:19 koha_1       |   > Cannot read properties of undefined (reading 'additional_field_values')

On Koha_Main/3069
Comment 94 Pedro Amorim 2024-10-24 16:00:55 UTC
Created attachment 173290 [details] [review]
Bug 35287: Fix tests

Have a single point go get_license to be used by both Licenses_spec.ts and AdditionalFields_spec.ts

Test plan, apply patch:
$ yarn js:build
$ cypress run --spec t/cypress/integration/AdditionalFields_spec.ts
$ cypress run --spec t/cypress/integration/Licenses_spec.ts