Updated to fix issues risen during feedback

Correctly updates the on-screen CI text feedback

4 files changed, 53 insertions, 67 deletions

diff --git a/app/assets/javascripts/lib/notify.js.coffee b/app/assets/javascripts/lib/notify.js.coffee
index bd409faba95..3f9ca39912c 100644
--- a/app/assets/javascripts/lib/notify.js.coffee
+++ b/app/assets/javascripts/lib/notify.js.coffee
@@ -1,10 +1,15 @@
 ((w) ->
+  notificationGranted = (message, opts, onclick) ->
+    notification = new Notification(message, opts)
+
+    if onclick
+      notification.onclick = onclick
+
   notifyPermissions = ->
     if 'Notification' of window
       Notification.requestPermission()
 
   notifyMe = (message, body, icon, onclick) ->
-    notification = undefined
     opts =
       body: body
       icon: icon
@@ -13,22 +18,13 @@
       # do nothing
     else if Notification.permission == 'granted'
       # If it's okay let's create a notification
-      notification = new Notification(message, opts)
-
-      if onclick
-        notification.onclick = onclick
+      notificationGranted message, opts, onclick
     else if Notification.permission != 'denied'
       Notification.requestPermission (permission) ->
         # If the user accepts, let's create a notification
         if permission == 'granted'
-          notification = new Notification(message, opts)
-
-          if onclick
-            notification.onclick = onclick
-        return
-    return
+          notificationGranted message, opts, onclick
 
   w.notify = notifyMe
   w.notifyPermissions = notifyPermissions
-  return
 ) window
diff --git a/app/assets/javascripts/merge_request_widget.js.coffee b/app/assets/javascripts/merge_request_widget.js.coffee
index b74b8c21fd5..0bb95e92158 100644
--- a/app/assets/javascripts/merge_request_widget.js.coffee
+++ b/app/assets/javascripts/merge_request_widget.js.coffee
@@ -2,8 +2,8 @@ class @MergeRequestWidget
   # Initialize MergeRequestWidget behavior
   #
   #   check_enable           - Boolean, whether to check automerge status
-  #   url_to_automerge_check - String, URL to use to check automerge status
-  #   url_to_ci_check        - String, URL to use to check CI status
+  #   merge_check_url - String, URL to use to check automerge status
+  #   ci_status_url        - String, URL to use to check CI status
   #
 
   constructor: (@opts) ->
@@ -31,7 +31,7 @@ class @MergeRequestWidget
       dataType: 'json'
 
   getMergeStatus: ->
-    $.get @opts.url_to_automerge_check, (data) ->
+    $.get @opts.merge_check_url, (data) ->
       $('.mr-state-widget').replaceWith(data)
 
   ciLabelForStatus: (status) ->
@@ -41,26 +41,28 @@ class @MergeRequestWidget
       status
 
   getCIStatus: ->
-    urlToCICheck = @opts.url_to_ci_check
     _this = @
-
     @fetchBuildStatusInterval = setInterval ( =>
       return if not @readyForCICheck
 
-      $.getJSON urlToCICheck, (data) =>
+      $.getJSON @opts.ci_status_url, (data) =>
         @readyForCICheck = true
 
         if @firstCICheck
           @firstCICheck = false
-          @opts.current_status = data.status
+          @opts.ci_status = data.status
+
+        if data.status isnt @opts.ci_status
+          @showCIState data.status
+          if data.coverage
+            @showCICoverage data.coverage
 
-        if data.status isnt @opts.current_status
           message = @opts.ci_message.replace('{{status}}', @ciLabelForStatus(data.status))
           message = message.replace('{{sha}}', data.sha)
           message = message.replace('{{title}}', data.title)
 
           notify(
-            "Build #{_this.ciLabelForStatus(data.status)}",
+            "Build #{@ciLabelForStatus(data.status)}",
             message,
             @opts.gitlab_icon,
             ->
@@ -68,19 +70,19 @@ class @MergeRequestWidget
               Turbolinks.visit _this.opts.builds_path
           )
 
-          @opts.current_status = data.status
+          @opts.ci_status = data.status
 
       @readyForCICheck = false
     ), 5000
 
-  getCiStatus: ->
-    $.get @opts.url_to_ci_check, (data) =>
-      this.showCiState data.status
+  getCIState: ->
+    $('.ci-widget-fetching').show()
+    $.getJSON @opts.ci_status_url, (data) =>
+      @showCIState data.status
       if data.coverage
-        this.showCiCoverage data.coverage
-    , 'json'
+        @showCICoverage data.coverage
 
-  showCiState: (state) ->
+  showCIState: (state) ->
     $('.ci_widget').hide()
     allowed_states = ["failed", "canceled", "running", "pending", "success", "skipped", "not_found"]
     if state in allowed_states
