Moodle
  1. Moodle
  2. MDL-38646

Mod folder - change column from show_expanded to showexpanded

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      48685

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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 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 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
          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
          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 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 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
          Mark Nelson added a comment -

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

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

          Branch has been rebased with lastest master.

          Show
          Rex Lorenzo added a comment - Branch has been rebased with lastest master.
          Hide
          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
          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
          Andrew Davis added a comment -

          Works as described. Passing.

          Show
          Andrew Davis added a comment - Works as described. Passing.
          Hide
          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
          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: