Use Grape DSL for environment endpoints

Also a couple of minor edits for this branch are included

6 files changed, 108 insertions, 97 deletions

diff --git a/config/routes.rb b/config/routes.rb
index ced204be7f7..371eb4bee7f 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -741,7 +741,7 @@ Rails.application.routes.draw do
           end
         end
 
-        resources :environments, constraints: { id: /\d+/ }
+        resources :environments
 
         resources :builds, only: [:index, :show], constraints: { id: /\d+/ } do
           collection do
diff --git a/doc/api/enviroments.md b/doc/api/enviroments.md
index 16bf2627fef..1e12ded448c 100644
--- a/doc/api/enviroments.md
+++ b/doc/api/enviroments.md
@@ -32,7 +32,7 @@ Example response:
 
 Creates a new environment with the given name and external_url.
 
-It returns 200 if the environment was successfully created, 400 for wrong parameters.
+It returns 201 if the environment was successfully created, 400 for wrong parameters.
 
 ```
 POST /projects/:id/environment
@@ -58,21 +58,25 @@ Example response:
 }
 ```
 
-## Delete an environment
+## Edit an existing environment
 
-It returns 200 if the environment was successfully deleted, and 404 if the environment does not exist.
+Updates an existing environment's name and/or external_url.
+
+It returns 200 if the environment was successfully updated. In case of an error, a status code 400 is returned.
 
 ```
-DELETE /projects/:id/environments/:environment_id
+PUT /projects/:id/environments/:environments_id
 ```
 
-| Attribute | Type    | Required | Description           |
-| --------- | ------- | -------- | --------------------- |
-| `id` | integer | yes | The ID of the project |
-| `environment_id` | integer | yes | The ID of the environment |
+| Attribute       | Type    | Required                          | Description                      |
+| --------------- | ------- | --------------------------------- | -------------------------------  |
+| `id`            | integer | yes                               | The ID of the project            |
+| `environment_id` | integer | yes | The ID of the environment  | The ID of the environment        |
+| `name`          | string  | no                                | The new name of the environment  |
+| `external_url`  | string  | no                                | The new external_url             |
 
 ```bash
-curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environment/1"
+curl -X PUT --data "name=staging&external_url=https://staging.example.gitlab.com" -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environment/1"
 ```
 
 Example response:
@@ -80,30 +84,26 @@ Example response:
 ```json
 {
   "id": 1,
-  "name": "deploy",
-  "external_url": "https://deploy.example.gitlab.com"
+  "name": "staging",
+  "external_url": "https://staging.example.gitlab.com"
 }
 ```
 
-## Edit an existing environment
-
-Updates an existing environment's name and/or external_url.
+## Delete an environment
 
-It returns 200 if the label was successfully updated, In case of an error, an additional error message is returned.
+It returns 200 if the environment was successfully deleted, and 404 if the environment does not exist.
 
 ```
-PUT /projects/:id/environments/:environments_id
+DELETE /projects/:id/environments/:environment_id
 ```
 
-| Attribute       | Type    | Required                          | Description                      |
-| --------------- | ------- | --------------------------------- | -------------------------------  |
-| `id`            | integer | yes                               | The ID of the project            |
-| `environment_id` | integer | yes | The ID of the environment  | The ID of the environment        |
-| `name`          | string  | no                                | The new name of the environment  |
-| `external_url`  | string  | no                                | The new external_url             |
+| Attribute | Type    | Required | Description           |
+| --------- | ------- | -------- | --------------------- |
+| `id` | integer | yes | The ID of the project |
+| `environment_id` | integer | yes | The ID of the environment |
 
 ```bash
-curl -X PUT --data "name=staging&external_url=https://staging.example.gitlab.com" -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environment/1"
+curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environment/1"
 ```
 
 Example response:
@@ -111,7 +111,7 @@ Example response:
 ```json
 {
   "id": 1,
-  "name": "staging",
-  "external_url": "https://staging.example.gitlab.com"
+  "name": "deploy",
+  "external_url": "https://deploy.example.gitlab.com"
 }
 ```
diff --git a/lib/api/api.rb b/lib/api/api.rb
index 9c960d74495..bd16806892b 100644
--- a/lib/api/api.rb
+++ b/lib/api/api.rb
@@ -7,6 +7,10 @@ module API
       rack_response({ 'message' => '404 Not found' }.to_json, 404)
     end
 