@@ -94,7 +96,7 @@ class @MergeRequestWidget
       $('.ci_widget.ci-error').show()
       @setMergeButtonClass('btn-danger')
 
-  showCiCoverage: (coverage) ->
+  showCICoverage: (coverage) ->
     text = 'Coverage ' + coverage + '%'
     $('.ci_widget:visible .ci-coverage').text(text)
 
diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml
index ccb2f9fa77e..2ee8e2de0e8 100644
--- a/app/views/projects/merge_requests/widget/_heading.html.haml
+++ b/app/views/projects/merge_requests/widget/_heading.html.haml
@@ -1,23 +1,12 @@
-- if @ci_commit
-  .mr-widget-heading
-    .ci_widget{class: "ci-#{@ci_commit.status}"}
-      = ci_status_icon(@ci_commit)
-      %span
-        Build
-        %span.ci-status-label
-          = ci_status_label(@ci_commit)
-      for
-      = succeed "." do
-        = link_to @ci_commit.short_sha, namespace_project_commit_path(@merge_request.source_project.namespace, @merge_request.source_project, @ci_commit.sha), class: "monospace"
-      %span.ci-coverage
-      = link_to "View details", builds_namespace_project_merge_request_path(@project.namespace, @project, @merge_request), class: "js-show-tab", data: {action: 'builds'}
-
-- elsif @merge_request.has_ci?
-  - # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX
-  - # Remove in later versions when services like Jenkins will set CI status via Commit status API
+- if @ci_commit or @merge_request.has_ci?
   .mr-widget-heading
+    - if @merge_request.has_ci?
+      .ci_widget.ci-widget-fetching
+        = icon('spinner spin')
+        %span
+          Checking CI status for #{@merge_request.last_commit_short_sha}…
     - %w[success skipped canceled failed running pending].each do |status|
-      .ci_widget{class: "ci-#{status}", style: "display:none"}
+      .ci_widget{ class: "ci-#{status}", style: ("display:none" unless status == @ci_commit.status) }
         = ci_icon_for_status(status)
         %span
           CI build
@@ -27,22 +16,20 @@
         = succeed "." do
           = link_to commit.short_id, namespace_project_commit_path(@merge_request.source_project.namespace, @merge_request.source_project, commit), class: "monospace"
         %span.ci-coverage
-        - if details_path = ci_build_details_path(@merge_request)
+        - if details_path = builds_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)
           = link_to "View details", details_path, :"data-no-turbolink" => "data-no-turbolink"
+    - if @merge_request.has_ci?
+      - # Compatibility with old CI integrations (ex jenkins) when you request status from CI server via AJAX
+      - # Remove in later versions when services like Jenkins will set CI status via Commit status API
+      .ci_widget.ci-not_found{style: "display:none"}
+        = icon("times-circle")
+        Could not find CI status for #{@merge_request.last_commit_short_sha}.
 
-    .ci_widget
-      = icon("spinner spin")
-      Checking CI status for #{@merge_request.last_commit_short_sha}…
-
-    .ci_widget.ci-not_found{style: "display:none"}
-      = icon("times-circle")
-      Could not find CI status for #{@merge_request.last_commit_short_sha}.
-
-    .ci_widget.ci-error{style: "display:none"}
-      = icon("times-circle")
-      Could not connect to the CI server. Please check your settings and try again.
+      .ci_widget.ci-error{style: "display:none"}
+        = icon("times-circle")
+        Could not connect to the CI server. Please check your settings and try again.
 
-  :javascript
-    $(function() {
-      merge_request_widget.getCiStatus();
-    });
+      :javascript
+        $(function() {
+          merge_request_widget.getCIState();
+        });
diff --git a/app/views/projects/merge_requests/widget/_show.html.haml b/app/views/projects/merge_requests/widget/_show.html.haml
index 6507c534a02..2be06aebe6c 100644
--- a/app/views/projects/merge_requests/widget/_show.html.haml
+++ b/app/views/projects/merge_requests/widget/_show.html.haml
@@ -10,12 +10,13 @@
 :javascript
   var merge_request_widget;
   var opts = {
-    url_to_automerge_check: "#{merge_check_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)}",
+    merge_check_url: "#{merge_check_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)}",
     check_enable: #{@merge_request.unchecked? ? "true" : "false"},
-    url_to_ci_check: "#{ci_status_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)}",
+    ci_status_url: "#{ci_status_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)}",
     gitlab_icon: "#{asset_path 'gitlab_logo.png'}",
-    current_status: "",
-    ci_message: "Build {{status}} for {{title}}\n{{sha}}",
+    ci_status: "",
+    ci_message: "Build {{status}} for \"{{title}}\"",
+    ci_enable: #{@project.ci_service ? "true" : "false"},
     builds_path: "#{builds_namespace_project_merge_request_path(@project.namespace, @project, @merge_request)}"
   };