Improve design after review

app/controllers/projects/environments_controller.rb

@@ -19,20 +19,22 @@ class Projects::EnvironmentsController < Projects::ApplicationController
 
   def create
     @environment = project.environments.create(create_params)
-    unless @environment.persisted?
+
+    if @environment.persisted?
+      redirect_to namespace_project_environment_path(project.namespace, project, @environment)
+    else
       render 'new'
-      return
     end
-
-    redirect_to namespace_project_environment_path(project.namespace, project, @environment)
   end
 
   def destroy
     if @environment.destroy
-      redirect_to namespace_project_environments_path(project.namespace, project), notice: 'Environment was successfully removed.'
+      flash[:notice] = 'Environment was successfully removed.'
     else
-      redirect_to namespace_project_environments_path(project.namespace, project), alert: 'Failed to remove environment.'
+      flash[:alert] = 'Failed to remove environment.'
     end
+
+    redirect_to namespace_project_environments_path(project.namespace, project)
   end
 
   private
@@ -42,7 +44,6 @@ class Projects::EnvironmentsController < Projects::ApplicationController
   end
 
   def environment
-    @environment ||= project.environments.find(params[:id].to_s)
-    @environment || render_404
+    @environment ||= project.environments.find_by!(id: params[:id])
   end
 end

app/models/ability.rb

@@ -18,6 +18,8 @@ class Ability
       when Namespace then namespace_abilities(user, subject)
       when GroupMember then group_member_abilities(user, subject)
       when ProjectMember then project_member_abilities(user, subject)
+      when Deployment then deployment_abilities(user, subject)
+      when Environment then environment_abilities(user, subject)
       when User then user_abilities
       else []
       end.concat(global_abilities(user))
@@ -249,9 +251,7 @@ class Ability
         :create_container_image,
         :update_container_image,
         :create_environment,
-        :update_environment,
-        :create_deployment,
-        :update_deployment,
+        :create_deployment
       ]
     end
 
@@ -269,6 +269,8 @@ class Ability
       @project_master_rules ||= project_dev_rules + [
         :push_code_to_protected_branches,
         :update_project_snippet,
+        :update_environment,
+        :update_deployment,
         :admin_milestone,
         :admin_project_snippet,
         :admin_project_member,
@@ -525,6 +527,14 @@ class Ability
       project_abilities(user, subject.project)
     end
 
+    def deployment_abilities(user, subject)
+      project_abilities(user, subject.project)
+    end
+
+    def environment_abilities(user, subject)
+      project_abilities(user, subject.project)
+    end
+
     private
 
     def restricted_public_level?

app/models/ci/build.rb

@@ -81,7 +81,8 @@ module Ci
         if build.environment.present?
           service = CreateDeploymentService.new(build.project, build.user,
                                                 environment: build.environment,
-                                                sha: build.sha, ref: build.ref,
+                                                sha: build.sha,
+                                                ref: build.ref,
                                                 tag: build.tag)
           service.execute(build)
         end

app/models/deployment.rb

@@ -6,10 +6,10 @@ class Deployment < ActiveRecord::Base
   belongs_to :user
   belongs_to :deployable, polymorphic: true
 
-  validates_presence_of :sha
-  validates_presence_of :ref
-  validates_associated :project
-  validates_associated :environment
+  validates :sha, presence: true
+  validates :ref, presence: true
+  validates :project, associated: true
+  validates :environment, associated: true
 
   delegate :name, to: :environment, prefix: true
 
@@ -22,7 +22,7 @@ class Deployment < ActiveRecord::Base
   end
 
   def short_sha
-    Commit::truncate_sha(sha)
+    Commit.truncate_sha(sha)
   end
 
   def last?

app/models/environment.rb

@@ -5,13 +5,12 @@ class Environment < ActiveRecord::Base
 
   validates :name,
             presence: true,
+            uniqueness: { scope: :project_id },
             length: { within: 0..255 },
             format: { with: Gitlab::Regex.environment_name_regex,
                       message: Gitlab::Regex.environment_name_regex_message }
 
-  validates_uniqueness_of :name, scope: :project_id
-
-  validates_associated :project
+  validates :project, associated: true
 
   def last_deployment
     deployments.last

app/services/create_deployment_service.rb

@@ -2,7 +2,9 @@ require_relative 'base_service'
 
 class CreateDeploymentService < BaseService
   def execute(deployable = nil)
-    environment = create_or_find_environment(params[:environment])
+    environment = project.environments.find_or_create_by(
+      name: params[:environment]
+    )
 
     project.deployments.create(
       environment: environment,
@@ -10,21 +12,7 @@ class CreateDeploymentService < BaseService
       tag: params[:tag],
       sha: params[:sha],
       user: current_user,
-      deployable: deployable,
+      deployable: deployable
     )
   end
-
-  private
-
-  def create_or_find_environment(environment)
-    find_environment(environment) || create_environment(environment)
-  end
-
-  def create_environment(environment)
-    project.environments.create(name: environment)
-  end
-
-  def find_environment(environment)
-    project.environments.find_by(name: environment)
-  end
 end

app/views/layouts/nav/_project.html.haml

@@ -40,7 +40,7 @@
             Code
 
     - if project_nav_tab? :pipelines
-      = nav_link(controller: :pipelines) do
+      = nav_link(controller: [:pipelines, :builds, :environments]) do
         = link_to project_pipelines_path(@project), title: 'Pipelines', class: 'shortcuts-pipelines' do
           %span
             Pipelines

app/views/projects/deployments/_commit.html.haml

+%div.branch-commit
+  - if deployment.ref
+    = link_to deployment.ref, namespace_project_commits_path(@project.namespace, @project, deployment.ref), class: "monospace"
+    ·
+  = link_to deployment.short_sha, namespace_project_commit_path(@project.namespace, @project, deployment.sha), class: "commit-id monospace"
+
+  %p.commit-title
+    %span
+      - if commit_title = deployment.commit_title
+        = link_to_gfm commit_title, namespace_project_commit_path(@project.namespace, @project, deployment.sha), class: "commit-row-message"
+      - else
+        Cant find HEAD commit for this branch

app/views/projects/deployments/_deployment.html.haml

@@ -3,29 +3,18 @@
     %strong= "##{deployment.iid}"
 
   %td
-    %div.branch-commit
-      - if deployment.ref
-        = link_to deployment.ref, namespace_project_commits_path(@project.namespace, @project, deployment.ref), class: "monospace"
-        ·
-      = link_to deployment.short_sha, namespace_project_commit_path(@project.namespace, @project, deployment.sha), class: "commit-id monospace"
-
-      %p.commit-title
-        %span
-          - if commit_title = deployment.commit_title
-            = link_to_gfm commit_title, namespace_project_commit_path(@project.namespace, @project, deployment.sha), class: "commit-row-message"
-          - else
-            Cant find HEAD commit for this branch
+    = render 'projects/deployments/commit', deployment: deployment
 
   %td
     - if deployment.deployable
-      = link_to namespace_project_build_path(@project.namespace, @project, deployment.deployable), class: "monospace" do
+      = link_to namespace_project_build_path(@project.namespace, @project, deployment.deployable) do
         = "#{deployment.deployable.name} (##{deployment.deployable.id})"
 
   %td
     #{time_ago_with_tooltip(deployment.created_at)}
 
   %td
-    - if can?(current_user, :update_deployment, @project) && deployment.deployable
+    - if can?(current_user, :update_deployment, deployment) && deployment.deployable
       .pull-right
         = link_to retry_namespace_project_build_path(@project.namespace, @project, deployment.deployable), method: :post, class: 'btn btn-build' do
           - if deployment.last?

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

@@ -7,18 +7,7 @@
 
   %td
     - if last_deployment
-      %div.branch-commit
-        - if last_deployment.ref
-          = link_to last_deployment.ref, namespace_project_commits_path(@project.namespace, @project, last_deployment.ref), class: "monospace"
-          ·
-        = link_to last_deployment.short_sha, namespace_project_commit_path(@project.namespace, @project, last_deployment.sha), class: "commit-id monospace"
-
-        %p.commit-title
-          %span
-            - if commit_title = last_deployment.commit_title
-              = link_to_gfm commit_title, namespace_project_commit_path(@project.namespace, @project, last_deployment.sha), class: "commit-row-message"
-            - else
-              Cant find HEAD commit for this branch
+      = render 'projects/deployments/commit', deployment: last_deployment
     - else
       %p.commit-title
         No deployments yet

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

@@ -20,5 +20,4 @@
           %th Environment
           %th Last deployment
           %th Date
-        - @environments.each do |environment|
-          = render 'environment', environment: environment
+        = render @environments

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

+- @no_container = true
 - page_title "New Environment"
 = render "projects/pipelines/head"
 
@@ -6,7 +7,7 @@
     %h4.prepend-top-0
       New Environment
 
-  = form_for @environment, url: namespace_project_environments_path(@project.namespace, @project), html: { id: "new-environment-form", class: "col-lg-9 js-new-environment-form js-requires-input" } do |f|
+  = form_for @environment, url: namespace_project_environments_path(@project.namespace, @project), html: { class: "col-lg-9" } do |f|
     = form_errors(@environment)
     .form-group
       = f.label :name, 'Environment name', class: 'label-light'

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

@@ -9,7 +9,7 @@
 
     .col-md-3
       .nav-controls
-        - if can?(current_user, :update_environment, @project)
+        - if can?(current_user, :update_environment, @environment)
           = link_to 'Destroy', namespace_project_environment_path(@project.namespace, @project, @environment), data: { confirm: 'Are you sure?' }, class: 'btn btn-danger', method: :delete
 
   - if @deployments.blank?

doc/permissions/permissions.md

@@ -28,7 +28,7 @@ documentation](../workflow/add-user/add-user.md).
 | Manage labels                         |         | ✓          | ✓           | ✓        | ✓      |
 | See a commit status                   |         | ✓          | ✓           | ✓        | ✓      |
 | See a container registry              |         | ✓          | ✓           | ✓        | ✓      |
-| See a environments                    |         | ✓          | ✓           | ✓        | ✓      |
+| See environments                      |         | ✓          | ✓           | ✓        | ✓      |
 | Manage merge requests                 |         |            | ✓           | ✓        | ✓      |
 | Create new merge request              |         |            | ✓           | ✓        | ✓      |
 | Create new branches                   |         |            | ✓           | ✓        | ✓      |
@@ -41,7 +41,7 @@ documentation](../workflow/add-user/add-user.md).
 | Create or update commit status        |         |            | ✓           | ✓        | ✓      |
 | Update a container registry           |         |            | ✓           | ✓        | ✓      |
 | Remove a container registry image     |         |            | ✓           | ✓        | ✓      |
-| Manage environments                   |         |            | ✓           | ✓        | ✓      |
+| Create new environments               |         |            | ✓           | ✓        | ✓      |
 | Create new milestones                 |         |            |             | ✓        | ✓      |
 | Add new team members                  |         |            |             | ✓        | ✓      |
 | Push to protected branches            |         |            |             | ✓        | ✓      |
@@ -54,6 +54,7 @@ documentation](../workflow/add-user/add-user.md).
 | Manage runners                        |         |            |             | ✓        | ✓      |
 | Manage build triggers                 |         |            |             | ✓        | ✓      |
 | Manage variables                      |         |            |             | ✓        | ✓      |
+| Delete environments                   |         |            |             | ✓        | ✓      |
 | Switch visibility level               |         |            |             |          | ✓      |
 | Transfer project to another namespace |         |            |             |          | ✓      |
 | Remove project                        |         |            |             |          | ✓      |