+    rescue_from Grape::Exceptions::ValidationErrors do |e|
+      error!({ messages: e.full_messages }, 400)
+    end
+
     rescue_from :all do |exception|
       # lifted from https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/debug_exceptions.rb#L60
       # why is this not wrapped in something reusable?
diff --git a/lib/api/environments.rb b/lib/api/environments.rb
index 532baec42c7..a50f007d697 100644
--- a/lib/api/environments.rb
+++ b/lib/api/environments.rb
@@ -3,82 +3,77 @@ module API
   class Environments < Grape::API
     before { authenticate! }
 
+    params do
+      requires :id, type: String, desc: 'The project ID'
+    end
     resource :projects do
-      # Get all labels of the project
-      #
-      # Parameters:
-      #   id (required) - The ID of a project
-      # Example Request:
-      #   GET /projects/:id/environments
+      desc 'Get all environments of the project' do
+        detail 'This feature was introduced in GitLab 8.11.'
+        success Entities::Environment
+      end
       get ':id/environments' do
         authorize! :read_environment, user_project
 
         present paginate(user_project.environments), with: Entities::Environment
       end
 
-      # Creates a new environment
-      #
-      # Parameters:
-      #   id    (required)        - The ID of a project
-      #   name  (required)        - The name of the environment to be created
-      #   external_url (optional) - URL on which this deployment is viewable
-      #
-      # Example Request:
-      #   POST /projects/:id/labels
+      desc 'Creates a new environment' do
+        detail 'This feature was introduced in GitLab 8.11.'
+        success Entities::Environment
+      end
+      params do
+        requires :name,           type: String,   desc: 'The name of the environment to be created'
+        optional :external_url,   type: String,   desc: 'URL on which this deployment is viewable'
+      end
       post ':id/environments' do
         authorize! :create_environment, user_project
-        required_attributes! [:name]
-
-        attrs = attributes_for_keys [:name, :external_url]
 
-        environment = user_project.environments.create(attrs)
+        create_params = declared(params, include_parent_namespaces: false).to_h
+        environment = user_project.environments.create(create_params)
 
-        if environment.valid?
+        if environment.persisted?
           present environment, with: Entities::Environment
         else
           render_validation_error!(environment)
         end
       end
 
-      # Deletes an existing environment
-      #
-      # Parameters:
-      #   id    (required)          - The ID of a project
-      #   environment_id (required) - The name of the environment to be deleted
-      #
-      # Example Request:
-      #   DELETE /projects/:id/environments/:environment_id
-      delete ':id/environments/:environment_id' do
-        authorize! :update_environment, user_project
-
-        environment = user_project.environments.find(params[:environment_id])
-
-        present environment.destroy, with: Entities::Environment
+      desc 'Updates an existing environment' do
+        detail 'This feature was introduced in GitLab 8.11.'
+        success Entities::Environment
+      end
+      params do
+        requires :environment_id, type: Integer,  desc: 'The environment ID'
+        optional :name,           type: String,   desc: 'The new environment name'
+        optional :external_url,   type: String,   desc: 'The new URL on which this deployment is viewable'
       end
-
-      # Updates an existing environment
-      #
-      # Parameters:
-      #   id              (required) - The ID of a project
-      #   environment_id  (required) - The ID of the environment
-      #   name            (optional) - The name of the label to be deleted
-      #   external_url    (optional) - The new name of the label
-      #
-      # Example Request:
-      #   PUT /projects/:id/environments/:environment_id
       put ':id/environments/:environment_id' do
         authorize! :update_environment, user_project
 
         environment = user_project.environments.find(params[:environment_id])
 
-        attrs = attributes_for_keys [:name, :external_url]
-
-        if environment.update(attrs)
+        update_params = declared(params, include_missing: false).extract!(:name, :external_url).to_h
+        if environment.update(update_params)
           present environment, with: Entities::Environment
         else
           render_validation_error!(environment)
         end
       end
+
+      desc 'Deletes an existing environment' do
+        detail 'This feature was introduced in GitLab 8.11.'
+        success Entities::Environment
+      end
+      params do
+        requires :environment_id, type: Integer,  desc: 'The environment ID'
+      end
+      delete ':id/environments/:environment_id' do
+        authorize! :update_environment, user_project
+
+        environment = user_project.environments.find(params[:environment_id])
+
+        present environment.destroy, with: Entities::Environment
+      end
     end
   end
 end
diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb
index ef2148be1bd..8a84ac0a7c7 100644
--- a/spec/models/environment_spec.rb
+++ b/spec/models/environment_spec.rb
@@ -27,6 +27,7 @@ describe Environment, models: true do
       env = build(:environment, external_url: "")
 
       expect(env.save).to be true
+      expect(env.external_url).to be_nil
     end
   end
 end
diff --git a/spec/requests/api/environments_spec.rb b/spec/requests/api/environments_spec.rb
index b731c58a206..d315e456dda 100644
--- a/spec/requests/api/environments_spec.rb
+++ b/spec/requests/api/environments_spec.rb
@@ -14,7 +14,7 @@ describe API::API, api: true  do
 
   describe 'GET /projects/:id/environments' do
     context 'as member of the project' do
-      it 'should return project environments' do
+      it 'returns project environments' do
         get api("/projects/#{project.id}/environments", user)
 
         expect(response).to have_http_status(200)
@@ -26,7 +26,7 @@ describe API::API, api: true  do
     end
 
     context 'as non member' do
-      it 'should return a 404 status code' do
+      it 'returns a 404 status code' do
         get api("/projects/#{project.id}/environments", non_member)
 
         expect(response).to have_http_status(404)
@@ -50,7 +50,7 @@ describe API::API, api: true  do
         expect(response).to have_http_status(400)
       end
 
-      it 'should return 400 if environment already exists' do
+      it 'returns a 400 if environment already exists' do
         post api("/projects/#{project.id}/environments", user), name: environment.name
 
         expect(response).to have_http_status(400)
@@ -61,42 +61,53 @@ describe API::API, api: true  do
       it 'rejects the request' do
         post api("/projects/#{project.id}/environments", non_member)
 
-        expect(response).to have_http_status(404)
+        expect(response).to have_http_status(400)
       end
     end
   end
 
-  describe 'DELETE /projects/:id/environments/:environment_id' do
-    context 'as a master' do
-      it 'should return 200 for an existing environment' do
-        delete api("/projects/#{project.id}/environments/#{environment.id}", user)
-
-        expect(response).to have_http_status(200)
-      end
-
-      it 'should return 404 for non existing id' do
-        delete api("/projects/#{project.id}/environments/12345", user)
+  describe 'PUT /projects/:id/environments/:environment_id' do
+    it 'returns a 200 if name and external_url are changed' do
+      url = 'https://mepmep.whatever.ninja'
+      put api("/projects/#{project.id}/environments/#{environment.id}", user),
+          name: 'Mepmep', external_url: url
 
-        expect(response).to have_http_status(404)
-        expect(json_response['message']).to eq('404 Not found')
-      end
+      expect(response).to have_http_status(200)
+      expect(json_response['name']).to eq('Mepmep')
+      expect(json_response['external_url']).to eq(url)
     end
-  end
 
-  describe 'PUT /projects/:id/environments/:environment_id' do
-    it 'should return 200 if name and external_url are changed' do
+    it "won't update the external_url if only the name is passed" do
+      url = environment.external_url
       put api("/projects/#{project.id}/environments/#{environment.id}", user),
-          name: 'Mepmep', external_url: 'https://mepmep.whatever.ninja'
+          name: 'Mepmep'
 
       expect(response).to have_http_status(200)
       expect(json_response['name']).to eq('Mepmep')
-      expect(json_response['external_url']).to eq('https://mepmep.whatever.ninja')
+      expect(json_response['external_url']).to eq(url)
     end
 
-    it 'should return 404 if the environment does not exist' do
+    it 'returns a 404 if the environment does not exist' do
       put api("/projects/#{project.id}/environments/12345", user)
 
       expect(response).to have_http_status(404)
     end
   end
+
+  describe 'DELETE /projects/:id/environments/:environment_id' do
+    context 'as a master' do
+      it 'returns a 200 for an existing environment' do
+        delete api("/projects/#{project.id}/environments/#{environment.id}", user)
+
+        expect(response).to have_http_status(200)
+      end
+
+      it 'returns a 404 for non existing id' do
+        delete api("/projects/#{project.id}/environments/12345", user)
+
+        expect(response).to have_http_status(404)
+        expect(json_response['message']).to eq('404 Not found')
+      end
+    end
+  end
 end