[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
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
--- 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.
Signed-off-by: Marcel Korpel
On Tue, 21 Jul 2015 at 17:59:33, Marcel Korpel wrote:
Signed-off-by: Marcel Korpel
--- 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
* Lukas Fleischer
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
(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.
Signed-off-by: Marcel Korpel
On Tue, 21 Jul 2015 at 17:59:34, Marcel Korpel wrote:
Signed-off-by: Marcel Korpel
--- 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 ""? Thanks!
+ } + + function remove_busy_indicator(sibling) { + sibling.next().remove(); + } +}); +</script> -- 2.4.6
* Lukas Fleischer
+ _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
(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.
participants (2)
-
Lukas Fleischer
-
Marcel Korpel