Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix admin edition filtering #386

Merged
merged 4 commits into from

2 participants

Murray Steele Bradley Wright
Murray Steele

For: https://www.pivotaltracker.com/story/show/46734111

There was a difference between not explicitly choosing a state and choosing "all" when hitting the admin edition index. Made "all" be the default if no state param is provided.

Took the opportunity to also do the same for world_location_ids (default to "all") and tidy up UI so that all dropdowns (org, world location, etc) will pass on other options (state, type, etc).

h-lame added some commits
Murray Steele h-lame EditionsController adds a default state param of 'active'
This is to avoid selecting 'all' from the filter menu and not having anything selected presenting different results.

For: https://www.pivotaltracker.com/story/show/46734111
0ae9f73
Murray Steele h-lame EditionsController adds a default world_location_ids param of 'all'
This is so the front end will highlight "All" world locations even if you didn't select one.

For: https://www.pivotaltracker.com/story/show/46734111
07124d5
Murray Steele h-lame Tidy up admin edition filtering UI
1. make sure each filter option that is rendered as a form has hidden fields for the filter options that make sense (state, type, etc..)
2. use the @filter.options instead of params to query values (because we now inject some defaults)
3. highlight the 'world location ids' options if selected
aff7899
Murray Steele h-lame Remove meaningless test for admin edition filter ui 838376f
Bradley Wright

:+1:for stripping all the hidden field nonsense.

Bradley Wright bradleywright merged commit 162fc6b into from
Bradley Wright bradleywright deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 25, 2013
  1. Murray Steele

    EditionsController adds a default state param of 'active'

    h-lame authored
    This is to avoid selecting 'all' from the filter menu and not having anything selected presenting different results.
    
    For: https://www.pivotaltracker.com/story/show/46734111
  2. Murray Steele

    EditionsController adds a default world_location_ids param of 'all'

    h-lame authored
    This is so the front end will highlight "All" world locations even if you didn't select one.
    
    For: https://www.pivotaltracker.com/story/show/46734111
  3. Murray Steele

    Tidy up admin edition filtering UI

    h-lame authored
    1. make sure each filter option that is rendered as a form has hidden fields for the filter options that make sense (state, type, etc..)
    2. use the @filter.options instead of params to query values (because we now inject some defaults)
    3. highlight the 'world location ids' options if selected
  4. Murray Steele
