Bug 38201 - VueJS architecture rethink
Summary: VueJS architecture rethink
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Bugs List
QA Contact: Testopia
URL: https://github.com/openfifth/koha/tre...
Keywords:
Depends on:
Blocks: 37893 39224 39320
  Show dependency treegraph
 
Reported: 2024-10-18 08:21 UTC by Pedro Amorim
Modified: 2025-04-01 08:43 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2024-10-18 08:21:50 UTC
The goal is to streamline and provide a standard to how a regular CRUD component is implemented, instead of having to implement everything every time a new CRUD is required.
Comment 1 Pedro Amorim 2024-10-18 08:26:04 UTC
Have a WIP on a branch at the moment, for discussion.
Comment 2 Pedro Amorim 2024-10-18 08:35:17 UTC
Bug 37301 is the first step. The idea here is to go one step furhter.
To have all CRUD forms be rendered dynamically from a list of resource attributes, instead of having to be implemented every time for every CRUD.
Tables to be rendered dynamically from the same list of resource attributes, instead of having to be implemented every time for every CRUD.
This would greatly ease future components development, and ensure further consistency and less bugs by having common overlapping code DRY.
Have one single way of rendering an AV dropdown on a form, have one single way of rendering a boolean input on a form, and so on.
Comment 3 Pedro Amorim 2024-10-18 09:13:11 UTC
Chatting with Matt, this should likely be based on bug 37930.
Comment 4 Jonathan Druart 2025-01-30 15:12:27 UTC
1. I think this is problematic for translatability:
+                    {{ $__("Remove this %s").format(resourceName) }}</a

+            {{ $__("Add new %s").format(resourceName) }}</a

+            $__("There are no %s created yet").format(resourceName)

2.
http://localhost:8081/cgi-bin/koha/preservation/trains/1
 a. Toolbar button "Add items" is not correctly styled: https://snipboard.io/W3Be1Y.jpg
 b. warning in the console:
[Vue warn]: Property "class" was accessed during render but is not defined on instance. 
  at <Toolbar key=0 to= 
Object { name: "TrainsFormAddItem", params: {…} }
 icon="plus"  ... > 

3. Create a train, then delete it from the list view
 a. The table is not refreshed and the train is still displayed
 b. warning in the console:
[Vue Router warn]: Discarded invalid param(s) "train_item_id" when navigating. See https://github.com/vuejs/router/blob/main/packages/router/CHANGELOG.md#414-2022-08-22 for more details.

4. AgreementResource.vue
+                                nameLC: __("user"),
+                                nameUC: __("Agreement user"),
+                                namePL: __("users"),


LC, UC, PL? Can it be verbose so we don't need to guess what it means?

5. Update users for an agreement:
https://snipboard.io/FywJhr.jpg
Change one field and all are updated, wrong binding here.

Oops, it actually happens for other linked resources.

See also the duplication of the error there: https://snipboard.io/jmvBbL.jpg

