Moodle
  1. Moodle
  2. MDL-4781

Site news - forum length setting has no effect for certain users

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.3, 1.9, 2.0.1, 2.2
    • Fix Version/s: 2.2.7, 2.3.4, 2.4.1
    • Component/s: Forum
    • Labels:
    • Environment:
      All
    • Database:
      MySQL
    • Testing Instructions:
      Hide

      1. As an administrator, create a very long post on the site news forum.
      2. Login as a different role (student, teacher).
      3. Verify that the post is shortened.

      Show
      1. As an administrator, create a very long post on the site news forum. 2. Login as a different role (student, teacher). 3. Verify that the post is shortened.
    • Affected Branches:
      MOODLE_15_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-4781-master

      Description

      Please see http://moodle.org/mod/forum/discuss.php?d=39697 and http://moodle.org/mod/forum/discuss.php?d=36918

      This makes it very messy to use news as the option for the front page.

      No difference if viewed in i.e. or firefox.

        Gliffy Diagrams

          Activity

          Hide
          Vy-Shane Sin Fat added a comment -

          I could not reproduce this with the latest 1.5, nor with the latest 1.6. This is an old bug. Maybe it's already been fixed?

          Show
          Vy-Shane Sin Fat added a comment - I could not reproduce this with the latest 1.5, nor with the latest 1.6. This is an old bug. Maybe it's already been fixed?
          Hide
          Helen Foster added a comment -

          Reopening this issue, as it remains a problem in Moodle 1.9.

          See also http://moodle.org/mod/forum/discuss.php?d=94543

          Show
          Helen Foster added a comment - Reopening this issue, as it remains a problem in Moodle 1.9. See also http://moodle.org/mod/forum/discuss.php?d=94543
          Hide
          Helen Foster added a comment -

          Just tested with latest 1.9+, and found that, with defaultfrontpageroleid set to student, an admin sees the reduced length site news item, whereas a student sees the full length item.

          Show
          Helen Foster added a comment - Just tested with latest 1.9+, and found that, with defaultfrontpageroleid set to student, an admin sees the reduced length site news item, whereas a student sees the full length item.
          Hide
          Petr Skoda added a comment -

          I think I fixed this recently, please reopen if you can replicate this...

          thanks!

          Show
          Petr Skoda added a comment - I think I fixed this recently, please reopen if you can replicate this... thanks!
          Hide
          Helen Foster added a comment -

          Reopening, as the problem remains in the latest 1.9+. Also, if the front page is set to display news items, then non-logged in users also see the full-length site news item.

          Show
          Helen Foster added a comment - Reopening, as the problem remains in the latest 1.9+. Also, if the front page is set to display news items, then non-logged in users also see the full-length site news item.
          Hide
          Petr Skoda added a comment -

          I have checked the code, the problem is that forum_shorten_post($message) uses a very simplified algorithm that does not work in many cases - it is not unicode aware at all.

          Solution would be to rewrite it completely and use tidyhtml or htmlpurifier, in anycase this will require a lot of work

          Show
          Petr Skoda added a comment - I have checked the code, the problem is that forum_shorten_post($message) uses a very simplified algorithm that does not work in many cases - it is not unicode aware at all. Solution would be to rewrite it completely and use tidyhtml or htmlpurifier, in anycase this will require a lot of work
          Hide
          Petr Skoda added a comment -

          hmm, maybe we could do some shortening using javascrip instead - like a small +/- icon at the bottom of longer posts...

          Show
          Petr Skoda added a comment - hmm, maybe we could do some shortening using javascrip instead - like a small +/- icon at the bottom of longer posts...
          Hide
          Helen Foster added a comment -

          Just noting another discussion about this problem:
          http://moodle.org/mod/forum/discuss.php?d=134924

          Show
          Helen Foster added a comment - Just noting another discussion about this problem: http://moodle.org/mod/forum/discuss.php?d=134924
          Hide
          Dušan Ristić added a comment -

          Using Moodle 1.9.9+ build 20100908

          Just hit this problem today, users that aren't registered/loged in are shown full text instead of short version with link.

          Made a fix and still not seeing any side effects. In moodle/mod/forum/lib.php line 4939 should look like:

          $link = forum_user_can_post($forum, $discussion, $USER, $cm, $course, $modcontext);

          I've just replaced it with:

          $link = forum_user_can_see_post($forum, $discussion, $post, $USER, $cm);

          It doesn't seems to be afecting other controls (edit/delete).

          Show
          Dušan Ristić added a comment - Using Moodle 1.9.9+ build 20100908 Just hit this problem today, users that aren't registered/loged in are shown full text instead of short version with link. Made a fix and still not seeing any side effects. In moodle/mod/forum/lib.php line 4939 should look like: $link = forum_user_can_post($forum, $discussion, $USER, $cm, $course, $modcontext); I've just replaced it with: $link = forum_user_can_see_post($forum, $discussion, $post, $USER, $cm); It doesn't seems to be afecting other controls (edit/delete).
          Hide
          Petr Skoda added a comment -

          reassigning to HQ

          Show
          Petr Skoda added a comment - reassigning to HQ
          Hide
          Helen Foster added a comment -

          Just checked and found 2.0.1 still affected by this issue. Only admins see shorted news forum posts. Non-logged in users, students and guests all see the full-length post.

          Show
          Helen Foster added a comment - Just checked and found 2.0.1 still affected by this issue. Only admins see shorted news forum posts. Non-logged in users, students and guests all see the full-length post.
          Hide
          Susan Mangan added a comment -

          I guess our posts haven't been incredibly long until now as I JUST noticed this bug!! ... after years of using Moodle. crazy. I log in as Admin to post messages on the front page and it always shortens so I never doubted it!!! Anyway - fyi another potential workaround that may or may not work for you is to post your full entry as a Site Event and hyperlink to it from the News Forum where you can post a shorter blurb there.

          Show
          Susan Mangan added a comment - I guess our posts haven't been incredibly long until now as I JUST noticed this bug!! ... after years of using Moodle. crazy. I log in as Admin to post messages on the front page and it always shortens so I never doubted it!!! Anyway - fyi another potential workaround that may or may not work for you is to post your full entry as a Site Event and hyperlink to it from the News Forum where you can post a shorter blurb there.
          Hide
          Chris Murad added a comment -

          I am encountering this Bug on 2.1 as well.

          Show
          Chris Murad added a comment - I am encountering this Bug on 2.1 as well.
          Hide
          Helen Foster added a comment -

          Thanks Chris, also in 2.2.

          Show
          Helen Foster added a comment - Thanks Chris, also in 2.2.
          Hide
          David Dyet added a comment -

          We are experiencing this on Moodle 2.2 as well.

          Show
          David Dyet added a comment - We are experiencing this on Moodle 2.2 as well.
          Hide
          Charles Fulton added a comment -

          Duan's proposed patch for 1.9.9 works with only minor tweaks. Is there a use case for forum_print_latest_discussions() not setting $link to true in all cases? It will anyway if there's even one reply, regardless of the user's ability to reply.

          Show
          Charles Fulton added a comment - Duan's proposed patch for 1.9.9 works with only minor tweaks. Is there a use case for forum_print_latest_discussions() not setting $link to true in all cases? It will anyway if there's even one reply, regardless of the user's ability to reply.
          Hide
          Adrian Greeve added a comment -

          Hi Charles,

          I think that the alteration is ok. It definitely fixes the problem with posts on the front page not being shortened when long. I was concerned about the problem that Petr mentioned about not being unicode aware and with a bit of testing I noticed that it doesn't handle some languages all that well. I think that might be out of the scope of this issue, so I'm happy for it to move to integration.

          Show
          Adrian Greeve added a comment - Hi Charles, I think that the alteration is ok. It definitely fixes the problem with posts on the front page not being shortened when long. I was concerned about the problem that Petr mentioned about not being unicode aware and with a bit of testing I noticed that it doesn't handle some languages all that well. I think that might be out of the scope of this issue, so I'm happy for it to move to integration.
          Hide
          Per Hessellund Laursen added a comment -

          In Moodle 2.2. we had the problem with no effect for certain users. After opening the forum for logged in users reply, the forum respect the length setting for all logged in users

          Show
          Per Hessellund Laursen added a comment - In Moodle 2.2. we had the problem with no effect for certain users. After opening the forum for logged in users reply, the forum respect the length setting for all logged in users
          Hide
          Eloy Lafuente (stronk7) added a comment -

          U P S T R E A M I Z E D !

          Many thanks, this is now available in all the repos (git & cvs).

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks, this is now available in all the repos (git & cvs). Closing, ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Doh,

          somehow this issue was closed incorrectly when processing all the integrated issues this week. (sort of most voted and current in integration filters mix). Apologies for the confusion, reseting to previous status!

          Ciao, Eloy

          Show
          Eloy Lafuente (stronk7) added a comment - Doh, somehow this issue was closed incorrectly when processing all the integrated issues this week. (sort of most voted and current in integration filters mix). Apologies for the confusion, reseting to previous status! Ciao, Eloy
          Hide
          Charles Fulton added a comment -

          Hi Adrian, could you push this to integration? Thanks, Charles.

          Show
          Charles Fulton added a comment - Hi Adrian, could you push this to integration? Thanks, Charles.
          Hide
          Adrian Greeve added a comment -

          Submitting back to integration after being accidentally closed.

          Show
          Adrian Greeve added a comment - Submitting back to integration after being accidentally closed.
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Hi Charles,

          I'm sending this back sorry.
          The reason being an incorrect function call.
          The use of $discussion as the third arg to forum_user_can_see_post. Looking at the phpdocs for forum_user_can_see_post the third arg should be the post that we are checking the users ability to view.

          In general if we see this sort of thing popping up we send it back even if we are 100% sure that at the time of submission it is working correctly. Off the top of my head there are two good reasons for this:

          1. The function expects it to be a post. Should the function evolve and interaction with the param change there is a very high chance of regressions and importantly not regressions with the function but with the calling code.
          2. The function trusts it is a post object. PHP passes all objects by reference, as part of the functions operation it may in fact modify the object [by reference]. That object may end up pretty messed up and may cause untold trouble if passed to further API functions.

          In general as well if you have to trick a function or pass fake vars then its best to take a close look at what you're trying to achieve and see what can be done.
          In this case I think perhaps its an easy solution, there is a function forum_user_can_see_discussion (directly above forum_user_can_see_post in forum/lib.php) that I imagine is doing what you want?

          Many thanks, and sorry for the fuss.
          Sam

          Show
          Sam Hemelryk added a comment - Hi Charles, I'm sending this back sorry. The reason being an incorrect function call. The use of $discussion as the third arg to forum_user_can_see_post. Looking at the phpdocs for forum_user_can_see_post the third arg should be the post that we are checking the users ability to view. In general if we see this sort of thing popping up we send it back even if we are 100% sure that at the time of submission it is working correctly. Off the top of my head there are two good reasons for this: The function expects it to be a post. Should the function evolve and interaction with the param change there is a very high chance of regressions and importantly not regressions with the function but with the calling code. The function trusts it is a post object. PHP passes all objects by reference, as part of the functions operation it may in fact modify the object [by reference] . That object may end up pretty messed up and may cause untold trouble if passed to further API functions. In general as well if you have to trick a function or pass fake vars then its best to take a close look at what you're trying to achieve and see what can be done. In this case I think perhaps its an easy solution, there is a function forum_user_can_see_discussion (directly above forum_user_can_see_post in forum/lib.php) that I imagine is doing what you want? Many thanks, and sorry for the fuss. Sam
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Charles Fulton added a comment -

          Thanks Sam for that detailed feedback--you're absolutely right and I feel silly for not even looking for that function. Rebased and updated.

          Show
          Charles Fulton added a comment - Thanks Sam for that detailed feedback--you're absolutely right and I feel silly for not even looking for that function. Rebased and updated.
          Hide
          Charles Fulton added a comment -

          Rebased against 2.4/2.5.

          Show
          Charles Fulton added a comment - Rebased against 2.4/2.5.
          Hide
          Adrian Greeve added a comment -

          Everything looks fine here. I'll push it back to integration.

          Show
          Adrian Greeve added a comment - Everything looks fine here. I'll push it back to integration.
          Hide
          Sam Hemelryk added a comment -

          Thanks Charles, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Charles, this has been integrated now
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Charles.
          Post is shortened in site news for admin/teacher/student.

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Charles. Post is shortened in site news for admin/teacher/student.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And your fantastic code has met core, hope they become good friends for a long period.

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!

            People

            • Votes:
              23 Vote for this issue
              Watchers:
              19 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: