Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/admin facet per set #280

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
36 changes: 36 additions & 0 deletions app/controllers/admin/facets_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,44 @@ def destroy
redirect_to admin_facets_path
end

def remove_item
begin
set_visibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need less ambiguity - either the action (and private method) should have the word "toggle", or set_visibility should take a parameter and set visibility to that value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should add - if we go with "toggle_item", we'd use it to replace both "add_item" and "remove_item"

flash[:success] = t('admin.facets.remove_item.success')
rescue
flash[:alert] = t('admin.facets.remove_item.fail')
end
redirect_to admin_facets_path

end

def add_item
begin
set_visibility
flash[:success] = t('admin.facets.add_item.success')
rescue
flash[:alert] = t('admin.facets.add_item.fail')
end
redirect_to admin_facets_path

end

private

def set_visibility
facet_item = find_item(params[:id])
if facet_item.visible
facet_item.visible = false
else
facet_item.visible = true
end
facet_item.save
end

def find_item(id)
FacetItem.find(id)
end

def find_facet(id)
FacetField.find(id)
end
Expand Down
1 change: 1 addition & 0 deletions app/controllers/catalog_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ class CatalogController < ApplicationController
include Hydra::Controller::ControllerBehavior
# This applies appropriate access controls to all solr queries
self.search_params_logic += [:add_access_controls_to_solr_params]
self.search_params_logic += [:filter_sets]
# Apply access controls to show.
before_filter :enforce_show_permissions, :only => :show

Expand Down
10 changes: 10 additions & 0 deletions app/facades/facet_configuration_facade.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,20 @@ def all_solr_fields

def solr_field_iterator
@solr_field_iterator ||= iterator_factory.new(all_solr_fields).map do |k, v|
setup_facet_item_set(v) if k == :set
[k, HasFacetField.new(v, grouped_facets[k].try(:first) || FacetField.new(:key => k))]
end
end

def setup_facet_item_set(value)
top_terms = value.topTerms.each_slice(2).to_a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm making a guess here, so I could be way off, but it looks like you could never configure items that aren't in the top terms list. Is that what we really want? (I'm not actually finding what it is that topTerms means)

top_terms.each { |item| facet_item item }
end

def facet_item(item)
FacetItem.where(:value => item.first).first_or_create
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this first or create do here exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It finds an instance of FacetItem in the table, and creates one if it doesn't exist yet.

end

def iterator_factory
FacetableSolrFieldIterator
end
Expand Down
3 changes: 3 additions & 0 deletions app/models/facet_item.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class FacetItem < ActiveRecord::Base
validates :value, :presence => true
end
21 changes: 21 additions & 0 deletions app/models/search_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,29 @@ def restrict_to_set(solr_params)
end
end

def filter_sets(solr_params)
solr_params[:fq] ||= []
if remove_items.any?
filter = remove_items.map { |i| "-\"#{i}\"" }.join("")
solr_params[:fq] << "set_ssim:("+filter+")"
end
end

def only_unreviewed(solr_params)
solr_params[:fq] ||= []
solr_params[:fq] << "reviewed_bsi:false"
end

private

def remove_items
items ||= []
FacetItem.all.each do |item|
unless item.visible?
items << item.value
end
end
items
end

end
2 changes: 1 addition & 1 deletion app/views/admin/facets/_field.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
<p class="form-control-static"> <%= field.distinct %> </p>
</div>
</div>
<%= render :partial => "top_terms", :locals => {:field => field} %>
<%= render :partial => "top_terms", :locals => {:field => field, :property => property} %>
</div>
</div>
</div>
11 changes: 10 additions & 1 deletion app/views/admin/facets/_top_terms.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,17 @@
<label class="control-label col-sm-2">Top Terms:</label>
<div class="col-sm-10">
<ul class="form-control-static">

<% field.topTerms.each_slice(2) do |term, count| %>
<li><%= term %> - <%= count %></li>
<li><%= term %> - <%= count %>
<% if property == :set %>
<% if FacetItem.find_by_value(term).visible? %>
<%= link_to("Remove", "/admin/facets/"+FacetItem.find_by_value(term).id.to_s+"/remove_item") %>
<% else %>
<%= link_to("Add", "/admin/facets/"+FacetItem.find_by_value(term).id.to_s+"/add_item") %>
<% end %>
<% end %>
</li>
<% end %>
</ul>
</div>
Expand Down
6 changes: 6 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ en:
destroy:
fail: "Failed to delete facet"
success: "Deleted facet"
add_item:
fail: "Failed to add item"
success: "Added facet item"
remove_item:
fail: "Failed to delete item"
success: "Deleted facet item"
form_templates:
create:
success: Created new template
Expand Down
5 changes: 3 additions & 2 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
devise_for :users
mount Hydra::RoleManagement::Engine => '/'


get '/admin', :to => 'admin#index', :as => "admin_index"
get '/admin/facets/:id/remove_item', :to => 'admin/facets#remove_item', :as => 'remove_item_admin_facet'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do routing specs for these new routes that were added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

get '/admin/facets/:id/add_item', :to => 'admin/facets#add_item', :as => 'add_item_admin_facet'

namespace :admin do
resources :facets
resources :form_templates
Expand Down Expand Up @@ -37,5 +39,4 @@
end
end


end
11 changes: 11 additions & 0 deletions db/migrate/20150915185324_create_facet_items.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class CreateFacetItems < ActiveRecord::Migration
def change
create_table :facet_items do |t|
t.string :value
t.boolean :visible, :default => true

t.timestamps null: false
end
add_index :facet_items, :value, unique: true
end
end
11 changes: 10 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20150814184553) do
ActiveRecord::Schema.define(version: 20150915185324) do

create_table "bookmarks", force: :cascade do |t|
t.integer "user_id", null: false
Expand All @@ -34,6 +34,15 @@

add_index "facet_fields", ["key"], name: "index_facet_fields_on_key", unique: true

create_table "facet_items", force: :cascade do |t|
t.string "value"
t.boolean "visible", default: true
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
end

add_index "facet_items", ["value"], name: "index_facet_items_on_value", unique: true

create_table "form_template_properties", force: :cascade do |t|
t.integer "form_template_id"
t.string "name", null: false
Expand Down
43 changes: 43 additions & 0 deletions spec/controllers/admin/facets_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,47 @@
end
end
end

describe "#remove_item" do
context "when the field doedn't exist" do
it "should redirect and show a flash" do
patch :remove_item, id: "1"
expect(response).to redirect_to admin_facets_path
expect(flash[:alert]).to eq I18n.t("admin.facets.remove_item.fail")
end
end
context "when the field exists" do
it "should remove item" do
FacetField.create(:key => "hello", :label => "world")
item = FacetItem.create(:value => "my set")
patch :remove_item, id: item.id.to_s
expect(response).to redirect_to admin_facets_path
expect(flash[:success]).to eq I18n.t("admin.facets.remove_item.success")
expect(FacetItem.where(:id => item.id).first.visible).to eq false
end
end
end

describe "#add_item" do
context "when the field doedn't exist" do
it "should redirect and show a flash" do
patch :add_item, id: "1"
expect(response).to redirect_to admin_facets_path
expect(flash[:alert]).to eq I18n.t("admin.facets.add_item.fail")
end
end
context "when the field exists" do
it "should add item" do
FacetField.create(:key => "hello", :label => "world")
item = FacetItem.create(:value => "my set")
patch :remove_item, id: item.id.to_s
expect(FacetItem.where(:id => item.id).first.visible).to eq false
patch :add_item, id: item.id.to_s
expect(response).to redirect_to admin_facets_path
expect(flash[:success]).to eq I18n.t("admin.facets.add_item.success")
expect(FacetItem.where(:id => item.id).first.visible).to eq true
end
end
end

