Moodle

Link to user profile in comments block fails

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 2.0.3, 2.1.4, 2.2.1
  • Fix Version/s: 2.1.6, 2.2.3
  • Component/s: Commenting
  • Labels:
  • Testing Instructions:
    Hide
    1. Goto a course>turn editing> add a comment block
    2. Add few comments as admin, make sure the correct course urls are displayed to you along with the comment
    3. Refresh the page, make sure links are still correct
    4. login as a student and make a comment and refresh
    5. make sure all commenter names are linked to correct profile urls
    Show
    Goto a course>turn editing> add a comment block Add few comments as admin, make sure the correct course urls are displayed to you along with the comment Refresh the page, make sure links are still correct login as a student and make a comment and refresh make sure all commenter names are linked to correct profile urls
  • Affected Branches:
    MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
  • Fixed Branches:
    MOODLE_21_STABLE, MOODLE_22_STABLE
  • Pull from Repository:
  • Pull Master Branch:
    MDL-28952-master
  • Pull Master Diff URL:

Description

Prerequisite:

In the comments block, there are links to each commenting user's profile of this type:
http://MYMOODLEDOMAIN/user/view.php?id=USERID&course=COURSEID

But: "&" should be "&".

Now, if a user clicks this link, the course parameter isn't recognized and the user is forwarded to
http://MYMOODLEDOMAIN/user/profile.php?id=USERID
which he may or may not be able to view.

Replacing in /comment/lib.php, line 598
$c->profileurl = $url->out();
with
$c->profileurl = $url->out(false);
solves the problem, the user is shown view.php as desired, but I don't know which side-effects this has.

Issue Links

has a non-specific relationship to

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-28953 Hide link to user profile in comments block when linked user isn't enrolled

  • Minor - Minor loss of function where workaround is possible
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Michael de Raadt added a comment -

Thanks for reporting this. I was able to replicate the problem.

It looks like this ampersand is being escaped unnecessarily.

The link is correct when it is loaded by Ajax after the comment is added, but it is incorrect when someone else views it or the page is reloaded.

Show
Michael de Raadt added a comment - Thanks for reporting this. I was able to replicate the problem. It looks like this ampersand is being escaped unnecessarily. The link is correct when it is loaded by Ajax after the comment is added, but it is incorrect when someone else views it or the page is reloaded.
Hide
Permalink
Alexander Bias added a comment -

I just verified that the problem is still present in 2.2.x
I would be grateful if the small patch could be included into moodle core

Show
Alexander Bias added a comment - I just verified that the problem is still present in 2.2.x I would be grateful if the small patch could be included into moodle core
Hide
Permalink
Ankit Agarwal added a comment -

$url->out should really be used only when we have to print the output
up for review
Thanks

Show
Ankit Agarwal added a comment - $url->out should really be used only when we have to print the output up for review Thanks
Hide
Permalink
Rossiani Wijaya added a comment -

Looks good Ankit.

+1 for integration.

Show
Rossiani Wijaya added a comment - Looks good Ankit. +1 for integration.
Hide
Permalink
Ankit Agarwal added a comment -

Thanks Rosie for the review.
Sending for integration
Thanks

Show
Ankit Agarwal added a comment - Thanks Rosie for the review. Sending for integration Thanks
Hide
Permalink
Sam Hemelryk added a comment -

Hi guys,

There is a bit of an issue here as you are changing the result that is returned from the get_comments method (comments API).
Normally we would only allow such a change in master as it backporting it to stable may cause peoples own code to break.
If there's a good reason to backport then it can be considered. Either way I think the original solution Alexander suggested may be the safer approach for STABLE branches.
(For instance Moodle's plugins database uses this API and would be affected)

I've added Dongsheng as a watcher on this issue. Dongsheng would you mind having a look at this and seeing what you think about changing the result of the get_comments API.
Is it a good idea or do you think we should avoid it.

Cheers
Sam

Show
Sam Hemelryk added a comment - Hi guys, There is a bit of an issue here as you are changing the result that is returned from the get_comments method (comments API). Normally we would only allow such a change in master as it backporting it to stable may cause peoples own code to break. If there's a good reason to backport then it can be considered. Either way I think the original solution Alexander suggested may be the safer approach for STABLE branches. (For instance Moodle's plugins database uses this API and would be affected) I've added Dongsheng as a watcher on this issue. Dongsheng would you mind having a look at this and seeing what you think about changing the result of the get_comments API. Is it a good idea or do you think we should avoid it. Cheers Sam
Hide
Permalink
Dongsheng Cai added a comment -

Hello all

I think using `$c->profileurl = $url->out(false);` makes sense. I cannot see problems of fixing & in url, after all it's the expected result, is it?

Show
Dongsheng Cai added a comment - Hello all I think using `$c->profileurl = $url->out(false);` makes sense. I cannot see problems of fixing & in url, after all it's the expected result, is it?
Hide
Permalink
Sam Hemelryk added a comment -

Hi Dongsheng,

$url->out(false) I think is certainly the fix for the stable branches.
But for the master branch what do you think about changing profileurl from a string, to a moodle_url object?

(sorry I could have been clearer in my earlier question)

Cheers
Sam

Show
Sam Hemelryk added a comment - Hi Dongsheng, $url->out(false) I think is certainly the fix for the stable branches. But for the master branch what do you think about changing profileurl from a string, to a moodle_url object? (sorry I could have been clearer in my earlier question) Cheers Sam
Hide
Permalink
Dongsheng Cai added a comment -

Hi Sam

I suggest to use $url->out(false).

The return value from get_comments will be used by json_encode, thus moodle_url::__toString will be called, currently this method is using moodle_url::out(true)

Show
Dongsheng Cai added a comment - Hi Sam I suggest to use $url->out(false). The return value from get_comments will be used by json_encode, thus moodle_url::__toString will be called, currently this method is using moodle_url::out(true)
Hide
Permalink
Sam Hemelryk added a comment -

Awesome thanks DS, that clears it up nicely!

All up to you now Ankit.

Show
Sam Hemelryk added a comment - Awesome thanks DS, that clears it up nicely! All up to you now Ankit.
Hide
Permalink
Ankit Agarwal added a comment -

Thanks guys for the feedback,
Changes made, submitting again for integration.
Thanks

Show
Ankit Agarwal added a comment - Thanks guys for the feedback, Changes made, submitting again for integration. Thanks
Hide
Permalink
Sam Hemelryk added a comment -

Thanks Ankit, this has been integrated now

Show
Sam Hemelryk added a comment - Thanks Ankit, this has been integrated now
Hide
Permalink
Adrian Greeve added a comment -

Tested on branches 2.1, 2.2 and master. The link works properly now.
Thanks.

Show
Adrian Greeve added a comment - Tested on branches 2.1, 2.2 and master. The link works properly now. Thanks.
Hide
Permalink
Sam Hemelryk added a comment -

Congratulations are in order, you've made it, or at least your code has!
It's now part of Moodle and both the git and cvs repositories have been updated.

This issue is being marked as fixed and closed.

Thank you.

Show
Sam Hemelryk added a comment - Congratulations are in order, you've made it, or at least your code has! It's now part of Moodle and both the git and cvs repositories have been updated. This issue is being marked as fixed and closed. Thank you.

People

Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: