Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0
    • Component/s: Commenting
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      15674
    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

        Activity

        Hide
        Dongsheng Cai added a comment -

        Added UI Mockup: <Comment interface>

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

        The first patch

        Show
        Dongsheng Cai added a comment - The first patch
        Hide
        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 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 Cai added a comment -

        Give more control to modules

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

        An example of forum callbacks.

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

        a demo of using comments api in forum

        Show
        Dongsheng Cai added a comment - a demo of using comments api in forum
        Hide
        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
        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
        Petr Škoda 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
        Petr Škoda 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
        Petr Škoda 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
        Petr Škoda 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
        Petr Škoda added a comment -

        8/ CSRF when deleting comments in admin UI

        Show
        Petr Škoda added a comment - 8/ CSRF when deleting comments in admin UI
        Hide
        Petr Škoda 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
        Petr Škoda 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 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 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
        Petr Škoda 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
        Petr Škoda 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 Cai added a comment -

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

        Show
        Dongsheng Cai added a comment - Thanks Petr, I will upload another patches to fix these problems tomorrow.
        Hide
        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 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
        Petr Škoda 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
        Petr Škoda 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 Cai added a comment -

        the new patch validates context level before calling get_coursemodule_from_id

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

        Added UI Mockup: <non-javascript comments UI>

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

        completed non-js UI

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

        some changes to support block

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

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

        Show
        Dongsheng Cai added a comment - 1. improve block UI 2. use OOP approach in blocks
        Hide
        Petr Škoda 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
        Petr Škoda 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 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 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
        Petr Škoda 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
        Petr Škoda 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 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 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
        Petr Škoda added a comment -

        sure, thanks a lot

        Show
        Petr Škoda added a comment - sure, thanks a lot
        Hide
        Dongsheng Cai added a comment -

        feel free to reopen if you find any bug

        Show
        Dongsheng Cai added a comment - feel free to reopen if you find any bug
        Hide
        Petr Škoda 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
        Petr Škoda 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
        Petr Škoda 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
        Petr Škoda 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 Cai added a comment -

        Sure, I will update comment code once David implemented it

        Show
        Dongsheng Cai added a comment - Sure, I will update comment code once David implemented it
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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 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 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
        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
        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
        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
        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 Cai added a comment -

        Martin,

        that's an error from format_text function, fixed now

        Show
        Dongsheng Cai added a comment - Martin, that's an error from format_text function, fixed now
        Hide
        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 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
        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
        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 Cai added a comment -

        Hi Eloy

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

        Thanks

        Show
        Dongsheng Cai added a comment - Hi Eloy I created a patch to migrate the old comments, is it ok to you? Thanks
        Hide
        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
        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 Cai added a comment -

        Thanks your super SQL, Eloy.

        I attached another patch here to use your SQL.

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

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

        Show
        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: