Moodle
  1. Moodle
  2. MDL-37687

Comments paging seems broken for guests with renderer's $page passed to comment:init()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4, 2.5
    • Fix Version/s: 2.4.2
    • Component/s: Commenting, Libraries
    • Labels:
    • Testing Instructions:
      Hide

      (pass in a renderer's $page property instead of nothing to commit::init() )
      1) create and display a plugin (or install local_plugins, ankit/marina has a working installation too) in the frontpage with a lot of comments on an entry so that it paginates.
      2) ensure that a non-logged-in-user can access the comments section of a plugin.
      3) access the plugin as a non-logged-in-user and see other comment pages as a non-logged-in-user only. (click on pages 2 onwards of the comments section)
      4) follow test instructions. pass this test.

      Show
      (pass in a renderer's $page property instead of nothing to commit::init() ) 1) create and display a plugin (or install local_plugins, ankit/marina has a working installation too) in the frontpage with a lot of comments on an entry so that it paginates. 2) ensure that a non-logged-in-user can access the comments section of a plugin. 3) access the plugin as a non-logged-in-user and see other comment pages as a non-logged-in-user only. (click on pages 2 onwards of the comments section) 4) follow test instructions. pass this test.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      MDL-37687_m24
    • Pull Master Branch:
      37687_master_only_accesslib_fix
    • Rank:
      46825

      Description

      1. With Chrome, go here https://moodle.org/plugins/view.php?plugin=theme_bootstrap
      2. Don't be logged in.
      3. Try to navigate to the second page of comments.

      Expected result: Comments area refreshes and shows second page.

      Actual result: "Unsupported redirect detected, script execution terminated"

        Issue Links

          Activity

          Hide
          Aparup Banerjee added a comment -

          oh you're right! i've duplicated this on chrome. Ah, on firefox "Incorrect sesskey submitted, form not accepted!" so my guess is i have to remove some auth going on in the ajax.

          Show
          Aparup Banerjee added a comment - oh you're right! i've duplicated this on chrome. Ah, on firefox "Incorrect sesskey submitted, form not accepted!" so my guess is i have to remove some auth going on in the ajax.
          Hide
          Aparup Banerjee added a comment -

          i've got a patch for this for moodle. This needs to be moved to become MDL.

          Show
          Aparup Banerjee added a comment - i've got a patch for this for moodle. This needs to be moved to become MDL.
          Hide
          Aparup Banerjee added a comment -

          ok, lib/tests/accesslib_test.php:281 fails if i make test_get_context_info_array() return the SITE's course so i've made it an optional mode so that it can be backported easily (to 2.4 for an easy update to moodle.org). also added to the unit test for the option.

          putting this up for peer-review

          Show
          Aparup Banerjee added a comment - ok, lib/tests/accesslib_test.php:281 fails if i make test_get_context_info_array() return the SITE's course so i've made it an optional mode so that it can be backported easily (to 2.4 for an easy update to moodle.org). also added to the unit test for the option. putting this up for peer-review
          Hide
          Rajesh Taneja added a comment -

          Hi! Aparup,

          As discussed, I can see comments in db activity (not in glossary) without any problem.
          Seems plugins need to work in same way as db and there is a better way to go about this problem.

          Cheers

          Show
          Rajesh Taneja added a comment - Hi! Aparup, As discussed, I can see comments in db activity (not in glossary) without any problem. Seems plugins need to work in same way as db and there is a better way to go about this problem. Cheers
          Hide
          Aparup Banerjee added a comment -

          Hi Raj,
          thanks for your review.
          i've investigated the differences here.
          mod/data/view.php does comment::init() (i.e.: $PAGE as default) where as
          in local_plugins i'm doing a comment::init($page) (plugin's renderer)

          it seems to use the $page within the renderer is the way as it is further modified within the renderer anyway and that is the true instance we want. In saying this there is no use of comment:init($page) within core.

          blocks/comments/block_comments.php:39: comment::init();
          blog/index.php:27:comment::init();
          mod/assign/submission/comments/locallib.php:60: comment::init();
          mod/assign/submission/comments/locallib.php:141: comment::init();
          mod/data/view.php:85: comment::init();
          mod/glossary/view.php:59:comment::init();

          so there is no way to test this correctly until these other modules/plugins are *corrected to pass in their own renderers (if any). i'll send this up for integrators to have a look at to get even more feedback.

          ps: in any case, i've now a quick fix for moodle.org will link an MDLSITE (this MDL was originally an mdlsite).

          Show
          Aparup Banerjee added a comment - Hi Raj, thanks for your review. i've investigated the differences here. mod/data/view.php does comment::init() (i.e.: $PAGE as default) where as in local_plugins i'm doing a comment::init($page) (plugin's renderer) it seems to use the $page within the renderer is the way as it is further modified within the renderer anyway and that is the true instance we want. In saying this there is no use of comment:init($page) within core. blocks/comments/block_comments.php:39: comment::init(); blog/index.php:27:comment::init(); mod/assign/submission/comments/locallib.php:60: comment::init(); mod/assign/submission/comments/locallib.php:141: comment::init(); mod/data/view.php:85: comment::init(); mod/glossary/view.php:59:comment::init(); so there is no way to test this correctly until these other modules/plugins are *corrected to pass in their own renderers (if any). i'll send this up for integrators to have a look at to get even more feedback. ps: in any case, i've now a quick fix for moodle.org will link an MDLSITE (this MDL was originally an mdlsite).
          Hide
          Aparup Banerjee added a comment -

          created MDLSITE-2121

          Show
          Aparup Banerjee added a comment - created MDLSITE-2121
          Hide
          Rajesh Taneja added a comment -

          Hello Aparup,

          Can you please update testing instructions and remove glossary.
          FYI: vistor can view comments on db activity without logging in.

          Show
          Rajesh Taneja added a comment - Hello Aparup, Can you please update testing instructions and remove glossary. FYI: vistor can view comments on db activity without logging in.
          Hide
          Aparup Banerjee added a comment -

          Hi Raj, i've updated the test. i'm not really sure how this can be tested by core itself so it will require an installation of some plugin to test in core.
          I suggested this be taken up hand in hand with some effort to determin the need for proper renderers within glossary and database activities.

          Show
          Aparup Banerjee added a comment - Hi Raj, i've updated the test. i'm not really sure how this can be tested by core itself so it will require an installation of some plugin to test in core. I suggested this be taken up hand in hand with some effort to determin the need for proper renderers within glossary and database activities.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, but I don't think this sort of "behavioral parameters" are good at all:

          1) What if you are asking for a category context
          2) Or what if tomorrow you want to get the "news forum" by default, would you be adding another new "thisspecialcontextreturnsfirstnewsforumifiscourse" ? LOL, crazy!

          So, I'd not modify the accesslib function at all and, instead would look for returned values and perform the changes in the caller. 100%.

          Talking about callers... is "comment_ajax.php" the only one needing fix? What happens with js disabled?

          Sorry, reopening. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, but I don't think this sort of "behavioral parameters" are good at all: 1) What if you are asking for a category context 2) Or what if tomorrow you want to get the "news forum" by default, would you be adding another new "thisspecialcontextreturnsfirstnewsforumifiscourse" ? LOL, crazy! So, I'd not modify the accesslib function at all and, instead would look for returned values and perform the changes in the caller. 100%. Talking about callers... is "comment_ajax.php" the only one needing fix? What happens with js disabled? Sorry, reopening. Ciao
          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
          Aparup Banerjee added a comment -

          ok that parameterisation was to be 'bc' in hopes of fixing moodle.org/plugins asap :-D
          (the issue seems to be only for ajax here)

          really the fix would've been https://github.com/nebgor/moodle/compare/moodle:master...nebgor:MDL-37687_master_only_accesslib_fix as this makes sense for the access lib api rather than having a null course returned.... but

          here's the 'alternative + reluctant' one : https://github.com/nebgor/moodle/compare/moodle:master...nebgor:MDL-37687_comments_ajax_fix

          Show
          Aparup Banerjee added a comment - ok that parameterisation was to be 'bc' in hopes of fixing moodle.org/plugins asap :-D (the issue seems to be only for ajax here) really the fix would've been https://github.com/nebgor/moodle/compare/moodle:master...nebgor:MDL-37687_master_only_accesslib_fix as this makes sense for the access lib api rather than having a null course returned.... but here's the 'alternative + reluctant' one : https://github.com/nebgor/moodle/compare/moodle:master...nebgor:MDL-37687_comments_ajax_fix
          Hide
          Aparup Banerjee added a comment -

          its late friday - i'm sending this up for integration review for next week.

          Show
          Aparup Banerjee added a comment - its late friday - i'm sending this up for integration review for next week.
          Hide
          Damyon Wiese added a comment -

          Thanks Aparup,

          I've pushed the "alternative + reluctant" version to master and 24.

          Show
          Damyon Wiese added a comment - Thanks Aparup, I've pushed the "alternative + reluctant" version to master and 24.
          Hide
          Damyon Wiese added a comment - - edited

          I didn't backport further than 2.4 as this bug is not reproducible in core and I'm not sure it affects anyone other than the plugins DB.

          Show
          Damyon Wiese added a comment - - edited I didn't backport further than 2.4 as this bug is not reproducible in core and I'm not sure it affects anyone other than the plugins DB.
          Hide
          Jason Fowler added a comment -

          All works fine Aparup

          Show
          Jason Fowler added a comment - All works fine Aparup
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work - this issue has made it! Moodle is now a little bit better.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks for your hard work - this issue has made it! Moodle is now a little bit better. Regards, Damyon

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: