Moodle
  1. Moodle
  2. MDL-26680

My Moodle reset to default button

    Details

    • Testing Instructions:
      Hide

      Run behat tests.

      Repeat the following steps on both your "my" page and profile page:

      1. Click "Customise this page"
      2. Confirm that the "Reset page to default" button appears
      3. Add some blocks
      4. Click on the "reset page to default" button
      5. Confirm that added blocks have been removed
      6. Confirm that you are no longer in edit mode
      7. Confirm that there were no error messages
      Show
      Run behat tests. Repeat the following steps on both your "my" page and profile page: Click "Customise this page" Confirm that the "Reset page to default" button appears Add some blocks Click on the "reset page to default" button Confirm that added blocks have been removed Confirm that you are no longer in edit mode Confirm that there were no error messages
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull Master Branch:
      MDL-26680-master
    • Rank:
      16275

      Description

      When you modify 'Default My Moodle' it dosen't currently update older users my moodle pages only new users we need a button that users can click to reset their my moodle page back to the default, it would also be good to be able to 'reset all' my moodle pages to default.

      1. clean.png
        16 kB
      2. Resettodefault.jpg
        35 kB
      3. standard.png
        22 kB

        Activity

        Hide
        Hubert Chathi added a comment -

        For resetting all users to default, I think the best way to do this is to add a button to the "Default My Moodle Page" configuration screen to reset all users' dashboards.

        This all applies to the profile page as well.

        Show
        Hubert Chathi added a comment - For resetting all users to default, I think the best way to do this is to add a button to the "Default My Moodle Page" configuration screen to reset all users' dashboards. This all applies to the profile page as well.
        Hide
        Wast Eemail added a comment -

        I agree. This is an absolute necessity. Admins must have a way to implement site-wide changes to critical pages like the My Moodle/My Home and Profile Page – at any time.

        We use the My Moodle page as a home page for all users and have disabled the site navigation/site home. My Moodle is a much better fit, and we must have the capability to keep or reset users back to our default my moodle/my home and profile page structures and layouts.

        Show
        Wast Eemail added a comment - I agree. This is an absolute necessity. Admins must have a way to implement site-wide changes to critical pages like the My Moodle/My Home and Profile Page – at any time. We use the My Moodle page as a home page for all users and have disabled the site navigation/site home. My Moodle is a much better fit, and we must have the capability to keep or reset users back to our default my moodle/my home and profile page structures and layouts.
        Hide
        Hubert Chathi added a comment -

        When you modify 'Default My Moodle' it dosen't currently update older users my moodle pages

        JFTR, this is not quite correct. It should update existing users' pages unless they have customized the page.

        Show
        Hubert Chathi added a comment - When you modify 'Default My Moodle' it dosen't currently update older users my moodle pages JFTR, this is not quite correct. It should update existing users' pages unless they have customized the page.
        Hide
        Sheila Stamp added a comment -

        A temporary way round this might be to do via the back door i.e. SQL. I've seen that there is a My_Pages table that appears to be created when a user accesses their My Moodle page. I presume there are other tables that are involved as well but don't know Moodle well enough.

        Has anyone tried?

        It might be difficult to sift between those My Moodle pages that have simply been accessed or those that have been modified (taking into account JFTR's comment above) but might give us a dirty fix until this feature gets added in.

        Any ideas?

        Show
        Sheila Stamp added a comment - A temporary way round this might be to do via the back door i.e. SQL. I've seen that there is a My_Pages table that appears to be created when a user accesses their My Moodle page. I presume there are other tables that are involved as well but don't know Moodle well enough. Has anyone tried? It might be difficult to sift between those My Moodle pages that have simply been accessed or those that have been modified (taking into account JFTR's comment above) but might give us a dirty fix until this feature gets added in. Any ideas?
        Hide
        Sheila Stamp added a comment -

        I found that I had My_Page entries for any user regardless of whether they had edited the page or not. At the time all users were set up with editing permission set on for My Moodle.

        I have had a go deleting the My_Pages entry for a student (after backing up the data) and it is now picking up the latest version of the default page. The Null record from what I can see is the default page information so I left that well alone.

        After adding a role for the students to prevent them editing their My Moodle page, My_Pages records are no longer being added which I guess ties in with what JFTR said.

        Show
        Sheila Stamp added a comment - I found that I had My_Page entries for any user regardless of whether they had edited the page or not. At the time all users were set up with editing permission set on for My Moodle. I have had a go deleting the My_Pages entry for a student (after backing up the data) and it is now picking up the latest version of the default page. The Null record from what I can see is the default page information so I left that well alone. After adding a role for the students to prevent them editing their My Moodle page, My_Pages records are no longer being added which I guess ties in with what JFTR said.
        Hide
        Hubert Chathi added a comment -

        Yes, the null record is for the default pages. There will also be records in the block_instances table for each of the blocks that got added to the page, and there may possibly be some records in the block_positions table. They probably won't do much harm if they're left in there, but ideally they should be removed as well.

        Show
        Hubert Chathi added a comment - Yes, the null record is for the default pages. There will also be records in the block_instances table for each of the blocks that got added to the page, and there may possibly be some records in the block_positions table. They probably won't do much harm if they're left in there, but ideally they should be removed as well.
        Hide
        William Stites added a comment - - edited

        If after resetting and even deleting the user record in the my_pages and not having the "Default My Moodle" setting applied, where else would you look?

        The issue we are having is the we are given a single column view, with not left or right block areas as well as having some of the blocks overlaying one another (image of overlay: http://ow.ly/i/dn9C).

        Show
        William Stites added a comment - - edited If after resetting and even deleting the user record in the my_pages and not having the "Default My Moodle" setting applied, where else would you look? The issue we are having is the we are given a single column view, with not left or right block areas as well as having some of the blocks overlaying one another (image of overlay: http://ow.ly/i/dn9C ).
        Hide
        Bryce added a comment -

        I agree with all of the comments here. This is the only reason why I am not upgrading from 1.9 to 2.03. My students use the My Moodle page extensively.

        Show
        Bryce added a comment - I agree with all of the comments here. This is the only reason why I am not upgrading from 1.9 to 2.03. My students use the My Moodle page extensively.
        Hide
        Guido Hornig added a comment -

        We encourage users to modify my home. In the moment there is no way to help them resetting it to default by them self, by a techer nor by the admin. (without using sql directly)

        Show
        Guido Hornig added a comment - We encourage users to modify my home. In the moment there is no way to help them resetting it to default by them self, by a techer nor by the admin. (without using sql directly)
        Hide
        Mark Andrews added a comment -

        Would this functionality not fit within in the usability focus of the 2.3 release?
        Perhaps the spec could be expanded to notify users that the 'default' my page has changed which they could then preview it prior to selecting it.

        Show
        Mark Andrews added a comment - Would this functionality not fit within in the usability focus of the 2.3 release? Perhaps the spec could be expanded to notify users that the 'default' my page has changed which they could then preview it prior to selecting it.
        Hide
        Michael Hughes added a comment -

        This would be essential for us. We had implemented extensions to the 1.9 My Moodle page (which matches the 2.x functionality) to encourage them to make it their own, but we always have had users who delete everything and can't work out how to get back to something usable.

        Show
        Michael Hughes added a comment - This would be essential for us. We had implemented extensions to the 1.9 My Moodle page (which matches the 2.x functionality) to encourage them to make it their own, but we always have had users who delete everything and can't work out how to get back to something usable.
        Hide
        Michael Aherne added a comment -

        I've put together an implementation of the user reset button (https://github.com/micaherne/moodle/commit/86d1ed73ce152048eb30e46e70c52beadfad889e)

        Could someone review it for me please?

        Show
        Michael Aherne added a comment - I've put together an implementation of the user reset button ( https://github.com/micaherne/moodle/commit/86d1ed73ce152048eb30e46e70c52beadfad889e ) Could someone review it for me please?
        Hide
        Michael Aherne added a comment -

        I've added a pull request for the user reset feature, but I'm not sure who to submit it to for peer review - can anyone help?

        Show
        Michael Aherne added a comment - I've added a pull request for the user reset feature, but I'm not sure who to submit it to for peer review - can anyone help?
        Hide
        Michael de Raadt added a comment -

        Hi, Michael.

        You don't have to find a specific peer reviewer to have an issue peer reviewed. You just need to click the button to request a peer review and it will appear on a list that HQ devs monitor.

        Before you do submit to peer review, you might want to see what sort of things a peer reviewer is looking for. Check out the checklist for peer reviewers...

        http://docs.moodle.org/dev/Peer_reviewing_checklist

        Creating a set of testing instructions would be useful and possibly applying your patch across multiple branches would help.

        Show
        Michael de Raadt added a comment - Hi, Michael. You don't have to find a specific peer reviewer to have an issue peer reviewed. You just need to click the button to request a peer review and it will appear on a list that HQ devs monitor. Before you do submit to peer review, you might want to see what sort of things a peer reviewer is looking for. Check out the checklist for peer reviewers... http://docs.moodle.org/dev/Peer_reviewing_checklist Creating a set of testing instructions would be useful and possibly applying your patch across multiple branches would help.
        Hide
        Hubert Chathi added a comment -

        Michael A,
        It's certainly a good start. A few comments: (note: this is an unofficial review, since I am not an HQ dev)

        • It would be better to put the checking for $edit as an else clause. That is, do: "if ($reset !== null) { ... } else if ($edit !== null) { ... }
        • The "if ($reset !== null)" branch should turn editing mode off.
        • Personally, I would call the language string "resetpage" – it's in the "my" language file, so the "mymoodle" part of the string name is redundant
        • Why are you re-copying the page in my_reset_page, after you reset the page? I would imagine that resetting the page should result in the user using the system default page, unless the user wants to re-customize their page.

        Ultimately (these are not issues with your patch, but these should probably be fixed before this issue is closed):

        • there should also be a button for resetting the "My profile" page – the patch should more-or-less be the same, though it means that the my_reset_page function added by this patch would need another parameter: $pagetype (just like my_copy_page's $pagetype parameter)
        • there should be an interface for admins to reset other users' "My home" and "My profile" pages
        • the simplest option would be to add a button on the page for editing the system default pages to reset all users' pages
        • a more complicated would be to create an interface that allows admins to see who has customized their pages, and to select users to reset, or to reset all users' pages. (This would be useful, for example, if admins accidentally gave certain users permission to edit their My Moodle pages.)
        • one issue with this is that, on a system with with many users, resetting everyone's My Moodle pages could take a very long time. I don't think there's an easy solution. One possible (though non-ideal) solution would be to delete the 'my_pages' records immediately, and delete the blocks in a cron.
        Show
        Hubert Chathi added a comment - Michael A, It's certainly a good start. A few comments: (note: this is an unofficial review, since I am not an HQ dev) It would be better to put the checking for $edit as an else clause. That is, do: "if ($reset !== null) { ... } else if ($edit !== null) { ... } The "if ($reset !== null)" branch should turn editing mode off. Personally, I would call the language string "resetpage" – it's in the "my" language file, so the "mymoodle" part of the string name is redundant Why are you re-copying the page in my_reset_page, after you reset the page? I would imagine that resetting the page should result in the user using the system default page, unless the user wants to re-customize their page. Ultimately (these are not issues with your patch, but these should probably be fixed before this issue is closed): there should also be a button for resetting the "My profile" page – the patch should more-or-less be the same, though it means that the my_reset_page function added by this patch would need another parameter: $pagetype (just like my_copy_page's $pagetype parameter) there should be an interface for admins to reset other users' "My home" and "My profile" pages the simplest option would be to add a button on the page for editing the system default pages to reset all users' pages a more complicated would be to create an interface that allows admins to see who has customized their pages, and to select users to reset, or to reset all users' pages. (This would be useful, for example, if admins accidentally gave certain users permission to edit their My Moodle pages.) one issue with this is that, on a system with with many users, resetting everyone's My Moodle pages could take a very long time. I don't think there's an easy solution. One possible (though non-ideal) solution would be to delete the 'my_pages' records immediately, and delete the blocks in a cron.
        Hide
        Michael Aherne added a comment -

        Hi Hubert, many thanks for the feedback on the patch! I'll get your suggestions implemented (probably early next week). I can't remember why I re-created the page in my_reset_page but, you're right, it probably doesn't have to be done that way.

        Can you expand on the thinking behind your second point a bit? I don't understand why someone who has just reset their page would want editing mode to be switched off.

        On your other points, I think it's a good idea to have an option for users to reset their "my profile" page, and I'll see if I can add that into the patch. I'm less convinced by the idea of having a separate admin interface to reset users' pages, given that an administrator can log in as a user and reset their page for them if required. I wonder if it would be useful if I were to create a sub-issue for the basic "user reset" functionality as there's no reason why that can't be implemented separately from the "bulk reset" admin functionality. Do you think that would be a good idea? Like you, I wouldn't like to see this issue being closed just because the "user reset" functionality had been implemented!

        Cheers

        Michael

        Show
        Michael Aherne added a comment - Hi Hubert, many thanks for the feedback on the patch! I'll get your suggestions implemented (probably early next week). I can't remember why I re-created the page in my_reset_page but, you're right, it probably doesn't have to be done that way. Can you expand on the thinking behind your second point a bit? I don't understand why someone who has just reset their page would want editing mode to be switched off. On your other points, I think it's a good idea to have an option for users to reset their "my profile" page, and I'll see if I can add that into the patch. I'm less convinced by the idea of having a separate admin interface to reset users' pages, given that an administrator can log in as a user and reset their page for them if required. I wonder if it would be useful if I were to create a sub-issue for the basic "user reset" functionality as there's no reason why that can't be implemented separately from the "bulk reset" admin functionality. Do you think that would be a good idea? Like you, I wouldn't like to see this issue being closed just because the "user reset" functionality had been implemented! Cheers Michael
        Hide
        Hubert Chathi added a comment -

        Regarding my second point, when a user resets their page, their page should be linked to the system default page, and they will be viewing the system default page, so they shouldn't be editing it.

        Regarding the admin interface, while it's certainly possible for admins to log in as a user and reset their page, it is very tedious to do if there are many users that need their pages reset. But yes, it probably makes sense to make these separate issues (sub-issues is probably fine, although I don't know what HQ's opinion about sub-issues is), especially since the admin interface would be much more work than the user reset button.

        Show
        Hubert Chathi added a comment - Regarding my second point, when a user resets their page, their page should be linked to the system default page, and they will be viewing the system default page, so they shouldn't be editing it. Regarding the admin interface, while it's certainly possible for admins to log in as a user and reset their page, it is very tedious to do if there are many users that need their pages reset. But yes, it probably makes sense to make these separate issues (sub-issues is probably fine, although I don't know what HQ's opinion about sub-issues is), especially since the admin interface would be much more work than the user reset button.
        Hide
        Michael Aherne added a comment -

        I've created a new version of this patch. Many thanks to Hubert for his comments - in my original patch I was misunderstanding how the my page customisation functionality works. I've changed it so it removes any customisations and then returns the user to the system page with editing turned off. It also works for the user profile page too now.

        Show
        Michael Aherne added a comment - I've created a new version of this patch. Many thanks to Hubert for his comments - in my original patch I was misunderstanding how the my page customisation functionality works. I've changed it so it removes any customisations and then returns the user to the system page with editing turned off. It also works for the user profile page too now.
        Hide
        Hubert Chathi added a comment -

        Hi Michael,
        This new patch looks pretty good. A couple of minor comments, though:

        • when resetting the user profile page, the pagetypepattern when looking up blocks should be 'user-profile', so you'll have to add an extra parameter to the my_reset_page function (see the my_copy_page function in the my/lib.php file)
        • in my_reset_page, if there is no my_pages record found (which means the user doesn't have the page customize it), I personally would just pretend that the reset succeeded, instead of indicating a failure. (I don't know what others think about that – it's just my opinion.)
        • the reset button should also appear when the user isn't editing the page (so also set $resetbutton in the if branch just above where you're currently setting $resetbutton)
        • perhaps a better way in my_reset_page to check whether the user has customized their page, instead of using record_exists, is to first call my_get_page, and then check $page->userid. So something like this:
          function my_reset_page($userid, $private=MY_PAGE_PRIVATE) {
              global $DB, $CFG;
          
              $page = my_get_page($userid, $private);
              if ($page->userid == $userid) {
                  // do the reset here
                  ...
                  return $systempage;
              }
          
              // user hasn't customized their page
              return $page; // here I'm returning $page (which will be the system default page) instead of false, to pretend that the reset succeeded
          }
          

          This will save a database call, and then you don't need to worry about checking $CFG->forcedefaultmymoodle. This is not a major issue, though.

        I haven't tried the patch out, but it looks good.

        Show
        Hubert Chathi added a comment - Hi Michael, This new patch looks pretty good. A couple of minor comments, though: when resetting the user profile page, the pagetypepattern when looking up blocks should be 'user-profile', so you'll have to add an extra parameter to the my_reset_page function (see the my_copy_page function in the my/lib.php file) in my_reset_page, if there is no my_pages record found (which means the user doesn't have the page customize it), I personally would just pretend that the reset succeeded, instead of indicating a failure. (I don't know what others think about that – it's just my opinion.) the reset button should also appear when the user isn't editing the page (so also set $resetbutton in the if branch just above where you're currently setting $resetbutton) perhaps a better way in my_reset_page to check whether the user has customized their page, instead of using record_exists, is to first call my_get_page, and then check $page->userid. So something like this: function my_reset_page($userid, $ private =MY_PAGE_PRIVATE) { global $DB, $CFG; $page = my_get_page($userid, $ private ); if ($page->userid == $userid) { // do the reset here ... return $systempage; } // user hasn't customized their page return $page; // here I'm returning $page (which will be the system default page) instead of false , to pretend that the reset succeeded } This will save a database call, and then you don't need to worry about checking $CFG->forcedefaultmymoodle. This is not a major issue, though. I haven't tried the patch out, but it looks good.
        Hide
        Michael Aherne added a comment -

        Hi Hubert, thanks for the feedback. I've implemented all your suggestions as-is. Would you mind having another look at it for me? Cheers!

        Show
        Michael Aherne added a comment - Hi Hubert, thanks for the feedback. I've implemented all your suggestions as-is. Would you mind having another look at it for me? Cheers!
        Hide
        Hubert Chathi added a comment -

        Michael, it looks good. I haven't tested it, and I only had a quick look, but as far as I can tell, it looks fine. I hope that someone from HQ can do a formal peer review.

        Show
        Hubert Chathi added a comment - Michael, it looks good. I haven't tested it, and I only had a quick look, but as far as I can tell, it looks fine. I hope that someone from HQ can do a formal peer review.
        Hide
        Michael Aherne added a comment -

        Thanks, Hubert. I've requested a formal peer review.

        Show
        Michael Aherne added a comment - Thanks, Hubert. I've requested a formal peer review.
        Hide
        David Monllaó added a comment - - edited

        Thanks Michael and Hubert for working on this, the patch looks really good, just adding a few comments according to http://docs.moodle.org/dev/Peer_reviewing_checklist

        • [Y] Syntax
        • [Y] Output
        • [Y] Whitespace
          • I've found a trailing whitespace in my/index.php (line 57) but is not part of your patch, probably your IDE will clean it when you save the file
        • [Y] Language
          • Not sure but maybe is better to add the error string in error.php
        • [Y] Databases
        • [N] Testing
          • No testing instructions provided
        • [Y] Security
        • [N] Documentation
          • According to the existing labels this issue should probably include docs_required
        • [Y] Git
          • The pull branch is a bit outdated, but I rebased against latests weekly master without conflicts
        • [Y] Sanity check
          • I have doubts about the 'Reset to default' button in the user profile, IMO for the language string it seems that you are resetting your profile to the default values instead of resetting your profile page blocks distribution. If is interesting to have this button also in the profile page as you said maybe a more explicit language string for the user profile could be good
          • In my/index.php, after resetting the page you are changing the context from user to system (https://github.com/micaherne/moodle/commit/d8721fa25a793d529247f0f06724f9a96b3f4133#L1R100) but you are not allowed to do it (you can see a debugging message about it) a possible solution for this is to redirect() the user to my/index.php after the reset, this way the system defaults will be loaded and you will be using the user context
        Show
        David Monllaó added a comment - - edited Thanks Michael and Hubert for working on this, the patch looks really good, just adding a few comments according to http://docs.moodle.org/dev/Peer_reviewing_checklist [Y] Syntax [Y] Output [Y] Whitespace I've found a trailing whitespace in my/index.php (line 57) but is not part of your patch, probably your IDE will clean it when you save the file [Y] Language Not sure but maybe is better to add the error string in error.php [Y] Databases [N] Testing No testing instructions provided [Y] Security [N] Documentation According to the existing labels this issue should probably include docs_required [Y] Git The pull branch is a bit outdated, but I rebased against latests weekly master without conflicts [Y] Sanity check I have doubts about the 'Reset to default' button in the user profile, IMO for the language string it seems that you are resetting your profile to the default values instead of resetting your profile page blocks distribution. If is interesting to have this button also in the profile page as you said maybe a more explicit language string for the user profile could be good In my/index.php, after resetting the page you are changing the context from user to system ( https://github.com/micaherne/moodle/commit/d8721fa25a793d529247f0f06724f9a96b3f4133#L1R100 ) but you are not allowed to do it (you can see a debugging message about it) a possible solution for this is to redirect() the user to my/index.php after the reset, this way the system defaults will be loaded and you will be using the user context
        Hide
        Richard van Iwaarden added a comment -

        Why is this not yet implemented? Is anyone working on this?

        Also, can someone give me an SQL statement that would reset all users tot the default 'my moodle' page the way I (as an admin) has set it up to be?

        Show
        Richard van Iwaarden added a comment - Why is this not yet implemented? Is anyone working on this? Also, can someone give me an SQL statement that would reset all users tot the default 'my moodle' page the way I (as an admin) has set it up to be?
        Hide
        Michael Aherne added a comment -

        Sorry, I had some peer review comments to deal with for this piece of work but it disappeared off my radar for some reason. I'll try to get the patch updated over the next week.

        Show
        Michael Aherne added a comment - Sorry, I had some peer review comments to deal with for this piece of work but it disappeared off my radar for some reason. I'll try to get the patch updated over the next week.
        Hide
        Michael Aherne added a comment - - edited

        I have made the changes suggested in the previous peer review (apart from moving the error string, which I wasn't sure about, and the peer reviewer wasn't sure about either):

        • Changed the language string to better describe what you're doing
        • Removed the trailing whitespace
        • Changed the logic to redirect after resetting
        • Added testing instructions and docs_required tag

        It has been so long since I got the previous peer review that it would be really useful if it could be peer reviewed again. (Apologies for the delay, by the way, I managed to completely lose this issue!)

        Show
        Michael Aherne added a comment - - edited I have made the changes suggested in the previous peer review (apart from moving the error string, which I wasn't sure about, and the peer reviewer wasn't sure about either): Changed the language string to better describe what you're doing Removed the trailing whitespace Changed the logic to redirect after resetting Added testing instructions and docs_required tag It has been so long since I got the previous peer review that it would be really useful if it could be peer reviewed again. (Apologies for the delay, by the way, I managed to completely lose this issue!)
        Hide
        David Monllaó added a comment -

        Removing myself as a PR, feel free to pick it as I'm currently working on other issues. Thanks.

        Show
        David Monllaó added a comment - Removing myself as a PR, feel free to pick it as I'm currently working on other issues. Thanks.
        Hide
        Richard van Iwaarden added a comment -

        Michael, I can not help you as we use a hosted Moodle. i can't install or alter anything.

        Still, do you have the SQL statement that could reset everyone to the default my Moodle page?

        Show
        Richard van Iwaarden added a comment - Michael, I can not help you as we use a hosted Moodle. i can't install or alter anything. Still, do you have the SQL statement that could reset everyone to the default my Moodle page?
        Hide
        Michael Aherne added a comment -

        Hi Richard, I don't have an SQL statement to do that, sorry.

        Show
        Michael Aherne added a comment - Hi Richard, I don't have an SQL statement to do that, sorry.
        Hide
        Dan Poltawski added a comment -

        Hi Michael,

        Thanks for taking this on!

        1. I think we will need behat tests for this new feature. Unfortunately it looks like we do not have tests for my moodle yet. However, we do have many course block tests, so I don't think that it will be too much to achieve this. If you need some help with this let me know.

        my/index.php:
        2. $userid = NULL; should be changed to lowercase null
        3. The spacing here doens't match the coding style:

        if(! $currentpage = my_reset_page($userid, MY_PAGE_PRIVATE)){
        

        my/lib.php:
        4. The lines look like there might be some overlength lines in my_reset_page
        5. There is trailing whitespace.

        user/profile.php
        6. There shouldn't be a comment with just a MDL number, better to explain what the branch is.

        Show
        Dan Poltawski added a comment - Hi Michael, Thanks for taking this on! 1. I think we will need behat tests for this new feature. Unfortunately it looks like we do not have tests for my moodle yet. However, we do have many course block tests, so I don't think that it will be too much to achieve this. If you need some help with this let me know. my/index.php: 2. $userid = NULL; should be changed to lowercase null 3. The spacing here doens't match the coding style: if (! $currentpage = my_reset_page($userid, MY_PAGE_PRIVATE)){ my/lib.php: 4. The lines look like there might be some overlength lines in my_reset_page 5. There is trailing whitespace. user/profile.php 6. There shouldn't be a comment with just a MDL number, better to explain what the branch is.
        Hide
        Michael Aherne added a comment -

        Thanks for the feedback, Dan! I've made changes 2-6 (2 wasn't actually part of my patch as such but I've fixed it anyway, and I just deleted the comment in 6 as it's really not necessary).

        I'm afraid I'm not really in a position to create behat tests for this at the moment. I'm aware of the behat framework and have had a quick look at it, but I don't have any experience of actually using it or any knowledge of how it's integrated into Moodle. I'll no doubt have to find out how it works at some point, but we're currently running 2.4 in our institution so I couldn't really justify the time I'd need to do this at the moment. Is there someone else who could write these?

        Show
        Michael Aherne added a comment - Thanks for the feedback, Dan! I've made changes 2-6 (2 wasn't actually part of my patch as such but I've fixed it anyway, and I just deleted the comment in 6 as it's really not necessary). I'm afraid I'm not really in a position to create behat tests for this at the moment. I'm aware of the behat framework and have had a quick look at it, but I don't have any experience of actually using it or any knowledge of how it's integrated into Moodle. I'll no doubt have to find out how it works at some point, but we're currently running 2.4 in our institution so I couldn't really justify the time I'd need to do this at the moment. Is there someone else who could write these?
        Hide
        Dan Poltawski added a comment -

        Yes, sure - I can have a go at it for you.

        Show
        Dan Poltawski added a comment - Yes, sure - I can have a go at it for you.
        Hide
        Michael Aherne added a comment -

        Thanks, Dan, that would be great!

        Show
        Michael Aherne added a comment - Thanks, Dan, that would be great!
        Hide
        Michael Aherne added a comment - - edited

        Hi Dan, I took the plunge and looked at the behat integration (which is brilliant ), and I've added some tests to the branch (https://github.com/micaherne/moodle/commit/46c2f3626d9b0f48175934fd18e0e5976f1e96ba). I think they're sane and they pass, but if you wouldn't mind having a quick look over them, that would be great. I'm not sure if you need them squished into a single commit for integration, but if you do just let me know and I'll get it done.

        Show
        Michael Aherne added a comment - - edited Hi Dan, I took the plunge and looked at the behat integration (which is brilliant ), and I've added some tests to the branch ( https://github.com/micaherne/moodle/commit/46c2f3626d9b0f48175934fd18e0e5976f1e96ba ). I think they're sane and they pass, but if you wouldn't mind having a quick look over them, that would be great. I'm not sure if you need them squished into a single commit for integration, but if you do just let me know and I'll get it done.
        Hide
        Dan Poltawski added a comment -

        Hi Michael,

        Great stuff - thanks for looking at behat.

        Regarding the feature files - I think you are on the right track, but I think that some of the background information is unnecessary (or should be used if taking the time to create the background):

        my/tests/behat/add_blocks.feature / my/tests/behat/reset_page.feature

        • I don't think there is a need for the course enrolment?
        • I think we need only one student to test with

        user/tests/behat/reset_page.feature / user/tests/behat/add_blocks.feature

        • Again, we are creating some users, but not testing anything with them - only using the admin. It would be better to do this as a student I think

        Finally - to make this even better, I think it would be good to customise the default my moodle page and test that this works. But feel free to ignore this comment as I don't think its directly related to this issue.

        thanks,
        Dan

        Show
        Dan Poltawski added a comment - Hi Michael, Great stuff - thanks for looking at behat. Regarding the feature files - I think you are on the right track, but I think that some of the background information is unnecessary (or should be used if taking the time to create the background): my/tests/behat/add_blocks.feature / my/tests/behat/reset_page.feature I don't think there is a need for the course enrolment? I think we need only one student to test with user/tests/behat/reset_page.feature / user/tests/behat/add_blocks.feature Again, we are creating some users, but not testing anything with them - only using the admin. It would be better to do this as a student I think Finally - to make this even better, I think it would be good to customise the default my moodle page and test that this works. But feel free to ignore this comment as I don't think its directly related to this issue. thanks, Dan
        Hide
        Michael Aherne added a comment -

        Thanks, Dan! I copied most of the background information from the blocks code, but didn't think to remove anything I wasn't using, so I'll look at doing that. The user tests are deliberately using the admin user as the student users didn't have permissions for any blocks to add to their profile page. I was a bit surprised at that, so I wonder if I need to fix something somewhere.

        Also, I noticed another problem with the tests is that I'm using steps like 'And I should not see "Reset page to default"', but I discovered after I uploaded the patch that the "I should see / not see" code doesn't find text in buttons, so this is passing, but isn't really testing anything! I'll see if I can work out how to check that a button exists and resubmit.

        Cheers

        Michael

        Show
        Michael Aherne added a comment - Thanks, Dan! I copied most of the background information from the blocks code, but didn't think to remove anything I wasn't using, so I'll look at doing that. The user tests are deliberately using the admin user as the student users didn't have permissions for any blocks to add to their profile page. I was a bit surprised at that, so I wonder if I need to fix something somewhere. Also, I noticed another problem with the tests is that I'm using steps like 'And I should not see "Reset page to default"', but I discovered after I uploaded the patch that the "I should see / not see" code doesn't find text in buttons, so this is passing, but isn't really testing anything! I'll see if I can work out how to check that a button exists and resubmit. Cheers Michael
        Hide
        Michael Aherne added a comment -

        Hi Dan, I've updated the tests to remove the extraneous data in the background, fixed my tests for the buttons, and added a simple test for the default my page.

        https://github.com/micaherne/moodle/commit/dee9fd2c8bbcb4416d4fc87f258b5520c635787d

        Show
        Michael Aherne added a comment - Hi Dan, I've updated the tests to remove the extraneous data in the background, fixed my tests for the buttons, and added a simple test for the default my page. https://github.com/micaherne/moodle/commit/dee9fd2c8bbcb4416d4fc87f258b5520c635787d
        Hide
        Dan Poltawski added a comment -

        Looks great, thanks a lot Michael - i'm sending it for integration

        Show
        Dan Poltawski added a comment - Looks great, thanks a lot Michael - i'm sending it for integration
        Hide
        Michael Aherne added a comment -

        Thanks, Dan. Do you need the commits squished into a single one or are multiple commits OK for integration?

        Show
        Michael Aherne added a comment - Thanks, Dan. Do you need the commits squished into a single one or are multiple commits OK for integration?
        Hide
        Dan Poltawski added a comment -

        Hi Michael,

        I think that they could be squished, but I don't feel strongly about it. (See http://docs.moodle.org/dev/Coding_style#Git_commits about our official guidance)

        One approach would be to add the behat tests proving the existing functionality as one commit, then a commit on top adding the new functionality with behat tests. But I don't think it matters too much.

        Show
        Dan Poltawski added a comment - Hi Michael, I think that they could be squished, but I don't feel strongly about it. (See http://docs.moodle.org/dev/Coding_style#Git_commits about our official guidance) One approach would be to add the behat tests proving the existing functionality as one commit, then a commit on top adding the new functionality with behat tests. But I don't think it matters too much.
        Hide
        Michael Aherne added a comment -

        OK, I'll leave them as they are!

        Show
        Michael Aherne added a comment - OK, I'll leave them as they are!
        Hide
        Marina Glancy added a comment -

        Hello, thanks a log Michael for working on it. Change looks very good. And behat

        I have two comments to the code:

        1. Processing of reset action needs session key validation "if ($reset && confirm_sesskey())", see https://github.com/moodle/moodle/blob/master/admin/blocks.php for examples.

        2. As far as I can see my_reset_page() will return false only if default page does not exist in DB. In which case the user's my page will be permanently broken from this moment on. So the error message 'There was an error resetting your page' is inaccurate. There was no error resetting the page, there is a Moodle setup error and it will be shown after user is redirected in the next line of code. I would just remove lines 126-130 and the return value from the function at all. And the new string is not necessary.

        3. You may argue with it but imho the "Reset page to default" button should appear only in editing mode and should have confirmation dialogue (like the one you have when you delete a single block from "My" page).

        I'm not reopening the issue, if you can provide fix soon enough I'll integrate it this week. Because otherwise it'll need to wait until after New Year.

        Thanks.

        Show
        Marina Glancy added a comment - Hello, thanks a log Michael for working on it. Change looks very good. And behat I have two comments to the code: 1. Processing of reset action needs session key validation "if ($reset && confirm_sesskey())", see https://github.com/moodle/moodle/blob/master/admin/blocks.php for examples. 2. As far as I can see my_reset_page() will return false only if default page does not exist in DB. In which case the user's my page will be permanently broken from this moment on. So the error message 'There was an error resetting your page' is inaccurate. There was no error resetting the page, there is a Moodle setup error and it will be shown after user is redirected in the next line of code. I would just remove lines 126-130 and the return value from the function at all. And the new string is not necessary. 3. You may argue with it but imho the "Reset page to default" button should appear only in editing mode and should have confirmation dialogue (like the one you have when you delete a single block from "My" page). I'm not reopening the issue, if you can provide fix soon enough I'll integrate it this week. Because otherwise it'll need to wait until after New Year. Thanks.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Based on Marina's comments and given it's already Thu... I'm reopening this for fixes and new integration cycle once ready. Thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Based on Marina's comments and given it's already Thu... I'm reopening this for fixes and new integration cycle once ready. 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
        Michael Aherne added a comment - - edited

        Hi Marina, thanks for your comments! I have implemented 1 & 2 as suggested and updated the pull request. As for 3, the reset button is only supposed to appear when in editing mode, and I haven't seen it appear otherwise. Can you give me steps to replicate this problem?

        I don't disagree that it would be nice to have a confirmation dialog but, having looked at the code for the block delete confirmation (in block_manager::process_url_delete()), I don't think this would be an easy or quick development. There are various lines of code in there that I don't really understand the ramifications of, and I'd find it hard to justify the time I'd need to understand this properly then to implement something similar for the reset button.

        Show
        Michael Aherne added a comment - - edited Hi Marina, thanks for your comments! I have implemented 1 & 2 as suggested and updated the pull request. As for 3, the reset button is only supposed to appear when in editing mode, and I haven't seen it appear otherwise. Can you give me steps to replicate this problem? I don't disagree that it would be nice to have a confirmation dialog but, having looked at the code for the block delete confirmation (in block_manager::process_url_delete() ), I don't think this would be an easy or quick development. There are various lines of code in there that I don't really understand the ramifications of, and I'd find it hard to justify the time I'd need to understand this properly then to implement something similar for the reset button.
        Hide
        CiBoT added a comment -

        Results for MDL-26680

        • Remote repository: git://github.com/micaherne/moodle.git
        Show
        CiBoT added a comment - Results for MDL-26680 Remote repository: git://github.com/micaherne/moodle.git Remote branch MDL-26680 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/346 Warning: The MDL-26680 -master branch at git://github.com/micaherne/moodle.git has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/346/artifact/work/smurf.html
        Hide
        Dan Poltawski added a comment -

        (sorry I missed this update and also the session key in my peer review).

        Sending for integration - Michael, I think you'll need to rebase though. Thanks!

        Show
        Dan Poltawski added a comment - (sorry I missed this update and also the session key in my peer review). Sending for integration - Michael, I think you'll need to rebase though. Thanks!
        Hide
        Michael Aherne added a comment -

        Cheers, Dan! That's it rebased.

        Show
        Michael Aherne added a comment - Cheers, Dan! That's it rebased.
        Hide
        Damyon Wiese added a comment -

        Hi Michael,

        This is a very good patch - thanks for working on it.

        A couple of very minor points:

        1. I can see the "Reset page to default" when I am not in editing mode (Will add screenshot). (Raised previously and not fixed).
        2. In standard - I see the two buttons side by side with no padding/margin between them and it looks very ugly. (Will add screenshot).
        3. In clean I see the buttons one above the other and it forces extra unused white space on the left hand side of the page - the buttons should appear next to each other with padding between them on all themes. (Will add screenshot).
        4. Very minor - but in behat feature files with only one scenario there is no need for a background section.

        I'll leave this issue open till Wednesday to see if you can get it updated in time for this week.

        Thanks again!

        Show
        Damyon Wiese added a comment - Hi Michael, This is a very good patch - thanks for working on it. A couple of very minor points: 1. I can see the "Reset page to default" when I am not in editing mode (Will add screenshot). (Raised previously and not fixed). 2. In standard - I see the two buttons side by side with no padding/margin between them and it looks very ugly. (Will add screenshot). 3. In clean I see the buttons one above the other and it forces extra unused white space on the left hand side of the page - the buttons should appear next to each other with padding between them on all themes. (Will add screenshot). 4. Very minor - but in behat feature files with only one scenario there is no need for a background section. I'll leave this issue open till Wednesday to see if you can get it updated in time for this week. Thanks again!
        Hide
        Damyon Wiese added a comment -

        And one more thing - don't forget to retest in RTL when you put the buttons on the same line as that is likely to require a separate css rule for RTL.

        Show
        Damyon Wiese added a comment - And one more thing - don't forget to retest in RTL when you put the buttons on the same line as that is likely to require a separate css rule for RTL.
        Hide
        Michael Aherne added a comment -

        Hi Damyon, thanks again for the feedback! I've fixed bug #1, and added style rules to base and bootstrapbase for #2 & #3 (which I tested with the Hebrew language pack and they appear to observe RTL properly). I haven't done anything about #4 as I don't have my behat environment set up on the machine I'm using at the moment, so I wouldn't be able to test it, but hoping we can live with it as-is.

        I also rebased, so if you feel it's ready to integrate it should be up to date.

        Cheers, Michael.

        Show
        Michael Aherne added a comment - Hi Damyon, thanks again for the feedback! I've fixed bug #1, and added style rules to base and bootstrapbase for #2 & #3 (which I tested with the Hebrew language pack and they appear to observe RTL properly). I haven't done anything about #4 as I don't have my behat environment set up on the machine I'm using at the moment, so I wouldn't be able to test it, but hoping we can live with it as-is. I also rebased, so if you feel it's ready to integrate it should be up to date. Cheers, Michael.
        Hide
        Damyon Wiese added a comment -

        Thanks Michael,

        Those changes look good and the behat thing is only minor.

        Integrated to master only.

        Show
        Damyon Wiese added a comment - Thanks Michael, Those changes look good and the behat thing is only minor. Integrated to master only.
        Hide
        Damyon Wiese added a comment -

        Note - I added a commit to add a "require_sesskey()" when reseting the page. It should really be there when copying a page for editing too - (git blames Martin for not adding it).

        Show
        Damyon Wiese added a comment - Note - I added a commit to add a "require_sesskey()" when reseting the page. It should really be there when copying a page for editing too - (git blames Martin for not adding it).
        Hide
        Damyon Wiese added a comment -

        I tested this in integration and all seems to work nicely.

        Thanks again!

        Show
        Damyon Wiese added a comment - I tested this in integration and all seems to work nicely. Thanks again!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I won't be saying "Thanks!" this week. I'm tired of it.

        For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely.

        Just closing this as fixed, ciao

        PS: Just a bit of black/cruel humor, sorry, LOL!

        Show
        Eloy Lafuente (stronk7) added a comment - I won't be saying "Thanks!" this week. I'm tired of it. For the good (and the bad), your code is now part of Moodle, the best LMS in the world. Hope you are contributing for that to continue being a fact (and not the opposite), sincerely. Just closing this as fixed, ciao PS: Just a bit of black/cruel humor, sorry, LOL!
        Hide
        Hubert Chathi added a comment -

        Since Eloy won't say thanks I will. Thanks to Michael for working on this. I wish I had some time to work on My Moodle, but I'm glad that it's not completely dead.

        Show
        Hubert Chathi added a comment - Since Eloy won't say thanks I will. Thanks to Michael for working on this. I wish I had some time to work on My Moodle, but I'm glad that it's not completely dead.
        Hide
        Dan Poltawski added a comment -

        Yes, thanks a lot for your perseverance Michael Aherne!

        Show
        Dan Poltawski added a comment - Yes, thanks a lot for your perseverance Michael Aherne !
        Hide
        Michael Aherne added a comment -

        Thanks, guys!

        Show
        Michael Aherne added a comment - Thanks, guys!

          People

          • Votes:
            42 Vote for this issue
            Watchers:
            35 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: