Bug 37301 - Further improve how we build Vue components
Summary: Further improve how we build Vue components
Status: RESOLVED MOVED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 37274
Blocks:
  Show dependency treegraph
 
Reported: 2024-07-10 10:19 UTC by Pedro Amorim
Modified: 2025-02-14 12:56 UTC (History)
7 users (show)

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


Attachments
Bug 37301: [PoC] DRY ToolbarButton (5.22 KB, patch)
2024-07-10 10:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Remove methods (3.48 KB, patch)
2024-07-10 13:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37301: Rename 'type' with 'action' (3.32 KB, patch)
2024-07-10 13:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37301: Restore to (4.55 KB, patch)
2024-07-10 13:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37301: PoC (Continuation): Add BaseResource and AgreementResource (6.46 KB, patch)
2024-07-11 12:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: (follow-up) PoC (Continuation): Add BaseResource and AgreementResource (2.19 KB, patch)
2024-07-11 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37301: Remove toToResource* methods to BaseResource (7.10 KB, patch)
2024-07-11 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37301: Add goToResourceShow to BaseResource (3.42 KB, patch)
2024-07-11 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 37301: Get rid of e.preventDefault (2.28 KB, patch)
2024-07-11 15:17 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: DRY delete (8.78 KB, patch)
2024-10-14 11:54 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Move table_url declaration to resource class (3.34 KB, patch)
2024-10-17 16:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to licenses (9.48 KB, patch)
2024-10-17 16:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to eHoldings local packages (10.47 KB, patch)
2024-10-17 16:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Restore possibility of a custom toolbar button (1.31 KB, patch)
2024-10-17 16:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to eholdings titles (10.25 KB, patch)
2024-10-17 16:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Move setWarning to BaseResource (1.35 KB, patch)
2024-10-17 16:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Fix: URL param needs to match attr_id (8.17 KB, patch)
2024-10-17 16:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Add required faicon for fontawesome compnent (1.09 KB, patch)
2024-10-17 16:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to usage statistics data providers (14.88 KB, patch)
2024-10-17 16:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to preservation trains (10.62 KB, patch)
2024-10-17 16:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Update cypress (3.85 KB, patch)
2024-10-17 16:24 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: [PoC] DRY ToolbarButton (5.22 KB, patch)
2024-11-15 16:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Remove methods (3.47 KB, patch)
2024-11-15 16:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Rename 'type' with 'action' (3.32 KB, patch)
2024-11-15 16:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: PoC (Continuation): Add BaseResource and AgreementResource (6.58 KB, patch)
2024-11-15 16:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: (follow-up) PoC (Continuation): Add BaseResource and AgreementResource (2.18 KB, patch)
2024-11-15 16:44 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Remove toToResource* methods to BaseResource (7.09 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Add goToResourceShow to BaseResource (3.50 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Get rid of e.preventDefault (2.28 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: DRY delete (8.82 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Move table_url declaration to resource class (3.34 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to licenses (9.69 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to eHoldings local packages (10.58 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Restore possibility of a custom toolbar button (1.31 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to eholdings titles (10.25 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Move setWarning to BaseResource (1.35 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Fix: URL param needs to match attr_id (8.17 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Add required faicon for fontawesome compnent (1.09 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to usage statistics data providers (14.88 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to preservation trains (10.62 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Update cypress (3.85 KB, patch)
2024-11-15 16:45 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: (QA follow-up): Fix Dialog_spec.ts (1.01 KB, patch)
2024-12-13 15:49 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: [PoC] DRY ToolbarButton (5.22 KB, patch)
2025-01-27 11:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Remove methods (3.53 KB, patch)
2025-01-27 11:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Rename 'type' with 'action' (3.38 KB, patch)
2025-01-27 11:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: PoC (Continuation): Add BaseResource and AgreementResource (6.58 KB, patch)
2025-01-27 11:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: (follow-up) PoC (Continuation): Add BaseResource and AgreementResource (2.19 KB, patch)
2025-01-27 11:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Remove toToResource* methods to BaseResource (7.13 KB, patch)
2025-01-27 11:22 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Add goToResourceShow to BaseResource (3.51 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Get rid of e.preventDefault (2.28 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: DRY delete (8.83 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Move table_url declaration to resource class (3.35 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to licenses (9.65 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to eHoldings local packages (10.55 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Restore possibility of a custom toolbar button (1.32 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to eholdings titles (10.22 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Move setWarning to BaseResource (1.36 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Fix: URL param needs to match attr_id (8.19 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Add required faicon for fontawesome compnent (1.09 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to usage statistics data providers (14.85 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Apply changes to preservation trains (10.58 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Update cypress (3.85 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: (QA follow-up): Fix Dialog_spec.ts (1.01 KB, patch)
2025-01-27 11:23 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 37301: Use goToResourceList - does not work (?) (2.92 KB, patch)
2025-01-30 10:43 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2024-07-10 10:19:45 UTC

    
Comment 1 Pedro Amorim 2024-07-10 10:22:54 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)
Comment 2 Jonathan Druart 2024-07-10 13:33:12 UTC
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.
Comment 3 Jonathan Druart 2024-07-10 13:33:15 UTC
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
Comment 4 Jonathan Druart 2024-07-10 13:33:17 UTC
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.
Comment 5 Pedro Amorim 2024-07-11 12:45:55 UTC
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
Comment 6 Pedro Amorim 2024-07-11 12:46:30 UTC
(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.
Comment 7 Jonathan Druart 2024-07-11 14:13:16 UTC
Created attachment 168827 [details] [review]
Bug 37301: (follow-up) PoC (Continuation): Add BaseResource and AgreementResource

To squash with previous patch
Comment 8 Jonathan Druart 2024-07-11 14:13:19 UTC
Created attachment 168828 [details] [review]
Bug 37301: Remove toToResource* methods to BaseResource
Comment 9 Jonathan Druart 2024-07-11 14:13:21 UTC
Created attachment 168829 [details] [review]
Bug 37301: Add goToResourceShow to BaseResource
Comment 10 Jonathan Druart 2024-07-11 14:15:33 UTC
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)
Comment 11 Pedro Amorim 2024-07-11 15:17:57 UTC
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.
Comment 12 Martin Renvoize (ashimema) 2024-07-16 06:36:27 UTC
I'm really exited by the work happening here.. more re-usability and DRYer code for the win.. watching closely.
Comment 13 Matt Blenkinsop 2024-07-16 08:25:25 UTC
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
Comment 14 Pedro Amorim 2024-07-25 17:31:19 UTC
(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.
Comment 15 Pedro Amorim 2024-10-14 11:54:12 UTC
Created attachment 172713 [details] [review]
Bug 37301: DRY delete
Comment 16 Jonathan Druart 2024-10-15 09:19:38 UTC
(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!
Comment 17 Pedro Amorim 2024-10-15 10:52:37 UTC
(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.
Comment 18 Jonathan Druart 2024-10-15 11:42:30 UTC
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"),
Comment 19 Pedro Amorim 2024-10-15 12:16:34 UTC
(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!
Comment 20 David Cook 2024-10-15 23:26:39 UTC
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.)
Comment 21 Pedro Amorim 2024-10-17 16:24:16 UTC
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
Comment 22 Pedro Amorim 2024-10-17 16:24:19 UTC
Created attachment 172912 [details] [review]
Bug 37301: Apply changes to licenses
Comment 23 Pedro Amorim 2024-10-17 16:24:22 UTC
Created attachment 172913 [details] [review]
Bug 37301: Apply changes to eHoldings local packages
Comment 24 Pedro Amorim 2024-10-17 16:24:25 UTC
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'
Comment 25 Pedro Amorim 2024-10-17 16:24:28 UTC
Created attachment 172915 [details] [review]
Bug 37301: Apply changes to eholdings titles
Comment 26 Pedro Amorim 2024-10-17 16:24:31 UTC
Created attachment 172916 [details] [review]
Bug 37301: Move setWarning to BaseResource

UsageStatisticsDataProvidersList needs it, and others in the future will as well
Comment 27 Pedro Amorim 2024-10-17 16:24:34 UTC
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.
Comment 28 Pedro Amorim 2024-10-17 16:24:37 UTC
Created attachment 172918 [details] [review]
Bug 37301: Add required faicon for fontawesome compnent
Comment 29 Pedro Amorim 2024-10-17 16:24:40 UTC
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
Comment 30 Pedro Amorim 2024-10-17 16:24:43 UTC
Created attachment 172920 [details] [review]
Bug 37301: Apply changes to preservation trains

Also update edit route from TrainsFormEdit to TrainsFormAddEdit for consistency
Comment 31 Pedro Amorim 2024-10-17 16:24:46 UTC
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
Comment 32 Pedro Amorim 2024-11-15 16:44:46 UTC
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)
Comment 33 Pedro Amorim 2024-11-15 16:44:48 UTC
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.
Comment 34 Pedro Amorim 2024-11-15 16:44:51 UTC
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
Comment 35 Pedro Amorim 2024-11-15 16:44:54 UTC
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
Comment 36 Pedro Amorim 2024-11-15 16:44:57 UTC
Created attachment 174616 [details] [review]
Bug 37301: (follow-up) PoC (Continuation): Add BaseResource and AgreementResource

To squash with previous patch
Comment 37 Pedro Amorim 2024-11-15 16:45:00 UTC
Created attachment 174617 [details] [review]
Bug 37301: Remove toToResource* methods to BaseResource
Comment 38 Pedro Amorim 2024-11-15 16:45:03 UTC
Created attachment 174618 [details] [review]
Bug 37301: Add goToResourceShow to BaseResource
Comment 39 Pedro Amorim 2024-11-15 16:45:05 UTC
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.
Comment 40 Pedro Amorim 2024-11-15 16:45:08 UTC
Created attachment 174620 [details] [review]
Bug 37301: DRY delete
Comment 41 Pedro Amorim 2024-11-15 16:45:11 UTC
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
Comment 42 Pedro Amorim 2024-11-15 16:45:14 UTC
Created attachment 174622 [details] [review]
Bug 37301: Apply changes to licenses
Comment 43 Pedro Amorim 2024-11-15 16:45:17 UTC
Created attachment 174623 [details] [review]
Bug 37301: Apply changes to eHoldings local packages
Comment 44 Pedro Amorim 2024-11-15 16:45:20 UTC
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'
Comment 45 Pedro Amorim 2024-11-15 16:45:23 UTC
Created attachment 174625 [details] [review]
Bug 37301: Apply changes to eholdings titles
Comment 46 Pedro Amorim 2024-11-15 16:45:26 UTC
Created attachment 174626 [details] [review]
Bug 37301: Move setWarning to BaseResource

UsageStatisticsDataProvidersList needs it, and others in the future will as well
Comment 47 Pedro Amorim 2024-11-15 16:45:30 UTC
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.
Comment 48 Pedro Amorim 2024-11-15 16:45:33 UTC
Created attachment 174628 [details] [review]
Bug 37301: Add required faicon for fontawesome compnent
Comment 49 Pedro Amorim 2024-11-15 16:45:36 UTC
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
Comment 50 Pedro Amorim 2024-11-15 16:45:39 UTC
Created attachment 174630 [details] [review]
Bug 37301: Apply changes to preservation trains

Also update edit route from TrainsFormEdit to TrainsFormAddEdit for consistency
Comment 51 Pedro Amorim 2024-11-15 16:45:42 UTC
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
Comment 52 Paul Derscheid 2024-11-22 11:47:39 UTC
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)
Comment 53 Pedro Amorim 2024-12-13 15:49:02 UTC
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
Comment 54 Pedro Amorim 2024-12-13 15:49:49 UTC
(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.
Comment 55 Pedro Amorim 2025-01-27 11:22:42 UTC
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)
Comment 56 Pedro Amorim 2025-01-27 11:22:45 UTC
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.
Comment 57 Pedro Amorim 2025-01-27 11:22:48 UTC
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
Comment 58 Pedro Amorim 2025-01-27 11:22:52 UTC
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
Comment 59 Pedro Amorim 2025-01-27 11:22:55 UTC
Created attachment 177177 [details] [review]
Bug 37301: (follow-up) PoC (Continuation): Add BaseResource and AgreementResource

To squash with previous patch
Comment 60 Pedro Amorim 2025-01-27 11:22:58 UTC
Created attachment 177178 [details] [review]
Bug 37301: Remove toToResource* methods to BaseResource
Comment 61 Pedro Amorim 2025-01-27 11:23:02 UTC
Created attachment 177179 [details] [review]
Bug 37301: Add goToResourceShow to BaseResource
Comment 62 Pedro Amorim 2025-01-27 11:23:05 UTC
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.
Comment 63 Pedro Amorim 2025-01-27 11:23:08 UTC
Created attachment 177181 [details] [review]
Bug 37301: DRY delete
Comment 64 Pedro Amorim 2025-01-27 11:23:12 UTC
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
Comment 65 Pedro Amorim 2025-01-27 11:23:15 UTC
Created attachment 177183 [details] [review]
Bug 37301: Apply changes to licenses
Comment 66 Pedro Amorim 2025-01-27 11:23:18 UTC
Created attachment 177184 [details] [review]
Bug 37301: Apply changes to eHoldings local packages
Comment 67 Pedro Amorim 2025-01-27 11:23:22 UTC
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'
Comment 68 Pedro Amorim 2025-01-27 11:23:25 UTC
Created attachment 177186 [details] [review]
Bug 37301: Apply changes to eholdings titles
Comment 69 Pedro Amorim 2025-01-27 11:23:28 UTC
Created attachment 177187 [details] [review]
Bug 37301: Move setWarning to BaseResource

UsageStatisticsDataProvidersList needs it, and others in the future will as well
Comment 70 Pedro Amorim 2025-01-27 11:23:32 UTC
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.
Comment 71 Pedro Amorim 2025-01-27 11:23:35 UTC
Created attachment 177189 [details] [review]
Bug 37301: Add required faicon for fontawesome compnent
Comment 72 Pedro Amorim 2025-01-27 11:23:39 UTC
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
Comment 73 Pedro Amorim 2025-01-27 11:23:42 UTC
Created attachment 177191 [details] [review]
Bug 37301: Apply changes to preservation trains

Also update edit route from TrainsFormEdit to TrainsFormAddEdit for consistency
Comment 74 Pedro Amorim 2025-01-27 11:23:46 UTC
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
Comment 75 Pedro Amorim 2025-01-27 11:23:50 UTC
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
Comment 76 Pedro Amorim 2025-01-27 11:24:04 UTC
Rebased after bug 38854
Comment 77 Jonathan Druart 2025-01-30 10:43:38 UTC
Created attachment 177351 [details] [review]
Bug 37301: Use goToResourceList - does not work (?)
Comment 78 Pedro Amorim 2025-01-30 10:56:36 UTC
(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.
Comment 79 Jonathan Druart 2025-01-30 15:21:11 UTC
So this is not ready for testing and should be pushed along with bug 38201.
Comment 80 Pedro Amorim 2025-02-14 12:55:59 UTC
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.