Moodle
  1. Moodle
  2. MDL-19430

user-determined order and number of courses on myMoodle

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.9.4, 1.9.5, 2.2.1
    • Fix Version/s: 2.4
    • Component/s: My home
    • Environment:
      Vista SP2, Apache 2.2.11, PHP 5.2.8 , MySQL 5.1.30
    • Database:
      MySQL
    • Testing Instructions:
      Hide
      1. Install patch
      2. Log in as user who is enrolled in 10 courses
      3. Go to "My home" page
      4. Click "Customise this page" and limit courses and make sure only those courses are visible
      5. Re-order courses and make sure order is saved
      6. Enable ajax and try drag-n-drop to reorder course on my home page and make sure it works
      7. Change configuration of "course_overview" block and make sure it works.
      Show
      Install patch Log in as user who is enrolled in 10 courses Go to "My home" page Click "Customise this page" and limit courses and make sure only those courses are visible Re-order courses and make sure order is saved Enable ajax and try drag-n-drop to reorder course on my home page and make sure it works Change configuration of "course_overview" block and make sure it works.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-mdl-19430
    • Rank:
      5334

      Description

      The current myMoodle centre column lists up to 21 courses a user has access to, and if there are more there is no way to change which ones are listed. Courses are listed in the order in which they are retrieved from the database.

      This patch will give users the option to:

      • choose the order in which courses are listed in the centre column of the myMoodle page,
      • set how many courses shall be displayed when loading myMoodle.

        Issue Links

          Activity

          Hide
          Minh-Tam Nguyen added a comment -

          parts of this sort-of go in the same direction as shown in the myMoodle-myCourses mockup in MDL-19124

          Show
          Minh-Tam Nguyen added a comment - parts of this sort-of go in the same direction as shown in the myMoodle-myCourses mockup in MDL-19124
          Hide
          Minh-Tam Nguyen added a comment -

          only a certain number of courses is displayed in normal mode

          Show
          Minh-Tam Nguyen added a comment - only a certain number of courses is displayed in normal mode
          Hide
          Minh-Tam Nguyen added a comment -

          all courses are displayed in editing mode. THe ser can move courses up and down and choose how many should be displayed when editing is turned off

          Show
          Minh-Tam Nguyen added a comment - all courses are displayed in editing mode. THe ser can move courses up and down and choose how many should be displayed when editing is turned off
          Hide
          Minh-Tam Nguyen added a comment -

          Attached file contains the local hack code so far. Effects demonstrated in screen shots.
          Mind you, it's still not done. The code is quite ugly and will need some cleaning up, and more importantly, some comments.

          The code in the zip needs to go to /moodle/local/. in addition to this, my code relies on
          http://tracker.moodle.org/browse/MDL-17446 ("LOCAL: my moodle centre column override") which Penny Leach wrote in December.

          Another thing that's not quite ready for prime time yet, is that I haven't created a database xml file in /moodle/local/db/ yet. It will come, though.
          As it is at the moment, the attached code requires the manual (by admin) creation of two new additional profile fields:
          "myNumCourses"
          "myorder"
          both are straight text fields (not text input), and it's clearly visible that I need to fix the names to be firstly consistent, and secondly in agreement with the coding guidelines.

          If you risk installing this code as it is right now, users will get two and a half new features on myMoodle:
          1: users have the option to change the order of courses on myMoodle by clicking up and down arrows while in editing mode.
          2: user can choose how many courses are displayed on myMoodle by changing the number in the drop-down box while in editing mode.
          2 1/2: course details such as due assignments are initially collapsed when the myMoodle page is loaded. they can be unfolded by clicking an icon.

          Show
          Minh-Tam Nguyen added a comment - Attached file contains the local hack code so far. Effects demonstrated in screen shots. Mind you, it's still not done. The code is quite ugly and will need some cleaning up, and more importantly, some comments. The code in the zip needs to go to /moodle/local/. in addition to this, my code relies on http://tracker.moodle.org/browse/MDL-17446 ("LOCAL: my moodle centre column override") which Penny Leach wrote in December. Another thing that's not quite ready for prime time yet, is that I haven't created a database xml file in /moodle/local/db/ yet. It will come, though. As it is at the moment, the attached code requires the manual (by admin) creation of two new additional profile fields: "myNumCourses" "myorder" both are straight text fields (not text input), and it's clearly visible that I need to fix the names to be firstly consistent, and secondly in agreement with the coding guidelines. If you risk installing this code as it is right now, users will get two and a half new features on myMoodle: 1: users have the option to change the order of courses on myMoodle by clicking up and down arrows while in editing mode. 2: user can choose how many courses are displayed on myMoodle by changing the number in the drop-down box while in editing mode. 2 1/2: course details such as due assignments are initially collapsed when the myMoodle page is loaded. they can be unfolded by clicking an icon.
          Hide
          Minh-Tam Nguyen added a comment -

          as this is a local hack, it requires MDL-17446

          Show
          Minh-Tam Nguyen added a comment - as this is a local hack, it requires MDL-17446
          Hide
          Minh-Tam Nguyen added a comment -

          the patch at the moment also needs an addition to styles_layout.css in your favourite (or standard) theme. THis will need to be moved to local as well, I suppose.

          Show
          Minh-Tam Nguyen added a comment - the patch at the moment also needs an addition to styles_layout.css in your favourite (or standard) theme. THis will need to be moved to local as well, I suppose.
          Hide
          Minh-Tam Nguyen added a comment -

          discussion about this is here: http://moodle.org/mod/forum/discuss.php?d=122236

          Show
          Minh-Tam Nguyen added a comment - discussion about this is here: http://moodle.org/mod/forum/discuss.php?d=122236
          Hide
          Minh-Tam Nguyen added a comment -

          I have uploaded a new package of files. The code has some better comments now and the style looks nicer.
          This still requires manual creation of profile fields and an edit to the standard style sheet, unfortunately.

          Show
          Minh-Tam Nguyen added a comment - I have uploaded a new package of files. The code has some better comments now and the style looks nicer. This still requires manual creation of profile fields and an edit to the standard style sheet, unfortunately.
          Hide
          Minh-Tam Nguyen added a comment -

          I reformulated the install.txt to be a little bit clearer and changed a few words and links.

          Show
          Minh-Tam Nguyen added a comment - I reformulated the install.txt to be a little bit clearer and changed a few words and links.
          Hide
          Mark Pearson added a comment -

          Minh-Tam,
          I got rather confused attempting to apply this patch to a 1.9.5 installation. The 'Affected Branches:" field above indicates MOODLE_19_STABLE so I was surprised to find reference in the patch file to a root level directory called 'local' which is not in the current 1.9.5 distribution. Moreover, Penny Leach's patch at MDL-17446 implies MOODLE_20_STABLE and I'm assuming that this directory is present in 2.0. However, I am not going to attempt to bring up version 2.0 as a production server at this point in development, so I'm wondering how to proceed. Do you have this 'backported' (I think that's the phrase) to 1.9.5? Do you have a moodle/index.php and moodle/local/lib.php file which is 1.9.5 compatible to use with your patch? Thanks for any help.
          Mark

          Show
          Mark Pearson added a comment - Minh-Tam, I got rather confused attempting to apply this patch to a 1.9.5 installation. The 'Affected Branches:" field above indicates MOODLE_19_STABLE so I was surprised to find reference in the patch file to a root level directory called 'local' which is not in the current 1.9.5 distribution. Moreover, Penny Leach's patch at MDL-17446 implies MOODLE_20_STABLE and I'm assuming that this directory is present in 2.0. However, I am not going to attempt to bring up version 2.0 as a production server at this point in development, so I'm wondering how to proceed. Do you have this 'backported' (I think that's the phrase) to 1.9.5? Do you have a moodle/index.php and moodle/local/lib.php file which is 1.9.5 compatible to use with your patch? Thanks for any help. Mark
          Hide
          Mark Pearson added a comment -

          This is the patch file referenced below from Penny Leach which implements custom handler code for display by MyMoodle. It's fixed for 1.9.5

          Show
          Mark Pearson added a comment - This is the patch file referenced below from Penny Leach which implements custom handler code for display by MyMoodle. It's fixed for 1.9.5
          Hide
          Mark Pearson added a comment -

          So, yes, I did get rather confused but I think I've sussed it out:
          1. If you're on Moodle v1.9.5 (like what I am) use the patch file above like this:
          a. get the file into your moodle system and move it into the moodle/my directory
          b. apply with $ patch <hook-for-a-local-my-moodle-195.patch
          2. Now download and unpack the above ZIP file and follow instructions in the install.txt. This will provide the handler referred to by the patch file above (which is what got me rather confused)
          Hope this helps.

          Show
          Mark Pearson added a comment - So, yes, I did get rather confused but I think I've sussed it out: 1. If you're on Moodle v1.9.5 (like what I am) use the patch file above like this: a. get the file into your moodle system and move it into the moodle/my directory b. apply with $ patch <hook-for-a-local-my-moodle-195.patch 2. Now download and unpack the above ZIP file and follow instructions in the install.txt. This will provide the handler referred to by the patch file above (which is what got me rather confused) Hope this helps.
          Hide
          Mark Pearson added a comment -

          To make life easier I've created a patch for the styles_layout.css. To apply:
          Download to your moodle site with ftp or scp
          $ cd moodle/theme/standard
          $ mv ../../mymoodle-styles-layout-css.patch .
          $ patch <mymoodle-styles-layout-css.patch
          That should do it! Nice system Minh-Tam! I like it.

          Show
          Mark Pearson added a comment - To make life easier I've created a patch for the styles_layout.css. To apply: Download to your moodle site with ftp or scp $ cd moodle/theme/standard $ mv ../../mymoodle-styles-layout-css.patch . $ patch <mymoodle-styles-layout-css.patch That should do it! Nice system Minh-Tam! I like it.
          Hide
          Minh-Tam Nguyen added a comment -

          I uploaded a new package of files: MDL-19430 (20090818).zip

          I have made a number of changes to improve security, maintainability and performance, as suggested by Ashley Holman.

          I have also made changes to the install.txt to hopefully avoid the confusion Mark went through.

          This package is straight from of my installation of 1.9.5, and Penny's patch also works fine on 1.9.5, once you create the 'local' folder.

          Show
          Minh-Tam Nguyen added a comment - I uploaded a new package of files: MDL-19430 (20090818).zip I have made a number of changes to improve security, maintainability and performance, as suggested by Ashley Holman. I have also made changes to the install.txt to hopefully avoid the confusion Mark went through. This package is straight from of my installation of 1.9.5, and Penny's patch also works fine on 1.9.5, once you create the 'local' folder.
          Hide
          Mark Pearson added a comment -

          I downloaded the MDL-19430 (20090818).zip and substituted it into a working 1.9.5 system where the previous version had worked fine. I found two errors:
          1) The pulldown menu for number of courses always has "10" at the bottom (not there before)
          2) Switch on debug in administration and get the following notices:

          Notice: Undefined variable: site in /usr/home/markp/public_html/moo-195/local/lib.php on line 169
          Notice: Trying to get property of non-object in /usr/home/markp/public_html/moo-195/local/lib.php on line 169

          I thought you should know.

          Show
          Mark Pearson added a comment - I downloaded the MDL-19430 (20090818).zip and substituted it into a working 1.9.5 system where the previous version had worked fine. I found two errors: 1) The pulldown menu for number of courses always has "10" at the bottom (not there before) 2) Switch on debug in administration and get the following notices: Notice: Undefined variable: site in /usr/home/markp/public_html/moo-195/local/lib.php on line 169 Notice: Trying to get property of non-object in /usr/home/markp/public_html/moo-195/local/lib.php on line 169 I thought you should know.
          Hide
          Minh-Tam Nguyen added a comment -

          Thank you Mark for those two points.
          The extra 10 (if the user has access to less than 10 courses) was mainly to allow people to set a higher number than they currently have, in anticipation of soon getting more.
          However your comment made me realise that this wasn't the right solution. Instead, I have now changed it to be "Always show all", 1, 2, 3, etc. up to the number of courses. THis shoudl allow users to set it to 'Always all' and know that new courses will show.
          I have also changed the code to avoid that error message. I guess I was a little too keen in removing unused variables.
          CHeers,
          Minh-Tam

          Show
          Minh-Tam Nguyen added a comment - Thank you Mark for those two points. The extra 10 (if the user has access to less than 10 courses) was mainly to allow people to set a higher number than they currently have, in anticipation of soon getting more. However your comment made me realise that this wasn't the right solution. Instead, I have now changed it to be "Always show all", 1, 2, 3, etc. up to the number of courses. THis shoudl allow users to set it to 'Always all' and know that new courses will show. I have also changed the code to avoid that error message. I guess I was a little too keen in removing unused variables. CHeers, Minh-Tam
          Hide
          Mark Pearson added a comment -

          Yep. That fixed it! This is a brilliant modification to the MyMoodle interface and changes the whole way that we use Moodle. No longer do I have to 'hide' courses from teachers – they can organise the display any way they want. I hope that this feature gets included in any MyMoodle improvements (certainly for 2.0).
          Cheers!
          Mark

          Show
          Mark Pearson added a comment - Yep. That fixed it! This is a brilliant modification to the MyMoodle interface and changes the whole way that we use Moodle. No longer do I have to 'hide' courses from teachers – they can organise the display any way they want. I hope that this feature gets included in any MyMoodle improvements (certainly for 2.0). Cheers! Mark
          Hide
          Minh-Tam Nguyen added a comment -

          I have uploaded a new version of the files, with changes addressing most of the points raised in the discussion forum: http://moodle.org/mod/forum/discuss.php?d=122236
          When in edit mode, there now is a greater delineation to show which courses will be shown and which ones won't, and the language files have been changed a little. The logic relating to the display of up and down arrows was changed a little, too.
          Cheers,
          Minh-Tam

          Show
          Minh-Tam Nguyen added a comment - I have uploaded a new version of the files, with changes addressing most of the points raised in the discussion forum: http://moodle.org/mod/forum/discuss.php?d=122236 When in edit mode, there now is a greater delineation to show which courses will be shown and which ones won't, and the language files have been changed a little. The logic relating to the display of up and down arrows was changed a little, too. Cheers, Minh-Tam
          Hide
          Minh-Tam Nguyen added a comment -

          This is the version we have now running on production.
          I also made a very quick video of how it works here: http://www.minhtam.info/a/screens/MDL-19430_%2820090904%29.swf

          Show
          Minh-Tam Nguyen added a comment - This is the version we have now running on production. I also made a very quick video of how it works here: http://www.minhtam.info/a/screens/MDL-19430_%2820090904%29.swf
          Hide
          Tim Lock added a comment -

          I have found that if a student blocks a contact, the unread messages are still counted in the MyMoodle page when it should only counting non-blocked contact messages with something like this :-

          select m.id,useridfrom,useridto,blocked from mdl_message m left join
          mdl_message_contacts mc on m.useridfrom=mc.contactid where m.useridto=XXXX
          and mc.userid=XXXX and mc.blocked=0;

          Current query is only checking for unread messages by :-

          if ($countmessages = count_records('message', 'useridto', $USER->id)) {

          Show
          Tim Lock added a comment - I have found that if a student blocks a contact, the unread messages are still counted in the MyMoodle page when it should only counting non-blocked contact messages with something like this :- select m.id,useridfrom,useridto,blocked from mdl_message m left join mdl_message_contacts mc on m.useridfrom=mc.contactid where m.useridto=XXXX and mc.userid=XXXX and mc.blocked=0; Current query is only checking for unread messages by :- if ($countmessages = count_records('message', 'useridto', $USER->id)) {
          Hide
          Michael de Raadt added a comment -

          This appears to still be a problem and people have duplicated this issue.

          I'm bumping the priority for this issue and linking a few issues to it.

          Show
          Michael de Raadt added a comment - This appears to still be a problem and people have duplicated this issue. I'm bumping the priority for this issue and linking a few issues to it.
          Hide
          Neill Magill added a comment -

          The code above does not seem to be quickly useful for 2.x.

          We have been having issues with some of the limits of the course overview block as well, so I did some development that allows the block to be more useful.

          https://github.com/NeillM/moodle/compare/master...course_overview_fix

          The changes allow the user to set how many courses they with to displayy at one time, change the order that courses are displayed in, based on one of several fields.

          If the user is enrolled on more courses than the display limit it will let them page back and forth through their courses.

          Show
          Neill Magill added a comment - The code above does not seem to be quickly useful for 2.x. We have been having issues with some of the limits of the course overview block as well, so I did some development that allows the block to be more useful. https://github.com/NeillM/moodle/compare/master...course_overview_fix The changes allow the user to set how many courses they with to displayy at one time, change the order that courses are displayed in, based on one of several fields. If the user is enrolled on more courses than the display limit it will let them page back and forth through their courses.
          Hide
          Minh-Tam Nguyen added a comment -

          Moodle 2 version of this patch by Netspot for University of Canberra and Flinders University:
          https://github.com/netspotau/moodle-block_my_course_overview

          Show
          Minh-Tam Nguyen added a comment - Moodle 2 version of this patch by Netspot for University of Canberra and Flinders University: https://github.com/netspotau/moodle-block_my_course_overview
          Hide
          Rajesh Taneja added a comment -

          Adding this to next sprint

          Show
          Rajesh Taneja added a comment - Adding this to next sprint
          Hide
          Rajesh Taneja added a comment -

          Thanks for the update Minh

          Hi Rajesh,
          it's great to see that this has been added to the next sprint. Thanks.
          When you're working on it, please be aware that the Moodle 2.x version of this moduls is on github at https://github.com/netspotau/moodle-block_my_course_overview
          I have observed one performance issue in both this myMoodle block but also in the core myMoodle, relating to users with more than 20 courses showing, due to Moodle querying so any courses.
          Cheers,
          Minh-Tam

          Show
          Rajesh Taneja added a comment - Thanks for the update Minh Hi Rajesh, it's great to see that this has been added to the next sprint. Thanks. When you're working on it, please be aware that the Moodle 2.x version of this moduls is on github at https://github.com/netspotau/moodle-block_my_course_overview I have observed one performance issue in both this myMoodle block but also in the core myMoodle, relating to users with more than 20 courses showing, due to Moodle querying so any courses. Cheers, Minh-Tam
          Hide
          Mark Pearson added a comment -

          It's great to see this being fixed in 2.x. Thanks.
          Mark

          Show
          Mark Pearson added a comment - It's great to see this being fixed in 2.x. Thanks. Mark
          Hide
          Adam Olley added a comment -

          Hi All,
          https://github.com/netspotau/moodle-block_my_course_overview has been updated with some fixes:

          • Add note about hidden courses when editing
          • Fix icon positioning in some browsers
          • Switch icon to match correct moving style (move_2d for ajax moving)
          Show
          Adam Olley added a comment - Hi All, https://github.com/netspotau/moodle-block_my_course_overview has been updated with some fixes: Add note about hidden courses when editing Fix icon positioning in some browsers Switch icon to match correct moving style (move_2d for ajax moving)
          Hide
          Mark Drechsler added a comment -

          Woohoo!

          Great news Rajesh

          Mark.

          Show
          Mark Drechsler added a comment - Woohoo! Great news Rajesh Mark.
          Hide
          Rajesh Taneja added a comment -

          Thanks Adam and Mark

          Show
          Rajesh Taneja added a comment - Thanks Adam and Mark
          Hide
          Rajesh Taneja added a comment -

          Looking at this in 2.x functionality has changed a bit.
          Not sure if this improvement will raise bug. In 2.x $CFG->mycoursesperpage has been added which allow admin to set max courses for user. Also, Admin can change the order in which user should view the courses (Settings -> Site administration -> courses -> Add/edit courses)

          I think we need more discussion, before we plan to implement this. As this will affect category sorting as well.
          Currently admin can set the max visible courses on my moodle page, and set the order in which courses are displayed to user (sorted by category and then courses within category). Sorting is set from admin section.

          FYI:
          Unfortunately we are only supporting 1.9 for security patches, so I am not considering 1.9 implementation.

          Show
          Rajesh Taneja added a comment - Looking at this in 2.x functionality has changed a bit. Not sure if this improvement will raise bug. In 2.x $CFG->mycoursesperpage has been added which allow admin to set max courses for user. Also, Admin can change the order in which user should view the courses (Settings -> Site administration -> courses -> Add/edit courses) I think we need more discussion, before we plan to implement this. As this will affect category sorting as well. Currently admin can set the max visible courses on my moodle page, and set the order in which courses are displayed to user (sorted by category and then courses within category). Sorting is set from admin section. FYI: Unfortunately we are only supporting 1.9 for security patches, so I am not considering 1.9 implementation.
          Hide
          Rajesh Taneja added a comment -

          Sorry, didn't get much time to look at this. Moving it to next sprint.

          Show
          Rajesh Taneja added a comment - Sorry, didn't get much time to look at this. Moving it to next sprint.
          Hide
          Rajesh Taneja added a comment -

          Estimate effort for working on this: 14hr.

          Show
          Rajesh Taneja added a comment - Estimate effort for working on this: 14hr.
          Hide
          Rajesh Taneja added a comment -

          Thanks Adam for the awesome code.
          I have done few changes and will push it for review now.

          Show
          Rajesh Taneja added a comment - Thanks Adam for the awesome code. I have done few changes and will push it for review now.
          Hide
          David Monllaó added a comment -

          Hi,

          The first commit gets rid of the remote courses https://github.com/rajeshtaneja/moodle/commit/a7da3061144e6c67f67f196a17b8ca45c0e3732b#L1L71 so it seems to be a loss of funcionality; this will also affect the JS methods, which are based only in local courses ids without taking into account the mnethostid

          Show
          David Monllaó added a comment - Hi, The first commit gets rid of the remote courses https://github.com/rajeshtaneja/moodle/commit/a7da3061144e6c67f67f196a17b8ca45c0e3732b#L1L71 so it seems to be a loss of funcionality; this will also affect the JS methods, which are based only in local courses ids without taking into account the mnethostid
          Hide
          Rajesh Taneja added a comment -

          Thanks for pointing that David,

          I have updated the patch to show remote courses.

          Show
          Rajesh Taneja added a comment - Thanks for pointing that David, I have updated the patch to show remote courses.
          Hide
          David Monllaó added a comment -

          All ok Rajesh, the only thing I see according to http://docs.moodle.org/dev/Peer_reviewing_checklist is that there is no sesskey checking in the move.php action

          Thanks

          Show
          David Monllaó added a comment - All ok Rajesh, the only thing I see according to http://docs.moodle.org/dev/Peer_reviewing_checklist is that there is no sesskey checking in the move.php action Thanks
          Hide
          Rajesh Taneja added a comment -

          Thanks David,

          Session check added in move.php

          Show
          Rajesh Taneja added a comment - Thanks David, Session check added in move.php
          Hide
          David Monllaó added a comment -

          Thanks Raj, all looks good for me

          Show
          David Monllaó added a comment - Thanks Raj, all looks good for me
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've been having a good look at this and have made the following notes.
          There are some cool things happening here but this still needs more polishing sorry.

          1. README.php - looks like its a copy of the docs page for this block? Would be great to simplfy that to what is needed or convert it to a docs page and remove it entirely. This would be the only block with a readme so perhaps best to remove it.
          2. @package should be block_course_overview.
          3. A few vars with underscores in their names. Just general coding style guff.
          4. block_course_overview.php
            1. + why set instance_can_be_hidden to true?
            2. + remove closing ?>
          5. locallib.php
            1. block_course_overview_get_overviews unused global vars
            2. + Should probably be using get_plugin_list_with_function rather than manually finding modules with callbacks.
            3. block_course_overview_get_child_shortnames unused globals
            4. + that is function is manually constructing a string (is complex but needs to be translatable).
            5. + looks like it could be using get_records_sql_menu rather than iterating to build $shortnames.
            6. + those course shortnames need to be run through format_string.
          6. renderer.php
            1. inclusion of message/lib.php should be moved to welcome_area method (looks like that is the only use) or much better yet included when needed in block_course_overview.php and the message count passed in as an argument (renderers should contain little to no logic).
            2. Please don't use globals within a renderer. $OUTPUT is available through $this->output and $PAGE is available through $this->page.
            3. course_overview moving the following calls out of the foreach will be good for performance. $this->pix_url, ajaxenabled and perhaps get_string if you can be bothered (not nearly as important as the other two).
            4. + there is a <br> that should be a <br /> (I'm just guessing xhtml)
            5. + course->fullname needs to be run through format_string (filters etc) please ensure you provide the context as an arg (required for new format calls)
            6. collapsible_region_start unused globals
            7. activity_display activity_display.$module strings need to be coming from the modules and not from the block_course_overview lang file. I also think they should be passed in rather than fetched within the render as renderers should contain minimal logic.
          7. save.php
            1. use require_sesskey instead of confirm_sesskey.
            2. MUST require_login there
          8. move.php
            1. use require_sesskey instead of confirm_sesskey.
            2. MUST require_login there
            3. The two debugging calls look like they should be exceptions. Assuming you don't hit that path through normal operation.
          9. settings.php
            1. Rather than iterating over $configs to set the plugin change the name of each setting to: block_course_overview/settingname. That way you don't need to manually set the plugin.
          10. styles.css
            1. Use of font-size property is undesireable. Especially h2.title which uses !important. We need to test this with all core themes if we are going to set that.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've been having a good look at this and have made the following notes. There are some cool things happening here but this still needs more polishing sorry. README.php - looks like its a copy of the docs page for this block? Would be great to simplfy that to what is needed or convert it to a docs page and remove it entirely. This would be the only block with a readme so perhaps best to remove it. @package should be block_course_overview. A few vars with underscores in their names. Just general coding style guff. block_course_overview.php + why set instance_can_be_hidden to true? + remove closing ?> locallib.php block_course_overview_get_overviews unused global vars + Should probably be using get_plugin_list_with_function rather than manually finding modules with callbacks. block_course_overview_get_child_shortnames unused globals + that is function is manually constructing a string (is complex but needs to be translatable). + looks like it could be using get_records_sql_menu rather than iterating to build $shortnames. + those course shortnames need to be run through format_string. renderer.php inclusion of message/lib.php should be moved to welcome_area method (looks like that is the only use) or much better yet included when needed in block_course_overview.php and the message count passed in as an argument (renderers should contain little to no logic). Please don't use globals within a renderer. $OUTPUT is available through $this->output and $PAGE is available through $this->page. course_overview moving the following calls out of the foreach will be good for performance. $this->pix_url, ajaxenabled and perhaps get_string if you can be bothered (not nearly as important as the other two). + there is a <br> that should be a <br /> (I'm just guessing xhtml) + course->fullname needs to be run through format_string (filters etc) please ensure you provide the context as an arg (required for new format calls) collapsible_region_start unused globals activity_display activity_display.$module strings need to be coming from the modules and not from the block_course_overview lang file. I also think they should be passed in rather than fetched within the render as renderers should contain minimal logic. save.php use require_sesskey instead of confirm_sesskey. MUST require_login there move.php use require_sesskey instead of confirm_sesskey. MUST require_login there The two debugging calls look like they should be exceptions. Assuming you don't hit that path through normal operation. settings.php Rather than iterating over $configs to set the plugin change the name of each setting to: block_course_overview/settingname. That way you don't need to manually set the plugin. styles.css Use of font-size property is undesireable. Especially h2.title which uses !important. We need to test this with all core themes if we are going to set that. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,

          I will update the code.

          Show
          Rajesh Taneja added a comment - Thanks Sam, I will update the code.
          Hide
          Rajesh Taneja added a comment -

          Carrying it over to next sprint.

          Show
          Rajesh Taneja added a comment - Carrying it over to next sprint.
          Hide
          Rajesh Taneja added a comment -

          Thanks for the comments Sam,

          I have updated code with most of your feedback. Few questions:
          4-1 Not sure, currently it is set to false.
          5-6 It is going through format_string, do I need something else ?
          6-7 It is done for custom message for different modules. If not found then custom message is shown. Do you want me to move these strings to corresponding modules?

          Show
          Rajesh Taneja added a comment - Thanks for the comments Sam, I have updated code with most of your feedback. Few questions: 4-1 Not sure, currently it is set to false. 5-6 It is going through format_string, do I need something else ? 6-7 It is done for custom message for different modules. If not found then custom message is shown. Do you want me to move these strings to corresponding modules?
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          4.1. Perhaps best to look into that and find out why its now being changed. If there is a good reason no probs at all, but if its just a random change then perhaps best to leave it as false.

          5.6. https://github.com/rajeshtaneja/moodle/blob/3877daee439a0d0a8de08bc5484823bfe730da7a/blocks/course_overview/locallib.php#L76 Each of those shortnames would need to be formatted by itself using the context of the course they belong to. Probably best to preload contexts in the SQL statement
          The string as a whole needs to be converted to a lang string, and should not be run through format_string once that is done.

          6.7. Yip, those strings need to be coming from the modules themselves, so that if someone writes their own module they can define a string that will be used by the course overview block. Of course it should still check the string exists and use the default string if not.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, 4.1. Perhaps best to look into that and find out why its now being changed. If there is a good reason no probs at all, but if its just a random change then perhaps best to leave it as false. 5.6. https://github.com/rajeshtaneja/moodle/blob/3877daee439a0d0a8de08bc5484823bfe730da7a/blocks/course_overview/locallib.php#L76 Each of those shortnames would need to be formatted by itself using the context of the course they belong to. Probably best to preload contexts in the SQL statement The string as a whole needs to be converted to a lang string, and should not be run through format_string once that is done. 6.7. Yip, those strings need to be coming from the modules themselves, so that if someone writes their own module they can define a string that will be used by the course overview block. Of course it should still check the string exists and use the default string if not. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          David, Can you please review this again.

          I have integrated all comments, suggested by Sam.

          Show
          Rajesh Taneja added a comment - David, Can you please review this again. I have integrated all comments, suggested by Sam.
          Hide
          David Monllaó added a comment -

          Hi Raj, it looks ok to me

          Show
          David Monllaó added a comment - Hi Raj, it looks ok to me
          Hide
          Rajesh Taneja added a comment -

          Thanks David,

          Pushing it back for integration review.

          Show
          Rajesh Taneja added a comment - Thanks David, Pushing it back for integration review.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Rajesh Taneja added a comment -

          Branch re-based.

          Show
          Rajesh Taneja added a comment - Branch re-based.
          Hide
          Sam Hemelryk added a comment -

          Thanks Raj + everyone!
          This has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Raj + everyone! This has been integrated now
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,

          It's nice to see this patch in moodle

          Thanks everyone, especially Adam for sharing this patch and thoughts.

          Show
          Rajesh Taneja added a comment - Thanks Sam, It's nice to see this patch in moodle Thanks everyone, especially Adam for sharing this patch and thoughts.
          Hide
          Frédéric Massart added a comment -

          The test passes but there is a minor error with the spacer image.

          blocks/course_overview/renderer.php:90:
          $html .= html_writer::empty_tag('img', array('src' => $this->pix_url('t/spacer'),
          
          should be
          
          $html .= html_writer::empty_tag('img', array('src' => $this->pix_url('spacer'),
          

          If someone could push that in, I'll pass the test.

          Show
          Frédéric Massart added a comment - The test passes but there is a minor error with the spacer image. blocks/course_overview/renderer.php:90: $html .= html_writer::empty_tag('img', array('src' => $this->pix_url('t/spacer'), should be $html .= html_writer::empty_tag('img', array('src' => $this->pix_url('spacer'), If someone could push that in, I'll pass the test.
          Hide
          Rajesh Taneja added a comment -

          Thanks Fred for spotting that.

          @Sam: I have added an other commit, can you please pull that.

          Show
          Rajesh Taneja added a comment - Thanks Fred for spotting that. @Sam: I have added an other commit, can you please pull that.
          Hide
          Sam Hemelryk added a comment -

          Thanks for spotting Fred, and thanks for creating a fix Raj.
          I've merged the fix in now and its all ready to be pulled and tested again.

          Show
          Sam Hemelryk added a comment - Thanks for spotting Fred, and thanks for creating a fix Raj. I've merged the fix in now and its all ready to be pulled and tested again.
          Hide
          Frédéric Massart added a comment -

          Test successful on master. Yeepee!

          Show
          Frédéric Massart added a comment - Test successful on master. Yeepee!
          Hide
          Aparup Banerjee added a comment -

          yay, it works!

          This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

          Thank you all for taking the time to get us here.

          cheers!

          Show
          Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!
          Hide
          Gordon McLeod added a comment -

          Hi,
          Just wanted to check - is there any step-by-step documentation on installing and using this? Our users are very keen to customise the My Moodle page, but I'm not clear if this will run in 2.3 or if we'll need to wait until next year when we plan to upgrade to 2.4, whether I also need to install the MDL-17446 patch mentioned or if that will come as part of the git clone, and where the files should be installed/cloned to?
          Thanks, Gordon.

          Show
          Gordon McLeod added a comment - Hi, Just wanted to check - is there any step-by-step documentation on installing and using this? Our users are very keen to customise the My Moodle page, but I'm not clear if this will run in 2.3 or if we'll need to wait until next year when we plan to upgrade to 2.4, whether I also need to install the MDL-17446 patch mentioned or if that will come as part of the git clone, and where the files should be installed/cloned to? Thanks, Gordon.
          Hide
          Rajesh Taneja added a comment -

          Hello Gordon,

          This should run on 2.3. No you don't have to install MDL-17446.
          You can use git cherry-pick and pick two commits from git.moodle.org (http://git.moodle.org/gw?p=moodle.git&a=search&h=HEAD&st=commit&s=mdl-19430)

          Let us know if you need more help on this.

          Show
          Rajesh Taneja added a comment - Hello Gordon, This should run on 2.3. No you don't have to install MDL-17446 . You can use git cherry-pick and pick two commits from git.moodle.org ( http://git.moodle.org/gw?p=moodle.git&a=search&h=HEAD&st=commit&s=mdl-19430 ) Let us know if you need more help on this.
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as this is documented now in http://docs.moodle.org/24/en/My_Moodle

          Show
          Mary Cooch added a comment - Removing docs_required label as this is documented now in http://docs.moodle.org/24/en/My_Moodle
          Hide
          Helen Foster added a comment -

          Just noting that the documentation about this new feature has been moved to http://docs.moodle.org/en/Course_overview_block

          Show
          Helen Foster added a comment - Just noting that the documentation about this new feature has been moved to http://docs.moodle.org/en/Course_overview_block
          Hide
          Daniel Kaelin added a comment -

          Customizing the course order on a My Moodle page does not work if a user is displaying all of their courses.

          The course order feature only works if they specify a certain number of courses to display. (10, 20, etc)

          I saw this issue was closed and I don't know if I'm running into a functionality bug or if anyone else has experienced this.

          Show
          Daniel Kaelin added a comment - Customizing the course order on a My Moodle page does not work if a user is displaying all of their courses. The course order feature only works if they specify a certain number of courses to display. (10, 20, etc) I saw this issue was closed and I don't know if I'm running into a functionality bug or if anyone else has experienced this.
          Hide
          Rajesh Taneja added a comment -

          Hello Daniel,

          I tried re-ordering courses on My home page, and it works fine.
          Can you please provide more information on how to reproduce this issue.
          Please make sure you are not getting an JS error while viewing all courses.

          Show
          Rajesh Taneja added a comment - Hello Daniel, I tried re-ordering courses on My home page, and it works fine. Can you please provide more information on how to reproduce this issue. Please make sure you are not getting an JS error while viewing all courses.
          Hide
          Daniel Kaelin added a comment - - edited

          The only error that is in the console for the page is this:

          event.returnValue is deprecated. Please use the standard event.preventDefault() instead.

          Version: 2012120307.00
          Release: 2.4.7 (Build: 20131111)

          How many courses are you reordering?

          The instructor / administrator in question has roles in 114 courses and has them all displayed on his my moodle page.

          The problem occurring is when you move courses using ajax editing they do not stay in the custom order. When you turn editing off (customize this page) the courses just return to their original order.

          The courses are a mix of visible and hidden courses but it doesn't appear that has anything to do with the issue. I've also looked at how the categories are affecting the course order and it doesn't seem to matter either. It isn't sorting them alphabetically either so I'm not entirely sure how the courses are even sorted. The original issue states it orders them based upon retrieval from the database but what determines how quickly one course is retrieved versus another?

          It just seems to me Moodle is not properly registering and updating the "course_overview_course_order" field in the mdl_user_preferences.

          Show
          Daniel Kaelin added a comment - - edited The only error that is in the console for the page is this: event.returnValue is deprecated. Please use the standard event.preventDefault() instead. Version: 2012120307.00 Release: 2.4.7 (Build: 20131111) How many courses are you reordering? The instructor / administrator in question has roles in 114 courses and has them all displayed on his my moodle page. The problem occurring is when you move courses using ajax editing they do not stay in the custom order. When you turn editing off (customize this page) the courses just return to their original order. The courses are a mix of visible and hidden courses but it doesn't appear that has anything to do with the issue. I've also looked at how the categories are affecting the course order and it doesn't seem to matter either. It isn't sorting them alphabetically either so I'm not entirely sure how the courses are even sorted. The original issue states it orders them based upon retrieval from the database but what determines how quickly one course is retrieved versus another? It just seems to me Moodle is not properly registering and updating the "course_overview_course_order" field in the mdl_user_preferences.
          Hide
          Rajesh Taneja added a comment -

          Hello Daniel,

          Can you please confirm which browser you are using. it will be helpful if you check this works on other browser.

          I checked with 15 courses and it seems to work fine. Visibility should not be a problem. Problem seems to be coming from JS error, which could be because of custom block as well.

          Can you track from where "event.returnValue is deprecated. Please use the standard event.preventDefault() instead." gets trigged.

          Show
          Rajesh Taneja added a comment - Hello Daniel, Can you please confirm which browser you are using. it will be helpful if you check this works on other browser. I checked with 15 courses and it seems to work fine. Visibility should not be a problem. Problem seems to be coming from JS error, which could be because of custom block as well. Can you track from where "event.returnValue is deprecated. Please use the standard event.preventDefault() instead." gets trigged.
          Hide
          Andrew Ruether added a comment -

          I have the seen the same behavior as Daniel – when a user has many courses, changing the order on the "Course Overview" block in the My Home page doesn't work. I checked the Apache error log and this is what shows up:

          Default exception handler: Coding error detected, it must be fixed by a programmer: 
          Invalid value in set_user_preference() call, value is is too long for the value column 
          Debug: 
          Error code: codingerror 
          line 1744 of /lib/moodlelib.php: coding_exception thrown
          line 65 of /blocks/course_overview/locallib.php: call to set_user_preference() 
          line 34 of /blocks/course_overview/save.php: call to block_course_overview_update_myorder(), 
          referer: https://moodle.xxxxxx.edu/my/index.php?mynumber=125

          It seems that the amount of data sent to the set_user_preference() call is too large.

          Show
          Andrew Ruether added a comment - I have the seen the same behavior as Daniel – when a user has many courses, changing the order on the "Course Overview" block in the My Home page doesn't work. I checked the Apache error log and this is what shows up: Default exception handler: Coding error detected, it must be fixed by a programmer: Invalid value in set_user_preference() call, value is is too long for the value column Debug: Error code: codingerror line 1744 of /lib/moodlelib.php: coding_exception thrown line 65 of /blocks/course_overview/locallib.php: call to set_user_preference() line 34 of /blocks/course_overview/save.php: call to block_course_overview_update_myorder(), referer: https: //moodle.xxxxxx.edu/my/index.php?mynumber=125 It seems that the amount of data sent to the set_user_preference() call is too large.
          Hide
          Rajesh Taneja added a comment -

          Aha, Thanks for spotting that Andrew,

          user_preferences can be of max 1333 chars and seems it's exceeding in this case.
          I have created MDL-43739 to handle this.

          Show
          Rajesh Taneja added a comment - Aha, Thanks for spotting that Andrew, user_preferences can be of max 1333 chars and seems it's exceeding in this case. I have created MDL-43739 to handle this.

            People

            • Votes:
              18 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: