Some changes

[bbehrens]

I added some unit tests to the area that was of particular interest to me. (I will happily add them all over the library, but only had time to do a few) I added some settings objects (it makes it a lot easier for us to use with our application) I added the ability for the library to be multi tenant aware. (Multiple sets of data for different parties in the same search domain)

I think there are a few other things. There weren't many tests in there for me to see if I had broken any of your existing functionality. I made every effort not to. We plan to do a lot of development on this product so if it's not to your liking, let me know.

Finally, we need to be able to access the library via a nuget feed. I'm happy to set everything up to get it published, but I did not want to publish something named amazing cloud search without asking you about it first.

Feel free to contact me with any questions or comments.

[martin-magakian]

Hi guys. It look like a big piece of work. Thanks for your help!

I started reading the code it look good to me. I just need to run it now. That's cool for the unit test. If you can take time to add them it would be perfect.

The only point is about documentation. Can you update the doc and add how to use your new functionalities ? I really believe a documentation using exemples is the key to give an overview and how to do it. Doing so lead to more user so more contribution.

Martin

[bbehrens]

For sure. I'll do that now

On Wed, May 29, 2013 at 9:57 PM, Martin Magakian notifications@github.comwrote:

Hi guys. It look like a big piece of work. Thanks for your help!

I started reading the code it look good to me. I just need to run it now. That's cool for the unit test. If you can take time to add them it would be perfect.

The only point is about documentation. Can you update the doc and add how to use your new functionalities ? I really believe a documentation using exemples is the key to give an overview and how to do it. Doing so lead to more user so more contribution.

Martin

— Reply to this email directly or view it on GitHubhttps://github.com/martin-magakian/Amazing-Cloud-Search/pull/1#issuecomment-18658911 .

Brandon Behrens 512.659.7171 (c)

[martin-magakian]

ho and I forgot to reply to your other question about the nuget package. Sure, it's a good idea. Go for it but can I get access to it as well for the following release ?

[bbehrens]

Of course. If you tell me your nuget.org username I'll make you an owner.

On Thu, May 30, 2013 at 10:19 AM, Martin Magakian notifications@github.comwrote:

ho and I forgot to reply to your other question about the nuget package. Sure, it's a good idea. Go for it but can I get access to it as well for the following release ?

— Reply to this email directly or view it on GitHubhttps://github.com/martin-magakian/Amazing-Cloud-Search/pull/1#issuecomment-18687230 .

Brandon Behrens 512.659.7171 (c)

[martin-magakian]

Thanks, my user name is martin-magakian on nuget.

[bbehrens]

Request is pending approval

On Thu, May 30, 2013 at 10:54 AM, Martin Magakian notifications@github.comwrote:

Thanks, my user name is martin-magakian on nuget.

— Reply to this email directly or view it on GitHubhttps://github.com/martin-magakian/Amazing-Cloud-Search/pull/1#issuecomment-18689622 .

Brandon Behrens 512.659.7171 (c)

[martin-magakian]

I just open your project, but I need visual studio 2012.

So I will continu my review on Monday or Thursday.

[martin-magakian]

Hi,

First good job. The unit test will help improving the library over time.

I have few feedback/question:

• Why creating class "MultiTenantCloudSearch" just to add a StringBooleanCondition for every search? I didn't try it yet but maybe we can add a param "PersistantBooleanCondition" in CloudSearch.cs who can do this job for us. Also PersistantBooleanCondition could be a "IBooleanCondition" so we can use it with all the "boolean conditions".

This solution sound more flexible to everyone.

I added the ability for the library to be multi tenant aware. (Multiple sets of data for different parties in the same search domain)

• I actually wonder if I understand correctly. You are speaking about something like a field "objectType" in the search domain? So when you process your search you just want to search within this field ?
• The Search() function of MultiTenant is calling SearchWithException() but just catch the exception and do the exact same thing as the Search() in CloudSearch. I don't get the point. Is it only for unit test ?

Thanks again, Martin Magakian

[martin-magakian]

Also ICloudSearchDocument now need to have a tenant, domain and text_relevance... Not everyone need that.

When added a document the only mandatory field was an id. Now every object need those column in database: tenant, domain, text_relevance

I think it's just a mistake and you didn't see it because you don't add data from the API. (And I didn't made unit test) I believe you just used those field to get the data from the WS, but it can't be in ICloudSearchDocument for the sake of being generic. You can fix it by moving those fields into your own object in order to get the result back from the WebService. (in the example the Movie class) The reflexion will take care of mapping your object to the result.

[bbehrens]

Thanks for looking at the changes so closely.

First, your comments about ICloudSearchDocument are on point. We needed that functionality and I put the properties on the incorrect object. I knew it at the time when I was doing it and forgot to circle back around and relocate them. I will move those.

Second, regarding the multitenant changes I made --

We have multiple customers we need to support on a single search domain. (We could use multiple search domains, but it's $100 / month per search domain) When one customer searches for data, it's a requirement that we don't ever return data for another client.

I added the MultiTenantCloudSearch class to handle restricting the data to a specified tenant so that the calling application does not have to concern itself with adding a boolean condition to restrict data by tenant.

The search in MultiTenantSearch first adds a tenant boolean condition to the search query and then calls the search method of CloudSearch. That's how I restrict the information I return by tenant. The try catch in MultiTenantCloudSearch is unecessary, since it's already there in the SearchWithException method.

I'll be putting a bunch of polish around our code and in our fork of AmazingCloudSearch over the next three weeks. Do you think it would be a good idea for us to have a skype conversation to make sure we are on the same page? I'd really like any additions / enhancements I made to benefit both you and the community.

On Tue, Jun 4, 2013 at 12:03 AM, Martin Magakian notifications@github.comwrote:

Also ICloudSearchDocument now need to have a tenant, domain and text_relevance... Not everyone need that.

When added a document the only mandatory field was an id. Now every object need those column in database: tenant, domain, text_relevance

I think it's just a mistake you didn't see because you don't add data from the API. I believe you just used those field to get the data from the WS, but it can't be in ICloudSearchDocument. You can fix it by moving those fields into your own object in order to get the result back from the WebService. (in the example the Movie class) The reflexion will take care of mapping your object to the result.

— Reply to this email directly or view it on GitHubhttps://github.com/martin-magakian/Amazing-Cloud-Search/pull/1#issuecomment-18889305 .

Brandon Behrens 512.659.7171 (c)

[martin-magakian]

Hi Gary, I didn't saw your commits. I will look at it soon.

Thanks.

[bbehrens]

We're right in the middle of a big push right now. As soon as that's over I'll have the time to do all the things we talked about. On Jul 1, 2013 4:19 AM, "Martin Magakian" notifications@github.com wrote:

any news ?

— Reply to this email directly or view it on GitHubhttps://github.com/martin-magakian/Amazing-Cloud-Search/pull/1#issuecomment-20271052 .

[martin-magakian]

Hi bbehrens, Hi chadmyers,

Sorry for the delay but I just accepted your pull request.

As discussed before:

1 -

ICloudSearchDocument now need to have a tenant, domain and text_relevance... Not everyone need that.

bbehrens: about ICloudSearchDocument are on point. We needed that functionality and I put the properties on the incorrect object.

So I removed the fields. You can move it in your own object.

2 -

MultiTenantCloudSearch no longer exist.

You can now add any kind of conditions using SearchCloud.AddPresistantCondition(IBooleanCondition condition) I believe it feel more generic like that.

Chadmyers: I also reviewed the 5MB limits. Nicly done !

---

Last thing I'm having trouble with this unit test "AmazingCloudSearch.Test.WhenSearching.ShouldCallSearchWithException" It look like I'm missing something with the injection. Can you have a look. And please explain me, I wonder !

[martin-magakian]

Also can you guys update the documentation about the "AddPresistantCondition" (maybe explain how to do the tenant idea with it ?) And maybe a "quick tip" about the 5 MB limit in the doc ?