Support multiple search environments for the same AN subscription

I just tried this and the clone button allows you to clone the environment. This means that as a site builder you can clone the Acquia search environment and do whatever you want with this new environment.

The only problem so far is that you can modify the URL, but maybe that is not even a problem

This means that as a site builder you can clone the Acquia search environment and do whatever you want with this new environment

Yes, you can clone it, however you do not get the Acquia Search specific settings (i.e. the choice to use the eDismax handler) because the logic is hard coded to the original "acquia_search_server_1" environment ID. It is also a non-intuitive step to force our customers to do if they want to add another environment that connects to their Acquia Search index.

The attached patch allows you to create new environments that connect to your Acquia Search account. I added a checkbox that says "Connect to Acquia Search" to simply the process. If we do end up supporting connecting to multiple indexes via the GUI, then the URL can be replaced with a list of indexes you can connect to or something.

Removing my local testing hacks :-(

Marking as needs work, because when you create a new environment the description defaults to "Acquia Search" no matter what is entered.

Resolved issue with not being able to change the description for the new environments connecting to Acquia Search.

Changing to needs-review...

+++ b/acquia_search/acquia_search.moduleundefined
@@ -38,7 +36,7 @@ function acquia_search_init() {
   if (arg(0) == 'admin' && arg(1) == 'config' && arg(2) == 'search' && arg(3) == 'apachesolr'
       && user_access('administer search')
       && acquia_agent_subscription_is_active()
-      && variable_get('apachesolr_default_environment') == ACQUIA_SEARCH_ENVIRONMENT_ID
+      && acquia_search_get_environment_connected(variable_get('apachesolr_default_environment'))

Can't we make this check less cryptic?
Or at least cleaner?

+++ b/acquia_search/acquia_search.moduleundefined
@@ -51,7 +49,10 @@ function acquia_search_init() {
+    $environment['name'] = t('Acquia Search');

Would be good if the Environment name is not changed, but altered. For example : Environment 1 (Powered by Acquia Search)

+++ b/acquia_search/acquia_search.moduleundefined
@@ -95,6 +96,37 @@ function acquia_search_cron() {
+ * Conveniance function that flags the environment as being connected to the

Convenience instead of conveniance?

Also I'd like to know if this makes it possible to connect to two different subscriptions? I would think not, since the subscription id's are kept in the connector.

Can't we make this check less cryptic?

I'm always up for better readability of code. What do you suggest?

Would be good if the Environment name is not changed, but altered. For example : Environment 1 (Powered by Acquia Search)

I'm not sure I agree. I'm not sure I disagree, either :-). As a site builder I absolutely hate when things that I enter via the GUI magically get modified. However, when viewing the list of environments I see how it would be helpful to show which ones are connected to Acquia Search. Note that the default behavior forces your Acquia Search environment to have the description of "Acquia Search". This patch maintains that functionality for the environment created by the module, but allows custom environments added through the GUI to have custom descriptions.

Convenience instead of conveniance?

My wife is a first grade teacher. It is a known fact that her students are more competent spellers than I.

Also I'd like to know if this makes it possible to connect to two different subscriptions?

To me this is a precursor to that functionality. In order to connect to multiple subscriptions, you have to support connecting to multiple Acquia Search connected environments first. To me that is a separate issue, though, and we should get this one working first before tackling that one. I do agree that the multiple subscription problem is bordering on critical based on how people are looking to use Acquia Search, and I would love to help tackle this as well.

arg(0) == 'admin' && arg(1) == 'config' && arg(2) == 'search' && arg(3) == 'apachesolr'

This can probably be 1 check instead of 4 different ones.

Most of the other ones can be checked individually and return false or exit the function?

What about some kind of color/visual view that it is connected with Acquia Search? I guess you could also add the Acquia subscription ID to the environment, but that is not future proof since we want multiple indexes per subscription ID soon.

The default behavior currently also disables your current default search index, even though you added another environment and you do not want the acquia search be the default, did you disable that? Maybe I missed that. What I actually envision is a question to the user that asks : Do you want Acquia Search to be your new search environment?

Thanks for the work!

Most of the other ones can be checked individually and return false or exit the function?

I'm cool with this, but at the same time that might best be done in a separate issue. My patch didn't introduce that, so I don't want to impose any collatoral damage as a result of unnecessary changes.

What about some kind of color/visual view that it is connected with Acquia Search?

Maybe a little Acquia Search icon in front or behind the environment description? That would be fun :-) I wonder what the UX people would suggest here?

Regarding the default behavior, I tried not to disturb that at all, and I am pretty sure I accomplished that. So it should work exactly as it does now in terms of when you enable the module. The only difference is that you can now add environments that connect to Acquia Search. When you add those environments manually, it works like any other environment in that you have to set it as the "default" manually if you want this behavior.

So just to spell this out more clearly, when you enable the module it will go through it's normal process of connecting to Acquia Serch, making it the default environment, etc. With the patch, you can now simply add more environments that might or might not connect to Acquia Search. Doing so will not set these environments as the default automatically, just like if you were adding non Acquia Search environments.

Created a complete different approach to this problem

Solves : Cloneability + allows you to connect to a different acquia network subscription if you would like to.
It also takes care about the naming issue. Disclaimer : I've done this on a train trip so don't judge me on the quality just yet, I did not have internet connection!

You are allowd to delete a cloned acquia environment, you are still not allowed to alter the url. You can alter the sub and key id per acquia search environment.

Possible todos is a little checkbox that asks if this should be an acquia search sub or not, might be easier than checking the environment on a certain name syntax, similar to cpliakas's patch

I should probably merge this with Chris Pliakas's patch about the edimax per environment.

attaching a revised patch with a small correction

You can alter the sub and key id per acquia search environment.

That's awesome. So if you have multiple subs this makes it possible to connect to them via the UI?

Possible todos is a little checkbox that asks if this should be an acquia search sub or not

+1. The goal of my patch was to make it trivial to connect to Acquia Search without having to look up information. I hope we can find a way to maintain that one piece.

So if you have multiple subs this makes it possible to connect to them via the UI?

Easily, see the following screenshots (sorry for the ridiculous names)

https://skitch.com/nick_vh/er2dh/drupal-nl-irc.freenode.net
https://skitch.com/nick_vh/er2r8/search-and-metadata-smartphone-compare
https://skitch.com/nick_vh/er2rm/search-and-metadata-smartphone-compare
https://skitch.com/nick_vh/er2rx/apache-solr-search-smartphone-compare

I don't think it is intrusive or complex at all, the only trick to keep it an acquia subscription is that you have to clone it from the sub that was originally added.

Comments welcome

Patch included with small namespacing fix

so, I finally merged the two patches. Cpliakas, I had to remove a bunch of your custom variable settings and I moved it to the generic saving mechanism for environments.
This means that the db is a bit less bloated and or custom :-)

Also fixed some of the flaws that were pending in this patch. Please test it and report back.

changing version

when the checkbox is marked :

https://skitch.com/nick_vh/er9a2/apache-solr-search-smartphone-compare

when the checkbox is unmarked :

https://skitch.com/nick_vh/er9ah/apache-solr-search-smartphone-compare

when the module is disabled it will remove the added environment and will unset any acquia specific configuration in the remaining environments.

improved the description for the custom connection a bit and added a link to the Acquia Agent

First, great work! Exciting feature.

On a clean install, the first time I visit the search page I get an "Undefined index" error since the $environment['conf']['acquia_search'] is not yet set in acquia_search_form_apachesolr_environment_edit_form_alter(). This error is also thrown when you apply the patch, so it will happen when users upgrade the module.

From a UX perspective, my opinion is that the fieldset text should be changed to something like "Connect using another subscription" or something similar, as "Configure Acquia Search" implies that you have more configuring to do. You also have to read the help text inside of the fieldset to know that these fields are not required unless you are connecting via another subscription. Based on various usability studies, we know that in general Drupal users don't always read the help text :-).

Also, I still think that the checkbox should be at the top of the form. If the Acquia Search module is installed, the site is most likely using Acquia Search. People generally start configuring forms from the top down and rarely scan through the whole thing prior to filling out the fields, so it isn't apparent there is an "easy connect" checkbox until after they have configured the difficult parts.

Not critical, but it would also be cool if people could enter their email address / password to get a list of subscriptions similar to when you first configure the Acquia Connector. That might be a little more complex to do so might be a good follow-up issue.

Thanks,
Chris

+++ b/acquia_search/acquia_search.module
@@ -35,11 +41,10 @@ function acquia_search_enable() {
+  if (acquia_search_get_environment_connected($environment) && user_access('administer search')
+      && acquia_agent_subscription_is_active() && empty($_POST)) {

Minor, but better to check access first, so as to fail quickly if access is denied.

+++ b/acquia_search/acquia_search.module
@@ -65,11 +73,32 @@ function acquia_search_enable_acquia_solr_environment() {
+      $environment['url'] = 'http://localhost:8983/solr';

Is the default localhost?

+++ b/acquia_search/acquia_search.module
@@ -65,11 +73,32 @@ function acquia_search_enable_acquia_solr_environment() {
+  // Leave custom configurations alone! ref: http://www.youtube.com/watch?v=kHmvkRoEowc

Funny, but maybe leave it out?

Also, after talking briefly to pwolanin, it looks like the derived key might be an issue. Different subs might have different salts, however the _acquia_search_derived_key() function assumes that the same salt is being used by the default subscription. Therefore there might be a derived key mismatch here causing authentication failures.

Instead of moving forward with this in a few different directions, it might be worth taking a step back and re-thinking the low-level API functions that are required. As part of the work being done for Acquia Search for Search API, I had to duplicate a lot of the authenticator / derived key logic as to not require a deep rearchitecture of this module.

Instead of patching what we have, it might be worth removing the single credential assumptions of the low level stuff and moving it to a library or set of functions that could be leveraged by Acquia Search or Acquia Search for Search API. We have an opportunity to take the API-based approach first and then at least have the ability to build a UI on top of it later.

I really hope we don't commit a solution that isn't at least open to the possibility of a UI on top of it. Although this is an edge case now, it is in a small part due to the fact that it is very difficult to do unless your Drupal ninjutsu is well established.

I'm not in favor of rethinking this massively. Don't re-architecture if it is not needed at the moment, and also not because we can. If you want to do this, it should be in a sandbox and not in this issue. Do you really think Acquia Search for Apachesolr + Acquia Search Api is a good module combo? I'd keep them together. It would probably not be meaningful enough and only makes the clutter bigger if you start generalizing that out.

Also, I really like the small UI for connection to another subscription, since that makes managing multisites so much easier. However, I'd be ok with removing this, I could easily make that a small custom add-on for the sites that need it.

So, can we focus and not drift off to a general API discussion in this thread?

In regards of the derived key, it is a generated one, so I'd be ok with storing the derived key and using that along the flow but the id and key combo is way easier to use. In UI and in code.

Goal for this patch :
make it easy to clone an acquia search sub
Allow multiple acquia environments
Allow somehow connections to another subscription using an ID/key and/or derived key combo, imho a key/id combo is easier for external developers to understand.

Thanks for all the input :-)

Don't re-architecture if it is not needed at the moment

Not re-architect. Refactor based on existing customer need and pain points. The fact that there are (at least) three versions of code that establish connections to a second subscription highlights a need for this.

Do you really think Acquia Search for Apachesolr + Acquia Search Api is a good module combo?

I'm not sure we are on the same page here. All I am saying is that we have an opportunity to consolidate and reuse code. If that is not in the cards, then we can have two separate lines of development. That is not necessarily a bad thing, but when I see an opportunity to align I feel it is necessary and productive to at least bring it up.

So, can we focus and not drift off to a general API discussion in this thread?

Sometimes staying the course is a good idea, other times it is not. The only way to know is to discuss :-)

Regarding the goals, it might be useful to highlight the target audience. For example, is this for power users, non-technical users, others? I have a specific persona that I am interested in, which is why I am focusing on certain areas that might not align with your target audience.

Thanks,
Chris

I don't feel familiar enough with Acquia Search or the code to mark this RTBC yet but I can say the patch applied fine and I haven't seen any errors.

Here's what I did:

* Installed apachesolr, applied this patch, and enabled the modules
* Once the service was ready I cloned the Acquia Search environment and made it default
* the search/site page was set to used the cloned environment, and a search on that page came back with results

Regarding a re-architect/refactor of the connection code, I agree with Chris but unless you all are willing to wait on this feature for that I'm ok getting this in and coming back to a refactor in a separate issue.

I am all for pushing ahead to get the feature in and refactoring later.

I do think the patch in #1190890-29: Support multiple search environments for the same AN subscription works and provides the API-like functionality (for Apache Solr Search Integration at least) to connect to multiple subscriptions. It cannot be reused by the Search API integration module, but we can tackle that later. The original intent of this issue was to have multiple environments connect to the same subscription, but somewhere along the line it evolved to supporting multiple subscriptions as well which is the only reason why I brought it up.

I still don't like that we took the UI completely out of the equation, and I also don't like that you have to clone an environment and cannot use the "Add new environment" link like you could in #1190890-5: Support multiple search environments for the same AN subscription. From a UX perspective I really liked the one click "Connect to Acquia Search" checkbox. I think it simplified the process since the "Add new environment" link is probably the first thing users will click on since "clone" is hidden way on the right of the page in a slop of other links.

In addition, I don't like that you have to know your second subscription's derived key and add it via drush through an environment variable (or some other method) to connect it to Acquia Search. It is not trivial to get the derived key, and having to use drush to set it seems unnecessarily complex. I have made my point of view known, so I am not going to continue arguing it. If the decision is made that this is fine, I'll drop it for now.

With all that being said, the original goal of this issue is accomplished by cloning the environment. It also has the added benefit of connecting to another subscription if you know how to get second one's derived key, which is a huge step forward over the current code. I know I sound cranky, but I do appreciate the efforts and advancements made in #1190890-29: Support multiple search environments for the same AN subscription. Marking this as RTBC since I did test the cloning functionality and connecting to multiple subscriptions. I also think it is flexible enough to add a UI on top of going forward.

Thanks,
Chris

+++ b/acquia_search/acquia_search.moduleundefined
@@ -32,16 +38,18 @@ function acquia_search_enable() {
+      $env_id = $arg[5] ? $arg[5] : variable_get('apachesolr_default_environment');

missing default value, should always be there Also problematic when porting to the D6 version.

Actually, it should be apachesolr_default_environment() instead of the variable_get

follow-up patch

committed to 7.x-2.x