Problem/Motivation

When searching issues on drupal.org, you are first presented with a "regular" search form (/project/issues) with filters:

  • Search for (keyword)
  • Project
  • Status
  • Priority
  • Category

If you first do a search there, perhaps typing in the keywords "advanced search" and then click the "Search" button, you will get search results.

If then you decide you really would prefer to use the "Advanced search" form, you click the "Advanced search" link and the filters you had entered before disappear which is bad UX. The user has to add the filters again. Obviously, they probably want some different filters (which is why they chose "Advanced search" in the first place), but it's likely that at minimum the keywords and project should be preserved.

Steps to reproduce

  1. Go to drupal.org/project/issues
  2. Type in "advanced search" (or any text you would like) into the "Search for" text box
  3. Click the "Search" button
  4. Click the "Advanced search" link
  5. Note that the "Search for" text box is empty

Proposed resolution

Figure out the mapping between the regular and the advanced search form filter values and potentially tack on the query params to the "Advanced search" link.

For example, regular search params:

?text=advanced+search&projects=Drupal.org+customizations&status=Open&priorities=200&categories=1

Would map to advanced search params:

?text=advanced+search&projects=Drupal.org+customizations&assigned=&submitted=&project_issue_followers=&status[]=Open&priorities[]=200&categories[]=1&issue_tags_op=%3D&issue_tags=

Note that there might be some gotchas for project-specific forms so make sure to test some specific projects.

Remaining tasks

  • Get consensus on this improvement
  • Create patch
  • Review patch
  • Commit patch :)
  • Deploy

User interface changes

Advanced search filters would be pre-filled with values if the regular search was already performed and they went to the advanced search from the regular search.

API changes

Adds a new alter hook, hook_project_issue_advanced_search_parameters_alter()
Adds a new internal helper method: _project_issue_convert_search_query()

CommentFileSizeAuthor
#19 3161134.16_17.interdiff.txt1.24 KBdww
#19 3161134-17.patch4.4 KBdww
#16 3161134-16.patch4.87 KBdww
#13 drupal-basic-search-open.png114.76 KBKristen Pol
#13 drupal-advanced-search-any-error.png215.75 KBKristen Pol
#13 drupal-basic-search-any.png118.06 KBKristen Pol
#13 drupal-advanced-search-open.png186.82 KBKristen Pol
#10 3161134.9_10.interdiff.txt958 bytesdww
#10 3161134-10.patch5.13 KBdww
#9 3161134-9.patch4.13 KBdww
#8 3161134-8.patch4.13 KBdww
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Kristen Pol created an issue. See original summary.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Issue summary: View changes
Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Issue summary: View changes
dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

@Kristen Pol pinged me on Slack about this. Pasting some notes from our convo here for posterity:

Here's the root of your problem in 3 URLs:
https://www.drupal.org/project/issues/project_issue?priorities=400
https://www.drupal.org/project/issues/search/project_issue?priorities=400
https://www.drupal.org/project/issues/search/project_issue?priorities[]=400

- - -

It's also a rat's nest since sites (like d.o) can/do have different filters than the default views that project_issue provides. :(
So it's hard to do this generally right in project_issue.
But that's what's providing the adv search links. :(

- - -

project_issue_query_result_links() is where the adv search link is created.

Which is invoked by project_issue/views/handlers/project_issue_handler_area_issue_search_links.inc which is what places them in the view.

- - -

Reading the summary, I'd also point out there are two cases... the site-wide search vs. advance search mentioned in the summary, but also the per-project versions of these, e.g. the links I'm pasting here. This 2nd case already correctly preserves the project, so that's a little more simple. But the per-project views have other fields not found sitewide (version, component, etc).

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented

Thanks @dww!

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm at Drupal Association commented

We should only need to forward the values for the filters on the basic issue search page. On Drupal.org, those are

  • Search for (text)
  • Status
  • Priority
  • Category
  • Version (optional, some projects do not have releases)
  • Component

I’m hoping we haven’t managed to get any of those diverged from project_issue module. If they haven’t, or we correct that, then this could go in project_issue itself. That would be much better from a maintenance perspective.

I’ve always wanted to try merging the advanced issue search pages into one. I haven’t seen a good solution for keeping the exposed filters less-cluttered, while keeping them easy to use.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Project: Drupal.org customizations » Project issue tracking
Version: 7.x-3.x-dev » 7.x-2.x-dev
Component: Code » Views integration

Okay, sounds good. I'm down to try to do this in project_issue, and only handle the filters provided with the default views. That'll certainly be easier, since that's what's generating the links in the first place.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Active » Needs review
FileSize
3161134-8.patch4.13 KB

Not the prettiest code ever, but it gets the job done. ;)

Site-wide

Regular search:
https://project-drupal.dev.devdrupal.org/project/issues?text=value&proje...

Contains advanced link:
https://project-drupal.dev.devdrupal.org/project/issues/search?text=valu...

Project-specific

Regular search:
https://project-drupal.dev.devdrupal.org/project/issues/drupal?text=filt...

Contains advanced link:
https://project-drupal.dev.devdrupal.org/project/issues/search/drupal?te...

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
FileSize
3161134-9.patch4.13 KB

Sorry, juggling too many directories. ;) Here's the actual working patch I meant to attach...

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
FileSize
3161134-10.patch5.13 KB
3161134.9_10.interdiff.txt958 bytes

Documenting the new alter hook, too...

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Hah, whoops. I just noticed the last link in #8 is getting the issue count when it shouldn't. ;)

Opened #3162080: Make sure issue-count.js is only kicking in on links to the current site for that.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Open questions (referenced in the @todo comment):

Should we actually keep inserting the 'Advanced search' link when we're already on the advanced search page itself?

