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.
Have a WIP on a branch at the moment, for discussion.
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.
Chatting with Matt, this should likely be based on bug 37930.
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...
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.
(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
(In reply to David Cook from comment #6) > +100 +100 to DRY? Yes I think we all agree on that :D
(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'