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

Badges ajax scripts should be managing output and headers in a better way

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.5.2, 2.6
    • Fix Version/s: 2.5.3
    • Component/s: Badges
    • Labels:
    • Testing Instructions:
      Hide

      This fix doesn't really fix a bug, but rather improves the way page headers are handled. So, by testing, we just need to make sure that nothing is broken.

      For the first 2 tests, your need to be able to push badges to the backpack (i.e. your web site should be accessible from the Internet). You need a user with any local badge earned and a user backpack connected.

      1. For testing changes in assertion.php: Login as a user and add this issued badge to your Mozilla backpack. Badge should be successfully added to the backpack. Check page headers while adding a badge to make sure that JSON is sent.

      2. For testing changes in backpackconnect.php: Try to connect and disconnect user backpack. Everything should work as before, and if user has a backpack account, it should be connected. It would be difficult to check page headers as the page is reloaded once backpack is connected. But it you can watch the headers while backpack is connecting, check backpackconnect.php for JSON headers.

      3. For testing changes in ajax.php: Login as admin. Go to Site Admin > Badges > Manage Badges. Check your browser for headers sent by ajax.php. Make sure they are of a type JSON.

      I have attached examples of page headers.

      Show
      This fix doesn't really fix a bug, but rather improves the way page headers are handled. So, by testing, we just need to make sure that nothing is broken. For the first 2 tests, your need to be able to push badges to the backpack (i.e. your web site should be accessible from the Internet). You need a user with any local badge earned and a user backpack connected. 1. For testing changes in assertion.php: Login as a user and add this issued badge to your Mozilla backpack. Badge should be successfully added to the backpack. Check page headers while adding a badge to make sure that JSON is sent. 2. For testing changes in backpackconnect.php: Try to connect and disconnect user backpack. Everything should work as before, and if user has a backpack account, it should be connected. It would be difficult to check page headers as the page is reloaded once backpack is connected. But it you can watch the headers while backpack is connecting, check backpackconnect.php for JSON headers. 3. For testing changes in ajax.php: Login as admin. Go to Site Admin > Badges > Manage Badges . Check your browser for headers sent by ajax.php. Make sure they are of a type JSON. I have attached examples of page headers.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-42072_master

      Description

      This is an issue that was introduced with MDL-40572

      badges/assertion.php should be managing headers and output better than it currently is.
      Perhaps the easiest way would be to simply define AJAX_SCRIPT at the top of the file, if not then lets copy the headers we need here and define things such that debugging and exception handling is minimised for this script.

        Gliffy Diagrams

        1. headers.png
          144 kB
        2. headers1.png
          150 kB

          Issue Links

            Activity

            Hide
            samhemelryk Sam Hemelryk added a comment -

            I'll leave this in your capable hands thanks Yuliya.

            Marked as triaged and must fix for 2.6

            Show
            samhemelryk Sam Hemelryk added a comment - I'll leave this in your capable hands thanks Yuliya. Marked as triaged and must fix for 2.6
            Hide
            ybozhko Yuliya Bozhko added a comment -

            Thanks, Sam! Fixed as you suggested with AJAX_SCRIPT

            Show
            ybozhko Yuliya Bozhko added a comment - Thanks, Sam! Fixed as you suggested with AJAX_SCRIPT
            Hide
            ybozhko Yuliya Bozhko added a comment -

            Also found that headers were not set in backpackconnect.php and ajax.php. Will fix them here too!

            Show
            ybozhko Yuliya Bozhko added a comment - Also found that headers were not set in backpackconnect.php and ajax.php. Will fix them here too!
            Hide
            fred Frédéric Massart added a comment -

            Hi Yuliya,

            the patch looks good to me. Could you just provide some testing instructions that cover the different files you have modified, also perhaps checking the HTTP headers using Firebug or so.

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Yuliya, the patch looks good to me. Could you just provide some testing instructions that cover the different files you have modified, also perhaps checking the HTTP headers using Firebug or so. Cheers, Fred
            Hide
            ybozhko Yuliya Bozhko added a comment -

            All done! Let me know you if you need more info

            Show
            ybozhko Yuliya Bozhko added a comment - All done! Let me know you if you need more info
            Hide
            fred Frédéric Massart added a comment -

            Seems good, thanks Yuliya!

            Show
            fred Frédéric Massart added a comment - Seems good, thanks Yuliya!
            Hide
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            samhemelryk Sam Hemelryk added a comment -

            Thanks Yuliya this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Yuliya this has been integrated now.
            Hide
            poltawski Dan Poltawski added a comment -

            grr. wrong button.

            Show
            poltawski Dan Poltawski added a comment - grr. wrong button.
            Hide
            markn Mark Nelson added a comment -

            Hi Yuliya, I found the issue MDL-42225 while testing this. If you could peer review that would be great.

            Show
            markn Mark Nelson added a comment - Hi Yuliya, I found the issue MDL-42225 while testing this. If you could peer review that would be great.
            Hide
            markn Mark Nelson added a comment -

            Ok, found MDL-42226 just after that one. If I find any more I will wait until this issue is tested before mentioning them.

            Show
            markn Mark Nelson added a comment - Ok, found MDL-42226 just after that one. If I find any more I will wait until this issue is tested before mentioning them.
            Hide
            ybozhko Yuliya Bozhko added a comment -

            Hi Mark,

            Submitted both fixes for integration. Thanks! I think this one MDL-42098 that was just added to integration caused the warnings

            Yuliya

            Show
            ybozhko Yuliya Bozhko added a comment - Hi Mark, Submitted both fixes for integration. Thanks! I think this one MDL-42098 that was just added to integration caused the warnings Yuliya
            Hide
            markn Mark Nelson added a comment -

            Thanks Yuliya for the quick turn around time. I suspect MDL-41489 is going to get quite a lot of issues due to MDL-42098!

            I haven't forgotten about testing this, just got stuck on a separate issue of mine! I will finish this shortly.

            Show
            markn Mark Nelson added a comment - Thanks Yuliya for the quick turn around time. I suspect MDL-41489 is going to get quite a lot of issues due to MDL-42098 ! I haven't forgotten about testing this, just got stuck on a separate issue of mine! I will finish this shortly.
            Hide
            markn Mark Nelson added a comment -

            Hi Yuliya, working as expected on integration master, but not on 2.5.

            I kept getting the message "We have encountered the following problem: Could not parse the undefined file at undefined file". I had a look at the link to the assertion page and found -

            <div class="notifytiny debuggingmessage">Coding problem: $PAGE->context was not set. You may have forgotten to call require_login() or $PAGE->set_context(). The page may not display correctly as a result<ul style="text-align: left"><li>line 451 of /lib/pagelib.php: call to debugging()</li><li>line 1454 of /lib/pagelib.php: call to moodle_page->magic_get_context()</li><li>line 1483 of /lib/setuplib.php: call to moodle_page->initialise_theme_and_output()</li><li>line 40 of /badges/assertion.php: call to bootstrap_renderer->__call()</li><li>line 40 of /badges/assertion.php: call to bootstrap_renderer->header()</li></ul></div>{"recipient":"sha256$a57c02c219f4a481520091779cbb77514792111ef8dc8b9b0f4c5c3a30ea77c8","salt":"badges1379478291","issued_on":"2013-10-10","evidence":"http:\/\/4182b9c2.ngrok.com\/i25\/badges\/badge.php?hash=6f968f6f256117a1d50ca80c09041ec428cb7859","badge":{"version":"0.5.0","name":"Legendary Badge","image":"http:\/\/4182b9c2.ngrok.com\/i25\/pluginfile.php\/1\/badges\/badgeimage\/4\/f1","description":"Legendary Badge","criteria":"http:\/\/4182b9c2.ngrok.com\/i25\/badges\/badge.php?hash=6f968f6f256117a1d50ca80c09041ec428cb7859","issuer":{"origin":"http:\/\/localhost","name":"Legendary Badge","contact":""}}}

            I created the patch https://github.com/markn86/moodle/commit/23119532765f3db59e326be6f09df13fe904a2a8 to fix this issue but am not sure if it is the correct way as my knowledge in this area is limited. If it looks good to you then I would ask an integrator to pull that into the 2.5 branch and set this back to waiting for testing.

            Thanks!

            Show
            markn Mark Nelson added a comment - Hi Yuliya, working as expected on integration master, but not on 2.5. I kept getting the message "We have encountered the following problem: Could not parse the undefined file at undefined file". I had a look at the link to the assertion page and found - <div class="notifytiny debuggingmessage">Coding problem: $PAGE->context was not set. You may have forgotten to call require_login() or $PAGE->set_context(). The page may not display correctly as a result<ul style="text-align: left"><li>line 451 of /lib/pagelib.php: call to debugging()</li><li>line 1454 of /lib/pagelib.php: call to moodle_page->magic_get_context()</li><li>line 1483 of /lib/setuplib.php: call to moodle_page->initialise_theme_and_output()</li><li>line 40 of /badges/assertion.php: call to bootstrap_renderer->__call()</li><li>line 40 of /badges/assertion.php: call to bootstrap_renderer->header()</li></ul></div>{"recipient":"sha256$a57c02c219f4a481520091779cbb77514792111ef8dc8b9b0f4c5c3a30ea77c8","salt":"badges1379478291","issued_on":"2013-10-10","evidence":"http:\/\/4182b9c2.ngrok.com\/i25\/badges\/badge.php?hash=6f968f6f256117a1d50ca80c09041ec428cb7859","badge":{"version":"0.5.0","name":"Legendary Badge","image":"http:\/\/4182b9c2.ngrok.com\/i25\/pluginfile.php\/1\/badges\/badgeimage\/4\/f1","description":"Legendary Badge","criteria":"http:\/\/4182b9c2.ngrok.com\/i25\/badges\/badge.php?hash=6f968f6f256117a1d50ca80c09041ec428cb7859","issuer":{"origin":"http:\/\/localhost","name":"Legendary Badge","contact":""}}} I created the patch https://github.com/markn86/moodle/commit/23119532765f3db59e326be6f09df13fe904a2a8 to fix this issue but am not sure if it is the correct way as my knowledge in this area is limited. If it looks good to you then I would ask an integrator to pull that into the 2.5 branch and set this back to waiting for testing. Thanks!
            Hide
            ybozhko Yuliya Bozhko added a comment -

            Hi Mark,

            This is strange as I can't reproduce it on my 2.5 test site Your patch looks fine if the context really needs to be set there. I have no idea why it is needed though, and why it doesn't break in 2.6...

            Yuliya

            Show
            ybozhko Yuliya Bozhko added a comment - Hi Mark, This is strange as I can't reproduce it on my 2.5 test site Your patch looks fine if the context really needs to be set there. I have no idea why it is needed though, and why it doesn't break in 2.6... Yuliya
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Interesting I could reproduce this in 25 and 26.
            I don't think setting the context here is correct, if I understand things correctly this script doesn't require moodle cookies.
            If so a better solution would be to use NO_MOODLE_COOKIES to disable cookies/sessions resolving this issue.

            Diffs at:

            Yuliya if you have a bit of time would you please confirm that this script doesn't need cookies and if possible give this a test.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Interesting I could reproduce this in 25 and 26. I don't think setting the context here is correct, if I understand things correctly this script doesn't require moodle cookies. If so a better solution would be to use NO_MOODLE_COOKIES to disable cookies/sessions resolving this issue. Diffs at: 25 https://github.com/samhemelryk/moodle/commit/42072-25i git pull git://github.com/samhemelryk/moodle.git 42072-25i 26 https://github.com/samhemelryk/moodle/commit/42072-26i git pull git://github.com/samhemelryk/moodle.git 42072-26i Yuliya if you have a bit of time would you please confirm that this script doesn't need cookies and if possible give this a test. Many thanks Sam
            Hide
            ybozhko Yuliya Bozhko added a comment -

            Thanks, Sam! As far as I know, cookies are not required. Testing now Will get back to you in a bit.

            Show
            ybozhko Yuliya Bozhko added a comment - Thanks, Sam! As far as I know, cookies are not required. Testing now Will get back to you in a bit.
            Hide
            ybozhko Yuliya Bozhko added a comment -

            Hi Sam,

            Your fix works fine for me. But as I said, I didn't have problems before.

            I checked cookies, and right now there is only MoodleSession cookie, but no MOODLEID1_ cookie. That's the only difference I spotted.

            Yuliya

            Show
            ybozhko Yuliya Bozhko added a comment - Hi Sam, Your fix works fine for me. But as I said, I didn't have problems before. I checked cookies, and right now there is only MoodleSession cookie, but no MOODLEID1_ cookie. That's the only difference I spotted. Yuliya
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Yuliya, I've pulled in my fix now and have sent back to testing... all yours again Mark

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Yuliya, I've pulled in my fix now and have sent back to testing... all yours again Mark
            Hide
            markn Mark Nelson added a comment -

            Thanks Sam, works now.

            Very very strange that both worked for Yuliya, only master worked for me and none for yourself.

            Show
            markn Mark Nelson added a comment - Thanks Sam, works now. Very very strange that both worked for Yuliya, only master worked for me and none for yourself.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels.

            Or, if you prefer, yes, you fixed that boring issue.

            Thanks anyway! Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels. Or, if you prefer, yes, you fixed that boring issue. Thanks anyway! Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13