Bug 34418 - Allow empty nodes in breadcrumb's elements
Summary: Allow empty nodes in breadcrumb's elements
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matt Blenkinsop
QA Contact:
URL:
Keywords:
: 34425 (view as bug list)
Depends on: 33169
Blocks: 30708
  Show dependency treegraph
 
Reported: 2023-07-26 11:14 UTC by Jonathan Druart
Modified: 2023-11-02 21:30 UTC (History)
8 users (show)

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


Attachments
Bug 34418: DO NOT PUSH - example (2.39 KB, patch)
2023-07-26 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: Allow empty nodes in breadcrumb's elements (1.10 KB, patch)
2023-07-26 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: Add is_empty flag (1.49 KB, patch)
2023-07-26 11:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: DO NOT PUSH - update example (968 bytes, patch)
2023-07-26 11:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: DO NOT PUSH - example (2.39 KB, patch)
2023-07-26 11:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: Allow empty nodes in breadcrumb's elements (1.09 KB, patch)
2023-07-26 11:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: Add is_empty flag (1.49 KB, patch)
2023-07-26 11:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: DO NOT PUSH - update example (968 bytes, patch)
2023-07-26 11:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: Replace dynamic parameter on unnamed route node (3.17 KB, patch)
2023-07-27 13:58 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 34418: Replace dynamic parameter on unnamed route node (3.24 KB, patch)
2023-07-31 14:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: DO NOT PUSH - example (2.46 KB, patch)
2023-08-03 08:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: Allow empty nodes in breadcrumb's elements (1.16 KB, patch)
2023-08-03 08:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: Add is_empty flag (1.56 KB, patch)
2023-08-03 08:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: DO NOT PUSH - update example (1.02 KB, patch)
2023-08-03 08:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: Replace dynamic parameter on unnamed route node (3.31 KB, patch)
2023-08-03 08:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34418: DO NOT PUSH - example (2.51 KB, patch)
2023-09-01 11:11 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34418: Allow empty nodes in breadcrumb's elements (1.22 KB, patch)
2023-09-01 11:11 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34418: Add is_empty flag (1.61 KB, patch)
2023-09-01 11:11 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34418: DO NOT PUSH - update example (1.07 KB, patch)
2023-09-01 11:11 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34418: Replace dynamic parameter on unnamed route node (3.37 KB, patch)
2023-09-01 11:11 UTC, Pedro Amorim
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:41 UTC
On bug 33169 we rewrite the way we build the breadcrumb for the Vue app. It leads to a missing use case, see bug 33169 comment 27:
"""
2. I am trying to adjust the code from bug 30708 to use this, but I am struggling. I don't know if it's me or this implementation that is not flexible enough.

I am trying to have, in the breadcrumbs:
/preservation/trains/3
=> Preservation > Trains > Show train

/preservation/trains/3/items/add
=> Preservation > Trains > Add item to train

Ideally we will need to display the name of the current resource, but that's another topic. I don't think it's feasible for now (discussed already in comment 18 and comment 19).

With my try on https://gitlab.com/joubu/Koha/-/commits/preservation_module-wip-33169 I am getting:

/preservation/trains/3
=> Preservation > Trains > Show train (OK)
/preservation/trains/3/items/add
=> Preservation > Trains > Show train > > Add item to train (KO there are 2 '>')
Additionally "Show train" is /preservation/trains/:train_id
"""
Comment 1 Jonathan Druart 2023-07-26 11:23:28 UTC
Created attachment 153902 [details] [review]
Bug 34418: DO NOT PUSH - example

Try the other patch with this patch applied.
Go to http://localhost:8081/cgi-bin/koha/erm/agreements/1/items/add

Without the patch you get:
    E-resource management > Agreements > Show agreement >  > Add item to train
With this patch applied:
    E-resource management > Agreements > Show agreement > Add item to train
Comment 2 Jonathan Druart 2023-07-26 11:23:30 UTC
Created attachment 153903 [details] [review]
Bug 34418: Allow empty nodes in breadcrumb's elements

Coming from an adaptation of "Bug 33169: DO NOT PUSH  Allow breadcrumbs to accept dynamic values"
Comment 3 Jonathan Druart 2023-07-26 11:26:14 UTC
Matt, I am wondering if we should not have a flag in the route definition instead, we already have is_base, is_navigation_item, is_end_node and is_default.