6.
(In reply to Pedro Amorim from comment #3)
> Chatting with Matt, this should likely be based on bug 37930.

It is currently not.
And bugs encountered on bug 37301 are fixed here. Can we have a clear tree?

7. Is there a TODO list? It seems that other components need adjustment to use this change (the remaining FormAdd, List, Show components). What's missing to make it ready for testing/QA and push?

8. With the additional abstractions everything is harder to read IMO, we should add documentation, at least for the "Base" components.

9. There is way too many changes for a single bug report, please split into smaller chunks next time. It's really hard to follow.

10. Cypress tests are failing:
       Spec                                              Tests  Passing  Failing
 │ ✖  AdditionalFields_spec.ts                 02:22       12        2       10        
 │ ✖  InfiniteScrollSelect_spec.ts             00:56        4        -        4        
 │ ✖  Admin/RecordSources_spec.ts              00:51        5        1        4        
 │ ✖  ERM/Agreements_spec.ts                   01:06        5        -        5        
 │ ✖  ERM/Licenses_spec.ts                     01:04        5        -        5
 │ ✖  ERM/UserRoles_spec.ts                    00:15        1        -        1        

11. I cannot save an agreement with a license
400 - Object { message: "Expected string - got null.", path: "/body/agreement_licenses/0/status" }

====

Overall I think the idea is good but it adds some "magic" that won't help new developer to grasp our VueJS code. I feel like you have been too far, and things won't be as easy to maintain as you think.

This is something weird:
 310                         {
 311                             name: "ended_on",
 312                             type: "component",
 313                             label: __("End date"),
 314                             componentPath: "./FlatPickrWrapper.vue",
 315                             required: false,
 316                             props: {
 317                                 id: {
 318                                     type: "string",
 319                                     value: "ended_on_",
 320                                     indexRequired: true,
 321                                 },
 322                             },
 323                         },

I expect more to see something like "type: date", and not a component path to the flatpickr component.

Same for this:
 122                 {
 123                     name: "vendor_id",
 124                     type: "component",
 125                     label: __("Vendor"),
 126                     showElement: {
 127                         type: "text",
 128                         value: "vendor.name",
 129                         link: {
 130                             href: "/cgi-bin/koha/acqui/supplier.pl",
 131                             params: {
 132                                 bookseller_id: "vendor_id",
 133                             },
 134                         },
 135                     },
 136                     componentPath: "./FormSelectVendors.vue",
 137                     props: {
 138                         id: {
 139                             type: "string",
 140                             value: "agreement_id_",
 141                             indexRequired: true,
 142                         },
 143                     },
 144                 },

This should be shorter, you don't want to duplicate this whenever a component will be linked to a vendor.
It could be "vendor_id is a vendor, and the value for this component is vendor.name, and is required". The other info will be copy/pasted, and increase the risk of regressions, inconsistencies, etc.



The 500 lines of definition of the agreement (resourceAttrs in AgreementResource.vue) are not trivial to understand, neither to know what does what.
I think we could make it a bit more readable by moving some redundant code that is in there: the relationshipWidget are defined in LicenseResource and AgreementResource for instance.

We should make sure the order of the attributes are always the same (how??), otherwise it get messy, for instance:
 339                             name: "notes",
 340                             required: false,
 341                             type: "text",
 342                             label: __("Notes"),

vs

 421                             name: "role",
 422                             type: "select",
 423                             label: __("Role"),
 424                             avCat: "av_user_roles",
 425                             required: true,



This pattern looks wrong:
 482                         resourceRelationships: {
 483                             type: "resourceProperty",
 484                             resourceProperty: "agreement_licenses",
 485                         },

But I don't have anything else to suggest now.


Sorry for the messy notes...
Comment 5 Pedro Amorim 2025-01-30 16:08:34 UTC
Thank you for your time here, Joubu. Your feedback is obviously very important and valuable.
We are going to fix the bugs you have raised and are still finishing this up, some things still require polish (some of this code was written as recently as today). Having said that, we're very grateful you've had the chance to review it right now.

(In reply to Jonathan Druart from comment #4)
> 
> 8. With the additional abstractions everything is harder to read IMO, we
> should add documentation, at least for the "Base" components.

Yes, we plan to have documentation here but we've not gotten to that point yet.
It will be great to have Koha developer documentation once we have this established pattern in place, to help guide new developers on how to setup their own VueJS module.

(In reply to Jonathan Druart from comment #4)
> 
> Overall I think the idea is good but it adds some "magic" that won't help
> new developer to grasp our VueJS code. I feel like you have been too far,
> and things won't be as easy to maintain as you think.

I'm really happy to read that you think the idea is good!
In terms of maintainability, I have to disagree. Particularly developing bug 37893 (which is a new full VueJS module), as a module implementer I have close to zero implementation work, as all the heavy lifting is already done for me and I have much less code that I need to write, thus much less chances of adding new buggy code.

We believe VueJS is the direction of travel in Koha development. Our goal is to ensure consistency on every existing and future VueJS module, something that does not happen in the rest of (old) Koha.

We want developers going forward to invest as little time as possible in:
- Implementing the 'create' routine of a 'Resource'
- Implementing the 'delete' routine of a 'Resource'
- Implementing the 'edit' routine of a 'Resource'
- Implementing the 'show' (read) routine of a 'Resource'
- Implementing a relationship form element with relational fields
- Implementing a relationship form dropdown element
- Implementing a 'date' form input element
- Implementing a toolbar for their module
- Implementing a table
- etc

All these things should only be implemented ONCE, and maintained in a single place. This work is not there yet but that's our goal. I agree that some of the 'magic' could be less obscure, but this is not final (nothing ever is) and we expect to iterate on this long after it's been pushed to core.
We want developers to invest time in the actual features their module should have. Not all the boilerplate.
By having all the modules share the same base components, we ensure far less bugs and establish a new coding paradigm that will encourage developers to implement new functionality at the 'Base' level whenever they need to do so, in a way that every other developer can also make use of it, instead of doing these things on their own specific module form, button, etc.

Thanks again, we'll come back when this is ready for review.
Comment 6 David Cook 2025-01-31 01:44:24 UTC
(In reply to Pedro Amorim from comment #5)
> All these things should only be implemented ONCE, and maintained in a single
> place. This work is not there yet but that's our goal. I agree that some of
> the 'magic' could be less obscure, but this is not final (nothing ever is)
> and we expect to iterate on this long after it's been pushed to core.
> We want developers to invest time in the actual features their module should
> have. Not all the boilerplate.
> By having all the modules share the same base components, we ensure far less
> bugs and establish a new coding paradigm that will encourage developers to
> implement new functionality at the 'Base' level whenever they need to do so,
> in a way that every other developer can also make use of it, instead of
> doing these things on their own specific module form, button, etc.

+100
Comment 7 Jonathan Druart 2025-02-03 11:54:27 UTC
(In reply to David Cook from comment #6)
> +100

+100 to DRY? Yes I think we all agree on that :D
Comment 8 Matt Blenkinsop 2025-02-14 13:45:37 UTC
(In reply to Jonathan Druart from comment #4)
> 1. I think this is problematic for translatability:
> +                    {{ $__("Remove this %s").format(resourceName) }}</a
> 
> +            {{ $__("Add new %s").format(resourceName) }}</a
> 
> +            $__("There are no %s created yet").format(resourceName)

We're now passing an i18n object with translated strings from the resource
 
> 2.
> http://localhost:8081/cgi-bin/koha/preservation/trains/1
>  a. Toolbar button "Add items" is not correctly styled:
> https://snipboard.io/W3Be1Y.jpg
>  b. warning in the console:
> [Vue warn]: Property "class" was accessed during render but is not defined
> on instance. 
>   at <Toolbar key=0 to= 
> Object { name: "TrainsFormAddItem", params: {…} }
>  icon="plus"  ... > 

This should be fixed now that the 'class' prop is gone from the toolbar

> 3. Create a train, then delete it from the list view
>  a. The table is not refreshed and the train is still displayed
>  b. warning in the console:
> [Vue Router warn]: Discarded invalid param(s) "train_item_id" when
> navigating. See
> https://github.com/vuejs/router/blob/main/packages/router/CHANGELOG.md#414-
> 2022-08-22 for more details.

This should also now be fixed, there was an issue with the reference to the datatable object


> 4. AgreementResource.vue
> +                                nameLC: __("user"),
> +                                nameUC: __("Agreement user"),
> +                                namePL: __("users"),
> 
> 
> LC, UC, PL? Can it be verbose so we don't need to guess what it means?

This is done

> 5. Update users for an agreement:
> https://snipboard.io/FywJhr.jpg
> Change one field and all are updated, wrong binding here.
> 
> Oops, it actually happens for other linked resources.
> 
> See also the duplication of the error there: https://snipboard.io/jmvBbL.jpg

This has been resolved, linked resources update independently
 
> 6.
> (In reply to Pedro Amorim from comment #3)
> > Chatting with Matt, this should likely be based on bug 37930.
> 
> It is currently not.
> And bugs encountered on bug 37301 are fixed here. Can we have a clear tree?

This isn't currently based on that work, 37930 is still in discussion so we could do with a clear direction on that bug before integrating it here, otherwise we may have to make a lot of changes depending on the outcome of 37930
 
> 7. Is there a TODO list? It seems that other components need adjustment to
> use this change (the remaining FormAdd, List, Show components). What's
> missing to make it ready for testing/QA and push?

Nothing official, there are a few nice to haves that we have noted for the future but all the critical stuff is included now (as far as we've spotted, see if you have any other ideas). We still need to migrate Preservation to this work. There are also some components that we would like to remove in the future like Agreement*Display.vue and also transferring FormSelectVendors to FormRelationshipSelect but these are possibly out of scope for now and could be follow-ups. Usage Statistics is probably also out of scope for this as it has some very specific functionality, same with the EBSCO components
 
> 8. With the additional abstractions everything is harder to read IMO, we
> should add documentation, at least for the "Base" components.

We've got JSDocs on the methods, we were considering adding a 'SkeletonResource.vue' component as an example like we have for atomic updates
 
> 9. There is way too many changes for a single bug report, please split into
> smaller chunks next time. It's really hard to follow.
> 
> 10. Cypress tests are failing:
>        Spec                                              Tests  Passing 
> Failing
>  │ ✖  AdditionalFields_spec.ts                 02:22       12        2      
> 10        
>  │ ✖  InfiniteScrollSelect_spec.ts             00:56        4        -      
> 4        
>  │ ✖  Admin/RecordSources_spec.ts              00:51        5        1      
> 4        
>  │ ✖  ERM/Agreements_spec.ts                   01:06        5        -      
> 5        
>  │ ✖  ERM/Licenses_spec.ts                     01:04        5        -      
> 5
>  │ ✖  ERM/UserRoles_spec.ts                    00:15        1        -      
> 1        

We haven't updated the tests yet while its still WIP, I was considering writing new tests to check all the different abstracted features 


> 11. I cannot save an agreement with a license
> 400 - Object { message: "Expected string - got null.", path:
> "/body/agreement_licenses/0/status" }

I believe this is also fixed 
 
> Overall I think the idea is good but it adds some "magic" that won't help
> new developer to grasp our VueJS code. I feel like you have been too far,
> and things won't be as easy to maintain as you think.
> 
> This is something weird:
>  310                         {
>  311                             name: "ended_on",
>  312                             type: "component",
>  313                             label: __("End date"),
>  314                             componentPath: "./FlatPickrWrapper.vue",
>  315                             required: false,
>  316                             props: {
>  317                                 id: {
>  318                                     type: "string",
>  319                                     value: "ended_on_",
>  320                                     indexRequired: true,
>  321                                 },
>  322                             },
>  323                         },
> 
> I expect more to see something like "type: date", and not a component path
> to the flatpickr component.

This is done, same for vendors

> 
> The 500 lines of definition of the agreement (resourceAttrs in
> AgreementResource.vue) are not trivial to understand, neither to know what
> does what.
> I think we could make it a bit more readable by moving some redundant code
> that is in there: the relationshipWidget are defined in LicenseResource and
> AgreementResource for instance.
> 
> We should make sure the order of the attributes are always the same (how??),
> otherwise it get messy, for instance:
>  339                             name: "notes",
>  340                             required: false,
>  341                             type: "text",
>  342                             label: __("Notes"),
> 
> vs
> 
>  421                             name: "role",
>  422                             type: "select",
>  423                             label: __("Role"),
>  424                             avCat: "av_user_roles",
>  425                             required: true,
> 

This would probably need a QA script check?
> 
> This pattern looks wrong:
>  482                         resourceRelationships: {
>  483                             type: "resourceProperty",
>  484                             resourceProperty: "agreement_licenses",
>  485                         },

This has also been fixed, its is now just one line with a key of 'resourceProperty'