From ae7839ffaada72c522ffcd9b3f4f1cc04027a720 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Sat, 15 Nov 2008 06:23:09 +0000 Subject: Revise the user login code. * Remove user registration link and popup from the theme; this shouldn't be done in a popup. Use ajaxform to simplify the way that we load the login popup. * Create form.html.php, this is a template for Forge based forms. * Move user validation rules into User_Model and let forms populate the rules into their forms as useful. * Undo r18688's changes regarding the REST code. We should never accept a null resource, this breaks the REST abstraction. * Change login and user controllers to use Forge which lets us delete login.html.php and user.html.php since those now are generated by the theme-owned form template --- core/controllers/item.php | 4 -- core/controllers/rest.php | 17 ++------ modules/user/controllers/login.php | 45 ++++++++----------- modules/user/controllers/logout.php | 4 +- modules/user/controllers/user.php | 76 +++++++++++++++++++++++++------- modules/user/models/user.php | 5 +++ modules/user/views/login.html.php | 21 --------- modules/user/views/user.html.php | 34 --------------- themes/default/css/screen.css | 84 ++++++++++++++++++------------------ themes/default/js/user.js | 79 +++++++++------------------------ themes/default/views/form.html.php | 54 +++++++++++++++++++++++ themes/default/views/header.html.php | 40 ++++++++--------- themes/default/views/page.html.php | 3 +- 13 files changed, 226 insertions(+), 240 deletions(-) delete mode 100644 modules/user/views/login.html.php delete mode 100644 modules/user/views/user.html.php create mode 100644 themes/default/views/form.html.php diff --git a/core/controllers/item.php b/core/controllers/item.php index 286eb66f..78ee7b0b 100644 --- a/core/controllers/item.php +++ b/core/controllers/item.php @@ -21,10 +21,6 @@ class Item_Controller extends REST_Controller { protected $resource_type = "item"; public function _get($item) { - if (empty($item)) { - // A null item is not allowed for albums or photos. - return Kohana::show_404(); - } // Redirect to the more specific resource type, since it will render // differently. We could also just delegate here, but it feels more appropriate // to have a single canonical resource mapping. diff --git a/core/controllers/rest.php b/core/controllers/rest.php index 6e0acbcb..09c85653 100644 --- a/core/controllers/rest.php +++ b/core/controllers/rest.php @@ -49,23 +49,14 @@ abstract class REST_Controller extends Controller { protected $resource_type = null; - public function dispatch($id=null) { + public function dispatch($id) { if ($this->resource_type == null) { throw new Exception("@todo ERROR_MISSING_RESOURCE_TYPE"); } - if ($id != null) { - // @todo this needs security checks - $resource = ORM::factory($this->resource_type, $id); - if (!$resource->loaded) { - return Kohana::show_404(); - } - } else if (request::method() == "get") { - // A null id and a request method of "get" just returns an empty form - // @todo figure out how to handle the input without and id - // @todo do we use put for create and post for update? - $resource = null; - } else { + // @todo this needs security checks + $resource = ORM::factory($this->resource_type, $id); + if (!$resource->loaded) { return Kohana::show_404(); } /** diff --git a/modules/user/controllers/login.php b/modules/user/controllers/login.php index 7c70bb0f..a0e9f403 100644 --- a/modules/user/controllers/login.php +++ b/modules/user/controllers/login.php @@ -19,38 +19,29 @@ */ class Login_Controller extends Controller { public function index() { - $form = new Forge(); - $form->input("username")->rules("required|length[4,32]"); - $form->password("password")->rules("required|length[5,40]"); - $form->submit("Login"); - print $form->render("login.html", true); - } - - public function process() { - $form = new Forge("login.html", true); - $form->input("username")->rules("required|length[4,32]"); - $form->password("password")->rules("required|length[5,40]"); - $form->submit("Login"); + $form = new Forge("login", "", "post", array("id" => "gLogin")); + $group = $form->group(_("Login")); + $group->input("name")->label(_("Name"))->id("gName")->class(null); + $group->password("password")->label(_("Password"))->id("gPassword")->class(null); + $group->submit(_("Login")); + $form->hidden("continue")->value($this->input->get("continue")); + $group->inputs["name"]->error_messages("invalid_login", _("Invalid name or password")); - $response = array(); if ($form->validate()) { - // Load the user - $user = ORM::factory("user")->where("name", $form->username->value)->find(); - if (!$user->loaded) { - $response["error_message"] = _("Invalid username or password"); - } else { - if (user::is_correct_password($user, $form->password->value)) { - user::login($user); - $response["error_message"] = ""; - } else { - $response["error_message"] = _("Invalid username or password"); + $user = ORM::factory("user")->where("name", $group->inputs["name"]->value)->find(); + if ($user->loaded && + user::is_correct_password($user, $group->password->value)) { + user::login($user); + $continue = $form->hidden["continue"]->value; + if ($continue) { + url::redirect($form->hidden["continue"]->value); } + return; + } else { + $group->inputs["name"]->add_error("invalid_login", 1); } - } else { - $response["error_message"] = _("Invalid username or password"); } - print json_encode($response); + print $form->render("form.html", false); } - } \ No newline at end of file diff --git a/modules/user/controllers/logout.php b/modules/user/controllers/logout.php index 34f27fff..524c79f8 100644 --- a/modules/user/controllers/logout.php +++ b/modules/user/controllers/logout.php @@ -24,6 +24,8 @@ class Logout_Controller extends Controller { } catch (Exception $e) { Kohana::log("error", $e); } - print json_encode(array("logout" => true)); + if ($this->input->get("continue")) { + url::redirect($this->input->get("continue")); + } } } \ No newline at end of file diff --git a/modules/user/controllers/user.php b/modules/user/controllers/user.php index 41cb9da5..a1085ae5 100644 --- a/modules/user/controllers/user.php +++ b/modules/user/controllers/user.php @@ -20,25 +20,59 @@ class User_Controller extends REST_Controller { protected $resource_type = "user"; + /** + * Return the form for creating / modifying users. + */ + private function _get_form($user) { + $form = new Forge("user/{$user->id}", "", "post", array("id" => "gUser")); + $group = $form->group(_("User Info")); + $group->input("name") + ->label(_("Name")) + ->id("gName") + ->class(null) + ->value($user->name); + $group->input("display_name") + ->label(_("Display Name")) + ->id("gDisplayName") + ->class(null) + ->value($user->display_name); + $group->password("password") + ->label(_("Password")) + ->id("gPassword") + ->class(null); + $group->input("email") + ->label(_("Email")) + ->id("gEmail") + ->class(null) + ->value($user->email); + $group->submit(_("Modify")); + $form->hidden("continue")->value($this->input->get("continue")); + + $this->_add_validation_rules(ORM::factory("user")->validation_rules, $form); + + return $form; + } + + /** + * @todo Refactor this into a more generic location + */ + private function _add_validation_rules($rules, $form) { + foreach ($form->inputs as $name => $input) { + if (isset($input->inputs)) { + $this->_add_validation_rules($rules, $input); + } + if (isset($rules[$name])) { + $input->rules($rules[$name]); + } + } + } + /** * @see Rest_Controller::_get($resource) */ public function _get($user) { - $userView = new View("user.html"); - if (empty($user)) { - // @todo remove this when rest_controller is changed to handle a post with no id - $user = ORM::factory("user"); - $user->save(); - // @todo remove this when rest_controller is changed to handle a post with no id ^ - $userView->user_id = $user->id; - $userView->action = _("User Registration"); - $userView->button_text = _("Register"); - } else { - $userView->user_id = $user->id; - $userView->action = _("User Modify"); - $userView->button_text = _("Modify"); - } - print $userView; + $form = $this->_get_form($user); + print $form->render("form.html", false); } /** @@ -51,7 +85,17 @@ class User_Controller extends REST_Controller { /** * @see Rest_Controller::_post($resource) */ - public function _post($resource) { + public function _post($user) { + $form = $this->_get_form($user); + if ($form->validate()) { + // @todo if we use the Validation class here, the ORM can just read the inputs directly. We + // need to investigate that. + // + // @todo + // Verify the user input, store it in the object. + // Show errors on validation failure. + // On success, redirect if there's a form->continue, else show an empty page. + } throw new Exception("@todo User_Controller::_post NOT IMPLEMENTED"); } diff --git a/modules/user/models/user.php b/modules/user/models/user.php index b09af033..1c56b34e 100644 --- a/modules/user/models/user.php +++ b/modules/user/models/user.php @@ -20,6 +20,11 @@ class User_Model extends ORM { protected $has_and_belongs_to_many = array("groups"); + var $validation_rules = array( + "name" => "required|length[4,32]", + "email" => "valid_email", + "password" => "required|length[5,40]"); + public function __set($column, $value) { switch ($column) { case "password": diff --git a/modules/user/views/login.html.php b/modules/user/views/login.html.php deleted file mode 100644 index 78e03b46..00000000 --- a/modules/user/views/login.html.php +++ /dev/null @@ -1,21 +0,0 @@ - -
"> -
- -
    -
  • - - -
    • -
  • - - -
    • -
  • - " /> -
    • -
-
-
-
-
diff --git a/modules/user/views/user.html.php b/modules/user/views/user.html.php deleted file mode 100644 index 614645e5..00000000 --- a/modules/user/views/user.html.php +++ /dev/null @@ -1,34 +0,0 @@ - -
"> -
- -
    -
  • - - - -
    • -
  • - - - -
    • -
  • - - -
    • -
  • - - - -
    • -
  • - - -
    • -
  • - -
    • -
-
-
diff --git a/themes/default/css/screen.css b/themes/default/css/screen.css index 5261706a..eeb4eeb9 100644 --- a/themes/default/css/screen.css +++ b/themes/default/css/screen.css @@ -576,7 +576,7 @@ table.gMetadata td.toggle { /** ** ******************************************************************* - * 8) Forms (general and specific) + * 8) Forms (general and specific) ** ******************************************************************* */ @@ -596,15 +596,15 @@ label { cursor: help; } -input[type="text"], +input[type="text"], input[type="password"], -textarea, -select { +textarea, +select { } -input[type="text"], -input[type="password"], -textarea, +input[type="text"], +input[type="password"], +textarea, .gValidationRule { width: 40%; } @@ -645,7 +645,7 @@ optgroup { select { } -textarea { +textarea { width: 99%; height: 12em; } @@ -655,16 +655,16 @@ button { /* ~~~~~~~~~ Form layout ~~~~~~~~~~ */ -form ul, form li { - list-style: none !important; +form ul, form li { + list-style: none !important; } -form ul { - margin: 0; +form ul { + margin: 0; padding: 0; } -form li { - margin-top: .5em; +form li { + margin-top: .5em; padding: .3em 1.5em .3em 1em; } @@ -676,16 +676,16 @@ form ul ul li { float: left; } -input, +input, textarea { - display: block; + display: block; clear: both; } /* ~~~~~~~~~ Inline fieldsets ~~~~~~~~~~ */ .gInline li { - float: left; + float: left; margin: 0; padding: .3em .5em .4em; text-align: left; @@ -694,7 +694,7 @@ textarea { .gInline input { } -.gInline input[type="Submit"] { +.gInline input[type="Submit"] { margin-top: 1em; } @@ -702,16 +702,16 @@ textarea { margin-top: 0; } -.gInline input[type="text"], -.gInline input[type="password"], -.gInline textarea, -.gInline .gValidationRule { +.gInline input[type="text"], +.gInline input[type="password"], +.gInline textarea, +.gInline .gValidationRule { width: 10em; } -input:focus, -textarea:focus, -option:focus { +input:focus, +textarea:focus, +option:focus { background-color: #ffc; } @@ -720,13 +720,13 @@ option:focus { .gRequired { } -ul.gError, -li.gError { +ul.gError, +li.gError { background-color: #ffdcdc; } -.gError label, -.gValidationRule { +.gError label, +.gValidationRule { color: red; } @@ -735,24 +735,24 @@ li.gError { margin-top: .5em; } -form.gError input[type="text"], -li.gError input[type="text"], -form.gError input[type="password"], -li.gError input[type="password"], -form.gError input[type="checkbox"], -li.gError input[type="checkbox"], -form.gError input[type="radio"], -li.gError input[type="radio"], -form.gError textarea, -li.gError textarea, -form.gError select, -li.gError select { +form.gError input[type="text"], +li.gError input[type="text"], +form.gError input[type="password"], +li.gError input[type="password"], +form.gError input[type="checkbox"], +li.gError input[type="checkbox"], +form.gError input[type="radio"], +li.gError input[type="radio"], +form.gError textarea, +li.gError textarea, +form.gError select, +li.gError select { border: 2px solid red; } /* ~~~~~~~~ form font size ~~~~~~ */ -#gHeader form, #gSidebar form { +#gHeader form, #gSidebar form { font-size: .9em; } diff --git a/themes/default/js/user.js b/themes/default/js/user.js index b389a67e..42ab1aa7 100644 --- a/themes/default/js/user.js +++ b/themes/default/js/user.js @@ -1,66 +1,29 @@ -$(document).ready(function() { - $("#gLoginForm").submit(function() { - process_login(); - return false; - }); - $("#gLogoutLink").click(function() { - process_logout(); - return false; - }); -}); - -function show_form(formName) { - $(formName + "Link").css({display: "none"}); - $(formName + "Text").css({display: "inline"}); - $(formName + "Close").css({display: "inline"}); - var url = $(formName + "Form").attr("formSrc"); - $.get(url, null, function(data, textStatus) { - $(formName + "Form").html(data); - $(formName + "Form").css({display: "block"}); +function show_login(url) { + $("#gLoginLink").hide(); + $("#gLoginClose").show(); + $.get(url, function(data) { + $("#gLoginFormContainer").html(data); + ajaxify_login_form(); }); } -function hide_form(formName) { - $(formName + "Link").css({display: "inline"}); - $(formName + "Form").css({display: "none"}); - $(formName + "Form").html(""); - $(formName + "Text").css({display: "none"}); - $(formName + "Close").css({display: "none"}); -} - -function process_login() { - $.ajax({ - url: $("#gLogin").attr("action"), - type: "POST", - data: $("#gLogin").serialize(), - dataType: "json", - error: function(XMLHttpRequest, textStatus, errorThrown) { - alert("textStatus: " + textStatus + "\nerrorThrown: " + errorThrown); - }, - success: function(data, textStatus) { - if (data.error_message != "") { - $("#gLoginMessage").html(data.error_message); - $("#gLoginMessage").css({display: "block"}); - $("#gLogin").addClass("gError"); - } else { +function ajaxify_login_form() { + $("form#gLogin").ajaxForm({ + target: "#gLoginFormContainer", + success: function(responseText, statusText) { + if (!responseText) { window.location.reload(); + } else { + ajaxify_login_form(); } - } + }, }); } -function process_logout() { - $.ajax({ - url: $("#gLogoutLink").attr("href"), - type: "GET", - dataType: "json", - error: function(XMLHttpRequest, textStatus, errorThrown) { - alert("textStatus: " + textStatus + "\nerrorThrown: " + errorThrown); - }, - success: function(data, textStatus) { - if (data.logout) { - window.location.reload(); - } - } - }); -} \ No newline at end of file +function close_login() { + $("#gLogin").remove(); + $("#gLoginClose").hide(); + $("#gLoginLink").show(); + $("input#gUsername").val(""); + $("input#gPassword").val(""); +} diff --git a/themes/default/views/form.html.php b/themes/default/views/form.html.php new file mode 100644 index 00000000..bc8d1339 --- /dev/null +++ b/themes/default/views/form.html.php @@ -0,0 +1,54 @@ +"; +} +if ($title) { + print ""; +} + +function DrawForm($inputs, $level=1) { + $error_messages = array(); + $prefix = str_repeat(" ", $level); + + foreach ($inputs as $input) { + if ($input->type == 'group') { + print "$prefix
\n"; + print "$prefix $input->name\n"; + print "$prefix \n"; + print "$prefix
\n"; + } else { + if ($input->error_messages()) { + $error_messages = array_merge($error_messages, $input->error_messages()); + print "$prefix
  • \n"; + } else { + print "$prefix
  • \n"; + } + if ($input->label()) { + print $prefix . " " . $input->label() . "\n"; + } + print $prefix . " " . $input->render() . "\n"; + print "$prefix
    • \n"; + if ($input->message()) { + print "$prefix
  • \n"; + print $prefix . " " . $input->message() . "\n"; + print "$prefix
    • \n"; + } + } + } + if ($error_messages) { + print "$prefix
    \n"; + foreach ($error_messages as $message) { + print "

    $message

    "; + } + print "$prefix

    \n"; + } +} +DrawForm($inputs); + +print($close); +?> diff --git a/themes/default/views/header.html.php b/themes/default/views/header.html.php index 24dd9331..8cba82ba 100644 --- a/themes/default/views/header.html.php +++ b/themes/default/views/header.html.php @@ -2,31 +2,25 @@ " src="url("images/logo.png") ?>" />

    title_edit ?>

    -
    +
    + +