Download & Extend Landing Page: Filter by version

Moving issue to the Redesign project. Check to see who has been assigned to implement this section and the staging server it'll live on during implementation here: http://groups.drupal.org/node/37064

I'm removing the Blue Cheese tag, as this isn't really a theme-related issue.

This is in the most recent comps. Unless there some objection, I see this sending the user to the full results page with the version filter applied. This seems like the best user experience to me, rather than refreshing the main D&E page.

mdorman, no objection from me !

This is something we're going to have to implement in drupalorg_search.module.

Or not? Maybe this can be done in project_solr itself, since project_solr knows about project_release and the "API compatibility stuff" itself....

No, sorry, I was confused. This definitely needs to be in drupalorg_search. ;)

But, tagging for sprint #2...

Ok, this has 2 patches to create the functionality.

project_solr

This first is more of a minor bug fix. Since we're taking values of im_vid_# and turning them into subqueries, some elements (and in this case the relevant element - project_solr_categories block) has trouble identifying the sort. The sort still works correctly with regards to solr, but our match in project_solr on strings starting with im_vid_3 causes it to break. To fix this issue, I made our subquery creation more strict (i.e. on anything other than im_vid_3). Technically, it might be even more correct to only have it change im_vid values to subqueries for vocabularies that are related to project type terms, but it would be more work. If that is preferable - since it is more correct - I can rewrite it to handle that.

drupalorg_search

Utilizes the filter values from the url to create the query. Recognizes the api_version alias and populates the value in the url. This required 2 new functions (the form + the form submit) and a rewrite of the base_query function to recognize the filters, call prepare_query so that the alias is recognized, and insert the static filter params in a different way so that they aren't clobbered by the filterstring.

Great, start, thanks! A few issues:

project_solr

A) Stray whitespace added here:

-    // Grab any meta-type specific facet (im_vid_X facets).
+    // Grab any meta-type specific facet (im_vid_X facets).    

B) Otherwise, I don't fully understand how your comment here matches the comment in the code. Basically, I don't really grok the bug you're fixing here, and neither issue comment #10 nor the code comment in the patch helps me understand. ;) Can you clarify?

drupalorg_search

C) Since this is drupalorg-specific code, there's no need to check for module_exists('project_release'). We can just add a dependency on project_release to the drupalorg_search.info.

D) Similarly, we might as well just call the form element "drupal_core" in here.

E) Likewise, we might as well just have the form label say "Show only modules for Drupal version" to match the comp at https://infrastructure.drupal.org/drupal.org-style-guide/prototype/downl...

F) This looks like cut+paste from another spot:

+  if (module_exists('project_release')) {
+    $terms = array();
+    $active_terms = project_release_compatibility_list();
+    foreach ($active_terms as $tid => $term_name) {
+      $active = $query->has_filter('im_project_release_api_tids', $tid);
+      if ($active) {
+        $current_tid = $tid;
+      }
+      $terms[$tid] = $term_name;
+    }
+    if (!empty($terms)) {
+      $terms = array('' => t('- Any -')) + $terms;
+      $form['api_version'] = array(
+        '#title' => t('Filter by compatibility'),
+        '#type' => 'select',
+        '#options' => $terms,
+        '#default_value' => $current_tid,
+      );
+    }
+  }

Should that be a helper function or something in project_solr?

G) Looks like the above would throw a PHP with $current_tid undefined in some cases.

Ok, project_solr patch done the right way was easier than anticipated. It's like we knew we were going to need this information and smartly made a utility function to retrieve it...way to go us.

New project_solr and drupalorg patches.
Only thing I didn't change was D because I think the consistency of having the same default feels preferable. If, down the road, we decide to call it something else, or fall back to the project terminology, I'd like things to stay consistent. If you feel strongly, though, I'll change it.

This also includes a bunch of changes to improve the readability of the project_solr aspect of the code.

1 more small change

Changing api_version to drupal_core for the field element naming.

I realize now that the project_solr hunk here is really just cleaning up stuff from #425986: Convert project browsing facet blocks to a navigation form, so it's a bit of scope creep, but I'm glad we're taking the time to make this code understandable. Patch #14 is a LOT better. That said:

H) It'd be nice if the PHPDoc block for project_solr_apachesolr_modify_query() gave a high-level overview of what this function is doing and why.

I) This code comment at the end of project_solr_apachesolr_modify_query() is still using the misleading terminology:

   // Add the meta-type specific facets back as an OR list.

I'm glad to see #15 to address (D). I was talking about the form element, which in turn controls the GET param for the version filter. To have the d.o URLs consistent, seems better to use 'drupal_core' for this form element. Thanks! #15 is also looking a lot better. But a few more minor things in there:

J) This comment doesn't make much sense to me:

+    // Additional criterias are set below rather than via $params
+    // because any params passed to the constructor will override
+    // our $params set filter values.

K) It'd be nice to add a comment about the implicit filters here:

+  // We add our fields after the prepare_query because prepare_query
+  // generates a call to parse_filters which destroys anything that
+  // does not get passed into the query on construction.
+  $query->add_filter('type', 'project_project');
+  $query->add_filter('im_vid_' . _project_get_vid(), DRUPALORG_MODULE_TID);

I get it that we have to add this ourselves after prepare_query(), but it's not obvious why we're adding these. It's because these particular filters are implicit from the URL, and don't appear anywhere in the GET params.

Also, maybe we're just being safe here, but what's the value of a filter on type == 'project_project' if we're already filtering on the module tid? Core taxonomy already enforces that anything with that tid must be a project node, no? Not necessarily worth changing in this patch, but just wondering out loud...

L) The core code standard is that PHPDoc blocks should have 1-line summary sentences, a newline, and then the detailed explanation. So these two could be edited to have a summary:

+/**
+ * Generate a form with a version selection to allow filtering page content
+ * based on the drupal version. Also includes path to allow the form to 
+ * potentially submit to other urls if desired.
+ */

+/**
+ * Utilizing the existing query, remove the version filter if it exists,
+ * add any new version filtering if it exists, and direct back to the relevant
+ * page with the appropriate filter string.
+ */

M) It's not 100% obvious why we care about passing in path as a form value. It's not clear how having path in the form like this actually facilitates submitting to other pages. Seems wonky to have to form_alter() it just for this. Can you explain how this would work with a code comment, or just simplify the patch or something?

N) Please end the file with a newline. ;)

Also, you didn't address (F) or (G) yet...

Thanks!
-Derek

I'll post fixes tomorrow. Quick feedback below.

F) Correct. Haven't addressed.
G) Define and gave a default value to $current_tid
H) Ok.
I) Right. Missed that one.
J) I'll try and write it so that it is more clear.
K) Same.
L) I'll try and add a summary tomorrow.
M) This was just so that this element could potentially be used elsewhere. If we don't have the path in the form, then we have to hard-code the path to go to download. If we're not using this elsewhere, potentially we don't care; in that case, I'll simply hard-code the value to download.
N) Ok.

Re: (M): my point is that we're still hard-coding it in the form builder. So, you have to form_alter() this to reuse it. What about passing that value in as an optional param to the form builder? Then it seems easy to re-use without hacks.

Thanks!
-Derek

Containing all of the cleanup (I believe) from above.
I want to get these out before my meeting, so if I missed anything, just note it, and I'll take care of it after that.
Thanks!
--Bill

Getting very close now. I re-rolled these for a few final things (since it seemed easier/faster to just fix them than to explain them, wait for a re-roll, re-re-review, etc). New patches (and interdiffs from #19) attached. Highlights:

- The helper function from (F) is now called project_solr_get_api_version_field() and it just returns a FAPI array. Instead of having to pass the $form and the array key to use as params, it's now the caller's responsibility to use the subarray as they see fit. I think this is a lot cleaner and simpler.

- The way you were handling t() for the form label was a little off, since t($title) isn't a good move. If a caller wants a different value, they pass in a t()'ed string as a param. If the param isn't defined, the default is a t() with a string literal.

- Fixed some typos in various code comments.

- Fixed formatting of various comments (linewrap at 80 chars, no trailing whitespace, function names have () after them to make it obvious it's a function, etc).

- Fixed the PHPDoc comment for drupalorg_search_version_form_submit() so it actually matches what the code is now doing. ;) We're *not* removing version filters anymore, since you're just starting from a new base query.

I haven't tested at all. But, once this is tested, I'm ready to commit and deploy. I should probably commit, since I'm going to need to do that for project_solr, so I might as well do it for drupalorg, too.

Thanks for all your work on this, Bill!
-Derek

I've tested and everything seems to work as expected.
I found 1 issue, but it has been around awhile (related to the add_filter() vs fq changes I made in drupalorg_search), so I added those corrections to project_solr in the following patch. I'm just reposting the drupalorg_search patch because I didn't make any changes in there and I don't want it to get left behind. Thank you for just making some of the small (text esp) changes. It helps a lot.
I definitely prefer the way you implemented the changes from F, so that's great.
Thanks again.
--Bill

Did a final review on #21. Looked fine to me. Committed to HEAD of project + drupalorg, merged into bzr, and deployed on both d.o and r.d.o. Working fine:

http://drupal.org/project/modules
http://redesign.drupal.org/download

Whoot! Great team work on this patch...

Cheers,
-Derek