Merge branch 'text-batch-1' into 'master'

Batch 1 of text improvements

Batch 1 of changes from my effort at !635 to walk through every piece of text in GitLab and see if it can be improved.

This batch includes:

- Improve text on error pages.
- Improve Git access error messages.
- Improve description of branch protection levels.
- Improve OAuth signup error message.
- Improve OAuth application flash messages.

cc @rspeicher

See merge request !642

app/controllers/omniauth_callbacks_controller.rb

@@ -65,8 +65,15 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
         redirect_to omniauth_error_path(oauth['provider'], error: error_message) and return
       end
     end
-  rescue Gitlab::OAuth::ForbiddenAction => e
-    flash[:notice] = e.message
+  rescue Gitlab::OAuth::SignupDisabledError => e
+    message = "Signing in using your #{oauth['provider']} account without a pre-existing GitLab account is not allowed."
+
+    if current_application_settings.signup_enabled?
+      message << " Create a GitLab account first, and then connect it to your #{oauth['provider']} account."
+    end
+
+    flash[:notice] = message
+    
     redirect_to new_user_session_path
   end
 

config/locales/doorkeeper.en.yml

@@ -31,7 +31,7 @@ en:
       messages:
         # Common error messages
         invalid_request: 'The request is missing a required parameter, includes an unsupported parameter value, or is otherwise malformed.'
-        invalid_redirect_uri: 'The redirect uri included is not valid.'
+        invalid_redirect_uri: 'The redirect URI included is not valid.'
         unauthorized_client: 'The client is not authorized to perform this request using this method.'
         access_denied: 'The resource owner or authorization server denied the request.'
         invalid_scope: 'The requested scope is invalid, unknown, or malformed.'
@@ -63,11 +63,11 @@ en:
     flash:
       applications:
         create:
-          notice: 'Application created.'
+          notice: 'The application was created successfully.'
         destroy:
-          notice: 'Application deleted.'
+          notice: 'The application was deleted successfully.'
         update:
-          notice: 'Application updated.'
+          notice: 'The application was updated successfully.'
       authorized_applications:
         destroy:
-          notice: 'Application revoked.'
+          notice: 'The application was revoked access.'

lib/api/internal.rb

@@ -24,10 +24,6 @@ module API
             User.find_by(id: params[:user_id])
           end
 
-        unless actor
-          return Gitlab::GitAccessStatus.new(false, 'No such user or key')
-        end
-
         project_path = params[:project]
         
         # Check for *.wiki repositories.
@@ -39,22 +35,14 @@ module API
 
         project = Project.find_with_namespace(project_path)
 
-        if project
-          access =
-            if wiki
-              Gitlab::GitAccessWiki.new(actor, project)
-            else
-              Gitlab::GitAccess.new(actor, project)
-            end
-
-          status = access.check(params[:action], params[:changes])
-        end
+        access =
+          if wiki
+            Gitlab::GitAccessWiki.new(actor, project)
+          else
+            Gitlab::GitAccess.new(actor, project)
+          end
 
-        if project && access.can_read_project?
-          status
-        else
-          Gitlab::GitAccessStatus.new(false, 'No such project')
-        end
+        access.check(params[:action], params[:changes])
       end
 
       #

lib/gitlab/access.rb

@@ -51,9 +51,9 @@ module Gitlab
 
       def protection_options
         {
-          "Not protected, developers and masters can (force) push and delete the branch" => PROTECTION_NONE,
-          "Partially protected, developers can also push but prevent all force pushes and deletion" => PROTECTION_DEV_CAN_PUSH,
-          "Fully protected, only masters can push and prevent all force pushes and deletion" => PROTECTION_FULL,
+          "Not protected: Both developers and masters can push new commits, force push, or delete the branch." => PROTECTION_NONE,
+          "Partially protected: Developers can push new commits, but cannot force push or delete the branch. Masters can do all of those." => PROTECTION_DEV_CAN_PUSH,
+          "Fully protected: Developers cannot push new commits, force push, or delete the branch. Only masters can do any of those." => PROTECTION_FULL,
         }
       end
 

lib/gitlab/git_access.rb

@@ -31,8 +31,7 @@ module Gitlab
     def can_push_to_branch?(ref)
       return false unless user
 
-      if project.protected_branch?(ref)  &&
-          !(project.developers_can_push_to_protected_branch?(ref) && project.team.developer?(user))
+      if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref)
         user.can?(:push_code_to_protected_branches, project)
       else
         user.can?(:push_code, project)
@@ -50,13 +49,25 @@ module Gitlab
     end
 
     def check(cmd, changes = nil)
+      unless actor
+        return build_status_object(false, "No user or key was provided.")
+      end
+
+      if user && !user_allowed?
+        return build_status_object(false, "Your account has been blocked.")
+      end
+
+      unless project && can_read_project?
+        return build_status_object(false, 'The project you were looking for could not be found.')
+      end
+
       case cmd
       when *DOWNLOAD_COMMANDS
         download_access_check
       when *PUSH_COMMANDS
         push_access_check(changes)
       else
-        build_status_object(false, "Wrong command")
+        build_status_object(false, "The command you're trying to execute is not allowed.")
       end
     end
 
@@ -64,7 +75,7 @@ module Gitlab
       if user
         user_download_access_check
       elsif deploy_key
-        deploy_key_download_access_check
+        build_status_object(true)
       else
         raise 'Wrong actor'
       end
@@ -74,39 +85,27 @@ module Gitlab
       if user
         user_push_access_check(changes)
       elsif deploy_key
-        build_status_object(false, "Deploy key not allowed to push")
+        build_status_object(false, "Deploy keys are not allowed to push code.")
       else
         raise 'Wrong actor'
       end
     end
 
     def user_download_access_check
-      if user && user_allowed? && user.can?(:download_code, project)
-        build_status_object(true)
-      else
-        build_status_object(false, "You don't have access")
+      unless user.can?(:download_code, project)
+        return build_status_object(false, "You are not allowed to download code from this project.")
       end
-    end
 
-    def deploy_key_download_access_check
-      if can_read_project?
-        build_status_object(true)
-      else
-        build_status_object(false, "Deploy key not allowed to access this project")
-      end
+      build_status_object(true)
     end
 
     def user_push_access_check(changes)
-      unless user && user_allowed?
-        return build_status_object(false, "You don't have access")
-      end
-
       if changes.blank?
         return build_status_object(true)
       end
 
       unless project.repository.exists?
-        return build_status_object(false, "Repository does not exist")
+        return build_status_object(false, "A repository for this project does not exist yet.")
       end
 
       changes = changes.lines if changes.kind_of?(String)
@@ -136,11 +135,24 @@ module Gitlab
           :push_code
         end
 
-      if user.can?(action, project)
-        build_status_object(true)
-      else
-        build_status_object(false, "You don't have permission")
+      unless user.can?(action, project)
+        status =
+          case action
+          when :force_push_code_to_protected_branches
+            build_status_object(false, "You are not allowed to force push code to a protected branch on this project.")
+          when :remove_protected_branches
+            build_status_object(false, "You are not allowed to deleted protected branches from this project.")
+          when :push_code_to_protected_branches
+            build_status_object(false, "You are not allowed to push code to protected branches on this project.")
+          when :admin_project
+            build_status_object(false, "You are not allowed to change existing tags on this project.")
+          else # :push_code
+            build_status_object(false, "You are not allowed to push code to this project.")
+          end 
+        return status
       end
+
+      build_status_object(true)
     end
 
     def forced_push?(oldrev, newrev)

lib/gitlab/git_access_wiki.rb

@@ -4,7 +4,7 @@ module Gitlab
       if user.can?(:write_wiki, project)
         build_status_object(true)
       else
-        build_status_object(false, "You don't have access")
+        build_status_object(false, "You are not allowed to write to this project's wiki.")
       end
     end
   end

lib/gitlab/o_auth/user.rb

@@ -5,7 +5,7 @@
 #
 module Gitlab
   module OAuth
-    class ForbiddenAction < StandardError; end
+    class SignupDisabledError < StandardError; end
 
     class User
       attr_accessor :auth_hash, :gl_user
@@ -99,7 +99,7 @@ module Gitlab
       end
 
       def unauthorized_to_create
-        raise ForbiddenAction.new("Unauthorized to create user, signup disabled for #{auth_hash.provider}")
+        raise SignupDisabledError
       end
     end
   end

public/404.html

 <!DOCTYPE html>
 <html>
 <head>
