Moodle
  1. Moodle
  2. MDL-32873

Separate out course ajax enabled checking to separate function

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      Open a course page with a number of course modules present

      • Ensure that all of the AJAX resource buttons still work
        • right/left
        • show/hide
        • group mode toggle (on activities)
        • delete
        • drag/drop
      • Ensure that the AJAX section buttons work as inticipated
        • show/hide
        • drag/drop

      Then:

      • disable ajax support by turning off the 'enableajax' setting
        • confirm that we're back to the non-ajax versions

      For bonus points:

      • (re-enable the sitewide enableajax setting)
      • open your current theme's config.php and add:

        $THEME->enablecourseajax = false;

        • confirm that we're back to the non-ajax versions of all icons
        • remove the theme config line
      • open your current format's lib.php and find the callback_$name_ajax_support function and change the capable line:

        $ajaxsupport->capable = false;

        • confirm that we're back to the non-ajax versions of all icons
        • change things back to normal
      Show
      Open a course page with a number of course modules present Ensure that all of the AJAX resource buttons still work right/left show/hide group mode toggle (on activities) delete drag/drop Ensure that the AJAX section buttons work as inticipated show/hide drag/drop Then: disable ajax support by turning off the 'enableajax' setting confirm that we're back to the non-ajax versions For bonus points: (re-enable the sitewide enableajax setting) open your current theme's config.php and add: $THEME->enablecourseajax = false; confirm that we're back to the non-ajax versions of all icons remove the theme config line open your current format's lib.php and find the callback_$name_ajax_support function and change the capable line: $ajaxsupport->capable = false; confirm that we're back to the non-ajax versions of all icons change things back to normal
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32873-master-1

      Description

      For MDL-31215 and others we sometimes need to check whether AJAX should be enabled so it would be beneficial to split this into a separate function.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Andrew Nicols added a comment -

            Patch supplied as part of MDL-31215

            Show
            Andrew Nicols added a comment - Patch supplied as part of MDL-31215
            Hide
            Dan Poltawski added a comment - - edited

            Well, now you have drawn attention to it..

            • Look into what user_is_editing() does. That $USER->editting check is redundant..
            • Could the front page course check be covered by the course_format_ajax_support callback? (as far as I know the front page doesn't have a course format).
            Show
            Dan Poltawski added a comment - - edited Well, now you have drawn attention to it.. Look into what user_is_editing() does. That $USER->editting check is redundant.. Could the front page course check be covered by the course_format_ajax_support callback? (as far as I know the front page doesn't have a course format).
            Hide
            Andrew Nicols added a comment -

            Thanks Dan,

            I've updated the function and rewritten it to make it easier to read. ISTR that you weren't keen on having long if clauses as this left the potential for errors and misreading.

            The existing code only tested the course format for non-site front pages already but I've made this more explicit and clearer
            I've also stopped checking $USER->editing – $PAGE->user_is_editing checks this
            I've also moved to get_site() instead of $SITE.

            Show
            Andrew Nicols added a comment - Thanks Dan, I've updated the function and rewritten it to make it easier to read. ISTR that you weren't keen on having long if clauses as this left the potential for errors and misreading. The existing code only tested the course format for non-site front pages already but I've made this more explicit and clearer I've also stopped checking $USER->editing – $PAGE->user_is_editing checks this I've also moved to get_site() instead of $SITE.
            Hide
            Andrew Nicols added a comment -

            On second thoughts, I've changed things to use $SITE->id instead. It's SITEID that's deprecated, not $SITE

            Andrew

            Show
            Andrew Nicols added a comment - On second thoughts, I've changed things to use $SITE->id instead. It's SITEID that's deprecated, not $SITE Andrew
            Hide
            Andrew Nicols added a comment -

            Is this better?

            Show
            Andrew Nicols added a comment - Is this better?
            Hide
            Dan Poltawski added a comment -

            Yep I like that

            Show
            Dan Poltawski added a comment - Yep I like that
            Hide
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) 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 guys,

            I've integrated this now, however I think we need to review this new function as well as include_course_ajax.
            The more I look at these two methods the less I like them.

            What gets to me about them is that both function is complete respect to the page that is about to be displayed and I think for sake of clarity the page should be passed into these two methods.
            At the moment things work based upon a course that is passed to these functions and the global page is acted upon.
            There things that eeks me about this the most is that is assumed the course being passed in is in relation to the course the page has been set up with.
            Now presently within Moodle this is likely to always be true. In fact presently is guaranteed to be true because include_course_ajax is called twice at definitive points close to the end of the script and course_ajax_enabled only once from within the previous function.
            However neither of these functions could be guaranteed to be accurate elsewhere, for include_course_ajax that's not likely a problem as its of very limited use. However course_ajax_enabled is more likely to be used elsewhere.
            I think to clarify this its best to pass a moodle_page object into the method rather than a course and use the course object that the page object is tracking.
            This way you can be sure that is it clear to developers the scope of these methods as well as ensure the accuracy of these methods.
            Again presently its not actually problem, it all comes down to future use. One foreseeable problem would be for instance if we started tracking the editing state for individual courses.
            Also just noting that it is always recommended to pass objects into API methods rather than rely on globals for this particular reason.

            Anyway, is in now.
            If others are in agreement with the above then I suggest we open a new issue to look into that.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi guys, I've integrated this now, however I think we need to review this new function as well as include_course_ajax. The more I look at these two methods the less I like them. What gets to me about them is that both function is complete respect to the page that is about to be displayed and I think for sake of clarity the page should be passed into these two methods. At the moment things work based upon a course that is passed to these functions and the global page is acted upon. There things that eeks me about this the most is that is assumed the course being passed in is in relation to the course the page has been set up with. Now presently within Moodle this is likely to always be true. In fact presently is guaranteed to be true because include_course_ajax is called twice at definitive points close to the end of the script and course_ajax_enabled only once from within the previous function. However neither of these functions could be guaranteed to be accurate elsewhere, for include_course_ajax that's not likely a problem as its of very limited use. However course_ajax_enabled is more likely to be used elsewhere. I think to clarify this its best to pass a moodle_page object into the method rather than a course and use the course object that the page object is tracking. This way you can be sure that is it clear to developers the scope of these methods as well as ensure the accuracy of these methods. Again presently its not actually problem, it all comes down to future use. One foreseeable problem would be for instance if we started tracking the editing state for individual courses. Also just noting that it is always recommended to pass objects into API methods rather than rely on globals for this particular reason. Anyway, is in now. If others are in agreement with the above then I suggest we open a new issue to look into that. Cheers Sam
            Hide
            Rossiani Wijaya added a comment -

            Hi Andrew,

            Could you provide testing instruction for this issue?

            Thanks.

            Show
            Rossiani Wijaya added a comment - Hi Andrew, Could you provide testing instruction for this issue? Thanks.
            Hide
            Andrew Nicols added a comment -

            Sorry - I meant to add these before.
            A

            Show
            Andrew Nicols added a comment - Sorry - I meant to add these before. A
            Hide
            Rossiani Wijaya added a comment -

            Moving workaround text to testing instructions

            Show
            Rossiani Wijaya added a comment - Moving workaround text to testing instructions
            Hide
            Rossiani Wijaya added a comment -

            Thanks Andrew for fixing and providing test instruction.

            This looks good.

            Test passed.

            Show
            Rossiani Wijaya added a comment - Thanks Andrew for fixing and providing test instruction. This looks good. Test passed.
            Hide
            Eloy Lafuente (stronk7) added a comment -

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

            Many thanks for the hard work, closing this as fixed.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - U P S T R E A M I Z E D ! Many thanks for the hard work, closing this as fixed. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: