Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: AJAX and JavaScript
    • Labels:
    • Testing Instructions:
      Hide
      1. Enable editing on the course page and make sure that course format is topics.
      2. Make sure that some sections have custom title and some default one.
      3. Drag any section over some sections with custom and default titles.
      4. Make sure that the order of titles is correct once you dropped the section.
      5. Change the course format to weekly and repeat steps 2-4.
      Show
      Enable editing on the course page and make sure that course format is topics. Make sure that some sections have custom title and some default one. Drag any section over some sections with custom and default titles. Make sure that the order of titles is correct once you dropped the section. Change the course format to weekly and repeat steps 2-4.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32824-master-2
    • Rank:
      39851

      Description

      This is partly my fault for loosing some markup I think, but we need to think about the case of the default topic title too.

      1/ Switch on paged section mode
      2/ Turn on editing on front page
      3/ Drag and drop sections

      Problems:

      • If the default title is there (e.g. week dates/'Topic 1') the titles do not get switched
      • The links don't get changed (it needs updating with section number)

      This is problematic, because if someone defines a section name the section name needs to stay with the text, but the link needs to be changed.

      I could probably define some markup for displaying the default title, and different markup for the user created title, then you could work out if the default title was being displayed or a new one. Thoughts required!

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Adding Andrew/Ruslan here.

          Show
          Dan Poltawski added a comment - Adding Andrew/Ruslan here.
          Hide
          Dan Poltawski added a comment -

          Also note that week titles are not being updated

          Show
          Dan Poltawski added a comment - Also note that week titles are not being updated
          Hide
          Dan Poltawski added a comment -

          Ruslan: ping. Are you able to work on this?

          Show
          Dan Poltawski added a comment - Ruslan: ping. Are you able to work on this?
          Hide
          Ruslan Kabalin added a comment -

          I will have a look today.

          Show
          Ruslan Kabalin added a comment - I will have a look today.
          Hide
          Dan Poltawski added a comment -

          Note - there is a related issue in: MDL-33146

          feel free to change the html generation for this. I think we need diffent classes depending on the title which is being shown.

          Show
          Dan Poltawski added a comment - Note - there is a related issue in: MDL-33146 feel free to change the html generation for this. I think we need diffent classes depending on the title which is being shown.
          Hide
          Ruslan Kabalin added a comment -

          It is pretty complicated to implement logic with JS swaps/modifications only, as there might be a mix of custom and default titles in the sections affected by single section move. The suggested fix makes background script return the new list of section titles with links if applicable, so that in JS the affected section titles will simply be updated. The fix is also re-factoring per format format.js and removes unnecessary left area swapping.

          Show
          Ruslan Kabalin added a comment - It is pretty complicated to implement logic with JS swaps/modifications only, as there might be a mix of custom and default titles in the sections affected by single section move. The suggested fix makes background script return the new list of section titles with links if applicable, so that in JS the affected section titles will simply be updated. The fix is also re-factoring per format format.js and removes unnecessary left area swapping.
          Hide
          Dan Poltawski added a comment -

          Ruslan and I discussed this.

          Show
          Dan Poltawski added a comment - Ruslan and I discussed this.
          Hide
          Ruslan Kabalin added a comment -

          Thanks to Dan's suggestions, I went further and made the file more format specific. I introduce a new callback function for ajax move request defined in format lib.php that returns the data if required, then we call function in format.js to process that data accordingly.

          Show
          Ruslan Kabalin added a comment - Thanks to Dan's suggestions, I went further and made the file more format specific. I introduce a new callback function for ajax move request defined in format lib.php that returns the data if required, then we call function in format.js to process that data accordingly.
          Hide
          Ruslan Kabalin added a comment -

          Ready for another review

          Show
          Ruslan Kabalin added a comment - Ready for another review
          Hide
          Dan Poltawski added a comment -

          Hi Ruslan,

          Really sorry but we've been looking at more related issues here and now have come to the conclusion that we need to add a new sectionid parameter to get to a section no matter where it is in the course structure. Thus meaning you would not need to update your link. I've linked this here.

          Show
          Dan Poltawski added a comment - Hi Ruslan, Really sorry but we've been looking at more related issues here and now have come to the conclusion that we need to add a new sectionid parameter to get to a section no matter where it is in the course structure. Thus meaning you would not need to update your link. I've linked this here.
          Hide
          Ruslan Kabalin added a comment -

          That does not change the implementation I guess, as we still need to move titles, which can be custom or default. I would integrate this one and then change the link in section_title() the way you need.

          Show
          Ruslan Kabalin added a comment - That does not change the implementation I guess, as we still need to move titles, which can be custom or default. I would integrate this one and then change the link in section_title() the way you need.
          Hide
          Dan Poltawski added a comment -

          True. Putting this on my list.

          Show
          Dan Poltawski added a comment - True. Putting this on my list.
          Hide
          Dan Poltawski added a comment -

          Please remember testing instructions, especially when your change is affecting more than the problem reported.

          Show
          Dan Poltawski added a comment - Please remember testing instructions, especially when your change is affecting more than the problem reported.
          Hide
          Dan Poltawski added a comment -

          Thanks Ruslan. I've integrated this now. If you can expand the testing instructions beyond my issue description that'd be great.

          FWIW I don't like that we need to do this, it doesn't feel right to me, but well thats just my silly feel of the issue and it fixes it. Thanks a lot for your work on it

          Show
          Dan Poltawski added a comment - Thanks Ruslan. I've integrated this now. If you can expand the testing instructions beyond my issue description that'd be great. FWIW I don't like that we need to do this, it doesn't feel right to me, but well thats just my silly feel of the issue and it fixes it. Thanks a lot for your work on it
          Hide
          Ruslan Kabalin added a comment -

          Thanks Dan, instruction is added.

          Show
          Ruslan Kabalin added a comment - Thanks Dan, instruction is added.
          Hide
          Frédéric Massart added a comment -

          Successful on master

          Show
          Frédéric Massart added a comment - Successful on master
          Hide
          Dan Poltawski added a comment -

          Congratulations!

          Your work has made into the latest Moodle release!

          You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!

          Show
          Dan Poltawski added a comment - Congratulations! Your work has made into the latest Moodle release! You are only authorised to celebrate after testing 15 Moodle 2.3 QA tests, thanks!
          Hide
          Martin Dougiamas added a comment -

          Apparently there is a regression in topics formats right now with random-looking results after drag/drop

          Show
          Martin Dougiamas added a comment - Apparently there is a regression in topics formats right now with random-looking results after drag/drop
          Hide
          Ruslan Kabalin added a comment -

          Works fine for me with the most recent master... Are you still able to see the problem, Martin?

          Show
          Ruslan Kabalin added a comment - Works fine for me with the most recent master... Are you still able to see the problem, Martin?

            People

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

              Dates

              • Created:
                Updated:
                Resolved: