Extended options are supported for listing resources 80 #92
Conversation
ghost
assigned 
|
Build fixed! |
app/models/resource.rb
Outdated
| # Search only the user's account. | ||
| # This can be removed once resource visibility rules are added. | ||
| scope = scope.where("account(resource_id) = ?", account) | ||
| scope = scope.where(Sequel.lit("account(resource_id) = ?", account)) |
There was a problem hiding this comment.
This and any similar changes were made to address deprecation warnings (from Sequel I believe).
There was a problem hiding this comment.
Gee, you read my mind.... I was wondering what this was doing here.
This change to the default behavior of Sequel doesn't seem as necessary when it's used to implement an API. (SQL injection doesn't seem like that much of an issue for us.) I wonder if we should add the auto_literal_strings extension.
There was a problem hiding this comment.
Yeah you're probably right, I think we could safely include the auto_literal_strings extension since we're not using the API to server external users. Maybe that could be a separate story, though.
There was a problem hiding this comment.
We can make it into a separate story, but then let's not make this change (even though it causes a deprecation warning).
There was a problem hiding this comment.
Okay, will undo these changes.
| end | ||
|
|
||
| Given(/^I create a new searchable resource(?: called "([^"]*)")?$/) do |identifier| | ||
| kind ||= "test-resource" |
There was a problem hiding this comment.
There's no kind param to this block.
app/models/resource.rb
Outdated
| # Search only the user's account. | ||
| # This can be removed once resource visibility rules are added. | ||
| scope = scope.where("account(resource_id) = ?", account) | ||
| scope = scope.where(Sequel.lit("account(resource_id) = ?", account)) |
There was a problem hiding this comment.
Gee, you read my mind.... I was wondering what this was doing here.
This change to the default behavior of Sequel doesn't seem as necessary when it's used to implement an API. (SQL injection doesn't seem like that much of an issue for us.) I wonder if we should add the auto_literal_strings extension.
|
|
||
| Scenario: Resource list includes a new resource | ||
|
|
||
| The most basic resource listing route returns all resources in an account. |
There was a problem hiding this comment.
This comment probably shouldn't get deleted.
|
|
||
| Scenario: The resource list can be filtered by resource kind. | ||
| Given I create a new resource |
There was a problem hiding this comment.
With the repetition of the Given clauses in this feature, it seems like there are two sets of scenarios here: one for a single resource, and one for a collection of resources.
I think you should split them up into two separate files.
There was a problem hiding this comment.
What if we don't bother explicitly testing the single resource case? It doesn't seem super necessary, plus the name of feature is "List resources with various types of filtering", which implies multiple resources anyways.
There was a problem hiding this comment.
Okay, I think I found a decent way to do this in a single file while still testing the single-result case.
There was a problem hiding this comment.
Yeah, I'm ok with getting rid of creating a single resource. So, we can get rid of the first scenario, but we need to keep the second two (testing for filtering by resource kind).
| When I successfully GET "/resources/cucumber/test-resource?search=target" | ||
| Then the resource list should only include the searched resources | ||
|
|
||
| Scenario: The resource list is filtered by limit. |
There was a problem hiding this comment.
"filtered by limit" and "filtered by offset" seem like awkward ways of describing what's happening.
There was a problem hiding this comment.
Very true. I've reworded these.
| end | ||
|
|
||
| Given(/^I create a new searchable resource(?: called "([^"]*)")?$/) do |identifier| | ||
| kind ||= "test-resource" |
There was a problem hiding this comment.
There's no kind parameter for this block, so there's no need for ||= here.
| @@ -0,0 +1,112 @@ | |||
| Sequel.migration do | |||
There was a problem hiding this comment.
Was this take verbatim from authz? If not, could you add some comments here about what you changed?
There was a problem hiding this comment.
I had to make a couple changes. Will document them.
| @@ -1,20 +1,66 @@ | |||
| @logged-in | |||
| Feature: List resources with various types of filtering | |||
| Feature: List resources and filter by the kind parameter | |||
There was a problem hiding this comment.
This was ok the way it was -- these scenarios do more than test filter by kind.
There was a problem hiding this comment.
Oops, yep. I changed this when I was trying to split things into two files and forgot to change it back. Will fix.
apotterri
deleted the
extended-options-are-supported-for-listing-resources-80
branch
…st-CNJR-1428 Skip flaky test in http proxy
Fixes #80.