Incorporate feedback

8 files changed, 45 insertions, 29 deletions

diff --git a/app/models/environment.rb b/app/models/environment.rb
index 9eff0fdab03..baed106e8c8 100644
--- a/app/models/environment.rb
+++ b/app/models/environment.rb
@@ -3,6 +3,8 @@ class Environment < ActiveRecord::Base
 
   has_many :deployments
 
+  before_validation :nullify_external_url
+
   validates :name,
             presence: true,
             uniqueness: { scope: :project_id },
@@ -12,9 +14,15 @@ class Environment < ActiveRecord::Base
 
   validates :external_url,
             uniqueness: { scope: :project_id },
-            length: { maximum: 255 }
+            length: { maximum: 255 },
+            allow_nil: true,
+            addressable_url: true
 
   def last_deployment
     deployments.last
   end
+
+  def nullify_external_url
+    self.external_url = nil if self.external_url.blank?
+  end
 end
diff --git a/db/migrate/20160725083350_add_external_url_to_enviroments.rb b/db/migrate/20160725083350_add_external_url_to_enviroments.rb
index e887341159b..21a8abd310b 100644
--- a/db/migrate/20160725083350_add_external_url_to_enviroments.rb
+++ b/db/migrate/20160725083350_add_external_url_to_enviroments.rb
@@ -1,6 +1,3 @@
-# See http://doc.gitlab.com/ce/development/migration_style_guide.html
-# for more information on how to write migrations for GitLab.
-
 class AddExternalUrlToEnviroments < ActiveRecord::Migration
   include Gitlab::Database::MigrationHelpers
 
diff --git a/db/schema.rb b/db/schema.rb
index 4365af98962..5b35a528e71 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160722221922) do
+ActiveRecord::Schema.define(version: 20160726093600) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
diff --git a/doc/api/enviroments.md b/doc/api/enviroments.md
index c4b844fe77e..16bf2627fef 100644
--- a/doc/api/enviroments.md
+++ b/doc/api/enviroments.md
@@ -32,8 +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
-and 409 if the environment already exists.
+It returns 200 if the environment was successfully created, 400 for wrong parameters.
 
 ```
 POST /projects/:id/environment
@@ -43,7 +42,7 @@ POST /projects/:id/environment
 | ------------- | ------- | -------- | ---------------------------- |
 | `id`          | integer | yes      | The ID of the project        |
 | `name`        | string  | yes      | The name of the environment  |
-| `external_url` | string  | yes     | Place to link to for this environment |
+| `external_url` | string  | no     | Place to link to for this environment |
 
 ```bash
 curl --data "name=deploy&external_url=https://deploy.example.gitlab.com" -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environments"
@@ -88,7 +87,7 @@ Example response:
 
 ## Edit an existing environment
 
-Updates an existing environments name and/or external_url.
+Updates an existing environment's name and/or external_url.
 
 It returns 200 if the label was successfully updated, In case of an error, an additional error message is returned.
 
diff --git a/lib/api/environments.rb b/lib/api/environments.rb
index 66a047f72fc..532baec42c7 100644
--- a/lib/api/environments.rb
+++ b/lib/api/environments.rb
@@ -30,9 +30,6 @@ module API
         required_attributes! [:name]
 
         attrs = attributes_for_keys [:name, :external_url]
-        environment = user_project.environments.find_by(name: attrs[:name])
-
-        conflict!('Environment already exists') if environment
 
         environment = user_project.environments.create(attrs)
 
@@ -52,7 +49,7 @@ module API
       # Example Request:
       #   DELETE /projects/:id/environments/:environment_id
       delete ':id/environments/:environment_id' do
-        authorize! :admin_environment, user_project
+        authorize! :update_environment, user_project
 
         environment = user_project.environments.find(params[:environment_id])
 
diff --git a/spec/controllers/projects/environments_controller_spec.rb b/spec/controllers/projects/environments_controller_spec.rb
index b91a99d6b2e..768105cae95 100644
--- a/spec/controllers/projects/environments_controller_spec.rb
+++ b/spec/controllers/projects/environments_controller_spec.rb
@@ -11,12 +11,10 @@ describe Projects::EnvironmentsController do
     sign_in(user)
   end
 
-  render_views
-
   describe 'GET show' do
     context 'with valid id' do
       it 'responds with a status code 200' do
-        get :show, namespace_id: project.namespace, project_id: project, id: environment.id
+        get :show, environment_params
 
         expect(response).to be_ok
       end
@@ -24,16 +22,18 @@ describe Projects::EnvironmentsController do
 
     context 'with invalid id' do
       it 'responds with a status code 404' do
-        get :show, namespace_id: project.namespace, project_id: project, id: 12345
+        params = environment_params
+        params[:id] = 12345
+        get :show, params
 
-        expect(response).to be_not_found
+        expect(response).to have_http_status(404)
       end
     end
   end
 
   describe 'GET edit' do
     it 'responds with a status code 200' do
-      get :edit, namespace_id: project.namespace, project_id: project, id: environment.id
+      get :edit, environment_params
 
       expect(response).to be_ok
     end
@@ -41,10 +41,18 @@ describe Projects::EnvironmentsController do
 
   describe 'PATCH #update' do
     it 'responds with a 302' do
-      patch :update, namespace_id: project.namespace, project_id:
-                      project, id: environment.id, environment: { external_url: 'https://git.gitlab.com' }
+      patch_params = environment_params.merge(environment: { external_url: 'https://git.gitlab.com' })
+      patch :update, patch_params
 
       expect(response).to have_http_status(302)
     end
   end
+
+  def environment_params
+    {
+      namespace_id: project.namespace,
+      project_id: project,
+      id: environment.id
+    }
+  end
 end
diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb
index 6c11cfc4c9b..ef2148be1bd 100644
--- a/spec/models/environment_spec.rb
+++ b/spec/models/environment_spec.rb
@@ -21,4 +21,12 @@ describe Environment, models: true do
 
     is_expected.to validate_uniqueness_of(:external_url).scoped_to(:project_id)
   end
+
+  describe '#nullify_external_url' do
+    it 'replaces a blank url with nil' do
+      env = build(:environment, external_url: "")
+
+      expect(env.save).to be true
+    end
+  end
 end
diff --git a/spec/requests/api/environments_spec.rb b/spec/requests/api/environments_spec.rb
index 822139dbf3b..b731c58a206 100644
--- a/spec/requests/api/environments_spec.rb
+++ b/spec/requests/api/environments_spec.rb
@@ -5,7 +5,7 @@ describe API::API, api: true  do
 
   let(:user)          { create(:user) }
   let(:non_member)    { create(:user) }
-  let(:project)       { create(:project, :private, creator_id: user.id, namespace: user.namespace) }
+  let(:project)       { create(:project, :private, namespace: user.namespace) }
   let!(:environment)  { create(:environment, project: project) }
 
   before do
@@ -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 labels' do
+      it 'should return project environments' do
         get api("/projects/#{project.id}/environments", user)
 
         expect(response).to have_http_status(200)
@@ -34,7 +34,7 @@ describe API::API, api: true  do
     end
   end
 
-  describe 'POST /projects/:id/labels' do
+  describe 'POST /projects/:id/environments' do
     context 'as a member' do
       it 'creates a environment with valid params' do
         post api("/projects/#{project.id}/environments", user), name: "mepmep"
@@ -50,11 +50,10 @@ describe API::API, api: true  do
         expect(response).to have_http_status(400)
       end
 
-      it 'should return 409 if environment already exists' do
+      it 'should return 400 if environment already exists' do
         post api("/projects/#{project.id}/environments", user), name: environment.name
 
-        expect(response).to have_http_status(409)
-        expect(json_response['message']).to eq('Environment already exists')
+        expect(response).to have_http_status(400)
       end
     end
 
@@ -87,11 +86,11 @@ describe API::API, api: true  do
   describe 'PUT /projects/:id/environments/:environment_id' do
     it 'should return 200 if name and external_url are changed' do
       put api("/projects/#{project.id}/environments/#{environment.id}", user),
-          name: 'Mepmep', external_url: 'mepmep.whatever.ninja'
+          name: 'Mepmep', external_url: 'https://mepmep.whatever.ninja'
 
       expect(response).to have_http_status(200)
       expect(json_response['name']).to eq('Mepmep')
-      expect(json_response['external_url']).to eq('mepmep.whatever.ninja')
+      expect(json_response['external_url']).to eq('https://mepmep.whatever.ninja')
     end
 
     it 'should return 404 if the environment does not exist' do