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.

Sign up for GitHub

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

Only display CollectionList if record is a collection #4170

Merged
merged 3 commits into from
Dec 19, 2024
Merged

Only display CollectionList if record is a collection #4170

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f35cb8
Adjusted to show collection list tab only if the record is a collection
LuomaJuha Dec 18, 2024
84ec1ec
Use getHierarchyTrees to determine if tab is viable
LuomaJuha Dec 18, 2024
fe43d88
Adjusted to only work with collection records if not in collection view
LuomaJuha Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions module/VuFind/src/VuFind/RecordTab/CollectionList.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,16 @@ public function getParams()
return $this->getResults()->getParams();
}

/**
* Is this tab active?
*
* @return bool
*/
public function isActive()
{
return parent::isActive() && $this->getRecordDriver()->tryMethod('getHierarchyTrees');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like isCollection might be the safest bet of all:

Suggested change
return parent::isActive() && $this->getRecordDriver()->tryMethod('getHierarchyTrees');
return parent::isActive() && $this->getRecordDriver()->tryMethod('isCollection');

Is there any reason not to do it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CollectionList can be active on subrecords and seems like not all of them implements is_hierarchy_id which is used in isCollection function check. That is why i was thinking about if using getHierarchyTopID would be better for this occasion 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible this is just a configuration issue? The hierarchy configuration controls which records are considered to be collections and which are not, based on the Solr index. If you have records that act like collections but do not have is_hierarchy_id set, that sounds like something non-standard, and I wonder if it would make more sense to customize record drivers than to introduce non-standard behavior to the record tab.

Bottom line: my feeling is that, at least for the core code, this tab is meant to operate on collections, so the best check we can apply is confirming that the record under consideration is a collection. Anything else feels like introducing different rules here than everywhere else, which doesn't make sense to me. But as I say, it's been a while since I've reviewed all this, so maybe I'm overlooking something -- my feeling, though, is that if something is wrong, the problem runs deeper than just this tab, and maybe needs to be addressed somewhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, actually this comment clears up my thoughts. I think the isCollection check makes alot more sense now in this context 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! I do think adding that check is an improvement, so if it works for you, I'll be happy to approve and merge this once the change is in place.

}

/**
* Get the processed search results.
*
Expand Down
Loading