Merge pull request #3830 from machisuji/feature/custom_first_login_redirect

drop my first_login; redirect to work packages if possible

app/controllers/account_controller.rb

@@ -30,6 +30,7 @@
 class AccountController < ApplicationController
   include CustomFieldsHelper
   include Concerns::OmniauthLogin
+  include Concerns::RedirectAfterLogin
 
   # prevents login action to be filtered by check_if_login_required application scope filter
   skip_before_filter :check_if_login_required
@@ -162,7 +163,7 @@ class AccountController < ApplicationController
 
       if user.save
         token.destroy
-        flash[:notice] = I18n.t(:notice_account_activated)
+        flash[:notice] = with_accessibility_notice I18n.t(:notice_account_activated)
       else
         flash[:error] = I18n.t(:notice_activation_failed)
       end
@@ -406,7 +407,7 @@ class AccountController < ApplicationController
     if @user.save
       session[:auth_source_registration] = nil
       self.logged_user = @user
-      flash[:notice] = l(:notice_account_activated)
+      flash[:notice] = with_accessibility_notice l(:notice_account_activated)
       redirect_to controller: '/my', action: 'account'
     end
     # Otherwise render register view again
@@ -477,7 +478,7 @@ class AccountController < ApplicationController
       self.logged_user = user
       opts[:after_login].call user if opts[:after_login]
 
-      flash[:notice] = l(:notice_account_registered_and_logged_in)
+      flash[:notice] = with_accessibility_notice l(:notice_account_registered_and_logged_in)
       redirect_after_login(user)
     else
       yield if block_given?
@@ -558,15 +559,6 @@ class AccountController < ApplicationController
     redirect_to action: 'login', back_url: params[:back_url]
   end
 
-  def redirect_after_login(user)
-    if user.first_login
-      user.update_attribute(:first_login, false)
-      redirect_to controller: '/my', action: 'first_login', back_url: params[:back_url]
-    else
-      redirect_back_or_default controller: '/my', action: 'page'
-    end
-  end
-
   def invited_user
     if session.include? :invitation_token
       token = Token.find_by(value: session[:invitation_token])
@@ -574,4 +566,10 @@ class AccountController < ApplicationController
       token.user
     end
   end
+
+  def with_accessibility_notice(text)
+    notice = link_translate(:notice_accessibility_mode, url: my_settings_url)
+
+    "#{text} #{notice}".html_safe
+  end
 end

app/controllers/concerns/redirect_after_login.rb

@@ -0,0 +1,73 @@
+#-- copyright
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
+##
+# Intended to be used by the AccountController to decide where to
+# send the user when they logged in.
+module Concerns::RedirectAfterLogin
+  def redirect_after_login(user)
+    if user.first_login
+      user.update_attribute(:first_login, false)
+
+      welcome_redirect
+    else
+      default_redirect
+    end
+  end
+
+  #    * * *
+
+  def welcome_redirect
+    project = welcome_project
+
+    if project && redirect_to_welcome_project?(current_user, project)
+      redirect_to welcome_redirect_url(project)
+    else
+      default_redirect
+    end
+  end
+
+  def welcome_redirect_url(project)
+    url_for controller: :work_packages, project_id: project.identifier
+  end
+
+  ##
+  # Only the first user as the creator of the OpenProject installation is
+  # supposed to be redirected like this.
+  def redirect_to_welcome_project?(user, _project)
+    User.not_builtin.count == 1 && user.admin?
+  end
+
+  def welcome_project
+    DemoData::ProjectSeeder::Data.find_demo_project
+  end
+
+  def default_redirect
+    redirect_back_or_default controller: '/my', action: 'page'
+  end
+end

app/controllers/my_controller.rb

@@ -122,20 +122,6 @@ class MyController < ApplicationController
     write_email_settings(redirect_to: :mail_notifications) if request.patch?
   end
 
-  def first_login
-    if request.get?
-      @user = User.current
-      @back_url = url_for(params[:back_url])
-
-    elsif request.post? || request.put?
-      User.current.pref.attributes = permitted_params.pref || {}
-      User.current.pref.save
-
-      flash[:notice] = l(:notice_account_updated)
-      redirect_back_or_default(controller: '/my', action: 'page')
-    end
-  end
-
   # Create a new feeds key
   def reset_rss_key
     if request.post?

app/seeders/demo_data/project_seeder.rb

@@ -70,16 +70,18 @@ module DemoData
     end
 
     def reset_demo_project
-      if delete_me = Project.find_by(identifier: I18n.t('seeders.demo_data.project.identifier'))
-        delete_me.destroy
+      delete_demo_project
+      create_demo_project
     end
 
-      Project.create!(
-        name:         I18n.t('seeders.demo_data.project.name'),
-        identifier:   I18n.t('seeders.demo_data.project.identifier'),
-        description:  I18n.t('seeders.demo_data.project.description'),
-        types:        Type.all
-      )
+    def create_demo_project
+      Project.create! project_data
+    end
+
+    def delete_demo_project
+      if delete_me = find_demo_project
+        delete_me.destroy
+      end
     end
 
     def set_members(project)
@@ -125,5 +127,40 @@ module DemoData
         description: I18n.t('seeders.demo_data.board.description')
       )
     end
+
+    module Data
+      module_function
+
+      def project_data
+        {
+          name:         project_name,
+          identifier:   project_identifier,
+          description:  project_description,
+          types:        project_types
+        }
+      end
+
+      def project_name
+        I18n.t('seeders.demo_data.project.name')
+      end
+
+      def project_identifier
+        I18n.t('seeders.demo_data.project.identifier')
+      end
+
+      def project_description
+        I18n.t('seeders.demo_data.project.description')
+      end
+
+      def project_types
+        Type.all
+      end
+
+      def find_demo_project
+        Project.find_by(identifier: project_identifier)
+      end
+    end
+
+    include Data
   end
 end

app/views/my/first_login.html.erb

@@ -1,40 +0,0 @@
-<%#-- copyright
-OpenProject is a project management system.
-Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
-
-This program is free software; you can redistribute it and/or
-modify it under the terms of the GNU General Public License version 3.
-
-OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
-Copyright (C) 2006-2013 Jean-Philippe Lang
-Copyright (C) 2010-2013 the ChiliProject Team
-
-This program is free software; you can redistribute it and/or
-modify it under the terms of the GNU General Public License
-as published by the Free Software Foundation; either version 2
-of the License, or (at your option) any later version.
-
-This program is distributed in the hope that it will be useful,
-but WITHOUT ANY WARRANTY; without even the implied warranty of
-MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-GNU General Public License for more details.
-
-You should have received a copy of the GNU General Public License
-along with this program; if not, write to the Free Software
-Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
-
-See doc/COPYRIGHT.rdoc for more details.
-
-++#%>
-
-<%= styled_form_tag({ controller: '/my', action: "first_login" }, {method: :put}) do %>
-
-  <%= back_url_hidden_field_tag %>
-
-  <h3 class="form--section-title"><%=l(:label_ui, app_title: Setting.app_title)%></h3>
-  <section class="form--section">
-    <%= render partial: "users/impaired_settings" %>
-  </section>
-
-  <%= styled_button_tag l(:button_save), class: '-highlight -with-icon icon-yes' %>
-<% end %>

config/locales/en.yml

@@ -1345,6 +1345,7 @@ en:
   noscript_heading: "JavaScript disabled"
   noscript_learn_more: "Learn more"
 
+  notice_accessibility_mode: The accessibility mode can be enabled in your account [settings](url).
   notice_account_activated: "Your account has been activated. You can now log in."
   notice_account_already_activated: The account has already been activated.
   notice_account_invalid_token: Invalid activation token

config/routes.rb