-  <title>The page you were looking for doesn't exist (404)</title>
+  <title>The page you're looking for could not be found (404)</title>
   <link href="/static.css" media="screen" rel="stylesheet" type="text/css" />
 </head>
 
 <body>
   <h1>404</h1>
-  <h3>The page you were looking for doesn't exist.</h3>
+  <h3>The page you're looking for could not be found.</h3>
   <hr/>
-  <p>You may have mistyped the address or the page may have moved.</p>
+  <p>Make sure the address is correct and that the page hasn't moved.</p>
+  <p>Please contact your GitLab administrator if you think this is a mistake.</p>
 </body>
 </html>

public/422.html

 <!DOCTYPE html>
 <html>
 <head>
-  <title>The change you wanted was rejected (422)</title>
+  <title>The change you requested was rejected (422)</title>
   <link href="/static.css" media="screen" rel="stylesheet" type="text/css" />
 </head>
 
 <body>
   <!-- This file lives in public/422.html -->
   <h1>422</h1>
-  <div>
-    <h2>The change you wanted was rejected.</h2>
-    <p>Maybe you tried to change something you didn't have access to.</p>
-  </div>
+  <h3>The change you requested was rejected.</h3>
+  <hr />
+  <p>Make sure you have access to the thing you tried to change.</p>
+  <p>Please contact your GitLab administrator if you think this is a mistake.</p>
 </body>
 </html>

public/500.html

 <!DOCTYPE html>
 <html>
 <head>
-  <title>We're sorry, but something went wrong (500)</title>
+  <title>Something went wrong (500)</title>
   <link href="/static.css" media="screen" rel="stylesheet" type="text/css" />
 </head>
 <body>
   <h1>500</h1>
-  <h3>We're sorry, but something went wrong.</h3>
+  <h3>Whoops, something went wrong on our end.</h3>
   <hr/>
+  <p>Try refreshing the page, or going back and attempting the action again.</p>
   <p>Please contact your GitLab administrator if this problem persists.</p>
 </body>
 </html>

public/502.html

@@ -6,8 +6,9 @@
 </head>
 <body>
   <h1>502</h1>
-  <h3>GitLab is not responding.</h3>
+  <h3>Whoops, GitLab is taking too much time to respond.</h3>
   <hr/>
+  <p>Try refreshing the page, or going back and attempting the action again.</p>
   <p>Please contact your GitLab administrator if this problem persists.</p>
 </body>
 </html>

public/deploy.html

 <!DOCTYPE html>
 <html>
   <head>
-    <title>Deploy in progress. Please try again in a few minutes</title>
+    <title>Deploy in progress</title>
     <link href="/static.css" media="screen" rel="stylesheet" type="text/css" />
   </head>
+
   <body>
-    <h1><center><img src="/gitlab_logo.png"/></center>Deploy in progress</h1>
-    <h3>Please try again in a few minutes or contact your administrator.</h3>
+    <h1>
+      <img src="/gitlab_logo.png" /><br />
+      Deploy in progress
+    </h1>
+    <h3>Please try again in a few minutes.</h3>
+    <hr/>
+    <p>Please contact your GitLab administrator if this problem persists.</p>
   </body>
 </html>

public/static.css

@@ -2,18 +2,24 @@ body {
   color: #666;
   text-align: center;
   font-family: "Helvetica Neue", Helvetica, Arial, sans-serif;
-  margin:0;
+  margin: 0;
   width: 800px;
   margin: auto;
   font-size: 14px;
 }
+
 h1 {
   font-size: 56px;
   line-height: 100px;
   font-weight: normal;
   color: #456;
 }
-h2 { font-size: 24px; color: #666; line-height: 1.5em; }
+
+h2 { 
+  font-size: 24px; 
+  color: #666; 
+  line-height: 1.5em;
+}
 
 h3 {
   color: #456;

spec/lib/gitlab/git_access_spec.rb

@@ -115,18 +115,10 @@ describe Gitlab::GitAccess do
       let(:actor) { key }
 
       context 'pull code' do
-        context 'allowed' do
-          before { key.projects << project }
-          subject { access.download_access_check }
-
-          it { expect(subject.allowed?).to be_truthy }
-        end
-
-        context 'denied' do
-          subject { access.download_access_check }
+        before { key.projects << project }
+        subject { access.download_access_check }
 
-          it { expect(subject.allowed?).to be_falsey }
-        end
+        it { expect(subject.allowed?).to be_truthy }
       end
     end
   end