Test plan: 1) Enable ERMModule 2) Visit ERM: <staff_url>/cgi-bin/koha/erm/erm.pl 3) Notice you now have a 'dashboard' homepage showing 4 different widgets 4) Click 'Open Widget Picker'. Remove and add different widgets. Close 5) Drag and drop the widgets around the dashboard, from left to right, top to bottom 6) Click the context menu (vertical dots) on each of the widgets. Click 'move to right', 'move to left', 'remove'. 7) On the 'Licenses needing action', notice there is a 'Settings' item option. Click that, test different settings values. Close settings. 8) Using the widget picker or context menu for the respective widgets, remove all widgets except one. Click away i.e. 'Agreements' on the left menu. Go back to home by clicking 'Home'. Notice your dashboard is as you left it. 9) Enable all widgets, rearrange them by dragging and dropping in different places. Repeat the previous exercise. Notice the dashboard keeps the widgets arranged as you left them. 10) Add some test data, on k-t-d run: bash <(curl -s https://gist.githubusercontent.com/ammopt/8d74feff048b0732ed0362a204c3d8cd/raw/170cf83263c736ae8655b9e0332f4867a87435be/bug_39320_test_data.sh) 11) Refresh the homepage dashboard, check the 'Counts' are correct and links work, check that the 'Latest SUSHI Counter jobs' table now populates and links to the data provider as well as respective job. 12) For 'Licenses needing action', test different settings values and confirm the table filters the results as expected. 13) As before, click away and comeback to ERM home. Notice your 'Licenses needing actions' settings are kept as you left them. 14) For the 'eUsage report' widget, notice it says 'No saved reports are availble to run'. Create a new usage report at: <staff_url>/cgi-bin/koha/erm/eusage/reports 15) Click the 'Create report' tab, input the mandatory fields and enter a report name at the bottom of the page. Click 'Save report'. 16) Come back to ERM home. Notice the 'eUsage report' widget now shows a dropdown with a disabled 'Run' button. Pick the report you just created. Notice the 'Run' button is now enabled. Click 'Run'. Notice you get the report results screen (will show nothing as there is no usage data in this test exercise). 17) k-t-d: Run cypress tests (reset_all beforehand to clear test data): cypress run --spec t/cypress/integration/ERM/ModuleDashboard_spec.ts
This is not used: koha-tmpl/intranet-tmpl/prog/js/vue/fetch/api-client.js: background_jobs: new BackgroundJobsAPIClient(), And this looks wrong: fetch/background-jobs-api-client.js + endpoint: "agreements", Actually there is no filtering on the SUSHI Counter jobs
This looks wrong: fetch/background-jobs-api-client.js + endpoint: "agreements", Actually there is no filtering on the SUSHI Counter jobs
Question : The appearance is quite different from other Koha module homepages, and the ability to add "widgets" is interesting but unprecedented. Would this page remain the only one designed on this model, or could it be reused for other pages if relevant?
It could of course be useful to ad a widget "agreements needing action". It was too complex to add ?
This is a great idea and a very promising proposal! Creating a customizable dashboard for the ERM module would be a real asset for improving user experience and highlighting key data. However, there is currently no patch attached to this bug, so it’s not possible to proceed with testing or signoff. Could you please add the relevant patch
(In reply to Ludovic from comment #6) > This is a great idea and a very promising proposal! Creating a customizable > dashboard for the ERM module would be a real asset for improving user > experience and highlighting key data. However, there is currently no patch > attached to this bug, so it’s not possible to proceed with testing or > signoff. Could you please add the relevant patch Hi Ludovic, the development is the linked branch: https://github.com/openfifth/koha/commits/bug_39320 I'm updating the status here to BLOCKED to reflect the fact that this bug is waiting on bug 38201 so it can be picked up again.
Created attachment 187513 [details] [review] Bug 39320: Add new npm dependencies: vue-draggable-next vue-cookies
Created attachment 187514 [details] [review] Bug 39320: VueJS framework: Add option to have a component in dialog
Created attachment 187515 [details] [review] Bug 39320: Preparation: Add base and wrapper components As well as wrapper support components base-widget.js composable
Created attachment 187516 [details] [review] Bug 39320: Preparation: ERM counts API endpoint Required for the follow-up ERMCounts widget
Created attachment 187517 [details] [review] Bug 39320: Add ERM specific dashboard widgets
Created attachment 187518 [details] [review] Bug 39320: Add dashboard to ERM home
Created attachment 187519 [details] [review] Bug 39320: Add FA icons to ERM Wwould be great to improve the way we load icons to be available everywhere in Vue instead of just the specific module but on a different scope.
Created attachment 187520 [details] [review] Bug 39320: Add 'home' left menu item to ERM This is a bit hacky, but after hours of fighting with this I wasn't able to come up with a better soltuion. I suspect stores/navigation.js is doing too much and changing too much of the default router-link behaviour but I may be wrong.
Created attachment 187521 [details] [review] Bug 39320: Add Cypress tests Test plan: 1) Enable ERMModule 2) Visit ERM: <staff_url>/cgi-bin/koha/erm/erm.pl 3) Notice you now have a 'dashboard' homepage showing 4 different widgets 4) Click 'Open Widget Picker'. Remove and add different widgets. Close 5) Drag and drop the widgets around the dashboard, from left to right, top to bottom 6) Click the context menu (vertical dots) on each of the widgets. Click 'move to right', 'move to left', 'remove'. 7) On the 'Licenses needing action', notice there is a 'Settings' item option. Click that, test different settings values. Close settings. 8) Using the widget picker or context menu for the respective widgets, remove all widgets except one. Click away i.e. 'Agreements' on the left menu. Go back to home by clicking 'Home'. Notice your dashboard is as you left it. 9) Enable all widgets, rearrange them by dragging and dropping in different places. Repeat the previous exercise. Notice the dashboard keeps the widgets arranged as you left them. 10) Add some test data, on k-t-d run: bash <(curl -s https://gist.githubusercontent.com/ammopt/8d74feff048b0732ed0362a204c3d8cd/raw/a0347d4199d176dde58ea1d1437d2e903c101132/bug_39320_test_data.sh) 11) Refresh the homepage dashboard, check the 'Counts' are correct and links work, check that the 'Latest SUSHI Counter jobs' table now populates and links to the data provider as well as respective job. 12) For 'Licenses needing action', test different settings values and confirm the table filters the results as expected. 13) As before, click away and comeback to ERM home. Notice your 'Licenses needing actions' settings are kept as you left them. 14) For the 'eUsage report' widget, notice it says 'No saved reports are availble to run'. Create a new usage report at: <staff_url>/cgi-bin/koha/erm/eusage/reports 15) Click the 'Create report' tab, input the mandatory fields and enter a report name at the bottom of the page. Click 'Save report'. 16) Come back to ERM home. Notice the 'eUsage report' widget now shows a dropdown with a disabled 'Run' button. Pick the report you just created. Notice the 'Run' button is now enabled. Click 'Run'. Notice you get the report results screen (will show nothing as there is no usage data in this test exercise). 17) k-t-d: Run cypress tests (reset_all beforehand to clear test data): cypress run --spec t/cypress/integration/ERM/ModuleDashboard_spec.ts
Hi all, I've revisited this for the first time in ~6 months now that bug 38201 has been pushed and this is no longer blocked. The functionality is the same. The test plan is the same. Code changes that were done done recently: - Rebased on bug 38201 - Removed background-api-client (not needed, not used) - Rewritten BaseWidget component to base-widget.js composable, drop 'extends' - Rewritten remaining components to vuejs composition api (dropped options api) to match code patterns from 38201 - Rewritten how the widget picker modal works. It now uses the framework Dialog with a new option that allows for a component to be rendered in the dialog. - Some light updates to style, font-size, added widget icon option
Created attachment 187522 [details] [review] Bug 39320: Preparation: Add base and wrapper components As well as wrapper support components base-widget.js composable
Created attachment 187563 [details] [review] Bug 39320: Follow-up: Add 'move up' and 'move down' options
Created attachment 187568 [details] [review] Bug 39320: Remove 'extends' from the Home component
I have tested this on a Sandbox and it is working as expected.
Created attachment 187635 [details] [review] Bug 39320: Add new npm dependencies: vue-draggable-next vue-cookies Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Created attachment 187636 [details] [review] Bug 39320: VueJS framework: Add option to have a component in dialog Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Created attachment 187637 [details] [review] Bug 39320: Preparation: ERM counts API endpoint Required for the follow-up ERMCounts widget Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Created attachment 187638 [details] [review] Bug 39320: Add ERM specific dashboard widgets Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Created attachment 187639 [details] [review] Bug 39320: Add dashboard to ERM home Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Created attachment 187640 [details] [review] Bug 39320: Add FA icons to ERM Would be great to improve the way we load icons to be available everywhere in Vue instead of just the specific module but on a different scope. Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Created attachment 187641 [details] [review] Bug 39320: Add 'home' left menu item to ERM This is a bit hacky, but after hours of fighting with this I wasn't able to come up with a better soltuion. I suspect stores/navigation.js is doing too much and changing too much of the default router-link behaviour but I may be wrong. Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Created attachment 187642 [details] [review] Bug 39320: Add Cypress tests Test plan: 1) Enable ERMModule 2) Visit ERM: <staff_url>/cgi-bin/koha/erm/erm.pl 3) Notice you now have a 'dashboard' homepage showing 4 different widgets 4) Click 'Open Widget Picker'. Remove and add different widgets. Close 5) Drag and drop the widgets around the dashboard, from left to right, top to bottom 6) Click the context menu (vertical dots) on each of the widgets. Click 'move to right', 'move to left', 'remove'. 7) On the 'Licenses needing action', notice there is a 'Settings' item option. Click that, test different settings values. Close settings. 8) Using the widget picker or context menu for the respective widgets, remove all widgets except one. Click away i.e. 'Agreements' on the left menu. Go back to home by clicking 'Home'. Notice your dashboard is as you left it. 9) Enable all widgets, rearrange them by dragging and dropping in different places. Repeat the previous exercise. Notice the dashboard keeps the widgets arranged as you left them. 10) Add some test data, on k-t-d run: bash <(curl -s https://gist.githubusercontent.com/ammopt/8d74feff048b0732ed0362a204c3d8cd/raw/a0347d4199d176dde58ea1d1437d2e903c101132/bug_39320_test_data.sh) 11) Refresh the homepage dashboard, check the 'Counts' are correct and links work, check that the 'Latest SUSHI Counter jobs' table now populates and links to the data provider as well as respective job. 12) For 'Licenses needing action', test different settings values and confirm the table filters the results as expected. 13) As before, click away and comeback to ERM home. Notice your 'Licenses needing actions' settings are kept as you left them. 14) For the 'eUsage report' widget, notice it says 'No saved reports are availble to run'. Create a new usage report at: <staff_url>/cgi-bin/koha/erm/eusage/reports 15) Click the 'Create report' tab, input the mandatory fields and enter a report name at the bottom of the page. Click 'Save report'. 16) Come back to ERM home. Notice the 'eUsage report' widget now shows a dropdown with a disabled 'Run' button. Pick the report you just created. Notice the 'Run' button is now enabled. Click 'Run'. Notice you get the report results screen (will show nothing as there is no usage data in this test exercise). 17) k-t-d: Run cypress tests (reset_all beforehand to clear test data): cypress run --spec t/cypress/integration/ERM/ModuleDashboard_spec.ts Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Created attachment 187643 [details] [review] Bug 39320: Preparation: Add base and wrapper components As well as wrapper support components base-widget.js composable Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Created attachment 187644 [details] [review] Bug 39320: Follow-up: Add 'move up' and 'move down' options Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
Created attachment 187645 [details] [review] Bug 39320: Remove 'extends' from the Home component Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk>
QA comments: 1. :deep CSS rules should be removed (or moved to css/vue.css) 2. Do we really need the /erm/counts route. It needs tests (missing btw), swagger spec, Mojo controller. When we could just hit /agreements count=1 and retrieve the X-Base-Total-Count headers. 3. Why store in cookie when you can use localstorage? (and don't need the new deps vue-cookies) 4. drag&drop - you cannot select something in a box, it automatically drag the box. Cannot we have a dedicated zone/icon for that drag&drop behaviour? 5. From the "Counts" widget the links are "#", we should link to eg. /agreements instead. It also generates a vue warning: """ [Vue Router warn]: history.state seems to have been manually replaced without preserving the necessary values. Make sure to preserve existing history state if you are manually calling history.replaceState: history.replaceState(history.state, '', url) """ 6. Links from the "Licenses needed action" are correct (http://localhost:8081/cgi-bin/koha/erm/licenses/1), but it fully reloads the app 7. ModuleDashboard/Widgets/ERMCounts.vue {{ $__("There are") }} It won't be correctly translatable, use placeholder (%s and format) instead. By the way I don't think you should use definition.labelSingular and definition.labelPlural here. It won't be translatable correctly in some languages. I don't have a suggestion to fix this and keep it in a loop however. 8. Widgets/ERMLatestSUSHIJobs.vue a. 117 0: ["show"], What's that? I see that in the second column: <a href="/cgi-bin/koha/erm/eusage/usage_data_providers/undefined" class="show" draggable="false"></a> b. The "View" job button: you are using a "viewJob" function. I think you should use the callback instead, see Vendors/VendorResource.vue, "Receive shipments" 712 callback: ({ id }, dt, event) => { 713 event.preventDefault(); 714 window.location.href = `/cgi-bin/koha/acqui/parcels.pl?booksellerid=${id}`; 715 }, 9. Job status Widgets/ERMLatestSUSHIJobs.vue 44 const job_statuses = [ 53 function get_job_status(status) { 54 const status_lib = job_statuses.find(s => s._id === status); 55 return status_lib ? status_lib._str : status; 56 } If we add a new status we will obviously forget this place. I wouldn't silently ignore an unknown status. Either we implement +string for /jobs or we raise an error. Also we are using the only occurrence of escapeHtml in all vue files, I would use escape_str (`return get_job_status(row.status).escapeHtml();`) 10. Cypress tests are failing: 1) ERM Module Dashboard Run eUsage report: AssertionError: Timed out retrying after 10000ms: Expected to find element: `.widget#ERMRunUsageReport .widget-content`, but never found it.
(In reply to Jonathan Druart from comment #33) > QA comments: > Either we implement +string for /jobs or we raise an error. Might not be a good idea to return non-translatable string actually. Or we could return them translated but I am not sure this is implemented already.
Created attachment 188094 [details] [review] Bug 39320: (QA follow-up): Remove deep CSS rules This CSS is in vue.css anyway
Created attachment 188095 [details] [review] Bug 39320: (QA follow-up): Add counts API tests prove t/db_dependent/api/v1/erm_counts.t
Created attachment 188096 [details] [review] Bug 39320: (QA follow-up): Use localStorage instead of vue-cookies
Created attachment 188097 [details] [review] Bug 39320: (QA follow-up): Fix counts links Use router-link instead of href='#'
Created attachment 188098 [details] [review] Bug 39320: (QA follow-up) Prevent full page reload when clicking a license
Created attachment 188099 [details] [review] Bug 39320: (QA follow-up): Use %s and format for 'There are' translatability
Created attachment 188100 [details] [review] Bug 39320: (QA follow-up): Removed unused show event
Created attachment 188101 [details] [review] Bug 39320: (QA follow-up): Fix cypress These tests have always passed for me running cypress on UI, before and after this patch. CLI tests however, were flaky and often times failed. It appears this is because each module refresh was flooding the server with HTTP requests causing the whole thing to bottleneck (?) With this patch, all widget requests are stubbed for every widget test, and tests now pass on CLI consistently. Passed 10/10 for me, whereas before it failed more often than passed. Run this: for i in {1..10}; do cypress run --spec t/cypress/integration/ERM/ModuleDashboard_spec.ts; done
Created attachment 188102 [details] [review] 39320: (QA follow-up): Add a handler for drag and drop
Created attachment 188103 [details] [review] Bug 39320: (QA follow-up): Add a handler for drag and drop
Created attachment 188104 [details] [review] Bug 39320: (QA follow-up): Use escape_str instead of .escapeHtml
(In reply to Jonathan Druart from comment #33) > QA comments: > > 1. :deep CSS rules should be removed (or moved to css/vue.css) Patch added > 2. Do we really need the /erm/counts route. It needs tests (missing btw), > swagger spec, Mojo controller. When we could just hit /agreements count=1 > and retrieve the X-Base-Total-Count headers. Patch with tests added. Yes we need, otherwise it's 6 HTTP requests instead of 1 just to get counts. > 3. Why store in cookie when you can use localstorage? (and don't need the > new deps vue-cookies) Patch added > 4. drag&drop - you cannot select something in a box, it automatically drag > the box. Cannot we have a dedicated zone/icon for that drag&drop behaviour? Patch added > 5. From the "Counts" widget the links are "#", we should link to eg. > /agreements instead. > It also generates a vue warning: > """ > [Vue Router warn]: history.state seems to have been manually replaced > without preserving the necessary values. Make sure to preserve existing > history state if you are manually calling history.replaceState: > > history.replaceState(history.state, '', url) > """ Patch added. I've not seen the vue router warn before or after. > 6. Links from the "Licenses needed action" are correct > (http://localhost:8081/cgi-bin/koha/erm/licenses/1), but it fully reloads > the app Patch added. > 7. ModuleDashboard/Widgets/ERMCounts.vue > {{ $__("There are") }} > It won't be correctly translatable, use placeholder (%s and format) instead. > By the way I don't think you should use definition.labelSingular and > definition.labelPlural here. It won't be translatable correctly in some > languages. > I don't have a suggestion to fix this and keep it in a loop however. Patch added. > 8. Widgets/ERMLatestSUSHIJobs.vue > a. 117 0: ["show"], > What's that? > I see that in the second column: > <a href="/cgi-bin/koha/erm/eusage/usage_data_providers/undefined" > class="show" draggable="false"></a> Patch added. > b. The "View" job button: you are using a "viewJob" function. I think you > should use the callback instead, see Vendors/VendorResource.vue, "Receive > shipments" > 712 callback: ({ id }, dt, event) => { > 713 event.preventDefault(); > 714 window.location.href = > `/cgi-bin/koha/acqui/parcels.pl?booksellerid=${id}`; > 715 }, Using viewJob is the same as using 'show' which is what we're using elsewhere in the framework for KohaTable action buttons. > 9. Job status > Widgets/ERMLatestSUSHIJobs.vue > 44 const job_statuses = [ > > 53 function get_job_status(status) { > 54 const status_lib = job_statuses.find(s => s._id === status); > 55 return status_lib ? status_lib._str : status; > 56 } > > If we add a new status we will obviously forget this place. I wouldn't > silently ignore an unknown status. It's not silently ignoring an unknown status. It prints the untranslatable status code if it doesn't find a match. > Either we implement +string for /jobs or we raise an error. > Also we are using the only occurrence of escapeHtml in all vue files, I > would use escape_str (`return get_job_status(row.status).escapeHtml();`) Patch added. > 10. Cypress tests are failing: > 1) ERM Module Dashboard > > Run eUsage report: > AssertionError: Timed out retrying after 10000ms: Expected to find > element: `.widget#ERMRunUsageReport .widget-content`, but never found it. Patch added.
(In reply to Pedro Amorim from comment #46) Thanks for the quick follow-ups! > > 2. Do we really need the /erm/counts route. It needs tests (missing btw), > > swagger spec, Mojo controller. When we could just hit /agreements count=1 > > and retrieve the X-Base-Total-Count headers. > > Patch with tests added. Yes we need, otherwise it's 6 HTTP requests instead > of 1 just to get counts. Not convince. I'd like Tomas's POV on /counts. > > 5. From the "Counts" widget the links are "#", we should link to eg. > > /agreements instead. > > It also generates a vue warning: > > """ > > [Vue Router warn]: history.state seems to have been manually replaced > > without preserving the necessary values. Make sure to preserve existing > > history state if you are manually calling history.replaceState: > > > > history.replaceState(history.state, '', url) > > """ > > Patch added. I've not seen the vue router warn before or after. I've seen it again, then didn't recreate. I also got "[Vue Router warn]: Discarded invalid param(s) "agreement_id" when navigating." when moving from /agreements to /home. Logging it here for the records, not blocker. > > 7. ModuleDashboard/Widgets/ERMCounts.vue > > {{ $__("There are") }} > > It won't be correctly translatable, use placeholder (%s and format) instead. > > By the way I don't think you should use definition.labelSingular and > > definition.labelPlural here. It won't be translatable correctly in some > > languages. > > I don't have a suggestion to fix this and keep it in a loop however. > > Patch added. ``` 120 const joined = parts.join(", "); 121 return $__("There are %s.").format(joined); ``` this is still impossible to translate I have no idea how to fix that, or... we cheat? what about moving the list to ul/li? ``` There are: 1 agreement 0 license etc. ``` > > 8. Widgets/ERMLatestSUSHIJobs.vue > > b. The "View" job button: you are using a "viewJob" function. I think you > > should use the callback instead, see Vendors/VendorResource.vue, "Receive > > shipments" > > 712 callback: ({ id }, dt, event) => { > > 713 event.preventDefault(); > > 714 window.location.href = > > `/cgi-bin/koha/acqui/parcels.pl?booksellerid=${id}`; > > 715 }, > > Using viewJob is the same as using 'show' which is what we're using > elsewhere in the framework for KohaTable action buttons. Yes, but it's more verbose... Not blocker. > > 9. Job status > > Widgets/ERMLatestSUSHIJobs.vue > > 44 const job_statuses = [ > > > > 53 function get_job_status(status) { > > 54 const status_lib = job_statuses.find(s => s._id === status); > > 55 return status_lib ? status_lib._str : status; > > 56 } > > > > If we add a new status we will obviously forget this place. I wouldn't > > silently ignore an unknown status. > > It's not silently ignoring an unknown status. It prints the untranslatable > status code if it doesn't find a match. Yes, so it's not visible, and for me it means it's ignored :) We are supposed to have this list updated when we add a new status to the job table. If we add "paused", devs will look at the regular pl/pm/tt, but for sure forget this vue file. I would display "Status unknown" or "Status unknown: ${status}" (or even alert) so we clearly see there is something missing in the code.
11. drap&drop If all widgets are on the left you cannot drop in the right column.
Created attachment 188151 [details] [review] Bug 39320: (follow-up) Add counts API tests Prevent tests to fail if data in DB
Created attachment 188279 [details] [review] Bug 39320: (QA follow-up): Add an empty drop area placeholder
Created attachment 188280 [details] [review] Bug 39320: (QA follow-up): Display 'Uknown status' if not one of the expected job statuses
Created attachment 188281 [details] [review] Bug 39320: (QA follow-up): Fix ERMCounts translatability
Created attachment 188282 [details] [review] Bug 39320: (QA follow-up): Style new ERMCounts list
(In reply to Jonathan Druart from comment #47) > > ``` > 120 const joined = parts.join(", "); > 121 return $__("There are %s.").format(joined); > ``` > > this is still impossible to translate > > I have no idea how to fix that, or... we cheat? > > what about moving the list to ul/li? > > ``` > There are: > > 1 agreement > 0 license > etc. > ``` > Fixed by: 1) Dropped the previous 'Use %s and format for 'There are' translatability' patch. 2) New patch 'Fix ERMCounts translatability' + 'Style new ERMCounts list' patches > > Yes, so it's not visible, and for me it means it's ignored :) > We are supposed to have this list updated when we add a new status to the > job table. If we add "paused", devs will look at the regular pl/pm/tt, but > for sure forget this vue file. I would display "Status unknown" or "Status > unknown: ${status}" (or even alert) so we clearly see there is something > missing in the code. Patch 'Display Uknown status' added. (In reply to Jonathan Druart from comment #48) > 11. drap&drop > If all widgets are on the left you cannot drop in the right column. Not true, you can, but maybe it's visually obvious. Patch 'Add an empty drop area placeholder' added.
Previous comment should read: but maybe it's not* visually obvious
Created attachment 188283 [details] [review] Bug 39320: (QA follow-up): Fix ERMCounts translatability
Created attachment 188284 [details] [review] Bug 39320: (QA follow-up): Fix ERMCounts translatability
Created attachment 188285 [details] [review] Bug 39320: (QA follow-up): Style new ERMCounts list
(In reply to Pedro Amorim from comment #57) > Created attachment 188284 [details] [review] [review] > Bug 39320: (QA follow-up): Fix ERMCounts translatability Squashed cypress tests updates to this patch.
Created attachment 188312 [details] [review] Bug 39320: Use __n for languages with 3 plural forms (or more)
(In reply to Pedro Amorim from comment #54) > (In reply to Jonathan Druart from comment #47) > > > > ``` > > 120 const joined = parts.join(", "); > > 121 return $__("There are %s.").format(joined); > > ``` > > > > this is still impossible to translate > > > > I have no idea how to fix that, or... we cheat? > > > > what about moving the list to ul/li? > > > > ``` > > There are: > > > > 1 agreement > > 0 license > > etc. > > ``` > > > > Fixed by: > 1) Dropped the previous 'Use %s and format for 'There are' translatability' > patch. > 2) New patch 'Fix ERMCounts translatability' + 'Style new ERMCounts list' > patches I've provided a patch to correctly fix the translatability, there are languages with many different forms ;) > (In reply to Jonathan Druart from comment #48) > > 11. drap&drop > > If all widgets are on the left you cannot drop in the right column. > > Not true, you can, but maybe it's visually obvious. Patch 'Add an empty drop > area placeholder' added. I can assure you that I couldn't drop a widget to the right when they were all on the left. Now with the last patches we have a "Drop widget here" area that is always displayed. IMO it should be displayed only when dragging.
(In reply to Jonathan Druart from comment #61) > (In reply to Pedro Amorim from comment #54) > > (In reply to Jonathan Druart from comment #48) > > > 11. drap&drop > > > If all widgets are on the left you cannot drop in the right column. > > > > Not true, you can, but maybe it's visually obvious. Patch 'Add an empty drop > > area placeholder' added. > > I can assure you that I couldn't drop a widget to the right when they were > all on the left. > > Now with the last patches we have a "Drop widget here" area that is always > displayed. IMO it should be displayed only when dragging. There is the "move right" workaround. We can move that to a separate bug if it is not trivial (but then I would suggest to obsolete 'Add an empty drop area placeholder')
Created attachment 188315 [details] [review] Bug 39320: (QA follow-up): Only show 'drop widgets here' area when actively dragging
(In reply to Jonathan Druart from comment #47) > (In reply to Pedro Amorim from comment #46) > > > 2. Do we really need the /erm/counts route. It needs tests (missing btw), > > > swagger spec, Mojo controller. When we could just hit /agreements count=1 > > > and retrieve the X-Base-Total-Count headers. > > > > Patch with tests added. Yes we need, otherwise it's 6 HTTP requests instead > > of 1 just to get counts. > > Not convince. I'd like Tomas's POV on /counts. Tomas, we are still waiting for you here.
Thank you for the QA review Joubu, your time spent here is greatly appreciated.
Created attachment 188372 [details] [review] Bug 39320: (QA follow-up): Remove big 'Processing...' when loading table widgets This prevents the big centered 'Processing...' from showing when moving widgets around the dashboard. Keeps the 'Loading...' at the widget level for table widgets, same as any other widget.
Created attachment 188375 [details] [review] Bug 39320: (QA follow-up): Fix widget with settings This was broken from the rewrite into bug 38201. Fixed here. Without this patch, if you click 'settings' on the licenses widgets and pick different values, you get errors on the console and everything breaks
Created attachment 188376 [details] [review] Bug 39320: (QA follow-up): Fix widget with settings This was broken from the rewrite into bug 38201. Fixed here. Without this patch, if you click 'settings' on the licenses widgets and pick different values, you get errors on the console and everything breaks.
(In reply to Jonathan Druart from comment #64) > (In reply to Jonathan Druart from comment #47) > > (In reply to Pedro Amorim from comment #46) > > > > 2. Do we really need the /erm/counts route. It needs tests (missing btw), > > > > swagger spec, Mojo controller. When we could just hit /agreements count=1 > > > > and retrieve the X-Base-Total-Count headers. > > > > > > Patch with tests added. Yes we need, otherwise it's 6 HTTP requests instead > > > of 1 just to get counts. > > > > Not convince. I'd like Tomas's POV on /counts. > > Tomas, we are still waiting for you here. I like the idea of always sending the count headers back. Less work too.
Created attachment 188637 [details] [review] Bug 39320: (ERMCounts QA follow-up): Use existing count method from the API client
Created attachment 188638 [details] [review] Bug 39320: (ERMCounts QA follow-up): Remove documents as an endpoint for listing that does not exist
Created attachment 188639 [details] [review] Bug 39320: (ERMCounts QA follow-up): Reimplement loading. Now present 'Loading' for each resource count that's waiting, rather than hang the whole widget waiting for all responses
Created attachment 188640 [details] [review] Bug 39320: (ERMCounts QA follow-up): Update Cypress cypress run --spec t/cypress/integration/ERM/ModuleDashboard_spec.ts
Created attachment 188641 [details] [review] Bug 39320: (ERMCounts QA follow-up): Clean-up previous ERMCounts implementation
Thank you Joubu and Tomas. I've reimplemented the ERMCounts to utilize the existing count headers instead of having its own erm/counts endpoint.
Created attachment 188830 [details] [review] Bug 39320: Add new npm dependencies: vue-draggable-next vue-cookies Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188831 [details] [review] Bug 39320: VueJS framework: Add option to have a component in dialog Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188832 [details] [review] Bug 39320: Preparation: ERM counts API endpoint Required for the follow-up ERMCounts widget Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188833 [details] [review] Bug 39320: Add ERM specific dashboard widgets Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188834 [details] [review] Bug 39320: Add dashboard to ERM home Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188835 [details] [review] Bug 39320: Add FA icons to ERM Would be great to improve the way we load icons to be available everywhere in Vue instead of just the specific module but on a different scope. Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188836 [details] [review] Bug 39320: Add 'home' left menu item to ERM This is a bit hacky, but after hours of fighting with this I wasn't able to come up with a better soltuion. I suspect stores/navigation.js is doing too much and changing too much of the default router-link behaviour but I may be wrong. Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188837 [details] [review] Bug 39320: Add Cypress tests Test plan: 1) Enable ERMModule 2) Visit ERM: <staff_url>/cgi-bin/koha/erm/erm.pl 3) Notice you now have a 'dashboard' homepage showing 4 different widgets 4) Click 'Open Widget Picker'. Remove and add different widgets. Close 5) Drag and drop the widgets around the dashboard, from left to right, top to bottom 6) Click the context menu (vertical dots) on each of the widgets. Click 'move to right', 'move to left', 'remove'. 7) On the 'Licenses needing action', notice there is a 'Settings' item option. Click that, test different settings values. Close settings. 8) Using the widget picker or context menu for the respective widgets, remove all widgets except one. Click away i.e. 'Agreements' on the left menu. Go back to home by clicking 'Home'. Notice your dashboard is as you left it. 9) Enable all widgets, rearrange them by dragging and dropping in different places. Repeat the previous exercise. Notice the dashboard keeps the widgets arranged as you left them. 10) Add some test data, on k-t-d run: bash <(curl -s https://gist.githubusercontent.com/ammopt/8d74feff048b0732ed0362a204c3d8cd/raw/a0347d4199d176dde58ea1d1437d2e903c101132/bug_39320_test_data.sh) 11) Refresh the homepage dashboard, check the 'Counts' are correct and links work, check that the 'Latest SUSHI Counter jobs' table now populates and links to the data provider as well as respective job. 12) For 'Licenses needing action', test different settings values and confirm the table filters the results as expected. 13) As before, click away and comeback to ERM home. Notice your 'Licenses needing actions' settings are kept as you left them. 14) For the 'eUsage report' widget, notice it says 'No saved reports are availble to run'. Create a new usage report at: <staff_url>/cgi-bin/koha/erm/eusage/reports 15) Click the 'Create report' tab, input the mandatory fields and enter a report name at the bottom of the page. Click 'Save report'. 16) Come back to ERM home. Notice the 'eUsage report' widget now shows a dropdown with a disabled 'Run' button. Pick the report you just created. Notice the 'Run' button is now enabled. Click 'Run'. Notice you get the report results screen (will show nothing as there is no usage data in this test exercise). 17) k-t-d: Run cypress tests (reset_all beforehand to clear test data): cypress run --spec t/cypress/integration/ERM/ModuleDashboard_spec.ts Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188838 [details] [review] Bug 39320: Preparation: Add base and wrapper components As well as wrapper support components base-widget.js composable Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188839 [details] [review] Bug 39320: (follow-up) Add 'move up' and 'move down' options Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188840 [details] [review] Bug 39320: Remove 'extends' from the Home component Sponsored-by: UKHSA (UK Health Security Agency) Signed-off-by: Jeremy Evans <Jeremy.Evans@ukhsa.gov.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188841 [details] [review] Bug 39320: (QA follow-up): Remove deep CSS rules This CSS is in vue.css anyway Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188842 [details] [review] Bug 39320: (QA follow-up): Add counts API tests prove t/db_dependent/api/v1/erm_counts.t Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188843 [details] [review] Bug 39320: (QA follow-up): Use localStorage instead of vue-cookies Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188844 [details] [review] Bug 39320: (QA follow-up): Fix counts links Use router-link instead of href='#' Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188845 [details] [review] Bug 39320: (QA follow-up) Prevent full page reload when clicking a license Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188846 [details] [review] Bug 39320: (QA follow-up): Removed unused show event Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188847 [details] [review] Bug 39320: (QA follow-up): Fix cypress These tests have always passed for me running cypress on UI, before and after this patch. CLI tests however, were flaky and often times failed. It appears this is because each module refresh was flooding the server with HTTP requests causing the whole thing to bottleneck (?) With this patch, all widget requests are stubbed for every widget test, and tests now pass on CLI consistently. Passed 10/10 for me, whereas before it failed more often than passed. Run this: for i in {1..10}; do cypress run --spec t/cypress/integration/ERM/ModuleDashboard_spec.ts; done Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188848 [details] [review] Bug 39320: (QA follow-up): Add a handler for drag and drop Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188849 [details] [review] Bug 39320: (QA follow-up): Use escape_str instead of .escapeHtml Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188850 [details] [review] Bug 39320: (follow-up) Add counts API tests Prevent tests to fail if data in DB Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188851 [details] [review] Bug 39320: (QA follow-up): Add an empty drop area placeholder Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188852 [details] [review] Bug 39320: (QA follow-up): Display 'Uknown status' if not one of the expected job statuses Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188853 [details] [review] Bug 39320: (QA follow-up): Fix ERMCounts translatability Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188854 [details] [review] Bug 39320: (QA follow-up): Style new ERMCounts list Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188855 [details] [review] Bug 39320: Use __n for languages with 3 plural forms (or more) Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188856 [details] [review] Bug 39320: (QA follow-up): Only show 'drop widgets here' area when actively dragging Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188857 [details] [review] Bug 39320: (QA follow-up): Remove big 'Processing...' when loading table widgets This prevents the big centered 'Processing...' from showing when moving widgets around the dashboard. Keeps the 'Loading...' at the widget level for table widgets, same as any other widget. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188858 [details] [review] Bug 39320: (QA follow-up): Fix widget with settings This was broken from the rewrite into bug 38201. Fixed here. Without this patch, if you click 'settings' on the licenses widgets and pick different values, you get errors on the console and everything breaks. Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188859 [details] [review] Bug 39320: (QA follow-up): Use existing count method from the API client Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188860 [details] [review] Bug 39320: (QA follow-up): Remove documents as an endpoint for listing that does not exist Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188861 [details] [review] Bug 39320: (QA follow-up): Reimplement loading. Now present 'Loading' for each resource count that's waiting, rather than hang the whole widget waiting for all responses Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188862 [details] [review] Bug 39320: (QA follow-up): Update Cypress cypress run --spec t/cypress/integration/ERM/ModuleDashboard_spec.ts Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188863 [details] [review] Bug 39320: (QA follow-up): Clean-up previous ERMCounts implementation Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Created attachment 188864 [details] [review] Bug 39320: Fix ERM breadcrumb causing page reload The main ERM route was missing a 'name' property, causing the navigation store to treat it as an external path (.pl extension). This resulted in breadcrumb links using href instead of router-link, causing unwanted page reloads. To test: 1. Navigate to ERM module 2. Go to any sub-section (Agreements, Licenses, etc.) 3. Click on 'E-resource management' in the breadcrumb => FAIL: Notice the page reloads! 4. Apply the patch 5. Repeat steps 1-3 => SUCCES: Verify the page doesn't reload and navigation works smoothly 6. Sign off :-D Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io>
Hi, I tested the feature and am happy with what I see * functionality * aesthetics * usability The only annoyance I found was the page reload when using the breadcrumb. I'm not sure if it was a 'feature', but I fixed it with a follow-up. I also adjusted the commit titles to silence the QA tools a bit. At this point I think this deserves some squashing too. But that's not a blocker anyway. I wasn't able to run the tests successfully. What I did: 1. Apply the patches 2. Run: $ ktd --shell k$ sudo yarn install --modules-folder /kohadevbox/node_modules k$ yarn build 3. Run: k$ run_cypress --spec t/cypress/integration/ERM/ModuleDashboard_spec.ts => FAIL: The 'Latest SUSHI Counter jobs' tests fail with a page error: ``` Error: The following error originated from your application code, not from Cypress. It was caused by an unhandled promise rejection. > JS Warning detected: "[Vue warn]: Unhandled error during execution of setup function" "\n" " at <RouterLink" "key=0" "to=" {"name":"ERMHome","params":{"erm_usage_data_provider_id":"1"}} "class=\"\"" ">" "\n" " at <NavigationItem" "key=0" "item=" [object Object] "params=" {"erm_usage_data_provider_id":"1"} ">" "\n" " at <Breadcrumbs>" "\n" " at <App>" When Cypress detects uncaught errors originating from your application it will automatically fail the current test. This behavior is configurable, and you can choose to turn this off by listening to the `uncaught:exception` event. ``` Hope it is an easy fix for y'all. Otherwise, great job! Hope to see some work for the admin home page and we start unifying it all :-D
I'm not familiar enough with the eUsage section and data providers. I tried to add some things manually but didn't manage to make 'Run eUsage report' do anything. And there's some errors in the browser console that might be related to the tests failure and this behavior. Again, as I'm not familiar it might probably be me (the functionality side, not the errors in the console).