Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37687

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4, 2.5
    • Fix Version/s: 2.4.2
    • Component/s: Comments, 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 Master Branch:
      37687_master_only_accesslib_fix

      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"

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              nebgor 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
              nebgor 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
              nebgor Aparup Banerjee added a comment -

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

              Show
              nebgor Aparup Banerjee added a comment - i've got a patch for this for moodle. This needs to be moved to become MDL.
              Hide
              nebgor 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
              nebgor 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
              rajeshtaneja 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
              rajeshtaneja 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
              nebgor 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
              nebgor 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
              nebgor Aparup Banerjee added a comment -

              created MDLSITE-2121

              Show
              nebgor Aparup Banerjee added a comment - created MDLSITE-2121
              Hide
              rajeshtaneja 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
              rajeshtaneja 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
              nebgor 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
              nebgor 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
              stronk7 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
              stronk7 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 CiBoT added a comment -

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

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              nebgor 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
              nebgor 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
              nebgor Aparup Banerjee added a comment -

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

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

              Thanks Aparup,

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

              Show
              damyon Damyon Wiese added a comment - Thanks Aparup, I've pushed the "alternative + reluctant" version to master and 24.
              Hide
              damyon 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 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
              phalacee Jason Fowler added a comment -

              All works fine Aparup

              Show
              phalacee Jason Fowler added a comment - All works fine Aparup
              Hide
              damyon 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 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:
                    Fix Release Date:
                    11/Mar/13