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

fixes #5235: it's impossible to create filters with invaid searches #1552

Closed
wants to merge 1 commit into from

Conversation

@dmitri-d
Copy link
Member

dmitri-d commented Jul 1, 2014

No description provided.

@@ -1,6 +1,10 @@
require 'test_helper'

class FilterTest < ActiveSupport::TestCase
before do
User.current = users :admin

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 2, 2014

Contributor

There's an as_admin helper which is preferable if you need it

@@ -177,4 +181,18 @@ class FilterTest < ActiveSupport::TestCase
assert_equal '(domain ~ test*) and (organization_id = 1)', f.search_condition
end

test "filter with a valid search string is valid" do
f = Factory.build(:filter, :search => "name = 'blah'", :resource_type => 'Domain')
assert f.valid?

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 2, 2014

Contributor

and as of e07f9a1, assert_valid and friends

def validate(record)
resource_class = record.resource_class
resource_class.search_for(record.search) unless (resource_class.nil? || record.search.nil?)
rescue => e

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 2, 2014

Contributor

Is it possible to be more specific? IIRC scoped_search has its own exceptions

@dmitri-d

This comment has been minimized.

Copy link
Member Author

dmitri-d commented Jul 3, 2014

@domcleal: ping?

assert_not_include f.taxonomy_search, ' or '
as_admin do
o = Factory.create :organization
f = Factory.build(:filter, :search => '', :unlimited => '1', :organization_ids => [o.id])

This comment has been minimized.

Copy link
@dLobatog

dLobatog Jul 4, 2014

Member

Asserts need not to be in the as_admin block, also 1-character variables could be changed to organization/filter. 😄

This comment has been minimized.

Copy link
@dLobatog

dLobatog Jul 4, 2014

Member

Same for the rest of the tests, I'd say include them in a context and refactor the common parts, like some assertions, some objects created.

@dmitri-d

This comment has been minimized.

Copy link
Member Author

dmitri-d commented Jul 7, 2014

@elobato: ping

f.reload
assert f.valid?
assert f.unlimited?
assert_nil f.taxonomy_search

This comment has been minimized.

Copy link
@dLobatog

dLobatog Jul 7, 2014

Member

It'd be good to include a

teardown do 
  assert f.valid?
end

since that assertion is done everywhere within this context, and is not specific to each test.

This comment has been minimized.

Copy link
@dmitri-d

dmitri-d Jul 7, 2014

Author Member

I think that'd reduce test readability -- I'd rather not do that.

This comment has been minimized.

Copy link
@dLobatog

dLobatog Jul 7, 2014

Member

Fair enough, maybe after would be more readable. Anyway this is up to discussion, I'll merge this for the moment and search for a way to discuss test standards 👍

@dLobatog

This comment has been minimized.

Copy link
Member

dLobatog commented Jul 7, 2014

Merged as 0545fd1, thanks @witlessbird !

@dLobatog dLobatog closed this Jul 7, 2014
@dmitri-d dmitri-d deleted the dmitri-d:5235 branch Dec 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.