|
Description
Pedro Amorim
2023-11-08 12:35:26 UTC
Created attachment 158725 [details] [review] Bug 35287: Add cypress tests Test plan, k-t-d: cd /kohadevbox/koha cypress run --spec "t/cypress/integration/AdditionalFields_spec.ts" Created attachment 158726 [details] [review] Bug 35287: Add unit tests Test plan, k-t-d: prove t/db_dependent/Koha/Object/Mixin/AdditionalFields.t Created attachment 158727 [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 Created attachment 158728 [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 Created attachment 158729 [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') Created attachment 158730 [details] [review] Bug 35287: REST API - Add additional fields support to ERM Licenses Created attachment 158731 [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) Created attachment 158732 [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 Created attachment 158733 [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 Created attachment 158734 [details] [review] Bug 35287: perltidy for green QA script 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> 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 159643 [details] [review] Bug 35287: perltidy for green QA script Signed-off-by: Edith Speller <edith.speller@ukhsa.gov.uk> Great work, thanks very much Pedro! 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? (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. Created attachment 165451 [details] [review] Bug 35287: Rebase after follow-up to 35197 (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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 165647 [details] [review] Bug 35287: Rebase after follow-up to 35197 Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu> 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. 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> 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> 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> 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> 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> 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> 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> 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> 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> 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) 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. Thanks Pedro, I've tested once again and everything worked fine. (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 :) 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. > 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)
Looks like there is an unused variable:
+sub extended_attributes_config {
+ my ($self) = @_;
+
+ my $tablename_query = $self->extended_attributes_tablename_query;
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 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"
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 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.
(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) (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) 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 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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 🔥 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 :)
Created attachment 173022 [details] [review] Bug 35287: (QA follow-up): Add v-bind:key Remove more unused imports 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. 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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? (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". 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 Pushed for 24.11! Well done everyone, thank you! Unfortunately this broke the existing cypress tests for licenses. Please fix in a follow-up. (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 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 I've removed the additional_work_needed keyword here as I believe it referred to the cypress fix submitted on 2024-10-24 and has already been pushed. (In reply to Pedro Amorim from comment #95) > I've removed the additional_work_needed keyword here as I believe it > referred to the cypress fix submitted on 2024-10-24 and has already been > pushed. Correct, thanks! New feature, no 24.05.x backport |