Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Breadcrumb behaviour with multiple parents is undefined, output is problematic #2274

Open
kayakr opened this issue Dec 13, 2023 · 4 comments

Comments

@kayakr
Copy link
Contributor

kayakr commented Dec 13, 2023

What steps does it take to reproduce the issue?
By default a repository item can be a member of multiple collections. If multiple member of relation values are added, the breadcrumb appears to concatenate them to suggest a hierarchy that doesn't exist.

e.g. on https://sandbox.islandora.ca/, edit Repository Item "Among the Big Guns, Camp Brighton, Charlottetown, P.E.I" to be a member of both Simple Image Collection (5) and Document Collection (2), then output breadcrumb becomes "Home > Document Collection > Simple Image Collection" suggesting that Simple Image Collection is a child of Document Collection when it is not.

Also, if I alter the islandora_breadcrumb BreadcrumbsTest.php to make nodes B and C children of A and node D a child of both B and C, e.g where <- means Member of

  • A <- B

  • A <- C

  • B <- D and C <- D
    the generated breadcrumb is: Home » Node C » Node A » Node B. Node C has been injected at start of breadcrumb.

  • When does this issue occur?
    See above

  • Which page does it occur on?
    Default display page for repository item.

  • What happens?
    Breadcrumb implies problematic hierarchy.

  • To whom does it occur (anonymous visitor, editor, administrator)?
    All users

  • What did you expect to happen?
    It would be good to have agreement about what the behaviour should be. e.g. render breadcrumb based on first relationship only, or render multiple breadcrumbs.

Which version of Islandora are you using?
2.x

Any related open or closed issues to this bug report?

Screenshots:
islandora-1002-breadcrumb-2023-12-14 at 11 47 25 AM

@kayakr
Copy link
Contributor Author

kayakr commented Dec 13, 2023

Also, the current breadcrumb logic doesn't take into account whether an ancestor is unpublished. In the example on islandora sandbox, if I unpublish Document collection, it still appears in breadcrumb even after cache clear. Since most visitors are unlikely to be allowed to view unpublished content, it should be removed from breadcrumb navigation.

@kayakr
Copy link
Contributor Author

kayakr commented Dec 13, 2023

We've been here before... #1777 Breadcrumbs for object in multiple collections

@kayakr kayakr transferred this issue from Islandora/islandora Dec 13, 2023
@kayakr
Copy link
Contributor Author

kayakr commented Dec 14, 2023

Possible patch below for excluding unpublished nodes from breadcrumb:

diff --git a/modules/islandora_breadcrumbs/src/IslandoraBreadcrumbBuilder.php b/modules/islandora_breadcrumbs/src/IslandoraBreadcrumbBuilder.php
index 5ed7f6d0..9dc4cc8d 100644
--- a/modules/islandora_breadcrumbs/src/IslandoraBreadcrumbBuilder.php
+++ b/modules/islandora_breadcrumbs/src/IslandoraBreadcrumbBuilder.php
@@ -101,6 +101,10 @@ class IslandoraBreadcrumbBuilder implements BreadcrumbBuilderInterface {
     // Add membership chain to the breadcrumb.
     foreach ($chain as $chainlink) {
       $node = $this->nodeStorage->load($chainlink);
+      // Exclude unpublished nodes from breadcrumb navigation.
+      if (!$node->isPublished()) {
+        continue;
+      }
       $breadcrumb->addCacheableDependency($node);
       $breadcrumb->addLink($node->toLink());
     }

@rosiel
Copy link
Member

rosiel commented Dec 20, 2023

We discussed this at the call! There are 2 issues here.

1 - lack of access of user to the members of the breadcrumb chain. We'd go further than just check for unpublished, and check for access. We think it's pretty simple to do but is not readily on our minds. We're not sure what to do when you can't access, but my idea was truncate the breadcrumbs at whatever node you can't access.

2 - multiple breadcrumb chains. Seth suggested going with the first parent listed. We can do this because memberOf is a field on the child, so you can use the 'delta' to determine which breadcrumb takes priority. This got some assent in the call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants