[aur-dev] [PATCH v2 0/3] Add JavaScript method to edit comments
For a better user experience, enhance the comment edit form with a JavaScript method to edit comments on the same page, using a customized RPC interface. If JavaScript is not available, the page falls back to a standard web page, where a user can edit a comment. Marcel Korpel (3): pkg_comment_form.php: Make printing of header conditional aurjson.class.php: Add method load_comment pkg_comments.php: Add JavaScript function to edit comments web/html/css/aurweb.css | 6 ++++++ web/html/images/ajax-loader.gif | Bin 0 -> 723 bytes web/html/index.php | 4 ++++ web/lib/aurjson.class.php | 26 +++++++++++++++++++++++++- web/template/pkg_comment_form.php | 5 +++++ web/template/pkg_comments.php | 30 ++++++++++++++++++++++++++++++ 6 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 web/html/images/ajax-loader.gif -- 2.4.6
For use in the new RPC interface to edit comments, the form shouldn't always print a header. Make this conditional. Signed-off-by: Marcel Korpel <marcel.korpel@gmail.com> --- Change since v1: * Include the <form> element in the markup when using the RPC interface. This is necessary to use normal form submission to save an edited comment. web/template/pkg_comment_form.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/web/template/pkg_comment_form.php b/web/template/pkg_comment_form.php index 16a92b1..24eb908 100644 --- a/web/template/pkg_comment_form.php +++ b/web/template/pkg_comment_form.php @@ -1,5 +1,8 @@ +<?php /* $no_header will be set when called from aurjson.class.php */ +if (!isset($no_header)): ?> <div id="generic-form" class="box"> <h2><?= (isset($comment_id)) ? __('Edit comment for: %s', htmlspecialchars($pkgbase_name)) : __("Add Comment"); ?></h2> +<?php endif; ?> <form action="<?= get_pkgbase_uri($pkgbase_name) ?>" method="post"> <fieldset> <?php @@ -23,5 +26,7 @@ if (isset($_REQUEST['comment']) && check_token()) { </p> </fieldset> </form> +<?php if (isset($no_header)): ?> </div> +<?php endif; -- 2.4.6
Signed-off-by: Marcel Korpel <marcel.korpel@gmail.com> --- Changes since v1: * Drop any POST method. * Drop save_comment() method and use the normal web form for submission. web/lib/aurjson.class.php | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/web/lib/aurjson.class.php b/web/lib/aurjson.class.php index a272741..a4f78d4 100644 --- a/web/lib/aurjson.class.php +++ b/web/lib/aurjson.class.php @@ -14,7 +14,7 @@ class AurJSON { private $version = 1; private static $exposed_methods = array( 'search', 'info', 'multiinfo', 'msearch', 'suggest', - 'suggest-pkgbase' + 'suggest-pkgbase', 'load-comment' ); private static $exposed_fields = array( 'name', 'name-desc' @@ -477,5 +477,29 @@ class AurJSON { return json_encode($result_array); } + + private function load_comment($http_data) { + $comment_id = $http_data['arg']; + if (isset($_GET['base_id']) && isset($_GET['pkgbase_name'])) { + $base_id = $_GET['base_id']; + $pkgbase_name = $_GET['pkgbase_name']; + + list($user_id, $comment) = comment_by_id($comment_id); + + if (!has_credential(CRED_COMMENT_EDIT, array($user_id)) || is_null($comment)) { + $output = false; + } else { + $no_header = true; + ob_start(); + include('pkg_comment_form.php'); + $html = ob_get_clean(); + $output = array('form' => $html); + } + } else { + $output = false; + } + + return json_encode($output); + } } -- 2.4.6
Signed-off-by: Marcel Korpel <marcel.korpel@gmail.com> --- Changes since v1: * Drop the AJAX form submission, instead use a normal form. * Pass variables $base_id and $pkgbase_name that are now needed when rendering the form. * Add loading indicator. web/html/css/aurweb.css | 6 ++++++ web/html/images/ajax-loader.gif | Bin 0 -> 723 bytes web/html/index.php | 4 ++++ web/template/pkg_comments.php | 30 ++++++++++++++++++++++++++++++ 4 files changed, 40 insertions(+) create mode 100644 web/html/images/ajax-loader.gif diff --git a/web/html/css/aurweb.css b/web/html/css/aurweb.css index b33726c..4fb256f 100644 --- a/web/html/css/aurweb.css +++ b/web/html/css/aurweb.css @@ -124,6 +124,12 @@ opacity: 1; } +.ajax-loader { + float: right; + position: relative; + top: 4px; +} + legend { padding: 1em 0; } diff --git a/web/html/images/ajax-loader.gif b/web/html/images/ajax-loader.gif new file mode 100644 index 0000000000000000000000000000000000000000..df07e7ec2076177c99b53d4d29a45f0db6b06a9a GIT binary patch literal 723 zcmZ?wbhEHb6ky<H_`<;O|NnpEv{esZe7gDQ-MdfU%`;a6x#5jFXKlVxKmX+MtIz$a zw&qPbv~b7uriG_ZU4Ic+v&}4Hb>Wo5uik&V|NP^(AHU+;_dI&}>C3lYM=m|vboAbp zdv88|`N04KivPL&TtkAL9RpmA^bD98f#Qn)q@0UV6H8K46v{J8G87WC5-W1@6I1ju z^V0Ge6o0aCasyTAfJ^{6l7UrML7^`tbKa5#T#rsMt#c4)wm4&2aJl;4?H%*^*q;ct zZ+YZ!f=91--8C-PwbPuinV^!8D8ZUAZ$+j|`^0?*ZXH_r=F;-s=Wq7D-W{Q@F^9F$ zTCh`s37bYUpw-=pI*&V4IF+P$l9wbc(l{x7eoOCbBdG(^nGZDWjsAGTTd?u$#mhT{ z{bn8t<<=6J=66T{n^C4fqn2>E3WhNCJ~l~G@x1uTreFAcY2|b4S-i`cPqf%2ZE*i3 z+J9zZu_cRC<?3tQyR_y8DPl9p2ofIGHbp#h37ovc<5E&ksO!lsv5&0c-cGyCn07cm z@P#sC?}=w8Sd-^@t-ShG3aj7DA;zc_#<r~3l(a1KW^3Z~jK_<%<<5%bQ+V^YX?vpJ z17^MHzAF7QOqk+z8O+R1FWC1Why$CG^dV+F0lH_!rgy7~WK@H;@IEkI|9iVk!F29# fT}NgWw#xj9(`7JWbB<iU1Zx11Y=$)`iGTqBb+`}S literal 0 HcmV?d00001 diff --git a/web/html/index.php b/web/html/index.php index 175a533..7068d76 100644 --- a/web/html/index.php +++ b/web/html/index.php @@ -160,6 +160,10 @@ if (!empty($tokens[1]) && '/' . $tokens[1] == get_pkg_route()) { header("Content-Type: text/css"); readfile("./$path"); break; + case "/images/ajax-loader.gif": + header("Content-Type: image/gif"); + readfile("./$path"); + break; case "/css/archnavbar/archlogo.gif": case "/images/new.png": header("Content-Type: image/png"); diff --git a/web/template/pkg_comments.php b/web/template/pkg_comments.php index 26fddfd..eac0da1 100644 --- a/web/template/pkg_comments.php +++ b/web/template/pkg_comments.php @@ -72,3 +72,33 @@ $count = pkgbase_comments_count($base_id, $include_deleted); </h3> </div> <?php endif; ?> +<script> +$(document).ready(function() { + $(".edit-comment").click(function () { + var parent_element = this.parentElement, + edit_form = $(parent_element).next(), + comment_id = parent_element.id.substr(8), + _this = $(this); + add_busy_indicator(_this); + $.getJSON('<?= get_uri('/rpc') ?>', + {type: 'load-comment', + arg: comment_id, + base_id: <?= htmlspecialchars($base_id) ?>, + pkgbase_name: '<?= htmlspecialchars($pkgbase_name) ?>'}, + function (data) { + remove_busy_indicator(_this); + edit_form.html(data.form); + edit_form.find('textarea').focus(); + }); + return false; + }); + + function add_busy_indicator(sibling) { + sibling.after('<img src="/images/ajax-loader.gif" class="ajax-loader" width="16" height="11" alt="Busy…">'); + } + + function remove_busy_indicator(sibling) { + sibling.next().remove(); + } +}); +</script> -- 2.4.6
On Tue, 21 Jul 2015 at 17:59:32, Marcel Korpel wrote:
For use in the new RPC interface to edit comments, the form shouldn't always print a header. Make this conditional.
Signed-off-by: Marcel Korpel <marcel.korpel@gmail.com> --- Change since v1: * Include the <form> element in the markup when using the RPC interface. This is necessary to use normal form submission to save an edited comment.
web/template/pkg_comment_form.php | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/web/template/pkg_comment_form.php b/web/template/pkg_comment_form.php index 16a92b1..24eb908 100644 --- a/web/template/pkg_comment_form.php +++ b/web/template/pkg_comment_form.php @@ -1,5 +1,8 @@ +<?php /* $no_header will be set when called from aurjson.class.php */ +if (!isset($no_header)): ?> [...]
$no_header is a bad name for a global variable. Another suggestion: Remove the <div></div> container and the heading. Add another template pkg_comment_box.php that looks like this: <div id="generic-form" class="box"> <h2><?= (isset($comment_id)) ? __('Edit comment for: %s', htmlspecialchars($pkgbase_name)) : __("Add Comment"); ?></h2> <?php include 'pkg_comment_form.php' ?> </div> Use that template instead of pkg_comment_form.php everywhere.
On Tue, 21 Jul 2015 at 17:59:33, Marcel Korpel wrote:
Signed-off-by: Marcel Korpel <marcel.korpel@gmail.com> --- Changes since v1: * Drop any POST method. * Drop save_comment() method and use the normal web form for submission.
web/lib/aurjson.class.php | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-)
diff --git a/web/lib/aurjson.class.php b/web/lib/aurjson.class.php index a272741..a4f78d4 100644 --- a/web/lib/aurjson.class.php +++ b/web/lib/aurjson.class.php @@ -14,7 +14,7 @@ class AurJSON { private $version = 1; private static $exposed_methods = array( 'search', 'info', 'multiinfo', 'msearch', 'suggest', - 'suggest-pkgbase' + 'suggest-pkgbase', 'load-comment'
I'd prefer get-comment (or fetch-comment) instead of load-comment.
); private static $exposed_fields = array( 'name', 'name-desc' @@ -477,5 +477,29 @@ class AurJSON {
return json_encode($result_array); } + + private function load_comment($http_data) { + $comment_id = $http_data['arg'];
Please cast this into an integer value using intval(), even if it is quoted or converted inside some helper function later.
+ if (isset($_GET['base_id']) && isset($_GET['pkgbase_name'])) {
Reduce nesting depth: if (!isset($_GET['base_id']) || !isset($_GET['pkgbase_name'])) { return json_encode(false); }
+ $base_id = $_GET['base_id'];
Cast this into an integer, see above.
+ $pkgbase_name = $_GET['pkgbase_name']; + + list($user_id, $comment) = comment_by_id($comment_id); + + if (!has_credential(CRED_COMMENT_EDIT, array($user_id)) || is_null($comment)) { + $output = false;
Reduce nesting depth. Early return here, no need for an else-block below.
+ } else { + $no_header = true;
This won't be needed when implementing the idea I mentioned in my reply to patch 1/3.
+ ob_start(); + include('pkg_comment_form.php'); + $html = ob_get_clean(); + $output = array('form' => $html);
We can directly return here when adding early returns for the error cases. I wonder whether it is a good idea to provide different data types, depending on whether an error occurred or not. Would it make sense to use json_results() and json_error() here?
+ } + } else { + $output = false; + } + + return json_encode($output); + } }
-- 2.4.6
On Tue, 21 Jul 2015 at 17:59:34, Marcel Korpel wrote:
Signed-off-by: Marcel Korpel <marcel.korpel@gmail.com> --- Changes since v1: * Drop the AJAX form submission, instead use a normal form. * Pass variables $base_id and $pkgbase_name that are now needed when rendering the form. * Add loading indicator.
web/html/css/aurweb.css | 6 ++++++ web/html/images/ajax-loader.gif | Bin 0 -> 723 bytes web/html/index.php | 4 ++++ web/template/pkg_comments.php | 30 ++++++++++++++++++++++++++++++ 4 files changed, 40 insertions(+) create mode 100644 web/html/images/ajax-loader.gif [...] diff --git a/web/template/pkg_comments.php b/web/template/pkg_comments.php index 26fddfd..eac0da1 100644 --- a/web/template/pkg_comments.php +++ b/web/template/pkg_comments.php @@ -72,3 +72,33 @@ $count = pkgbase_comments_count($base_id, $include_deleted); </h3> </div> <?php endif; ?> +<script> +$(document).ready(function() { + $(".edit-comment").click(function () { + var parent_element = this.parentElement, + edit_form = $(parent_element).next(), + comment_id = parent_element.id.substr(8),
Hardcoding the prefix length here seems error prone. How about splitting after the first hyphen instead?
+ _this = $(this);
Do we need an extra variable here?
+ add_busy_indicator(_this); + $.getJSON('<?= get_uri('/rpc') ?>', + {type: 'load-comment', + arg: comment_id, + base_id: <?= htmlspecialchars($base_id) ?>,
We should use intval() instead of htmlspecialchars() here.
+ pkgbase_name: '<?= htmlspecialchars($pkgbase_name) ?>'},
Can we use htmlspecialchars() here? If it is the right function to use, we need to add ENT_QUOTES at least...
+ function (data) {
Broken indentation?
+ remove_busy_indicator(_this); + edit_form.html(data.form); + edit_form.find('textarea').focus(); + });
Broken indentation (closing curly brace not aligned with the function keyword)?
+ return false; + }); + + function add_busy_indicator(sibling) { + sibling.after('<img src="/images/ajax-loader.gif" class="ajax-loader" width="16" height="11" alt="Busy…">');
Do we need to close that tag? Like "<img [...] />"? Thanks!
+ } + + function remove_busy_indicator(sibling) { + sibling.next().remove(); + } +}); +</script> -- 2.4.6
* Lukas Fleischer <lfleischer@archlinux.org> (Tue, 21 Jul 2015 18:34:27 +0200):
+ _this = $(this);
Do we need an extra variable here?
Yes, we need to copy 'this' here to be able to use it within the callback function, where 'this' has a different meaning.
+ pkgbase_name: '<?= htmlspecialchars($pkgbase_name) ?>'},
Can we use htmlspecialchars() here? If it is the right function to use, we need to add ENT_QUOTES at least...
Nope, we need json_encode() here.
+ function (data) {
Broken indentation?
Why? I indented all parameters with one tab, putting them on one line creates a very long line. You're correct about all the other things you mentioned.
On Tue, 21 Jul 2015 at 18:49:01, Marcel Korpel wrote:
* Lukas Fleischer <lfleischer@archlinux.org> (Tue, 21 Jul 2015 18:34:27 +0200):
+ _this = $(this);
Do we need an extra variable here?
Yes, we need to copy 'this' here to be able to use it within the callback function, where 'this' has a different meaning.
Sounds sensible. Thanks for clarifying!
+ pkgbase_name: '<?= htmlspecialchars($pkgbase_name) ?>'},
Can we use htmlspecialchars() here? If it is the right function to use, we need to add ENT_QUOTES at least...
Nope, we need json_encode() here.
Okay!
+ function (data) {
Broken indentation?
Why? I indented all parameters with one tab, putting them on one line creates a very long line.
Not sure. Looking at the existing JavaScript code in our code base, it seems like we prefer the following indentation style: $.getJSON([...], { [...] /* parameters here */ }, function(data) { [...] /* function body here */ });
You're correct about all the other things you mentioned.
* Lukas Fleischer <lfleischer@archlinux.org> (Tue, 21 Jul 2015 18:25:18 +0200):
We can directly return here when adding early returns for the error cases. I wonder whether it is a good idea to provide different data types, depending on whether an error occurred or not.
I don't think so, but at least this isn't a public API.
Would it make sense to use json_results() and json_error() here?
In all cases, you mean, error or not? Could be, but suggest() and suggest_pkgbase() also don't make use of them and they aren't public APIs either, but directly used by AJAX requests on the site.
On Tue, 21 Jul 2015 at 20:57:33, Marcel Korpel wrote:
* Lukas Fleischer <lfleischer@archlinux.org> (Tue, 21 Jul 2015 18:25:18 +0200):
We can directly return here when adding early returns for the error cases. I wonder whether it is a good idea to provide different data types, depending on whether an error occurred or not.
I don't think so, but at least this isn't a public API.
Would it make sense to use json_results() and json_error() here?
In all cases, you mean, error or not?
Yeah, in all cases.
Could be, but suggest() and suggest_pkgbase() also don't make use of them and they aren't public APIs either, but directly used by AJAX requests on the site.
Agreed. I had the idea of potentially making it part of the public API but it probably isn't too useful -- especially since it provides a whole comment edit form instead of the plain comment only.
participants (2)
-
Lukas Fleischer
-
Marcel Korpel