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

Allow page view derived Advanced Search blocks on any page #843

Conversation

nigelgbanks
Copy link
Member

@nigelgbanks nigelgbanks commented Jul 20, 2021

Allows the Advanced Search block to be used on pages
where its corresponding view is not present. Now it
will redirect to the route associated with the view
it was generated from.

This is not valid for Advanced Search blocks that
are derived from non-page based views, as there is
no known location in which to redirect the query.

GitHub Issue: Islandora/documentation#1869

  • Other Relevant Links (Google Groups discussion, related pull requests,
    Release pull requests, etc.)

https://islandora.slack.com/archives/CM5PPAV28/p1626110292349300

What does this Pull Request do?

Allows advanced search blocks to be used on pages where the view that they are derived from is not present. Only applies to page views, as they have a location to redirect to.

How should this be tested?

  • On an existing site, place a page view derived advanced search block on say the front page.
  • Confirm that searching with it does not work
  • Update islandora to the latest commit in this pull request
  • Clear the cache
  • Attempt to search on the front page again with the advanced search block
  • Should now redirect the user to the page associated with the view used to derive the advanced search block

Example of previous behaviour

error

Behaviour after fix

fixed

Interested parties

@Islandora/8-x-committers

Allows the Advanced Search block to be used on pages
where its corresponding view is not present. Now it
will redirect to the route associated with the view
it was generated from.

This is not valid for Advanced Search blocks that
are derived from non-page based views, as there is
no known location in which to redirect the query.
@nigelgbanks nigelgbanks marked this pull request as ready for review July 20, 2021 15:07
kylehuynh205 pushed a commit to Islandora/advanced_search that referenced this pull request Jul 23, 2021
@dannylamb
Copy link

dannylamb commented Jul 24, 2021

I just tried this fix out and I can't seem to get it to go. I've confirmed that solr search is working with non-advanced search, but every time I use the advanced search block it's almost as if the query never fires after the redirect.

image

I'm getting redirected to https://islandora.traefik.me/solr-search/content?a[0][f]=title&a[0][i]=IS&a[0][v]=example when that happens. There's nothing in the JS console, but in my logs I'm getting this:

Notice: Undefined index: recursive in Drupal\islandora_advanced_search\Form\AdvancedSearchForm->buildUrl() (line 395 of /var/www/drupal/web/modules/contrib/islandora/modules/islandora_advanced_search/src/Form/AdvancedSearchForm.php)

@nigelgbanks
Copy link
Member Author

@dannylamb I just pushed a fix for the notice, regardless I would expect that line:

$recurse = filter_var(isset($values['recursive']) ? $values['recursive'] : FALSE, FILTER_VALIDATE_BOOLEAN);

To set $recurse to FALSE. That seems to hold out given the URL you mention: https://islandora.traefik.me/solr-search/content?a[0][f]=title&a[0][i]=IS&a[0][v]=example doesn't contain &recurse=1.

When you search without the Advanced search module, what page does it redirect you to, is it different? I'm thinking there must be some configuration issue, though not sure what it is without access to the site.

@nigelgbanks
Copy link
Member Author

@dannylamb what's the commit of isle-dc and the values you have for TAG in the environment, I try to reproduce this locally.

@dannylamb
Copy link

Let me spin another environment up and I'll give you the exact steps I'm doing to reproduce. Since this is a bugfix, I'd love to get it in for the release.

@dannylamb
Copy link

dannylamb commented Jul 27, 2021

I did this with a make local using islandora-sandbox as a codebase. I'm not installing from config. Just getting a fresh and clean Islandora and selectively installing what I need to test. It's probably part of this process that I'm messing up and maybe you can spot something I'm doing wrong.

  • Get the latest isle-dc. I'm on HEAD.
  • sudo rm -rf codebase; git clone https://github.com/islandora-devops/islandora-sandbox codebase
  • rm codebase/composer.lock
  • cp sample.env .env
  • Set ENVIRONMENT=local in .env
  • Set TAG=1.0.0-alpha-9 in .env
  • make local
  • docker-compose exec drupal with-contenv bash -lc 'drush -y en facets name islandora_search'
  • docker-compose exec drupal with-contenv bash -lc 'drush -y fim controlled_access_terms_defaults'
  • docker-compose exec drupal with-contenv bash -lc 'drush -y fim islandora_defaults'
  • docker-compose exec drupal with-contenv bash -lc 'drush -y fim islandora_search' (it will complain about not being able to reach the solr server. that's fine. the next step fixes it)
  • make hydrate

Then I ingested some content and searched for it to confirm the islandora_search search box is working.

After that I pulled in this PR, enabled islandora_advanced_search, placed the block, and configured it to look for the title field. Then I disabled the other two search blocks. After that, searching based on title is giving me no results.

@nigelgbanks
Copy link
Member Author

Followed the steps and figured out was wrong in the configuration, though there is something else messed up not related to the changes, in that saving any block manually as a user causes it to disappear, so not sure whats up with that. The setting that was causing problems is on the view. The views search block has a setting that says input is required, if that is set to Input Required it will prevent a search from occurring unless that block is populated (note this block is different from the advanced search block).

basic-input

I'm push up another commit that documents this configuration requirement.

@nigelgbanks
Copy link
Member Author

nigelgbanks commented Jul 27, 2021

@dannylamb could you review again and change that setting to confirm everything is hunky dory. Thanks!

@dannylamb
Copy link

dannylamb commented Jul 28, 2021 via email

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.

2 participants