Bug 34417 - ERM's breadcrumb (Vue) does not display the entity's name
Summary: ERM's breadcrumb (Vue) does not display the entity's name
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on: 33169
Blocks:
  Show dependency treegraph
 
Reported: 2023-07-26 11:14 UTC by Jonathan Druart
Modified: 2023-08-31 14:20 UTC (History)
6 users (show)

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


Attachments
Bug 34417: Allow breadcrumbs to accept dynamic values (20.70 KB, patch)
2023-07-26 11:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34417: Move logic to http client (15.80 KB, patch)
2023-07-27 10:46 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 34417: Allow breadcrumbs to accept dynamic values (20.70 KB, patch)
2023-07-27 11:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34417: Move logic to http client (15.86 KB, patch)
2023-07-27 11:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34417: Add routeClass to routes and api call (5.73 KB, patch)
2023-08-04 11:58 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 34417: [ALTERNATIVE PATCH] Set entity's name from the component itself (5.52 KB, patch)
2023-08-04 13:07 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 Jonathan Druart 2023-07-26 11:14:39 UTC
For instance we have "Show agreement" instead of the agreement's name.
Comment 1 Jonathan Druart 2023-07-26 11:46:43 UTC
Created attachment 153914 [details] [review]
Bug 34417: Allow breadcrumbs to accept dynamic values

This patch is an example of allowing a breadcrumb to accept a dynamic value such as an agreement or license name.
Comment 2 Jonathan Druart 2023-07-26 12:15:05 UTC
There are two things I would like to note here:
1. It does not feel correct to not invalidate the parameter when the view changed. We could end up with a name from the previous view. It's highlighted especially now, as we don't have all the routes modified.

2. It does not feel optimal to have to modify each component. It would be nicer if we could have the value set in the route definition.
For instance we could have: title: $__("Show {agreement.name}")
Also we would not need the 'dynamic' flag and title could stay a string.
I have no idea how this could be implemented however.
Comment 3 Matt Blenkinsop 2023-07-26 13:40:04 UTC
(In reply to Jonathan Druart from comment #2)
> There are two things I would like to note here:
> 1. It does not feel correct to not invalidate the parameter when the view
> changed. We could end up with a name from the previous view. It's
> highlighted especially now, as we don't have all the routes modified.

Agreed, I'll add a line to reset it after each render

> 2. It does not feel optimal to have to modify each component. It would be
> nicer if we could have the value set in the route definition.
> For instance we could have: title: $__("Show {agreement.name}")
> Also we would not need the 'dynamic' flag and title could stay a string.
> I have no idea how this could be implemented however.


This would be preferable but I couldn't think of a way of doing this as the routes are currently generated statically when the application loads. I've just been re-reviewing and I wonder if we could pass a function as the title property...? Then maybe we could emit something from within the API client rather than each component which that function could pick up. That way the data passing is centralised between the api client and the navigation store. I'll have a play around and see if I can get something to work
Comment 4 Matt Blenkinsop 2023-07-27 10:46:07 UTC
Created attachment 153958 [details] [review]
Bug 34417: Move logic to http client

This patch moves the logic for setting dynamic breadcrumb values to the http client to avoid modifying all components that need dynamic values.

This patch also fixes the warning about discarding invalid params when navigating

Test plan:
Create some agreements/licenses/titles/packages and navigate through the ERM module especially to the "show" pages and the "edit" pages for those respective items. On these pages there should be a descriptive breadcrumb using either the name or title of the item.
Comment 5 Matt Blenkinsop 2023-07-27 10:54:33 UTC
Hopefully this applies correctly following the split out from 33169 - let me know if not and I can rebase
Comment 6 Jonathan Druart 2023-07-27 11:24:08 UTC
Created attachment 153959 [details] [review]
Bug 34417: Allow breadcrumbs to accept dynamic values

This patch is an example of allowing a breadcrumb to accept a dynamic value such as an agreement or license name.
Comment 7 Jonathan Druart 2023-07-27 11:24:12 UTC
Created attachment 153960 [details] [review]
Bug 34417: Move logic to http client

This patch moves the logic for setting dynamic breadcrumb values to the http client to avoid modifying all components that need dynamic values.

This patch also fixes the warning about discarding invalid params when navigating

Test plan:
Create some agreements/licenses/titles/packages and navigate through the ERM module especially to the "show" pages and the "edit" pages for those respective items. On these pages there should be a descriptive breadcrumb using either the name or title of the item.
Comment 8 Jonathan Druart 2023-07-27 11:24:20 UTC
(In reply to Matt Blenkinsop from comment #5)
> Hopefully this applies correctly following the split out from 33169 - let me
> know if not and I can rebase

I didn't, but I fixed the conflicts.
Comment 9 Jonathan Druart 2023-07-27 11:28:03 UTC
Interesting, but having this code in _fetchJSON seems wrong. It will be called too many times.

Also it's not possible to have "Edit {agreement.name}", it is what we do in other places.
Comment 10 Jonathan Druart 2023-07-27 11:37:58 UTC
The identifier is too generic and we are not asking for a specific object, only an attribute's name, I can imagine easily side-effects:
show an agreement will also display info about the license:
 get /agreement/1
 get /licenses/1
you have agreement.name=foo and license.name=bar
asking in the route for "name" will be ambiguous, and the breadcrumbs will display randomly 'foo' or 'bar'.

It does not happen in our case because we embed the license's info in the GET agreements/:id endpoint, but you get the idea.
Comment 11 Matt Blenkinsop 2023-07-27 15:07:45 UTC
I'm looking at adding a class to each route to determine whether its an agreement/license etc (programatically, not manually in erm.js). I think I'll move the logic out of _fetchJSON into the get() request as that should be the only time we need dynamic data in the breadcrumb. Within that I can then use the added route class to determine which requests should actually set parameters (e.g. licenses shouldn't trigger it when an agreement route is called)
Comment 12 Matt Blenkinsop 2023-08-04 11:58:28 UTC
Created attachment 154258 [details] [review]
Bug 34417: Add routeClass to routes and api call

This patch adds a class to match the route to the API call and ensure that no items with the same property (e.g. license.name and agreement.name) can lead to incorrect breadcrumbs.

Code is also moved into the get() request function to reduce the number of times it is called
Comment 13 Jonathan Druart 2023-08-04 13:07:42 UTC
Created attachment 154263 [details] [review]
Bug 34417: [ALTERNATIVE PATCH] Set entity's name from the component itself

I would have preferred to not modify the component to pass what we have
fetched, but I think it's cleaner than hacking the low level fetch
function.

This is just a POC and work only on the "show agreement" view
Comment 14 Jonathan Druart 2023-08-04 13:19:30 UTC
Paul, we need you here.
Comment 15 Matt Blenkinsop 2023-08-04 13:40:48 UTC
Passing the data from the component is for sure the cleanest method. Setting the breadcrumb from the get() function was the best method I could find for avoiding doing that but I think it will be harder to maintain if we get a lot more edge cases/more complicated routes
Comment 16 Jonathan Druart 2023-08-31 14:11:16 UTC
Should we go with this alternative patch if we don't find anything better?
Comment 17 Matt Blenkinsop 2023-08-31 14:20:08 UTC
I think that is the best option of the two, far less likely to introduce regressions