Created attachment 168709 [details] [review] Bug 37301: [PoC] DRY ToolbarButton This is only a PoC applied to Agreements for discussion. This is a continuation of bug 37274. Bug 37274 applied the ToolbarButton to missing components but it was mostly replacing WET for WET. This is DRYer: - All mark-up related to toolbar buttons is now written once (css class, icons, elements) - All labels are now written once (except 'Add' for translatability)
Created attachment 168739 [details] [review] Bug 37301: Remove methods We do not need the methods, we can emit from the template. Also rename the events to the wording more consistent with what we have.
Created attachment 168740 [details] [review] Bug 37301: Rename 'type' with 'action' 'type' is too generic. Also move to else-if and add a else block to ease debugging in case an unsupported action is passed
Created attachment 168741 [details] [review] Bug 37301: Restore to For discussion. I quite liked it, we didn't need an extra method. The problem I can see is that edit and add behave differently that delete. Is that a problem? If we drop this patch we need to decide a better wording: editAgreement is IMO the action of editing an agreement. Here I think we need maybe goToEditResourceForm and goToAddResourceform for the event names.
Created attachment 168800 [details] [review] Bug 37301: PoC (Continuation): Add BaseResource and AgreementResource This is the building blocks for a much consistent approach, for example: AgreementsShow -> delete_agreement && AgreementsList -> doDelete should be DRY into AgreementResource Any WET code currently in setup for list+show of a resource can (and should) be moved into <resourceName>Resource Any WET code currently in setup for all 'resources' can (and should) be moved into BaseResource and others
(In reply to Jonathan Druart from comment #4) > Created attachment 168741 [details] [review] [review] > Bug 37301: Restore to > If we drop this patch we need to decide a better wording: editAgreement > is IMO the action of editing an agreement. Here I think we need maybe > goToEditResourceForm and goToAddResourceform for the event names. I dropped this patch. Submitted a follow-up. Let me know what you think.
Created attachment 168827 [details] [review] Bug 37301: (follow-up) PoC (Continuation): Add BaseResource and AgreementResource To squash with previous patch
Created attachment 168828 [details] [review] Bug 37301: Remove toToResource* methods to BaseResource
Created attachment 168829 [details] [review] Bug 37301: Add goToResourceShow to BaseResource
1. One question, for discussion: Should not we have those "resource" component part of a separate directory? Not sure but wondering. 2. Next steps could be: * continue to discuss on the existing bug, iterate with the AgreementsShow and AgreementsList * once we agree on a good-enough-for-now solution we create a separate bug for the BaseComponent idea * Build 37301 on top of it * Use the BaseComponent from everywhere (yet another bug report)
Created attachment 168836 [details] [review] Bug 37301: Get rid of e.preventDefault Feels wrong to have datatable+event related args in BaseResource, remove that. The only reason we needed preventDefault was because 'show' action was a link with an href, but we dont even need that, and add 'role="button"' for cursor:pointer mouseover interaction.
I'm really exited by the work happening here.. more re-usability and DRYer code for the win.. watching closely.
This looks very promising, should offer some good improvements and hopefully can be used to try and re-implement bug 34417 (In reply to Jonathan Druart from comment #10) > 1. One question, for discussion: > Should not we have those "resource" component part of a separate directory? > Not sure but wondering. I think this would be sensible, the resource components don't actually render any template to the UI themselves so it might be useful to isolate them from the ones that do
(In reply to Jonathan Druart from comment #10) > 1. One question, for discussion: > Should not we have those "resource" component part of a separate directory? > Not sure but wondering. > > 2. Next steps could be: > * continue to discuss on the existing bug, iterate with the AgreementsShow > and AgreementsList > * once we agree on a good-enough-for-now solution we create a separate bug > for the BaseComponent idea > * Build 37301 on top of it > * Use the BaseComponent from everywhere (yet another bug report) fwiw I'm on board with all of the above.
Created attachment 172713 [details] [review] Bug 37301: DRY delete
(In reply to Pedro Amorim from comment #15) > Created attachment 172713 [details] [review] [review] > Bug 37301: DRY delete I would make the success "message" configurable as well. Otherwise it looks great!
(In reply to Jonathan Druart from comment #16) > I would make the success "message" configurable as well. > Otherwise it looks great! Thanks! Sure, it can be further iterated on. I'm trying to limit this scope to not worrying about making flexible things that don't absolutely need it. The delete message being flexible would be nice to have as an option but I'm not considering it important right now. The main reason being every delete message we have currently is "Are you sure you want to delete x?". They're all the same and WET. Same goes for "Yes, delete" and "No, do not delete". I firmly believe these things should be written once and maintained in a single place.
Sorry if it was not clear enough. I was talking about the success message, (setMessage from success). And there are not the same, that is why I am asking. I have checked before :) koha-tmpl/intranet-tmpl/prog/js/vue/components/Preservation/WaitingList.vue 331 this.$__("Item removed from the waiting list"),
(In reply to Jonathan Druart from comment #18) > Sorry if it was not clear enough. I was talking about the success message, > (setMessage from success). And there are not the same, that is why I am > asking. > > I have checked before :) > > koha-tmpl/intranet-tmpl/prog/js/vue/components/Preservation/WaitingList.vue > 331 this.$__("Item removed from the waiting > list"), Ah! Fair!
Btw, would it be possible to get the components bundled as a library, which could then be imported into client-side apps? I love the idea of Koha Plugins being able to re-use Koha's Vue components by dynamically importing them at runtime. I think that Koha Plugins using Koha Vue components and Mojolicious REST API is a natural fit. (It could also make it easier to slowly refactor other parts of Koha to use Vue components.)
Created attachment 172911 [details] [review] Bug 37301: Move table_url declaration to resource class This is relevant because doResourceDelete expected table_url to be implemented, but LicensesList does not implement it for example. We now have a getResourceTableUrl implemented by all resources
Created attachment 172912 [details] [review] Bug 37301: Apply changes to licenses
Created attachment 172913 [details] [review] Bug 37301: Apply changes to eHoldings local packages
Created attachment 172914 [details] [review] Bug 37301: Restore possibility of a custom toolbar button EHoldingsLocalTitles needs this for 'Import from list' and 'Import from KBART file'
Created attachment 172915 [details] [review] Bug 37301: Apply changes to eholdings titles
Created attachment 172916 [details] [review] Bug 37301: Move setWarning to BaseResource UsageStatisticsDataProvidersList needs it, and others in the future will as well
Created attachment 172917 [details] [review] Bug 37301: Fix: URL param needs to match attr_id url param needs to match attr_id usage statistics is non-standard when compared to agreements+licenses etc. using usage_data_provider_id but resource id field is erm_usage_data_provider_id. This patch is fixing that.
Created attachment 172918 [details] [review] Bug 37301: Add required faicon for fontawesome compnent
Created attachment 172919 [details] [review] Bug 37301: Apply changes to usage statistics data providers This is also removing UsageStatissticsDataProvidersToolbar as that is not required, and updating the UsageStatisticsDataProvidersSummary button to a ToolbarButton instead of a router-link
Created attachment 172920 [details] [review] Bug 37301: Apply changes to preservation trains Also update edit route from TrainsFormEdit to TrainsFormAddEdit for consistency
Created attachment 172921 [details] [review] Bug 37301: Update cypress Local packages were named 'package' on the delete confirmation dialog, but 'local package' on the deleted confirmation message. Both these places now use the same i18n.display_name from the respective Resource component so these label tests need to be updated. Same for local titles. Test plan: cypress run --spec t/cypress/integration/ERM/DataProviders_spec.ts cypress run --spec t/cypress/integration/ERM/Agreements_spec.ts cypress run --spec t/cypress/integration/ERM/Licenses_spec.ts cypress run --spec t/cypress/integration/ERM/Titles_spec.ts cypress run --spec t/cypress/integration/ERM/Packages_spec.ts
Created attachment 174612 [details] [review] Bug 37301: [PoC] DRY ToolbarButton This is only a PoC applied to Agreements for discussion. This is a continuation of bug 37274. Bug 37274 applied the ToolbarButton to missing components but it was mostly replacing WET for WET. This is DRYer: - All mark-up related to toolbar buttons is now written once (css class, icons, elements) - All labels are now written once (except 'Add' for translatability)
Created attachment 174613 [details] [review] Bug 37301: Remove methods We do not need the methods, we can emit from the template. Also rename the events to the wording more consistent with what we have.
Created attachment 174614 [details] [review] Bug 37301: Rename 'type' with 'action' 'type' is too generic. Also move to else-if and add a else block to ease debugging in case an unsupported action is passed
Created attachment 174615 [details] [review] Bug 37301: PoC (Continuation): Add BaseResource and AgreementResource This is the building blocks for a much consistent approach, for example: AgreementsShow -> delete_agreement && AgreementsList -> doDelete should be DRY into AgreementResource Any WET code currently in setup for list+show of a resource can (and should) be moved into <resourceName>Resource Any WET code currently in setup for all 'resources' can (and should) be moved into BaseResource and others
Created attachment 174616 [details] [review] Bug 37301: (follow-up) PoC (Continuation): Add BaseResource and AgreementResource To squash with previous patch
Created attachment 174617 [details] [review] Bug 37301: Remove toToResource* methods to BaseResource
Created attachment 174618 [details] [review] Bug 37301: Add goToResourceShow to BaseResource
Created attachment 174619 [details] [review] Bug 37301: Get rid of e.preventDefault Feels wrong to have datatable+event related args in BaseResource, remove that. The only reason we needed preventDefault was because 'show' action was a link with an href, but we dont even need that, and add 'role="button"' for cursor:pointer mouseover interaction.
Created attachment 174620 [details] [review] Bug 37301: DRY delete
Created attachment 174621 [details] [review] Bug 37301: Move table_url declaration to resource class This is relevant because doResourceDelete expected table_url to be implemented, but LicensesList does not implement it for example. We now have a getResourceTableUrl implemented by all resources
Created attachment 174622 [details] [review] Bug 37301: Apply changes to licenses
Created attachment 174623 [details] [review] Bug 37301: Apply changes to eHoldings local packages
Created attachment 174624 [details] [review] Bug 37301: Restore possibility of a custom toolbar button EHoldingsLocalTitles needs this for 'Import from list' and 'Import from KBART file'
Created attachment 174625 [details] [review] Bug 37301: Apply changes to eholdings titles
Created attachment 174626 [details] [review] Bug 37301: Move setWarning to BaseResource UsageStatisticsDataProvidersList needs it, and others in the future will as well
Created attachment 174627 [details] [review] Bug 37301: Fix: URL param needs to match attr_id url param needs to match attr_id usage statistics is non-standard when compared to agreements+licenses etc. using usage_data_provider_id but resource id field is erm_usage_data_provider_id. This patch is fixing that.
Created attachment 174628 [details] [review] Bug 37301: Add required faicon for fontawesome compnent
Created attachment 174629 [details] [review] Bug 37301: Apply changes to usage statistics data providers This is also removing UsageStatissticsDataProvidersToolbar as that is not required, and updating the UsageStatisticsDataProvidersSummary button to a ToolbarButton instead of a router-link
Created attachment 174630 [details] [review] Bug 37301: Apply changes to preservation trains Also update edit route from TrainsFormEdit to TrainsFormAddEdit for consistency
Created attachment 174631 [details] [review] Bug 37301: Update cypress Local packages were named 'package' on the delete confirmation dialog, but 'local package' on the deleted confirmation message. Both these places now use the same i18n.display_name from the respective Resource component so these label tests need to be updated. Same for local titles. Test plan: cypress run --spec t/cypress/integration/ERM/DataProviders_spec.ts cypress run --spec t/cypress/integration/ERM/Agreements_spec.ts cypress run --spec t/cypress/integration/ERM/Licenses_spec.ts cypress run --spec t/cypress/integration/ERM/Titles_spec.ts cypress run --spec t/cypress/integration/ERM/Packages_spec.ts
I get two failing tests :/ Running: Titles_spec.ts (1 of 1) Title CRUD operations ✓ Import titles (5737ms) ✓ List title (3268ms) ✓ Add title (10740ms) ✓ Edit title (2773ms) ✓ Show title (3982ms) 1) Delete title 5 passing (39s) 1 failing 1) Title CRUD operations Delete title: AssertionError: Timed out retrying after 10000ms: Expected to find content: 'remove this local title' within the element: <h1> but never did. at Context.eval (webpack://koha/./t/cypress/integration/ERM/Titles_spec.ts:455:49) Running: Packages_spec.ts (1 of 1) Package CRUD operations ✓ List package (6329ms) ✓ Add package (5587ms) ✓ Edit package (3546ms) ✓ Show package (4711ms) 1) Delete package 4 passing (33s) 1 failing 1) Package CRUD operations Delete package: AssertionError: Timed out retrying after 10000ms: Expected to find content: 'remove this local package' within the element: <h1> but never did. at Context.eval (webpack://koha/./t/cypress/integration/ERM/Packages_spec.ts:281:49)
Created attachment 175450 [details] [review] Bug 37301: (QA follow-up): Fix Dialog_spec.ts Run the test spec before and after applying the patch: cypress run --spec t/cypress/integration/ERM/Dialog_spec.ts
(In reply to Paul Derscheid from comment #52) > I get two failing tests :/ Thanks Paul. I ran all cypress tests and the only that failed for me was the one that the follow-up patch fixes.
Created attachment 177173 [details] [review] Bug 37301: [PoC] DRY ToolbarButton This is only a PoC applied to Agreements for discussion. This is a continuation of bug 37274. Bug 37274 applied the ToolbarButton to missing components but it was mostly replacing WET for WET. This is DRYer: - All mark-up related to toolbar buttons is now written once (css class, icons, elements) - All labels are now written once (except 'Add' for translatability)
Created attachment 177174 [details] [review] Bug 37301: Remove methods We do not need the methods, we can emit from the template. Also rename the events to the wording more consistent with what we have.
Created attachment 177175 [details] [review] Bug 37301: Rename 'type' with 'action' 'type' is too generic. Also move to else-if and add a else block to ease debugging in case an unsupported action is passed
Created attachment 177176 [details] [review] Bug 37301: PoC (Continuation): Add BaseResource and AgreementResource This is the building blocks for a much consistent approach, for example: AgreementsShow -> delete_agreement && AgreementsList -> doDelete should be DRY into AgreementResource Any WET code currently in setup for list+show of a resource can (and should) be moved into <resourceName>Resource Any WET code currently in setup for all 'resources' can (and should) be moved into BaseResource and others
Created attachment 177177 [details] [review] Bug 37301: (follow-up) PoC (Continuation): Add BaseResource and AgreementResource To squash with previous patch
Created attachment 177178 [details] [review] Bug 37301: Remove toToResource* methods to BaseResource
Created attachment 177179 [details] [review] Bug 37301: Add goToResourceShow to BaseResource
Created attachment 177180 [details] [review] Bug 37301: Get rid of e.preventDefault Feels wrong to have datatable+event related args in BaseResource, remove that. The only reason we needed preventDefault was because 'show' action was a link with an href, but we dont even need that, and add 'role="button"' for cursor:pointer mouseover interaction.
Created attachment 177181 [details] [review] Bug 37301: DRY delete
Created attachment 177182 [details] [review] Bug 37301: Move table_url declaration to resource class This is relevant because doResourceDelete expected table_url to be implemented, but LicensesList does not implement it for example. We now have a getResourceTableUrl implemented by all resources
Created attachment 177183 [details] [review] Bug 37301: Apply changes to licenses
Created attachment 177184 [details] [review] Bug 37301: Apply changes to eHoldings local packages
Created attachment 177185 [details] [review] Bug 37301: Restore possibility of a custom toolbar button EHoldingsLocalTitles needs this for 'Import from list' and 'Import from KBART file'
Created attachment 177186 [details] [review] Bug 37301: Apply changes to eholdings titles
Created attachment 177187 [details] [review] Bug 37301: Move setWarning to BaseResource UsageStatisticsDataProvidersList needs it, and others in the future will as well
Created attachment 177188 [details] [review] Bug 37301: Fix: URL param needs to match attr_id url param needs to match attr_id usage statistics is non-standard when compared to agreements+licenses etc. using usage_data_provider_id but resource id field is erm_usage_data_provider_id. This patch is fixing that.
Created attachment 177189 [details] [review] Bug 37301: Add required faicon for fontawesome compnent
Created attachment 177190 [details] [review] Bug 37301: Apply changes to usage statistics data providers This is also removing UsageStatissticsDataProvidersToolbar as that is not required, and updating the UsageStatisticsDataProvidersSummary button to a ToolbarButton instead of a router-link
Created attachment 177191 [details] [review] Bug 37301: Apply changes to preservation trains Also update edit route from TrainsFormEdit to TrainsFormAddEdit for consistency
Created attachment 177192 [details] [review] Bug 37301: Update cypress Local packages were named 'package' on the delete confirmation dialog, but 'local package' on the deleted confirmation message. Both these places now use the same i18n.display_name from the respective Resource component so these label tests need to be updated. Same for local titles. Test plan: cypress run --spec t/cypress/integration/ERM/DataProviders_spec.ts cypress run --spec t/cypress/integration/ERM/Agreements_spec.ts cypress run --spec t/cypress/integration/ERM/Licenses_spec.ts cypress run --spec t/cypress/integration/ERM/Titles_spec.ts cypress run --spec t/cypress/integration/ERM/Packages_spec.ts
Created attachment 177193 [details] [review] Bug 37301: (QA follow-up): Fix Dialog_spec.ts Run the test spec before and after applying the patch: cypress run --spec t/cypress/integration/ERM/Dialog_spec.ts
Rebased after bug 38854
Created attachment 177351 [details] [review] Bug 37301: Use goToResourceList - does not work (?)
(In reply to Jonathan Druart from comment #77) > Created attachment 177351 [details] [review] [review] > Bug 37301: Use goToResourceList - does not work (?) Hey, yeah that looks like it. We (me and Matt) have been working extensively on this. And I'm very confident this is fixed in the follow-up bug. Bug 38201. Here is a commit in bug 38201 that fixes it for agreements for example: https://github.com/PTFS-Europe/koha/commit/878a136e73a5b6fbc7432f03d07b7e0b06a34c64 I understand everything should be fixed here as this is the first step, but I want to make clear that most of the vuejs rewrite work is now sitting on bug 38201. Here bug 37301 is just the first step. I say I'm very confident these things are fixed in the follow-up bug as we're building 2 new modules on top of it and have been testing and updating it frequently.
So this is not ready for testing and should be pushed along with bug 38201.
The commits here have been moved (and renamed) to bug 38201. Initially, back in July 2024, and then back in October 2024, the plan was to have this piece be pushed as a first step of a larger VueJS architecture rethink. That larger VueJS architecture rethink has now been materialized in bug 38201 and, even though is not ready yet, the patches here make more sense now in the context of bug 38201.