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

Adds link to exhibits containing each item in the item browse detail #83

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

NedHenry
Copy link

Some Omeka admins at UCSC have had trouble figuring out which items have been added to which exhibits. We identified a need to see which exhibits an item belongs to while browsing through items. This simple addition to the ExhibitBuilder plugin adds to the item detail view a link to each exhibit containing the item, along with the exhibit title.

@zerocrates
Copy link
Member

Is the ACL change related? It does a few things that definitely don't look quite right.

@NedHenry
Copy link
Author

I must have made a mistake submitting the pull request; I did not intend to
submit any acl changes. We have made some local acl changes to one of our
omeka instances, and some of these must have been added to the code to
merge. The only change I intended to pull is a hook to
admin_item_browse_detailed. Let me look over my work. I will submit another
pull request with only the intended changes.
Thanks for catching this!
Ned

Is the ACL change related? It does a few things that definitely don't look
quite right.


Reply to this email directly or view it on GitHub
#83 (comment)
.

@NedHenry
Copy link
Author

I had indeed made a mistake, and committed two changes to the fork associated with this pull request which were meant to remain on a branch local to one of our Omeka installations. I believe that I have now corrected the problem, and have left only the relatively simple change I originally meant to suggest. Thanks for noticing and informing me of the problem!

@zerocrates
Copy link
Member

I'm concerned about the performance implications of this strategy: if you're showing 20 items per page and you have 10 exhibits, this adds 220 new database queries to every browse page. Doing this through joins is probably the better move (i.e., directly select only the exhibits that contain your item, rather than selecting all the exhibits and checking each one).

@NedHenry
Copy link
Author

That is a very good point, of course. I believe I have updated the pull request to use a single database query per page load, no matter how many items are displayed or how many exhibits exist. Thanks for the feedback and patience! No offense taken if you decide this functionality isn't right for the plugin.

@zerocrates
Copy link
Member

Unfortunately I think this one goes too far in the other direction: your single query returns every item that exists in any exhibit, which is likely to cause memory limit problems.

I think the happy medium is to basically switch your query around: in the detail hook, run a query that retrieves all the Exhibits the item appears in (so, start with a query against the Exhibits table and do the joins that way and then group by exhibit ID). The end result is more queries (one for each item), but each one is manageably small.

As a matter of organization, it also probably makes sense to implement the query as a method on the Exhibit Table class (findByItem() or something similar), to keep the SQL more contained there and to make it easier to, for example, add an infobox to the item show page containing the same information without having to duplicate the query.

@NedHenry
Copy link
Author

I see what you mean. Thanks for the detailed suggestion. I made the changes you suggested, and I agree it is much better this way.

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

Successfully merging this pull request may close these issues.

None yet

2 participants