diff --git a/app/services/chat_names/request_service.rb b/app/services/chat_names/authorize_user_service.rb similarity index 73% rename from app/services/chat_names/request_service.rb rename to app/services/chat_names/authorize_user_service.rb index c67b93f932f..321bf3a9205 100644 --- a/app/services/chat_names/request_service.rb +++ b/app/services/chat_names/authorize_user_service.rb @@ -1,5 +1,5 @@ module ChatNames - class RequestService + class AuthorizeUserService include Gitlab::Routing.url_helpers def initialize(service, params) @@ -8,13 +8,19 @@ module ChatNames end def execute - token = chat_name_token.store!(chat_name_params) + return unless chat_name_params.values.all?(&:present?) + + token = request_token new_profile_chat_name_url(token: token) if token end private + def request_token + chat_name_token.store!(chat_name_params) + end + def chat_name_token Gitlab::ChatNameToken.new end diff --git a/app/services/chat_names/find_user_service.rb b/app/services/chat_names/find_user_service.rb index 6b7f75430a8..28e3e155be1 100644 --- a/app/services/chat_names/find_user_service.rb +++ b/app/services/chat_names/find_user_service.rb @@ -1,12 +1,23 @@ module ChatNames class FindUserService - def initialize(chat_names, params) - @chat_names = chat_names + def initialize(service, params) + @service = service @params = params end def execute - @chat_names.find_by( + chat_name = find_chat_name + return unless chat_name + + chat_name.update(used_at: Time.now) + chat_name.user + end + + private + + def find_chat_name + ChatName.find_by( + service: @service, team_id: @params[:team_id], chat_id: @params[:user_id] ) diff --git a/app/views/profiles/chat_names/index.html.haml b/app/views/profiles/chat_names/index.html.haml index f90ac4c6a03..ae0b6336944 100644 --- a/app/views/profiles/chat_names/index.html.haml +++ b/app/views/profiles/chat_names/index.html.haml @@ -1,4 +1,4 @@ -- page_title "Chat" +- page_title 'Chat' = render 'profiles/head' .row.prepend-top-default @@ -20,7 +20,7 @@ %th Service %th Team domain %th Nickname - %th Created + %th Last used %th %tbody - @chat_names.each do |chat_name| @@ -41,8 +41,14 @@ = chat_name.service.title %td= chat_name.team_domain %td= chat_name.chat_name - %td= chat_name.created_at - %td= link_to "Remove", profile_chat_name_path(chat_name), method: :delete, class: "btn btn-danger pull-right", data: { confirm: "Are you sure you want to revoke this nickname?" } + %td= + - if chat_name.used_at + time_ago_with_tooltip(chat_name.used_at) + - else + Never + + %td + = link_to 'Remove', profile_chat_name_path(chat_name), method: :delete, class: 'btn btn-danger pull-right', data: { confirm: 'Are you sure you want to revoke this nickname?' } - else .settings-message.text-center diff --git a/db/migrate/20161113184239_create_user_chat_names_table.rb b/db/migrate/20161113184239_create_user_chat_names_table.rb index f9ab2adf2a9..1e138fba202 100644 --- a/db/migrate/20161113184239_create_user_chat_names_table.rb +++ b/db/migrate/20161113184239_create_user_chat_names_table.rb @@ -5,13 +5,14 @@ class CreateUserChatNamesTable < ActiveRecord::Migration def change create_table :chat_names do |t| - t.integer "user_id", null: false - t.integer "service_id", null: false - t.string "team_id" - t.string "team_domain" - t.string "chat_id" - t.string "chat_name" - t.timestamps + t.integer :user_id, null: false + t.integer :service_id, null: false + t.string :team_id, null: false + t.string :team_domain + t.string :chat_id, null: false + t.string :chat_name + t.datetime :used_at + t.timestamps null: false end add_index :chat_names, [:user_id, :service_id], unique: true diff --git a/db/schema.rb b/db/schema.rb index 5f25f0a305f..1c1a7a37096 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -152,15 +152,16 @@ ActiveRecord::Schema.define(version: 20161113184239) do create_table "chat_names", force: :cascade do |t| t.integer "user_id", null: false t.integer "service_id", null: false - t.string "team_id" + t.string "team_id", null: false t.string "team_domain" - t.string "chat_id" + t.string "chat_id", null: false t.string "chat_name" - t.datetime "created_at" - t.datetime "updated_at" + t.datetime "used_at" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false end - add_index "chat_names", ["service_id", "team_id", "user_id"], name: "index_chat_names_on_service_id_and_team_id_and_user_id", unique: true, using: :btree + add_index "chat_names", ["service_id", "team_id", "chat_id"], name: "index_chat_names_on_service_id_and_team_id_and_chat_id", unique: true, using: :btree add_index "chat_names", ["user_id", "service_id"], name: "index_chat_names_on_user_id_and_service_id", unique: true, using: :btree create_table "ci_application_settings", force: :cascade do |t| diff --git a/lib/gitlab/chat_name_token.rb b/lib/gitlab/chat_name_token.rb index c8349839219..543f038a4d4 100644 --- a/lib/gitlab/chat_name_token.rb +++ b/lib/gitlab/chat_name_token.rb @@ -5,7 +5,7 @@ module Gitlab attr_reader :token TOKEN_LENGTH = 50 - EXPIRY_TIME = 1800 + EXPIRY_TIME = 10.minutes # 10 minutes def initialize(token = new_token) @token = token diff --git a/spec/factories/chat_names.rb b/spec/factories/chat_names.rb new file mode 100644 index 00000000000..24225468d55 --- /dev/null +++ b/spec/factories/chat_names.rb @@ -0,0 +1,16 @@ +FactoryGirl.define do + factory :chat_name, class: ChatName do + user factory: :user + service factory: :service + + team_id 'T0001' + team_domain 'Awesome Team' + + sequence :chat_id do |n| + "U#{n}" + end + sequence :chat_name do |n| + "user#{n}" + end + end +end diff --git a/spec/lib/gitlab/chat_name_token_spec.rb b/spec/lib/gitlab/chat_name_token_spec.rb new file mode 100644 index 00000000000..8d7e7a99059 --- /dev/null +++ b/spec/lib/gitlab/chat_name_token_spec.rb @@ -0,0 +1,39 @@ +require 'spec_helper' + +describe Gitlab::ChatNameToken, lib: true do + context 'when using unknown token' do + let(:token) { } + + subject { described_class.new(token).get } + + it 'returns empty data' do + is_expected.to be_nil + end + end + + context 'when storing data' do + let(:data) { + { key: 'value' } + } + + subject { described_class.new(@token) } + + before do + @token = described_class.new.store!(data) + end + + it 'returns stored data' do + expect(subject.get).to eq(data) + end + + context 'and after deleting them' do + before do + subject.delete + end + + it 'data are removed' do + expect(subject.get).to be_nil + end + end + end +end diff --git a/spec/models/chat_name_spec.rb b/spec/models/chat_name_spec.rb new file mode 100644 index 00000000000..b02971cab82 --- /dev/null +++ b/spec/models/chat_name_spec.rb @@ -0,0 +1,16 @@ +require 'spec_helper' + +describe ChatName, models: true do + subject { create(:chat_name) } + + it { is_expected.to belong_to(:service) } + it { is_expected.to belong_to(:user) } + + it { is_expected.to validate_presence_of(:user) } + it { is_expected.to validate_presence_of(:service) } + it { is_expected.to validate_presence_of(:team_id) } + it { is_expected.to validate_presence_of(:chat_id) } + + it { is_expected.to validate_uniqueness_of(:user_id).scoped_to(:service_id) } + it { is_expected.to validate_uniqueness_of(:chat_id).scoped_to(:service_id, :team_id) } +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3b152e15b61..be6767855c7 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -33,6 +33,7 @@ describe User, models: true do it { is_expected.to have_many(:award_emoji).dependent(:destroy) } it { is_expected.to have_many(:builds).dependent(:nullify) } it { is_expected.to have_many(:pipelines).dependent(:nullify) } + it { is_expected.to have_many(:chat_names).dependent(:destroy) } describe '#group_members' do it 'does not include group memberships for which user is a requester' do diff --git a/spec/services/chat_names/authorize_user_service_spec.rb b/spec/services/chat_names/authorize_user_service_spec.rb new file mode 100644 index 00000000000..f8c26e51bfc --- /dev/null +++ b/spec/services/chat_names/authorize_user_service_spec.rb @@ -0,0 +1,27 @@ +require 'spec_helper' + +describe ChatNames::AuthorizeUserService, services: true do + describe '#execute' do + let(:service) { create(:service) } + + subject { described_class.new(service, params).execute } + + context 'when all parameters are valid' do + let(:params) { { team_id: 'T0001', team_domain: 'myteam', user_id: 'U0001', user_name: 'user' } } + + it 'requests a new token' do + is_expected.to include('http') + is_expected.to include('://') + is_expected.to include('token=') + end + end + + context 'when there are missing parameters' do + let(:params) { { } } + + it 'does not request a new token' do + is_expected.to be_nil + end + end + end +end diff --git a/spec/services/chat_names/find_user_service_spec.rb b/spec/services/chat_names/find_user_service_spec.rb new file mode 100644 index 00000000000..cf5844069f9 --- /dev/null +++ b/spec/services/chat_names/find_user_service_spec.rb @@ -0,0 +1,36 @@ +require 'spec_helper' + +describe ChatNames::FindUserService, services: true do + describe '#execute' do + let(:service) { create(:service) } + + subject { described_class.new(service, params).execute } + + context 'find user mapping' do + let(:user) { create(:user) } + let!(:chat_name) { create(:chat_name, user: user, service: service) } + + context 'when existing user is requested' do + let(:params) { { team_id: chat_name.team_id, user_id: chat_name.chat_id } } + + it 'returns existing user' do + is_expected.to eq(user) + end + + it 'updates when last time chat name was used' do + subject + + expect(chat_name.reload.used_at).to be_like_time(Time.now) + end + end + + context 'when different user is requested' do + let(:params) { { team_id: chat_name.team_id, user_id: 'non-existing-user' } } + + it 'returns existing user' do + is_expected.to be_nil + end + end + end + end +end