Merge branch 'zj-env-external-url' into 'master'

Add an URL field to Enviroments

## What does this MR do?

Adds a field to the `enviroments` table to expose later in other features. Now I see the task list below, I noticed I forgot some minor things, but Ill adress those after the first review. 

## Are there points in the code the reviewer needs to double check?

The field is a string on the database, thus limited to 255 chars, which seems more than enough.

## What are the relevant issue numbers?

Closes #19527

## Screenshots (if relevant)

![Screen_Shot_2016-07-25_at_15.22.16](/uploads/5c3f39466f8e2114195270006aed20c0/Screen_Shot_2016-07-25_at_15.22.16.png)![Screen_Shot_2016-07-25_at_15.22.53](/uploads/408cf86610137dd4a861d15dcd677a2c/Screen_Shot_2016-07-25_at_15.22.53.png)![Screen_Shot_2016-07-25_at_15.23.10](/uploads/927c506931f4e0338cdbbc58678bd308/Screen_Shot_2016-07-25_at_15.23.10.png)

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5469

CHANGELOG

@@ -10,6 +10,7 @@ v 8.11.0 (unreleased)
   - Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable
   - Optimize maximum user access level lookup in loading of notes
   - Add "No one can push" as an option for protected branches. !5081
+  - Environments have an url to link to
   - Limit git rev-list output count to one in forced push check
   - Clean up unused routes (Josef Strzibny)
   - Add green outline to New Branch button. !5447 (winniehell)

app/controllers/projects/environments_controller.rb

@@ -2,8 +2,8 @@ class Projects::EnvironmentsController < Projects::ApplicationController
   layout 'project'
   before_action :authorize_read_environment!
   before_action :authorize_create_environment!, only: [:new, :create]
-  before_action :authorize_update_environment!, only: [:destroy]
-  before_action :environment, only: [:show, :destroy]
+  before_action :authorize_update_environment!, only: [:edit, :update, :destroy]
+  before_action :environment, only: [:show, :edit, :update, :destroy]
 
   def index
     @environments = project.environments
@@ -17,13 +17,24 @@ class Projects::EnvironmentsController < Projects::ApplicationController
     @environment = project.environments.new
   end
 
+  def edit
+  end
+
   def create
-    @environment = project.environments.create(create_params)
+    @environment = project.environments.create(environment_params)
 
     if @environment.persisted?
       redirect_to namespace_project_environment_path(project.namespace, project, @environment)
     else
-      render 'new'
+      render :new
+    end
+  end
+
+  def update
+    if @environment.update(environment_params)
+      redirect_to namespace_project_environment_path(project.namespace, project, @environment)
+    else
+      render :edit
     end
   end
 
@@ -39,8 +50,8 @@ class Projects::EnvironmentsController < Projects::ApplicationController
 
   private
 
-  def create_params
-    params.require(:environment).permit(:name)
+  def environment_params
+    params.require(:environment).permit(:name, :external_url)
   end
 
   def environment

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 },
@@ -10,7 +12,17 @@ class Environment < ActiveRecord::Base
             format: { with: Gitlab::Regex.environment_name_regex,
                       message: Gitlab::Regex.environment_name_regex_message }
 
+  validates :external_url,
+            uniqueness: { scope: :project_id },
+            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

app/views/projects/environments/_form.html.haml

-= form_for @environment, url: namespace_project_environments_path(@project.namespace, @project), html: { class: 'col-lg-9' } do |f|
+.row.prepend-top-default.append-bottom-default
+  .col-lg-3
+    %h4.prepend-top-0
+      Environments
+    %p
+      Environments allow you to track deployments of your application
+      = succeed "." do
+        = link_to "Read more about environments", help_page_path("ci/environments")
+
+  = form_for [@project.namespace.becomes(Namespace), @project, @environment], html: { class: 'col-lg-9' } do |f|
     = form_errors(@environment)
+
     .form-group
       = f.label :name, 'Name', class: 'label-light'
       = f.text_field :name, required: true, class: 'form-control'
-  = f.submit 'Create environment', class: 'btn btn-create'
+    .form-group
+      = f.label :external_url, 'External URL', class: 'label-light'
+      = f.url_field :external_url, class: 'form-control'
+
+    .form-actions
+      = f.submit 'Save', class: 'btn btn-save'
       = link_to 'Cancel', namespace_project_environments_path(@project.namespace, @project), class: 'btn btn-cancel'

app/views/projects/environments/edit.html.haml

+- page_title "Edit", @environment.name, "Environments"
+
+%h3.page-title
+  Edit environment
+%hr
+= render 'form'

app/views/projects/environments/new.html.haml

 - page_title 'New Environment'
 
-.row.prepend-top-default.append-bottom-default
-  .col-lg-3
-    %h4.prepend-top-0
-      New Environment
-    %p
-      Environments allow you to track deployments of your application
-      = succeed "." do
-        = link_to "Read more about environments", help_page_path("ci/environments")
-
-  = render 'form'
+%h3.page-title
+  New environment
+%hr
+= render 'form'

app/views/projects/environments/show.html.haml

@@ -6,10 +6,10 @@
   .top-area
     .col-md-9
       %h3.page-title= @environment.name.capitalize
-
     .col-md-3
       .nav-controls
         - if can?(current_user, :update_environment, @environment)
+          = link_to 'Edit', edit_namespace_project_environment_path(@project.namespace, @project, @environment), class: 'btn'
           = link_to 'Destroy', namespace_project_environment_path(@project.namespace, @project, @environment), data: { confirm: 'Are you sure you want to delete this environment?' }, class: 'btn btn-danger', method: :delete
 
   - if @deployments.blank?

config/routes.rb

@@ -741,7 +741,7 @@ Rails.application.routes.draw do
           end
         end
 
-        resources :environments, only: [:index, :show, :new, :create, :destroy]
+        resources :environments
 
         resources :builds, only: [:index, :show], constraints: { id: /\d+/ } do
           collection do

db/migrate/20160725083350_add_external_url_to_enviroments.rb

+class AddExternalUrlToEnviroments < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column(:environments, :external_url, :string)
+  end
+end

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"
@@ -430,6 +430,7 @@ ActiveRecord::Schema.define(version: 20160722221922) do
     t.string   "name",         null: false
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.string   "external_url"
   end
 
   add_index "environments", ["project_id", "name"], name: "index_environments_on_project_id_and_name", using: :btree

doc/api/enviroments.md

+# Environments
+
+## List environments
+
+Get all environments for a given project.
+
+```
+GET /projects/:id/environments
+```
+
+| Attribute | Type    | Required | Description           |
+| --------- | ------- | -------- | --------------------- |
+| `id`      | integer | yes      | The ID of the project |
+
+```bash
+curl -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/projects/1/environments
+```
+
+Example response:
+
+```json
+[
+  {
+    "id": 1,
+    "name": "Env1",
+    "external_url": "https://env1.example.gitlab.com"
+  }
+]
+```
+
+## Create a new environment
+
+Creates a new environment with the given name and external_url.
+
+It returns 201 if the environment was successfully created, 400 for wrong parameters.
+
+```
+POST /projects/:id/environment
+```
+
+| Attribute     | Type    | Required | Description                  |
+| ------------- | ------- | -------- | ---------------------------- |
+| `id`          | integer | yes      | The ID of the project        |
+| `name`        | string  | yes      | The name of the 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"
+```
+
+Example response:
+
+```json
+{
+  "id": 1,
+  "name": "deploy",
+  "external_url": "https://deploy.example.gitlab.com"
+}
+```
+
+## Edit an existing environment
+
+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.
+
+```
+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  | 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 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:
+
+```json
+{
+  "id": 1,
+  "name": "staging",
+  "external_url": "https://staging.example.gitlab.com"
+}
+```
+
+## Delete an environment
+
+It returns 200 if the environment was successfully deleted, and 404 if the environment does not exist.
+
+```
+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 |
+
+```bash
+curl -X DELETE -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/environment/1"
+```
+
+Example response:
+
+```json
+{
+  "id": 1,
+  "name": "deploy",
+  "external_url": "https://deploy.example.gitlab.com"
+}
+```

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?
@@ -32,6 +36,7 @@ module API
     mount ::API::CommitStatuses
     mount ::API::Commits
     mount ::API::DeployKeys
+    mount ::API::Environments
     mount ::API::Files
     mount ::API::GroupMembers
     mount ::API::Groups

lib/api/entities.rb

@@ -496,6 +496,10 @@ module API
       expose :key, :value
     end
 
+    class Environment < Grape::Entity
+      expose :id, :name, :external_url
+    end
+
     class RepoLicense < Grape::Entity
       expose :key, :name, :nickname
       expose :featured, as: :popular

lib/api/environments.rb

+module API
+  # Environments RESTfull API endpoints
+  class Environments < Grape::API
+    before { authenticate! }
+
+    params do
+      requires :id, type: String, desc: 'The project ID'
+    end
+    resource :projects do
+      desc 'Get all environments of the project' do
+        detail 'This feature was introduced in GitLab 8.11.'
+        success Entities::Environment
+      end
+      params do
+        optional :page,     type: Integer, desc: 'Page number of the current request'
+        optional :per_page, type: Integer, desc: 'Number of items per page'
+      end
+      get ':id/environments' do
+        authorize! :read_environment, user_project
+
+        present paginate(user_project.environments), with: Entities::Environment
+      end
+
+      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
+
+        create_params = declared(params, include_parent_namespaces: false).to_h
+        environment = user_project.environments.create(create_params)
+
+        if environment.persisted?
+          present environment, with: Entities::Environment
+        else
+          render_validation_error!(environment)
+        end
+      end
+
+      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
+      put ':id/environments/:environment_id' do
+        authorize! :update_environment, user_project
+
+        environment = user_project.environments.find(params[:environment_id])
+        
+        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

spec/controllers/projects/environments_controller_spec.rb

+require 'spec_helper'
+
+describe Projects::EnvironmentsController do
+  let(:environment) { create(:environment) }
+  let(:project)     { environment.project }
+  let(:user)        { create(:user) }
+
+  before do
+    project.team << [user, :master]
+
+    sign_in(user)
+  end
+
+  describe 'GET show' do
+    context 'with valid id' do
+      it 'responds with a status code 200' do
+        get :show, environment_params
+
+        expect(response).to be_ok
+      end
+    end
+
+    context 'with invalid id' do
+      it 'responds with a status code 404' do
+        params = environment_params
+        params[:id] = 12345
+        get :show, params
+
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
+
+  describe 'GET edit' do
+    it 'responds with a status code 200' do
+      get :edit, environment_params
+
+      expect(response).to be_ok
+    end
+  end
+
+  describe 'PATCH #update' do
+    it 'responds with a 302' do
+      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

spec/factories/environments.rb

@@ -3,5 +3,6 @@ FactoryGirl.define do
     sequence(:name) { |n| "environment#{n}" }
 
     project factory: :empty_project
+    sequence(:external_url) { |n| "https://env#{n}.example.gitlab.com" }
   end
 end

spec/features/environments_spec.rb

@@ -140,7 +140,7 @@ feature 'Environments', feature: true do
       context 'for valid name' do
         before do
           fill_in('Name', with: 'production')
-          click_on 'Create environment'
+          click_on 'Save'
         end
 
         scenario 'does create a new pipeline' do
@@ -151,7 +151,7 @@ feature 'Environments', feature: true do
       context 'for invalid name' do
         before do
           fill_in('Name', with: 'name with spaces')
-          click_on 'Create environment'
+          click_on 'Save'
         end
 
         scenario 'does show errors' do

spec/models/environment_spec.rb

@@ -11,4 +11,23 @@ describe Environment, models: true do
   it { is_expected.to validate_presence_of(:name) }
   it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) }
   it { is_expected.to validate_length_of(:name).is_within(0..255) }
+
+  it { is_expected.to validate_length_of(:external_url).is_within(0..255) }
+
+  # To circumvent a not null violation of the name column:
+  # https://github.com/thoughtbot/shoulda-matchers/issues/336
+  it 'validates uniqueness of :external_url' do
+    create(:environment)
+
+    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
+      expect(env.external_url).to be_nil
+    end
+  end
 end

spec/requests/api/environments_spec.rb

+require 'spec_helper'
+
+describe API::API, api: true  do
+  include ApiHelpers
+
+  let(:user)          { create(:user) }
+  let(:non_member)    { create(:user) }
+  let(:project)       { create(:project, :private, namespace: user.namespace) }
+  let!(:environment)  { create(:environment, project: project) }
+
+  before do
+    project.team << [user, :master]
+  end
+
+  describe 'GET /projects/:id/environments' do
+    context 'as member of the project' do
+      it_behaves_like 'a paginated resources' do
+        let(:request) { get api("/projects/#{project.id}/environments", user) }
+      end
+
+      it 'returns project environments' do
+        get api("/projects/#{project.id}/environments", user)
+
+        expect(response).to have_http_status(200)
+        expect(json_response).to be_an Array
+        expect(json_response.size).to eq(1)
+        expect(json_response.first['name']).to eq(environment.name)
+        expect(json_response.first['external_url']).to eq(environment.external_url)
+      end
+    end
+
+    context 'as non member' do
+      it 'returns a 404 status code' do
+        get api("/projects/#{project.id}/environments", non_member)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
+
+  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"
+
+        expect(response).to have_http_status(201)
+        expect(json_response['name']).to eq('mepmep')
+        expect(json_response['external']).to be nil
+      end
+
+      it 'requires name to be passed' do
+        post api("/projects/#{project.id}/environments", user), external_url: 'test.gitlab.com'
+
+        expect(response).to have_http_status(400)
+      end
+
+      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)
+      end
+    end
+
+    context 'a non member' do
+      it 'rejects the request' do
+        post api("/projects/#{project.id}/environments", non_member), name: 'gitlab.com'
+
+        expect(response).to have_http_status(404)
+      end
+
+      it 'returns a 400 when the required params are missing' do
+        post api("/projects/12345/environments", non_member), external_url: 'http://env.git.com'
+      end
+    end
+  end
+
+  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(200)
+      expect(json_response['name']).to eq('Mepmep')
+      expect(json_response['external_url']).to eq(url)
+    end
+
+    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'
+
+      expect(response).to have_http_status(200)
+      expect(json_response['name']).to eq('Mepmep')
+      expect(json_response['external_url']).to eq(url)
+    end
+
+    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
+
+    context 'a non member' do
+      it 'rejects the request' do
+        delete api("/projects/#{project.id}/environments/#{environment.id}", non_member)
+
+        expect(response).to have_http_status(404)
+      end
+    end
+  end
+end