Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: 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

      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!

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              poltawski Dan Poltawski added a comment -

              Adding Andrew/Ruslan here.

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

              Also note that week titles are not being updated

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

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

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

              I will have a look today.

              Show
              kabalin Ruslan Kabalin added a comment - I will have a look today.
              Hide
              poltawski 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
              poltawski 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
              kabalin 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
              kabalin 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
              poltawski Dan Poltawski added a comment -

              Ruslan and I discussed this.

              Show
              poltawski Dan Poltawski added a comment - Ruslan and I discussed this.
              Hide
              kabalin 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
              kabalin 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
              kabalin Ruslan Kabalin added a comment -

              Ready for another review

              Show
              kabalin Ruslan Kabalin added a comment - Ready for another review
              Hide
              poltawski 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
              poltawski 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
              kabalin 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
              kabalin 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
              poltawski Dan Poltawski added a comment -

              True. Putting this on my list.

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

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

              Show
              poltawski Dan Poltawski added a comment - Please remember testing instructions, especially when your change is affecting more than the problem reported.
              Hide
              poltawski 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
              poltawski 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
              kabalin Ruslan Kabalin added a comment -

              Thanks Dan, instruction is added.

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

              Successful on master

              Show
              fred Frédéric Massart added a comment - Successful on master
              Hide
              poltawski 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
              poltawski 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
              dougiamas Martin Dougiamas added a comment -

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

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

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

              Show
              kabalin 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:
                    Fix Release Date:
                    25/Jun/12