Commit 0d9a6fe7b16e52cc4d5595d6b26552c39911cf07
1 parent
9a06dd4a
Exists in
master
and in
4 other branches
User's blocked field refactored to use state machine
Showing
12 changed files
with
39 additions
and
32 deletions
Show diff stats
app/controllers/admin/users_controller.rb
| @@ -45,7 +45,7 @@ class Admin::UsersController < Admin::ApplicationController | @@ -45,7 +45,7 @@ class Admin::UsersController < Admin::ApplicationController | ||
| 45 | end | 45 | end |
| 46 | 46 | ||
| 47 | def unblock | 47 | def unblock |
| 48 | - if admin_user.update_attribute(:blocked, false) | 48 | + if admin_user.activate |
| 49 | redirect_to :back, alert: "Successfully unblocked" | 49 | redirect_to :back, alert: "Successfully unblocked" |
| 50 | else | 50 | else |
| 51 | redirect_to :back, alert: "Error occured. User was not unblocked" | 51 | redirect_to :back, alert: "Error occured. User was not unblocked" |
app/controllers/application_controller.rb
| @@ -30,7 +30,7 @@ class ApplicationController < ActionController::Base | @@ -30,7 +30,7 @@ class ApplicationController < ActionController::Base | ||
| 30 | end | 30 | end |
| 31 | 31 | ||
| 32 | def reject_blocked! | 32 | def reject_blocked! |
| 33 | - if current_user && current_user.blocked | 33 | + if current_user && current_user.blocked? |
| 34 | sign_out current_user | 34 | sign_out current_user |
| 35 | flash[:alert] = "Your account is blocked. Retry when an admin unblock it." | 35 | flash[:alert] = "Your account is blocked. Retry when an admin unblock it." |
| 36 | redirect_to new_user_session_path | 36 | redirect_to new_user_session_path |
| @@ -38,7 +38,7 @@ class ApplicationController < ActionController::Base | @@ -38,7 +38,7 @@ class ApplicationController < ActionController::Base | ||
| 38 | end | 38 | end |
| 39 | 39 | ||
| 40 | def after_sign_in_path_for resource | 40 | def after_sign_in_path_for resource |
| 41 | - if resource.is_a?(User) && resource.respond_to?(:blocked) && resource.blocked | 41 | + if resource.is_a?(User) && resource.respond_to?(:blocked?) && resource.blocked? |
| 42 | sign_out resource | 42 | sign_out resource |
| 43 | flash[:alert] = "Your account is blocked. Retry when an admin unblock it." | 43 | flash[:alert] = "Your account is blocked. Retry when an admin unblock it." |
| 44 | new_user_session_path | 44 | new_user_session_path |
app/models/user.rb
| @@ -25,7 +25,7 @@ | @@ -25,7 +25,7 @@ | ||
| 25 | # dark_scheme :boolean default(FALSE), not null | 25 | # dark_scheme :boolean default(FALSE), not null |
| 26 | # theme_id :integer default(1), not null | 26 | # theme_id :integer default(1), not null |
| 27 | # bio :string(255) | 27 | # bio :string(255) |
| 28 | -# blocked :boolean default(FALSE), not null | 28 | +# state :string(255) |
| 29 | # failed_attempts :integer default(0) | 29 | # failed_attempts :integer default(0) |
| 30 | # locked_at :datetime | 30 | # locked_at :datetime |
| 31 | # extern_uid :string(255) | 31 | # extern_uid :string(255) |
| @@ -87,10 +87,27 @@ class User < ActiveRecord::Base | @@ -87,10 +87,27 @@ class User < ActiveRecord::Base | ||
| 87 | 87 | ||
| 88 | delegate :path, to: :namespace, allow_nil: true, prefix: true | 88 | delegate :path, to: :namespace, allow_nil: true, prefix: true |
| 89 | 89 | ||
| 90 | + state_machine :state, initial: :active do | ||
| 91 | + after_transition any => :blocked do |user, transition| | ||
| 92 | + # Remove user from all projects and | ||
| 93 | + user.users_projects.find_each do |membership| | ||
| 94 | + return false unless membership.destroy | ||
| 95 | + end | ||
| 96 | + end | ||
| 97 | + | ||
| 98 | + event :block do | ||
| 99 | + transition active: :blocked | ||
| 100 | + end | ||
| 101 | + | ||
| 102 | + event :activate do | ||
| 103 | + transition blocked: :active | ||
| 104 | + end | ||
| 105 | + end | ||
| 106 | + | ||
| 90 | # Scopes | 107 | # Scopes |
| 91 | scope :admins, -> { where(admin: true) } | 108 | scope :admins, -> { where(admin: true) } |
| 92 | - scope :blocked, -> { where(blocked: true) } | ||
| 93 | - scope :active, -> { where(blocked: false) } | 109 | + scope :blocked, -> { with_state(:blocked) } |
| 110 | + scope :active, -> { with_state(:active) } | ||
| 94 | scope :alphabetically, -> { order('name ASC') } | 111 | scope :alphabetically, -> { order('name ASC') } |
| 95 | scope :in_team, ->(team){ where(id: team.member_ids) } | 112 | scope :in_team, ->(team){ where(id: team.member_ids) } |
| 96 | scope :not_in_team, ->(team){ where('users.id NOT IN (:ids)', ids: team.member_ids) } | 113 | scope :not_in_team, ->(team){ where('users.id NOT IN (:ids)', ids: team.member_ids) } |
| @@ -260,17 +277,6 @@ class User < ActiveRecord::Base | @@ -260,17 +277,6 @@ class User < ActiveRecord::Base | ||
| 260 | MergeRequest.cared(self) | 277 | MergeRequest.cared(self) |
| 261 | end | 278 | end |
| 262 | 279 | ||
| 263 | - # Remove user from all projects and | ||
| 264 | - # set blocked attribute to true | ||
| 265 | - def block | ||
| 266 | - users_projects.find_each do |membership| | ||
| 267 | - return false unless membership.destroy | ||
| 268 | - end | ||
| 269 | - | ||
| 270 | - self.blocked = true | ||
| 271 | - save | ||
| 272 | - end | ||
| 273 | - | ||
| 274 | def projects_limit_percent | 280 | def projects_limit_percent |
| 275 | return 100 if projects_limit.zero? | 281 | return 100 if projects_limit.zero? |
| 276 | (personal_projects.count.to_f / projects_limit) * 100 | 282 | (personal_projects.count.to_f / projects_limit) * 100 |
app/views/admin/users/_form.html.haml
| @@ -61,7 +61,7 @@ | @@ -61,7 +61,7 @@ | ||
| 61 | .span4 | 61 | .span4 |
| 62 | - unless @admin_user.new_record? | 62 | - unless @admin_user.new_record? |
| 63 | .alert.alert-error | 63 | .alert.alert-error |
| 64 | - - if @admin_user.blocked | 64 | + - if @admin_user.blocked? |
| 65 | %p This user is blocked and is not able to login to GitLab | 65 | %p This user is blocked and is not able to login to GitLab |
| 66 | = link_to 'Unblock User', unblock_admin_user_path(@admin_user), method: :put, class: "btn btn-small" | 66 | = link_to 'Unblock User', unblock_admin_user_path(@admin_user), method: :put, class: "btn btn-small" |
| 67 | - else | 67 | - else |
app/views/admin/users/index.html.haml
| @@ -53,7 +53,7 @@ | @@ -53,7 +53,7 @@ | ||
| 53 | | 53 | |
| 54 | = link_to 'Edit', edit_admin_user_path(user), id: "edit_#{dom_id(user)}", class: "btn btn-small" | 54 | = link_to 'Edit', edit_admin_user_path(user), id: "edit_#{dom_id(user)}", class: "btn btn-small" |
| 55 | - unless user == current_user | 55 | - unless user == current_user |
| 56 | - - if user.blocked | 56 | + - if user.blocked? |
| 57 | = link_to 'Unblock', unblock_admin_user_path(user), method: :put, class: "btn btn-small success" | 57 | = link_to 'Unblock', unblock_admin_user_path(user), method: :put, class: "btn btn-small success" |
| 58 | - else | 58 | - else |
| 59 | = link_to 'Block', block_admin_user_path(user), confirm: 'USER WILL BE BLOCKED! Are you sure?', method: :put, class: "btn btn-small btn-remove" | 59 | = link_to 'Block', block_admin_user_path(user), confirm: 'USER WILL BE BLOCKED! Are you sure?', method: :put, class: "btn btn-small btn-remove" |
app/views/admin/users/show.html.haml
| @@ -3,7 +3,7 @@ | @@ -3,7 +3,7 @@ | ||
| 3 | %h3.page_title | 3 | %h3.page_title |
| 4 | = image_tag gravatar_icon(@admin_user.email, 90), class: "avatar s90" | 4 | = image_tag gravatar_icon(@admin_user.email, 90), class: "avatar s90" |
| 5 | = @admin_user.name | 5 | = @admin_user.name |
| 6 | - - if @admin_user.blocked | 6 | + - if @admin_user.blocked? |
| 7 | %span.cred (Blocked) | 7 | %span.cred (Blocked) |
| 8 | - if @admin_user.admin | 8 | - if @admin_user.admin |
| 9 | %span.cred (Admin) | 9 | %span.cred (Admin) |
app/views/team_members/_team_member.html.haml
| @@ -20,7 +20,7 @@ | @@ -20,7 +20,7 @@ | ||
| 20 | %span.label This is you! | 20 | %span.label This is you! |
| 21 | - if @project.namespace_owner == user | 21 | - if @project.namespace_owner == user |
| 22 | %span.label Owner | 22 | %span.label Owner |
| 23 | - - elsif user.blocked | 23 | + - elsif user.blocked? |
| 24 | %span.label Blocked | 24 | %span.label Blocked |
| 25 | - elsif allow_admin | 25 | - elsif allow_admin |
| 26 | = link_to project_team_member_path(@project, user), confirm: remove_from_project_team_message(@project, user), method: :delete, class: "btn-tiny btn btn-remove" do | 26 | = link_to project_team_member_path(@project, user), confirm: remove_from_project_team_message(@project, user), method: :delete, class: "btn-tiny btn btn-remove" do |
app/views/teams/members/_show.html.haml
| @@ -23,7 +23,7 @@ | @@ -23,7 +23,7 @@ | ||
| 23 | %span.btn.disabled This is you! | 23 | %span.btn.disabled This is you! |
| 24 | - if @team.owner == user | 24 | - if @team.owner == user |
| 25 | %span.btn.disabled Owner | 25 | %span.btn.disabled Owner |
| 26 | - - elsif user.blocked | 26 | + - elsif user.blocked? |
| 27 | %span.btn.disabled.blocked Blocked | 27 | %span.btn.disabled.blocked Blocked |
| 28 | - elsif allow_admin | 28 | - elsif allow_admin |
| 29 | = link_to team_member_path(@team, user), confirm: remove_from_user_team_message(@team, user), method: :delete, class: "btn-tiny btn btn-remove", title: "Remove from team" do | 29 | = link_to team_member_path(@team, user), confirm: remove_from_user_team_message(@team, user), method: :delete, class: "btn-tiny btn btn-remove", title: "Remove from team" do |
db/schema.rb
| @@ -11,7 +11,7 @@ | @@ -11,7 +11,7 @@ | ||
| 11 | # | 11 | # |
| 12 | # It's strongly recommended to check this file into your version control system. | 12 | # It's strongly recommended to check this file into your version control system. |
| 13 | 13 | ||
| 14 | -ActiveRecord::Schema.define(:version => 20130220133245) do | 14 | +ActiveRecord::Schema.define(:version => 20130304105317) do |
| 15 | 15 | ||
| 16 | create_table "events", :force => true do |t| | 16 | create_table "events", :force => true do |t| |
| 17 | t.string "target_type" | 17 | t.string "target_type" |
| @@ -261,7 +261,6 @@ ActiveRecord::Schema.define(:version => 20130220133245) do | @@ -261,7 +261,6 @@ ActiveRecord::Schema.define(:version => 20130220133245) do | ||
| 261 | t.boolean "dark_scheme", :default => false, :null => false | 261 | t.boolean "dark_scheme", :default => false, :null => false |
| 262 | t.integer "theme_id", :default => 1, :null => false | 262 | t.integer "theme_id", :default => 1, :null => false |
| 263 | t.string "bio" | 263 | t.string "bio" |
| 264 | - t.boolean "blocked", :default => false, :null => false | ||
| 265 | t.integer "failed_attempts", :default => 0 | 264 | t.integer "failed_attempts", :default => 0 |
| 266 | t.datetime "locked_at" | 265 | t.datetime "locked_at" |
| 267 | t.string "extern_uid" | 266 | t.string "extern_uid" |
| @@ -269,10 +268,10 @@ ActiveRecord::Schema.define(:version => 20130220133245) do | @@ -269,10 +268,10 @@ ActiveRecord::Schema.define(:version => 20130220133245) do | ||
| 269 | t.string "username" | 268 | t.string "username" |
| 270 | t.boolean "can_create_group", :default => true, :null => false | 269 | t.boolean "can_create_group", :default => true, :null => false |
| 271 | t.boolean "can_create_team", :default => true, :null => false | 270 | t.boolean "can_create_team", :default => true, :null => false |
| 271 | + t.string "state" | ||
| 272 | end | 272 | end |
| 273 | 273 | ||
| 274 | add_index "users", ["admin"], :name => "index_users_on_admin" | 274 | add_index "users", ["admin"], :name => "index_users_on_admin" |
| 275 | - add_index "users", ["blocked"], :name => "index_users_on_blocked" | ||
| 276 | add_index "users", ["email"], :name => "index_users_on_email", :unique => true | 275 | add_index "users", ["email"], :name => "index_users_on_email", :unique => true |
| 277 | add_index "users", ["extern_uid", "provider"], :name => "index_users_on_extern_uid_and_provider", :unique => true | 276 | add_index "users", ["extern_uid", "provider"], :name => "index_users_on_extern_uid_and_provider", :unique => true |
| 278 | add_index "users", ["name"], :name => "index_users_on_name" | 277 | add_index "users", ["name"], :name => "index_users_on_name" |
lib/api/entities.rb
| @@ -2,11 +2,11 @@ module Gitlab | @@ -2,11 +2,11 @@ module Gitlab | ||
| 2 | module Entities | 2 | module Entities |
| 3 | class User < Grape::Entity | 3 | class User < Grape::Entity |
| 4 | expose :id, :username, :email, :name, :bio, :skype, :linkedin, :twitter, | 4 | expose :id, :username, :email, :name, :bio, :skype, :linkedin, :twitter, |
| 5 | - :dark_scheme, :theme_id, :blocked, :created_at, :extern_uid, :provider | 5 | + :dark_scheme, :theme_id, :state, :created_at, :extern_uid, :provider |
| 6 | end | 6 | end |
| 7 | 7 | ||
| 8 | class UserBasic < Grape::Entity | 8 | class UserBasic < Grape::Entity |
| 9 | - expose :id, :username, :email, :name, :blocked, :created_at | 9 | + expose :id, :username, :email, :name, :state, :created_at |
| 10 | end | 10 | end |
| 11 | 11 | ||
| 12 | class UserLogin < UserBasic | 12 | class UserLogin < UserBasic |
lib/gitlab/auth.rb
| @@ -41,10 +41,12 @@ module Gitlab | @@ -41,10 +41,12 @@ module Gitlab | ||
| 41 | password_confirmation: password, | 41 | password_confirmation: password, |
| 42 | projects_limit: Gitlab.config.gitlab.default_projects_limit, | 42 | projects_limit: Gitlab.config.gitlab.default_projects_limit, |
| 43 | }, as: :admin) | 43 | }, as: :admin) |
| 44 | + @user.save! | ||
| 45 | + | ||
| 44 | if Gitlab.config.omniauth['block_auto_created_users'] && !ldap | 46 | if Gitlab.config.omniauth['block_auto_created_users'] && !ldap |
| 45 | - @user.blocked = true | 47 | + @user.block |
| 46 | end | 48 | end |
| 47 | - @user.save! | 49 | + |
| 48 | @user | 50 | @user |
| 49 | end | 51 | end |
| 50 | 52 |
spec/models/user_spec.rb
| @@ -25,7 +25,7 @@ | @@ -25,7 +25,7 @@ | ||
| 25 | # dark_scheme :boolean default(FALSE), not null | 25 | # dark_scheme :boolean default(FALSE), not null |
| 26 | # theme_id :integer default(1), not null | 26 | # theme_id :integer default(1), not null |
| 27 | # bio :string(255) | 27 | # bio :string(255) |
| 28 | -# blocked :boolean default(FALSE), not null | 28 | +# state :string(255) default(FALSE), not null |
| 29 | # failed_attempts :integer default(0) | 29 | # failed_attempts :integer default(0) |
| 30 | # locked_at :datetime | 30 | # locked_at :datetime |
| 31 | # extern_uid :string(255) | 31 | # extern_uid :string(255) |
| @@ -140,7 +140,7 @@ describe User do | @@ -140,7 +140,7 @@ describe User do | ||
| 140 | 140 | ||
| 141 | it "should block user" do | 141 | it "should block user" do |
| 142 | user.block | 142 | user.block |
| 143 | - user.blocked.should be_true | 143 | + user.blocked?.should be_true |
| 144 | end | 144 | end |
| 145 | end | 145 | end |
| 146 | 146 | ||
| @@ -149,7 +149,7 @@ describe User do | @@ -149,7 +149,7 @@ describe User do | ||
| 149 | User.delete_all | 149 | User.delete_all |
| 150 | @user = create :user | 150 | @user = create :user |
| 151 | @admin = create :user, admin: true | 151 | @admin = create :user, admin: true |
| 152 | - @blocked = create :user, blocked: true | 152 | + @blocked = create :user, state: :blocked |
| 153 | end | 153 | end |
| 154 | 154 | ||
| 155 | it { User.filter("admins").should == [@admin] } | 155 | it { User.filter("admins").should == [@admin] } |