Issue 1788

[alexdryden]

This fixes #1788 by adding a site setting to limit the properties and classes that populate the advanced item search property select and resource-class select elements. Users can limit the options to properties/classes used by any Omeka S site, or to only those properties used in this site, or keep the default behavior to show all properties and classes for all installed vocabularies.

[Daniel-KM]

Note that there was already a pull request that had the same aim three years ago (#1423), still alive.

[zerocrates]

Apologies to all on the sluggish (in some cases extremely sluggish) response to these... it's definitely a good feature.

This version of the change with its support for both "any site" and "current site" filtering is probably preferable to the prior one, so I think we'll consolidate to this one.

We'll probably have to tweak some of the language... for one I think the "any/all sites" option is actually "used by any resource," whether or not it's on a site. Also, for consistency with other usages we'd prefer the new key used here for the query to be site_id, I think.

Finally, I think it's going to end up cleaner to just pass the proper used/site_id params into the query option for the elements when using them rather than introducing a new option that sets them. The default "all" mode doesn't need to pass anything, "used anywhere" can pass just used and used in this site can pass just site_id.

[alexdryden]

Awesome, thanks @zerocrates. I can start making the changes you describe this week.

[alexdryden]

I think I got the first two parts addressed, but let me know if they still need work or if I misunderstood.

For the third part, I think I understand what you mean, but I don't have the implementation correct in b4bba8f because it is messing up the return when neither used or site_id is used. I think the solution is to only add the 'query' => $query option to the element only when we need to add either used or site_id.

I'll keep working at it but let me know if my diagnosis is incorrect or if there is convenient example you can point me to that implements this pattern correctly.

[zerocrates]

When you say it's "messing up," what's happening? Passing an empty-array query option should be the same as passing no query at all.

[alexdryden]

Passing an empty-array query option

Ah--that must have been it. I was adding the used or site_id values to the $query variable that was already instantiated and populated within the view. When I instantiate a new array and pass that to the select element it works as expected.

It was populating the select with only the properties/classes of a single Vocabulary instead of all installed vocabularies/classes. I'm still not exactly sure why passing the existing $query array without used or site_id would make a difference. I'll double-check on a fresh install to make sure it isn't something idiosyncratic about my dev environment.

[alexdryden]

Okay--the behavior I was seeing now makes sense to me. The parameters I was accidentally passing by re-using the existing $query array were:

"sort_by" => "created",
"sort_order" => "desc",
"page" => "1",

So, the select elements were getting populated with only one "page" worth of properties and classes from the most recent vocabulary I imported. These erroneously sent parameters weren't affecting the results when I was limited to just the properties/classes used by the site or those used by any resource because my test items used less than one page worth of properties/classes. Adding this comment to document the bug.

[zerocrates]

Yeah, you definitely don't want to use the whole search query and pass it as the query to the select element: that would cause problems like what you saw.

[alexdryden]

Okay, just ran gulp test:cs and there shouldn't be any errors on those tests now. I'm not sure how to configure the db credentials for test:php but if you need me to run those too just let me know.

[zerocrates]

We can have Github Actions run all the tests for you, it's no problem.

The test credentials aren't anything complex if you'd like to set them up for yourself: just make a new database just for testing (it will be wiped between runs) and fill in its credentials in application/test/config/database.ini.

[zerocrates]

So just one minor thing: since this has kind of walked back to leaving the previous "used" queries alone, can you restore the code for them as they were? As it is, there's some non-significant changes like changing !empty to isset && truthy and changes to linebreaks and so on, that just clutter up the change. This PR should be "all green," i.e. not changing any existing lines, I think.

Oh: and when you want to look up the site, you don't need to go and get the slug and then call into the API and all that: we set it to the top-level view object, so you should be able to just do $this->layout()->site to get it.

[alexdryden]

Thanks, John!

[zerocrates]

Thanks for working on this with us, Alex.

I've committed a squash of this to develop, with a couple more changes. There's some code-level cosmetics stuff, some things like aligning to the structure of the site settings which we've changed in the meantime since you started this PR, and some changes to the text for the settings. That stuff is folded into the single squashed commit.

Additionally, I have an immediately following commit that converts your queries for site_id to use subqueries: for some reason the MySQL query optimizer works better with that structure here, so there's very substantially improved performance with the modified queries vs. the ones in the PR.