Cleanup acf-util/password to move logic from the controller to the model

2 files changed, 125 insertions, 128 deletions

diff --git a/app/acf-util/password-controller.lua b/app/acf-util/password-controller.lua
index 0715b50..264aadc 100644
--- a/app/acf-util/password-controller.lua
+++ b/app/acf-util/password-controller.lua
@@ -10,61 +10,15 @@ end
 function mymodule.editme(self)
 	-- just to make sure can't modify any other user from this action
 	self.clientdata.userid = self.sessiondata.userinfo.userid
-	return self.handle_form(self, function()
-			local value = self.model.read_user(self, self.sessiondata.userinfo.userid)
-			-- We don't allow a user to modify his own roles
-			-- Since they can't modify roles, we should restrict the available options for home
-			value.value.home.option = {""}
-			local tmp1, tmp2 = roles.get_roles_perm(self, value.value.roles.value)
-	        	table.sort(tmp2)
-			for i,h in ipairs(tmp2) do
-				if h ~= "/acf-util/logon/logoff" and h ~= "/acf-util/logon/logon" then
-					value.value.home.option[#value.value.home.option+1] = h
-				end
-			end
-			value.value.roles = nil
-			return value
-		end, function(self, value)
-			-- If password and password_confirm are blank, don't set them
-			local pw, pwc
-			if value.value.password.value == "" and value.value.password_confirm.value == "" then
-				pw = value.value.password
-				pwc = value.value.password_confirm
-				value.value.password = nil
-				value.value.password_confirm = nil
-			end
-			value = self.model.update_user(self, value)
-			if pw then
-				value.value.password = pw
-				value.value.password_confirm = pwc
-			end
-			return value
-		end, self.clientdata, "Save", "Edit My Settings", "Saved user")
+	return self.handle_form(self, self.model.read_user_without_roles, self.model.update_user, self.clientdata, "Save", "Edit My Settings", "Saved user")
 end
 
 function mymodule.edituser(self)
-	return self.handle_form(self, function()
-			return self.model.read_user(self, self.clientdata.userid)
-		end, function(self, value)
-			-- If password and password_confirm are blank, don't set them
-			local pw, pwc
-			if value.value.password.value == "" and value.value.password_confirm.value == "" then
-				pw = value.value.password
-				pwc = value.value.password_confirm
-				value.value.password = nil
-				value.value.password_confirm = nil
-			end
-			value = self.model.update_user(self, value)
-			if pw then
-				value.value.password = pw
-				value.value.password_confirm = pwc
-			end
-			return value
-		end, self.clientdata, "Save", "Edit User Settings", "Saved user")
+	return self.handle_form(self, self.model.read_user, self.model.update_user, self.clientdata, "Save", "Edit User Settings", "Saved user")
 end
 
 function mymodule.newuser(self)
-	return self.handle_form(self, function() return self.model.read_user(self) end, self.model.create_user, self.clientdata, "Create", "Create New User", "Created user")
+	return self.handle_form(self, self.model.get_new_user, self.model.create_user, self.clientdata, "Create", "Create New User", "Created user")
 end
 
 function mymodule.deleteuser(self)
diff --git a/app/acf-util/password-model.lua b/app/acf-util/password-model.lua
index 972269d..0016e2f 100644
--- a/app/acf-util/password-model.lua
+++ b/app/acf-util/password-model.lua
@@ -17,26 +17,24 @@ local weak_password = function(password)
 	return false, nil
 end
 
--- validate the settings (ignore password if it's nil)
-local validate_settings = function(settings)
-	-- Username, password, roles, skin, and home are allowed to not exist, just leave the same
-	-- Set errtxt when entering invalid values
+local validate_settings = function(settings, create)
+	-- Set errtxt when encountering invalid values
 	if (#settings.value.userid.value == 0) then settings.value.userid.errtxt = "You need to enter a valid userid!" end
 	if string.find(settings.value.userid.value, "[^%w_]") then settings.value.userid.errtxt = "Can only contain letters, numbers, and '_'" end
-	if settings.value.username and string.find(settings.value.username.value, "%p") then settings.value.username.errtxt = "Cannot contain punctuation" end
-	if settings.value.password then
-		if (#settings.value.password.value == 0) then
-			settings.value.password.errtxt = "Password cannot be blank!"
-		elseif (not settings.value.password_confirm) or (settings.value.password.value ~= settings.value.password_confirm.value) then
-			settings.value.password.errtxt = "You entered wrong password/confirmation"
-		else
-			local weak_password_result, weak_password_errormessage = weak_password(settings.value.password.value)
-			if (weak_password_result) then settings.value.password.errtxt = weak_password_errormessage end
-		end
+	if string.find(settings.value.username.value, "%p") then settings.value.username.errtxt = "Cannot contain punctuation" end
+	-- Blank password is allowed for edit, indicates to leave the same
+	if (#settings.value.password.value == 0) and create then
+		settings.value.password.errtxt = "Password cannot be blank!"
+	elseif (settings.value.password.value ~= settings.value.password_confirm.value) then
+		settings.value.password.errtxt = "You entered wrong password/confirmation"
+	elseif (#settings.value.password.value ~= 0) then
+		local weak_password_result, weak_password_errormessage = weak_password(settings.value.password.value)
+		if (weak_password_result) then settings.value.password.errtxt = weak_password_errormessage end
 	end
+	-- roles will not exist for editme action
 	if settings.value.roles then modelfunctions.validatemulti(settings.value.roles) end
-	if settings.value.skin then modelfunctions.validateselect(settings.value.skin) end
-	if settings.value.home then modelfunctions.validateselect(settings.value.home) end
+	modelfunctions.validateselect(settings.value.skin)
+	modelfunctions.validateselect(settings.value.home)
 
 	-- Return false if any errormessages are set
 	for name,value in pairs(settings.value) do
@@ -48,13 +46,80 @@ local validate_settings = function(settings)
 	return true, settings
 end
 
-function mymodule.create_user(self, settings)
-	return mymodule.update_user(self, settings, true)
+local function get_blank_user(self)
+	local result = cfe({ type="group", value={}, label="User Account" })
+	
+	if not avail_roles then
+		avail_roles = roles.list_all_roles(self)
+		for x,role in ipairs(avail_roles) do
+			if role==roles.guest_role then
+				table.remove(avail_roles,x)
+				break
+			end
+		end
+	end
+	
+	-- Call into skins controller to get the list of skins
+	if not avail_skins then
+		avail_skins = {""}
+		local contrl = self:new("acf-util/skins")
+		skins = contrl.model.get_update(contrl)
+		contrl:destroy()
+		for i,s in ipairs(skins.value.skin.option) do
+			avail_skins[#avail_skins + 1] = s.value or s
+		end
+	end
+
+	-- Call into ?? controller to get the list of home actions
+	if not avail_homes then
+		avail_homes = {""}
+		local tmp1, tmp2 = roles.get_all_permissions(self)
+	        table.sort(tmp2)
+		for i,h in ipairs(tmp2) do
+			avail_homes[#avail_homes+1] = h
+		end
+	end
+
+	-- Passwords are set to empty string
+	result.value.userid = cfe({ value=user, label="User id", seq=1 })
+	result.value.username = cfe({ value="", label="Real name", seq=2 })
+	result.value.password = cfe({ type="password", value="", label="Password", seq=4 })
+	result.value.password_confirm = cfe({ type="password", value="", label="Password (confirm)", seq=5 })
+	result.value.roles = cfe({ type="multi", value={}, label="Roles", option=avail_roles or {}, seq=3 })
+	result.value.skin = cfe({ type="select", value="", label="Skin", option=avail_skins or {""}, seq=7 })
+	result.value.home = cfe({ type="select", value="", label="Home", option=avail_homes or {""}, seq=6 })
+
+	return result
 end
 
-function mymodule.update_user(self, settings, create)
-	local success, settings = validate_settings(settings)
+local function get_user(self, userid)
+	local result = get_blank_user(self)
+	result.value.userid.key = true
+	result.value.userid.value = userid
+
+	if result.value.userid.value ~= "" then
+		result.value.userid.readonly = true
+		local userinfo = authenticator.get_userinfo(self, result.value.userid.value)
+		if not userinfo then
+			result.value.userid.errtxt = "User does not exist"
+			userinfo = {}
+		else
+			for n,v in pairs(userinfo) do
+				if result.value[n] and n ~= "password" then result.value[n].value = v end
+			end
+		end
+	end
+
+	return result
+end
 
+function mymodule.create_user(self, settings, submit)
+	return mymodule.update_user(self, settings, submit, true)
+end
+
+function mymodule.update_user(self, settings, submit, create)
+	local success, settings = validate_settings(settings, create)
+	
 	if success then
 		local userinfo = authenticator.get_userinfo(self, settings.value.userid.value)
 		if userinfo and create then
@@ -69,7 +134,11 @@ function mymodule.update_user(self, settings, create)
 	if success then
 		local userinfo = {}
 		for name,val in pairs(settings.value) do
-			userinfo[name] = val.value
+			-- If password is blank, don't set it
+			if name == "password" and val.value == "" then
+			else
+				userinfo[name] = val.value
+			end
 		end
 		success = authenticator.write_userinfo(self, userinfo)
 	end
@@ -85,70 +154,44 @@ function mymodule.update_user(self, settings, create)
 	return settings
 end
 
+function mymodule.read_user(self, clientdata)
+	-- create a temp result so handle_clientdata only handles userid
+	local tmpresult = cfe({type="group", value={userid=cfe()} })
+	self.handle_clientdata(tmpresult, clientdata)
+	return get_user(self, tmpresult.value.userid.value)
+end
 
-function mymodule.read_user(self, user)
-	local result = {}
-        result.userid = cfe({ value=user, label="User id", seq=1 })
-	if user and user ~= "" then
-		result.userid.readonly = true
-	end
-	
-	local userinfo = {}
-	if not user then
-		local userlist = authenticator.list_users(self)
-		if #userlist == 0 then
-			-- There are no users yet, suggest some values
-			result.userid.value = "root"
-			userinfo = { userid="root", username="Admin account", roles={"ADMIN"} }
-		end
-	else
-		userinfo = authenticator.get_userinfo(self, user)
-		if not userinfo then
-			result.userid.errtxt = "User does not exist"
-			userinfo = {}
-		end
-	end
+function mymodule.get_new_user(self, clientdata)
+	local result = get_blank_user(self)
 
-	if not avail_roles then
-		avail_roles = roles.list_all_roles(self)
-		for x,role in ipairs(avail_roles) do
-			if role==roles.guest_role then
-				table.remove(avail_roles,x)
-				break
-			end
-		end
-	end
-	
-	-- Call into skins controller to get the list of skins
-	if not avail_skins then
-		avail_skins = {""}
-		local contrl = self:new("acf-util/skins")
-		skins = contrl.model.get_update(contrl)
-		contrl:destroy()
-		for i,s in ipairs(skins.value.skin.option) do
-			avail_skins[#avail_skins + 1] = s.value or s
-		end
+	-- Special handling for case where no users exist yet
+	local userlist = authenticator.list_users(self)
+	if #userlist == 0 then
+		-- There are no users yet, suggest some values
+		result.value.userid.value = "root"
+		result.value.username.value = "Admin account"
+		result.value.roles.value = {"ADMIN"}
 	end
 
-	-- Call into ?? controller to get the list of home actions
-	if not avail_homes then
-		avail_homes = {""}
-		local tmp1, tmp2 = roles.get_all_permissions(self)
-	        table.sort(tmp2)
-		for i,h in ipairs(tmp2) do
-			avail_homes[#avail_homes+1] = h
+	return result
+end
+
+function mymodule.read_user_without_roles(self, clientdata)
+	local result = mymodule.read_user(self, clientdata)
+	
+	-- We don't allow a user to modify his own roles
+	-- Since they can't modify roles, we should restrict the available options for home
+	result.value.home.option = {""}
+	local tmp1, tmp2 = roles.get_roles_perm(self, result.value.roles.value)
+	table.sort(tmp2)
+	for i,h in ipairs(tmp2) do
+		if h ~= "/acf-util/logon/logoff" and h ~= "/acf-util/logon/logon" then
+			result.value.home.option[#result.value.home.option+1] = h
 		end
 	end
+	result.value.roles = nil
 
-	-- Passwords are set to empty string
-	result.username = cfe({ value=userinfo.username or "", label="Real name", seq=2 })
-	result.password = cfe({ type="password", value="", label="Password", seq=4 })
-	result.password_confirm = cfe({ type="password", value="", label="Password (confirm)", seq=5 })
-	result.roles = cfe({ type="multi", value=userinfo.roles or {}, label="Roles", option=avail_roles or {}, seq=3 })
-	result.skin = cfe({ type="select", value=userinfo.skin or "", label="Skin", option=avail_skins or {""}, seq=7 })
-	result.home = cfe({ type="select", value=userinfo.home or "", label="Home", option=avail_homes or {""}, seq=6 })
-
-	return cfe({ type="group", value=result, label="User Account" })
+	return result
 end
 
 function mymodule.get_users(self)
@@ -158,7 +201,7 @@ function mymodule.get_users(self)
 	table.sort(userlist)
 	
 	for x,user in pairs(userlist) do
-		users[#users+1] = mymodule.read_user(self, user)
+		users[#users+1] = get_user(self, user)
 	end
 
 	return cfe({ type="group", value=users, label="User Accounts" })