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
    • Rank:
      39940

      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.

        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: