Uploaded image for project: '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
    • Sprint:
      FRONTEND Sprint 6
    • Story Points (Obsolete):
      20
    • 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.

        Gliffy Diagrams

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

          Issue Links

            Activity

            Hide
            salvetore 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
            salvetore 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
            dougiamas 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
            dougiamas 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
            lazydaisy 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
            lazydaisy 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
            gb2048 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
            gb2048 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
            salvetore 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
            salvetore 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
            poltawski 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
            poltawski 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
            lazydaisy 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
            lazydaisy 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
            poltawski 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
            poltawski 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
            dougiamas 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
            dougiamas 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
            dougiamas 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
            dougiamas 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            markn 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
            markn 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
            jerome 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
            jerome 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
            markn 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
            markn 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
            poltawski 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
            poltawski 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
            markn 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
            markn 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
            jerome 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
            jerome 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
            markn 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
            markn 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
            poltawski 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
            poltawski 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
            jerome Jérôme Mouneyrac added a comment -

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

            Show
            jerome Jérôme Mouneyrac added a comment - I made the hack suggested in c). Sending for integration.
            Hide
            damyon 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 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            jerome 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
            jerome 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 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 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
            jerome 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
            jerome 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 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 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
            jerome 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
            jerome 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
            samhemelryk 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
            samhemelryk 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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 Damyon Wiese added a comment -

            +1 for Sams proposal.

            Show
            damyon Damyon Wiese added a comment - +1 for Sams proposal.
            Hide
            poltawski 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
            poltawski 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 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 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
            jerome 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
            jerome 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
            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
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

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

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited This is hold until MDL-41604 gets approved, keeping it under integration for now ...
            Hide
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            jerome 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
            jerome 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
            jerome Jérôme Mouneyrac added a comment -

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

            Show
            jerome Jérôme Mouneyrac added a comment - after thinking maybe 2-a) + 2-b) + 4) is the right solution...
            Hide
            jerome 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
            jerome 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
            jerome Jérôme Mouneyrac added a comment -

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

            Show
            jerome Jérôme Mouneyrac added a comment - Sent to integration, it still require to have MDLSITE-2494 fixed.
            Hide
            jerome 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
            jerome 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
            markn 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
            markn 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            markn Mark Nelson added a comment -

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

            Show
            markn Mark Nelson added a comment - Do we have any stats out there on the amount of people using this theme?
            Hide
            jerome 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
            jerome 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
            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
            marina 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 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 Marina Glancy added a comment -

            git grep 2013101100.01
            git grep mymobile

            Show
            marina Marina Glancy added a comment - git grep 2013101100.01 git grep mymobile
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Thanks Marina, fixing that.

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Marina, fixing that.
            Hide
            jerome 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
            jerome 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
            stronk7 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
            stronk7 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 CiBoT added a comment -

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

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

            No problem Eloy. Making the changes...

            Show
            jerome Jérôme Mouneyrac added a comment - No problem Eloy. Making the changes...
            Hide
            jerome 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
            jerome 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
            jerome Jérôme Mouneyrac added a comment -

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

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

            Done

            Show
            jerome Jérôme Mouneyrac added a comment - Done
            Hide
            stronk7 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
            stronk7 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
            nebgor 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
            nebgor 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
            jerome Jérôme Mouneyrac added a comment -

            I'm changing the issue priority to blocker.

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

            Thanks Jerome, integrated in master

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

            Finally finished. It took forever.
            Test passed.

            Show
            abgreeve Adrian Greeve added a comment - Finally finished. It took forever. Test passed.
            Hide
            marina 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 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
            marycooch 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
            marycooch 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
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  18/Nov/13

                  Agile