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

Remove fully enabled feature flags that have been enabled for years #3375

Merged
merged 9 commits into from
Feb 21, 2024
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ New entries in this file should aim to provide a meaningful amount of informatio

## [Unreleased]

### Removed
- Remove fully enabled feature flags that have been enabled for years [PR#3375](https://github.com/ualbertalib/jupiter/pull/3375)

### Chores
- Update Bundler to v2.5.5 to match production [PR#3374](https://github.com/ualbertalib/jupiter/pull/3374)

Expand Down
3 changes: 1 addition & 2 deletions app/controllers/collections_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ def show
base_restriction_key: Item.solr_exporter_class.solr_name_for(:member_of_paths, role: :pathing),
value: @collection.path,
params:,
current_user:,
fulltext: Flipper.enabled?(:fulltext_search, current_user)
current_user:
)
@results = search_query_index.results
@search_models = search_query_index.search_models
Expand Down
3 changes: 1 addition & 2 deletions app/controllers/profile_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ def index
base_restriction_key: Item.solr_exporter_class.solr_name_for(:owner, role: :exact_match),
value: @user.id,
params:,
current_user: @user,
fulltext: Flipper.enabled?(:fulltext_search, current_user)
current_user: @user
)
@results = search_query_index.results
@search_models = search_query_index.search_models
Expand Down
3 changes: 1 addition & 2 deletions app/controllers/search_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,7 @@ def index
search_query_index = UserSearchService.new(
search_models: models,
params:,
current_user:,
fulltext: Flipper.enabled?(:fulltext_search, current_user)
current_user:
)
@results = search_query_index.results
@search_models = search_query_index.search_models
Expand Down
6 changes: 1 addition & 5 deletions app/decorators/facets/default_facet_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,7 @@ def facet_search_link
end

def display
if Flipper.enabled?(:facet_badge_category_name)
"#{@category_name}: #{display_value}"
else
display_value
end
"#{@category_name}: #{display_value}"
end

def display_value
Expand Down
8 changes: 1 addition & 7 deletions app/models/jupiter_core/search.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,7 @@ def self.faceted_search(q: '', facets: [], ranges: [], models: [], as: nil, full

base_query << q if q.present?
facets.each do |key, values|
if Flipper.enabled?(:or_facets)
fq << %Q(#{key}:\(#{values.collect { |value| "\"#{value}\"" }.join(' OR ')}\))
else
values.each do |value|
fq << %Q(#{key}:"#{value}")
end
end
fq << %Q(#{key}:\(#{values.collect { |value| "\"#{value}\"" }.join(' OR ')}\))
end
ranges.each do |key, value|
fq << "#{key}:[#{value[:begin]} TO #{value[:end]}]"
Expand Down
7 changes: 3 additions & 4 deletions app/services/user_search_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ class UserSearchService

attr_reader :search_models, :invalid_date_range

def initialize(current_user:, params:, base_restriction_key: nil, value: nil, # rubocop:disable Metrics/ParameterLists
search_models: [Item, Thesis], fulltext: false)
def initialize(current_user:, params:, base_restriction_key: nil, value: nil,
search_models: [Item, Thesis])
if base_restriction_key.present? && value.blank?
raise ArgumentError, 'Must supply both a base_restriction_key and a value'
end
Expand All @@ -18,7 +18,6 @@ def initialize(current_user:, params:, base_restriction_key: nil, value: nil, #
@search_models = search_models
@search_params = search_params(params)
@current_user = current_user
@fulltext = fulltext
end

def results
Expand All @@ -31,7 +30,7 @@ def results

search_options = { q: query, models: search_models, as: @current_user,
facets:, ranges: @search_params[:ranges],
fulltext: @fulltext }
fulltext: true }

# Sort by relevance if a search term is present and no explicit sort field has been chosen
sort_fields = @search_params[:sort]
Expand Down
8 changes: 3 additions & 5 deletions app/views/layouts/admin.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,9 @@
<li class="nav-item">
<%= link_to t('.feature_flags'), flipper_path, class: 'nav-link' %>
</li>
<% if Flipper.enabled?(:batch_ingest, current_user) %>
<li class="nav-item">
<%= active_link_to t('admin.batch_ingests.index.header'), admin_batch_ingests_path, class: 'nav-link', active: :exclusive %>
</li>
<% end %>
<li class="nav-item">
<%= active_link_to t('admin.batch_ingests.index.header'), admin_batch_ingests_path, class: 'nav-link', active: :exclusive %>
</li>
</ul>
<%= yield %>
</div>
Expand Down
14 changes: 1 addition & 13 deletions test/controllers/search_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,24 +69,12 @@ class SearchControllerTest < ActionDispatch::IntegrationTest
assert_no_match(/<a href="\/items\/#{@item4.id}">/, response.body)
end

test 'should NOT render highlights on search result page by default' do
get search_url, params: { search: 'French' }

assert_response :success

assert_no_match(/<mark>French<\/mark>/, response.body)
end

test 'should render highlights on search result page when feature flag is on' do
Flipper.enable(:fulltext_search)

test 'should render highlights on search result page by default' do
get search_url, params: { search: 'French' }

assert_response :success

assert_match(/<mark>French<\/mark>/, response.body)

Flipper.disable(:fulltext_search)
end

test 'search with invalid date range shows alert' do
Expand Down
22 changes: 4 additions & 18 deletions test/integration/solr_bad_request_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,10 @@
class SolrBadRequestTest < ActionDispatch::IntegrationTest

test 'request in which solr gives 400 gives a proper response' do
get search_url, params: { facets: { all_subjects_sim: ['Antimicrobial', '"blown-pack"'] } }
Copy link
Contributor Author

@murny murny Feb 12, 2024

Choose a reason for hiding this comment

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

These are passing now, so might have to do with :or_facets feature flag being enabled now.

The only one failing now is the last one which had special characters ("\"\\'Bacteriocins").

So I assume it was failing before because we passing an array or a hash with multiple values and now with :or_facets feature flag being always enabled this is okay?

So just simplify the test and added a comment on what we are actually testing here.


assert_response :bad_request

get search_url, params: { facets: { all_subjects_sim: ['"A Most Extraordinary Case"'] } }

assert_response :bad_request

get search_url, params: { facets: { all_subjects_sim: ['"291" (Gallery)'],
departments_sim: ['Department of Art and Design'],
item_type_with_status_sim: ['thesis'],
member_of_paths_dpsim: ['db9a4e71-f809-4385-a274-048f28eb6814'] } }

assert_response :bad_request

get search_url, params: { direction: 'asc',
facets: { all_subjects_sim: ['Meat spoilage', "\"\\'Bacteriocins", '"blown-pack"'] },
sort: 'title' }
# Special characters can cause solr to give a 400 error, so let's test that we are handling that properly
get search_url, params: {
facets: { all_subjects_sim: ["\"\\'Bacteriocins"] }
}

assert_response :bad_request
end
Expand Down
18 changes: 1 addition & 17 deletions test/models/search_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,7 @@

class SearchTest < ActiveSupport::TestCase

test 'default behaviour within a facet is AND and between facets is AND' do
results = JupiterCore::Search.faceted_search(models: [Item],
facets: {
all_subjects_sim: ['Fancy things',
'Practical things'],
all_contributors_sim: ['Joe Blow']
})

# rubocop:disable Layout/LineLength
assert_includes results.criteria[:fq],
'all_subjects_sim:"Fancy things" AND all_subjects_sim:"Practical things" AND all_contributors_sim:"Joe Blow"'
# rubocop:enable Layout/LineLength
end

test 'default behaviour within a facet is OR and between facets is AND when feature flag is on' do
Flipper.enable(:or_facets)
test 'default behaviour within a facet is OR and between facets is AND' do
item1 = items(:item_fancy)
item2 = items(:item_practical)

Expand All @@ -34,7 +19,6 @@ class SearchTest < ActiveSupport::TestCase

assert_includes results.criteria[:fq],
'all_subjects_sim:("Fancy things" OR "Practical things") AND all_contributors_sim:("Joe Blow")'
Flipper.disable(:or_facets)
end

end
3 changes: 1 addition & 2 deletions test/services/user_search_service_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ class UserSearchServiceTest < ActiveSupport::TestCase

search = UserSearchService.new(
current_user:,
params:,
fulltext: true
params:
)

# Only Admin item has "French" in its description
Expand Down
5 changes: 1 addition & 4 deletions test/system/search_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,7 @@ class SearchTest < ApplicationSystemTestCase
badge.assert_selector 'span.badge', text: 'Fancy Collection 1'
end

test 'facet badge should have category when flipped' do
Flipper.enable(:facet_badge_category_name)
test 'facet badge should have category' do
visit root_path
fill_in name: 'search', with: 'Fancy'
click_button 'Search'
Expand All @@ -219,8 +218,6 @@ class SearchTest < ApplicationSystemTestCase
href: search_path(search: 'Fancy'))

badge.assert_selector 'span.badge', text: 'Collections: Fancy Community/Fancy Collection 1'

Flipper.disable(:facet_badge_category_name)
end

test 'anybody should be able to view community/collection hits via tabs' do
Expand Down
Loading