end
11 changes: 7 additions & 4 deletions spec/facades/facet_configuration_facade_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@
"bad_key_ssim" =>
{
"distinct" => 3
},
"set_ssim" =>
{
"distinct" => 2,
"topTerms" => ["collection1", 1, "collection2", 1]
}
}
end
Expand All @@ -37,7 +42,6 @@
context "when there are active facets" do
it "should contain settings for them" do
facet = FacetField.create(:key => "title")

expect(subject.active.to_h.keys.first).to eq :title
expect(subject.active.to_h.values.first.distinct).to eq 5
expect(subject.active.to_h.values.first.facet).to eq facet
Expand All @@ -48,15 +52,14 @@
describe "#inactive" do
context "when there are no active facets" do
it "should have all the fields" do
expect(subject.inactive.to_h.keys).to eq [:workType, :alternative, :title]
expect(subject.inactive.to_h.keys).to eq [:workType, :alternative, :title, :set]
expect(subject.inactive.to_h[:workType].facet).not_to be_persisted
end
end
context "when there are active facets" do
it "should have only the non-active fields" do
FacetField.create(:key => "title")

expect(subject.inactive.to_h.keys).to eq [:workType, :alternative]
expect(subject.inactive.to_h.keys).to eq [:workType, :alternative, :set]
end
end
end
Expand Down
8 changes: 8 additions & 0 deletions spec/factories/facet_items.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
FactoryGirl.define do
sequence(:value) { |n| "MyString#{n}" }
factory :facet_item do
value
visible true
end

end
8 changes: 8 additions & 0 deletions spec/models/facet_item_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
require 'rails_helper'

RSpec.describe FacetItem, type: :model do
describe "validations" do
it { should validate_presence_of :value }
end

end
26 changes: 26 additions & 0 deletions spec/models/search_builder_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,30 @@
end
end

describe "#filter_sets" do
let(:processor_chain) { [:filter_sets] }
let(:add_visible_item) { create :facet_item, :value => "banana", :visible => true }
let(:remove_first_item) { create :facet_item, :value => "hello", :visible => false }
let(:remove_second_item) { create :facet_item, :value => "world", :visible => false }

context "when there is no set" do
it "should not do anything" do
expect(subject.processed_parameters[:fq]).to eq []
end
end

context "when there is a set with multiple items" do
it "should display visible items" do
add_visible_item
expect(subject.processed_parameters[:fq].first).not_to match(/banana/)
end
it "should filter out removed items" do
add_visible_item
remove_first_item
remove_second_item
expect(subject.processed_parameters[:fq]).to eq ["set_ssim:(-\"hello\"-\"world\")"]
expect(subject.processed_parameters[:fq].first).not_to match(/banana/)
end
end
end
end
2 changes: 2 additions & 0 deletions spec/rails_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
# instead of true.
config.use_transactional_fixtures = true

config.include FactoryGirl::Syntax::Methods

# RSpec Rails can automatically mix in different behaviours to your tests
# based on their file location, for example enabling you to call `get` and
# `post` in specs under `spec/controllers`.
Expand Down
7 changes: 7 additions & 0 deletions spec/routing/admin_facet_routing_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,11 @@
it "should route delete /admin/facet/1 to facets#destroy" do
expect(delete '/admin/facets/1').to route_to :controller => "admin/facets", :action => "destroy", :id => "1"
end
it "should route /admin/facets/1/remove_item to admin/facets#remove_item" do
expect(get '/admin/facets/1/remove_item').to route_to :controller => "admin/facets", :action => "remove_item", :id=> "1"
end
it "should route /admin/facets/1/add_item to admin/facets#add_item" do
expect(get '/admin/facets/1/add_item').to route_to :controller => "admin/facets", :action => "add_item", :id=> "1"
end

end