Skip to content

Issue 1002 : Updated api/users#1014

Merged
reidka merged 10 commits into
MarkUsProject:masterfrom
danielstjules:issue-1002
Mar 6, 2013
Merged

Issue 1002 : Updated api/users#1014
reidka merged 10 commits into
MarkUsProject:masterfrom
danielstjules:issue-1002

Conversation

@danielstjules

Copy link
Copy Markdown
Contributor

This follows the details specified at #1002

Quick overview:
-api/users now uses :id for show and update methods
-limit, offset and filter parameters are available for retrieving collections (index method)
-the filter parameter is available to show and index methods to restrict which attributes are returned for each resource
-The changes do not conflict with any of the other current API routes

@benjaminvialle

Copy link
Copy Markdown
Member

Hi Daniel,

Your code looks very good and clean.
I ran tests and they all pass, except one :
test: An authenticated request to api/users testing index function should ignore invalid filters. (Api::UsersControllerTest) [test/functional/api/users_controller_test.rb:137]:
Expected exactly 3 elements matching "user", found 0.
is not true.

Would it be interesting to put output to log each time API are called?

For me, your code is ready to be merged. I'd like someone else to test your code :) and then we will merge it!

@danielstjules

Copy link
Copy Markdown
Contributor Author

@benjaminvialle I'm not sure why you're getting one failed test. When I run the tests, ie using bundle exec rake test:functionals TEST=test/functional/api/users_controller_test.rb I get the following output:

daniel:Markus danielstjules$ bundle exec rake test:functionals TEST=test/functional/api/users_controller_test.rb
Faraday: you may want to install system_timer for reliable timeouts
DEPRECATION WARNING: config.action_view.debug_rjs will be removed in 3.1, from 3.1 onwards you will need to install prototype-rails to continue to use RJS templates . (called from /Users/danielstjules/Markus/config/environment.rb:9)
Faraday: you may want to install system_timer for reliable timeouts
Loaded suite /Users/danielstjules/.rvm/gems/ruby-1.8.7-p371@global/gems/rake-10.0.3/lib/rake/rake_test_loader
Started
.................................
Finished in 2.448195 seconds.

33 tests, 118 assertions, 0 failures, 0 errors

Any idea what's going on?

@danielstjules

Copy link
Copy Markdown
Contributor Author

I haven't been able to reproduce any errors with that test condition. The code in question is:

  should 'ignore invalid filters' do
    get 'index', :filter => 'type:student,badfilter:invalid'
    assert_response :success
    assert_select 'user', 3
    assert @response.body.include?(@new_user1.user_name)
    assert @response.body.include?(@new_user2.user_name)
    assert @response.body.include?(@new_user3.user_name)
  end

And it's within the following context:

# Testing GET api/users
context 'testing index function' do
  # Create three test accounts
  setup do
    @new_user1 = Student.create(:user_name => 'ApiTestStudent1',
      :last_name => 'ApiTesters', :first_name => 'ApiTesting1')
    @new_user2 = Student.create(:user_name => "ApiTestStudent2",
      :last_name => 'ApiTesters', :first_name => 'ApiTesting2')
    @new_user3 = Student.create(:user_name => 'ApiTestStudent3',
      :last_name => 'ApiTesters3', :first_name => 'ApiTesting3')
  end

The idea is that all 3 users should be returned in the result's body as badfiler:invalid will be ignored, and all 3 users are of type student. No tests within that context modify those users, so I don't know why it would fail. A manual curl request imitating the test case also works for me:

curl -H 'Authorization: MarkUsAuth YourAuthKeyHere' "http://localhost:3000/api/users.xml?filter=type:student,badfilter:invalid"

What version of Ruby are you using, and Rails?

@m-wu

m-wu commented Feb 27, 2013

Copy link
Copy Markdown
Contributor

I ran the above tests and they all passed for me too:

mike@ubuntu:~/markus/Markus$ bundle exec rake test:functionals TEST=test/functional/api/users_controller_test.rb
Faraday: you may want to install system_timer for reliable timeouts
DEPRECATION WARNING: config.action_view.debug_rjs will be removed in 3.1, from 3.1 onwards you will need to install prototype-rails to continue to use RJS templates . (called from /home/mike/markus/Markus/config/environment.rb:9)
Faraday: you may want to install system_timer for reliable timeouts
Loaded suite /home/mike/.rvm/gems/ruby-1.8.7-p371@global/gems/rake-10.0.3/lib/rake/rake_test_loader
Started
.................................
Finished in 6.013658 seconds.

33 tests, 118 assertions, 0 failures, 0 errors

@benjaminvialle

Copy link
Copy Markdown
Member

Hi @danielstjules ,

I was using Ruby 1.9.3-p194 and RoR 3.0.19.

I will test this code again this evening. Stay tuned ;)

@danielstjules

Copy link
Copy Markdown
Contributor Author

Thanks! I'm unable to test on 1.9.3 as I couldn't build the svn ruby bindings on OSX using that version. I believe swig fails during make. It looks like both Mike and I are using 1.8.7-p371 though.

@danielstjules

Copy link
Copy Markdown
Contributor Author

@benjaminvialle Don't mean to bother you, but did you try running the tests again? And any chance you could run that curl request and see if you get any results?

I'm gonna reinstall Fedora and try building the svn ruby bindings with 1.9.3 on it to test it myself as well.

@benjaminvialle

Copy link
Copy Markdown
Member

@danielstjules I have done the tests again. They all paa. Sorry for the noise. Your patch is ready to be merged for me .

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Spelling error

@m-bodmer

m-bodmer commented Mar 4, 2013

Copy link
Copy Markdown
Contributor

Other than spelling errors I commented on, pull request looks good!

@danielstjules

Copy link
Copy Markdown
Contributor Author

@m-bodmer @benjaminvialle Thanks! I've got the spellchecker enabled now lol

reidka added a commit that referenced this pull request Mar 6, 2013
@reidka reidka merged commit 2b80b5b into MarkUsProject:master Mar 6, 2013
@jerboaa

jerboaa commented Mar 6, 2013

Copy link
Copy Markdown
Member

@danielstjules Please also update the wiki page as to how to use the new API if you get a chance. I expect this changed and we are lacking user documentation for it anyway :) Thanks!

@danielstjules

Copy link
Copy Markdown
Contributor Author

Don't worry - it's part of the long-term plan :) I've got some rough documentation already written up, and will probably make an update to the wiki later this month.

@danielstjules danielstjules deleted the issue-1002 branch March 6, 2013 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants