|
Description
Pedro Amorim
2024-07-10 10:19:45 UTC
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 |