Currently the Vue router is set up with urls assigned to components with no "name" property. This means that in the components, where a router-link tag is used the full URL has been written out again. In the event that a url needs to be changed in the router, the url will also have to be changed in every component that uses that route. By using the "name" property we can avoid this and make maintenance of the router much more efficient.
Created attachment 146439 [details] [review] Bug_32932: Update router to use object references This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings
Created attachment 146442 [details] [review] Bug_32932: Update router to use object references This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings
Created attachment 146616 [details] [review] Bug_32932: Update router to use object references This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings
Created attachment 146617 [details] [review] Bug_32932: Update router to use object references - agreements This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings
Created attachment 146618 [details] [review] Bug 32932: Update router to use name property for licences As per previous commit for agreements
Created attachment 146624 [details] [review] Bug 32932: Update router to use name property for eholdings As per previous commit for agreements
Created attachment 146625 [details] [review] Bug 32932: Update ermmain to use router name property Updating the router links in the homepage as per previous commits
Created attachment 146965 [details] [review] Bug_32932: Update router to use object references - agreements This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings
Created attachment 146966 [details] [review] Bug 32932: Update router to use name property for licences As per previous commit for agreements
Created attachment 146967 [details] [review] Bug 32932: Update router to use name property for eholdings As per previous commit for agreements
Created attachment 146968 [details] [review] Bug 32932: Update ermmain to use router name property Updating the router links in the homepage as per previous commits Signed-off-by: Jacob O'Mara <jacob.omara@ptfs-europe.com>
Will need rebasing once bug 32983 and its dependency tree are pushed to master as routes.js has moved/been renamed. I'll apply the dependency tree and rebase, then re-submit once master is up to date with current ERM refactoring work
Created attachment 147119 [details] [review] Bug_32932: Update router to use object references - agreements This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings
Created attachment 147120 [details] [review] Bug 32932: Update router to use name property for licences As per previous commit for agreements
Created attachment 147121 [details] [review] Bug 32932: Update router to use name property for eholdings As per previous commit for agreements
Created attachment 147122 [details] [review] Bug 32932: Update ermmain to use router name property Updating the router links in the homepage as per previous commits
REBASED: Now depends on 32983 to reflect current refactoring work. Test plan as before
With the new dialog implementation there are no more '*Delete.vue' files.
Created attachment 148115 [details] [review] Bug 32932: (rebase) Rebased on top of bug 32991 This patch rebases on top of changes made in bug 32991 and also fixes an incorrect router reference in EHoldingsLocalTitlesFormAdd
Patch still doesn't apply: 14:43 $ git bz apply 32932 Bug 32932 - Re-structure Vue router-links to use "name" instead of urls 147119 - Bug_32932: Update router to use object references - agreements 147120 - Bug 32932: Update router to use name property for licences 147121 - Bug 32932: Update router to use name property for eholdings 147122 - Bug 32932: Update ermmain to use router name property 148115 - Bug 32932: (rebase) Rebased on top of bug 32991 Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug_32932: Update router to use object references - agreements error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/js/vue/components/ERM/AgreementsFormAdd.vue). error: could not build fake ancestor Patch failed at 0001 Bug_32932: Update router to use object references - agreements hint: Use 'git am --show-current-patch' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug32932-Update-router-to-use-object-references----8WDpli.patch
I'll try to reapply your patches and make them work
Created attachment 148551 [details] [review] Bug 32932: Update router to use object references - agreements This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings
Created attachment 148552 [details] [review] Bug 32932: Update router to use name property for licences As per previous commit for agreements
Created attachment 148553 [details] [review] Bug 32932: Update router to use name property for eholdings As per previous commit for agreements
Created attachment 148554 [details] [review] Bug 32932: Update ermmain to use router name property Updating the router links in the homepage as per previous commits
Created attachment 148558 [details] [review] Bug 32932: Update router to use object references - agreements This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Created attachment 148559 [details] [review] Bug 32932: Update router to use name property for licences As per previous commit for agreements Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Created attachment 148560 [details] [review] Bug 32932: Update router to use name property for eholdings As per previous commit for agreements Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Created attachment 148561 [details] [review] Bug 32932: Update ermmain to use router name property Updating the router links in the homepage as per previous commits Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
- this.$router.push( - "/cgi-bin/koha/erm/eholdings/local/titles" - ) + this.$router.push({ + name: "EHoldingsLocalPackagesList", + }) You are replacing titles with packages here.
I am getting a blank page on /cgi-bin/koha/erm/eholdings/local/titles/2 Uncaught Error: No match for {"name":"EHoldingslocalPackagesList","params":{}} "local" vs "Local"
Created attachment 149210 [details] [review] Bug 32932: (follow-up) Fix copy/paste bug in local titles form add and fix bad link definition in Main.vue
Created attachment 149707 [details] [review] Bug 32932: (follow-up) Fix copy/paste bug in local titles form add and fix bad link definition in Main.vue
Created attachment 149709 [details] [review] Bug 32932: Update router to use object references - agreements This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
squashed commits for rebase to apply
Needs another rebase
Created attachment 149934 [details] [review] Bug 32932: Update router to use object references - agreements This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Created attachment 149935 [details] [review] Bug 32932: Update router to use object references - agreements This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Functionality all tests out good, QA scripts are happy :) Passing QA
Created attachment 151529 [details] [review] Bug 32932: Update router to use object references - agreements This patch updates the Vue files for ERM to use router object links rather than urls - this will make it much simpler to maintain the router if urls need to be updated. This first patch only updates the files in the agreements section of ERM as a first step to introduce the topic for discussion. Test plan: 1) Apply patch 2) Navigate to the Agreements section of ERM and click on different navigation links/buttons 3) The links/buttons should all work as normal 4) Review the code to see that the component files now refer back to the router using objects rather than url strings Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Pushed to master for 23.11. Nice work everyone, thanks!
I think we need a test to prevent regressions.
The current test suite does a lot of navigation using .click() events - this should cover the router functionality for those routes/links. Do we just need a test that tests the routes that aren't currently included in existing cypress tests or are you thinking of a different testing method?
I was more thinking: how do we make sure future changes won't introduce non-named URLs in ERM or even other vue files?
(In reply to Jonathan Druart from comment #44) > I was more thinking: how do we make sure future changes won't introduce > non-named URLs in ERM or even other vue files? Might be worth seeing if there's a way of using ESLint to check?
ESLint could work if we create a custom rule, or we'd need to add something into the pre-commit hook to check Vue files for urls
This patch makes Titles_spec.ts fail: "ReferenceError: resource_id is not defined" in "Show title". Can you have a look please?
Created attachment 152231 [details] [review] Bug 32932: Fix resource_id param for EHoldingsLocalResourcesShow Caught by cypress tests "ReferenceError: resource_id is not defined" in "Show title".
(In reply to Jonathan Druart from comment #47) > This patch makes Titles_spec.ts fail: > "ReferenceError: resource_id is not defined" in "Show title". > > Can you have a look please? Don't worry, I fixed it!
Nice work everyone! Pushed to oldstable for 22.11.x
The follow-up has been pushed to master, 23.05 and 22.11. Removing the 'additional_work_needed' flag, and closing.