launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/lib/lp/answers/templates/questionmessage-display.pt b/lib/lp/answers/templates/questionmessage-display.pt
> index b69b4b6..e41a0a7 100644
> --- a/lib/lp/answers/templates/questionmessage-display.pt
> +++ b/lib/lp/answers/templates/questionmessage-display.pt
> @@ -5,9 +5,10 @@
>  <div
>    itemscope=""
>    itemtype="http://schema.org/UserComments";
> -  tal:define="css_classes view/getBoardCommentCSSClass"
> +  tal:define="css_classes python: view.getBoardCommentCSSClass()"

Seems unnecessary?

>    tal:attributes="class string:${css_classes};
> -                  id string:comment-${context/index}">
> +                  id string:comment-${context/index};
> +                  data-baseurl context/fmt:url">
>    <div class="boardCommentDetails">
>      <table>
>        <tbody>
> @@ -25,8 +26,20 @@
>        itemprop="commentTime"
>        tal:attributes="title context/datecreated/fmt:datetime;
>          datetime context/datecreated/fmt:isodate"
> -      tal:content="context/datecreated/fmt:displaydate">Thursday
> -    13:21</time>:
> +      tal:content="context/datecreated/fmt:displaydate">Thursday 13:21
> +    </time><span class="editable-message-last-edit-date"><span tal:condition="not:context/date_last_edited">:</span>

Is there any way to rearrange this to avoid having to render the colon in two places with relatively large amounts of tag noise around them?  Not the end of the world, but it's a bit unfortunate.  I realize you have to be careful about whitespace.

> +        <tal:last-edit condition="context/date_last_edited">
> +          (last edit <time
> +              itemprop="editTime"
> +              tal:attributes="title context/date_last_edited/fmt:datetime;
> +                datetime context/date_last_edited/fmt:isodate"
> +              tal:content="context/date_last_edited/fmt:displaydate" />):
> +        </tal:last-edit>
> +      </span>
> +    </td>
> +    <td>
> +      <img class="sprite edit action-icon editable-message-edit-btn"
> +           tal:condition="view/can_edit"/>
>      </td>
>      <td class="bug-comment-index">
>        <a
> diff --git a/lib/lp/services/messages/javascript/messages.edit.js b/lib/lp/services/messages/javascript/messages.edit.js
> new file mode 100644
> index 0000000..5cb6156
> --- /dev/null
> +++ b/lib/lp/services/messages/javascript/messages.edit.js
> @@ -0,0 +1,208 @@
> +/* Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
> + * GNU Affero General Public License version 3 (see the file LICENSE).
> + *
> + * This modules controls HTML comments in order to make them editable. To do
> + * so, it requires:
> + *  - A div container with the class .editable-message containing everything
> + *      else related to the message
> + *  - A data-baseurl="/path/to/msg" on the .editable-message container
> + *  - A .editable-message-body container with the original msg content
> + *  - A .editable-message-edit-btn element inside the main container, that will
> + *      switch the view to edit form when clicked.
> + *  - A .editable-message-form, with a textarea and 2 buttons:
> + *      .editable-message-update-btn and  .editable-message-cancel-btn.
> + *  - A .editable-message-last-edit-date span, where we update the date of the
> + *       last message editing.
> + *
> + * Once those HTML elements are available in the page, this module should be
> + * initialized with `lp.services.messages.edit.setup()`.
> + *
> + * @module Y.lp.services.messages.edit
> + * @requires node, DOM, lp.client
> + */
> +YUI.add('lp.services.messages.edit', function(Y) {
> +    var module = Y.namespace('lp.services.messages.edit');
> +
> +    module.msg_edit_success_notification = (
> +        "Message edited, but the original content may still be publicly " +
> +        "visible using the API.<br />Please, " +

No comma after "Please".

> +        "<a href='https://launchpad.net/+apidoc/devel.html#message'>" +
> +        "check the API documentation</a> in case " +

Missing "you" after "in case".

Also, I assume we don't want to be directing people to the API permanently.  Could we have an XXX comment around here explaining what needs to be done before we can drop this message?

> +        "need to remove old message revisions."
> +    );
> +    module.msg_edit_error_notification = (
> +        "There was an error updating the comment. " +
> +        "Please, try again in some minutes."

No comma after "Please", and perhaps "a few minutes" rather than "some minutes".

> +    );
> +
> +    module.htmlify_msg = function(text) {

This needs `text = text.replace(/&/g, "&amp;");` before doing anything else.

> +        text = text.replace(/</g, "&lt;");
> +        text = text.replace(/>/g, "&gt;");
> +        text = text.replace(/\n/g, "<br/>");
> +        return "<p>" + text    + "</p>";
> +    };
> +
> +    module.showEditMessageField = function(msg_body, msg_form) {
> +        msg_body.setStyle('display', 'none');
> +        msg_form.setStyle('display', 'block');
> +    };
> +
> +    module.hideEditMessageField = function(msg_body, msg_form) {
> +        msg_body.setStyle('display', 'block');
> +        msg_form.setStyle('display', 'none');
> +    };
> +
> +    module.saveMessageContent = function(
> +            msg_path, new_content, on_success, on_failure) {
> +        var msg_url = "/api/devel" + msg_path;
> +        var config = {
> +            on: {
> +                 success: on_success,
> +                 failure: on_failure
> +             },
> +            parameters: {"new_content": new_content}
> +        };
> +        this.lp_client.named_post(msg_url, 'editContent', config);
> +    };
> +
> +    module.showNotification = function(container, msg, can_dismiss) {
> +        can_dismiss = can_dismiss || false;
> +        // Clean up previous notification.
> +        module.hideNotification(container);
> +        container.setStyle('position', 'relative');
> +        var node = Y.Node.create(
> +            "<div class='editable-message-notification'>" +
> +            "  <p class='block-sprite large-warning'>" +
> +            msg +
> +            "  </p>" +
> +            "</div>");
> +         container.append(node);
> +         if (can_dismiss) {
> +             var dismiss = Y.Node.create(
> +                "<div class='editable-message-notification-dismiss'>" +
> +                "  <input type='button' value=' Ok ' />" +

Normally `value="OK"`.

> +                "</div>");
> +             dismiss.on('click', function() {
> +                module.hideNotification(container);
> +             });
> +             node.append(dismiss);
> +         }
> +    };
> +
> +    module.hideNotification = function(container) {
> +        var notification = container.one(".editable-message-notification");
> +        if(notification) {
> +            notification.remove();
> +        }
> +    };
> +
> +    module.showLoading = function(container) {
> +        module.showNotification(
> +            container,
> +            '<img class="spinner" src="/@@/spinner" alt="Loading..." />');
> +    };
> +
> +    module.hideLoading = function(container) {
> +        module.hideNotification(container);
> +    };
> +
> +    // What to do when a user clicks a message's "edit" button.
> +    module.onEditClick = function(elements) {
> +        // When clicking edit icon, show the edit form and focus on the
> +        // text area.
> +        module.showEditMessageField(elements.msg_body, elements.msg_form);
> +        elements.msg_form.one('textarea').getDOMNode().focus();
> +    }
> +
> +    // What to do when a user clicks "cancel edit" button.
> +    module.onEditCancelClick = function(elements) {
> +        module.hideEditMessageField(elements.msg_body, elements.msg_form);
> +    };
> +
> +    // What to do when a user clicks the update button after editing a msg.
> +    module.onUpdateClick = function(elements, baseurl) {
> +        // When clicking on "update" button, disable UI elements and send a
> +        // request to update the message at the backend.
> +        module.showLoading(elements.container);
> +        var textarea = elements.textarea.getDOMNode();
> +        var new_content = textarea.value;
> +        textarea.disabled = true;
> +        elements.update_btn.getDOMNode().disabled = true;
> +
> +        module.saveMessageContent(
> +            baseurl, new_content,
> +            function(err) { module.onMessageSaved(elements, new_content); },
> +            function(err) { module.onMessageSaveError(elements, err); }
> +        );
> +    };
> +
> +    // What to do when a message is saved in the backend.
> +    module.onMessageSaved = function(elements, new_content) {
> +        // When finished updating at the backend, re-enable UI
> +        // elements and display the new message.
> +        var html_msg = module.htmlify_msg(new_content);
> +        elements.msg_body_text.getDOMNode().innerHTML = html_msg;
> +        module.hideEditMessageField(
> +            elements.msg_body, elements.msg_form);
> +        elements.textarea.getDOMNode().disabled = false;
> +        elements.update_btn.getDOMNode().disabled = false;
> +        module.hideLoading(elements.container);
> +        module.showNotification(
> +            elements.container,
> +            module.msg_edit_success_notification, true);
> +        elements.last_edit.getDOMNode().innerHTML = (
> +            ' (last edit a moment ago): ');
> +    };
> +
> +    // What to do when a message fails to update on the backend.
> +    module.onMessageSaveError = function(elements, err) {
> +        // When something goes wrong at the backend, re-enable
> +        // UI elements and display an error.
> +        module.showNotification(
> +            elements.container,
> +            module.msg_edit_error_notification,  true);
> +        elements.textarea.getDOMNode().disabled = false;
> +        elements.update_btn.getDOMNode().disabled = false;
> +    };
> +
> +    module.wireEventHandlers = function(container) {
> +        var node = container.getDOMNode();
> +        var baseurl = node.dataset.baseurl;
> +        var elements = {
> +            "container": container,
> +            "msg_body": container.one('.editable-message-body'),
> +            "msg_body_text": container.one('.editable-message-text'),
> +            "msg_form": container.one('.editable-message-form'),
> +            "edit_btn": container.one('.editable-message-edit-btn'),
> +            "update_btn": container.one('.editable-message-update-btn'),
> +            "cancel_btn": container.one('.editable-message-cancel-btn'),
> +            "last_edit": container.one('.editable-message-last-edit-date')
> +        };
> +        elements.textarea = elements.msg_form.one('textarea');
> +
> +        module.hideEditMessageField(elements.msg_body, elements.msg_form);
> +
> +        // If the edit button is not present, do not try to bind the
> +        // handlers.
> +        if (!elements.edit_btn || !baseurl) {
> +            return;
> +        }
> +
> +        elements.edit_btn.on('click', function(e) {
> +            module.onEditClick(elements);
> +        });
> +
> +        elements.update_btn.on('click', function(e) {
> +            module.onUpdateClick(elements, baseurl);
> +        });
> +
> +        elements.cancel_btn.on('click', function(e) {
> +            module.onEditCancelClick(elements);
> +        });
> +    };
> +
> +    module.setup = function() {
> +        this.lp_client = new Y.lp.client.Launchpad();
> +        Y.all('.editable-message').each(module.wireEventHandlers);
> +    };
> +}, '0.1', {'requires': ['lp.client', 'node', 'DOM']});


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/402522
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:comment-editing-revisions-api.