Trying to get the coverage running for all directories, I got stuck at the
user's one was failing because the testcase is still Scr based, when the
module is actually using the YaPI.
Once I tried to make the test pass I realized the code was not structured in a
way that makes it easy to test. Mainly, I found the following flaws:
- The controller did direct calls to YaPI
I pointed at this pattern in a previous email. If you want to test this, you
will need to stub the YaPI, which is a lot of noise for a controller
- The controller called instance modifier methods
This is a very confusing way of writing code. Basically, you are in a method,
lets say index. And from it you call another method, lets say fill_users,
which modifies an instance variable, which is later used in the code.
def index
# this method modifies/creates @var
fill_vars
# and from now, the code relies on what fill_vars did
@var.do_something
It is very hard to figure out that fill_users does something with @users if
you don't look at the implementation. As fill_users does not require anything
from the controller, why does it need instance variables? either move the code
to a module, or where it belongs, In this case, it makes sense to actually put
it into the class where it belongs: User
The only reason why in rails you have @variables is because the view accesses
them. But 1) if it is not passed to the view, there is no reason for member
variables 2) member variables is not a way to pass data across functions, it
is the state of the object!
Refactorings done:
- all the instance modifier methods where removed. Similar methods are now in
the User class:
users = User.find_all
user = User.find(id)
user = User.create(id)
user.save
user.destroy
The API imitates the ActiveRecord/ActiveResource one. Why not, that is what
people is used to see in controllers.
This also allows to separate testing in 2 parts:
- the User class test, for which mocha stubs YaPI calls
- the controller class, for which mocha stubs User class class
This makes the controller test much simpler, you need to stub User class to
return arrays of User, and not deal with stubing the complicated YaPI calls,
why should the controller know about those anyway. It is the User class the
class that is hiding that complexity.
I haven't enabled the User class tests because I get this when calling YaPI
calls in my machine:
TypeError: can't convert Array into String
/usr/lib64/ruby/vendor_ruby/1.8/dbus/type.rb:108:in `+'
/usr/lib64/ruby/vendor_ruby/1.8/dbus/type.rb:108:in `to_s'
/usr/lib64/ruby/vendor_ruby/1.8/dbus/marshall.rb:352:in `append'
However I wanted to post the patch for review (and I guess I still need to see
if it works, once the tests are done)
Another pattern is where to use static methods or not. Consider the destroy
method. If you implement it in the class User
def destroy
# do something with yapi and @id
end
You can define it as a class method:
def self.destroy(id)
# do something with yapi and id
end
And then define the previous one just as:
def destroy
self.class.destroy(@id)
end
Then you can use either as User.destroy("schubi") or as user.destroy (assuming
user is an User object)
--
Duncan Mac-Vicar P. - Engineering Manager, YaST
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nuernberg)
diff --git a/plugins/users/app/controllers/users_controller.rb b/plugins/users/app/controllers/users_controller.rb
index 59a0f1e..0e3d4e2 100644
--- a/plugins/users/app/controllers/users_controller.rb
+++ b/plugins/users/app/controllers/users_controller.rb
@@ -1,5 +1,4 @@
# import YastService class FIXME move into the model...
-require "yast_service"
include ApplicationHelper
@@ -8,132 +7,7 @@ class UsersController < ApplicationController
before_filter :login_required
def initialize
- @scr = Scr.instance
end
-
-#--------------------------------------------------------------------------------
-#
-#local methods
-#
-#--------------------------------------------------------------------------------
- def get_user_list
- @users = []
- parameters = {
- # how to index hash with users
- "index" => ["s", "uid"],
- # attributes to return for each user
- "user_attributes" => ["as", [ "cn" ]]
- }
- users_map = YastService.Call("YaPI::USERS::UsersGet", parameters)
- if users_map.nil?
- puts "something wrong happened -------------------------------------"
- else
- users_map.each do |key, val|
- user = User.new
- user.uid = key
- user.cn = val["cn"]
- @users << user
- end
- end
- end
-
- def get_user (id)
- parameters = {
- # user to find
- "uid" => [ "s", id ],
- # list of attributes to return;
- "user_attributes" => [ "as", [
- "cn", "uidNumber", "homeDirectory",
- "grouplist", "uid", "loginShell", "groupname"
- ]]
- }
- user_map = YastService.Call("YaPI::USERS::UserGet", parameters)
- # TODO check if it is not empty
-
- @user = User.new
-
- #FIXME why User.new (user_map) does not work?
-
- # convert camel-cased YaST keys to ruby's under_scored ones
- @user.grouplist = user_map["grouplist"]
- @user.home_directory = user_map["homeDirectory"]
- @user.groupname = user_map["groupname"]
- @user.login_shell = user_map["loginShell"]
- @user.uid = id
- @user.uid_number = user_map["uidNumber"]
- @user.cn = user_map["cn"]
-
- return true
- end
-
- # -------------------------------------------------------
- # modify existing user
- def update_user userId
- config = {
- "type" => [ "s", "local" ],
- "uid" => [ "s", @user.uid ]
- }
- # => FIXME convert ruby's under_scored keys to YaST's camel-cased ones
- data = {
- "uid" => [ "s", @user.uid]
- }
-
- ret = YastService.Call("YaPI::USERS::UserModify", config, data)
-
- logger.debug "Command returns: #{ret.inspect}"
-
- @error_string = ret
- return (ret == "")
- end
-
- # -------------------------------------------------------
- # add the new local user
- def add_user
-
- # FIXME mandatory parameters must be required on web-client side...
- config = {
- "type" => [ "s", "local" ]
- }
- data = {
- "uid" => [ "s", @user.uid]
- }
- # FIXME convert ruby's under_scored keys to YaST's camel-cased ones
- data["cn"] = [ "s", @user.cn ] unless @user.cn.blank?
- data["userPassword"] = [ "s", @user.user_password ] unless @user.user_password.blank?
- data["homeDirectory"] = [ "s", @user.home_directory ] unless @user.home_directory.blank?
- data["loginShell"] = [ "s", @user.login_shell ] unless @user.login_shell.blank?
- data["uidNumber"] = [ "s", @user.uid_number ] unless @user.uid_number.blank?
- data["groupname"] = [ "s", @user.groupname ] unless @user.groupname.blank?
-
- ret = YastService.Call("YaPI::USERS::UserAdd", config, data)
-
- logger.debug "Command returns: #{ret.inspect}"
-
- @error_string = ret
- return (ret == "")
- end
-
- # -------------------------------------------------------
- # delete existing local user
- def delete_user
-
- config = {
- "type" => [ "s", "local" ],
- "uid" => [ "s", @user.uid ]
- }
-
- ret = YastService.Call("YaPI::USERS::UserDelete", config)
- logger.debug "Command returns: #{ret}"
-
- @error_string = ret
- return (ret == "")
- end
-
-#--------------------------------------------------------------------------------
-#
-# actions
-#
-#--------------------------------------------------------------------------------
# GET /users
# GET /users.xml
@@ -142,7 +16,7 @@ class UsersController < ApplicationController
unless permission_check("org.opensuse.yast.modules.yapi.users.usersget")
render ErrorResult.error(403, 1, "no permission") and return
end
- get_user_list
+ @users = User.find_all
end
# GET /users/1
@@ -154,9 +28,17 @@ class UsersController < ApplicationController
if params[:id].blank?
render ErrorResult.error(404, 2, "empty parameter") and return
end
- unless get_user params[:id]
- render ErrorResult.error(404, 2, "user not found") and return
+
+ begin
+ # try to find the user, and 404 if it does not exist
+ @user = User.find(params[:id])
+ if @user.nil?
+ render ErrorResult.error(404, 2, "user not found") and return
+ end
+ rescue Exception => e
+ render ErrorResult.error(500, 2, e.message) and return
end
+
end
@@ -168,15 +50,12 @@ class UsersController < ApplicationController
render ErrorResult.error(403, 1, "no permission") and return
end
- @user = User.new
- if @user.update_attributes(params[:users])
- add_user
- if @error_string != ""
- render ErrorResult.error(404, @error_id, @error_string) and return
- end
- else
- render ErrorResult.error(404, 2, "wrong parameter") and return
+ begin
+ @user = User.create(params[:users])
+ rescue Exception => e
+ render ErrorResult.error(404, @error_id, @error_string) and return
end
+
render :show
end
@@ -186,19 +65,23 @@ class UsersController < ApplicationController
unless permission_check("org.opensuse.yast.modules.yapi.users.usermodify")
render ErrorResult.error(403, 1, "no permission") and return
end
- @user = User.new
+
if params[:users] && params[:users][:uid]
params[:id] = params[:users][:uid] #for sync only
end
- get_user params[:id]
- if @user.update_attributes(params[:users])
- update_user params[:id]
- if @error_string != ""
- render ErrorResult.error(404, @error_id, @error_string) and return
+
+ begin
+ begin
+ @user = User.find(params[:id])
+ rescue Exception => e
+ render ErrorResult.error(404, 2, e.message) and return
end
- else
- render ErrorResult.error(404, 2, "wrong parameter") and return
- end
+ @user.load_attributes(params[:users])
+ @user.save
+ rescue Exception => e
+ # FIXME here should be internal error I guess
+ render ErrorResult.error(404, 2, e.message) and return
+ end
render :show
end
@@ -209,14 +92,14 @@ class UsersController < ApplicationController
unless permission_check("org.opensuse.yast.modules.yapi.users.userdelete")
render ErrorResult.error(403, 1, "no permission") and return
end
- get_user params[:id]
- delete_user
- logger.debug "DELETE: #...@user.inspect}"
- if @error_string != ""
- render ErrorResult.error(404, @error_id, @error_string) and return
- else
- render :show
+
+ begin
+ @user = User.find(params[:id])
+ @user.destroy
+ rescue Exception => e
+ render ErrorResult.error(404, @error_id, e.message) and return
end
+ render :show
end
end
diff --git a/plugins/users/app/models/user.rb b/plugins/users/app/models/user.rb
index ba13e5b..8a0396f 100644
--- a/plugins/users/app/models/user.rb
+++ b/plugins/users/app/models/user.rb
@@ -1,55 +1,159 @@
+require 'yast_service'
+
+# User model, not ActiveRecord based, but a
+# thin model over the YaPI, with some
+# ActiveRecord like convenience API
class User
- attr_accessor :cn,
- :uid,
- :uid_number,
- :gid_number,
- :grouplist,
- :groupname,
- :home_directory,
- :login_shell,
- :user_password,
- :addit_data,
- :type
+ attr_accessor_with_default :cn, ""
+ attr_accessor_with_default :uid, ""
+ attr_accessor_with_default :uid_number, ""
+ attr_accessor_with_default :gid_number, ""
+ attr_accessor_with_default :grouplist, {}
+ attr_accessor_with_default :groupname, ""
+ attr_accessor_with_default :home_directory, ""
+ attr_accessor_with_default :login_shell, ""
+ attr_accessor_with_default :user_password, ""
+ attr_accessor_with_default :type, "local"
- def id
- @uid
+ def initialize
+ end
+
+ # users = User.find_all
+ def self.find_all
+ users = []
+ parameters = {
+ # how to index hash with users
+ "index" => ["s", "uid"],
+ # attributes to return for each user
+ "user_attributes" => ["as", [ "cn" ]]
+ }
+ users_map = YastService.Call("YaPI::USERS::UsersGet", parameters)
+ if users_map.nil?
+ raise "Can't get user list"
+ else
+ users_map.each do |key, val|
+ user = User.new
+ user.uid = key
+ user.cn = val["cn"]
+ users << user
+ end
+ end
+ users
end
- def id=(id_val)
- @uid = id_val
+ # load the attributes of the user
+ def self.find(id)
+ user = User.new
+ parameters = {
+ # user to find
+ "uid" => [ "s", id ],
+ # list of attributes to return;
+ "user_attributes" =>
+ [ "as", [ "cn", "uidNumber", "homeDirectory",
+ "grouplist", "uid", "loginShell", "groupname" ] ]
+ }
+ user_map = YastService.Call("YaPI::USERS::UserGet", parameters)
+
+ raise "Got no data while loading user attributes" if user_map.empty?
+
+ load_data(user_map)
+ user.uid = id
+ end
+
+ # User.destroy("joe")
+ def self.destroy(uid)
+ # delete existing local user
+ config = {
+ "type" => [ "s", "local" ],
+ "uid" => [ "s", uid ]
+ }
+
+ ret = YastService.Call("YaPI::USERS::UserDelete", config)
+ Rails.logger "Command returns: #{ret}"
+ # @error_string = ret
+ return (ret == "")
+ end
+
+ # user.destroy
+ def destroy
+ self.class.destroy(uid)
+ end
+
+ def save
+ config = { "type" => [ "s", "local" ],
+ "uid" => [ "s", @user.uid ]
+ }
+ data = retrieve_data
+ ret = YastService.Call("YaPI::USERS::UserModify", config, data)
+
+ logger.debug "Command returns: #{ret.inspect}"
+ raise ret if not ret.blank?
+ true
end
- def initialize
- @cn = ""
- @uid = ""
- @uid_number = ""
- @gid_number = ""
- @grouplist = {}
- @groupname = ""
- @home_directory = ""
- @login_shell = ""
- @user_password = ""
- @type = "local"
+ # load a internally used data hash
+ # with camel-cased values
+ def load_data(data)
+ attrs = {}
+ data.each do |key, value|
+ attrs.store(key.underscore, value)
+ end
+ load_attribures(attrs)
end
- def update_attributes usr
- return false if usr==nil
- @grouplist = usr[:grouplist]
- @home_directory = usr[:home_directory]
- @type = usr[:type]
- @groupname = usr[:groupname]
- @login_shell = usr[:login_shell]
- @user_password = usr[:user_password]
- @uid = usr[:uid]
- @uid_number = usr[:uid_number]
- @gid_number = usr[:gid_number]
- @cn = usr[:cn]
-
- return true
+ # load a hash of attributes
+ def load_attributes(attrs)
+ return false if attrs.nil?
+ attrs.each do |key, value|
+ if self.respond_to?(key.to_sym)
+ self.send("#{key}=".to_sym, value)
+ end
+ end
+ true
end
+ # retrieves the internally used data
+ # hash with camel-cased values
+ def retrieve_data
+ data = { }
+ [ :cn, :uid, :uid_number, :gid_number, :grouplist, :groupname, :home_directory, :login_shell, :user_password, :addit_data, :type ].each do |attr_name|
+ if self.respond_to?(attr_name)
+ attr = self.send(attr_name)
+ data.store(attr_name.to_s.camelize(:lower), ['s', attr]) unless attr.blank?
+ end
+ end
+ data
+ end
+
+ # create a user in the local system
+ def self.create(attrs)
+ config = {}
+ user = User.new
+ user.load_attributes(attrs)
+ data = user.retrieve_data
+
+ config.store("type", [ "s", "local" ])
+ data.store("uid", [ "s", user.uid])
+
+ ret = YastService.Call("YaPI::USERS::UserAdd", config, data)
+
+ Rails.logger.debug "Command returns: #{ret.inspect}"
+ # @error_string = ret
+ raise ret if not ret.blank?
+ user
+ end
+
+ def id
+ @uid
+ end
+
+ def id=(id_val)
+ @uid = id_val
+ end
+
+
def to_xml( options = {} )
xml = options[:builder] ||= Builder::XmlMarkup.new(options)
xml.instruct! unless options[:skip_instruct]
@@ -80,5 +184,4 @@ class User
return hash.to_json
end
-
end
diff --git a/plugins/users/test/functional/users_controller_test.rb b/plugins/users/test/functional/users_controller_test.rb
index fa977bb..fdd7067 100644
--- a/plugins/users/test/functional/users_controller_test.rb
+++ b/plugins/users/test/functional/users_controller_test.rb
@@ -2,6 +2,7 @@ require File.expand_path(File.dirname(__FILE__) + "/../test_helper")
require 'test/unit'
require 'rubygems'
require "scr"
+require "yast_service"
require 'mocha'
@@ -12,11 +13,11 @@ class UsersControllerTest < ActionController::TestCase
@request = ActionController::TestRequest.new
# http://railsforum.com/viewtopic.php?id=1719
@request.session[:account_id] = 1 # defined in fixtures
- Scr.any_instance.stubs(:initialize)
- Scr.any_instance.stubs(:execute).with(['/sbin/yast2', 'users', 'list']).returns({:stderr=>"schubi19 \nschubi2 \nschubi5 \ntuxtux \n", :exit=>16, :stdout=>""})
- Scr.any_instance.stubs(:execute).with(['/sbin/yast2', 'users', 'show', 'username=schubi5']).returns({:stderr=>"Full Name:\n\tschubi5\nList of Groups:\n\t\nDefault Group:\n\tusers\nHome Directory:\n\t/home/schubi5\nLogin Shell:\n\t/bin/bash\nLogin Name:\n\tschubi5\nUID:\n\t1005\n", :exit=>16, :stdout=>""})
+
+ User.stubs(:find_all).returns([])
end
+
test "access index" do
get :index
assert_response :success
@@ -37,12 +38,15 @@ class UsersControllerTest < ActionController::TestCase
end
test "access show" do
+ u = User.new
+ u.load_attributes({:uid => "schubi5"})
+ User.stubs(:find).with("schubi5").returns(u)
get :show, :id => "schubi5"
assert_response :success
end
test "access show with wrong user" do
- Scr.any_instance.stubs(:execute).with(['/sbin/yast2', 'users', 'show', 'username=schubi_not_found']).returns({:stderr=>"There is no such user.\n", :exit=>0, :stdout=>""})
+ User.stubs(:find).with("schubi_not_found").returns(nil)
get :show, :id => "schubi_not_found"
assert_response 404
end
