[Ovirt-devel] [PATCH] includes basic search functionality.

[Jason Guiditta]

Ok, couple bits of feedback inline, but works well overall for me.

On Wed, 2008-07-30 at 18:36 -0400, Scott Seago wrote:

> +
> +  def results_json
> +    results_internal
> +
> +
> +    json_hash = {}
> +    json_hash[:page] = @page
> +    json_hash[:total] = @results.matches_estimated
> +    json_hash[:rows] = @results.results.collect do |result|
> +      item_hash = {}
> +      item = result[:model]
> +      item_hash[:id] = item.class.name+"_"+item.id.to_s
> +      item_hash[:cell] = ["display_name", "display_class"].collect do |attr|
> +        if attr.is_a? Array
> +          value = item
> +          attr.each { |attr_item| value = value.send(attr_item)}
> +          value
> +        else
> +          item.send(attr)
> +        end
> +      end
> +      item_hash[:cell] << result[:percent]
> +      item_hash
> +    end
> +    render :json => json_hash.to_json
> +
> +  end
> +end
We are starting to have a lot of these *_json methods.  I wonder if we
should instead drop the '_json' part so if we want to change the return
type we could just have a param passed in (maybe json is the default)?
Just feels like we are being a bit implementation-specific. If everyone
agrees with this idea, maybe rename this one, and we can start cleaning
up the others as we go.


> diff --git a/wui/src/app/models/host.rb b/wui/src/app/models/host.rb
> index 80acaeb..33a5fa1 100644
> --- a/wui/src/app/models/host.rb
> +++ b/wui/src/app/models/host.rb
> @@ -40,6 +40,12 @@ class Host < ActiveRecord::Base
>      end
>    end
>  
> +  acts_as_xapian :texts => [ :hostname, :uuid ],
> +                 :values => [ [ :created_at, 0, "created_at", :date ],
> +                              [ :updated_at, 1, "updated_at", :date ] ],
> +                 :terms => [ [ :hostname, 'H', "hostname" ] ]
> +
> +
I know they are all 'QEMU' now, but if we are going to have different
hypervisor types, perhaps that would be a useful thing to search on as
well.  I could kind of see the 'arch' field also, though that is
arguably less useful.

>    KVM_HYPERVISOR_TYPE = "KVM"
>    HYPERVISOR_TYPES = [KVM_HYPERVISOR_TYPE]
>    STATE_UNAVAILABLE = "unavailable"
> @@ -75,4 +81,11 @@ class Host < ActiveRecord::Base
>    def cpu_speed
>      "FIX ME!"
>    end
> +
> +  def display_name
> +    hostname
> +  end
> +  def display_class
> +    "Host"
> +  end
>  end

> diff --git a/wui/src/app/models/storage_pool.rb b/wui/src/app/models/storage_pool.rb
> index a135047..39b6a08 100644
> --- a/wui/src/app/models/storage_pool.rb
> +++ b/wui/src/app/models/storage_pool.rb
> @@ -32,6 +32,7 @@ class StoragePool < ActiveRecord::Base
>  
>    validates_presence_of :ip_addr, :hardware_pool_id
>  
> +  acts_as_xapian :texts => [ :ip_addr, :target, :export_path ]
Would 'type' perhaps be useful?

>    ISCSI = "iSCSI"
>    NFS   = "NFS"
>    STORAGE_TYPES = { ISCSI => "Iscsi",
> @@ -55,4 +56,8 @@ class StoragePool < ActiveRecord::Base
>    def get_type_label
>      STORAGE_TYPES.invert[self.class.name.gsub("StoragePool", "")]
>    end
> +  def display_class
> +    "Storage Pool"
> +  end
> +
>  end
diff --git a/wui/src/app/views/search/_grid.rhtml
b/wui/src/app/views/search/_grid.rhtml
> new file mode 100644
> index 0000000..6de9074
> --- /dev/null
> +++ b/wui/src/app/views/search/_grid.rhtml
> @@ -0,0 +1,40 @@
> +<% per_page = 40 %>
> +<div id="<%= table_id %>_div">
> +<%= '<form id="#{table_id}_form">' if checkboxes %>
> +<table id="<%= table_id %>" style="display:none"></table>
> +<%= '</form>' if checkboxes %>
> +</div>
> +<script type="text/javascript">
> +    $("#<%= table_id %>").flexigrid
> +    (
> +    {
> +    url: '<%=  url_for :controller => "search",
> +                       :action => "results_json" %>',
> +    params: [{name: "terms", value: '<%=terms%>'},
> +             {name: "model", value: '<%=model%>'},
> +             {name: "checkboxes", value: <%=checkboxes%>}],
> +    dataType: 'json',
> +    colModel : [
> +        <%= "{display: '', width : 20, align: 'left', process:
> #{table_id}checkbox}," if checkboxes %>
> +        {display: 'Name', width : 200, align: 'left'},
> +        {display: 'Type', width : 120, align: 'left'},
> +        {display: 'Rank', width : 60, align: 'left'}
> +       ],

As I mentioned in irc, I think '% Match'  would be clearer than
'Rank' (especially in the case of 1 result).  So aside from that (mostly
opinion-based) stuff, ACK, works fine for me.

-j