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