Moodle
  1. Moodle
  2. MDL-25241

Forum search results no longer show rating, or scales

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Forum
    • Labels:
    • Testing Instructions:
      Hide

      In Site Administration turn off "enableajax".
      Go into a forum and view a discussion. Make a note of a word mentioned in a post and of the ratings situation for that post. Make sure that you choose a post that you are able to rate.
      Type the word into the search forums box (its towards the top right of the screen).
      Check that the rating matches that which was displayed within the forum discussion.
      Submit a rating and check that you are returned to the search page.

      In Site Administration turn on "enableajax".
      Search for the same forum post.
      Check that you can submit a rating via ajax.

      Show
      In Site Administration turn off "enableajax". Go into a forum and view a discussion. Make a note of a word mentioned in a post and of the ratings situation for that post. Make sure that you choose a post that you are able to rate. Type the word into the search forums box (its towards the top right of the screen). Check that the rating matches that which was displayed within the forum discussion. Submit a rating and check that you are returned to the search page. In Site Administration turn on "enableajax". Search for the same forum post. Check that you can submit a rating via ajax.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      MDL-25241_forum_search_ratings
    • Rank:
      2790

      Issue Links

        Activity

        Hide
        Helen Foster added a comment -

        Linking to possibly related issue.

        Show
        Helen Foster added a comment - Linking to possibly related issue.
        Hide
        Sam Hemelryk added a comment -

        Hi Andrew,

        I think this is probably more in your realm of expertise.
        forum/search.php is finding a pile of posts and printing but obviously isn't establishing post->rating for those posts.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Andrew, I think this is probably more in your realm of expertise. forum/search.php is finding a pile of posts and printing but obviously isn't establishing post->rating for those posts. Cheers Sam
        Hide
        Andrew Davis added a comment - - edited

        Im after a peer review. This works but I have the feeling I'm doing something wrong. Monday afternoon and I can think.

        Specifically the problem is this

        $ratingoptions->returnurl = htmlspecialchars_decode($PAGE->url);//dont want &'s encoded
        

        It feels wrong having to manually unescape the URL but if I dont I wind up with the browser trying to visit a url like

        /mod/forum/search.php?search=hi there admin&id=2

        and get the error "A required parameter (id) was missing"

        Show
        Andrew Davis added a comment - - edited Im after a peer review. This works but I have the feeling I'm doing something wrong. Monday afternoon and I can think. Specifically the problem is this $ratingoptions->returnurl = htmlspecialchars_decode($PAGE->url); //dont want &'s encoded It feels wrong having to manually unescape the URL but if I dont I wind up with the browser trying to visit a url like /mod/forum/search.php?search=hi there admin&id=2 and get the error "A required parameter (id) was missing"
        Hide
        Sam Hemelryk added a comment -

        Hey dude,

        A quick check of the code and I didn't spot anything other than what you already know. (am on the iPad so it was a quick look)
        Have a go with:

        $PAGE->url->out(false);
        

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hey dude, A quick check of the code and I didn't spot anything other than what you already know. (am on the iPad so it was a quick look) Have a go with: $PAGE->url->out( false ); Cheers Sam
        Hide
        Andrew Davis added a comment -

        Thanks Sam

        That makes non-ajax ratings work in master. Ajax ratings however now appear to be broken. I'm seeing the same issue as Michael reports in MDL-29333.

        Show
        Andrew Davis added a comment - Thanks Sam That makes non-ajax ratings work in master. Ajax ratings however now appear to be broken. I'm seeing the same issue as Michael reports in MDL-29333 .
        Hide
        Rick Jerz added a comment -

        Hi everybody, I'm Rick, the one who started 209333. I just want to let you know that I still get this JSON error intermittently when I grade a forum post. However, I cannot pinpoint precisely when it occurs. Usually, when I am grading forum posts I am doing so fairly quickly. The JSON message pops up, and then goes away. Then I think "Hmmm, what did I just do?" It happened so quickly that I can't remember, but I think it is related to leaving the forum discussion, that is, jumping to another topic.

        Show
        Rick Jerz added a comment - Hi everybody, I'm Rick, the one who started 209333. I just want to let you know that I still get this JSON error intermittently when I grade a forum post. However, I cannot pinpoint precisely when it occurs. Usually, when I am grading forum posts I am doing so fairly quickly. The JSON message pops up, and then goes away. Then I think "Hmmm, what did I just do?" It happened so quickly that I can't remember, but I think it is related to leaving the forum discussion, that is, jumping to another topic.
        Hide
        Andrew Davis added a comment -

        This issue can be integrated independently of MDL-29333 however it cannot be tested until MDL-29333 has been integrated.

        Show
        Andrew Davis added a comment - This issue can be integrated independently of MDL-29333 however it cannot be tested until MDL-29333 has been integrated.
        Hide
        Andrew Davis added a comment -

        This is now up for peer review. Ill add the other branches once that is done.

        Show
        Andrew Davis added a comment - This is now up for peer review. Ill add the other branches once that is done.
        Hide
        Sam Hemelryk added a comment -

        Thanks Andrew, looks good to me

        Show
        Sam Hemelryk added a comment - Thanks Andrew, looks good to me
        Hide
        Aparup Banerjee added a comment -

        note: MDL-29333 has been integrated at this point.

        Show
        Aparup Banerjee added a comment - note: MDL-29333 has been integrated at this point.
        Hide
        Aparup Banerjee added a comment -

        spoke with Andrew about this.

        It might be more beneficial to add the ratings info to search results from within the forum_search_posts() instead so we can see from other places: mod/forum/user.php

        Show
        Aparup Banerjee added a comment - spoke with Andrew about this. It might be more beneficial to add the ratings info to search results from within the forum_search_posts() instead so we can see from other places: mod/forum/user.php
        Hide
        Andrew Davis added a comment -

        I'm half way through implementing that suggestion and have noticed something that may make adding ratings handling to forum_search_posts() undesirable. To attach ratings to the posts we need to know what forum each post is in so we have to do something along these lines (obviously could be done as a single query):

        if (! $discussion = $DB->get_record('forum_discussions', array('id' => $post->discussion))) {
                print_error('invaliddiscussionid', 'forum');
            }
            if (! $forum = $DB->get_record('forum', array('id' => "$discussion->forum"))) {
                print_error('invalidforumid', 'forum');
            }
        

        then get some data from $forum. The problem is that search.php then has to repeat this same process. Moving the ratings handling into forum_search_posts() means at least one extra trip to the database for every post the search returns. I could fix this by making more significant alterations to forum_search_posts() to make it attach all the required discussion and forum information to each post it returns. I'm not sure if its worth the effort.

        Also, mod/forum/user.php is actually already attaching ratings to the posts. See line 134 and 165. If I do go ahead with the modifications to forum_search_posts() the ratings code will need to be removed from mod/forum/user.php

        Show
        Andrew Davis added a comment - I'm half way through implementing that suggestion and have noticed something that may make adding ratings handling to forum_search_posts() undesirable. To attach ratings to the posts we need to know what forum each post is in so we have to do something along these lines (obviously could be done as a single query): if (! $discussion = $DB->get_record('forum_discussions', array('id' => $post->discussion))) { print_error('invaliddiscussionid', 'forum'); } if (! $forum = $DB->get_record('forum', array('id' => "$discussion->forum" ))) { print_error('invalidforumid', 'forum'); } then get some data from $forum. The problem is that search.php then has to repeat this same process. Moving the ratings handling into forum_search_posts() means at least one extra trip to the database for every post the search returns. I could fix this by making more significant alterations to forum_search_posts() to make it attach all the required discussion and forum information to each post it returns. I'm not sure if its worth the effort. Also, mod/forum/user.php is actually already attaching ratings to the posts. See line 134 and 165. If I do go ahead with the modifications to forum_search_posts() the ratings code will need to be removed from mod/forum/user.php
        Hide
        Sam Hemelryk added a comment -

        Hmm perhaps worth consideration is that I am presently trying to get a patch through that will see mod/forum/users.php no longer using the forum_search_posts, instead it has its own method forum_get_posts_by_user.
        I'm asking Eloy to have a look at it before it goes up for integration, obviously its not guaranteed to go in yet however perhaps its worth waiting to see as thats 1 less place using that method.

        Food for thought... MDL-28615 is the issue

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hmm perhaps worth consideration is that I am presently trying to get a patch through that will see mod/forum/users.php no longer using the forum_search_posts, instead it has its own method forum_get_posts_by_user. I'm asking Eloy to have a look at it before it goes up for integration, obviously its not guaranteed to go in yet however perhaps its worth waiting to see as thats 1 less place using that method. Food for thought... MDL-28615 is the issue Cheers Sam
        Hide
        Aparup Banerjee added a comment -

        ok spoke with Andrew and Sam, proceeding to integrate this as is for now.. a strong effort in progress in MDL-28615 which seems to be refactoring a lot anyway.

        and this is integrated and ready for testing

        Show
        Aparup Banerjee added a comment - ok spoke with Andrew and Sam, proceeding to integrate this as is for now.. a strong effort in progress in MDL-28615 which seems to be refactoring a lot anyway. and this is integrated and ready for testing
        Hide
        Rajesh Taneja added a comment -

        Works Great
        Thanks for fixing this Andrew.

        Show
        Rajesh Taneja added a comment - Works Great Thanks for fixing this Andrew.
        Hide
        Aparup Banerjee added a comment -

        fixes have been rolled merrily up the stream! Thanks everybody!

        Show
        Aparup Banerjee added a comment - fixes have been rolled merrily up the stream! Thanks everybody!

          People

          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: