Bug 32806 - Some Vue files need to be moved for better reusability
Summary: Some Vue files need to be moved for better reusability
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 32030 32609 32923
Blocks: 32939
  Show dependency treegraph
 
Reported: 2023-02-02 19:56 UTC by Jonathan Druart
Modified: 2023-12-28 20:43 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This is an important architectural improvement to aid in future maintenance and expansion of the Vue based modules (erm) not available in Koha.
Version(s) released in:
23.05.00,22.11.04


Attachments
Bug 32806: Move Vue files for reusability (23.67 KB, patch)
2023-02-02 19:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: Rename main.js to erm.js (1.23 KB, patch)
2023-02-02 20:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: Rename ERMHome and ERMMain to Home and Main (2.71 KB, patch)
2023-02-02 20:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: Move routes.js to routes/erm.js (5.50 KB, patch)
2023-02-02 20:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: (follow-up) Rename main.js to erm.js (2.30 KB, patch)
2023-02-07 15:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: Move main-erm.ts to modules/erm.ts (2.05 KB, patch)
2023-02-07 19:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: (follow-up) Rename main.js to erm.js (2.37 KB, patch)
2023-02-10 09:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: Move main-erm.ts to modules/erm.ts (2.05 KB, patch)
2023-02-10 09:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: Rename main.js to erm.js (1.25 KB, patch)
2023-02-14 14:04 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Rename ERMHome and ERMMain to Home and Main (2.72 KB, patch)
2023-02-14 14:04 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move routes.js to routes/erm.js (5.51 KB, patch)
2023-02-14 14:04 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: (follow-up) Rename main.js to erm.js (2.39 KB, patch)
2023-02-14 14:04 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move main-erm.ts to modules/erm.ts (2.14 KB, patch)
2023-02-14 14:04 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move Vue files for reusability (23.63 KB, patch)
2023-02-14 14:07 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Rename main.js to erm.js (1.25 KB, patch)
2023-02-14 14:07 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Rename ERMHome and ERMMain to Home and Main (2.72 KB, patch)
2023-02-14 14:07 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move routes.js to routes/erm.js (5.51 KB, patch)
2023-02-14 14:07 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: (follow-up) Rename main.js to erm.js (2.39 KB, patch)
2023-02-14 14:07 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move main-erm.ts to modules/erm.ts (2.14 KB, patch)
2023-02-14 14:07 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move Vue files for reusability (23.63 KB, patch)
2023-02-14 14:27 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Rename main.js to erm.js (1.25 KB, patch)
2023-02-14 14:27 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Rename ERMHome and ERMMain to Home and Main (2.72 KB, patch)
2023-02-14 14:27 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move routes.js to routes/erm.js (5.51 KB, patch)
2023-02-14 14:27 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: (follow-up) Rename main.js to erm.js (2.39 KB, patch)
2023-02-14 14:27 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move main-erm.ts to modules/erm.ts (2.14 KB, patch)
2023-02-14 14:27 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move Vue files for reusability (23.69 KB, patch)
2023-02-14 21:49 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Rename main.js to erm.js (1.30 KB, patch)
2023-02-14 21:49 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Rename ERMHome and ERMMain to Home and Main (2.78 KB, patch)
2023-02-14 21:49 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move routes.js to routes/erm.js (5.57 KB, patch)
2023-02-14 21:49 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: (follow-up) Rename main.js to erm.js (2.45 KB, patch)
2023-02-14 21:49 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move main-erm.ts to modules/erm.ts (2.14 KB, patch)
2023-02-14 21:49 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 32806: Move Vue files for reusability (23.66 KB, patch)
2023-02-15 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: Rename main.js to erm.js (1.29 KB, patch)
2023-02-15 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: Rename ERMHome and ERMMain to Home and Main (2.77 KB, patch)
2023-02-15 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: Move routes.js to routes/erm.js (5.56 KB, patch)
2023-02-15 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: (follow-up) Rename main.js to erm.js (2.43 KB, patch)
2023-02-15 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: Move main-erm.ts to modules/erm.ts (2.12 KB, patch)
2023-02-15 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32806: Move Vue files for reusability (23.65 KB, patch)
2023-02-23 14:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32806: Rename main.js to erm.js (1.34 KB, patch)
2023-02-23 14:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32806: Rename ERMHome and ERMMain to Home and Main (2.82 KB, patch)
2023-02-23 14:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32806: Move routes.js to routes/erm.js (5.61 KB, patch)
2023-02-23 14:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32806: (follow-up) Rename main.js to erm.js (2.49 KB, patch)
2023-02-23 14:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32806: Move main-erm.ts to modules/erm.ts (2.18 KB, patch)
2023-02-23 14:51 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 32806: Move Vue files for reusability (23.71 KB, patch)
2023-02-24 12:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32806: Rename main.js to erm.js (1.40 KB, patch)
2023-02-24 12:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32806: Rename ERMHome and ERMMain to Home and Main (2.88 KB, patch)
2023-02-24 12:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32806: Move routes.js to routes/erm.js (5.67 KB, patch)
2023-02-24 12:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32806: (follow-up) Rename main.js to erm.js (2.54 KB, patch)
2023-02-24 12:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 32806: Move main-erm.ts to modules/erm.ts (2.23 KB, patch)
2023-02-24 12:02 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-02-02 19:56:13 UTC
So far the Vue files are only used by the ERM module, but some needs to be move in a parent directory to make them reusable by another module.
Comment 1 Jonathan Druart 2023-02-02 19:57:43 UTC
Created attachment 146030 [details] [review]
Bug 32806: Move Vue files for reusability
Comment 2 Jonathan Druart 2023-02-02 20:13:21 UTC
Created attachment 146033 [details] [review]
Bug 32806: Rename main.js to erm.js
Comment 3 Jonathan Druart 2023-02-02 20:17:35 UTC
Created attachment 146035 [details] [review]
Bug 32806: Rename ERMHome and ERMMain to Home and Main

