Added UI Mockup: <Comment interface>

The first patch

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

Give more control to modules

An example of forum callbacks.

a demo of using comments api in forum

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

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

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()

8/ CSRF when deleting comments in admin UI

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.

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?

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

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

> _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

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

the new patch validates context level before calling get_coursemodule_from_id

Added UI Mockup: <non-javascript comments UI>

completed non-js UI

some changes to support block

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

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

else

</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

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?

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)

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?

sure, thanks a lot

feel free to reopen if you find any bug

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

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)

Sure, I will update comment code once David implemented it

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.

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.

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

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

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

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

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

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.

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

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

Martin,

that's an error from format_text function, fixed now

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?

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

Hi Eloy

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

Thanks

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

Thanks your super SQL, Eloy.

I attached another patch here to use your SQL.

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