fixes #2969 - remove all legacy api code in UI controllers + fixes #2985 remove #as_json + fixes #2986 remove #show #855

isratrade · 2013-08-25T15:23:19Z

I left the json code in the following controllers as they don't appear in the API code

StatisticsController
TasksController

Also, the json code used by the UI was of course not removed

ohadlevy · 2013-08-29T06:16:31Z

app/controllers/autosign_controller.rb

@@ -8,10 +8,7 @@ def index
      autosign = []
      error e
    end
-    respond_to do |format|
-      format.html { @autosign = autosign.paginate :page => params[:page], :per_page => 20 }
-      format.json {render :json => autosign }


i dont think we have that over the API, can you add a ticket to add that?

@ohadlevy, yes this was added as http://projects.theforeman.org/issues/2983

ohadlevy · 2013-08-29T06:20:54Z

two minor comments

please remove the routes where you removed the show action
should we remove the as_json from the model as well?

isratrade · 2013-08-29T06:56:50Z

@ohadlevy, yes I will also remove the as_json from the models as well. My assumption is these were only used for the API. I will check if any changes after the ajax dropdown lists.

isratrade · 2013-08-29T08:05:47Z

@ohadlevy, re-committed.

reverted autosign_controller to include json
removed #as_json methods
removed #show methods and routes when not needed

isratrade · 2013-08-29T08:08:51Z

@domcleal, @ohadlevy, if I split out fixes #2985 or #2986 into separate PR's, all the json tests removed in fixes #2969 will fail.

ohadlevy · 2013-08-29T08:55:15Z

@isratrade it seems to be failing, can you have a look?

isratrade · 2013-09-02T08:34:23Z

app/views/api/v1/hosts/show.json.rabl

@@ -1,12 +1,16 @@
 object @host

-attributes :name, :id, :ip, :environment, :last_report, :updated_at, :created_at, :mac,


:environment was moved below to a child node, since it caused an error "no method key?", I assume since the as_json was removed from the Environment model.

isratrade · 2013-09-02T08:38:41Z

I added issue #3006 - Add to API v1 /api/hosts/disabled as /hosts/disabled with json response no longer works

domcleal · 2013-09-02T14:17:34Z

test/functional/compute_resources_controller_test.rb

@@ -6,23 +6,6 @@ class ComputeResourcesControllerTest < ActionController::TestCase
    @your_compute_resource = compute_resources(:yourcompute)
  end

-  test "should not get index when not permitted" do


we should keep this test, but remove the JSON bit of the request

domcleal · 2013-09-02T14:30:55Z

This will wait for Greg's #858 as there's going to be a very small conflict on the reports model as_json I think, it'll be merged shortly.

domcleal · 2013-09-06T19:04:40Z

Could you rebase this please @isratrade? Thanks!

isratrade · 2013-09-08T08:37:46Z

@domcleal, rebased

domcleal · 2013-09-08T08:43:59Z

@isratrade did you see the comments about tests? Some of them could be quite useful if made non-JSON specific.

isratrade · 2013-09-09T08:36:50Z

@domcleal, @ohadlevy, re-committed with fixes in last commit that I will squash after your review.
Btw, I didn't remove the as_json in report.rb or log.rb as I know @GregSutcliffe's recent PR modified these. Should they be removed and re-constructed in api/v1

domcleal · 2013-09-09T09:25:40Z

yes, I think they should be removed

isratrade · 2013-09-09T09:29:52Z

rebased and recommited with as_json from log.rb and report.rb removed

ohadlevy · 2013-09-09T09:38:01Z

@isratrade I'm getting two errors:

inished tests in 109.746279s, 6.3328 tests/s, 11.6268 assertions/s.

  1) Failure:
Api::V1::HostsControllerTest#test_0019_if only authorize_login_delegation is set, REMOTE_USER should be
        ignored for API requests [/home/olevy/git/foreman/test/functional/api/v1/hosts_controller_test.rb:170]:
Expected response to be a <401>, but was <200>

  2) Failure:
Api::V1::HostsControllerTest#test_0020_if both authorize_login_delegation{,_api} are unset,
        REMOTE_USER should ignored in all cases [/home/olevy/git/foreman/test/functional/api/v1/hosts_controller_test.rb:184]:
Expected response to be a <401>, but was <200>

GregSutcliffe · 2013-09-09T10:03:05Z

this looks good from the Reports api viewpoint now, thanks @isratrade

ohadlevy · 2013-09-09T11:01:24Z

I think we need to remove the api handling from the application_controller
as well.

isratrade · 2013-09-09T13:42:57Z

@ohadlevy, @domcleal, recommitted

added separate commit for deprecation notice
removed json test from settings_controller_test
modified test_audit_comment test in conflig_templates_controller_test

isratrade · 2013-09-09T15:08:16Z

+1

domcleal · 2013-09-09T16:18:25Z

@isratrade this needs a rebase now, because of the change to the hosts API rabl, thanks!

isratrade · 2013-09-09T17:56:09Z

app/views/api/v1/hosts/show.json.rabl

@@ -1,6 +1,6 @@
 object @host

-attributes :name, :id, :ip, :environment_id, :environment, :last_report, :updated_at, :created_at, :mac,


@domcleal, rebased with conflicts resolved in hosts#show json. :environment was removed since there is no as_json method. It was replaced by child :environment in line #19 to keep consistent with UI controller output for v1.

domcleal · 2013-09-10T09:46:13Z

I consider the "warning" in 62e6c84 less of a warning and more of a final error.. could we just use a normal error response?

…abl views used for API

isratrade · 2013-09-10T14:44:50Z

@domcleal, I'm not sure this is what you wanted but the response for json calls to a UI controller is

{
    "message": "You must add /api/ to the beginning on your URI such as 0.0.0.0:3000/api/environments"
}

I need to create the json manually, since I'm in the UI ApplicationController controller and don't have access to the render_error method in Api::BaseController. However, render_error just calls one of several templates in api/v1/errors (or v2) that just have a simple response with node(:message). I also added to the log an error message with the real exception ActionView::MissingTemplate that I rescued from

isratrade · 2013-09-10T15:08:46Z

recommitted. Returns status 400 and includes actual exception (ActionView::MissingTemplate) in logger.error in addition to deprecation message

domcleal · 2013-09-10T15:47:37Z

Thanks @isratrade, that's much better. Unfortunately I can't get it to work, do you see this bug, or what's happening?

$ git co -
HEAD is now at f5ec783... fixes #2969 - add deprecation response for api calls to UI routes
$ curl -s -u admin:changeme -H 'Accept: application/json' "http://0.0.0.0:3000/hosts/iridium.redhat.com"
 $

(a single space in the output)

Started GET "/hosts/iridium.redhat.com" for 127.0.0.1 at 2013-09-10 16:45:27 +0100
Processing by HostsController#show as JSON
  Parameters: {"id"=>"iridium.redhat.com"}
  User Load (0.1ms)  SELECT "users".* FROM "users" WHERE "users"."login" = 'admin' LIMIT 1
  AuthSource Load (0.1ms)  SELECT "auth_sources".* FROM "auth_sources" WHERE "auth_sources"."id" = 1 LIMIT 1
  User Load (0.2ms)  SELECT "users".* FROM "users" WHERE (login='admin') LIMIT 1
Authenticated user Admin User against INTERNAL authentication source
  User Load (0.1ms)  SELECT "users".* FROM "users" WHERE "users"."login" = 'admin' ORDER BY firstname LIMIT 1
Setting current user thread-local variable to admin
   (0.0ms)  begin transaction
   (0.2ms)  UPDATE "users" SET "last_login_on" = '2013-09-10 15:45:29.651730', "updated_at" = '2013-09-10 15:45:29.652912' WHERE "users"."id" = 1
   (33.3ms)  commit transaction
  Role Load (0.1ms)  SELECT "roles".* FROM "roles" WHERE "roles"."name" = 'Anonymous' LIMIT 1
  Role Exists (0.1ms)  SELECT 1 AS one FROM "roles" INNER JOIN "user_roles" ON "roles"."id" = "user_roles"."role_id" WHERE "user_roles"."user_id" = 1 AND "roles"."id" = 8 LIMIT 1
Setting current user thread-local variable to admin
Setting current user thread-local variable to nil
  User Load (0.1ms)  SELECT "users".* FROM "users" WHERE "users"."login" = 'admin' LIMIT 1
Authorized user admin(Admin User)
Setting current user thread-local variable to admin
  Host::Base Load (0.1ms)  SELECT "hosts".* FROM "hosts" WHERE "hosts"."name" = 'iridium.redhat.com' LIMIT 1
Completed 406 Not Acceptable in 389ms (ActiveRecord: 35.7ms)

isratrade · 2013-09-10T16:12:15Z

@domcleal, I'm also getting 406 for /hosts or hosts/:id and not sure why. I was testing with /domains and /environments.

There's another issue that came up. I can't rescue_from ActionController::RoutingError
rails/rails#671
unless I use a catch-all route and we didn't want to do this since it messes up engine routes.

So for show actions such as domains/12, I get a routing error because :show was removed from routes.rb when it's not used.

I don't have a good solution, since there are some places in the UI that uses JSON, so we can't capture all JSON requests.

@ohadlevy, any ideas

lzap · 2013-09-10T16:15:40Z

This pull request is considered as blocker for Foreman 1.3 release.

…son output that was removed

…routes

isratrade · 2013-09-10T18:20:28Z

I removed commit 2b52e77 for issue #2986 that removed the show method and show in routes.rb when not used. I will re-commit this at a later stage after we don't need the deprecation message any more on UI controllers.
Also, the 406 error for hosts and some other controllers was because of NO format.json when there IS a respond_to block. If there is NO respond_to block, then it's OK. So, I added format.json where necessary only for the purposes to avoid 406 error and get the deprecation error json response.

domcleal · 2013-09-10T21:16:03Z

Good find, it worked properly in my test.

Thanks for the patches @isratrade, merged in as 9dc1001..bc89eb3 for Foreman 1.3.

ohadlevy reviewed Aug 29, 2013
View reviewed changes

isratrade reviewed Sep 2, 2013
View reviewed changes

domcleal reviewed Sep 2, 2013
View reviewed changes

ohadlevy mentioned this pull request Sep 8, 2013

fixes #2741 - rails 3 syntax #801

Closed

isratrade reviewed Sep 9, 2013
View reviewed changes

Joseph Mitchell Magen added 2 commits September 10, 2013 17:17

fixes theforeman#2969 - remove all legacy api code in UI controllers

92a3d7a

fixes theforeman#2985 - remove #as_json method from AR models since r…

9a20200

…abl views used for API

Joseph Mitchell Magen added 2 commits September 10, 2013 20:59

fixes theforeman#3052 - reports#show json rabl template to match as_j…

0b35f32

…son output that was removed

fixes theforeman#2969 - add deprecation response for api calls to UI …

08ebf12

…routes

domcleal closed this Sep 10, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #2969 - remove all legacy api code in UI controllers + fixes #2985 remove #as_json + fixes #2986 remove #show #855

fixes #2969 - remove all legacy api code in UI controllers + fixes #2985 remove #as_json + fixes #2986 remove #show #855

isratrade commented Aug 25, 2013

ohadlevy Aug 29, 2013

isratrade Sep 9, 2013

ohadlevy Sep 9, 2013

ohadlevy commented Aug 29, 2013

isratrade commented Aug 29, 2013

isratrade commented Aug 29, 2013

isratrade commented Aug 29, 2013

ohadlevy commented Aug 29, 2013

isratrade Sep 2, 2013

isratrade commented Sep 2, 2013

domcleal Sep 2, 2013

domcleal commented Sep 2, 2013

domcleal commented Sep 6, 2013

isratrade commented Sep 8, 2013

domcleal commented Sep 8, 2013

isratrade commented Sep 9, 2013

domcleal commented Sep 9, 2013

isratrade commented Sep 9, 2013

ohadlevy commented Sep 9, 2013

GregSutcliffe commented Sep 9, 2013

ohadlevy commented Sep 9, 2013

isratrade commented Sep 9, 2013

isratrade commented Sep 9, 2013

domcleal commented Sep 9, 2013

isratrade Sep 9, 2013

domcleal commented Sep 10, 2013

isratrade commented Sep 10, 2013

isratrade commented Sep 10, 2013

domcleal commented Sep 10, 2013

isratrade commented Sep 10, 2013

lzap commented Sep 10, 2013

isratrade commented Sep 10, 2013

domcleal commented Sep 10, 2013

		@@ -1,12 +1,16 @@
		object @host

		attributes :name, :id, :ip, :environment, :last_report, :updated_at, :created_at, :mac,

		@@ -1,6 +1,6 @@
		object @host

		attributes :name, :id, :ip, :environment_id, :environment, :last_report, :updated_at, :created_at, :mac,

fixes #2969 - remove all legacy api code in UI controllers + fixes #2985 remove #as_json + fixes #2986 remove #show #855

fixes #2969 - remove all legacy api code in UI controllers + fixes #2985 remove #as_json + fixes #2986 remove #show #855

Conversation

isratrade commented Aug 25, 2013

ohadlevy Aug 29, 2013

Choose a reason for hiding this comment

isratrade Sep 9, 2013

Choose a reason for hiding this comment

ohadlevy Sep 9, 2013

Choose a reason for hiding this comment

ohadlevy commented Aug 29, 2013

isratrade commented Aug 29, 2013

isratrade commented Aug 29, 2013

isratrade commented Aug 29, 2013

ohadlevy commented Aug 29, 2013

isratrade Sep 2, 2013

Choose a reason for hiding this comment

isratrade commented Sep 2, 2013

domcleal Sep 2, 2013

Choose a reason for hiding this comment

domcleal commented Sep 2, 2013

domcleal commented Sep 6, 2013

isratrade commented Sep 8, 2013

domcleal commented Sep 8, 2013

isratrade commented Sep 9, 2013

domcleal commented Sep 9, 2013

isratrade commented Sep 9, 2013

ohadlevy commented Sep 9, 2013

GregSutcliffe commented Sep 9, 2013

ohadlevy commented Sep 9, 2013

isratrade commented Sep 9, 2013

isratrade commented Sep 9, 2013

domcleal commented Sep 9, 2013

isratrade Sep 9, 2013

Choose a reason for hiding this comment

domcleal commented Sep 10, 2013

isratrade commented Sep 10, 2013

isratrade commented Sep 10, 2013

domcleal commented Sep 10, 2013

isratrade commented Sep 10, 2013

lzap commented Sep 10, 2013

isratrade commented Sep 10, 2013

domcleal commented Sep 10, 2013