Details

    • Type: New Feature
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Comments
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE

      Gliffy Diagrams

      1. Comment interface.bmml
        3 kB
        Dongsheng Cai
      2. comments_api_v11.patch
        66 kB
        Dongsheng Cai
      3. forum_commenting.patch
        4 kB
        Dongsheng Cai
      4. migrating.diff
        2 kB
        Dongsheng Cai
      5. non-javascript comments UI.bmml
        4 kB
        Dongsheng Cai
      1. Comment interface.png
        57 kB
      2. non-javascript comments UI.png
        63 kB

        Issue Links

        1.
        A new comments block, so people can add commenting anywhere Sub-task Closed Dongsheng Cai
         
        2.
        commets api for glossary Sub-task Closed Dongsheng Cai
         
        3.
        comments api for database module Sub-task Closed Dongsheng Cai
         
        4.
        migrate old comment_exception coding style to new general AJAX_SCRIPT support Sub-task Closed Petr Skoda
         
        5.
        Add ajax confirmation for deleting of comments Sub-task Closed Dongsheng Cai
         
        6.
        do not use qualified_me() in comments api to find current page Sub-task Closed Dongsheng Cai
         
        7.
        Remove hardcoded CSS styles from comments output Sub-task Closed Dongsheng Cai
         
        8.
        Do not rely on ### target in JS code Sub-task Closed Dongsheng Cai
         
        9.
        do not pass by reference to plugin_callback Sub-task Closed Dongsheng Cai
         
        10. get_comments() should have $renderer parameter Sub-task Open Unassigned
         
        11.
        $CFG->commentsperpage is ignored Sub-task Closed Dongsheng Cai
         
        12. New capability delete own comments might be useful Sub-task Open Unassigned
         
        13.
        Consider creating of core render subtype for comments Sub-task Closed Unassigned
         
        14.
        use $PAGE->course instead of $COURSE Sub-task Closed Dongsheng Cai
         
        15.
        Improve comments performance Sub-task Closed Dongsheng Cai
         
        16.
        DB update operations do not return bools any more, remove the result handling from comments Sub-task Closed Dongsheng Cai
         
        17.
        do not use $CFG for temporary flags Sub-task Closed Dongsheng Cai
         
        18.
        use new theme pix output in comments Sub-task Closed Dongsheng Cai
         
        19.
        potential page parameter collision in nonjs comments paging code Sub-task Closed Dongsheng Cai
         
        20.
        merge admins.js and comments.js to one fine + finish migration to new YUI coding style Sub-task Closed Dongsheng Cai
         
        21.
        do not use redirect messages if page not already started Sub-task Closed Dongsheng Cai
         
        22.
        "_" prefix not used for private methods in 2.0 anymore Sub-task Closed Dongsheng Cai
         
        23.
        if you add new comment old comments can not be deleted any more Sub-task Closed Dongsheng Cai
         
        24.
        hide comment_select_all checkbox if JS not enabled and add label for accessibility Sub-task Closed Dongsheng Cai
         

          Activity

          Hide
          dongsheng Dongsheng Cai added a comment -

          Added UI Mockup: <Comment interface>

          Show
          dongsheng Dongsheng Cai added a comment - Added UI Mockup: <Comment interface>
          Hide
          dongsheng Dongsheng Cai added a comment -

          The first patch

          Show
          dongsheng Dongsheng Cai added a comment - The first patch
          Hide
          dongsheng Dongsheng Cai added a comment -

          Created callbacks see http://docs.moodle.org/en/Development:Comments_2.0#Moodle_modules_callback
          Will attach a patch tomorrow.

          Show
          dongsheng Dongsheng Cai added a comment - Created callbacks see http://docs.moodle.org/en/Development:Comments_2.0#Moodle_modules_callback Will attach a patch tomorrow.
          Hide
          dongsheng Dongsheng Cai added a comment -

          Give more control to modules

          Show
          dongsheng Dongsheng Cai added a comment - Give more control to modules
          Hide
          dongsheng Dongsheng Cai added a comment -

          An example of forum callbacks.

          Show
          dongsheng Dongsheng Cai added a comment - An example of forum callbacks.
          Hide
          dongsheng Dongsheng Cai added a comment -

          a demo of using comments api in forum

          Show
          dongsheng Dongsheng Cai added a comment - a demo of using comments api in forum
          Hide
          nicolasconnault Nicolas Connault added a comment -

          Dongsheng, here are a few issues I've identified so far in your patch:

          1. In lib/commentlib.php, the comment::delete() function returns false whatever the reason. Because of this, the AJAX interface cannot know why the deletion failed. It looks like there is a similar problem in comment::add()
          2. You do a check for the moodle/comment:delete capability when deleting the comment, but not when printing the "Delete" icon. This shows a delete icon for users who do not have the capability to delete
          3. The delete() method also returns false if the current user ($USER) is not the author of the comment being deleted. Once again, if this is a valid restriction, the delete icon should not have been printed in the first place. There should be no links in Moodle that lead to a "Insufficient capabilities" type of error.
          4. Related to 3., I think that Admins/teachers should be able to delete other users' comments, because these could be offensive. You may need two capabilities for this: moodle/comment:deleteown and moodle/comment:deleteany, which overrides the first one

          Show
          nicolasconnault Nicolas Connault added a comment - Dongsheng, here are a few issues I've identified so far in your patch: 1. In lib/commentlib.php, the comment::delete() function returns false whatever the reason. Because of this, the AJAX interface cannot know why the deletion failed. It looks like there is a similar problem in comment::add() 2. You do a check for the moodle/comment:delete capability when deleting the comment, but not when printing the "Delete" icon. This shows a delete icon for users who do not have the capability to delete 3. The delete() method also returns false if the current user ($USER) is not the author of the comment being deleted. Once again, if this is a valid restriction, the delete icon should not have been printed in the first place. There should be no links in Moodle that lead to a "Insufficient capabilities" type of error. 4. Related to 3., I think that Admins/teachers should be able to delete other users' comments, because these could be offensive. You may need two capabilities for this: moodle/comment:deleteown and moodle/comment:deleteany, which overrides the first one
          Hide
          skodak Petr Skoda added a comment -

          1/ the order of database fields seems pretty random, I would recommend:
          id, contextid, commentarea, itemid, content, userid, timecreated, timemodified

          2/ please do not allow guest user to comment anywhere - this is a big spam risk and would be a huge XSS risk too if html allowed

          3/ I must have missed the text format discussion again - at present there is no format field which implies plain text, problems are

          • glossary and other places already use HTML, how are we going to upgrade them - I guess keeping the old custom code is not an option
          • multilang is not compatible with plain text format, are we going to create some new exception here?
          • this looks similar to feedback field in gradebook which may require similar solution.

          4/ please do not use the obsoleted dml structures like:
          if ($DB->delete_records('comments', array('id'=>$commentid))) {
          return true;
          }

          this also includes returning "false x array()" such as in get_comments() method

          5/ the commentlib is mixing everything in one class (UI, accessc ontrol, communication) - it will not be possible to use this code outside of AJAX - we do need something which will work in Web Services and old style html too I am afraid

          Show
          skodak Petr Skoda added a comment - 1/ the order of database fields seems pretty random, I would recommend: id, contextid, commentarea, itemid, content, userid, timecreated, timemodified 2/ please do not allow guest user to comment anywhere - this is a big spam risk and would be a huge XSS risk too if html allowed 3/ I must have missed the text format discussion again - at present there is no format field which implies plain text, problems are glossary and other places already use HTML, how are we going to upgrade them - I guess keeping the old custom code is not an option multilang is not compatible with plain text format, are we going to create some new exception here? this looks similar to feedback field in gradebook which may require similar solution. 4/ please do not use the obsoleted dml structures like: if ($DB->delete_records('comments', array('id'=>$commentid))) { return true; } this also includes returning "false x array()" such as in get_comments() method 5/ the commentlib is mixing everything in one class (UI, accessc ontrol, communication) - it will not be possible to use this code outside of AJAX - we do need something which will work in Web Services and old style html too I am afraid
          Hide
          skodak Petr Skoda added a comment -

          6/ require_login($course, false, $cm) is mandatory for anything activity related because it contains a lot of access control logic, doing just require_login() is not enough

          7/ _setup_module() contains some strange code for course module fetching, we need to look at context level first, we can not use random instanceid in get_coursemodule_from_id()

          Show
          skodak Petr Skoda added a comment - 6/ require_login($course, false, $cm) is mandatory for anything activity related because it contains a lot of access control logic, doing just require_login() is not enough 7/ _setup_module() contains some strange code for course module fetching, we need to look at context level first, we can not use random instanceid in get_coursemodule_from_id()
          Hide
          skodak Petr Skoda added a comment -

          8/ CSRF when deleting comments in admin UI

          Show
          skodak Petr Skoda added a comment - 8/ CSRF when deleting comments in admin UI
          Hide
          skodak Petr Skoda added a comment -

          9/ I do not like the all-mighty comment capabilities - if you allow somebody to comment at system level (such as blog) they will have that right everywhere. Solution might be to not define these capabilities and let each component handle that separately in callback.

          Show
          skodak Petr Skoda added a comment - 9/ I do not like the all-mighty comment capabilities - if you allow somebody to comment at system level (such as blog) they will have that right everywhere. Solution might be to not define these capabilities and let each component handle that separately in callback.
          Hide
          dongsheng Dongsheng Cai added a comment - - edited

          Thanks for reviewing my patches.

          1. fixed
          2. I did check posting permission before inserting into database, am I miss something?
          3. I will talk to Martin
          4. What is the correct structures? Is it about the return value? Should I return the value from delete_records?
          5. I have a plan to support non-js comments, I will make some changes for it
          6. fixed.
          7. Right, working on a fix
          8. will use moodle form to replace it
          9. does that mean we need to remove capabilities and use callbacks only?

          Show
          dongsheng Dongsheng Cai added a comment - - edited Thanks for reviewing my patches. 1. fixed 2. I did check posting permission before inserting into database, am I miss something? 3. I will talk to Martin 4. What is the correct structures? Is it about the return value? Should I return the value from delete_records? 5. I have a plan to support non-js comments, I will make some changes for it 6. fixed. 7. Right, working on a fix 8. will use moodle form to replace it 9. does that mean we need to remove capabilities and use callbacks only?
          Hide
          skodak Petr Skoda added a comment -

          2. user account with username "guest" is an exception - we do not allow it to post any content - see forum, blog, etc.
          4. delete_records() is returning true for BC reasons only, it will never return false now, get_records() and friends now return empty arrays instead of false when no items exist
          9. my +1 to use callbacks only for everything

          Show
          skodak Petr Skoda added a comment - 2. user account with username "guest" is an exception - we do not allow it to post any content - see forum, blog, etc. 4. delete_records() is returning true for BC reasons only, it will never return false now, get_records() and friends now return empty arrays instead of false when no items exist 9. my +1 to use callbacks only for everything
          Hide
          dongsheng Dongsheng Cai added a comment -

          Thanks Petr, I will upload another patches to fix these problems tomorrow.

          Show
          dongsheng Dongsheng Cai added a comment - Thanks Petr, I will upload another patches to fix these problems tomorrow.
          Hide
          dongsheng Dongsheng Cai added a comment -

          > _setup_module() contains some strange code for course module fetching, we need to look at context level first, we can not use random instanceid in get_coursemodule_from_id()

          The instanceid is a passed in parameter ($context->instanceid), the developer who using comments api should know what is the correct context, did I misunderstand?

          I introduced a new function _setup_course, because require_login requires courseid.

          Need to discuss in a meeting:
          1. text format
          2. use callbacks only for everything, remove capabilities

          My Todos:
          1. complete admin UI, use moodle form
          2. non-js support

          Show
          dongsheng Dongsheng Cai added a comment - > _setup_module() contains some strange code for course module fetching, we need to look at context level first, we can not use random instanceid in get_coursemodule_from_id() The instanceid is a passed in parameter ($context->instanceid), the developer who using comments api should know what is the correct context, did I misunderstand? I introduced a new function _setup_course, because require_login requires courseid. Need to discuss in a meeting: 1. text format 2. use callbacks only for everything, remove capabilities My Todos: 1. complete admin UI, use moodle form 2. non-js support
          Hide
          skodak Petr Skoda added a comment -

          whenever you want to use instanceid from context you must validate context level, because it is the cm->id if and only if the context level is equal to CONTEXT_MODULE

          I guess we could meet this week with MD and the rest of interested devs in jabber chat

          Show
          skodak Petr Skoda added a comment - whenever you want to use instanceid from context you must validate context level, because it is the cm->id if and only if the context level is equal to CONTEXT_MODULE I guess we could meet this week with MD and the rest of interested devs in jabber chat
          Hide
          dongsheng Dongsheng Cai added a comment -

          the new patch validates context level before calling get_coursemodule_from_id

          Show
          dongsheng Dongsheng Cai added a comment - the new patch validates context level before calling get_coursemodule_from_id
          Hide
          dongsheng Dongsheng Cai added a comment -

          Added UI Mockup: <non-javascript comments UI>

          Show
          dongsheng Dongsheng Cai added a comment - Added UI Mockup: <non-javascript comments UI>
          Hide
          dongsheng Dongsheng Cai added a comment -

          completed non-js UI

          Show
          dongsheng Dongsheng Cai added a comment - completed non-js UI
          Hide
          dongsheng Dongsheng Cai added a comment -

          some changes to support block

          Show
          dongsheng Dongsheng Cai added a comment - some changes to support block
          Hide
          dongsheng Dongsheng Cai added a comment -

          1. improve block UI
          2. use OOP approach in blocks

          Show
          dongsheng Dongsheng Cai added a comment - 1. improve block UI 2. use OOP approach in blocks
          Hide
          skodak Petr Skoda added a comment -

          1/ this does not make any sense at all:
          <code>
          $cmt_id = $DB->insert_record('comments', $newcmt);
          if (!empty($cmt_id))

          { $newcmt->id = $cmt_id; $newcmt->time = userdate($now); $newcmt->username = fullname($USER); $newcmt->content = format_text($newcmt->content); $newcmt->avatar = print_user_picture($USER, $this->course->id, NULL, 16, true); return $newcmt; }

          else

          { return COMMENT_ERROR_DB; }

          </code>

          because use exceptions now.

          I personally do not like error constants COMMENT_ERROR_MODULE_REJECT and COMMENT_ERROR_INSUFFICIENT_CAPS either, this is exactly the place where exceptions make sense - in 99% of cases it succeeds because the access control and other logic is printed correctly in HTML before submission, only if something changes concurrently or somebody tries to hack moodle exception occurs.

          2/ pretty please stop including formslib from core libraries! instead create a separate file with your form definition and include it only when needed

          3/ class comments have some weird spacing in commentlib.php - it breaks phpdocs rules pretty badly I think

          4/ using optional_param() deen in library functions is not recommended - is that really necessary?

          5/ magic methods like setup_course() which try to guess or not good coding style IMO

          6/ moodle/comment:post and moodle/comment:view or not assignable or overridable in a standard way - this will cause major usability problem and a lot of confusion, why is it still there?

          7/ $cm = get_coursemodule_from_id('', $context->instanceid); - is a total nonsense, we MUST verify context level first to make sure it really is CONTEXT_MODULE!

          8/ $courseid = optional_param('courseid', SITEID, PARAM_INT); $contextid = optional_param('contextid', SYSCONTEXTID, PARAM_INT);
          those constants should be used in core only and defaults in ajax code are not used anyway

          9/ license headers are missing or incomplete

          Show
          skodak Petr Skoda added a comment - 1/ this does not make any sense at all: <code> $cmt_id = $DB->insert_record('comments', $newcmt); if (!empty($cmt_id)) { $newcmt->id = $cmt_id; $newcmt->time = userdate($now); $newcmt->username = fullname($USER); $newcmt->content = format_text($newcmt->content); $newcmt->avatar = print_user_picture($USER, $this->course->id, NULL, 16, true); return $newcmt; } else { return COMMENT_ERROR_DB; } </code> because use exceptions now. I personally do not like error constants COMMENT_ERROR_MODULE_REJECT and COMMENT_ERROR_INSUFFICIENT_CAPS either, this is exactly the place where exceptions make sense - in 99% of cases it succeeds because the access control and other logic is printed correctly in HTML before submission, only if something changes concurrently or somebody tries to hack moodle exception occurs. 2/ pretty please stop including formslib from core libraries! instead create a separate file with your form definition and include it only when needed 3/ class comments have some weird spacing in commentlib.php - it breaks phpdocs rules pretty badly I think 4/ using optional_param() deen in library functions is not recommended - is that really necessary? 5/ magic methods like setup_course() which try to guess or not good coding style IMO 6/ moodle/comment:post and moodle/comment:view or not assignable or overridable in a standard way - this will cause major usability problem and a lot of confusion, why is it still there? 7/ $cm = get_coursemodule_from_id('', $context->instanceid); - is a total nonsense, we MUST verify context level first to make sure it really is CONTEXT_MODULE! 8/ $courseid = optional_param('courseid', SITEID, PARAM_INT); $contextid = optional_param('contextid', SYSCONTEXTID, PARAM_INT); those constants should be used in core only and defaults in ajax code are not used anyway 9/ license headers are missing or incomplete
          Hide
          dongsheng Dongsheng Cai added a comment -

          1. The reason I don't use exception is that moodle_exception doesn't work in ajax, I need to return json string here.
          4. what is the cons of using optional_param? I did that to make comments api easier to use, otherwise the other developers need to use optional_param to get parameters outside and passed in, that's way too complicated
          6. We talked about this in the meeting, and we agreed to have both capabilities and plugin callbacks, and I confirmed this with Martin yesterday, do you mean we should keep moodle/comment:delete?
          8. Can you explain further? If there is no defaults there, should we drop the request directly?

          Show
          dongsheng Dongsheng Cai added a comment - 1. The reason I don't use exception is that moodle_exception doesn't work in ajax, I need to return json string here. 4. what is the cons of using optional_param? I did that to make comments api easier to use, otherwise the other developers need to use optional_param to get parameters outside and passed in, that's way too complicated 6. We talked about this in the meeting, and we agreed to have both capabilities and plugin callbacks, and I confirmed this with Martin yesterday, do you mean we should keep moodle/comment:delete? 8. Can you explain further? If there is no defaults there, should we drop the request directly?
          Hide
          skodak Petr Skoda added a comment -

          1. it does work in ajax - you just need to enclose everything in try {} catch and jsencode the exception details in fact this is the main reason why print_error() should throw exceptions instead of just printing stuff to output and exiting
          2. we have to use function parameters, sorry optional_param() makes the code useless for commandline, web services, etc. - so the rule is no optional_param in core functions
          6. delete seems a special case for admin only - may be ok; I thought we agreed that callbacks are the right way, because as I said above general capability used in system and course context at the same time is a huge problem
          8. there are some new coding style rules for license headers and phpdocs - it looks like those new rules are not obeyed in the comments code (minor issue)

          Show
          skodak Petr Skoda added a comment - 1. it does work in ajax - you just need to enclose everything in try {} catch and jsencode the exception details in fact this is the main reason why print_error() should throw exceptions instead of just printing stuff to output and exiting 2. we have to use function parameters, sorry optional_param() makes the code useless for commandline, web services, etc. - so the rule is no optional_param in core functions 6. delete seems a special case for admin only - may be ok; I thought we agreed that callbacks are the right way, because as I said above general capability used in system and course context at the same time is a huge problem 8. there are some new coding style rules for license headers and phpdocs - it looks like those new rules are not obeyed in the comments code (minor issue)
          Hide
          dongsheng Dongsheng Cai added a comment -

          Thanks for explanation, Petr, about 8, you explained 9 for me

          I will fixed up those issues as soon as possible, let us leave 6 for Monday meeting? OK?

          Show
          dongsheng Dongsheng Cai added a comment - Thanks for explanation, Petr, about 8, you explained 9 for me I will fixed up those issues as soon as possible, let us leave 6 for Monday meeting? OK?
          Hide
          skodak Petr Skoda added a comment -

          sure, thanks a lot

          Show
          skodak Petr Skoda added a comment - sure, thanks a lot
          Hide
          dongsheng Dongsheng Cai added a comment -

          feel free to reopen if you find any bug

          Show
          dongsheng Dongsheng Cai added a comment - feel free to reopen if you find any bug
          Hide
          skodak Petr Skoda added a comment -

          reopening:

          $context   = get_context_instance_by_id($contextid);
          if ($context->contextlevel == CONTEXT_MODULE) {
              $cm = get_coursemodule_from_id('', $context->instanceid);
          } else {
              $cm = null;
          }

          is still in comment_post.php

          Show
          skodak Petr Skoda added a comment - reopening: $context = get_context_instance_by_id($contextid); if ($context->contextlevel == CONTEXT_MODULE) { $cm = get_coursemodule_from_id('', $context->instanceid); } else { $cm = null; } is still in comment_post.php
          Hide
          skodak Petr Skoda added a comment -

          looking at the exception handling code in comments a bit more, I think we should implement a general support for this - I think the easiest solution is to override the default exception handler in all ajax scripts, David is going to need it too, so I hope he will implement it first for amos and then you could update comments (it can wait after the beta, I discussed this with him already today)

          Show
          skodak Petr Skoda added a comment - looking at the exception handling code in comments a bit more, I think we should implement a general support for this - I think the easiest solution is to override the default exception handler in all ajax scripts, David is going to need it too, so I hope he will implement it first for amos and then you could update comments (it can wait after the beta, I discussed this with him already today)
          Hide
          dongsheng Dongsheng Cai added a comment -

          Sure, I will update comment code once David implemented it

          Show
          dongsheng Dongsheng Cai added a comment - Sure, I will update comment code once David implemented it
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited

          Just a quick comment. Last week I asked about comments belonging to block or to parent context, and it was generally agreed that belonging to block was better.

          Today I've seen this change that, simply, without further explanation, breaks that approach:

          http://cvs.moodle.org/moodle/blocks/comments/block_comments.php.diff?r1=1.7&r2=1.8

          and make comments to belong to "parent" (page = module/course) context.

          It seems that it was to allow sticky blocks to display different comments. Well, just wondering where all the last-week rationale went. No more problem than that and also IMO, losing some functionality as far as we aren't going to be able to have more than one group of comments in the same context, imagine one course where you want to have some feedback (comments) about future activities and about course organization. That will be simply impossible, once we attach comments to parent context.

          Not arguing too much, just commenting it's an important change / assumption. Perrhaps the best would be to be able to define, in the block, how we want it to behave (using own OR parent context). That way we would have both worlds as a possibility.

          Edited: Or, as Petr suggested some mins ago, have 2 (different) blocks, one for own comments (stored in block context) and other for course comments (able to be sticky along all the course).

          Ciao

          PS: From a backup/restore perspective it doesn't hurts at all, as far as we are generating one comments.xml file for each context, so really it doesn't matter where comments are associated.

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - - edited Just a quick comment. Last week I asked about comments belonging to block or to parent context, and it was generally agreed that belonging to block was better. Today I've seen this change that, simply, without further explanation, breaks that approach: http://cvs.moodle.org/moodle/blocks/comments/block_comments.php.diff?r1=1.7&r2=1.8 and make comments to belong to "parent" (page = module/course) context. It seems that it was to allow sticky blocks to display different comments. Well, just wondering where all the last-week rationale went. No more problem than that and also IMO, losing some functionality as far as we aren't going to be able to have more than one group of comments in the same context, imagine one course where you want to have some feedback (comments) about future activities and about course organization. That will be simply impossible, once we attach comments to parent context. Not arguing too much, just commenting it's an important change / assumption. Perrhaps the best would be to be able to define, in the block, how we want it to behave (using own OR parent context). That way we would have both worlds as a possibility. Edited: Or, as Petr suggested some mins ago, have 2 (different) blocks, one for own comments (stored in block context) and other for course comments (able to be sticky along all the course). Ciao PS: From a backup/restore perspective it doesn't hurts at all, as far as we are generating one comments.xml file for each context, so really it doesn't matter where comments are associated.
          Hide
          dougiamas Martin Dougiamas added a comment -

          Sorry, Eloy, not sure where that was "generally agreed".

          In my mind the main purpose for comments block is to provide feedback about the page it's on. That's what teachers are excited about when they see it (I've been demoing it that way for months). I think the strongest use case by far is for using the block to add comments to the page.

          Currently those blocks can't have introductions or custom titles so I don't think it's so useful to be able to have more than one of these blocks on a page.

          Comments in block context might be useful for some other scenarios so maybe that's another block, or a setting in the block, but for now it's most useful to do it the way it is now.

          Show
          dougiamas Martin Dougiamas added a comment - Sorry, Eloy, not sure where that was "generally agreed". In my mind the main purpose for comments block is to provide feedback about the page it's on. That's what teachers are excited about when they see it (I've been demoing it that way for months). I think the strongest use case by far is for using the block to add comments to the page. Currently those blocks can't have introductions or custom titles so I don't think it's so useful to be able to have more than one of these blocks on a page. Comments in block context might be useful for some other scenarios so maybe that's another block, or a setting in the block, but for now it's most useful to do it the way it is now.
          Hide
          dougiamas Martin Dougiamas added a comment -

          I think the main difference is that if you decide you want to have feedback over the whole course, and you add a comment block and make it sticky, then it's not so useful to have comments about a forum page showing up in a wiki page.

          Sorry about the misunderstanding in chat, I think I didn't understand the issue well.

          Ciao man

          Show
          dougiamas Martin Dougiamas added a comment - I think the main difference is that if you decide you want to have feedback over the whole course, and you add a comment block and make it sticky, then it's not so useful to have comments about a forum page showing up in a wiki page. Sorry about the misunderstanding in chat, I think I didn't understand the issue well. Ciao man
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Well,

          oki, I see the point about adding comments dynamically to different contexts, depending of where the block is showed, I'm not going to continue discussing about the new/old differences.

          In any case, if the new approach is the one to be... there are two little things that I don't like in the last commit (where the change happened): http://cvs.moodle.org/moodle/blocks/comments/block_comments.php.diff?r1=1.7&r2=1.8

          1) If the comments belong to the module/course... which is the point about to register them with area "block_comments". Should be "general comments" or whatever. No very important for now, but...
          2) The comment->itemid CANNOT be the block id anymore. Has to be null (or whatever is the default for that field), but never the block id.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Well, oki, I see the point about adding comments dynamically to different contexts, depending of where the block is showed, I'm not going to continue discussing about the new/old differences. In any case, if the new approach is the one to be... there are two little things that I don't like in the last commit (where the change happened): http://cvs.moodle.org/moodle/blocks/comments/block_comments.php.diff?r1=1.7&r2=1.8 1) If the comments belong to the module/course... which is the point about to register them with area "block_comments". Should be "general comments" or whatever. No very important for now, but... 2) The comment->itemid CANNOT be the block id anymore. Has to be null (or whatever is the default for that field), but never the block id. Ciao
          Hide
          dougiamas Martin Dougiamas added a comment -

          Wierd because the "new" way is in fact the "old old way". AFAIK.

          Anyway, yes those itemids seem wrong. Dongsheng can you review?

          Show
          dougiamas Martin Dougiamas added a comment - Wierd because the "new" way is in fact the "old old way". AFAIK. Anyway, yes those itemids seem wrong. Dongsheng can you review?
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Just hope the "old" one won't become the "new new" one in the future, or this is going to be a real ping-pong nightmare. :-P

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Just hope the "old" one won't become the "new new" one in the future, or this is going to be a real ping-pong nightmare. :-P
          Hide
          dongsheng Dongsheng Cai added a comment -

          Didn't think much on the itemid and area issues...

          > 1) If the comments belong to the module/course... which is the point about to register them with area "block_comments". Should be "general comments" or whatever. No very important for now

          Can we call it page_comments?

          > 2) The comment->itemid CANNOT be the block id anymore. Has to be null (or whatever is the default for that field), but never the block id.

          That's absolutely right, I will fix it by tomorrow.

          Thanks everyone.

          Show
          dongsheng Dongsheng Cai added a comment - Didn't think much on the itemid and area issues... > 1) If the comments belong to the module/course... which is the point about to register them with area "block_comments". Should be "general comments" or whatever. No very important for now Can we call it page_comments? > 2) The comment->itemid CANNOT be the block id anymore. Has to be null (or whatever is the default for that field), but never the block id. That's absolutely right, I will fix it by tomorrow. Thanks everyone.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          One more thing...

          should "old" comments (those belonging to block)... be moved to course/module contexts as part of the upgrade? Else... they will be "orphaned" in DB forever. Note we are in dev stages so I don't know it's critical but... sites already testing 2.0 will, suddenly, lost all their block comments.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - One more thing... should "old" comments (those belonging to block)... be moved to course/module contexts as part of the upgrade? Else... they will be "orphaned" in DB forever. Note we are in dev stages so I don't know it's critical but... sites already testing 2.0 will, suddenly, lost all their block comments. Ciao
          Hide
          dougiamas Martin Dougiamas added a comment -

          I'm also getting JS errors relating to $PAGE->context not being defined on site and my moodle page.

          Show
          dougiamas Martin Dougiamas added a comment - I'm also getting JS errors relating to $PAGE->context not being defined on site and my moodle page.
          Hide
          dongsheng Dongsheng Cai added a comment -

          Martin,

          that's an error from format_text function, fixed now

          Show
          dongsheng Dongsheng Cai added a comment - Martin, that's an error from format_text function, fixed now
          Hide
          dongsheng Dongsheng Cai added a comment -

          Hi, Everyone

          I am going to change comment area name to 'page_comments', and itemid = 0, agree?

          > should "old" comments (those belonging to block)... be moved to course/module contexts as part of the upgrade? Else... they will be "orphaned" in DB forever. Note we are in dev stages so I don't know it's critical but... sites already testing 2.0 will, suddenly, lost all their block comments.

          Hi, Eloy, because we couldn't know the new context id, so there is no way to migrate them to the new comments, we can easily find them all by comment area, but not sure if we should delete them silently, should we warn them during upgrading?

          Show
          dongsheng Dongsheng Cai added a comment - Hi, Everyone I am going to change comment area name to 'page_comments', and itemid = 0, agree? > should "old" comments (those belonging to block)... be moved to course/module contexts as part of the upgrade? Else... they will be "orphaned" in DB forever. Note we are in dev stages so I don't know it's critical but... sites already testing 2.0 will, suddenly, lost all their block comments. Hi, Eloy, because we couldn't know the new context id, so there is no way to migrate them to the new comments, we can easily find them all by comment area, but not sure if we should delete them silently, should we warn them during upgrading?
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Hi Dong,

          but we do know the "default" parent context, isn't it? I mean, if the block has been instanced at course level, it's the course context and if the block was instanced at activity level then it's the module context. In other words, I think it's a matter of iterate over all the existing comments belonging to blocks... and then update their context to be the block->parentcontextid (and the comments are to that "page_comments" name, of course).

          That way all the old comments will continue being available in the new (parent) context.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Hi Dong, but we do know the "default" parent context, isn't it? I mean, if the block has been instanced at course level, it's the course context and if the block was instanced at activity level then it's the module context. In other words, I think it's a matter of iterate over all the existing comments belonging to blocks... and then update their context to be the block->parentcontextid (and the comments are to that "page_comments" name, of course). That way all the old comments will continue being available in the new (parent) context. Ciao
          Hide
          dongsheng Dongsheng Cai added a comment -

          Hi Eloy

          I created a patch to migrate the old comments, is it ok to you?

          Thanks

          Show
          dongsheng Dongsheng Cai added a comment - Hi Eloy I created a patch to migrate the old comments, is it ok to you? Thanks
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Bloody hell, I lost my comment when it was near finished! Blame phpMyAdmin!

          Summary:

          1) Please, test your patches before proposing them, lol. I think the one proposed above is not working ok.
          2) You are using recordset there. Perfect. But you must close it always.
          3) There is one typo "conextid"

          And finally, although not critical for this upgrade, as far as nobody will have thousands of "block_comments" out there (only testers and in a few places), please always consider doing the job 100% in SQL if possible. Only it it isn't possible we should go to PHP iterations (way, way slower).

          I've tried these SQLs in my test environment:

          UPDATE mdl_comments SET 
              contextid = (SELECT parentcontextid
                             FROM mdl_block_instances
                            WHERE id = mdl_comments.itemid
                              AND blockname = 'comments'),
              commentarea = 'page_comments',
              itemid = 0
          WHERE commentarea = 'block_comments'
           AND itemid != 0
           AND EXISTS (SELECT 'x'
                         FROM mdl_block_instances
                        WHERE id = mdl_comments.itemid
                          AND blockname = 'comments');

          DELETE FROM mdl_comments
          WHERE commentarea = 'block_comments';

          The first looks for all the old "block_comments" comments and, if matching block instance is found, then assign them to the parentcontext of block while also adjusting commentarea and itemid as expected.

          The second deletes all the orphaned records which block doesn't ever exists.

          Feel free to continue with you approach (recordset) or use the queries above (as said, it's not critical in this exact upgrade step). Just a recommendation to, always, think if there is a 100% SQL way to do the job.

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Bloody hell, I lost my comment when it was near finished! Blame phpMyAdmin! Summary: 1) Please, test your patches before proposing them, lol. I think the one proposed above is not working ok. 2) You are using recordset there. Perfect. But you must close it always. 3) There is one typo "conextid" And finally, although not critical for this upgrade, as far as nobody will have thousands of "block_comments" out there (only testers and in a few places), please always consider doing the job 100% in SQL if possible. Only it it isn't possible we should go to PHP iterations (way, way slower). I've tried these SQLs in my test environment: UPDATE mdl_comments SET contextid = (SELECT parentcontextid FROM mdl_block_instances WHERE id = mdl_comments.itemid AND blockname = 'comments'), commentarea = 'page_comments', itemid = 0 WHERE commentarea = 'block_comments' AND itemid != 0 AND EXISTS (SELECT 'x' FROM mdl_block_instances WHERE id = mdl_comments.itemid AND blockname = 'comments'); DELETE FROM mdl_comments WHERE commentarea = 'block_comments'; The first looks for all the old "block_comments" comments and, if matching block instance is found, then assign them to the parentcontext of block while also adjusting commentarea and itemid as expected. The second deletes all the orphaned records which block doesn't ever exists. Feel free to continue with you approach (recordset) or use the queries above (as said, it's not critical in this exact upgrade step). Just a recommendation to, always, think if there is a 100% SQL way to do the job. Ciao
          Hide
          dongsheng Dongsheng Cai added a comment -

          Thanks your super SQL, Eloy.

          I attached another patch here to use your SQL.

          Show
          dongsheng Dongsheng Cai added a comment - Thanks your super SQL, Eloy. I attached another patch here to use your SQL.
          Hide
          dougiamas Martin Dougiamas added a comment -

          Resolving this for now, will let subtasks and other bugs sort themselves out.

          Show
          dougiamas Martin Dougiamas added a comment - Resolving this for now, will let subtasks and other bugs sort themselves out.

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                24/Nov/10