This page is out of date. Refresh to see the latest.
2  app/assets/stylesheets/admin/document_index.scss
View
@@ -4,7 +4,7 @@
margin-top: 1em;
}
- .author-filter, .organisation-filter {
+ .author-filter, .organisation-filter, .world-location-filter {
form {
margin: 0 -15px;
padding: 0 15px;
6 app/controllers/admin/editions_controller.rb
View
@@ -238,6 +238,10 @@ def params_filters
sanitized_filters(params.slice(:type, :state, :organisation, :author, :page, :title, :world_location_ids))
end
+ def params_filters_with_default_state
+ params_filters.reverse_merge(state: 'active', world_location_ids: 'all')
+ end
+
def sanitized_filters(filters)
valid_states = %w[ active imported draft submitted rejected published scheduled ]
filters.delete(:state) unless filters[:state].nil? || valid_states.include?(filters[:state].to_s)
@@ -245,7 +249,7 @@ def sanitized_filters(filters)
end
def filter
- @filter ||= params_filters.any? && EditionFilter.new(edition_class, current_user, params_filters)
+ @filter ||= params_filters.any? && EditionFilter.new(edition_class, current_user, params_filters_with_default_state)
end
def detect_other_active_editors
31 app/helpers/admin/editions_helper.rb
View
@@ -21,18 +21,41 @@ def admin_document_series_header_link
admin_header_link "Document Series", admin_document_series_index_path, /^#{Whitehall.router_prefix}\/admin\/document_series/
end
- def link_to_filter(link, options, html_options={})
- content_tag(:li, link_to(link, url_for(params.slice('state', 'type', 'author', 'organisation', 'title', 'world_location_ids').merge(options)), html_options), class: filter_class(options))
+ def link_to_filter(link, options, filter, html_options={})
+ content_tag(:li, link_to(link, url_for(filter.options.slice('state', 'type', 'author', 'organisation', 'title', 'world_location_ids').merge(options)), html_options), class: active_filter_if_options_match_class(filter, options))
end
- def filter_class(options)
+ def active_filter_if_options_match_class(filter, options)
current = options.keys.all? do |key|
- options[key].to_param == params[key].to_param
+ options[key].to_param == filter.options[key].to_param
end
'active' if current
end
+ def active_filter_unless_values_match_class(filter, key, *disallowed_values)
+ filter_value = filter.options[key]
+ 'active' if filter_value && disallowed_values.none? { |disallowed_value| filter_value == disallowed_value }
+ end
+
+ def pass_through_filter_options_as_hidden_fields(filter, *options_to_pass)
+ options_to_pass.map do |option_to_pass|
+ value = filter.options[option_to_pass]
+ pass_through_filter_value_as_hidden_field(option_to_pass, value)
+ end.join.html_safe
+ end
+
+ def pass_through_filter_value_as_hidden_field(filter_name, filter_value)
+ return '' unless filter_value
+ if filter_value.is_a?(Array)
+ filter_value.map do |value_to_pass|
+ hidden_field_tag "#{filter_name}[]", value_to_pass
+ end.join.html_safe
+ else
+ hidden_field_tag filter_name, filter_value
+ end
+ end
+
def viewing_all_active_editions?
params[:state] == 'active'
end
85 app/views/admin/editions/index.html.erb
View
@@ -11,77 +11,72 @@
<li class="nav-header">Filter by Title</li>
<li>
<%= form_tag("", method: :get, class: "pull-left") do %>
- <%= hidden_field_tag :organisation, params[:organisation] if params[:organisation] %>
- <%= hidden_field_tag :type, params[:type] if params[:type] %>
- <%= hidden_field_tag :state, params[:state] if params[:state] %>
- <%= hidden_field_tag :author, params[:author] if params[:author] %>
- <input type="search" id="search_title" name="title" value="<%= params[:title] %>" placeholder="Search title">
+ <%= pass_through_filter_options_as_hidden_fields(@filter, :organisation, :type, :state, :author, :world_location_ids) %>
+ <input type="search" id="search_title" name="title" value="<%= @filter.options[:title] %>" placeholder="Search title">
<% end %>
</li>
<li class="nav-header">Filter by Author</li>
- <%= link_to_filter('Me', {author: current_user, organisation: nil}, title: "Show documents by me") %>
- <%= link_to_filter('My department', {author: nil, organisation: current_user.organisation}, title: "Show documents by my department") %>
+ <%= link_to_filter('Me', {author: current_user, organisation: nil}, @filter, title: "Show documents by me") %>
+ <%= link_to_filter('My department', {author: nil, organisation: current_user.organisation}, @filter, title: "Show documents by my department") %>
- <li class="organisation-filter <%= (params[:organisation] && params[:organisation] != current_user.organisation_id) ? 'active' : nil %>">
+ <li class="organisation-filter <%= active_filter_unless_values_match_class(@filter, :organisation, current_user.organisation_id) %>">
<%= form_tag("", method: :get) do %>
- <%= select_tag :organisation, options_from_collection_for_select(Organisation.all - [current_user.organisation], 'id', 'select_name', params[:organisation]), class: 'chzn-select', include_blank: true, data: { placeholder: "Other organisations..." } %>
- <%= hidden_field_tag :type, params[:type] if params[:type] %>
- <%= hidden_field_tag :state, params[:state] if params[:state] %>
+ <%= select_tag :organisation, options_from_collection_for_select(Organisation.with_translations(:en).all - [current_user.organisation], 'id', 'select_name', @filter.options[:organisation]), class: 'chzn-select', include_blank: true, data: { placeholder: "Other organisations..." } %>
+ <%= pass_through_filter_options_as_hidden_fields(@filter, :type, :state, :world_location_ids) %>
<%= submit_tag "Go", class: "btn" %>
<% end %>
</li>
- <%= link_to_filter('Everyone', {author: nil, organisation: nil}, title: "Show documents by everyone") %>
- <li class="author-filter <%= (params[:author] && params[:author] != current_user.id.to_s) ? 'active' : nil %>">
+ <%= link_to_filter('Everyone', {author: nil, organisation: nil}, @filter, title: "Show documents by everyone") %>
+ <li class="author-filter <%= active_filter_unless_values_match_class(@filter, :author, current_user.id.to_s) %>">
<%= form_tag("", method: :get) do %>
- <%= select_tag :author, options_from_collection_for_select(User.all - [current_user], 'id', 'name', params[:author]), class: 'chzn-select', include_blank: true, data: { placeholder: "Other authors..." } %>
- <%= hidden_field_tag :type, params[:type] if params[:type] %>
- <%= hidden_field_tag :state, params[:state] if params[:state] %>
+ <%= select_tag :author, options_from_collection_for_select(User.all - [current_user], 'id', 'name', @filter.options[:author]), class: 'chzn-select', include_blank: true, data: { placeholder: "Other authors..." } %>
+ <%= pass_through_filter_options_as_hidden_fields(@filter, :type, :state, :world_location_ids) %>
<%= submit_tag "Go", class: "btn" %>
<% end %>
</li>
<li class="nav-header">Filter by World locations</li>
<% if current_user.world_locations.any? %>
- <%= link_to_filter('My Locations', {world_location_ids: current_user.world_locations.map(&:id)}, title: "Show documents for your locations") %>
+ <%= link_to_filter('My Locations', {world_location_ids: current_user.world_locations.map(&:id)}, @filter, title: "Show documents for your locations") %>
<% end %>
- <%= link_to_filter('Everywhere', {world_location_ids: "all"}, title: "Show documents for all locations") %>
- <li class="world-location-filter">
+ <%= link_to_filter('Everywhere', {world_location_ids: "all"}, @filter, title: "Show documents for all locations") %>
+ <li class="world-location-filter <%= active_filter_unless_values_match_class(@filter, :world_location_ids, 'all', current_user.world_locations.map(&:id)) %>">
<%= form_tag("", method: :get) do %>
- <%= select_tag :world_location_ids,
- options_from_collection_for_select(WorldLocation.ordered_by_name, 'id', 'name', params[:world_location_ids]),
- multiple: true,
- class: 'chzn-select',
- data: { placeholder: "World locations..."} %>
-
+ <%= select_tag :world_location_ids,
+ options_from_collection_for_select(WorldLocation.ordered_by_name, 'id', 'name', @filter.options[:world_location_ids]),
+ multiple: true,
+ class: 'chzn-select',
+ data: { placeholder: "World locations..."} %>
+ <%= pass_through_filter_options_as_hidden_fields(@filter, :organisation, :type, :state, :author) %>
<%= submit_tag "Go", class: "btn" %>
<% end %>
</li>
<li class="nav-header">Filter by Kind</li>
- <%= link_to_filter 'All', {type: nil}, title: "Show all kinds of document" %>
- <%= link_to_filter 'Policies', {type: 'policy'}, title: "Show only policies" %>
- <%= link_to_filter 'Publications', {type: 'publication'}, title: "Show only publications" %>
- <%= link_to_filter 'News articles', {type: 'news_article'}, title: "Show only news articles" %>
- <%= link_to_filter 'Consultations', {type: 'consultation'}, title: "Show only consultations" %>
- <%= link_to_filter 'Speeches', {type: 'speech'}, title: "Show only speeches" %>
- <%= link_to_filter 'Detailed guide', {type: 'detailed_guide'}, title: "Show only detailed guides" %>
- <%= link_to_filter 'Worldwide priorities', {type: 'worldwide_priorities'}, title: "Show only worldwide priorities" %>
- <%= link_to_filter 'World location news article', {type: 'world_location_news_article'}, title: "Show only worldwide priorities" %>
- <%= link_to_filter 'Case studies', {type: 'case_studies'}, title: "Show only case studies" %>
- <%= link_to_filter 'Statistical data sets', {type: 'statistical_data_sets'}, title: "Show only statistical data sets" %>
+ <%= link_to_filter 'All', {type: nil}, @filter, title: "Show all kinds of document" %>
+ <%= link_to_filter 'Policies', {type: 'policy'}, @filter, title: "Show only policies" %>
+ <%= link_to_filter 'Publications', {type: 'publication'}, @filter, title: "Show only publications" %>
+ <%= link_to_filter 'News articles', {type: 'news_article'}, @filter, title: "Show only news articles" %>
+ <%= link_to_filter 'Consultations', {type: 'consultation'}, @filter, title: "Show only consultations" %>
+ <%= link_to_filter 'Speeches', {type: 'speech'}, @filter, title: "Show only speeches" %>
+ <%= link_to_filter 'Detailed guide', {type: 'detailed_guide'}, @filter, title: "Show only detailed guides" %>
+ <%= link_to_filter 'Worldwide priorities', {type: 'worldwide_priorities'}, @filter, title: "Show only worldwide priorities" %>
+ <%= link_to_filter 'World location news article', {type: 'world_location_news_article'}, @filter, title: "Show only worldwide priorities" %>
+ <%= link_to_filter 'Case studies', {type: 'case_studies'}, @filter, title: "Show only case studies" %>
+ <%= link_to_filter 'Statistical data sets', {type: 'statistical_data_sets'}, @filter, title: "Show only statistical data sets" %>
<% if current_user.can_handle_fatalities? %>
- <%= link_to_filter 'Fatality notices', {type: 'fatality_notices'}, title: "Show only fatality notices" %>
+ <%= link_to_filter 'Fatality notices', {type: 'fatality_notices'}, @filter, title: "Show only fatality notices" %>
<% end %>
<li class="nav-header">Filter by state</li>
- <% document_type = (params[:type] || "document").pluralize %>
- <%= link_to_filter 'All', {state: :active}, title: "Show #{document_type} in any workflow state" %>
- <%= link_to_filter 'Imported (pre-draft)', {state: :imported}, title: "Show only imported pre-draft #{document_type}" %>
- <%= link_to_filter 'Drafts', {state: :draft}, title: "Show only draft #{document_type}" %>
- <%= link_to_filter 'Submitted', {state: :submitted}, title: "Show only submitted #{document_type}" %>
- <%= link_to_filter 'Rejected', {state: :rejected}, title: "Show only rejected #{document_type}" %>
- <%= link_to_filter 'Scheduled', {state: :scheduled}, title: "Show only scheduled #{document_type}" %>
- <%= link_to_filter 'Published', {state: :published}, title: "Show only published #{document_type}" %>
+ <% document_type = (@filter.options[:type] || "document").pluralize %>
+ <%= link_to_filter 'All', {state: :active}, @filter, title: "Show #{document_type} in any workflow state" %>
+ <%= link_to_filter 'Imported (pre-draft)', {state: :imported}, @filter, title: "Show only imported pre-draft #{document_type}" %>
+ <%= link_to_filter 'Drafts', {state: :draft}, @filter, title: "Show only draft #{document_type}" %>
+ <%= link_to_filter 'Submitted', {state: :submitted}, @filter, title: "Show only submitted #{document_type}" %>
+ <%= link_to_filter 'Rejected', {state: :rejected}, @filter, title: "Show only rejected #{document_type}" %>
+ <%= link_to_filter 'Scheduled', {state: :scheduled}, @filter, title: "Show only scheduled #{document_type}" %>
+ <%= link_to_filter 'Published', {state: :published}, @filter, title: "Show only published #{document_type}" %>
</ul>
</nav>
</div>
31 test/functional/admin/editions_controller_test.rb
View
@@ -192,25 +192,39 @@ class Admin::EditionsControllerTest < ActionController::TestCase
test 'should pass filter parameters to an edition filter' do
stub_filter = stub_edition_filter
- Admin::EditionsController::EditionFilter.expects(:new).with(anything, anything, {"state" => "draft", "type" => "policy"}).returns(stub_filter)
+ Admin::EditionsController::EditionFilter.expects(:new).with(anything, anything, has_entries("state" => "draft", "type" => "policy")).returns(stub_filter)
get :index, state: :draft, type: :policy
end
test "should not pass blank parameters to the edition filter" do
stub_filter = stub_edition_filter
- Admin::EditionsController::EditionFilter.expects(:new).with(anything, anything, {"state" => "draft"}).returns(stub_filter)
+ Admin::EditionsController::EditionFilter.expects(:new).with(anything, anything, Not(has_key("author"))).returns(stub_filter)
get :index, state: :draft, author: ""
end
- test 'should strip out any invalid states passed as parameters' do
+ test 'should strip out any invalid states passed as parameters and replace them with "active"' do
stub_filter = stub_edition_filter
- Admin::EditionsController::EditionFilter.expects(:new).with(anything, anything, {"type" => "policy"}).returns(stub_filter)
+ Admin::EditionsController::EditionFilter.expects(:new).with(anything, anything, has_entry("state" => "active")).returns(stub_filter)
get :index, state: :haxxor_method, type: :policy
end
+ test 'should add state param set to "active" if none is supplied' do
+ stub_filter = stub_edition_filter
+ Admin::EditionsController::EditionFilter.expects(:new).with(anything, anything, has_entry("state" => "active")).returns(stub_filter)
+
+ get :index, type: :policy
+ end
+
+ test 'should add world_location_ids param set to "all" if none is supplied' do
+ stub_filter = stub_edition_filter
+ Admin::EditionsController::EditionFilter.expects(:new).with(anything, anything, has_entry("world_location_ids" => "all")).returns(stub_filter)
+
+ get :index, type: :policy
+ end
+
view_test 'should distinguish between edition types when viewing the list of editions' do
policy = create(:draft_policy)
publication = create(:draft_publication)
@@ -347,12 +361,6 @@ class Admin::EditionsControllerTest < ActionController::TestCase
assert_select "tr.force_published"
end
- view_test "should link to all active editions" do
- get :index, state: :draft
-
- assert_select "a[href='#{admin_editions_path(state: :active)}']"
- end
-
test "should not display the featured column when viewing all active editions" do
create(:published_news_article)
@@ -451,7 +459,8 @@ class Admin::EditionsControllerTest < ActionController::TestCase
def stub_edition_filter(attributes = {})
default_attributes = {
editions: Kaminari.paginate_array(attributes[:editions] || []).page(1),
- page_title: '', edition_state: '', valid?: true
+ page_title: '', edition_state: '', valid?: true,
+ options: {}
}
stub('edition filter', default_attributes.merge(attributes.except(:editions)))
end
Something went wrong with that request. Please try again.