From 6f714dfb4a9b823ab75508f252d06e19e286d5f2 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 16 Nov 2016 23:10:27 +0100 Subject: [PATCH] Improve code design after code review --- app/controllers/profiles/chat_names_controller.rb | 4 ++-- app/services/chat_names/find_user_service.rb | 2 +- app/views/profiles/chat_names/index.html.haml | 14 ++++++++------ app/views/profiles/chat_names/new.html.haml | 4 ++-- .../20161113184239_create_user_chat_names_table.rb | 2 +- db/schema.rb | 2 +- lib/gitlab/chat_name_token.rb | 2 +- spec/lib/gitlab/chat_name_token_spec.rb | 4 +--- .../chat_names/authorize_user_service_spec.rb | 4 +--- spec/services/chat_names/find_user_service_spec.rb | 2 +- spec/support/matchers/be_url.rb | 5 +++++ 11 files changed, 24 insertions(+), 21 deletions(-) create mode 100644 spec/support/matchers/be_url.rb diff --git a/app/controllers/profiles/chat_names_controller.rb b/app/controllers/profiles/chat_names_controller.rb index 0d3bdeff416..7f9c8e64cca 100644 --- a/app/controllers/profiles/chat_names_controller.rb +++ b/app/controllers/profiles/chat_names_controller.rb @@ -13,7 +13,7 @@ class Profiles::ChatNamesController < Profiles::ApplicationController new_chat_name = current_user.chat_names.new(chat_name_params) if new_chat_name.save - flash[:notice] = "Authorized chat nickname #{new_chat_name.chat_name}" + flash[:notice] = "Authorized #{new_chat_name.chat_name}" else flash[:alert] = "Could not authorize chat nickname. Try again!" end @@ -34,7 +34,7 @@ class Profiles::ChatNamesController < Profiles::ApplicationController @chat_name = chat_names.find(params[:id]) if @chat_name.destroy - flash[:notice] = "Delete chat nickname: #{@chat_name.chat_name}!" + flash[:notice] = "Deleted chat nickname: #{@chat_name.chat_name}!" else flash[:alert] = "Could not delete chat nickname #{@chat_name.chat_name}." end diff --git a/app/services/chat_names/find_user_service.rb b/app/services/chat_names/find_user_service.rb index 28e3e155be1..08079fdaf70 100644 --- a/app/services/chat_names/find_user_service.rb +++ b/app/services/chat_names/find_user_service.rb @@ -9,7 +9,7 @@ module ChatNames chat_name = find_chat_name return unless chat_name - chat_name.update(used_at: Time.now) + chat_name.update(last_used_at: Time.now) chat_name.user end diff --git a/app/views/profiles/chat_names/index.html.haml b/app/views/profiles/chat_names/index.html.haml index ae0b6336944..6d8d606583d 100644 --- a/app/views/profiles/chat_names/index.html.haml +++ b/app/views/profiles/chat_names/index.html.haml @@ -6,7 +6,7 @@ %h4.prepend-top-0 = page_title %p - You can see your Chat integrations. + You can see your Chat accounts. .col-lg-9 %h5 Active chat names (#{@chat_names.length}) @@ -39,11 +39,13 @@ = link_to service.title, edit_namespace_project_service_path(project.namespace, project, service) - else = chat_name.service.title - %td= chat_name.team_domain - %td= chat_name.chat_name - %td= - - if chat_name.used_at - time_ago_with_tooltip(chat_name.used_at) + %td + = chat_name.team_domain + %td + = chat_name.chat_name + %td + - if chat_name.last_used_at + time_ago_with_tooltip(chat_name.last_used_at) - else Never diff --git a/app/views/profiles/chat_names/new.html.haml b/app/views/profiles/chat_names/new.html.haml index 0b9ee8c71ad..f635acf96e2 100644 --- a/app/views/profiles/chat_names/new.html.haml +++ b/app/views/profiles/chat_names/new.html.haml @@ -1,11 +1,11 @@ %h3.page-title Authorization required %main{:role => "main"} %p.h4 - Authorize the chat user + Authorize %strong.text-info= @chat_name_params[:chat_name] to use your account? - %hr/ + %hr .actions = form_tag profile_chat_names_path, method: :post do = hidden_field_tag :token, @chat_name_token.token diff --git a/db/migrate/20161113184239_create_user_chat_names_table.rb b/db/migrate/20161113184239_create_user_chat_names_table.rb index 1e138fba202..97b597654f7 100644 --- a/db/migrate/20161113184239_create_user_chat_names_table.rb +++ b/db/migrate/20161113184239_create_user_chat_names_table.rb @@ -11,7 +11,7 @@ class CreateUserChatNamesTable < ActiveRecord::Migration t.string :team_domain t.string :chat_id, null: false t.string :chat_name - t.datetime :used_at + t.datetime :last_used_at t.timestamps null: false end diff --git a/db/schema.rb b/db/schema.rb index 3688df3a238..5732b1b73c1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -159,7 +159,7 @@ ActiveRecord::Schema.define(version: 20161113184239) do t.string "team_domain" t.string "chat_id", null: false t.string "chat_name" - t.datetime "used_at" + t.datetime "last_used_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false end diff --git a/lib/gitlab/chat_name_token.rb b/lib/gitlab/chat_name_token.rb index 543f038a4d4..1b081aa9b1d 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 = 10.minutes # 10 minutes + EXPIRY_TIME = 10.minutes def initialize(token = new_token) @token = token diff --git a/spec/lib/gitlab/chat_name_token_spec.rb b/spec/lib/gitlab/chat_name_token_spec.rb index 8d7e7a99059..8c1e6efa9db 100644 --- a/spec/lib/gitlab/chat_name_token_spec.rb +++ b/spec/lib/gitlab/chat_name_token_spec.rb @@ -12,9 +12,7 @@ describe Gitlab::ChatNameToken, lib: true do end context 'when storing data' do - let(:data) { - { key: 'value' } - } + let(:data) { { key: 'value' } } subject { described_class.new(@token) } diff --git a/spec/services/chat_names/authorize_user_service_spec.rb b/spec/services/chat_names/authorize_user_service_spec.rb index f8c26e51bfc..2ecee58e92d 100644 --- a/spec/services/chat_names/authorize_user_service_spec.rb +++ b/spec/services/chat_names/authorize_user_service_spec.rb @@ -10,9 +10,7 @@ describe ChatNames::AuthorizeUserService, services: true 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=') + is_expected.to be_url end end diff --git a/spec/services/chat_names/find_user_service_spec.rb b/spec/services/chat_names/find_user_service_spec.rb index cf5844069f9..5b885b2c657 100644 --- a/spec/services/chat_names/find_user_service_spec.rb +++ b/spec/services/chat_names/find_user_service_spec.rb @@ -20,7 +20,7 @@ describe ChatNames::FindUserService, services: true do it 'updates when last time chat name was used' do subject - expect(chat_name.reload.used_at).to be_like_time(Time.now) + expect(chat_name.reload.last_used_at).to be_like_time(Time.now) end end diff --git a/spec/support/matchers/be_url.rb b/spec/support/matchers/be_url.rb new file mode 100644 index 00000000000..f8096af1b22 --- /dev/null +++ b/spec/support/matchers/be_url.rb @@ -0,0 +1,5 @@ +RSpec::Matchers.define :be_url do |_| + match do |actual| + URI.parse(actual) rescue false + end +end