We should convert the left menus to Vue islands so that the components can be used both in Koha and in Vue apps where needed
Created attachment 176877 [details] [review] Bug 38941: Convert the acquisitions menu to a Vue web component This is an example patch for creating a Vue island component that can be used throughout Koha. The component can then also be used in the work in Bug 38010 to migrate Vendors to Vue. This removes duplication of code by having a .inc file as well as a vue component to maintain Test plan: 1) Apply all patches including dependencies 2) Bug 37911 requires a Vue upgrade so you will need to run yarn install, then run a yarn build 3) Navigate to the acquisitions home page 4) The left menu should appear as before 5) Switch on the EDIFACT syspref - the acq menu should now display the edifact related options 6) Try logging in as a new patron with reduced permissions, the menu should update correctly according to the permissions
Created attachment 176878 [details] [review] Bug 38941: Add a cypress test
This is a POC - interested to hear any ideas for improvement. I considered using the NavigationItem component to render the links but it changes the HTML structure slightly and is largely dependent on the Vue router
Created attachment 178243 [details] [review] Bug 38941: Convert the acquisitions menu to a Vue web component This is an example patch for creating a Vue island component that can be used throughout Koha. The component can then also be used in the work in Bug 38010 to migrate Vendors to Vue. This removes duplication of code by having a .inc file as well as a vue component to maintain Test plan: 1) Apply all patches including dependencies 2) Bug 37911 requires a Vue upgrade so you will need to run yarn install, then run a yarn build 3) Navigate to the acquisitions home page 4) The left menu should appear as before 5) Switch on the EDIFACT syspref - the acq menu should now display the edifact related options 6) Try logging in as a new patron with reduced permissions, the menu should update correctly according to the permissions
Created attachment 178244 [details] [review] Bug 38941: Add a cypress test
Tried testing on ByWater Solutions sandbox and on a local git build, but both end up with an error and the left menu doesn't appear at all. Seems like the code is looking for a module that's not there? Uncaught Error: Cannot find module '../stores/permissions' webpackMissingModule https://staff-bugiton.sandbox.bywatersolutions.com/intranet-tmpl/prog/js/vue/dist/islands_24.1200012.js:16487 <anonymous> https://staff-bugiton.sandbox.bywatersolutions.com/intranet-tmpl/prog/js/vue/dist/islands_24.1200012.js:16487 <anonymous> https://staff-bugiton.sandbox.bywatersolutions.com/intranet-tmpl/prog/js/vue/dist/islands_24.1200012.js:16774 islands_24.1200012.js:16487:50
Hi Janne, this bug has 2 dependencies that are not resolved yet. The sandboxes are not very clear when there is a conflict on applying the patches. Maybe have a look at the git log if you can.
Created attachment 181491 [details] [review] Bug 38941: Add a cypress test
Created attachment 181492 [details] [review] Bug 38941: Convert the acquisitions menu to a Vue web component This is an example patch for creating a Vue island component that can be used throughout Koha. The component can then also be used in the work in Bug 38010 to migrate Vendors to Vue. This removes duplication of code by having a .inc file as well as a vue component to maintain Test plan: 1) Apply all patches including dependencies 2) Bug 37911 requires a Vue upgrade so you will need to run yarn install, then run a yarn build 3) Navigate to the acquisitions home page 4) The left menu should appear as before 5) Switch on the EDIFACT syspref - the acq menu should now display the edifact related options 6) Try logging in as a new patron with reduced permissions, the menu should update correctly according to the permissions
Created attachment 181493 [details] [review] Bug 38941: Add a cypress test
Created attachment 181626 [details] [review] Bug 38941: Rebase for follow-up to bug 37911 Bug 37911 added a follow-up that removed the original entry point from rspack to use the esm.js files
Created attachment 181627 [details] [review] Bug 38941: Convert the acquisitions menu to a Vue web component This is an example patch for creating a Vue island component that can be used throughout Koha. The component can then also be used in the work in Bug 38010 to migrate Vendors to Vue. This removes duplication of code by having a .inc file as well as a vue component to maintain Test plan: 1) Apply all patches including dependencies 2) Bug 37911 requires a Vue upgrade so you will need to run yarn install, then run a yarn build 3) Navigate to the acquisitions home page 4) The left menu should appear as before 5) Switch on the EDIFACT syspref - the acq menu should now display the edifact related options 6) Try logging in as a new patron with reduced permissions, the menu should update correctly according to the permissions Signed-off-by: Michaela <michaela.sieber@kit.edu>
Created attachment 181628 [details] [review] Bug 38941: Add a cypress test Signed-off-by: Michaela <michaela.sieber@kit.edu>
Created attachment 181629 [details] [review] Bug 38941: Rebase for follow-up to bug 37911 Bug 37911 added a follow-up that removed the original entry point from rspack to use the esm.js files Signed-off-by: Michaela <michaela.sieber@kit.edu>
Created attachment 181711 [details] [review] Bug 38941: Add a cypress test
Created attachment 181724 [details] [review] Bug 38941: Convert the acquisitions menu to a Vue web component This is an example patch for creating a Vue island component that can be used throughout Koha. The component can then also be used in the work in Bug 38010 to migrate Vendors to Vue. This removes duplication of code by having a .inc file as well as a vue component to maintain Test plan: 1) Apply all patches including dependencies 2) Bug 37911 requires a Vue upgrade so you will need to run yarn install, then run a yarn build 3) Navigate to the acquisitions home page 4) The left menu should appear as before 5) Switch on the EDIFACT syspref - the acq menu should now display the edifact related options 6) Try logging in as a new patron with reduced permissions, the menu should update correctly according to the permissions Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 181725 [details] [review] Bug 38941: Add a cypress test Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 181726 [details] [review] Bug 38941: Rebase for follow-up to bug 37911 Bug 37911 added a follow-up that removed the original entry point from rspack to use the esm.js files Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
1) QA script FAIL koha-tmpl/intranet-tmpl/prog/en/includes/acquisitions-menu.inc FAIL filters missing_filter at line 25 ([% SET islands = Asset.js("js/vue/dist/islands.esm.js").match('(src="([^"]+)")').1 %] <script src="[% islands %]" type="module">) missing_filter at line 28 ( import { hydrate } from "[% islands %]";) I fixed it with the automated script in a separate commit. Please check! 2) Question about permission check We have this here, which appears to be a bit of a double up on first glance - do we need this in order to over both TT and vue cases where the menu is used? + suggestionscreate="[% CAN_user_suggestions_suggestions_create | html %]" + suggestionsmanage="[% CAN_user_suggestions_suggestions_manage | html %]" + suggestionsdelete="[% CAN_user_suggestions_suggestions_delete | html %]" And then: + v-if=" + suggestionscreate || + suggestionsmanage || + suggestionsdelete || + isUserPermitted( + 'CAN_user_suggestions_suggestions_create' + ) || + isUserPermitted( + 'CAN_user_suggestions_suggestions_manage' + ) || + isUserPermitted('CAN_user_suggestions_suggestions_delete') + " + >
1) - I can see now that this code is removed by 38010. So dropping my follow-up.
Pushed for 25.05! Well done everyone, thank you!
Enhancement not pushed to 24.11.x
Little question... I notice this bug depends on bug 38930, but then we're using Template::Toolkit to set attributes mapped to permissions on the acquisitions-menu web component. Looking again at the template, it looks like some parts of the template use the permission store from bug 38930 and some use the attributes/properties for the web component? Seems like it's only half-implemented here?
Hi David, the component that is used in the custom element can be used either as a Vue custom element in template toolkit or as a component in a Vue application. When used in a Vue app we get access to the pinia stores (permissionsStore) and can use the method provided by the permissionsStore to check permissions. Outside of a vue app however, we don't get access to a populated permissionsStore so we need to be able to accept the permissions from the template toolkit variables as component props
(In reply to Matt Blenkinsop from comment #24) > the component that is used in the custom element can be used > either as a Vue custom element in template toolkit or as a component in a > Vue application. When used in a Vue app we get access to the pinia stores > (permissionsStore) and can use the method provided by the permissionsStore > to check permissions. Outside of a vue app however, we don't get access to a > populated permissionsStore so we need to be able to accept the permissions > from the template toolkit variables as component props Looking back at the patch again, I see that it does support both methodologies for all cases. On first read, I thought it was using one methodology in one place and the other methodology in other places. My bad! That said, can you explain things more? In the case of acquisitions menu, it looks like you do have access to the pinia stores, but that it's just not being populated? It also looks like it gets mounted as a single component Vue app in the end? Looking at bug 38930, it looks like the permissionsStore is not used anywhere currently? There's the ERM example which shows an ERM-specific implementation for using the permissionsStore, but that seems suboptimal. When I first heard of bug 38930, I thought it was going to introduce a core API endpoint for fetching current user details and then put the permissions in the permissionsStore for use by Vue components. Even outside of Vue, I reckon having an API endpoint to fetch that information would be useful.
> That said, can you explain things more? In the case of acquisitions menu, it > looks like you do have access to the pinia stores, but that it's just not > being populated? It also looks like it gets mounted as a single component > Vue app in the end? Looking at bug 38930, it looks like the permissionsStore > is not used anywhere currently? There's the ERM example which shows an > ERM-specific implementation for using the permissionsStore, but that seems > suboptimal. > > When I first heard of bug 38930, I thought it was going to introduce a core > API endpoint for fetching current user details and then put the permissions > in the permissionsStore for use by Vue components. Even outside of Vue, I > reckon having an API endpoint to fetch that information would be useful. Its currently being used by the Vendors Vue module - when the acquisitions menu is loaded as part of the vendors vue app we have access to the permissions store as it is populated by the module's config endpoint and the menu is just used as a generic vue component rather than a custom element. When it is used as a custom element, the permissionsStore is technically available but isn't populated as there is no specific vue app with a config endpoint to populate from. A future enhancement could certainly be to update the permissions store to self-populate and remove the need for the TT variables
(In reply to Matt Blenkinsop from comment #26) > Its currently being used by the Vendors Vue module - Ah I see it now. Both the dependency chain in Bugzilla and the import of ./Islands/AcquisitionsMenu.vue in koha-tmpl/intranet-tmpl/prog/js/vue/components/LeftMenu.vue. > when the acquisitions > menu is loaded as part of the vendors vue app we have access to the > permissions store as it is populated by the module's config endpoint and the > menu is just used as a generic vue component rather than a custom element. I see what you mean when looking at /cgi-bin/koha/acqui/acqui-home.pl vs /cgi-bin/koha/acquisition/vendors?supplier= > When it is used as a custom element, the permissionsStore is technically > available but isn't populated as there is no specific vue app with a config > endpoint to populate from. A future enhancement could certainly be to update > the permissions store to self-populate and remove the need for the TT > variables I reckon that would be a much more DRY option for sure. Anyway, cool beans. Thanks for explaining all that.