Moodle
  1. Moodle
  2. MDL-6674

New AJAX in weekly format has a few serious but probably easy-to-fix bugs

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.7
    • Fix Version/s: 1.7, 1.8
    • Component/s: AJAX and JavaScript, Course
    • Labels:
      None
    • Affected Branches:
      MOODLE_17_STABLE
    • Fixed Branches:
      MOODLE_17_STABLE, MOODLE_18_STABLE
    • Rank:
      28256

      Description

      Hi, Ed

      Can you look at this urgently and help complete the AJAX implementation?

      Locations:

      course/rest.php - the old commands.php
      lib/ajax/ajaxcourse.js - the old JS from the topics format
      lib/ajax/section_classes.js - renamed back from php recently

      To try AJAX in the weekly course format:

      1) Set the course format to "weekly" in the settings.
      2) Turn on AJAX in the user profile preferences
      3) Admin > Server > Debugging .... set the level to DEBUG_DEVELOPER
      4) Go to the course and "Turn editing on".

      It nearly works but I think it's not detecting sections properly, because

      • moving activities (resources) will always end up in the top section, because beforeId is always 0.
      • moving blocks will send a bad block id and lose them

      Help!

        Issue Links

          Activity

          Hide
          added a comment -

          I apologize for the delay on this, I have had a string of exams over the last week. I will have a chance to look at it later this week.

          Show
          added a comment - I apologize for the delay on this, I have had a string of exams over the last week. I will have a chance to look at it later this week.
          Hide
          Martin Dougiamas added a comment -

          Great! Keep in touch, even if you think you can't solve this!

          Show
          Martin Dougiamas added a comment - Great! Keep in touch, even if you think you can't solve this!
          Hide
          Edward Coyne added a comment -

          Just an update, It seems that this problem isnt limited to just the weekly format it is now doing this in the topics format also. I am trying to find what has changed to cause this problem I will keep this thread updated.

          Show
          Edward Coyne added a comment - Just an update, It seems that this problem isnt limited to just the weekly format it is now doing this in the topics format also. I am trying to find what has changed to cause this problem I will keep this thread updated.
          Hide
          Edward Coyne added a comment -

          addendum: This is referring to the blocks problem I havent really had a chance to look into the resources issue. The problem with the blocks is that the block weight is getting miscalculated now so in the database there will be multiple blocks with the same weight and moodle will only read one of them. I am looking into what has changed to effect teh calculation of the block weights.

          Show
          Edward Coyne added a comment - addendum: This is referring to the blocks problem I havent really had a chance to look into the resources issue. The problem with the blocks is that the block weight is getting miscalculated now so in the database there will be multiple blocks with the same weight and moodle will only read one of them. I am looking into what has changed to effect teh calculation of the block weights.
          Hide
          Martin Dougiamas added a comment -

          Thanks. I don't think anything changed regarding the block implementation.

          Just so you know, we're hoping to get to Beta status in a few days.

          Show
          Martin Dougiamas added a comment - Thanks. I don't think anything changed regarding the block implementation. Just so you know, we're hoping to get to Beta status in a few days.
          Hide
          Martin Dougiamas added a comment -

          Any progress on this, Ed?

          Show
          Martin Dougiamas added a comment - Any progress on this, Ed?
          Hide
          Martin Dougiamas added a comment -

          I'm probably going to have to remove this functionality from Moodle if we can't fix it.

          Show
          Martin Dougiamas added a comment - I'm probably going to have to remove this functionality from Moodle if we can't fix it.
          Hide
          Edward Coyne added a comment -

          I have been working with it but I only really have time on the weekends. What is the timeline for going beta? I will be able to fix this issue but probably not until this weekend.

          Show
          Edward Coyne added a comment - I have been working with it but I only really have time on the weekends. What is the timeline for going beta? I will be able to fix this issue but probably not until this weekend.
          Hide
          Martin Dougiamas added a comment -

          The timeline for beta was two days ago, so we are holding on overtime ... please keep in touch ... it's hard to justify putting someone else on the AJAX learning curve if you say are working on it, but if I know you aren't working on it then I could have done that. At this point though, it's you or nothing :-P

          Show
          Martin Dougiamas added a comment - The timeline for beta was two days ago, so we are holding on overtime ... please keep in touch ... it's hard to justify putting someone else on the AJAX learning curve if you say are working on it, but if I know you aren't working on it then I could have done that. At this point though, it's you or nothing :-P
          Hide
          Edward Coyne added a comment -

          well I will see if I can work on it before this weekend, otherwise I will let you know how it goes this weekend.

          Show
          Edward Coyne added a comment - well I will see if I can work on it before this weekend, otherwise I will let you know how it goes this weekend.
          Hide
          Dan Poltawski added a comment -

          I've had a bash at fixing the issue. This seems to provide the correct ids for the previous and new blocks.. I've tested it (briefly) on IE and FF and it seems to work. But I am a not really a javascript man

          Show
          Dan Poltawski added a comment - I've had a bash at fixing the issue. This seems to provide the correct ids for the previous and new blocks.. I've tested it (briefly) on IE and FF and it seems to work. But I am a not really a javascript man
          Hide
          Dan Poltawski added a comment -

          Sorry, I meant to say the activities issue. Not looked at the blocks yet.

          Show
          Dan Poltawski added a comment - Sorry, I meant to say the activities issue. Not looked at the blocks yet.
          Hide
          Dan Poltawski added a comment -

          Hmm I notice that with or without my above patch there are times when Internet Explorer can become unresponsive when after moving activites around. Can't find specifics to replicate it yet though.

          Show
          Dan Poltawski added a comment - Hmm I notice that with or without my above patch there are times when Internet Explorer can become unresponsive when after moving activites around. Can't find specifics to replicate it yet though.
          Hide
          Vy-Shane Sin Fat added a comment -

          Thanks for the patch Dan. However, I think that beforeId should still be targetId, and not el.previousId. I've committed a fix based on your patch.

          Can you check whether moving activities is working for you now?

          Show
          Vy-Shane Sin Fat added a comment - Thanks for the patch Dan. However, I think that beforeId should still be targetId, and not el.previousId. I've committed a fix based on your patch. Can you check whether moving activities is working for you now?
          Hide
          Dan Poltawski added a comment -

          Hmm. Not quite sure how that all works, but it seems to be ok!

          For some reason I thought the before/after ids were mixed up. (Can anyone englighten me on how to view the debugging info?)

          Show
          Dan Poltawski added a comment - Hmm. Not quite sure how that all works, but it seems to be ok! For some reason I thought the before/after ids were mixed up. (Can anyone englighten me on how to view the debugging info?)
          Hide
          Edward Coyne added a comment -

          The easiest way to view the debugging info is if you are using firefox and the 'firebug' javascript console extension it will output it to there (this is YUI default). Otherwise you can add this line:
          "<script>var myLogReader = new YAHOO.widget.LogReader();<script>" and it should create a loging pane on the bottom of the window. I thought I had a commented line in the format.php about it but I think I may have taken it out.
          If anyone can suggest a better way to alow access to the information for non-firebug users than let me know.

          Show
          Edward Coyne added a comment - The easiest way to view the debugging info is if you are using firefox and the 'firebug' javascript console extension it will output it to there (this is YUI default). Otherwise you can add this line: "<script>var myLogReader = new YAHOO.widget.LogReader();<script>" and it should create a loging pane on the bottom of the window. I thought I had a commented line in the format.php about it but I think I may have taken it out. If anyone can suggest a better way to alow access to the information for non-firebug users than let me know.
          Hide
          Dan Poltawski added a comment -

          I've been trying to recreate the block bug - I think it is related to putting a block in that is bigger than another block (and hence touching multiple block boundaries) - if that makes sense.

          Show
          Dan Poltawski added a comment - I've been trying to recreate the block bug - I think it is related to putting a block in that is bigger than another block (and hence touching multiple block boundaries) - if that makes sense.
          Hide
          Edward Coyne added a comment -

          I have made the changes to fix the block repositioning.

          For future reference the problem was:
          When the blocks were being repositioned the way it worked was that each block would send its new position to the backend, this position would then be recorded. The problem came up when the rest.php script was changed to require a block_instance object be created before the script could continue. This was problematic because during the moving process there would be times when two blocks would have the same weight temporarily then one would change to its final weight, but because the blocks_get_by_page() function stores the blocks in an array by weight it would only see one of the two blocks with that weight rendering the second block inaccessible by rest.php script and this would cause the changes to not be made, thus we would end up with two blocks with the same weight permanently.

          The solution:
          I have rewritten the javascript and the rest.php to do two separate atomic moves of the blocks. one happens in the javascript and another happens on the serverside in the rest.php using blocks_execute_repositioning_atomic() (a extended version of blocks_execute_repositioning). There is one small bug I am still working out of this system then I will commit the changes.

          Show
          Edward Coyne added a comment - I have made the changes to fix the block repositioning. For future reference the problem was: When the blocks were being repositioned the way it worked was that each block would send its new position to the backend, this position would then be recorded. The problem came up when the rest.php script was changed to require a block_instance object be created before the script could continue. This was problematic because during the moving process there would be times when two blocks would have the same weight temporarily then one would change to its final weight, but because the blocks_get_by_page() function stores the blocks in an array by weight it would only see one of the two blocks with that weight rendering the second block inaccessible by rest.php script and this would cause the changes to not be made, thus we would end up with two blocks with the same weight permanently. The solution: I have rewritten the javascript and the rest.php to do two separate atomic moves of the blocks. one happens in the javascript and another happens on the serverside in the rest.php using blocks_execute_repositioning_atomic() (a extended version of blocks_execute_repositioning). There is one small bug I am still working out of this system then I will commit the changes.
          Hide
          Edward Coyne added a comment -

          With the patch that I commented on earlier and Dan and Vy-Shane's work on the activities this issue looks to be resolved.

          Show
          Edward Coyne added a comment - With the patch that I commented on earlier and Dan and Vy-Shane's work on the activities this issue looks to be resolved.
          Hide
          Dan Poltawski added a comment -

          Ed> this seems to now stop blocks from having their (weighting) being rearranged?

          Show
          Dan Poltawski added a comment - Ed> this seems to now stop blocks from having their (weighting) being rearranged?
          Hide
          Edward Coyne added a comment -

          It stops the problem that we were having earlier where multiple blocks would have the same weight and position which would cause them to disapear when you reloaded the page.

          Show
          Edward Coyne added a comment - It stops the problem that we were having earlier where multiple blocks would have the same weight and position which would cause them to disapear when you reloaded the page.
          Hide
          Martin Dougiamas added a comment -

          Thanks heaps, Ed. We'll test this today.

          Show
          Martin Dougiamas added a comment - Thanks heaps, Ed. We'll test this today.
          Hide
          Vy-Shane Sin Fat added a comment -

          Ed, the block positions don't seem to get updated in the database when I move blocks around within the same column.

          Moving a block from one column to another always results in the block being added as the topmost block in the destination column.

          Show
          Vy-Shane Sin Fat added a comment - Ed, the block positions don't seem to get updated in the database when I move blocks around within the same column. Moving a block from one column to another always results in the block being added as the topmost block in the destination column.
          Hide
          Edward Coyne added a comment -

          Hmm odd I will look into that tonight (EST).

          Show
          Edward Coyne added a comment - Hmm odd I will look into that tonight (EST).
          Hide
          Edward Coyne added a comment -

          I cannot reproduce your problem with moving blocks from one column to another it always inserts them properly for me.

          I do see the problem with them moving in the same column though and I am looking into how to resolve it now.

          Show
          Edward Coyne added a comment - I cannot reproduce your problem with moving blocks from one column to another it always inserts them properly for me. I do see the problem with them moving in the same column though and I am looking into how to resolve it now.
          Hide
          Vy-Shane Sin Fat added a comment -

          Scratch that, I can't reproduce the problem with moving blocks to a different column anymore.

          Show
          Vy-Shane Sin Fat added a comment - Scratch that, I can't reproduce the problem with moving blocks to a different column anymore.
          Hide
          Edward Coyne added a comment -

          I have committed a change to address the problem with moving the blocks in the same column.

          For reference:
          The problem was caused by a clause in the blocks_execute_repositioning() that returned from the function if the target position(column) and the original position(column) were the same. Why I didnt notice this at first is beyond me.

          I also addressed another issue that had not come up because it was being blocked by the first problem. When the list of weights was being recacluated for the column the target weight should have been changed with it to match the new list. This was not happening.

          Show
          Edward Coyne added a comment - I have committed a change to address the problem with moving the blocks in the same column. For reference: The problem was caused by a clause in the blocks_execute_repositioning() that returned from the function if the target position(column) and the original position(column) were the same. Why I didnt notice this at first is beyond me. I also addressed another issue that had not come up because it was being blocked by the first problem. When the list of weights was being recacluated for the column the target weight should have been changed with it to match the new list. This was not happening.
          Hide
          Vy-Shane Sin Fat added a comment -

          Thanks Ed, it's looking good. Marking as fixed. Can someone else verify as well and close the issue?

          Show
          Vy-Shane Sin Fat added a comment - Thanks Ed, it's looking good. Marking as fixed. Can someone else verify as well and close the issue?
          Hide
          Nicolas Martignoni added a comment -

          I'm afraid I can reproduce the "ressource" faulty behaviour, at least partially.

          If a resource is present in a section before I turn editing mode on, when I move it in another empty section, it is automatically and wrongly placed on the top of the section, above the edit icon. But when I take i again (immediately, without turning off and on editing mode) and move it to its original section, the move works correctly.

          Tested on 1.7 beta.

          Show
          Nicolas Martignoni added a comment - I'm afraid I can reproduce the "ressource" faulty behaviour, at least partially. If a resource is present in a section before I turn editing mode on, when I move it in another empty section, it is automatically and wrongly placed on the top of the section, above the edit icon. But when I take i again (immediately, without turning off and on editing mode) and move it to its original section, the move works correctly. Tested on 1.7 beta.
          Hide
          Vy-Shane Sin Fat added a comment -

          Thanks. Should be fixed now. We're getting there

          Show
          Vy-Shane Sin Fat added a comment - Thanks. Should be fixed now. We're getting there
          Hide
          Nicolas Martignoni added a comment -

          Yes, it works now on 1.7beta. Closing. Many thanks.

          Show
          Nicolas Martignoni added a comment - Yes, it works now on 1.7beta. Closing. Many thanks.
          Hide
          Dennis Daniels added a comment -

          Look at the screen grab...

          Show
          Dennis Daniels added a comment - Look at the screen grab...
          Hide
          Dennis Daniels added a comment -

          Look at the screen grab...

          Show
          Dennis Daniels added a comment - Look at the screen grab...
          Hide
          Dennis Daniels added a comment -

          Look at the screen grab...

          Show
          Dennis Daniels added a comment - Look at the screen grab...

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: