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

          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