Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-38646

Mod folder - change column from show_expanded to showexpanded

    Details

    • Type: Task
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Resource
    • Labels:
    • Testing Instructions:
      Hide

      Please follow existing test instructions in MDL-30790.

      Also, need to test backup/resource procedure with the showexpanded config setting set/unset.

      Note that having two folder resources show inline on a course page will not work until MDL-38634 is integrated.

      Show
      Please follow existing test instructions in MDL-30790 . Also, need to test backup/resource procedure with the showexpanded config setting set/unset. Note that having two folder resources show inline on a course page will not work until MDL-38634 is integrated.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:

      Description

      This is a followup task from a new feature that was added to mod_folder (MDL-30790). The column name for the new setting needs renaming from show_expanded to showexpanded.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Note I've added 3 blockers to this. Ideally this should be done after them (to search & modify all uses together).

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Note I've added 3 blockers to this. Ideally this should be done after them (to search & modify all uses together). Ciao
            Hide
            salvetore Michael de Raadt added a comment -

            Hi, Rex.

            I've added you as a watcher as you were the assignee of the original issue.

            If you would like to work on this issue, please assign it to yourself.

            Show
            salvetore Michael de Raadt added a comment - Hi, Rex. I've added you as a watcher as you were the assignee of the original issue. If you would like to work on this issue, please assign it to yourself.
            Hide
            rex Rex Lorenzo added a comment -

            Patch submitted. Did refactoring to rename all instances of folder's show_expanded to showexpanded. Also added upgrade code to rename database field and site config setting.

            One concern that I am unsure of how to handle is what to do with backup files that have the old show_expanded value. Right now, I believe, the code will just use the site config setting, if it doesn't find the showexpanded value in the backup file.

            Since the code with the old show_expanded value was in the ALPHA/BETA status, do we not need to worry about it?

            Show
            rex Rex Lorenzo added a comment - Patch submitted. Did refactoring to rename all instances of folder's show_expanded to showexpanded. Also added upgrade code to rename database field and site config setting. One concern that I am unsure of how to handle is what to do with backup files that have the old show_expanded value. Right now, I believe, the code will just use the site config setting, if it doesn't find the showexpanded value in the backup file. Since the code with the old show_expanded value was in the ALPHA/BETA status, do we not need to worry about it?
            Hide
            markn Mark Nelson added a comment -

            Hi Rex, patch looks good. A few points.

            1. Just a note to integrators (if they were unaware like I was): I was going to ask why use the "set_config('show_expanded', null, 'folder');" rather than delete it from the table, but I see this function does that already if the value is null.
            2. When performing the rename of the field it is required to perform a check to ensure that the field exists before doing the rename. This is because upgrades can be performed multiple times (in this case I can't see that happening but it is for consistency). Imagine including an upgrade for 2.3 and 2.4, each commit would have a different version number in the if statement (eg. "if ($oldversion < #versionnumber)"), meaning if someone was to upgrade from 2.3, to 2.4 this step would be executed again.
            3. In regards to your question regarding backups that have the old value - I do not believe it is necessary to worry about this. Like you said, this was only introduced in the early stages of 2.5, before the official release. If they do happen to have a backup from the beta they wish to use (highly unlikely), it will still set the value to what is currently specified in the config. You could perform a check to see if $data->show_expanded existed, and if it did, then set showexpanded to this value. However, this is overkill and I feel completely unnecessary.
            Show
            markn Mark Nelson added a comment - Hi Rex, patch looks good. A few points. Just a note to integrators (if they were unaware like I was): I was going to ask why use the "set_config('show_expanded', null, 'folder');" rather than delete it from the table, but I see this function does that already if the value is null. When performing the rename of the field it is required to perform a check to ensure that the field exists before doing the rename. This is because upgrades can be performed multiple times (in this case I can't see that happening but it is for consistency). Imagine including an upgrade for 2.3 and 2.4, each commit would have a different version number in the if statement (eg. "if ($oldversion < #versionnumber)"), meaning if someone was to upgrade from 2.3, to 2.4 this step would be executed again. In regards to your question regarding backups that have the old value - I do not believe it is necessary to worry about this. Like you said, this was only introduced in the early stages of 2.5, before the official release. If they do happen to have a backup from the beta they wish to use (highly unlikely), it will still set the value to what is currently specified in the config. You could perform a check to see if $data->show_expanded existed, and if it did, then set showexpanded to this value. However, this is overkill and I feel completely unnecessary.
            Hide
            rex Rex Lorenzo added a comment -

            I made the change as requested by the peer review to check for the field first before renaming. But I should note that I was just using the code provided by the XMLDB editor. The XMLDB editor code generation for the field renaming should be modified to add in this check for future developers.

            Show
            rex Rex Lorenzo added a comment - I made the change as requested by the peer review to check for the field first before renaming. But I should note that I was just using the code provided by the XMLDB editor. The XMLDB editor code generation for the field renaming should be modified to add in this check for future developers.
            Hide
            markn Mark Nelson added a comment -

            Hi Rex, thanks for pointing that out! I will create another issue for that.

            Show
            markn Mark Nelson added a comment - Hi Rex, thanks for pointing that out! I will create another issue for that.
            Hide
            poltawski Dan Poltawski 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
            poltawski Dan Poltawski 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
            rex Rex Lorenzo added a comment -

            Branch has been rebased with lastest master.

            Show
            rex Rex Lorenzo added a comment - Branch has been rebased with lastest master.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Rex, this has been integrated now.

            Just noting I did make one additional commit.
            I've renamed the field in the original upgrade statement. The reason being that this field was originally added in master and we're changing it only 1 month later, and still before the next release.
            By renaming in the original upgrade block as well only developers who have upgraded Moodle in the past month will need to perform the field rename.
            Not essential as the field would have been created and then renamed before being used however a nicety I think none the less.

            Many thanks
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Rex, this has been integrated now. Just noting I did make one additional commit. I've renamed the field in the original upgrade statement. The reason being that this field was originally added in master and we're changing it only 1 month later, and still before the next release. By renaming in the original upgrade block as well only developers who have upgraded Moodle in the past month will need to perform the field rename. Not essential as the field would have been created and then renamed before being used however a nicety I think none the less. Many thanks Sam
            Hide
            andyjdavis Andrew Davis added a comment -

            Works as described. Passing.

            Show
            andyjdavis Andrew Davis added a comment - Works as described. Passing.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            I feel myself really alone tonight! So was time to push your fixes upstream!

            "Lest we forget. We will remember them."

            Thanks and ciao!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13