Moodle
  1. Moodle
  2. MDL-40874

Retire MyMobile as a standard theme

    Details

    • Testing Instructions:
      Hide

      Test 1

      1. Install a site without the patch.
      2. Add some custom css in the MyMobile theme setting page.
      3. Set a course theme, user theme and category theme to MyMobile (you need to go in Appareance > Theme settings first to enable the three settings. Then you'll find each settings in in the course edit page, user profile and add course page.)
      4. In theme selector, set MyMobile to everything (legacy, mobile...).
      5. Reapply the patch.
      6. Upgrade
      7. Check that in the config_plugins and config_log tables there are no records with "plugin" field set to theme_mymobile.
      8. Check that the course, user, and category theme fallback to Clean.
      9. Check that the front page fallbacks to Clean on desktop and mobile.

      Test 2 (keeping MyMobile)

      1. Install a site without the patch.
      2. Add some custom css in the MyMobile theme setting page.
      3. In theme selector, set MyMobile as default.
      4. Apply the patch
      5. Download the mymobile them from Moodle.org https://moodle.org/plugins/pluginversion.php?id=4563 and copy back the file (alternatively read the theme/upgrade.txt, it must be clear what you are mean to do)
      6. Upgrade
      7. Check that everything is still set to MyMobile and that MyMobile settings still exists.
      Show
      Test 1 Install a site without the patch. Add some custom css in the MyMobile theme setting page. Set a course theme, user theme and category theme to MyMobile (you need to go in Appareance > Theme settings first to enable the three settings. Then you'll find each settings in in the course edit page, user profile and add course page.) In theme selector, set MyMobile to everything (legacy, mobile...). Reapply the patch. Upgrade Check that in the config_plugins and config_log tables there are no records with "plugin" field set to theme_mymobile. Check that the course, user, and category theme fallback to Clean. Check that the front page fallbacks to Clean on desktop and mobile. Test 2 (keeping MyMobile) Install a site without the patch. Add some custom css in the MyMobile theme setting page. In theme selector, set MyMobile as default. Apply the patch Download the mymobile them from Moodle.org https://moodle.org/plugins/pluginversion.php?id=4563 and copy back the file (alternatively read the theme/upgrade.txt, it must be clear what you are mean to do) Upgrade Check that everything is still set to MyMobile and that MyMobile settings still exists.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull Master Branch:
      MDL-40874-master-10
    • Story Points:
      20
    • Rank:
      115
    • Sprint:
      FRONTEND Sprint 6

      Description

      I'm not sure what the usage of the MyMobile theme is. It filled a need when we did not have any themes suitable for mobiles. However, the MyMobile theme is buggy and inconsistent with other Moodle standard themes. Some work has been done recently by Gareth J Barnard and Mary Evans.

      Now that we have responsive themes based on Bootstrap, I think we should consider retiring MyMobile.

      1. Selection_003.png
        38 kB
      2. warning_about_mymobile_being_removed.png
        109 kB

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Mary and Gareth: I've added you as watchers on this issue as I'd value your opinion.

          Show
          Michael de Raadt added a comment - Mary and Gareth: I've added you as watchers on this issue as I'd value your opinion.
          Hide
          Martin Dougiamas added a comment -

          +1 to do this in 2.6, but it needs an upgrade step to fix sites that may be upgrading from older versions.

          Show
          Martin Dougiamas added a comment - +1 to do this in 2.6, but it needs an upgrade step to fix sites that may be upgrading from older versions.
          Hide
          Mary Evans added a comment - - edited

          Hi Michael,

          Thank you for adding Gareth and myself to this tracker issue. Gareth and I are well aware that this was on the cards , so in that respect it is not a shock. A few months ago Gareth was willing to fix MyMobile and was making good progress, until a chance conversation with Martin in dev chat. when it was mooted that MyMobile was likely to be depreciated by 2.6.

          I would like to say, however, that it does look nicer than Bootstrap, and the fact that the technology around this kind of theme is still evolving, and knowing the move to convert jQuery to jsRosettaStone there is still a possibility that a new mobile theme could evolve from the MyMobile theme. But that would be a Plugin Add-on, so I think it is OK to drop MyMobile from Moodle.

          There is little or no interest concerning the MyMobile theme on the Themes' forum over the last few months, so my thinking is that either people are happy with what is available, or they all use iPads which displays a standard Moodle theme very well because of the high resolution these devices use.

          I think the biggest challenge coming up is the touch screen which is more difficult to accommodate in Moodle, but I think generally we are making headway especially with larger editing icons, and the new addition of fonts in themes which will in turn lead to better and more varied usage of icon fonts for touch screens in the future.

          Show
          Mary Evans added a comment - - edited Hi Michael, Thank you for adding Gareth and myself to this tracker issue. Gareth and I are well aware that this was on the cards , so in that respect it is not a shock. A few months ago Gareth was willing to fix MyMobile and was making good progress, until a chance conversation with Martin in dev chat. when it was mooted that MyMobile was likely to be depreciated by 2.6. I would like to say, however, that it does look nicer than Bootstrap, and the fact that the technology around this kind of theme is still evolving, and knowing the move to convert jQuery to jsRosettaStone there is still a possibility that a new mobile theme could evolve from the MyMobile theme. But that would be a Plugin Add-on, so I think it is OK to drop MyMobile from Moodle. There is little or no interest concerning the MyMobile theme on the Themes' forum over the last few months, so my thinking is that either people are happy with what is available, or they all use iPads which displays a standard Moodle theme very well because of the high resolution these devices use. I think the biggest challenge coming up is the touch screen which is more difficult to accommodate in Moodle, but I think generally we are making headway especially with larger editing icons, and the new addition of fonts in themes which will in turn lead to better and more varied usage of icon fonts for touch screens in the future.
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Michael de Raadt,

          First of all, thank you for inviting me to express my opinion on this topic.

          Before MyMobile is retired I think that all outstanding issues with Bootstrapbase / Clean should be resolved and any perceived usability problems dealt with.

          My current understanding is that New School Learning has a large client base for their version and I have a version with all the fixes I could make (until the time of the conversation with Martin) and the ability to easily use the jQueryMobile swatch tool to create different colour schemes which could be made available to the community as an optional add-on upon MyMobile's demise. If this would help with the transition as 'it's removed from core but there is this add-on for free'.

          As MyMobile depends on both jQuery and jQueryMobile it is not a viable candidate for jsRosettaStone as it would be a substantial amount of work to convert and then maintain with their communities developments. It would be a perpetual reinvention of the wheel.

          At a higher level of abstraction the case that this theme and Bootstrap have highlighted is the need to support devices in an increasing mobile world. Who knows what new technologies / developments will come along to support 'touch' capability or even verbal control. It might be the case that jQueryMobile really takes off, adds lots of new features and this is worth putting back. It's a matter of rapidly adapting. Therefore at the current moment in time it is worth removing just to save HQ time in support to facilitate effort on the renderers / layouts front and perhaps having a back-end UI like Wordpress.

          So +1 for removal at this time. Also close / defer all outstanding MyMobile issues to reduce the 7300'ish current open issues . But not to loose sight of what it attempts to solve or the need to make Bootstrapbase / Clean a better than viable replacement.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Michael de Raadt , First of all, thank you for inviting me to express my opinion on this topic. Before MyMobile is retired I think that all outstanding issues with Bootstrapbase / Clean should be resolved and any perceived usability problems dealt with. My current understanding is that New School Learning has a large client base for their version and I have a version with all the fixes I could make (until the time of the conversation with Martin) and the ability to easily use the jQueryMobile swatch tool to create different colour schemes which could be made available to the community as an optional add-on upon MyMobile's demise. If this would help with the transition as 'it's removed from core but there is this add-on for free'. As MyMobile depends on both jQuery and jQueryMobile it is not a viable candidate for jsRosettaStone as it would be a substantial amount of work to convert and then maintain with their communities developments. It would be a perpetual reinvention of the wheel. At a higher level of abstraction the case that this theme and Bootstrap have highlighted is the need to support devices in an increasing mobile world. Who knows what new technologies / developments will come along to support 'touch' capability or even verbal control. It might be the case that jQueryMobile really takes off, adds lots of new features and this is worth putting back. It's a matter of rapidly adapting. Therefore at the current moment in time it is worth removing just to save HQ time in support to facilitate effort on the renderers / layouts front and perhaps having a back-end UI like Wordpress. So +1 for removal at this time. Also close / defer all outstanding MyMobile issues to reduce the 7300'ish current open issues . But not to loose sight of what it attempts to solve or the need to make Bootstrapbase / Clean a better than viable replacement. Cheers, Gareth
          Hide
          Michael de Raadt added a comment -

          Thanks for your feedback, guys. Certainly getting the Bootstrap-based themes working well is a current priority for us.

          Show
          Michael de Raadt added a comment - Thanks for your feedback, guys. Certainly getting the Bootstrap-based themes working well is a current priority for us.
          Hide
          Dan Poltawski added a comment -

          I disagree that mymobile is any better than bootstrap especially as when it breaks, it breaks much spectacularly and you often can't do what you are trying to achieve.

          +1 to remove it (but I didn't like it in the first place )

          Show
          Dan Poltawski added a comment - I disagree that mymobile is any better than bootstrap especially as when it breaks, it breaks much spectacularly and you often can't do what you are trying to achieve. +1 to remove it (but I didn't like it in the first place )
          Hide
          Mary Evans added a comment -

          @DanP, I was meaning 'aesthetically' in that it looks prettier, pity it does not work too well.

          Anyway, whilst I add my +1 to retire MyMobile as a standard theme, I would like to thank John Stabinger for working on the MyMobile theme originally, at a time when Moodle was not that well geared up to work on Mobiles. It was not an easy task, and one filled with problems from the start. However, what he achieved, in the face of adversity, was good in terms of what themes can do.

          Themes, can make things look pretty, but if the structure and workings of a system is flawed then no amount of padding, frills and pretty colours can fix it. Things need fixing from the inside out. So in that respect Moodle has still a long way to go to achieve what people need when it comes to viewing Moodle on a Mobile device.

          So Thank you John Stabinger for all your hard work and dedication with this theme in the early days. I know it did not work out as you had hoped, but you tried, and that is the main thing.

          Cheers
          Mary

          Show
          Mary Evans added a comment - @DanP, I was meaning 'aesthetically' in that it looks prettier, pity it does not work too well. Anyway, whilst I add my +1 to retire MyMobile as a standard theme, I would like to thank John Stabinger for working on the MyMobile theme originally, at a time when Moodle was not that well geared up to work on Mobiles. It was not an easy task, and one filled with problems from the start. However, what he achieved, in the face of adversity, was good in terms of what themes can do. Themes, can make things look pretty, but if the structure and workings of a system is flawed then no amount of padding, frills and pretty colours can fix it. Things need fixing from the inside out. So in that respect Moodle has still a long way to go to achieve what people need when it comes to viewing Moodle on a Mobile device. So Thank you John Stabinger for all your hard work and dedication with this theme in the early days. I know it did not work out as you had hoped, but you tried, and that is the main thing. Cheers Mary
          Hide
          Dan Poltawski added a comment -

          > So Thank you John Stabinger for all your hard work and dedication with this theme in the early days.

          +100

          Show
          Dan Poltawski added a comment - > So Thank you John Stabinger for all your hard work and dedication with this theme in the early days. +100
          Hide
          Martin Dougiamas added a comment -

          Agreed, thanks John.

          And +100 to remove this in 2.6.

          Which means we need some upgrade code here to make sure no-one sees any errors.

          I guess the logic is something like:

          • If they have a MyMobile selected as any theme anywhere (various site themes on devices, course themes, user themes):
          • * If Clean is installed then replace with Clean, else
          • * Make it blank

          We also need to print a message to admins (and perhaps email them) to let them know that this was done, in case they need to check anything.

          Show
          Martin Dougiamas added a comment - Agreed, thanks John. And +100 to remove this in 2.6. Which means we need some upgrade code here to make sure no-one sees any errors. I guess the logic is something like: If they have a MyMobile selected as any theme anywhere (various site themes on devices, course themes, user themes): * If Clean is installed then replace with Clean, else * Make it blank We also need to print a message to admins (and perhaps email them) to let them know that this was done, in case they need to check anything.
          Hide
          Martin Dougiamas added a comment -

          One issue could be if someone has made a theme inheriting from this one.

          Solution from Sam H: Update our inheritance code so that if a parent theme is missing then failover nicely to Standard.

          Show
          Martin Dougiamas added a comment - One issue could be if someone has made a theme inheriting from this one. Solution from Sam H: Update our inheritance code so that if a parent theme is missing then failover nicely to Standard.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          When someone created a theme requiring theme_mymobile (in version.php, not the inheritance in config.php):
          a) we display the attached screenshot. Then it's the job of the administrator to "git revert", properly uninstall the extending theme.
          b) during upgrade, we check each extending themes, edit their version.php on the fly (theme_mymobile => theme_standard) so the upgrade continue.
          c) in the upgrade core function, we do a specific hack to ignore the dependency error for theme_mymobile.

          In a) it's quite a pain for the user but I guess if the user created a mymobile extending theme then it is managed by an experienced user. However if the user used a theme he didn't create, the user may not know what to do.
          In b) users could complain that Moodle edited their theme php files. We can imagine someone copying the theme in another site (older version) and the theme is not working as we edited the php file and it would be now extending from standard.
          In c) it's hacky.

          I would go for b), I would just want your opinion.

          Show
          Jérôme Mouneyrac added a comment - - edited When someone created a theme requiring theme_mymobile (in version.php, not the inheritance in config.php): a) we display the attached screenshot. Then it's the job of the administrator to "git revert", properly uninstall the extending theme. b) during upgrade, we check each extending themes, edit their version.php on the fly (theme_mymobile => theme_standard) so the upgrade continue. c) in the upgrade core function, we do a specific hack to ignore the dependency error for theme_mymobile. In a) it's quite a pain for the user but I guess if the user created a mymobile extending theme then it is managed by an experienced user. However if the user used a theme he didn't create, the user may not know what to do. In b) users could complain that Moodle edited their theme php files. We can imagine someone copying the theme in another site (older version) and the theme is not working as we edited the php file and it would be now extending from standard. In c) it's hacky. I would go for b), I would just want your opinion.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Note that when a user will try to install a mymobile extending theme once this issue is fixed, then the use case will be a).

          Show
          Jérôme Mouneyrac added a comment - - edited Note that when a user will try to install a mymobile extending theme once this issue is fixed, then the use case will be a).
          Hide
          Jérôme Mouneyrac added a comment - - edited

          I though the solution b) (automatically editing the theme php file) was the best because:

          • I think it's the best for people managing no-critical sites.
          • Administrators who manage critical sites most likely read the release notes. So they'll know that their php files have been outrageously edited by Moodle.

          However I hesitated with a) (the admin manually uninstall the extending themes) because it seems obvious that some work has been done for theming Moodle and the core clean theme will automatically lose all the theming. So I agree with a) too finally - personally if I theme mymobile, I probably would want to theme "clean" before to switch.

          Show
          Jérôme Mouneyrac added a comment - - edited I though the solution b) (automatically editing the theme php file) was the best because: I think it's the best for people managing no-critical sites. Administrators who manage critical sites most likely read the release notes. So they'll know that their php files have been outrageously edited by Moodle. However I hesitated with a) (the admin manually uninstall the extending themes) because it seems obvious that some work has been done for theming Moodle and the core clean theme will automatically lose all the theming. So I agree with a) too finally - personally if I theme mymobile, I probably would want to theme "clean" before to switch.
          Hide
          Jérôme Mouneyrac added a comment -

          After talking about it in the scrumm meting we decided to go with a). So ending it to peer-review

          Show
          Jérôme Mouneyrac added a comment - After talking about it in the scrumm meting we decided to go with a). So ending it to peer-review
          Hide
          Mark Nelson added a comment -

          Hi Jerome,

          I edited the instructions as the name of themes were not themeA and themeB, but were just the names of the zips you attached. It would make testing this confusing as people would have to make a mental note about which theme was in each zip. I also made it clearer on what needs to be done. I also removed one of the steps as it does not seem appropriate in a real scenario, the step was "Remove the patch.", which is not something a normal administrator would do and without doing this I am stuck (explained further on).

          lib/themelib.php
          1. The indentation is wrong. You are using tabs, when it should be four spaces.
          2. I suggest adding underscores to your function names to make it more readable.
          General
          1. The page displayed during upgrade is confusing. It says that I am missing one of the dependencies for Boxxie2, which is theme_mymobile, but that was intentionally deleted during the upgrade. It is going to leave the administrator confused, they will probably end up downloading that theme just so they can get past the upgrade process.
          2. The testing instructions (before I removed the step) asked the user to the remove the patch, but this isn't a real life scenario. I was unable to actually uninstall and remove the themes as the upgrade process wouldn't proceed. The patch MDL-41245 requires that you run the upgrade process after uninstalling a plugin. However, now I am stuck on the page telling me that Boxxie2 depends on theme_mymobile, but I can't uninstall Boxxie2 on the plugins overview page because Boxxie3 depends on it, which I can't finish uninstalling because I can't proceed on the admin/index.php due to the dependency!
          Show
          Mark Nelson added a comment - Hi Jerome, I edited the instructions as the name of themes were not themeA and themeB, but were just the names of the zips you attached. It would make testing this confusing as people would have to make a mental note about which theme was in each zip. I also made it clearer on what needs to be done. I also removed one of the steps as it does not seem appropriate in a real scenario, the step was "Remove the patch.", which is not something a normal administrator would do and without doing this I am stuck (explained further on). lib/themelib.php The indentation is wrong. You are using tabs, when it should be four spaces. I suggest adding underscores to your function names to make it more readable. General The page displayed during upgrade is confusing. It says that I am missing one of the dependencies for Boxxie2, which is theme_mymobile, but that was intentionally deleted during the upgrade. It is going to leave the administrator confused, they will probably end up downloading that theme just so they can get past the upgrade process. The testing instructions (before I removed the step) asked the user to the remove the patch, but this isn't a real life scenario. I was unable to actually uninstall and remove the themes as the upgrade process wouldn't proceed. The patch MDL-41245 requires that you run the upgrade process after uninstalling a plugin. However, now I am stuck on the page telling me that Boxxie2 depends on theme_mymobile, but I can't uninstall Boxxie2 on the plugins overview page because Boxxie3 depends on it, which I can't finish uninstalling because I can't proceed on the admin/index.php due to the dependency!
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Thanks Mark, for updating the test instruction and for your suggestion. I'll make the changes (PS: I just switched my environment in case people wonder).

          About the removed step, I think it all depends on how we explain clearly things in the release notes. We talked about the situation in the scrum meeting (https://tracker.moodle.org/browse/MDL-40874?focusedCommentId=240213&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-240213 - however it's good to have more opinion). When the dependency error appears, then the administrator needs to read the release note and find out that he needs to revert the changes. This case is going to be very rare - it only concerns people having extended the mymobile theme - we suppose these people know what they are doing and are able to revert an update. I'll add the test steps back and send for integration to have integrator opinion too.

          Note: I was installing https://moodle.org/plugins/view.php?plugin=assignfeedback_pdf with the automatic install from Moodle.org. Then I got stuck into the dependency error because I should have installed a different plugin before. The only soution was to manually install the other plugin. This case is more likely to impact more people that are not able to access the server.

          Show
          Jérôme Mouneyrac added a comment - - edited Thanks Mark, for updating the test instruction and for your suggestion. I'll make the changes (PS: I just switched my environment in case people wonder). About the removed step, I think it all depends on how we explain clearly things in the release notes. We talked about the situation in the scrum meeting ( https://tracker.moodle.org/browse/MDL-40874?focusedCommentId=240213&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-240213 - however it's good to have more opinion). When the dependency error appears, then the administrator needs to read the release note and find out that he needs to revert the changes. This case is going to be very rare - it only concerns people having extended the mymobile theme - we suppose these people know what they are doing and are able to revert an update. I'll add the test steps back and send for integration to have integrator opinion too. Note: I was installing https://moodle.org/plugins/view.php?plugin=assignfeedback_pdf with the automatic install from Moodle.org. Then I got stuck into the dependency error because I should have installed a different plugin before. The only soution was to manually install the other plugin. This case is more likely to impact more people that are not able to access the server.
          Hide
          Mark Nelson added a comment -

          Jerome,

          I strongly disagree with the step that you will need to revert the patch in order for the upgrade to work. This is not a realistic situation at all. The admin may know how to use git, and may read the release notes but that does not mean they will know they have to revert the patch in order for it to work. IMO The user should NEVER have to touch Moodle core code (unless they obviously want to add customisations), in this case your issue will occur even if they have no customisations at all.

          Show
          Mark Nelson added a comment - Jerome, I strongly disagree with the step that you will need to revert the patch in order for the upgrade to work. This is not a realistic situation at all. The admin may know how to use git, and may read the release notes but that does not mean they will know they have to revert the patch in order for it to work. IMO The user should NEVER have to touch Moodle core code (unless they obviously want to add customisations), in this case your issue will occur even if they have no customisations at all.
          Hide
          Dan Poltawski added a comment -

          Hi,

          I agree with Mark that this doesn't seem practical. We need to abort doing anything in advance I think (not thought about this deeply).

          Also the patch seems to include themelib in upgrade code, that is a nono: http://docs.moodle.org/dev/Upgrade_API#Upgrade_code_restrictions

          Show
          Dan Poltawski added a comment - Hi, I agree with Mark that this doesn't seem practical. We need to abort doing anything in advance I think (not thought about this deeply). Also the patch seems to include themelib in upgrade code, that is a nono: http://docs.moodle.org/dev/Upgrade_API#Upgrade_code_restrictions
          Hide
          Mark Nelson added a comment -

          Ah right, wasn't aware of that! I guess it makes sense since we may deprecate/remove functions and end up breaking the upgrade process.

          Show
          Mark Nelson added a comment - Ah right, wasn't aware of that! I guess it makes sense since we may deprecate/remove functions and end up breaking the upgrade process.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Thanks Dan, can you pick your favorite alternative. The ones I suggested are:

          a) we display the attached screenshot. Then it's the job of the administrator to "git revert", properly uninstall the extending theme.
          b) during upgrade, we check each extending themes, edit their version.php on the fly (theme_mymobile => theme_standard) so the upgrade continue.
          c) in the upgrade core function, we do a specific hack to ignore the dependency error for theme_mymobile.

          PS: Dan I knew the issue about calling core function that but some code from upgrade do call Moodle core lib functions, so I thought it was ok and clean. The alternative to themelib.php function is to add the code into upgradelib.php. So either the functions are floating around in themelib.php, either I remove the clean recursivity part. For the sake of being maintainable and because I thought it could be nice to have these functions, I wrote them in themelib.php. But I don't mind to put all code - non recursive - into upgradelib.php. I'll do this change.
          PS: the FRONTEND team picked a)

          Show
          Jérôme Mouneyrac added a comment - - edited Thanks Dan, can you pick your favorite alternative. The ones I suggested are: a) we display the attached screenshot. Then it's the job of the administrator to "git revert", properly uninstall the extending theme. b) during upgrade, we check each extending themes, edit their version.php on the fly (theme_mymobile => theme_standard) so the upgrade continue. c) in the upgrade core function, we do a specific hack to ignore the dependency error for theme_mymobile. PS: Dan I knew the issue about calling core function that but some code from upgrade do call Moodle core lib functions, so I thought it was ok and clean. The alternative to themelib.php function is to add the code into upgradelib.php. So either the functions are floating around in themelib.php, either I remove the clean recursivity part. For the sake of being maintainable and because I thought it could be nice to have these functions, I wrote them in themelib.php. But I don't mind to put all code - non recursive - into upgradelib.php. I'll do this change. PS: the FRONTEND team picked a)
          Hide
          Mark Nelson added a comment -

          I don't think b) is really an option as you can't ensure that the files will be writeable.

          Show
          Mark Nelson added a comment - I don't think b) is really an option as you can't ensure that the files will be writeable.
          Hide
          Dan Poltawski added a comment -

          The code must be duplicated in upgradelib.php, it may seem nasty but in future what happens:

          1. Someone changes the themelib function, adding a dependency on some external code which needs upgrade to happen
          2. Your upgrade step has a circular dependency on the upgrade and people can't upgrade.

          And well, again without fully understanding this issue I think a specific hack c) is much better than requiring admins to revert changes in git.

          Show
          Dan Poltawski added a comment - The code must be duplicated in upgradelib.php, it may seem nasty but in future what happens: Someone changes the themelib function, adding a dependency on some external code which needs upgrade to happen Your upgrade step has a circular dependency on the upgrade and people can't upgrade. And well, again without fully understanding this issue I think a specific hack c) is much better than requiring admins to revert changes in git.
          Hide
          Jérôme Mouneyrac added a comment -

          I made the hack suggested in c). Sending for integration.

          Show
          Jérôme Mouneyrac added a comment - I made the hack suggested in c). Sending for integration.
          Hide
          Damyon Wiese added a comment -

          Thanks Jerome,

          Maybe you discussed this in a later scrum - but in the version I recall we decided to:

          a) Update user/course/system preferences that were set to mymobile to now use clean
          b) Detect when loading a theme with a missing parent and fallback on the standard theme (with appropriate debugging)

          Deleting all config for custom themes is not acceptable. The user should be able to upgrade, find that mymobile has been removed, install it from the plugins db - and then continue on as normal.

          -1 for requiring git reverts
          -1 for deleting data from custom themes.
          -1 for calling core functions from the upgrade code

          Reopening this as it needs further work.
          Also (just a reminder) - please keep the testing instructions in sync with your changes.

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Thanks Jerome, Maybe you discussed this in a later scrum - but in the version I recall we decided to: a) Update user/course/system preferences that were set to mymobile to now use clean b) Detect when loading a theme with a missing parent and fallback on the standard theme (with appropriate debugging) Deleting all config for custom themes is not acceptable. The user should be able to upgrade, find that mymobile has been removed, install it from the plugins db - and then continue on as normal. -1 for requiring git reverts -1 for deleting data from custom themes. -1 for calling core functions from the upgrade code Reopening this as it needs further work. Also (just a reminder) - please keep the testing instructions in sync with your changes. Thanks, Damyon
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Thanks Damyon. I'll remove the core function calls.

          Just for the info about the current patch:

          • it doesn't require git revert
          • it doesn't require a fallback as the themes set on mymobile or extending mymobile now use the Clean theme. Imo it makes more sense to change the mobile theme to clean than implementing a fallback to the Standard theme. The themes extending mymobile are obviously for mobile and they are more suited to be changed to the Clean theme. Plus a fallback would be additional code to maintain for something I don't consider to be the correct solution. However if most of people want the fallback solution, I'll make extending mymobile themes fallback to the Standard theme, no problem
          • I kept the test instructions up to date.
          Show
          Jérôme Mouneyrac added a comment - - edited Thanks Damyon. I'll remove the core function calls. Just for the info about the current patch: it doesn't require git revert it doesn't require a fallback as the themes set on mymobile or extending mymobile now use the Clean theme. Imo it makes more sense to change the mobile theme to clean than implementing a fallback to the Standard theme. The themes extending mymobile are obviously for mobile and they are more suited to be changed to the Clean theme. Plus a fallback would be additional code to maintain for something I don't consider to be the correct solution. However if most of people want the fallback solution, I'll make extending mymobile themes fallback to the Standard theme, no problem I kept the test instructions up to date.
          Hide
          Damyon Wiese added a comment -

          Note - the theme fallback a good idea not just for this case of removing mymobile - it stops the site from breaking when there is any problem with the installed themes.

          You are actually deleting all of the config and any other custom data for any theme that has inherited from mymobile in this version of the patch. Even if they then install mymobile from the plugins db - they have lost all of their settings.

          Show
          Damyon Wiese added a comment - Note - the theme fallback a good idea not just for this case of removing mymobile - it stops the site from breaking when there is any problem with the installed themes. You are actually deleting all of the config and any other custom data for any theme that has inherited from mymobile in this version of the patch. Even if they then install mymobile from the plugins db - they have lost all of their settings.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Yes I thought about other theme but actually is that good? When someone create a theme that extend something wrong, they see the standard theme, is it not more wrong than displaying an error? Other people using this created theme could think that the theme they have installed is the standard theme? I think it's not that good.

          I don't understand what you mean by they have lost all of their settings, which settings?

          Show
          Jérôme Mouneyrac added a comment - - edited Yes I thought about other theme but actually is that good? When someone create a theme that extend something wrong, they see the standard theme, is it not more wrong than displaying an error? Other people using this created theme could think that the theme they have installed is the standard theme? I think it's not that good. I don't understand what you mean by they have lost all of their settings, which settings?
          Hide
          Damyon Wiese added a comment -

          Sorry - looking again, I misread the patch. I thought you were also calling plugin_delete on all the subthemes.

          Show
          Damyon Wiese added a comment - Sorry - looking again, I misread the patch. I thought you were also calling plugin_delete on all the subthemes.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          No problem Damyon. Let me know after talking with other integrator what you all prefer, I still stand on my opinion that setting all the "mymobile" (original and extending) themes on Clean is better than falling back on standard because:

          • the patch is consistent in its behavior (all not working theme are replaced by the same theme)
          • users are not tricked into thinking a theme is the Standard theme when they have a theme extending an none existing theme.
          • less code => better maintenance
          • Clean is a mobile replacement for a deprecated mobile theme - Standard is not.
          Show
          Jérôme Mouneyrac added a comment - - edited No problem Damyon. Let me know after talking with other integrator what you all prefer, I still stand on my opinion that setting all the "mymobile" (original and extending) themes on Clean is better than falling back on standard because: the patch is consistent in its behavior (all not working theme are replaced by the same theme) users are not tricked into thinking a theme is the Standard theme when they have a theme extending an none existing theme. less code => better maintenance Clean is a mobile replacement for a deprecated mobile theme - Standard is not.
          Hide
          Sam Hemelryk added a comment - - edited

          Ok so I'm just going to dump down some of my thoughts here now (I've tried this once before but it go to wordy and I never submit it).
          I'll start by summarising what I think should happen:

          I think we should be super duper passive with this issue.
          Remove the mymobile theme folder and its files (obviously) and add a comment to reflect its removal and a link + instructions to reinstall in theme/upgrade.txt.
          During upgrade I think we should do scan for any uses of the mymobile theme, and if there are none then we uninstall the theme.
          If there are any uses of the mymobile theme then lets not do anything within upgrade, leave its settings there. The theme doesn't appear to introduce anything more than a few settings so worst case we end up with a little bit of stale data in on the site.
          The upside is that if someone is using the theme they won't lose there settings.
          That is the only required change I think we should make.
          We then make sure that the following is true:

          1. If a theme has been selected that does not physically exist we fall back to theme_config::DEFAULT_THEME. Regardless of everything that is our default theme, when clean is suitably polished I'm sure we'll switch to that.
          2. Do the same if one of theme's parent is missing.
          3. On the theme selector page ensure there is a notification at the top if a theme has been selected for the site that does exist.
          4. The same as above for the course, course category, and user settings page.

          Under all circumstances a theme not existing can not lead to an error, such a thing could potentially break upgrade triggering and other core check we make before output starts.
          It should never lead to a notice (debugging or otherwise) triggered when the theme is selected. That process MUST NOT generate output for the same reason as above (although less impact).
          Really the process of selecting a theme must always succeed.

          And that's it. Nothing more. All we do is try to provide a nice fall-over that will not change anything that the user sees except that the theme they selected is no the one they get.

          Must suggestion is don't try to be smart.
          Perhaps the one thing we could do that would be really nice (but perhaps better as a separate issue) would be to provide a clean up utility for removing old theme related data from a database. It should be reasonably easy to create and would allow those people concerned about stale data to maintain a clean site (no matter how trivial the data).
          Actually definitely a separate issue if we decide to go that way, I actually recall someone may have already done that.

          Show
          Sam Hemelryk added a comment - - edited Ok so I'm just going to dump down some of my thoughts here now (I've tried this once before but it go to wordy and I never submit it). I'll start by summarising what I think should happen: I think we should be super duper passive with this issue. Remove the mymobile theme folder and its files (obviously) and add a comment to reflect its removal and a link + instructions to reinstall in theme/upgrade.txt. During upgrade I think we should do scan for any uses of the mymobile theme, and if there are none then we uninstall the theme. If there are any uses of the mymobile theme then lets not do anything within upgrade, leave its settings there. The theme doesn't appear to introduce anything more than a few settings so worst case we end up with a little bit of stale data in on the site. The upside is that if someone is using the theme they won't lose there settings. That is the only required change I think we should make. We then make sure that the following is true: If a theme has been selected that does not physically exist we fall back to theme_config::DEFAULT_THEME. Regardless of everything that is our default theme, when clean is suitably polished I'm sure we'll switch to that. Do the same if one of theme's parent is missing. On the theme selector page ensure there is a notification at the top if a theme has been selected for the site that does exist. The same as above for the course, course category, and user settings page. Under all circumstances a theme not existing can not lead to an error, such a thing could potentially break upgrade triggering and other core check we make before output starts. It should never lead to a notice (debugging or otherwise) triggered when the theme is selected. That process MUST NOT generate output for the same reason as above (although less impact). Really the process of selecting a theme must always succeed. And that's it. Nothing more. All we do is try to provide a nice fall-over that will not change anything that the user sees except that the theme they selected is no the one they get. Must suggestion is don't try to be smart. Perhaps the one thing we could do that would be really nice (but perhaps better as a separate issue) would be to provide a clean up utility for removing old theme related data from a database. It should be reasonably easy to create and would allow those people concerned about stale data to maintain a clean site (no matter how trivial the data). Actually definitely a separate issue if we decide to go that way, I actually recall someone may have already done that.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          These are some great instruction Sam, thanks for having a look. I'll create an issue for implementing an alert message for none installed theme used as settings. (one of my point during against the fallback to Standard).

          At the end the Standard fallback solution doesn't solve (to me the obvious) of what users prefer:
          Mymobile => Clean, Extending MyMobile => Clean
          or
          MyMobile => Clean, Extending MyMobile => Standard
          But I agree it's total peanuts after thinking how many people are going to be impacted. I agree with the default theme fallback solution.

          Created: MDL-41602 (admin tool theme remover), MDL-41603 (alert message for uninstalled theme), MDL-41604 (fallback to default theme when not existing theme or parent)

          Show
          Jérôme Mouneyrac added a comment - - edited These are some great instruction Sam, thanks for having a look. I'll create an issue for implementing an alert message for none installed theme used as settings. (one of my point during against the fallback to Standard). At the end the Standard fallback solution doesn't solve (to me the obvious) of what users prefer: Mymobile => Clean, Extending MyMobile => Clean or MyMobile => Clean, Extending MyMobile => Standard But I agree it's total peanuts after thinking how many people are going to be impacted. I agree with the default theme fallback solution. Created: MDL-41602 (admin tool theme remover), MDL-41603 (alert message for uninstalled theme), MDL-41604 (fallback to default theme when not existing theme or parent)
          Hide
          Jérôme Mouneyrac added a comment -

          To summarize as I created some other issues, this issue is blocked by MDL-41604 (fallback to default theme) and it is now about:
          a) remove mymobile files
          b) theme/upgrade.txt: add some remove info + link/instruction to reinstall mymobile.
          c) when mymobile is set nowhere, then uninstall it during the upgrade.
          d) check that when mymobile has not been uninstalled, everything still works fine (i.e. fallback to default theme correctly).

          Show
          Jérôme Mouneyrac added a comment - To summarize as I created some other issues, this issue is blocked by MDL-41604 (fallback to default theme) and it is now about: a) remove mymobile files b) theme/upgrade.txt: add some remove info + link/instruction to reinstall mymobile. c) when mymobile is set nowhere, then uninstall it during the upgrade. d) check that when mymobile has not been uninstalled, everything still works fine (i.e. fallback to default theme correctly).
          Hide
          Damyon Wiese added a comment -

          +1 for Sams proposal.

          Show
          Damyon Wiese added a comment - +1 for Sams proposal.
          Hide
          Dan Poltawski added a comment -

          Adding Must fix for 2.6 because i'm telling people not to test/fix issues in this in 2.6.

          Show
          Dan Poltawski added a comment - Adding Must fix for 2.6 because i'm telling people not to test/fix issues in this in 2.6.
          Hide
          Damyon Wiese added a comment -

          Thanks Jerome - +1 from me - this seems to do only what we agreed to.

          I'll send to integration if the blocker looks OK.

          Show
          Damyon Wiese added a comment - Thanks Jerome - +1 from me - this seems to do only what we agreed to. I'll send to integration if the blocker looks OK.
          Hide
          Jérôme Mouneyrac added a comment -

          Sending to integration, this issue is dependant of MDL-41604 (and a little bit of MDLSITE-2494 too)

          Show
          Jérôme Mouneyrac added a comment - Sending to integration, this issue is dependant of MDL-41604 (and a little bit of MDLSITE-2494 too)
          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
          Eloy Lafuente (stronk7) added a comment - - edited

          This is hold until MDL-41604 gets approved, keeping it under integration for now ...

          Show
          Eloy Lafuente (stronk7) added a comment - - edited This is hold until MDL-41604 gets approved, keeping it under integration for now ...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          added docs_required (surely it needs to go to a clear place in release notes) and ui_change labels.

          Show
          Eloy Lafuente (stronk7) added a comment - added docs_required (surely it needs to go to a clear place in release notes) and ui_change labels.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi MDL-41604 has been integrated, so this is unlocked… but:

          1) Need to review the db/lib/upgrade.php.

          a) You are using get_field() incorrecly. It throws debugging if multiple are found. Use record_exists() or similar instead.

          Also, apart from this there are, in db/lib/upgrade.php

          2) We are leaving there orphaned data, just in case the site admin has the occurrence of reinstalling theme_mymobile. And I must confess that I hate "orphaned data just in case". IMO, what we should do is:

          a) In upgrade instruction, state clearly that, if the site is using theme_mymobile and wants to continue using it, then it must be present in place BEFORE running the upgrade. Else all uses will be moved to "xxxxx" (standard, clean or whatever).

          b) In upgrade… if theme_mymobile is installed (some file_exists() can tell us), then do nothing. Else delete all configs, logs and replace all uses (site/cat/course/user/mnet) to "xxxxx" (standard, clean or whatever). Final goal: 0 orphaned data.

          3) Due to some recent changes, the current patch is conflicting really badly. Needs review/rebase/conflict-solving.

          4) The hack in lib/pluginlib.php is really necessary? Note I've not verified it extensively but things like that should not be necessary.

          Other than that, ok. But I think this needs a bit more of work, so reopening. Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi MDL-41604 has been integrated, so this is unlocked… but: 1) Need to review the db/lib/upgrade.php. a) You are using get_field() incorrecly. It throws debugging if multiple are found. Use record_exists() or similar instead. Also, apart from this there are, in db/lib/upgrade.php 2) We are leaving there orphaned data, just in case the site admin has the occurrence of reinstalling theme_mymobile. And I must confess that I hate "orphaned data just in case". IMO, what we should do is: a) In upgrade instruction, state clearly that, if the site is using theme_mymobile and wants to continue using it, then it must be present in place BEFORE running the upgrade. Else all uses will be moved to "xxxxx" (standard, clean or whatever). b) In upgrade… if theme_mymobile is installed (some file_exists() can tell us), then do nothing. Else delete all configs, logs and replace all uses (site/cat/course/user/mnet) to "xxxxx" (standard, clean or whatever). Final goal: 0 orphaned data. 3) Due to some recent changes, the current patch is conflicting really badly. Needs review/rebase/conflict-solving. 4) The hack in lib/pluginlib.php is really necessary? Note I've not verified it extensively but things like that should not be necessary. Other than that, ok. But I think this needs a bit more of work, so reopening. Thanks!
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Thanks Eloy for your review.

          2-b) I done something similar in my first patches: https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-40874-master-2#diff-2815e308b3c8af139c799c4b19d841fcR284
          Sam suggested to not overwrite the data, and keep it simple which I agreed. So it's "orphaned data" versus "modifying user data". Note in favor of orphaned data, the admin could make a DB search, and replace the mobile theme by a different one. When overwriting, admins won't be able to detect which users picked the mymobile theme. I'm ok with both solutions, they both have their faults and it seems to me they are both not going to happen that much neither going to be damageable (even thought "orphaned data" seems a bit more admin friendly).

          Show
          Jérôme Mouneyrac added a comment - - edited Thanks Eloy for your review. 2-b) I done something similar in my first patches: https://github.com/mouneyrac/moodle/compare/moodle:master...MDL-40874-master-2#diff-2815e308b3c8af139c799c4b19d841fcR284 Sam suggested to not overwrite the data, and keep it simple which I agreed. So it's "orphaned data" versus "modifying user data". Note in favor of orphaned data, the admin could make a DB search, and replace the mobile theme by a different one. When overwriting, admins won't be able to detect which users picked the mymobile theme. I'm ok with both solutions, they both have their faults and it seems to me they are both not going to happen that much neither going to be damageable (even thought "orphaned data" seems a bit more admin friendly).
          Hide
          Jérôme Mouneyrac added a comment -

          after thinking maybe 2-a) + 2-b) + 4) is the right solution...

          Show
          Jérôme Mouneyrac added a comment - after thinking maybe 2-a) + 2-b) + 4) is the right solution...
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Eloy, I implemented both solutions.

          "modifying user data": https://github.com/mouneyrac/moodle/compare/f8eff10319e1bd67e10634e79002b850702946c6...MDL-40874-master-7 (also see attached picture)

          "Orphaned data":
          https://github.com/mouneyrac/moodle/compare/f8eff10319e1bd67e10634e79002b850702946c6...MDL-40874-master-5

          I'll retest all and send to integration the "modifying user data".

          Show
          Jérôme Mouneyrac added a comment - - edited Eloy, I implemented both solutions. "modifying user data": https://github.com/mouneyrac/moodle/compare/f8eff10319e1bd67e10634e79002b850702946c6...MDL-40874-master-7 (also see attached picture) "Orphaned data": https://github.com/mouneyrac/moodle/compare/f8eff10319e1bd67e10634e79002b850702946c6...MDL-40874-master-5 I'll retest all and send to integration the "modifying user data".
          Hide
          Jérôme Mouneyrac added a comment -

          Sent to integration, it still require to have MDLSITE-2494 fixed.

          Show
          Jérôme Mouneyrac added a comment - Sent to integration, it still require to have MDLSITE-2494 fixed.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          argh... as we said the mymobile theme was going to be removed, we didn't care to check/fix mymobile theme, it's now broken (can't even go through the upgrade process)... we can not suggest to reinstall it in the current state...

          I talk a bit around, it's been suggested that it's the reason that we remove it, to not maintain it. So I see two solutions:
          a) fix mymobile theme for 2.6 so at least the user can go through the upgrade process
          b) do not mention about reinstalling, losing data... then the patch does not make too much sense for checking stuff.

          To me we should at least fix the upgrade process, I created MDL-42346

          Show
          Jérôme Mouneyrac added a comment - - edited argh... as we said the mymobile theme was going to be removed, we didn't care to check/fix mymobile theme, it's now broken (can't even go through the upgrade process)... we can not suggest to reinstall it in the current state... I talk a bit around, it's been suggested that it's the reason that we remove it, to not maintain it. So I see two solutions: a) fix mymobile theme for 2.6 so at least the user can go through the upgrade process b) do not mention about reinstalling, losing data... then the patch does not make too much sense for checking stuff. To me we should at least fix the upgrade process, I created MDL-42346
          Hide
          Mark Nelson added a comment -

          If it's going to be removed couldn't we force change theme to clean then perform the uninstall? IMO, there is no point wasting resources to fix something we want to remove.

          Show
          Mark Nelson added a comment - If it's going to be removed couldn't we force change theme to clean then perform the uninstall? IMO, there is no point wasting resources to fix something we want to remove.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Let's have a look if we can fix it quickly (it sounds pretty bad to me to remove something without delivering a solution to reinstall it and have it working in the previous state)

          Show
          Jérôme Mouneyrac added a comment - - edited Let's have a look if we can fix it quickly (it sounds pretty bad to me to remove something without delivering a solution to reinstall it and have it working in the previous state)
          Hide
          Jérôme Mouneyrac added a comment -

          ok I have a simple fix, I'll add it in the Moodle.org mymobile theme. Publishing Moodle.org mymobile theme and we're all good.

          Show
          Jérôme Mouneyrac added a comment - ok I have a simple fix, I'll add it in the Moodle.org mymobile theme. Publishing Moodle.org mymobile theme and we're all good.
          Hide
          Mark Nelson added a comment -

          Do we have any stats out there on the amount of people using this theme?

          Show
          Mark Nelson added a comment - Do we have any stats out there on the amount of people using this theme?
          Hide
          Jérôme Mouneyrac added a comment - - edited

          I added a new MyMobile version to the Moodle.org plugin: https://moodle.org/plugins/pluginversion.php?id=4563. Edited the patch string to mention the link, submitting to integration.

          PS: Mark, I don't know if we have the stats.

          Show
          Jérôme Mouneyrac added a comment - - edited I added a new MyMobile version to the Moodle.org plugin: https://moodle.org/plugins/pluginversion.php?id=4563 . Edited the patch string to mention the link, submitting to integration. PS: Mark, I don't know if we have the stats.
          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
          Marina Glancy added a comment -

          Hi Jerome,
          can you please rebase over current integration and also remove 'mymobile' from the list of standard plugins in lib/classes/plugin_manager.php

          PS why 'removemymobilewarning' CSS is added to base but is not added to bootstrapbase? Why do we need a new CSS class at all, can't we use existing error, notification, etc.?

          Show
          Marina Glancy added a comment - Hi Jerome, can you please rebase over current integration and also remove 'mymobile' from the list of standard plugins in lib/classes/plugin_manager.php PS why 'removemymobilewarning' CSS is added to base but is not added to bootstrapbase? Why do we need a new CSS class at all, can't we use existing error, notification, etc.?
          Hide
          Marina Glancy added a comment -
          git grep 2013101100.01
          git grep mymobile
          
          Show
          Marina Glancy added a comment - git grep 2013101100.01 git grep mymobile
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Marina, fixing that.

          Show
          Jérôme Mouneyrac added a comment - Thanks Marina, fixing that.
          Hide
          Jérôme Mouneyrac added a comment -

          Done Marina I removed the css rule, used existing one that display well and similar purpose, removed the last mymobile reference, and rebased.

          Show
          Jérôme Mouneyrac added a comment - Done Marina I removed the css rule, used existing one that display well and similar purpose, removed the last mymobile reference, and rebased.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oh, Jerome…

          I'm really afraid, but I think your latest code is sort of "over-reaction" to my proposal of handling orphaned data.

          I like your code 99%, but there is an 1% we cannot add to core. I know you've put tons of love on it, so I'm afraid but the "admin/renderer hack" must disappear from there completely. That's not proper code for a renderer and we never have done that when a core plugin has been killed. Those are the reasons I called it a "hack", with love.

          So I'd propose to, simply:

          1) delete the admin/renderer hack completely (lang strings, new method, …) And change testing instructions accordingly. We just have to verify final results when the plugin is re-installed manually and when it's not.
          2) Move target version from your 2013102203.01 to 2013102400.01, without modifying anything else, looks perfect.
          3) Then in these 3 places, add a important line commenting that the module will be uninstalled and everybody moved to clean and also that the only way to keep the site using mymobile is to install it manually from the plugins database BEFORE upgrading. The places are:

          I think the 1), 2), 3) above are easily achievable and we can then proceed with this once and forever. Again, sorry if my ideas above implied the creation of the "renderer hack" that now I'm asking for annihilation.

          Also, once ready, feel free to ping Marina, she has volunteered to immediate integration of this issue.

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oh, Jerome… I'm really afraid, but I think your latest code is sort of "over-reaction" to my proposal of handling orphaned data. I like your code 99%, but there is an 1% we cannot add to core. I know you've put tons of love on it, so I'm afraid but the "admin/renderer hack" must disappear from there completely. That's not proper code for a renderer and we never have done that when a core plugin has been killed. Those are the reasons I called it a "hack", with love. So I'd propose to, simply: 1) delete the admin/renderer hack completely (lang strings, new method, …) And change testing instructions accordingly. We just have to verify final results when the plugin is re-installed manually and when it's not. 2) Move target version from your 2013102203.01 to 2013102400.01, without modifying anything else, looks perfect. 3) Then in these 3 places, add a important line commenting that the module will be uninstalled and everybody moved to clean and also that the only way to keep the site using mymobile is to install it manually from the plugins database BEFORE upgrading. The places are: theme/upgrade.txt : You have the information there but missing the BEFORE information. 2.6 release notes: http://docs.moodle.org/dev/Moodle_2.6_release_notes 2.6 upgrading: http://docs.moodle.org/26/en/Upgrading_to_Moodle_2.6 I think the 1), 2), 3) above are easily achievable and we can then proceed with this once and forever. Again, sorry if my ideas above implied the creation of the "renderer hack" that now I'm asking for annihilation. Also, once ready, feel free to ping Marina, she has volunteered to immediate integration of this issue. Thanks and ciao
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Jérôme Mouneyrac added a comment -

          No problem Eloy. Making the changes...

          Show
          Jérôme Mouneyrac added a comment - No problem Eloy. Making the changes...
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Done, to make the review easy I didn't change the previous commit and added a new commit. (I just notice that instead of start development I pressed sent to integration, hopefully you didn't look at it that quickly, anyway it's ready for integration now )

          Show
          Jérôme Mouneyrac added a comment - - edited Done, to make the review easy I didn't change the previous commit and added a new commit. (I just notice that instead of start development I pressed sent to integration, hopefully you didn't look at it that quickly, anyway it's ready for integration now )
          Hide
          Jérôme Mouneyrac added a comment -

          @#$% something got wrong in my push missing a change...

          Show
          Jérôme Mouneyrac added a comment - @#$% something got wrong in my push missing a change...
          Hide
          Jérôme Mouneyrac added a comment -

          Done

          Show
          Jérôme Mouneyrac added a comment - Done
          Hide
          Eloy Lafuente (stronk7) 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
          Eloy Lafuente (stronk7) 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
          Aparup Banerjee added a comment -

          This has been re-entered into plugins directory (https://moodle.org/plugins/view.php?plugin=theme_mymobile) for 2.6. If this is doesn't go through before release please ping!

          Show
          Aparup Banerjee added a comment - This has been re-entered into plugins directory ( https://moodle.org/plugins/view.php?plugin=theme_mymobile ) for 2.6. If this is doesn't go through before release please ping!
          Hide
          Jérôme Mouneyrac added a comment -

          I'm changing the issue priority to blocker.

          Show
          Jérôme Mouneyrac added a comment - I'm changing the issue priority to blocker.
          Hide
          Marina Glancy added a comment -

          Thanks Jerome, integrated in master

          Show
          Marina Glancy added a comment - Thanks Jerome, integrated in master
          Hide
          Adrian Greeve added a comment -

          Finally finished. It took forever.
          Test passed.

          Show
          Adrian Greeve added a comment - Finally finished. It took forever. Test passed.
          Hide
          Marina Glancy added a comment -

          Thanks a lot Adrian! The testing instructions are massive but it's very important to remove the theme properly

          Show
          Marina Glancy added a comment - Thanks a lot Adrian! The testing instructions are massive but it's very important to remove the theme properly
          Hide
          Mary Cooch added a comment -

          I have edited the 2.6 docs to remove references to MyMobile as a standard theme and have changed its page http://docs.moodle.org/26/en/MyMobile_theme to reflect its new status as a theme in the plugins directory (more details needed) I have seen MyMobile is also mentioned in http://docs.moodle.org/26/en/Upgrading so I am going to remove the docs_required label.

          Show
          Mary Cooch added a comment - I have edited the 2.6 docs to remove references to MyMobile as a standard theme and have changed its page http://docs.moodle.org/26/en/MyMobile_theme to reflect its new status as a theme in the plugins directory (more details needed) I have seen MyMobile is also mentioned in http://docs.moodle.org/26/en/Upgrading so I am going to remove the docs_required label.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It's Friday, I'm tired so I won't be very imaginative today.

          No matter of that, yes, you did it! Thanks for your collaboration!

          Closing this as fixed!

          Show
          Eloy Lafuente (stronk7) added a comment - It's Friday, I'm tired so I won't be very imaginative today. No matter of that, yes, you did it! Thanks for your collaboration! Closing this as fixed!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Agile