@@ -543,7 +543,6 @@ OpenProject::Application.routes.draw do
     get '/my/page_layout', action: 'page_layout'
     get '/my/password', action: 'password'
     post '/my/change_password', action: 'change_password'
-    match '/my/first_login', action: 'first_login', via: [:get, :put]
     get '/my/page', action: 'page'
     match '/my/account', action: 'account', via: [:get, :patch]
     match '/my/settings', action: 'settings', via: [:get, :patch]

lib/redmine/i18n.rb

@@ -69,6 +69,47 @@ module Redmine
       Setting.date_format.blank? ? ::I18n.l(date.to_date) : date.strftime(Setting.date_format)
     end
 
+    ##
+    # Gives a translation and inserts links into designated spots within it
+    # in the style of markdown links. Instead of the actual URL only names for
+    # the respective links are used in the translation.
+    #
+    # The method then expects a hash mapping each of those keys to actual URLs.
+    #
+    # For example:
+    #
+    #     en.yml:
+    #       en:
+    #         logged_out: You have been logged out. Click [here](login) to login again.
+    #
+    # Which would then be used like this:
+    #
+    #     link_translate(:logged_out, login: login_url)
+    #
+    # @param i18n_key [String] The I18n key to translate.
+    # @param links [Hash] Link names mapped to URLs.
+    def link_translate(i18n_key, links = {})
+      translation = ::I18n.t(i18n_key.to_s)
+      result = translation.scan(link_regex).inject(translation) do |t, matches|
+        link, text, key = matches
+        href = String(links[key.to_sym])
+
+        t.sub(link, "<a href=\"#{href}\">#{text}</a>")
+      end
+
+      result.html_safe
+    end
+
+    ##
+    # Example: in `foo [bar](name) baz` matches:
+    #
+    #   - `[bar](name)`
+    #   - `bar`
+    #   - `name`
+    def link_regex
+      /(\[(.+?)\]\((.+?)\))/
+    end
+
     # format the time in the user time zone if one is set
     # if none is set and the time is in utc time zone (meaning it came from active record), format the date in the system timezone
     # otherwise just use the date in the time zone attached to the time

spec/controllers/account_controller_spec.rb

@@ -81,7 +81,7 @@ describe AccountController, type: :controller do
                                                                auth_source_id: 66)
         post :login, username: 'foo', password: 'bar'
 
-        expect(response).to redirect_to my_first_login_path
+        expect(response).to redirect_to my_page_path
         user = User.find_by_login('foo')
         expect(user).to be_an_instance_of User
         expect(user.auth_source_id).to eq(66)
@@ -295,6 +295,8 @@ describe AccountController, type: :controller do
       end
 
       context 'with password login enabled' do
+        # expects `redirect_to_path`
+        shared_examples 'automatic self registration succeeds' do
           before do
             post :register, user: {
               login: 'register',
@@ -306,10 +308,10 @@ describe AccountController, type: :controller do
             }
           end
 
-        it 'redirects to first_login page' do
+          it 'redirects to my page' do
             is_expected.to respond_with :redirect
             expect(assigns[:user]).not_to be_nil
-          is_expected.to redirect_to(my_first_login_path)
+            is_expected.to redirect_to(redirect_to_path)
             expect(User.where(login: 'register').last).not_to be_nil
           end
 
@@ -320,6 +322,37 @@ describe AccountController, type: :controller do
           end
         end
 
+        context 'without demo project' do
+          let(:redirect_to_path) { my_page_path }
+
+          it_behaves_like 'automatic self registration succeeds'
+        end
+
+        context 'with demo project' do
+          let!(:project) { FactoryGirl.create :project, identifier: 'demo' }
+
+          before do
+            allow(controller).to receive(:welcome_project).and_return(project)
+          end
+
+          context 'with the user not being admin' do
+            let(:redirect_to_path) { my_page_path }
+
+            it_behaves_like 'automatic self registration succeeds'
+          end
+
+          context 'with the user being admin' do
+            let(:redirect_to_path) { '/projects/demo/work_packages' }
+
+            before do
+              allow_any_instance_of(User).to receive(:admin?).and_return(true)
+            end
+
+            it_behaves_like 'automatic self registration succeeds'
+          end
+        end
+      end
+
       context 'with password login disabled' do
         before do
           allow(OpenProject::Configuration).to receive(:disable_password_login?).and_return(true)