We don't need to repeat "ERM"
Comment 4 Jonathan Druart 2023-02-02 20:23:33 UTC
Created attachment 146036 [details] [review]
Bug 32806: Move routes.js to routes/erm.js
Comment 5 Agustín Moyano 2023-02-06 21:43:53 UTC
Hi Jonathan, to reuse stuff I like your idea to create folders by type (routes, stores, components, ..). 

Maybe we can add another folder named "modules" where we place all "main.ts" files, renaming them as "erm.ts" or "import_sources.ts". Or, instead of creating a "modules" folder, we could have a single "main.ts" and make all modules in routes to lazy load ( https://router.vuejs.org/guide/advanced/lazy-loading.html )

About the components folder, in my implementation, I added a "shared" subfolder for all shared components, but I don't mind keeping them in the components folder directly.

I loved the idea of generating the breadcrumbs from the router specification, but the definition of it was kind of confusing, and hard to follow, so I took your idea and changed it a little bit.

I added a couple more shared components, one of them being the implementation of Datatables in Vue (called KohaTables). I see that you have here the "composable" folder where you call functions of Datatables, but I saw that the creators of Datatables had an implementation for Vue, so I thought it best to use that.

Thanks!
Comment 6 Jonathan Druart 2023-02-07 08:49:26 UTC
(In reply to Agustín Moyano from comment #5)
> Hi Jonathan, to reuse stuff I like your idea to create folders by type
> (routes, stores, components, ..). 
> 
> Maybe we can add another folder named "modules" where we place all "main.ts"
> files, renaming them as "erm.ts" or "import_sources.ts". Or, instead of
> creating a "modules" folder, we could have a single "main.ts" and make all
> modules in routes to lazy load (
> https://router.vuejs.org/guide/advanced/lazy-loading.html )

I think I quite like the idea to have them visible at the root instead of hiding in the sub directory, but I am not strongly attached to it.

> About the components folder, in my implementation, I added a "shared"
> subfolder for all shared components, but I don't mind keeping them in the
> components folder directly.

Same as above, it was to make them more visible, but that one could consider it messy.

> I loved the idea of generating the breadcrumbs from the router
> specification, but the definition of it was kind of confusing, and hard to
> follow, so I took your idea and changed it a little bit.

The breadcrumb for the ERM module is quite complex, hence the complexity of the file :)
About your implementation, there is something weird, you don't define the routes, and "Add import source" leads to admin/import-sources.pl/add. Is that expected?
On erm I've decided to rewrite the url and have something following the REST routes (/cgi-bin/koha/erm/eholdings/local/packages/add for instance)

> I added a couple more shared components, one of them being the
> implementation of Datatables in Vue (called KohaTables). I see that you have
> here the "composable" folder where you call functions of Datatables, but I
> saw that the creators of Datatables had an implementation for Vue, so I
> thought it best to use that.

Yes but then you will need to reimplement all what we have in datatables.js, and I don't think that's something worthwhile (at least I would not consider it a priority for now). In your implementation there is problem with the pagination (NaN), the order by does not work, there is no column filtering, etc.

I think we need to be consistent between the modules that are going to use Vue. We should apply the changes to the existing modules, then use them for the new ones.
Comment 7 Jonathan Druart 2023-02-07 08:49:49 UTC
Putting in discussion, but we need to move forward quickly here.
Comment 8 Pedro Amorim 2023-02-07 11:31:44 UTC
This looks really good.
Had a look at Agustin's implementation, also looks really good. For discussion sake, here:
https://gitlab.com/thekesolutions/Koha/-/tree/import_source_vue/koha-tmpl/intranet-tmpl/prog/js/vue

I agree with everything and don't have much to add really, some thoughts at a glance:
- Personally, I like Agustin's "modules" folder idea.
- Not a huge fan of "fetch" folder, mostly the name, but don't have a great alternative for it either.
- What about t/cypress? Should we prefix all module related tests with modulename and leave all tests inside integration folder where they currently live?
Comment 9 Jonathan Druart 2023-02-07 15:55:55 UTC
Created attachment 146337 [details] [review]
Bug 32806: (follow-up) Rename main.js to erm.js
Comment 10 Jonathan Druart 2023-02-07 19:55:37 UTC
Created attachment 146357 [details] [review]
Bug 32806: Move main-erm.ts to modules/erm.ts
Comment 11 Agustín Moyano 2023-02-08 18:33:48 UTC
(In reply to Jonathan Druart from comment #6)

> > About the components folder, in my implementation, I added a "shared"
> > subfolder for all shared components, but I don't mind keeping them in the
> > components folder directly.
> 
> Same as above, it was to make them more visible, but that one could consider
> it messy.

I believe we can keep them at components folder


> 
> > I loved the idea of generating the breadcrumbs from the router
> > specification, but the definition of it was kind of confusing, and hard to
> > follow, so I took your idea and changed it a little bit.
> 
> The breadcrumb for the ERM module is quite complex, hence the complexity of
> the file :)
> About your implementation, there is something weird, you don't define the
> routes, and "Add import source" leads to admin/import-sources.pl/add. Is
> that expected?
> On erm I've decided to rewrite the url and have something following the REST
> routes (/cgi-bin/koha/erm/eholdings/local/packages/add for instance)
> 

I defined my routes in import-source/tree.js

That definition gets imported to stores/menu.js, and when that happens the tree gets analyzed, and some attributes are added.

from there, the menu store has actions that allow you to get the routes, left-hand submenu and breadcrums

About the url, if I don't make admin/import-sources.pl the root of my routes, when users enter, they would see an empty page... I really don't want users to click on something to see the table.. the other routes depend on this one, so the /add and /:id gets appended

> > I added a couple more shared components, one of them being the
> > implementation of Datatables in Vue (called KohaTables). I see that you have
> > here the "composable" folder where you call functions of Datatables, but I
> > saw that the creators of Datatables had an implementation for Vue, so I
> > thought it best to use that.
> 
> Yes but then you will need to reimplement all what we have in datatables.js,
> and I don't think that's something worthwhile (at least I would not consider
> it a priority for now). In your implementation there is problem with the
> pagination (NaN), the order by does not work, there is no column filtering,
> etc.
> 

Ok, but I still believe all logic of Datatables should be encapsulated by a single component... meaning that what it is in composables/datatables.js should be logic that belongs to a table component (Datatable, Kohatable, or whatever you believe best to call it). In that case, if somewhere in the future we want to replace Datatables with something else, there is a single place to touch and is more consistent with Vue's "spirit" of things

> I think we need to be consistent between the modules that are going to use
> Vue. We should apply the changes to the existing modules, then use them for
> the new ones.

Agreed, I made my bug depend on this one, just to be in sync

Thanks
Comment 12 Jonathan Druart 2023-02-08 20:01:47 UTC
(In reply to Agustín Moyano from comment #11)
> (In reply to Jonathan Druart from comment #6)
> > The breadcrumb for the ERM module is quite complex, hence the complexity of
> > the file :)
> > About your implementation, there is something weird, you don't define the
> > routes, and "Add import source" leads to admin/import-sources.pl/add. Is
> > that expected?
> > On erm I've decided to rewrite the url and have something following the REST
> > routes (/cgi-bin/koha/erm/eholdings/local/packages/add for instance)
> 
> I defined my routes in import-source/tree.js
> 
> That definition gets imported to stores/menu.js, and when that happens the
> tree gets analyzed, and some attributes are added.
> 
> from there, the menu store has actions that allow you to get the routes,
> left-hand submenu and breadcrums
> 
> About the url, if I don't make admin/import-sources.pl the root of my
> routes, when users enter, they would see an empty page... I really don't
> want users to click on something to see the table.. the other routes depend
> on this one, so the /add and /:id gets appended

You need the RewriteRule in apache's config.
Another thing is the translations, did you try if that works with your implementation?

I don't mind replacing the breadcrumbs system we have for ERM, but to be honest I spent a lot of time to reach this point, and my I am not sure what you have now is ready to replace it. Feel free to provide an implementation for the ERM module to see if there is feature parity.

https://snipboard.io/9f6qPL.jpg
Comment 13 Jonathan Druart 2023-02-10 09:56:23 UTC
Created attachment 146465 [details] [review]
Bug 32806: (follow-up) Rename main.js to erm.js
Comment 14 Jonathan Druart 2023-02-10 09:56:26 UTC
Created attachment 146466 [details] [review]
Bug 32806: Move main-erm.ts to modules/erm.ts
Comment 15 Jonathan Druart 2023-02-14 09:24:06 UTC
Can we move forward here?
Comment 16 Paul Derscheid 2023-02-14 14:04:08 UTC
Created attachment 146619 [details] [review]
Bug 32806: Rename main.js to erm.js
Comment 17 Paul Derscheid 2023-02-14 14:04:11 UTC
Created attachment 146620 [details] [review]
Bug 32806: Rename ERMHome and ERMMain to Home and Main

We don't need to repeat "ERM"
Comment 18 Paul Derscheid 2023-02-14 14:04:14 UTC
Created attachment 146621 [details] [review]
Bug 32806: Move routes.js to routes/erm.js
Comment 19 Paul Derscheid 2023-02-14 14:04:17 UTC
Created attachment 146622 [details] [review]
Bug 32806: (follow-up) Rename main.js to erm.js
Comment 20 Paul Derscheid 2023-02-14 14:04:20 UTC
Created attachment 146623 [details] [review]
Bug 32806: Move main-erm.ts to modules/erm.ts

Looks fine.

Signed-off-by: LMSCloudPaulD <paul.derscheid@lmscloud.de>
Comment 21 Paul Derscheid 2023-02-14 14:07:40 UTC
Created attachment 146626 [details] [review]
Bug 32806: Move Vue files for reusability

Forgot one..
Comment 22 Paul Derscheid 2023-02-14 14:07:43 UTC
Created attachment 146627 [details] [review]
Bug 32806: Rename main.js to erm.js
Comment 23 Paul Derscheid 2023-02-14 14:07:46 UTC
Created attachment 146628 [details] [review]
Bug 32806: Rename ERMHome and ERMMain to Home and Main

We don't need to repeat "ERM"
Comment 24 Paul Derscheid 2023-02-14 14:07:49 UTC
Created attachment 146629 [details] [review]
Bug 32806: Move routes.js to routes/erm.js
Comment 25 Paul Derscheid 2023-02-14 14:07:52 UTC
Created attachment 146630 [details] [review]
Bug 32806: (follow-up) Rename main.js to erm.js
Comment 26 Paul Derscheid 2023-02-14 14:07:55 UTC
Created attachment 146631 [details] [review]
Bug 32806: Move main-erm.ts to modules/erm.ts

Looks fine.

Signed-off-by: LMSCloudPaulD <paul.derscheid@lmscloud.de>
Comment 27 Paul Derscheid 2023-02-14 14:27:44 UTC
Created attachment 146634 [details] [review]
Bug 32806: Move Vue files for reusability
Comment 28 Paul Derscheid 2023-02-14 14:27:47 UTC
Created attachment 146635 [details] [review]
Bug 32806: Rename main.js to erm.js
Comment 29 Paul Derscheid 2023-02-14 14:27:49 UTC
Created attachment 146636 [details] [review]
Bug 32806: Rename ERMHome and ERMMain to Home and Main

We don't need to repeat "ERM"
Comment 30 Paul Derscheid 2023-02-14 14:27:52 UTC
Created attachment 146637 [details] [review]
Bug 32806: Move routes.js to routes/erm.js
Comment 31 Paul Derscheid 2023-02-14 14:27:55 UTC
Created attachment 146638 [details] [review]
Bug 32806: (follow-up) Rename main.js to erm.js
Comment 32 Paul Derscheid 2023-02-14 14:27:58 UTC
Created attachment 146639 [details] [review]
Bug 32806: Move main-erm.ts to modules/erm.ts

Looks fine.

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 33 David Nind 2023-02-14 19:27:41 UTC
Hi Paul.

I think you may need to change the bug status to Signed Off - it looks like you have signed it off to me.

Also, normally the signed off line is in all the patches.

David
Comment 34 Paul Derscheid 2023-02-14 21:49:17 UTC
Created attachment 146647 [details] [review]
Bug 32806: Move Vue files for reusability

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 35 Paul Derscheid 2023-02-14 21:49:21 UTC
Created attachment 146648 [details] [review]
Bug 32806: Rename main.js to erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 36 Paul Derscheid 2023-02-14 21:49:24 UTC
Created attachment 146649 [details] [review]
Bug 32806: Rename ERMHome and ERMMain to Home and Main

We don't need to repeat "ERM"

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 37 Paul Derscheid 2023-02-14 21:49:28 UTC
Created attachment 146650 [details] [review]
Bug 32806: Move routes.js to routes/erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 38 Paul Derscheid 2023-02-14 21:49:31 UTC
Created attachment 146651 [details] [review]
Bug 32806: (follow-up) Rename main.js to erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 39 Paul Derscheid 2023-02-14 21:49:34 UTC
Created attachment 146652 [details] [review]
Bug 32806: Move main-erm.ts to modules/erm.ts

Looks fine.

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 40 Paul Derscheid 2023-02-14 21:51:11 UTC
I hope that I fixed it now. Thanks David!
Comment 41 Jonathan Druart 2023-02-15 08:51:00 UTC
Created attachment 146661 [details] [review]
Bug 32806: Move Vue files for reusability

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 42 Jonathan Druart 2023-02-15 08:51:03 UTC
Created attachment 146662 [details] [review]
Bug 32806: Rename main.js to erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 43 Jonathan Druart 2023-02-15 08:51:06 UTC
Created attachment 146663 [details] [review]
Bug 32806: Rename ERMHome and ERMMain to Home and Main

We don't need to repeat "ERM"

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 44 Jonathan Druart 2023-02-15 08:51:09 UTC
Created attachment 146664 [details] [review]
Bug 32806: Move routes.js to routes/erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 45 Jonathan Druart 2023-02-15 08:51:13 UTC
Created attachment 146665 [details] [review]
Bug 32806: (follow-up) Rename main.js to erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 46 Jonathan Druart 2023-02-15 08:51:16 UTC
Created attachment 146666 [details] [review]
Bug 32806: Move main-erm.ts to modules/erm.ts

Looks fine.

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 47 Jonathan Druart 2023-02-15 08:51:44 UTC
Patches rebased on top of bug 32898 and bug 32923.
Comment 48 Pedro Amorim 2023-02-23 14:51:41 UTC
Created attachment 147241 [details] [review]
Bug 32806: Move Vue files for reusability

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 49 Pedro Amorim 2023-02-23 14:51:44 UTC
Created attachment 147242 [details] [review]
Bug 32806: Rename main.js to erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 50 Pedro Amorim 2023-02-23 14:51:48 UTC
Created attachment 147243 [details] [review]
Bug 32806: Rename ERMHome and ERMMain to Home and Main

We don't need to repeat "ERM"

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 51 Pedro Amorim 2023-02-23 14:51:51 UTC
Created attachment 147244 [details] [review]
Bug 32806: Move routes.js to routes/erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 52 Pedro Amorim 2023-02-23 14:51:54 UTC
Created attachment 147245 [details] [review]
Bug 32806: (follow-up) Rename main.js to erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 53 Pedro Amorim 2023-02-23 14:51:57 UTC
Created attachment 147246 [details] [review]
Bug 32806: Move main-erm.ts to modules/erm.ts

Looks fine.

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 54 David Cook 2023-02-23 22:38:55 UTC
(In reply to Agustín Moyano from comment #11)
> Ok, but I still believe all logic of Datatables should be encapsulated by a
> single component... meaning that what it is in composables/datatables.js
> should be logic that belongs to a table component (Datatable, Kohatable, or
> whatever you believe best to call it). In that case, if somewhere in the
> future we want to replace Datatables with something else, there is a single
> place to touch and is more consistent with Vue's "spirit" of things
 
Agreed. It seems like we've got quite a bit of duplicated code in the different components. I'm not sure that we need so many? I wonder if some things are components when they could be regular Javascript objects or JSON objects. Would need to take a deeper look to know for sure...
Comment 55 Nick Clemens 2023-02-24 12:02:40 UTC
Created attachment 147299 [details] [review]
Bug 32806: Move Vue files for reusability

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 Nick Clemens 2023-02-24 12:02:42 UTC
Created attachment 147300 [details] [review]
Bug 32806: Rename main.js to erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 57 Nick Clemens 2023-02-24 12:02:45 UTC
Created attachment 147301 [details] [review]
Bug 32806: Rename ERMHome and ERMMain to Home and Main

We don't need to repeat "ERM"

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 58 Nick Clemens 2023-02-24 12:02:48 UTC
Created attachment 147302 [details] [review]
Bug 32806: Move routes.js to routes/erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 59 Nick Clemens 2023-02-24 12:02:51 UTC
Created attachment 147303 [details] [review]
Bug 32806: (follow-up) Rename main.js to erm.js

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 60 Nick Clemens 2023-02-24 12:02:53 UTC
Created attachment 147304 [details] [review]
Bug 32806: Move main-erm.ts to modules/erm.ts

Looks fine.

Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 61 Tomás Cohen Arazi 2023-02-27 14:22:38 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 62 Matt Blenkinsop 2023-02-28 18:55:29 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 63 Lucas Gass 2023-03-08 14:08:50 UTC
Not needed in 22.05.x
Comment 64 Andrii Nugged 2023-03-08 21:19:15 UTC
Is this lost in this ticket or did I miss a dependency?
Because I don't see any physical renaming for

main.js -> erm.js

and it fails to create a transitional piloting build for our production piloting library for 22.11.x (but I assume master branch too?):

  make[1]: *** No rule to make target 'koha-tmpl/intranet- 
  tmpl/prog/js/vue/dist/erm.js', needed by 'pm_to_blib'.  Stop.

so this error means that the target is not present, and I noticed that the .../js/dist/ folder still contains main.*, but not erm.*.

I did renaming additionally (this patch below), and the build was successful. 


---
 koha-tmpl/intranet-tmpl/prog/js/vue/dist/{main.js => erm.js}      | 0
 .../prog/js/vue/dist/{main.js.LICENSE.txt => erm.js.LICENSE.txt}  | 0
 2 files changed, 0 insertions(+), 0 deletions(-)
 rename koha-tmpl/intranet-tmpl/prog/js/vue/dist/{main.js => erm.js} (100%)
 rename koha-tmpl/intranet-tmpl/prog/js/vue/dist/{main.js.LICENSE.txt => erm.js.LICENSE.txt} (100%)

diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/dist/main.js b/koha-tmpl/intranet-tmpl/prog/js/vue/dist/erm.js
similarity index 100%
rename from koha-tmpl/intranet-tmpl/prog/js/vue/dist/main.js
rename to koha-tmpl/intranet-tmpl/prog/js/vue/dist/erm.js
diff --git a/koha-tmpl/intranet-tmpl/prog/js/vue/dist/main.js.LICENSE.txt b/koha-tmpl/intranet-tmpl/prog/js/vue/dist/erm.js.LICENSE.txt
similarity index 100%
rename from koha-tmpl/intranet-tmpl/prog/js/vue/dist/main.js.LICENSE.txt
rename to koha-tmpl/intranet-tmpl/prog/js/vue/dist/erm.js.LICENSE.txt
--
Comment 65 Martin Renvoize 2023-03-10 12:13:18 UTC
This depends on bug 32609 which we haven't backported.

Now.. I'm considering we perhaps should backport that to 22.11 for maintainability.

I'll have a chat with Matt about this.. it's easy to miss these things and it falls on my bad tuition that he missed this.