|
Description
Pedro Amorim
2024-09-06 11:27:34 UTC
Created attachment 171680 [details] [review] Bug 37856: [DONT PUSH] dbic changes Created attachment 171681 [details] [review] Bug 37856: Append platform parameter to SUSHI url if defined Created attachment 171682 [details] [review] Bug 37856: atomicupdate Created attachment 171683 [details] [review] Bug 37856: Add service_platform field The change from this patch itself should be self-explanatory. Testing for this requires credentials to a specific provider. But the parameter is only added to the URL if defined. Test plan, ktd, apply patch, run: $ yarn js:build $ yarn api:bundle $ koha-plack --restart kohadev 1) Enable ERMModule 2) Create a new provider, visit: <staff_url>/cgi-bin/koha/erm/eusage/usage_data_providers/add 3) Ensure 'Service platform' now shows. Create the provider and put something in this field. 4) View the provider, ensure the field is shown, visit: <staff_url>/cgi-bin/koha/erm/eusage/usage_data_providers/1 Ensure all tests are still passing: $ cypress run --spec t/cypress/integration/ERM/DataProviders_spec.ts $ prove t/db_dependent/api/v1/erm_usage_* Created attachment 171773 [details] [review] Bug 37856: Consider 'Exceptions' not being part of 'Report_Header' When fixing the issue around the missing platform parameter, we noticed this particular provider also returns 'Exceptions' as its own thing, not part of 'Report_Header', despite documentation: https://countermetrics.stoplight.io/docs/counter-sushi-api/7cp0h7n1hm2en-tr-j1-journal-requests-controlled From the link above, search for 'Exceptions'. Notice it should be under 'Report_Header'. Either way, this patch accounts for this possibility. Created attachment 171774 [details] [review] Bug 37856: Consider absence of 'Report_Items' The SUSHI provider in question for this patchset also does not return any Report_Items entry. This patch accounts for that. Created attachment 171775 [details] [review] Bug 37856: Add tests This is using the groundwork from bug 37963. This is testing the specific error this bug is addressing: Required parameter Platform is missing It is also testing that the error message is correctly shown as a background job report message to the user Test plan: prove t/db_dependent/Koha/BackgroundJob/ErmSushiHarvester.t Created attachment 171893 [details] [review] Bug 37856: [DONT PUSH] dbic changes Signed-off-by: David Nind <david@davidnind.com> Created attachment 171894 [details] [review] Bug 37856: Append platform parameter to SUSHI url if defined Signed-off-by: David Nind <david@davidnind.com> Created attachment 171895 [details] [review] Bug 37856: atomicupdate Signed-off-by: David Nind <david@davidnind.com> Created attachment 171896 [details] [review] Bug 37856: Add service_platform field The change from this patch itself should be self-explanatory. Testing for this requires credentials to a specific provider. But the parameter is only added to the URL if defined. Test plan, ktd, apply patch, run: $ yarn js:build $ yarn api:bundle $ koha-plack --restart kohadev 1) Enable ERMModule 2) Create a new provider, visit: <staff_url>/cgi-bin/koha/erm/eusage/usage_data_providers/add 3) Ensure 'Service platform' now shows. Create the provider and put something in this field. 4) View the provider, ensure the field is shown, visit: <staff_url>/cgi-bin/koha/erm/eusage/usage_data_providers/1 Ensure all tests are still passing: $ cypress run --spec t/cypress/integration/ERM/DataProviders_spec.ts $ prove t/db_dependent/api/v1/erm_usage_* Signed-off-by: David Nind <david@davidnind.com> Created attachment 171897 [details] [review] Bug 37856: Consider 'Exceptions' not being part of 'Report_Header' When fixing the issue around the missing platform parameter, we noticed this particular provider also returns 'Exceptions' as its own thing, not part of 'Report_Header', despite documentation: https://countermetrics.stoplight.io/docs/counter-sushi-api/7cp0h7n1hm2en-tr-j1-journal-requests-controlled From the link above, search for 'Exceptions'. Notice it should be under 'Report_Header'. Either way, this patch accounts for this possibility. Signed-off-by: David Nind <david@davidnind.com> Created attachment 171898 [details] [review] Bug 37856: Consider absence of 'Report_Items' The SUSHI provider in question for this patchset also does not return any Report_Items entry. This patch accounts for that. Signed-off-by: David Nind <david@davidnind.com> Created attachment 171899 [details] [review] Bug 37856: Add tests This is using the groundwork from bug 37963. This is testing the specific error this bug is addressing: Required parameter Platform is missing It is also testing that the error message is correctly shown as a background job report message to the user Test plan: prove t/db_dependent/Koha/BackgroundJob/ErmSushiHarvester.t Signed-off-by: David Nind <david@davidnind.com> I've signed off, but wasn't really able to test steps 3 and 4 for the 'Add service_platform' patch, and the 'Consider 'Exceptions' not being part of 'Report_Header'' patch. The tests all pass, and as I understand it the tests mock these. Testing notes (using KTD): 1. Run databaseupdate and restart_all, after applying the patches and running yarn js:build and yarn api:bundle. Created attachment 172040 [details] [review] Bug 37856: [DONT PUSH] dbic changes Signed-off-by: David Nind <david@davidnind.com> Created attachment 172042 [details] [review] Bug 37856: Append platform parameter to SUSHI url if defined Signed-off-by: David Nind <david@davidnind.com> Created attachment 172043 [details] [review] Bug 37856: atomicupdate Signed-off-by: David Nind <david@davidnind.com> Created attachment 172044 [details] [review] Bug 37856: Add service_platform field The change from this patch itself should be self-explanatory. Testing for this requires credentials to a specific provider. But the parameter is only added to the URL if defined. Test plan, ktd, apply patch, run: $ yarn js:build $ yarn api:bundle $ koha-plack --restart kohadev 1) Enable ERMModule 2) Create a new provider, visit: <staff_url>/cgi-bin/koha/erm/eusage/usage_data_providers/add 3) Ensure 'Service platform' now shows. Create the provider and put something in this field. 4) View the provider, ensure the field is shown, visit: <staff_url>/cgi-bin/koha/erm/eusage/usage_data_providers/1 Ensure all tests are still passing: $ cypress run --spec t/cypress/integration/ERM/DataProviders_spec.ts $ prove t/db_dependent/api/v1/erm_usage_* Signed-off-by: David Nind <david@davidnind.com> Created attachment 172045 [details] [review] Bug 37856: Consider 'Exceptions' not being part of 'Report_Header' When fixing the issue around the missing platform parameter, we noticed this particular provider also returns 'Exceptions' as its own thing, not part of 'Report_Header', despite documentation: https://countermetrics.stoplight.io/docs/counter-sushi-api/7cp0h7n1hm2en-tr-j1-journal-requests-controlled From the link above, search for 'Exceptions'. Notice it should be under 'Report_Header'. Either way, this patch accounts for this possibility. Signed-off-by: David Nind <david@davidnind.com> Created attachment 172046 [details] [review] Bug 37856: Consider absence of 'Report_Items' The SUSHI provider in question for this patchset also does not return any Report_Items entry. This patch accounts for that. Signed-off-by: David Nind <david@davidnind.com> Created attachment 172047 [details] [review] Bug 37856: Add tests This is using the groundwork from bug 37963. This is testing the specific error this bug is addressing: Required parameter Platform is missing It is also testing that the error message is correctly shown as a background job report message to the user Test plan: prove t/db_dependent/Koha/BackgroundJob/ErmSushiHarvester.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 172048 [details] [review] Bug 37856: (QA follow-up) Tidy Thanks for QA, Nick. Adding 24.05 and 23.11 release candidate keywords as this improves how the SUSHI response is handled widening the pool of providers Koha is able to work with. The way the database update outputs messages is a bit non-standard. I fixed the messages (we are not mentioning the bug number every time for example), but I didn't change the structure. I think what it should have done was printing different messages in case of the column already existing or not, but the "Done" seems not quite right. Not terribly important overall, just noting for future. Pushed for 24.11! Well done everyone, thank you! Created attachment 172155 [details] [review] Bug 37856: Fix existing test A service_platform value was being randomly to 'service_platform' as is a new attribute. Assign undef to that to fix existing test Created attachment 172156 [details] [review] Bug 37856: Add new test to test platform is correctly added to build_url prove t/db\_dependent/Koha/ERM/EUsage/UsageDataProvider.t Added 2 follow-up test patches to fix existing tests and add a new one. Adding additional_work_needed keyword to notify Katrin of these 2 follow-ups. 19 say_success( $out, "Done" ); Is this expected? "Done"? Created attachment 172215 [details] [review] Bug 37856: Update atomicupdate messages (In reply to Katrin Fischer from comment #25) > The way the database update outputs messages is a bit non-standard. I fixed > the messages (we are not mentioning the bug number every time for example), > but I didn't change the structure. > I think what it should have done was printing different messages in case of > the column already existing or not, but the "Done" seems not quite right. > Not terribly important overall, just noting for future. (In reply to Jonathan Druart from comment #30) > 19 say_success( $out, "Done" ); > > Is this expected? "Done"? The original patch: https://github.com/Koha-Community/Koha/commit/2e4b1ca42f7a940e8329ed860dda102deb629db0#diff-661419b80761834f79ce9100a1f2a132b804ee7cdf4efffe14c29d93bec810e4R17 Had: say_success( $out, "Bug 37856 - Done" ); I wanted to provide indication that the update of that particular bug was done, regardless if a change on the database happened or not. This is clearly causing confusion, this is now showing messages more in line with standard I believe. *** Bug 37058 has been marked as a duplicate of this bug. *** Too many merge conflicts in ErmSushiHarvester.t for me to solve, not knowing how this code works. (In reply to Lucas Gass (lukeg) from comment #33) > Too many merge conflicts in ErmSushiHarvester.t for me to solve, not knowing > how this code works. Looking here to provide patches for 24.05 (In reply to Lucas Gass (lukeg) from comment #33) > Too many merge conflicts in ErmSushiHarvester.t for me to solve, not knowing > how this code works. Last follow-up patch here wasn't applying (and wasn't pushed to main) so I made it obsolete. The reason you're getting the conflicts is because bug 37963 needs to be applied before this one, I believe. Backported to 24.05.x for upcoming 24.05.06 Pushed to 23.11.x for 23.11.11 Note that in 24.05.x the commit message has wrong bug id : 5eed8269b2 Bug 37526: DBIC Schema updates This may need to be fixed in release notes ;) |