spec/controllers/concerns/omniauth_login_spec.rb

@@ -73,8 +73,7 @@ describe AccountController, type: :controller do
         end
 
         it 'redirects to the first login page with a back_url' do
-          expect(response).to redirect_to(
-            my_first_login_path(back_url: 'https://example.net/some_back_url'))
+          expect(response).to redirect_to(my_page_path)
         end
       end
 
@@ -167,7 +166,7 @@ describe AccountController, type: :controller do
                                   firstname: 'Foo',
                                   lastname: 'Smith',
                                   mail: 'foo@bar.com' }
-          expect(response).to redirect_to my_first_login_path
+          expect(response).to redirect_to my_page_path
 
           user = User.find_by_login('login@bar.com')
           expect(user).to be_an_instance_of(User)
@@ -382,7 +381,7 @@ describe AccountController, type: :controller do
 
               post :omniauth_login
 
-              expect(response).to redirect_to my_first_login_path
+              expect(response).to redirect_to my_page_path
               # authorization is successful which results in the registration
               # of a new user in this case because we changed the provider
               # and there isn't a user with that identity URL yet ...

spec/features/auth/login_spec.rb

@@ -82,12 +82,6 @@ describe 'Login', type: :feature do
         fill_in('new_password_confirmation', with: new_user_password)
         click_link_or_button I18n.t(:button_save)
       end
-      expect(current_path).to eql my_first_login_path
-
-      # we just save the form and go on
-      within('#main') do
-        click_link_or_button I18n.t(:button_save)
-      end
 
       # on the my page
       expect(current_path).to eql my_page_path

spec/features/auth/omniauth_spec.rb

@@ -137,7 +137,8 @@ describe 'Omniauth authentication', type: :feature do
 
   shared_examples 'omniauth user registration' do
     it 'should register new user' do
-      visit '/auth/developer'
+      visit '/'
+      find_link('Omniauth Developer').click
 
       # login form developer strategy
       fill_in('first_name', with: user.firstname)
@@ -186,9 +187,6 @@ describe 'Omniauth authentication', type: :feature do
       fill_in('user_lastname', with: user.lastname)
       click_link_or_button 'Submit'
 
-      # now, we see the my/first_login page and just save
-      click_link_or_button 'Save'
-
       expect(current_url).to eql account_lost_password_url
     end
 

spec/features/users/create_spec.rb

@@ -125,9 +125,9 @@ describe 'create users', type: :feature, selenium: true do
 
           click_button 'Sign in'
 
-          expect(page).to have_text 'My account'
+          expect(page).to have_text 'My page'
           expect(page).to have_text 'bobfirst boblast'
-          expect(current_path).to eq '/my/first_login'
+          expect(current_path).to eq '/my/page'
         end
       end
     end

spec/lib/redmine/i18n_spec.rb

@@ -151,5 +151,24 @@ module OpenProject
         expect(find_language(:DE)).to eql :de
       end
     end
+
+    describe 'link_translation' do
+      before do
+        allow(::I18n)
+          .to receive(:t)
+          .with('translation_with_a_link')
+          .and_return('There is a [link](url_1) in this translation! Maybe even [two](url_2)?')
+      end
+
+      it 'allows to insert links into translations' do
+        translated = link_translate :translation_with_a_link,
+                                    url_1: 'http://openproject.com/foobar',
+                                    url_2: '/baz'
+
+        expect(translated).to eq(
+          "There is a <a href=\"http://openproject.com/foobar\">link</a> in this translation!" +
+          " Maybe even <a href=\"/baz\">two</a>?")
+      end
+    end
   end
 end