Shouldn't we have another one to flag this kind of node? Also it will clearly show in the routes that this node is always empty and can permit to catch bug more easily (Seeing the 2 "> >" in case it's empty but should not).
Comment 4 Jonathan Druart 2023-07-26 11:33:24 UTC
Created attachment 153904 [details] [review]
Bug 34418: Add is_empty flag
Comment 5 Jonathan Druart 2023-07-26 11:33:26 UTC
Created attachment 153905 [details] [review]
Bug 34418: DO NOT PUSH - update example
Comment 6 Jonathan Druart 2023-07-26 11:41:47 UTC
Created attachment 153910 [details] [review]
Bug 34418: DO NOT PUSH - example

Try the other patch with this patch applied.
Go to http://localhost:8081/cgi-bin/koha/erm/agreements/1/items/add

Without the patch you get:
    E-resource management > Agreements > Show agreement >  > Add item to train
With this patch applied:
    E-resource management > Agreements > Show agreement > Add item to train
Comment 7 Jonathan Druart 2023-07-26 11:41:50 UTC
Created attachment 153911 [details] [review]
Bug 34418: Allow empty nodes in breadcrumb's elements

Coming from an adaptation of "Bug 33169: DO NOT PUSH  Allow breadcrumbs to accept dynamic values"
Comment 8 Jonathan Druart 2023-07-26 11:41:53 UTC
Created attachment 153912 [details] [review]
Bug 34418: Add is_empty flag
Comment 9 Jonathan Druart 2023-07-26 11:41:57 UTC
Created attachment 153913 [details] [review]
Bug 34418: DO NOT PUSH - update example
Comment 10 Matt Blenkinsop 2023-07-26 13:42:25 UTC
(In reply to Jonathan Druart from comment #3)
> Matt, I am wondering if we should not have a flag in the route definition
> instead, we already have is_base, is_navigation_item, is_end_node and
> is_default.
> 
> Shouldn't we have another one to flag this kind of node? Also it will
> clearly show in the routes that this node is always empty and can permit to
> catch bug more easily (Seeing the 2 "> >" in case it's empty but should not).

This is definitely cleaner!
Comment 11 Jonathan Druart 2023-07-27 08:25:58 UTC
*** Bug 34425 has been marked as a duplicate of this bug. ***
Comment 12 Agustín Moyano 2023-07-27 13:58:51 UTC
Created attachment 153983 [details] [review]
Bug 34418: Replace dynamic parameter on unnamed route node
Comment 13 Agustín Moyano 2023-07-27 14:01:00 UTC
The last patch I pushed is to fix the link when it has parameters, for example, the "/preservation/trains/:train_id" on the Show train breadcrumb
Comment 14 Jonathan Druart 2023-07-27 14:29:12 UTC
(In reply to Agustín Moyano from comment #13)
> The last patch I pushed is to fix the link when it has parameters, for
> example, the "/preservation/trains/:train_id" on the Show train breadcrumb

I am not sure to understand what this patch is trying to fix, could you provide more detail please?

Does it apply to the ERM module as well?

I have tried on the wip for the preservation module, and:

Preservation > Trains > Show train > Add item to train
I have "Show train" with /cgi-bin/koha/preservation/trains/1

With and without this last patch.

I have adjusted the remote branch preservation_module-wip-33169
Comment 15 Agustín Moyano 2023-07-27 18:19:49 UTC
(In reply to Jonathan Druart from comment #14)
> (In reply to Agustín Moyano from comment #13)
> > The last patch I pushed is to fix the link when it has parameters, for
> > example, the "/preservation/trains/:train_id" on the Show train breadcrumb
> 
> I am not sure to understand what this patch is trying to fix, could you
> provide more detail please?
> 
> Does it apply to the ERM module as well?
> 
> I have tried on the wip for the preservation module, and:
> 
> Preservation > Trains > Show train > Add item to train
> I have "Show train" with /cgi-bin/koha/preservation/trains/1
> 
> With and without this last patch.
> 
> I have adjusted the remote branch preservation_module-wip-33169

According to vue-router docs, "params are ignored if a path is provided, which is not the case for query, as shown in the example above. Instead, you need to provide the name of the route or manually specify the whole path with any parameter"

That means that when we build the breadcrumb if in your routes tree you did not specify a name, NavigationItem will render the second choice

<router-link
    v-else-if="item.path"
    :to="item.path"
    :class="{ disabled: item.disabled }"
>

Where item.path would be /preservation/trains/:train_id

I did not check that branch, but if you added names to your router (as ERM has) this patch would have no effect
Comment 16 Agustín Moyano 2023-07-27 18:25:12 UTC
I see that you added 'name: "TrainsShow"' to the node, I believe that if you remove that name, you will see the difference with or without the patch
Comment 17 Jonathan Druart 2023-07-31 14:34:00 UTC
Created attachment 154095 [details] [review]
Bug 34418: Replace dynamic parameter on unnamed route node

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Jonathan Druart 2023-07-31 14:35:50 UTC
(In reply to Agustín Moyano from comment #16)
> I see that you added 'name: "TrainsShow"' to the node, I believe that if you
> remove that name, you will see the difference with or without the patch

Ok, I see now. It's working great!

For instance, when on:
Preservation > Trains > Show train > Add item to train
"Show train" is "preservation/trains/42" with this patch
Without this patch is was "preservation/trains/:train_id".
Comment 19 Jonathan Druart 2023-08-03 08:32:58 UTC
Created attachment 154210 [details] [review]
Bug 34418: DO NOT PUSH - example

Try the other patch with this patch applied.
Go to http://localhost:8081/cgi-bin/koha/erm/agreements/1/items/add

Without the patch you get:
    E-resource management > Agreements > Show agreement >  > Add item to train
With this patch applied:
    E-resource management > Agreements > Show agreement > Add item to train

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 20 Jonathan Druart 2023-08-03 08:33:01 UTC
Created attachment 154211 [details] [review]
Bug 34418: Allow empty nodes in breadcrumb's elements

Coming from an adaptation of "Bug 33169: DO NOT PUSH  Allow breadcrumbs to accept dynamic values"

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2023-08-03 08:33:04 UTC
Created attachment 154212 [details] [review]
Bug 34418: Add is_empty flag

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2023-08-03 08:33:07 UTC
Created attachment 154213 [details] [review]
Bug 34418: DO NOT PUSH - update example

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2023-08-03 08:33:10 UTC
Created attachment 154214 [details] [review]
Bug 34418: Replace dynamic parameter on unnamed route node

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2023-08-04 13:18:29 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2023-08-04 13:19:35 UTC Comment hidden (obsolete)
Comment 26 Pedro Amorim 2023-09-01 11:11:13 UTC
Created attachment 155098 [details] [review]
Bug 34418: DO NOT PUSH - example

Try the other patch with this patch applied.
Go to http://localhost:8081/cgi-bin/koha/erm/agreements/1/items/add

Without the patch you get:
    E-resource management > Agreements > Show agreement >  > Add item to train
With this patch applied:
    E-resource management > Agreements > Show agreement > Add item to train

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 27 Pedro Amorim 2023-09-01 11:11:15 UTC
Created attachment 155099 [details] [review]
Bug 34418: Allow empty nodes in breadcrumb's elements

Coming from an adaptation of "Bug 33169: DO NOT PUSH  Allow breadcrumbs to accept dynamic values"

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 28 Pedro Amorim 2023-09-01 11:11:18 UTC
Created attachment 155100 [details] [review]
Bug 34418: Add is_empty flag

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 29 Pedro Amorim 2023-09-01 11:11:21 UTC
Created attachment 155101 [details] [review]
Bug 34418: DO NOT PUSH - update example

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 30 Pedro Amorim 2023-09-01 11:11:24 UTC
Created attachment 155102 [details] [review]
Bug 34418: Replace dynamic parameter on unnamed route node

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 31 Tomás Cohen Arazi 2023-09-01 14:24:53 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 32 Fridolin Somers 2023-09-08 05:53:50 UTC
Depends on Bug 33169 not in 23.05.x
Comment 33 Fridolin Somers 2023-11-02 21:30:16 UTC
(In reply to Fridolin Somers from comment #32)
> Depends on Bug 33169 not in 23.05.x

In the end Bug 33169 is backported.
But since this is minor I propose to keep it that way.