Seems pretty weird. I'm not totally clear why we still do this. I know it was like this ages ago when I first started maintaining this whole mess, and we kept it working like that to "preserve BC" (sort of) ever since. Once upon a time, before the era of the 'Reset' button, I would at times use it as a reset link to clear a complicated search. But 'Reset' is a way better solution for that. Is there any reason to keep it?

Should we kill it here or propose that in a separate issue? Who exactly would need to sign off on such a change?

Thanks,
-Derek

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Issue summary: View changes
FileSize
drupal-advanced-search-open.png186.82 KB
drupal-basic-search-any.png118.06 KB
drupal-advanced-search-any-error.png215.75 KB
drupal-basic-search-open.png114.76 KB

Thanks @dww! I haven't looked at the code yet but have tested pretty thoroughly.

I noticed a couple things so I'm adding some screenshots:

1) The basic search defaults to - Open issues - yet when you do the advanced search, - Open issues - isn't selected. This behavior is the same if I switch away from - Open issues - to something else and then back to - Open issues -.

2) If I switch to - Any - in the basic search and then go to the advanced search, I get an error: An illegal choice has been detected. Please contact the site administrator.

As for your questions from #12:

Should we actually keep inserting the 'Advanced search' link when we're already on the advanced search page itself?

I think this is confusing and should be removed.

Should we kill it here or propose that in a separate issue? Who exactly would need to sign off on such a change?

I would include it in this issue. I don't know about the sign off but the user is not losing any functionality. It's just less confusing, IMO.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Needs work

Whoops!

UPDATE: I quickly reviewed the code and didn't see anything glaring other than it'll obviously need to change if the advanced search link is removed from the advanced search page.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm at Drupal Association commented

Should we actually keep inserting the 'Advanced search' link when we're already on the advanced search page itself?

Seems pretty weird. I'm not totally clear why we still do this. I know it was like this ages ago when I first started maintaining this whole mess, and we kept it working like that to "preserve BC" (sort of) ever since. Once upon a time, before the era of the 'Reset' button, I would at times use it as a reset link to clear a complicated search. But 'Reset' is a way better solution for that. Is there any reason to keep it?

Should we kill it here or propose that in a separate issue? Who exactly would need to sign off on such a change?

I don’t remember why we would have that at all. I can see it being used as a reset, I suspect that’s not common. I think go ahead and commit removing it, either here or a separate issue, and we’ll see if anyone misses it.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
FileSize
3161134-16.patch4.87 KB

Thanks for the reviews and testing!

Spun off #3162278: Remove 'Advanced search' link when you're already on advanced search for that part. Definitely cleans this up considerably to do that, first.

Re: #13.1: That's all by design. Regular search defaults to only 'Open' issues, whereas advanced defaults to showing everything. We don't want to change that here, so we shouldn't change that default behavior if you haven't touched the status.

Re: #13.2: Good catch. I'll fix that next.

To start, here's a re-roll/re-build after #3162278-2.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Issue summary: View changes
Status: Needs work » Needs review

Fixes #13.2: We always want to ignore any regular search filter set to 'All' when we land on advanced search.

Also removes a bogus hunk that leaked into #16.

Now deployed on https://project-drupal.dev.devdrupal.org if anyone wants to re-test.

E.g. #13.2:

https://project-drupal.dev.devdrupal.org/project/issues/drupal?text=filt...

sends you to:

https://project-drupal.dev.devdrupal.org/project/issues/search/drupal?te...

(which no longer has an 'Advanced search' link, thanks to #3162278: Remove 'Advanced search' link when you're already on advanced search ;)

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Needs work

Woot! I think it's still a bit odd that if I actively choose - Open issues - by choosing something else and searching and then choosing it again, that it doesn't default to - Open issues - on the advanced form but I can live without that based on the reasoning above. :)

I tested all the filters again and they are otherwise working great.

I was going to mark RTBC, but not sure why this got commented out so marking back to needs work in case that needs fixing.

+++ b/project_issue.module
@@ -1165,7 +1165,7 @@ function _project_issue_filter_tips($delta, $format, $long = FALSE) {
-  $regex = '(?:(?<!\w)\[#\d+(?:-[\d\.]+)?(@)?\](?!\w))|<pre>.*?<\/pre>|<code>.*?<\/code>|<a(?:[^>"\']|"[^"]*"|\'[^\']*\')*>.*?<\/a>';
+  //$regex = '(?:(?<!\w)\[#\d+(?:-[\d\.]+)?(@)?\](?!\w))|<pre>.*?<\/pre>|<code>.*?<\/code>|<a(?:[^>"\']|"[^"]*"|\'[^\']*\')*>.*?<\/a>';
dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs work » Needs review
FileSize
3161134-17.patch4.4 KB
3161134.16_17.interdiff.txt1.24 KB

Ugh! Forgot to attach the patch to #17. ;) That hunk you commented on is what I meant by: "Also removes a bogus hunk that leaked into #16." But you wouldn't know that, since I completely failed to upload the fix. ;)

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Reviewed & tested by the community

Interdiff looks good, thanks!

  • dww committed 8a8918a on 7.x-2.x 
    Issue #3161134 by dww, Kristen Pol: Preserve issue search form filters...
dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Assigned: Unassigned » drumm
Issue tags: +Needs deployment

Great, thanks again for reviews and testing.
Pushed to 7.x-2.x.
Tagging for Neil to deploy.

Yay,
-Derek

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm at Drupal Association commented
Assigned: drumm » Unassigned
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs deployment

This is now deployed.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented

Woot! Thanks @dww and @drumm!

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Sweet, yay. ;)

Thanks, @Kristen Pol for suggesting this!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.