Details

    • Type: New Feature New Feature
    • Status: Reopened
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.4, 2.5, 2.6.1
    • Fix Version/s: 2.7
    • Component/s: Assignment
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a new assignment and enable both the 'due date' and tick the 'display due date'.
      2. Confirm that the 'due date' is displayed on the course page underneath the assignment.
      3. Create a second assignment and disable the 'due date' and tick the 'display due date'.
      4. Confirm that the 'due date' is not displayed underneath the assignment on the course page.
      5. Create a third assignment and enable the 'due date' and un-tick the 'display due date'.
      6. Confirm that the 'due date' is not displayed underneath the assignment on the course page.
      7. Create a fourth assignment and enable the 'due date', enable 'Display description on course page' and tick the 'display due date'.
      8. Confirm that the 'due date' is displayed underneath the assignment description underneath the assignment on the course page.
      9. Backup and restore the course to a new course without users.
      10. Confirm that the first, second, third and fourth assignments are displayed as in steps 2, 4, 6 and 8 respectively.

      Show
      1. Create a new assignment and enable both the 'due date' and tick the 'display due date'. 2. Confirm that the 'due date' is displayed on the course page underneath the assignment. 3. Create a second assignment and disable the 'due date' and tick the 'display due date'. 4. Confirm that the 'due date' is not displayed underneath the assignment on the course page. 5. Create a third assignment and enable the 'due date' and un-tick the 'display due date'. 6. Confirm that the 'due date' is not displayed underneath the assignment on the course page. 7. Create a fourth assignment and enable the 'due date', enable 'Display description on course page' and tick the 'display due date'. 8. Confirm that the 'due date' is displayed underneath the assignment description underneath the assignment on the course page. 9. Backup and restore the course to a new course without users. 10. Confirm that the first, second, third and fourth assignments are displayed as in steps 2, 4, 6 and 8 respectively.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-37490-m10
    • Rank:
      47131

      Description

      Whilst preparing training material it occurred to me that a user needs to click on an assignment to discover the due date if set and that it would be helpful if it was displayed on the course page underneath the assignment like the description can be.

      Then the user would be able to quickly prioritise their work.

      1. add1.png
        3 kB
      2. add2.png
        6 kB
      3. GBDATE.png
        3 kB
      4. MDL_37490_enabled.png
        7 kB
      5. MDL-37490_admin_settings.png
        44 kB
      6. mdl-37490-apr-1.png
        6 kB
      7. mdl-37490-apr-2.png
        3 kB
      8. wrapped.png
        50 kB

        Issue Links

          Activity

          Hide
          Gareth J Barnard added a comment -

          Image 'add1.png' example usage.

          Show
          Gareth J Barnard added a comment - Image 'add1.png' example usage.
          Hide
          Gareth J Barnard added a comment -

          Image 'add2.png' example setting.

          Show
          Gareth J Barnard added a comment - Image 'add2.png' example setting.
          Show
          Gareth J Barnard added a comment - First version of patch: https://github.com/gjb2048/moodle/commit/76534138d657adc8e757d59909ffdaac4cee9e45
          Hide
          Ray Lawrence added a comment -

          Another drop-down... Can I suggest a check-box "Display" to the right of "Enable".

          Will this be in the module default options?

          May I also suggest that the date is displayed in a similar way/location to the File resource type/size information:

          1. Mode consistent experience throughout.
          2. Date will hard to see if Description is displayed on course page.
          3. Another line makes the course page longer again....

          Looking handy though

          Show
          Ray Lawrence added a comment - Another drop-down... Can I suggest a check-box "Display" to the right of "Enable". Will this be in the module default options? May I also suggest that the date is displayed in a similar way/location to the File resource type/size information: 1. Mode consistent experience throughout. 2. Date will hard to see if Description is displayed on course page. 3. Another line makes the course page longer again.... Looking handy though
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Ray,

          I'm not sure how to make it right of 'Enable' as on the form code the 'enable' box is added as an option on the 'duedate' element. But it could be a check-box underneath.

          It is in the module default options as 'No'.

          The date is displayed in the same format as it is on the assignment summary page when clicked on by a teacher / administrator to be consistent with the rest of the module - unless you mean the style?

          Different formatting could be applied using a theme standard style - need to look for one or can you suggest one?

          It will be longer, but it is an option and on well designed courses it will not be an issue. As an option, control is with the user rather than an enforced scroll of death.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Ray, I'm not sure how to make it right of 'Enable' as on the form code the 'enable' box is added as an option on the 'duedate' element. But it could be a check-box underneath. It is in the module default options as 'No'. The date is displayed in the same format as it is on the assignment summary page when clicked on by a teacher / administrator to be consistent with the rest of the module - unless you mean the style? Different formatting could be applied using a theme standard style - need to look for one or can you suggest one? It will be longer, but it is an option and on well designed courses it will not be an issue. As an option, control is with the user rather than an enforced scroll of death. Cheers, Gareth
          Hide
          Ray Lawrence added a comment - - edited

          I was thinking more like this (with the date not file info of course).

          See GBDATE image above.

          Show
          Ray Lawrence added a comment - - edited I was thinking more like this (with the date not file info of course). See GBDATE image above.
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Ray,

          This is the code from '/mod/assign/mod_form.php' where the 'duedate' and 'displayduedate' elements are created:

          $mform->addElement('date_time_selector', 'duedate', get_string('duedate', 'assign'), array('optional'=>true));
          $mform->addHelpButton('duedate', 'duedate', 'assign');
          $mform->setDefault('duedate', time()+7*24*3600);
          $mform->addElement('selectyesno', 'displayduedate', get_string('displayduedate', 'assign'));
          $mform->addHelpButton('displayduedate', 'displayduedate', 'assign');
          $mform->setDefault('displayduedate', 0);
          

          And as can be seen the 'enabled' part if the 'duedate' is actually controlled by:

          array('optional'=>true)
          

          Making it tricky to put 'displayduedate' alongside unless you know of a way to manipulate forms?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Ray, This is the code from '/mod/assign/mod_form.php' where the 'duedate' and 'displayduedate' elements are created: $mform->addElement('date_time_selector', 'duedate', get_string('duedate', 'assign'), array('optional'=> true )); $mform->addHelpButton('duedate', 'duedate', 'assign'); $mform->setDefault('duedate', time()+7*24*3600); $mform->addElement('selectyesno', 'displayduedate', get_string('displayduedate', 'assign')); $mform->addHelpButton('displayduedate', 'displayduedate', 'assign'); $mform->setDefault('displayduedate', 0); And as can be seen the 'enabled' part if the 'duedate' is actually controlled by: array('optional'=> true ) Making it tricky to put 'displayduedate' alongside unless you know of a way to manipulate forms? Cheers, Gareth
          Hide
          Ray Lawrence added a comment -

          Thanks. Unfortunately I have zero devt skills so can't help with this bit.

          Show
          Ray Lawrence added a comment - Thanks. Unfortunately I have zero devt skills so can't help with this bit.
          Hide
          Gareth J Barnard added a comment -

          Dear Ray,

          Ok. I'll see what css styles would be appropriate instead.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Ray, Ok. I'll see what css styles would be appropriate instead. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Just added a master branch of the code for easy integration after learning about branches on MDL-33115. First commit is https://github.com/gjb2048/moodle/commit/24314990476c1cdf26c355b3c62f53b62330dc52

          The master branch I assume is the one to make changes for integration on?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Just added a master branch of the code for easy integration after learning about branches on MDL-33115 . First commit is https://github.com/gjb2048/moodle/commit/24314990476c1cdf26c355b3c62f53b62330dc52 The master branch I assume is the one to make changes for integration on? Cheers, Gareth
          Hide
          Damyon Wiese added a comment -

          Thanks for submitting this feature Gareth,

          I'll take a look at the code as soon as I can and give you some detailed feedback.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Thanks for submitting this feature Gareth, I'll take a look at the code as soon as I can and give you some detailed feedback. Regards, Damyon
          Hide
          Damyon Wiese added a comment -

          Re: branches - the best branch to work off for new features is "master" (which is 2.5dev).

          Show
          Damyon Wiese added a comment - Re: branches - the best branch to work off for new features is "master" (which is 2.5dev).
          Hide
          Gareth J Barnard added a comment -

          Thanks Damyon, I'll make any alterations if any to the master branch for the issue.

          Show
          Gareth J Barnard added a comment - Thanks Damyon, I'll make any alterations if any to the master branch for the issue.
          Hide
          Damyon Wiese added a comment -

          Thanks Gareth for your patch.

          First some general comments on this feature.

          I agree with Ray about adding it as a checkbox next to the due date. This way it does not make the settings page even longer. The label for the checkbox can just be "Display on course page" because it will be in the same row as the due date.

          The way to do this is to create an array of form elements and add it to the form with $mform->addGroup() (There are some addGroup examples in mod_assign).

          Now some general comments on coding style. These are mostly things that are documented here: http://docs.moodle.org/dev/Coding_style. Once you have gotten feedback on a few patches these rules become easier to follow.

          1. There are tabs for indentation - must be 4 spaces
          2. There is trailing whitespace
          3. In lib.php you have used '<p>' directly - should be using html_writer for that.
          4. The comment for the column should be "If set the due date will be displayed on the course page"
          5. All comments should start with a capital and end with a punctuation mark
          6. in lib.php you need a space after "&&"
          7. I recommend installing the codechecker plugin from the plugins database and running it on all patches before you submit them. The assignment module should be (almost) free of warnings/errors so if there are new ones - they have been added by your code.
          8. Your git commit messages must be in this format:
            MDL-XXX <component> <short message (try and keep line below 80 chars)>
            
            <optional longer message>
            

          e.g.

          MDL-37490 mod_assign: Add setting to show the due date on the course page
          
          This is an awesome patch!
          

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Thanks Gareth for your patch. First some general comments on this feature. I agree with Ray about adding it as a checkbox next to the due date. This way it does not make the settings page even longer. The label for the checkbox can just be "Display on course page" because it will be in the same row as the due date. The way to do this is to create an array of form elements and add it to the form with $mform->addGroup() (There are some addGroup examples in mod_assign). Now some general comments on coding style. These are mostly things that are documented here: http://docs.moodle.org/dev/Coding_style . Once you have gotten feedback on a few patches these rules become easier to follow. There are tabs for indentation - must be 4 spaces There is trailing whitespace In lib.php you have used '<p>' directly - should be using html_writer for that. The comment for the column should be "If set the due date will be displayed on the course page" All comments should start with a capital and end with a punctuation mark in lib.php you need a space after "&&" I recommend installing the codechecker plugin from the plugins database and running it on all patches before you submit them. The assignment module should be (almost) free of warnings/errors so if there are new ones - they have been added by your code. Your git commit messages must be in this format: MDL-XXX <component> < short message ( try and keep line below 80 chars)> <optional longer message> e.g. MDL-37490 mod_assign: Add setting to show the due date on the course page This is an awesome patch! Thanks, Damyon
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Damyon,

          Thank you for reviewing the patch, I will make the changes you request on the master branch using the useful links and information you have kindly supplied. Would you like the Moodle 2.4 branch updated too or will integration only be on the master branch?

          Kind regards,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Damyon, Thank you for reviewing the patch, I will make the changes you request on the master branch using the useful links and information you have kindly supplied. Would you like the Moodle 2.4 branch updated too or will integration only be on the master branch? Kind regards, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon,

          Ran the code through the code checker and getting lots of:

          1: End of line character is invalid; expected "\n" but found "\r\n"

          How do I fix this? Editor thing? TortoiseGit thing?

          I'm using Netbeans and Notepad++ for editing.

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon, Ran the code through the code checker and getting lots of: 1: End of line character is invalid; expected "\n" but found "\r\n" How do I fix this? Editor thing? TortoiseGit thing? I'm using Netbeans and Notepad++ for editing. Thanks, Gareth
          Hide
          Gareth J Barnard added a comment -

          Additional, reading up on the \r\n issue, I'm using windows, so they are there, but I believe that TortoiseGit converts to the \n only when pushing to be Unix style.

          Show
          Gareth J Barnard added a comment - Additional, reading up on the \r\n issue, I'm using windows, so they are there, but I believe that TortoiseGit converts to the \n only when pushing to be Unix style.
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon,

          I have almost got the form working and made the other changes (help needs to change though) but I cannot get the due date optionality to now function with the code:

          $name = get_string('duedate', 'assign').$OUTPUT->help_icon('duedate','assign');
          $duedateelements[] = $mform->createElement('date_time_selector', 'duedate', $name, array('optional'=>true));
          $mform->setDefault('duedate', time()+7*24*3600);
          
          $duedateelements[] = $mform->createElement('checkbox', 'displayduedate', null, get_string('displayduedate', 'assign'));
          $mform->setDefault('displayduedate', 0);
          
          $mform->addGroup($duedateelements, 'duedategrp', $name, null, false);
          

          I've had to use the method 'createElement' because of 'addGroup'. The 'addHelpButton' has been replaced with a call to $OUTPUT as calling 'addHelpButton' results in an error when used with 'createElement' instead of 'addElement' and if I use 'addElement' then the elements are duplicated. There is no 'createHelpButton' method available.

          The master wip branch has been updated to reflect this.

          I'm going to ask in the forums for help too. It is the only sticking point that is stopping completion. Example image to be attached.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon, I have almost got the form working and made the other changes (help needs to change though) but I cannot get the due date optionality to now function with the code: $name = get_string('duedate', 'assign').$OUTPUT->help_icon('duedate','assign'); $duedateelements[] = $mform->createElement('date_time_selector', 'duedate', $name, array('optional'=> true )); $mform->setDefault('duedate', time()+7*24*3600); $duedateelements[] = $mform->createElement('checkbox', 'displayduedate', null , get_string('displayduedate', 'assign')); $mform->setDefault('displayduedate', 0); $mform->addGroup($duedateelements, 'duedategrp', $name, null , false ); I've had to use the method 'createElement' because of 'addGroup'. The 'addHelpButton' has been replaced with a call to $OUTPUT as calling 'addHelpButton' results in an error when used with 'createElement' instead of 'addElement' and if I use 'addElement' then the elements are duplicated. There is no 'createHelpButton' method available. The master wip branch has been updated to reflect this. I'm going to ask in the forums for help too. It is the only sticking point that is stopping completion. Example image to be attached. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          File 'MDL_37490_enabled.png' showing enabled issue.

          Show
          Gareth J Barnard added a comment - File 'MDL_37490_enabled.png' showing enabled issue.
          Hide
          Ray Lawrence added a comment -

          Sorry for not letting this one lie... I still think this would be even neater if the text (Due date: day month your time) was displayed inline with the link in the same way as file type /size are for File resources.

          Damyon, do you have any suggestions on how this could be incorporated?

          Show
          Ray Lawrence added a comment - Sorry for not letting this one lie... I still think this would be even neater if the text (Due date: day month your time) was displayed inline with the link in the same way as file type /size are for File resources. Damyon, do you have any suggestions on how this could be incorporated?
          Hide
          Gareth J Barnard added a comment -

          Dear Ray,

          I consider that on standard resolution devices the text would be prone to wrapping, so I'll upload a screen shot taken from a 1024x600 net book illustrating a hacked page.

          The code that needs to be changed is in '/course/lib.php' method 'print_section()':

          if ($url = $mod->get_url()) {
              // Display link itself.
              $activitylink = html_writer::empty_tag('img', array('src' => $mod->get_icon_url(),
                  'class' => 'iconlarge activityicon', 'alt' => $mod->modfullname)) . $accesstext .
                  html_writer::tag('span', $instancename . $altname, array('class' => 'instancename'));
              echo html_writer::link($url, $activitylink, array('class' => $linkcss, 'onclick' => $onclick)) .
                  $groupinglabel;
          
               // If specified, display extra content after link.
              if ($content) {
                  $contentpart = html_writer::tag('div', $content, array('class' =>
                  trim('contentafterlink ' . $textclasses)));
              }
          } else {
              // No link, so display only content.
              $contentpart = html_writer::tag('div', $accesstext . $content, array('class' => $textcss));
          }
          

          But this will affect the description as well if altered here, so therefore a major change if the due date comes before the description. The code above is used for all activities, therefore either clever use of css and tags within the assign module or refactoring all modules that have additional content such that they have a delegated method in their renderer class instead of a pull methodology in print_section().

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Ray, I consider that on standard resolution devices the text would be prone to wrapping, so I'll upload a screen shot taken from a 1024x600 net book illustrating a hacked page. The code that needs to be changed is in '/course/lib.php' method 'print_section()': if ($url = $mod->get_url()) { // Display link itself. $activitylink = html_writer::empty_tag('img', array('src' => $mod->get_icon_url(), 'class' => 'iconlarge activityicon', 'alt' => $mod->modfullname)) . $accesstext . html_writer::tag('span', $instancename . $altname, array('class' => 'instancename')); echo html_writer::link($url, $activitylink, array('class' => $linkcss, 'onclick' => $onclick)) . $groupinglabel; // If specified, display extra content after link. if ($content) { $contentpart = html_writer::tag('div', $content, array('class' => trim('contentafterlink ' . $textclasses))); } } else { // No link, so display only content. $contentpart = html_writer::tag('div', $accesstext . $content, array('class' => $textcss)); } But this will affect the description as well if altered here, so therefore a major change if the due date comes before the description. The code above is used for all activities, therefore either clever use of css and tags within the assign module or refactoring all modules that have additional content such that they have a delegated method in their renderer class instead of a pull methodology in print_section(). Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Image 'wrapped.png' illustrating 'hacked' html / css to demonstrate the issue of inline 'Due date' on a standard display with a short assignment name.

          Show
          Gareth J Barnard added a comment - Image 'wrapped.png' illustrating 'hacked' html / css to demonstrate the issue of inline 'Due date' on a standard display with a short assignment name.
          Hide
          Ray Lawrence added a comment -

          Hmm. OK fair point but that's the same potential issue as the info appended to the file link. A shorter date/time format would help.

          That prompted another thought. Will this need to respect the locale settings for global use? (You may have already coded that).

          Show
          Ray Lawrence added a comment - Hmm. OK fair point but that's the same potential issue as the info appended to the file link. A shorter date/time format would help. That prompted another thought. Will this need to respect the locale settings for global use? (You may have already coded that).
          Hide
          Gareth J Barnard added a comment -

          Dear Ray,

          Already considered that by using the 'userdate()' function as per the code that displays the date / time when you click on the assignment. Looked there in the first instance to see how it was done, if no appropriate function then would have been a bug with existing code.

          I'm keen to keep the same formatting to be consistent.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Ray, Already considered that by using the 'userdate()' function as per the code that displays the date / time when you click on the assignment. Looked there in the first instance to see how it was done, if no appropriate function then would have been a bug with existing code. I'm keen to keep the same formatting to be consistent. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Daymon,

          Finally cracked the form issue! My master branch is up to date. I have sorted most of the issues. I did notice issues with the code checker in files I had not touched. It's now late and I do need to run the code checker again on my modifications to be sure and test a few logic paths. In the mean time could you kindly glance at the recent changes.

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Daymon, Finally cracked the form issue! My master branch is up to date. I have sorted most of the issues. I did notice issues with the code checker in files I had not touched. It's now late and I do need to run the code checker again on my modifications to be sure and test a few logic paths. In the mean time could you kindly glance at the recent changes. Thanks, Gareth
          Hide
          Damyon Wiese added a comment -

          Hi Gareth,

          Thanks for working on this. As this is a new feature it will not be backported as per the integration backporting policy here (so no need to update the 24 branch):

          http://docs.moodle.org/dev/Integration_Review#Backporting

          I do not think we should be trying to get the date to display inline on the course page - if that is an important improvement it should be split into a separate issue and voted/worked on later.

          Re the latest code:
          Yes - you are almost there!

          A couple more things:

          1. advcheckbox is really for checkbox groups - better to just use checkbox
          2. You should pass array('optional'=>true) as the 4th param to createElement('date_time_selector'...). This will take care of the "Enabled" checkbox and you can remove all that extra code for setting the default (I haven't tried this - let me know if it doesn't work right).

          Some more info on '\r\n' here:
          https://moodle.org/mod/forum/discuss.php?d=192582
          Note - it is not recommended to have git do line ending translations - because if you check a file out and it has '\r\n' because of someone elses bad code, make one change, then commit it - it will remove those line endings - and this will be included in your patch - which is an unrelated change and is more likely to create conflicts later.

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Hi Gareth, Thanks for working on this. As this is a new feature it will not be backported as per the integration backporting policy here (so no need to update the 24 branch): http://docs.moodle.org/dev/Integration_Review#Backporting I do not think we should be trying to get the date to display inline on the course page - if that is an important improvement it should be split into a separate issue and voted/worked on later. Re the latest code: Yes - you are almost there! A couple more things: advcheckbox is really for checkbox groups - better to just use checkbox You should pass array('optional'=>true) as the 4th param to createElement('date_time_selector'...). This will take care of the "Enabled" checkbox and you can remove all that extra code for setting the default (I haven't tried this - let me know if it doesn't work right). Some more info on '\r\n' here: https://moodle.org/mod/forum/discuss.php?d=192582 Note - it is not recommended to have git do line ending translations - because if you check a file out and it has '\r\n' because of someone elses bad code, make one change, then commit it - it will remove those line endings - and this will be included in your patch - which is an unrelated change and is more likely to create conflicts later. Regards, Damyon
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon,

          I have updated the code not to use 'advcheckbox', but I had originally used it because according to the API it returns '0' when not set which was useful when updating the database. However, using just 'checkbox' introduces more code in more than one place in locallib.php (not very OO). But, I have made the change as requested and you can see the effect in 'locallib.php'

          I cannot 'pass array('optional'=>true) as the 4th param to createElement('date_time_selector'...)' because it does not work (please kindly see my past comments). When passed to addElement then the disabled bit works, when passed to createElement it does not, so had no choice in the way it's written. Plus I could not find an easy way of it hiding the display due date checkbox too, so defining it separately and using a single 'disabledIf' on the group is neater.

          I've 'codechecked' and eliminated any errors that were mine. I've disabled AutoCrLf in Git so should be no confusion / conflicts - mod_form.php and locallib.php only have LF's, lib.php has CRLF's but this might be historical - I'm not sure given the other files.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon, I have updated the code not to use 'advcheckbox', but I had originally used it because according to the API it returns '0' when not set which was useful when updating the database. However, using just 'checkbox' introduces more code in more than one place in locallib.php (not very OO). But, I have made the change as requested and you can see the effect in 'locallib.php' I cannot 'pass array('optional'=>true) as the 4th param to createElement('date_time_selector'...)' because it does not work (please kindly see my past comments). When passed to addElement then the disabled bit works, when passed to createElement it does not, so had no choice in the way it's written. Plus I could not find an easy way of it hiding the display due date checkbox too, so defining it separately and using a single 'disabledIf' on the group is neater. I've 'codechecked' and eliminated any errors that were mine. I've disabled AutoCrLf in Git so should be no confusion / conflicts - mod_form.php and locallib.php only have LF's, lib.php has CRLF's but this might be historical - I'm not sure given the other files. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese and Michael de Raadt,

          Is there anything I can do to help get this in before the 29th March deadline?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese and Michael de Raadt , Is there anything I can do to help get this in before the 29th March deadline? Cheers, Gareth
          Hide
          Damyon Wiese added a comment -

          Hi Gareth,

          Sorry for the delay - I was reviewing another big issue for 2.5 (submission attempt history MDL-36804). I'll be able to get back to this on wednesday and as it's already sent for review it has already made the cut off for 2.5.

          • Damyon
          Show
          Damyon Wiese added a comment - Hi Gareth, Sorry for the delay - I was reviewing another big issue for 2.5 (submission attempt history MDL-36804 ). I'll be able to get back to this on wednesday and as it's already sent for review it has already made the cut off for 2.5. Damyon
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          Thank you.

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , Thank you. Gareth
          Hide
          Damyon Wiese added a comment -

          Hi Gareth - the master branch for this issue has gone missing from github?

          Show
          Damyon Wiese added a comment - Hi Gareth - the master branch for this issue has gone missing from github?
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          I've put the branch back from my local copy but notice that it was not squashed. So tried to do so but install.xml is odd, please see https://github.com/gjb2048/moodle/compare/wip-MDL-37490-m2. Not quite sure how to fix this yet!

          Perhaps if you could kindly peer review the code and I could then sort things if any changes were to be made. If not, then I'll rebase etc. and get everything clean for a single commit.

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , I've put the branch back from my local copy but notice that it was not squashed. So tried to do so but install.xml is odd, please see https://github.com/gjb2048/moodle/compare/wip-MDL-37490-m2 . Not quite sure how to fix this yet! Perhaps if you could kindly peer review the code and I could then sort things if any changes were to be made. If not, then I'll rebase etc. and get everything clean for a single commit. Thanks, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Damyon Wiese,

          All now squashed and rebased against latest master into a new master pull branch

          'wip-MDL-37490-m3'

          . Old

          'wip-MDL-37490-master'

          and

          'wip-MDL-37490-m2'

          there for reference.

          I noticed that you'd fixed the version date, so latest update incorporates a minor increment.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Damyon Wiese , All now squashed and rebased against latest master into a new master pull branch 'wip-MDL-37490-m3' . Old 'wip-MDL-37490-master' and 'wip-MDL-37490-m2' there for reference. I noticed that you'd fixed the version date, so latest update incorporates a minor increment. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          wip-MDL-37490_m3 branch rebased to latest master.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , wip- MDL-37490 _m3 branch rebased to latest master. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          wip-MDL-37490_m3 branch rebased to latest master.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , wip- MDL-37490 _m3 branch rebased to latest master. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          wip-MDL-37490_m3 branch rebased to latest master.

          Anything I can do to help?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , wip- MDL-37490 _m3 branch rebased to latest master. Anything I can do to help? Cheers, Gareth
          Hide
          Damyon Wiese added a comment -

          Thanks Gareth,

          I pushed a branch with an extra commit that has some minor cleanups.

          • Another version bump
          • Use ' for language strings
          • Use $assignment->get_renderer() not $OUTPUT

          This is ready for integration - adding Martin to vote on when it should land.

          Show
          Damyon Wiese added a comment - Thanks Gareth, I pushed a branch with an extra commit that has some minor cleanups. Another version bump Use ' for language strings Use $assignment->get_renderer() not $OUTPUT This is ready for integration - adding Martin to vote on when it should land.
          Hide
          Martin Dougiamas added a comment -

          Can someone show me what it looks like? Recent screenshots?

          Show
          Martin Dougiamas added a comment - Can someone show me what it looks like? Recent screenshots?
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Damyon Wiese,

          Sorry!!!!

          Just realised that the pull branch incorporates two pull branches and not one. My mistake. I've put things back the way they were.

          I'll post a new screen shot soon Martin.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Damyon Wiese , Sorry!!!! Just realised that the pull branch incorporates two pull branches and not one. My mistake. I've put things back the way they were. I'll post a new screen shot soon Martin. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Damyon Wiese,

          Just checked out your branch to get screen shots and am getting:

          Coding error detected, it must be fixed by a programmer: Improper use of the assignment class. Cannot load the assignment record.
          
          Debug info:
          Error code: codingerror
          Stack trace:
          
              line 1060 of \mod\assign\locallib.php: coding_exception thrown
              line 89 of \mod\assign\mod_form.php: call to assign->get_instance()
              line 191 of \lib\formslib.php: call to mod_assign_mod_form->definition()
              line 71 of \course\moodleform_mod.php: call to moodleform->moodleform()
              line 248 of \course\modedit.php: call to moodleform_mod->moodleform_mod()
          

          When trying to add an assignment.

          The line is:

          if ($assignment->get_instance()->duedate > 0) {
          

          So looks like:

          $this->instance = $DB->get_record('assign', $params, '*', MUST_EXIST);
          

          Is failing.

          This was working a few months ago.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Damyon Wiese , Just checked out your branch to get screen shots and am getting: Coding error detected, it must be fixed by a programmer: Improper use of the assignment class. Cannot load the assignment record. Debug info: Error code: codingerror Stack trace: line 1060 of \mod\assign\locallib.php: coding_exception thrown line 89 of \mod\assign\mod_form.php: call to assign->get_instance() line 191 of \lib\formslib.php: call to mod_assign_mod_form->definition() line 71 of \course\moodleform_mod.php: call to moodleform->moodleform() line 248 of \course\modedit.php: call to moodleform_mod->moodleform_mod() When trying to add an assignment. The line is: if ($assignment->get_instance()->duedate > 0) { So looks like: $ this ->instance = $DB->get_record('assign', $params, '*', MUST_EXIST); Is failing. This was working a few months ago. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          I think there is a new separate issue going on here as I've cleaned out my DB and reinstalled.

          It seems that '$this->context' is null in '/mod/assign/locallib.php' when 'get_instance()' uses 'if ($this->get_course_module())' where '$this->coursemodule' is null in 'if ($this->coursemodule)' and therefore:

                  if (!$this->context) {
                      return null;
                  }
          

          happens and therefore:

                  if ($this->get_course_module()) {
                      $params = array('id' => $this->get_course_module()->instance);
                      $this->instance = $DB->get_record('assign', $params, '*', MUST_EXIST);
                  }
          

          Fails to get the DB record and populate '$this->instance'.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , I think there is a new separate issue going on here as I've cleaned out my DB and reinstalled. It seems that '$this->context' is null in '/mod/assign/locallib.php' when 'get_instance()' uses 'if ($this->get_course_module())' where '$this->coursemodule' is null in 'if ($this->coursemodule)' and therefore: if (!$ this ->context) { return null ; } happens and therefore: if ($ this ->get_course_module()) { $params = array('id' => $ this ->get_course_module()->instance); $ this ->instance = $DB->get_record('assign', $params, '*', MUST_EXIST); } Fails to get the DB record and populate '$this->instance'. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          Ok, looks like a bit of extra logic required in 'mod_form.php':

                  try {
                      $duedate = $assignment->get_instance()->duedate;
                  }
                  catch (Exception $e) {
                      $duedate = 0;
                  }
                  if ($duedate > 0) {
                      $mform->setDefault('duedate', $assignment->get_instance()->duedate);
                      $mform->setDefault('duedateenable', 1);
                  } else {
                      $mform->setDefault('duedate', time()+7*24*3600);
                      $mform->setDefault('duedateenable', 0);
                      $mform->setType('duedateenable', PARAM_BOOL);
                  }
          

          Because there are two commits and your branch, not sure how to add my commit to your commit!

          Tested and works with new and existing assignments.

          Show
          Gareth J Barnard added a comment - - edited Ok, looks like a bit of extra logic required in 'mod_form.php': try { $duedate = $assignment->get_instance()->duedate; } catch (Exception $e) { $duedate = 0; } if ($duedate > 0) { $mform->setDefault('duedate', $assignment->get_instance()->duedate); $mform->setDefault('duedateenable', 1); } else { $mform->setDefault('duedate', time()+7*24*3600); $mform->setDefault('duedateenable', 0); $mform->setType('duedateenable', PARAM_BOOL); } Because there are two commits and your branch, not sure how to add my commit to your commit! Tested and works with new and existing assignments.
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Damyon Wiese,

          Ok, I think I have this licked. I've made the code changes to fix the 'no DB record' issue, code checked my changes, rebased to squash and cherry picked your change such that there are two clean commits. So new branch https://github.com/gjb2048/moodle/compare/wip-MDL-37490-m6 created with all the commits and credit to respective authors.

          But this means that your 'MDL-37490-master' branch contains an out of date commit from me that I've just squashed in mine as stated above.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Damyon Wiese , Ok, I think I have this licked. I've made the code changes to fix the 'no DB record' issue, code checked my changes, rebased to squash and cherry picked your change such that there are two clean commits. So new branch https://github.com/gjb2048/moodle/compare/wip-MDL-37490-m6 created with all the commits and credit to respective authors. But this means that your ' MDL-37490 -master' branch contains an out of date commit from me that I've just squashed in mine as stated above. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Martin Dougiamas,

          Images 'mdl-37490-apr-1.png' and 'mdl-37490-apr-2.png' show up to date screen shots of the functionality.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Martin Dougiamas , Images 'mdl-37490-apr-1.png' and 'mdl-37490-apr-2.png' show up to date screen shots of the functionality. Cheers, Gareth
          Hide
          Dan Poltawski added a comment -

          Holding this as it has arrived in freeze time and is a new feature.

          Show
          Dan Poltawski added a comment - Holding this as it has arrived in freeze time and is a new feature.
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          I've rebased against the 2nd May release of 2.5beta and resolved a conflict with the database version, keeping it the same as what you had already integrated. New branch (m7) created for this just in case the rebase went wrong.

          No code has been harmed in the application of this rebase.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , I've rebased against the 2nd May release of 2.5beta and resolved a conflict with the database version, keeping it the same as what you had already integrated. New branch (m7) created for this just in case the rebase went wrong. No code has been harmed in the application of this rebase. Cheers, Gareth
          Hide
          Damyon Wiese added a comment -

          Thanks Gareth

          Martin needed to +1 this for it to go in 2.5 and it's a bit late now - so this won't get pulled in until after the on-sync period now (2.5 + 1 month).

          Show
          Damyon Wiese added a comment - Thanks Gareth Martin needed to +1 this for it to go in 2.5 and it's a bit late now - so this won't get pulled in until after the on-sync period now (2.5 + 1 month).
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese

          Ok ta.

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese Ok ta. Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Damyon Wiese,

          I've just rebased wip-MDL-37490-m7. I take it that the end of the on-sync period is the release of 2.5.1 or I've got to wait 6 days as 2.5 was released on the 14th May?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Damyon Wiese , I've just rebased wip- MDL-37490 -m7. I take it that the end of the on-sync period is the release of 2.5.1 or I've got to wait 6 days as 2.5 was released on the 14th May? Cheers, Gareth
          Hide
          Sam Hemelryk added a comment -

          Thanks everyone, after a quick discussion with Damyon I am happy with this change and its been integrated.
          It has landed to master only and will be available for 2.6. As per our integration policy new features and improvements don't get backported.
          There has been a bit of discussion about backporting this, if someone feels strongly that this should be backported please create an issue and we can put it up for discussion.
          For information on our backport policy please refer to http://docs.moodle.org/dev/Integration_Review#Process_for_requesting_a_non_bug-fix_backport

          Show
          Sam Hemelryk added a comment - Thanks everyone, after a quick discussion with Damyon I am happy with this change and its been integrated. It has landed to master only and will be available for 2.6. As per our integration policy new features and improvements don't get backported. There has been a bit of discussion about backporting this, if someone feels strongly that this should be backported please create an issue and we can put it up for discussion. For information on our backport policy please refer to http://docs.moodle.org/dev/Integration_Review#Process_for_requesting_a_non_bug-fix_backport
          Hide
          Dan Poltawski added a comment -

          This is causing all calls of phpunit carnage for me:

          https://gist.github.com/danpoltawski/5754429

          Show
          Dan Poltawski added a comment - This is causing all calls of phpunit carnage for me: https://gist.github.com/danpoltawski/5754429
          Hide
          Damyon Wiese added a comment -

          Hi Gareth,

          This failed unit tests in integration. The tests picked up that the changes to duedate handling have changed the API for the generator. I'll pick this up and rework it - but I don't have time to get it sorted today - so this issue will have to come out of integration and I'll resubmit it for next week.

          Show
          Damyon Wiese added a comment - Hi Gareth, This failed unit tests in integration. The tests picked up that the changes to duedate handling have changed the API for the generator. I'll pick this up and rework it - but I don't have time to get it sorted today - so this issue will have to come out of integration and I'll resubmit it for next week.
          Hide
          Sam Hemelryk added a comment -

          Aha, bugs bugs bugs. Certainly this needs to be tidied up and the impact of the webservices assessed properly.

          I've reverted the two commits now and am reopening this.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Aha, bugs bugs bugs. Certainly this needs to be tidied up and the impact of the webservices assessed properly. I've reverted the two commits now and am reopening this. Many thanks Sam
          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
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          No problem, please let me know if there is anything I can do. So much agro over one table attribute!

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , No problem, please let me know if there is anything I can do. So much agro over one table attribute! Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          I went to do a rebase and lost all the changes because of the reverting commit by Sam on the 11th June - SHA 'b618f2d9f745761f070eec722a5e076454649a42'. So to get them back I had to revert that commit but this meant that your commit was lost as the changes were combined - sorry.

          So, I've now created an 'm8' branch which hopefully has both sets of changes.

          Ok, with the web services question I looked at /db/services.php' and then 'externallib.php' to find that the only current services are ones that get information out of Moodle, specifically the only relevant one is 'mod_assign_get_assignments' and that lists the assignments. So I consider if the due date is displayed on the screen not to be important in that context as something you would not need to know from that perspective.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , I went to do a rebase and lost all the changes because of the reverting commit by Sam on the 11th June - SHA 'b618f2d9f745761f070eec722a5e076454649a42'. So to get them back I had to revert that commit but this meant that your commit was lost as the changes were combined - sorry. So, I've now created an 'm8' branch which hopefully has both sets of changes. Ok, with the web services question I looked at /db/services.php' and then 'externallib.php' to find that the only current services are ones that get information out of Moodle, specifically the only relevant one is 'mod_assign_get_assignments' and that lists the assignments. So I consider if the due date is displayed on the screen not to be important in that context as something you would not need to know from that perspective. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear all,

          Branch wip-MDL-37490-m8 rebased to Moodle 2.6dev (Build: 20130627).

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear all, Branch wip- MDL-37490 -m8 rebased to Moodle 2.6dev (Build: 20130627). Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear all,

          Branch wip-MDL-37490-m8 rebased to Moodle 2.6dev (Build: 20130704).

          Cheers,
          Gareth

          Show
          Gareth J Barnard added a comment - Dear all, Branch wip- MDL-37490 -m8 rebased to Moodle 2.6dev (Build: 20130704). Cheers, Gareth
          Hide
          Mary Evans added a comment -

          Gareth, I've set this for Integration Review otherwise you will miss the slot for this week. If I have done the wrong thing let me know.

          Cheers
          Mary

          Show
          Mary Evans added a comment - Gareth, I've set this for Integration Review otherwise you will miss the slot for this week. If I have done the wrong thing let me know. Cheers Mary
          Hide
          Gareth J Barnard added a comment -

          Dear Mary Evans,

          Not at all - thank you

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mary Evans , Not at all - thank you Cheers, Gareth
          Hide
          Damyon Wiese added a comment -

          Hi Gareth,

          Sorry for not checking the branch again earlier - but looking at this now and it breaks the admin defaults for the due date.

          To be honest I think we need to give up on having the setting on the same line as the duedate because that will cause more bugs than its worth.

          I'm reopening this again and I'll try and look at it in more detail later this week.

          Show
          Damyon Wiese added a comment - Hi Gareth, Sorry for not checking the branch again earlier - but looking at this now and it breaks the admin defaults for the due date. To be honest I think we need to give up on having the setting on the same line as the duedate because that will cause more bugs than its worth. I'm reopening this again and I'll try and look at it in more detail later this week.
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Damyon Wiese,

          I have spent weeks of effort over seven months trying to get this one useful improvement done and I'm not about to give up now.

          All it is one binary toggle to show a line of text. I cannot believe how difficult that can be to achieve. How on earth can it break admin defaults for the due date? Please can you gave a screen shot of the breakage or we allow this setting to be on a new line or is it the issue described in 'MDL-37490_admin_settings.png' in which case that is easy as its the same as settings for a theme etc. and then use defaults when creating a new assignment - not completely trivial but certainly doable.

          Pragmatically if there is no hope in adding something as simple as this into core code then there are some serious issues that need to be addressed - do you not agree?

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Damyon Wiese , I have spent weeks of effort over seven months trying to get this one useful improvement done and I'm not about to give up now. All it is one binary toggle to show a line of text. I cannot believe how difficult that can be to achieve. How on earth can it break admin defaults for the due date? Please can you gave a screen shot of the breakage or we allow this setting to be on a new line or is it the issue described in ' MDL-37490 _admin_settings.png' in which case that is easy as its the same as settings for a theme etc. and then use defaults when creating a new assignment - not completely trivial but certainly doable. Pragmatically if there is no hope in adding something as simple as this into core code then there are some serious issues that need to be addressed - do you not agree? Cheers, Gareth
          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
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          I went looking for how I could solve this and found the admin 'duedate' in the 'settings.php' file and then looked for where it was used using a search on 'get_config' within the 'assign' folder. Started to scratch my head as could not find the setting being used and wondered if there was a bug, so looked at the Moodle 2.5 code to find that the admin 'duedate' setting did not exist. So, therefore it would have been impossible for this issue to deal with the admin settings situation at the time of writing.

          So, in order to proceed could you kindly tell me the purpose of the 'advanced' tick box on the settings page so that I know how to implement this.

          Many thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , I went looking for how I could solve this and found the admin 'duedate' in the 'settings.php' file and then looked for where it was used using a search on 'get_config' within the 'assign' folder. Started to scratch my head as could not find the setting being used and wondered if there was a bug, so looked at the Moodle 2.5 code to find that the admin 'duedate' setting did not exist. So, therefore it would have been impossible for this issue to deal with the admin settings situation at the time of writing. So, in order to proceed could you kindly tell me the purpose of the 'advanced' tick box on the settings page so that I know how to implement this. Many thanks, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          I hope that you are better.

          So I can proceed with this, please could you tell me the purpose of the advanced tick box?

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , I hope that you are better. So I can proceed with this, please could you tell me the purpose of the advanced tick box? Thanks, Gareth
          Hide
          Gareth J Barnard added a comment - - edited

          Dear Damyon Wiese,

          Please could you kindly answer my question of the 8th July above. As all the reasons now for not proceeding with this appear to be because of new code that was added after this was ready for peer review. If no answer is forthcoming, then I'll just have to proceed along 'best guess'.

          Many thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Dear Damyon Wiese , Please could you kindly answer my question of the 8th July above. As all the reasons now for not proceeding with this appear to be because of new code that was added after this was ready for peer review. If no answer is forthcoming, then I'll just have to proceed along 'best guess'. Many thanks, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          Please could you answer my question of the 8th July above. Is there anything I can do to help?

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , Please could you answer my question of the 8th July above. Is there anything I can do to help? Thanks, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          If you could spare some time in your busy schedule, please could you kindly answer my question of the 8th July above.

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , If you could spare some time in your busy schedule, please could you kindly answer my question of the 8th July above. Thanks, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          Ok, I can see potentially why the inline form setting might cause issues. But I was badgered for ages about that which lead away from the real implementation of being able to show the information in the first place. If it is the form that is the sticking issue and with collapse-able form technology in place then it is a minor matter of not having it inline but as an expandable option.

          So, therefore please could you comment and kindly answer my question of the 8th July 2013 above.

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , Ok, I can see potentially why the inline form setting might cause issues. But I was badgered for ages about that which lead away from the real implementation of being able to show the information in the first place. If it is the form that is the sticking issue and with collapse-able form technology in place then it is a minor matter of not having it inline but as an expandable option. So, therefore please could you comment and kindly answer my question of the 8th July 2013 above. Thanks, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          Please see 'mdl_37490_sad.jpg'.

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , Please see 'mdl_37490_sad.jpg'. Gareth
          Hide
          Damyon Wiese added a comment -

          Dear Gareth,

          The reasons this is not receiving immediate attention is because:

          a) The priority is minor
          b) It has a low number of votes
          c) It is not "waiting for peer review"
          d) There were other (more important) changes being implemented at the time this was first attempted
          e) For good reason - the effort that individual developers put into things has to not be a consideration of whether something should or should not be implemented - it should be on the merits of the issue.

          Admin defaults / advanced:
          Look at:
          /admin/settings.php?section=modsettingassign
          Change some default and click advanced for a few settings
          Look at the assignment settings form - the advanced settings are not shown until you click "Show more"

          This is implemented by adding flags to the admin settings in the settings form, and then changes to moodleform_mod to automatically apply the defaults and advanced settings. By changing the names of the fields and combining multiple fields in a single line - you will break this code.

          So it no longer makes sense to add this checkbox inline - you will have to put it on it's own line in the form (and I feel, push more important things down the page).

          Show
          Damyon Wiese added a comment - Dear Gareth, The reasons this is not receiving immediate attention is because: a) The priority is minor b) It has a low number of votes c) It is not "waiting for peer review" d) There were other (more important) changes being implemented at the time this was first attempted e) For good reason - the effort that individual developers put into things has to not be a consideration of whether something should or should not be implemented - it should be on the merits of the issue. Admin defaults / advanced: Look at: /admin/settings.php?section=modsettingassign Change some default and click advanced for a few settings Look at the assignment settings form - the advanced settings are not shown until you click "Show more" This is implemented by adding flags to the admin settings in the settings form, and then changes to moodleform_mod to automatically apply the defaults and advanced settings. By changing the names of the fields and combining multiple fields in a single line - you will break this code. So it no longer makes sense to add this checkbox inline - you will have to put it on it's own line in the form (and I feel, push more important things down the page).
          Hide
          Gareth J Barnard added a comment -

          Dear Damyon Wiese,

          Firstly, thank you for taking the time to respond to my nagging. Sorry that I had to but I had no where to go with this as you had not explained why there were issues and did say that you would look into it within a week and never responded:

          "Sorry for not checking the branch again earlier - but looking at this now and it breaks the admin defaults for the due date.
          To be honest I think we need to give up on having the setting on the same line as the duedate because that will cause more bugs than its worth.
          I'm reopening this again and I'll try and look at it in more detail later this week."

          I was not asking for immediate attention, but then I don't really classify seven weeks as immediate when you promised you would look into it. If you had got back to be within that week even to say 'I'm sorry I've not got time for two months' then I would have understood and not nagged.

          Ok,

          1. If there are issues with inline elements breaking the advanced functionality of a form, then this must be a 'bug' to be raised in a separate issue. As I can remember lots of talk being placed on improving forms and therefore 'inlining' would be a good way of solving this.
          2. I find it disheartening that useful functionality is stalled because of issues of the switch that activates it.
          3. Looking at MDL-372, votes appear to have no effect in getting an improvement implemented.
          4. From what you have listed, I still have no where to go with this, the arguments you present end up in a 'Catch-22'. I don't believe in a no-win situation with software as it's so much in our power to control and change things for the better.

          I'm going to think about how this can be progressed, cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Damyon Wiese , Firstly, thank you for taking the time to respond to my nagging. Sorry that I had to but I had no where to go with this as you had not explained why there were issues and did say that you would look into it within a week and never responded: "Sorry for not checking the branch again earlier - but looking at this now and it breaks the admin defaults for the due date. To be honest I think we need to give up on having the setting on the same line as the duedate because that will cause more bugs than its worth. I'm reopening this again and I'll try and look at it in more detail later this week." I was not asking for immediate attention, but then I don't really classify seven weeks as immediate when you promised you would look into it. If you had got back to be within that week even to say 'I'm sorry I've not got time for two months' then I would have understood and not nagged. Ok, 1. If there are issues with inline elements breaking the advanced functionality of a form, then this must be a 'bug' to be raised in a separate issue. As I can remember lots of talk being placed on improving forms and therefore 'inlining' would be a good way of solving this. 2. I find it disheartening that useful functionality is stalled because of issues of the switch that activates it. 3. Looking at MDL-372 , votes appear to have no effect in getting an improvement implemented. 4. From what you have listed, I still have no where to go with this, the arguments you present end up in a 'Catch-22'. I don't believe in a no-win situation with software as it's so much in our power to control and change things for the better. I'm going to think about how this can be progressed, cheers, Gareth
          Hide
          Mitchell van Gerwen added a comment -

          I am not sure if this is meant this way, but will the extended due date shown to?

          Show
          Mitchell van Gerwen added a comment - I am not sure if this is meant this way, but will the extended due date shown to?
          Hide
          Gareth J Barnard added a comment -

          Dear Mitchell van Gerwen,

          Not planned to have the extended due date too.

          To be honest I've given up on the idea for the moment because of being unable to get it past Damyon Wiese as can be seen in the comments above. The main stopping point appears to be the form technology, which I hear is due to be re-written. Perhaps with sufficient API changes and an actual desire for the functionality in the first place, one day this might actually happen.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Mitchell van Gerwen , Not planned to have the extended due date too. To be honest I've given up on the idea for the moment because of being unable to get it past Damyon Wiese as can be seen in the comments above. The main stopping point appears to be the form technology, which I hear is due to be re-written. Perhaps with sufficient API changes and an actual desire for the functionality in the first place, one day this might actually happen. Cheers, Gareth
          Hide
          Mitchell van Gerwen added a comment -

          Hello Gareth,

          Thank you for the quick and clear answer. Thanks!

          Show
          Mitchell van Gerwen added a comment - Hello Gareth, Thank you for the quick and clear answer. Thanks!
          Hide
          Mitchell van Gerwen added a comment -

          Hello Gareth and Damyon,

          Something came up my mind to show the due date to students on a clearer manner. Is it possible that the due date is shown in teh upcoming events block and calender? Currently, only the original time is shown and not teh extended due date. I have created an improvement suggestion in Moodle tracker:

          https://tracker.moodle.org/browse/MDL-42952

          Maybe this is what you are looking For Gareth? This way, The students doesn't have to go to the assignment to discover the extended due date. I am curieus for your opinion

          Show
          Mitchell van Gerwen added a comment - Hello Gareth and Damyon, Something came up my mind to show the due date to students on a clearer manner. Is it possible that the due date is shown in teh upcoming events block and calender? Currently, only the original time is shown and not teh extended due date. I have created an improvement suggestion in Moodle tracker: https://tracker.moodle.org/browse/MDL-42952 Maybe this is what you are looking For Gareth? This way, The students doesn't have to go to the assignment to discover the extended due date. I am curieus for your opinion
          Hide
          Gareth J Barnard added a comment - - edited

          Hi Mitchell van Gerwen,

          MDL-42952 looks a very interesting proposition. I did not know that even the due date showed up in the calendar let alone the extended due date was missing. Something I have just voted for.

          In my teaching experience I found that students needed things to be really 'in your face' and as such calendars were something that adults use! Which is why I thought this proposal was very worthwhile because it provided the means to put the information right there. So therefore I'm more in favour of 'it's right there under your nose and you did not have to click on anything special to find it' argument when students use the 'I have not done it because I could not find the deadline date' excuse.

          MDL-42952 does have a baring on the implementation of this as I would see that if the extended date existed (or was different to the due date - I don't know how things work in that area yet) then it should be displayed instead.

          The key sticking point for this issue appears to be not actually how to display the information but how to turn it on and off in the settings.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Hi Mitchell van Gerwen , MDL-42952 looks a very interesting proposition. I did not know that even the due date showed up in the calendar let alone the extended due date was missing. Something I have just voted for. In my teaching experience I found that students needed things to be really 'in your face' and as such calendars were something that adults use! Which is why I thought this proposal was very worthwhile because it provided the means to put the information right there. So therefore I'm more in favour of 'it's right there under your nose and you did not have to click on anything special to find it' argument when students use the 'I have not done it because I could not find the deadline date' excuse. MDL-42952 does have a baring on the implementation of this as I would see that if the extended date existed (or was different to the due date - I don't know how things work in that area yet) then it should be displayed instead. The key sticking point for this issue appears to be not actually how to display the information but how to turn it on and off in the settings. Cheers, Gareth
          Hide
          Mitchell van Gerwen added a comment -

          Hello Gareth,

          Thanks for voting. I am glad someone agrees with me

          Show
          Mitchell van Gerwen added a comment - Hello Gareth, Thanks for voting. I am glad someone agrees with me
          Hide
          Gareth J Barnard added a comment -

          Fixed issue with 'Enabled' and 'Advanced' administrator settings. Now works without alteration to outside assignment core code. Visually identical to previous patch. Branch up to date with version 2014020700.00 release 2.7dev (Build: 20140207). Submitting for peer review.

          Show
          Gareth J Barnard added a comment - Fixed issue with 'Enabled' and 'Advanced' administrator settings. Now works without alteration to outside assignment core code. Visually identical to previous patch. Branch up to date with version 2014020700.00 release 2.7dev (Build: 20140207). Submitting for peer review.
          Hide
          CiBoT added a comment -

          Results for MDL-37490

          • Remote repository: git://github.com/gjb2048/moodle.git
          Show
          CiBoT added a comment - Results for MDL-37490 Remote repository: git://github.com/gjb2048/moodle.git Remote branch wip- MDL-37490 -m9 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1170 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1170/artifact/work/smurf.html
          Hide
          Gareth J Barnard added a comment -

          Fixed missing space issue on line 277 of lib.php as reported by the Smurf under the direction of CiBoT.

          Show
          Gareth J Barnard added a comment - Fixed missing space issue on line 277 of lib.php as reported by the Smurf under the direction of CiBoT.
          Hide
          Mitchell van Gerwen added a comment -

          Hello Gareth,

          I see this new feature is making progress. I also see this new feature will be released in version 2.7. this improvement ( https://tracker.moodle.org/browse/MDL-42952 ) will also be implemented in version 2.7 fine, great!

          Will this new feature also include the extended due date? I am afraid that this new feature for example displays 2 march 2014, but the upcoming events block and calender displays for example 4 march 2014.

          the students will see 2 march 2014 on the course page and the student see the extended due date 4 march 2014 in the upcoming event block and calender. That is not consistent and confusing for the student.

          Will the assigment due date on the course page include teh extended due date?

          Show
          Mitchell van Gerwen added a comment - Hello Gareth, I see this new feature is making progress. I also see this new feature will be released in version 2.7. this improvement ( https://tracker.moodle.org/browse/MDL-42952 ) will also be implemented in version 2.7 fine, great! Will this new feature also include the extended due date? I am afraid that this new feature for example displays 2 march 2014, but the upcoming events block and calender displays for example 4 march 2014. the students will see 2 march 2014 on the course page and the student see the extended due date 4 march 2014 in the upcoming event block and calender. That is not consistent and confusing for the student. Will the assigment due date on the course page include teh extended due date?
          Hide
          Gareth J Barnard added a comment - - edited

          Hi Mitchell van Gerwen,

          Oh, it has only made progress in being made to work with the current code. There is a very long way to go before it gets into Moodle core if at all. Its not currently planned to have the extended due date as I would want this to be accepted first. And the extended due date conceptually is another tracker issue.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - - edited Hi Mitchell van Gerwen , Oh, it has only made progress in being made to work with the current code. There is a very long way to go before it gets into Moodle core if at all. Its not currently planned to have the extended due date as I would want this to be accepted first. And the extended due date conceptually is another tracker issue. Cheers, Gareth
          Hide
          Mitchell van Gerwen added a comment -

          Hello Gareth,

          OK, thank you for the information.

          Greetings,

          Mitchell

          Show
          Mitchell van Gerwen added a comment - Hello Gareth, OK, thank you for the information. Greetings, Mitchell
          Hide
          Gareth J Barnard added a comment -

          Branch rebased.

          Show
          Gareth J Barnard added a comment - Branch rebased.
          Hide
          Gareth J Barnard added a comment -

          Rebased.

          Show
          Gareth J Barnard added a comment - Rebased.
          Hide
          Mary Evans added a comment -

          I'm taking this for a spin...since Damyon is over worked.

          Show
          Mary Evans added a comment - I'm taking this for a spin...since Damyon is over worked.
          Hide
          Mary Evans added a comment -

          I have Peer Reviewed this and cannot find fault with it at all. It works as described. Code looks OK.

          I'm setting this for Integration Review.

          Thanks Gareth.

          Show
          Mary Evans added a comment - I have Peer Reviewed this and cannot find fault with it at all. It works as described. Code looks OK. I'm setting this for Integration Review. Thanks Gareth.
          Hide
          Eloy Lafuente (stronk7) added a comment -

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

          TIA and ciao

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

          Branch rebased

          Show
          Gareth J Barnard added a comment - Branch rebased
          Hide
          CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          Marina Glancy added a comment -

          Hi Gareth, thanks for working on it.

          It looks like this triggered another major problem - I can not upgrade because upgrade page calls some blocks and tries to rebuild course cache:

           Error reading from database
          Debug info: ERROR: column "displayduedate" does not exist
          LINE 1: ...submissionsfromdate, intro, introformat, duedate, displaydue...
          ^
          SELECT id, name, alwaysshowdescription, allowsubmissionsfromdate, intro, introformat, duedate, displayduedate FROM m_assign WHERE id = $1
          [array (
          0 => '1',
          )]
          Error code: dmlreadexception
          Stack trace:
          
              line 443 of /lib/dml/moodle_database.php: dml_read_exception thrown
              line 240 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
              line 760 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
              line 1476 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql()
              line 1448 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
              line 1427 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
              line 263 of /mod/assign/lib.php: call to moodle_database->get_record()
              line 1080 of /course/lib.php: call to assign_get_coursemodule_info()
              line 617 of /lib/modinfolib.php: call to get_array_of_activities()
              line 450 of /lib/modinfolib.php: call to course_modinfo::build_course_cache()
              line 411 of /lib/modinfolib.php: call to course_modinfo->__construct()
              line 2013 of /lib/modinfolib.php: call to course_modinfo::instance()
              line 1832 of /lib/navigationlib.php: call to get_fast_modinfo()
              line 1896 of /lib/navigationlib.php: call to global_navigation->generate_sections_and_activities()
              line 443 of /course/format/lib.php: call to global_navigation->load_generic_course_sections()
              line 1811 of /lib/navigationlib.php: call to format_base->extend_course_navigation()
              line 1114 of /lib/navigationlib.php: call to global_navigation->load_course_sections()
              line 222 of /blocks/navigation/block_navigation.php: call to global_navigation->initialise()
              line 178 of /blocks/navigation/block_navigation.php: call to block_navigation->get_navigation()
              line 294 of /blocks/moodleblock.class.php: call to block_navigation->get_content()
              line 236 of /blocks/moodleblock.class.php: call to block_base->formatted_contents()
              line 956 of /lib/blocklib.php: call to block_base->get_content_for_output()
              line 1008 of /lib/blocklib.php: call to block_manager->create_block_contents()
              line 361 of /lib/outputrenderers.php: call to block_manager->ensure_content_created()
              line 33 of /theme/clean/layout/maintenance.php: call to core_renderer->standard_head_html()
              line 877 of /lib/outputrenderers.php: call to include()
              line 807 of /lib/outputrenderers.php: call to core_renderer->render_page_layout()
              line ? of unknownfile: call to core_renderer->header()
              line 240 of /lib/outputrenderers.php: call to call_user_func_array()
              line 206 of /admin/renderer.php: call to plugin_renderer_base->__call()
              line 206 of /admin/renderer.php: call to core_admin_renderer->header()
              line 417 of /admin/index.php: call to core_admin_renderer->upgrade_plugin_check_page()
          
          Show
          Marina Glancy added a comment - Hi Gareth, thanks for working on it. It looks like this triggered another major problem - I can not upgrade because upgrade page calls some blocks and tries to rebuild course cache: Error reading from database Debug info: ERROR: column "displayduedate" does not exist LINE 1: ...submissionsfromdate, intro, introformat, duedate, displaydue... ^ SELECT id, name, alwaysshowdescription, allowsubmissionsfromdate, intro, introformat, duedate, displayduedate FROM m_assign WHERE id = $1 [array ( 0 => '1', )] Error code: dmlreadexception Stack trace: line 443 of /lib/dml/moodle_database.php: dml_read_exception thrown line 240 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end() line 760 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end() line 1476 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql() line 1448 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql() line 1427 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() line 263 of /mod/assign/lib.php: call to moodle_database->get_record() line 1080 of /course/lib.php: call to assign_get_coursemodule_info() line 617 of /lib/modinfolib.php: call to get_array_of_activities() line 450 of /lib/modinfolib.php: call to course_modinfo::build_course_cache() line 411 of /lib/modinfolib.php: call to course_modinfo->__construct() line 2013 of /lib/modinfolib.php: call to course_modinfo::instance() line 1832 of /lib/navigationlib.php: call to get_fast_modinfo() line 1896 of /lib/navigationlib.php: call to global_navigation->generate_sections_and_activities() line 443 of /course/format/lib.php: call to global_navigation->load_generic_course_sections() line 1811 of /lib/navigationlib.php: call to format_base->extend_course_navigation() line 1114 of /lib/navigationlib.php: call to global_navigation->load_course_sections() line 222 of /blocks/navigation/block_navigation.php: call to global_navigation->initialise() line 178 of /blocks/navigation/block_navigation.php: call to block_navigation->get_navigation() line 294 of /blocks/moodleblock.class.php: call to block_navigation->get_content() line 236 of /blocks/moodleblock.class.php: call to block_base->formatted_contents() line 956 of /lib/blocklib.php: call to block_base->get_content_for_output() line 1008 of /lib/blocklib.php: call to block_manager->create_block_contents() line 361 of /lib/outputrenderers.php: call to block_manager->ensure_content_created() line 33 of /theme/clean/layout/maintenance.php: call to core_renderer->standard_head_html() line 877 of /lib/outputrenderers.php: call to include() line 807 of /lib/outputrenderers.php: call to core_renderer->render_page_layout() line ? of unknownfile: call to core_renderer->header() line 240 of /lib/outputrenderers.php: call to call_user_func_array() line 206 of /admin/renderer.php: call to plugin_renderer_base->__call() line 206 of /admin/renderer.php: call to core_admin_renderer->header() line 417 of /admin/index.php: call to core_admin_renderer->upgrade_plugin_check_page()
          Hide
          Marina Glancy added a comment -

          I created an issue MDL-44882 for it and upgraded via CLI, so continue testing and reviewing:

          Please correct field creation to:

          $field = new xmldb_field('displayduedate', XMLDB_TYPE_INTEGER, '2', null, XMLDB_NOTNULL, null, '0', 'duedate');
          

          Do you use XMLDB editor to create the upgrade code?

          Another concerns is that it does not have a default setting in Site administration>Plugins>Activity modules>Assignment>Assignment settings

          And the last one: due date may be different for different students. You really need to implement callback assign_cm_info_dynamic() or assign_cm_info_view() and inside call $cm->set_content() or $cm->set_after_link(). See example in forum_cm_info_view() that displays number of unread posts.

          Damyon Wiese will be the correct person to ask how to get the ACTUAL due date for the current user.

          And among the minor things - please avoid using string concatenations, use string value 'Due: {$a}' instead. Also add some class to the <p> tag so it can be styled

          I'm reopening this now
          Thanks

          Show
          Marina Glancy added a comment - I created an issue MDL-44882 for it and upgraded via CLI, so continue testing and reviewing: Please correct field creation to: $field = new xmldb_field('displayduedate', XMLDB_TYPE_INTEGER, '2', null , XMLDB_NOTNULL, null , '0', 'duedate'); Do you use XMLDB editor to create the upgrade code? Another concerns is that it does not have a default setting in Site administration>Plugins>Activity modules>Assignment>Assignment settings And the last one: due date may be different for different students. You really need to implement callback assign_cm_info_dynamic() or assign_cm_info_view() and inside call $cm->set_content() or $cm->set_after_link(). See example in forum_cm_info_view() that displays number of unread posts. Damyon Wiese will be the correct person to ask how to get the ACTUAL due date for the current user. And among the minor things - please avoid using string concatenations, use string value 'Due: {$a}' instead. Also add some class to the <p> tag so it can be styled I'm reopening this now Thanks
          Hide
          Damyon Wiese added a comment -

          Hi Gareth,

          I still don't have time to do a full review on this - but Marina asked me to quickly check it. I also don't like the try / catch in the mod_form - you could use isset() or empty() there.

          I am also concerned whether this patch breaks the handling of the defaults for the due date field. I suspect it does but don't have time to test it - this should at least be added to the testing instructions.

          Show
          Damyon Wiese added a comment - Hi Gareth, I still don't have time to do a full review on this - but Marina asked me to quickly check it. I also don't like the try / catch in the mod_form - you could use isset() or empty() there. I am also concerned whether this patch breaks the handling of the defaults for the due date field. I suspect it does but don't have time to test it - this should at least be added to the testing instructions.
          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
          Gareth J Barnard added a comment -

          Hi Marina Glancy and Damyon Wiese,

          Thank you for reviewing the patch, ok:

          1. It was over a year ago that I did the xmldb_field so I cannot remember how I added it.
          2. I'll look at 'Site administration>Plugins>Activity modules>Assignment>Assignment settings' but did think it was there?
          3. I'll look at the due date fetching code as did not find a good API method to call.
          4. I'll think about adding in a class to the 'p' as thought that infinite CSS would be over the top and possibly could be targeted in CSS without a specific tag, even though this patch would only define the class but not actually style it. Perhaps, instead of having so much granular CSS styles there should be a finite 'pallet' of styles used throughout as this clearly falls into the set of 'small text' type things. Having so much granular control makes for a management nightmare. Thoughts Mary Evans?
          5. I'll change the strings as although string concatenation would be faster, I can see the multi-lang issue and why string value is needed.
          6. I'll look into removing try - catch. Odd, must have adapted the code from somewhere, could be other places that this type of thing exists. I only tend to use try - catch in Java.
          7. I'm pretty sure that the handing of defaults is fixed for the due date field but will get around to adding in a test.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Hi Marina Glancy and Damyon Wiese , Thank you for reviewing the patch, ok: It was over a year ago that I did the xmldb_field so I cannot remember how I added it. I'll look at 'Site administration>Plugins>Activity modules>Assignment>Assignment settings' but did think it was there? I'll look at the due date fetching code as did not find a good API method to call. I'll think about adding in a class to the 'p' as thought that infinite CSS would be over the top and possibly could be targeted in CSS without a specific tag, even though this patch would only define the class but not actually style it. Perhaps, instead of having so much granular CSS styles there should be a finite 'pallet' of styles used throughout as this clearly falls into the set of 'small text' type things. Having so much granular control makes for a management nightmare. Thoughts Mary Evans ? I'll change the strings as although string concatenation would be faster, I can see the multi-lang issue and why string value is needed. I'll look into removing try - catch. Odd, must have adapted the code from somewhere, could be other places that this type of thing exists. I only tend to use try - catch in Java. I'm pretty sure that the handing of defaults is fixed for the due date field but will get around to adding in a test. Cheers, Gareth
          Hide
          Mary Evans added a comment -

          I did not see any problem in the way it was styled Gareth, however if I had been doing this I would have enclosed it in a div with class="due-date" just so you can target it easily and style it accordingly.
           

                  $result->content .= html_writer::start_tag('div', array('class'=> 'due-date'));
                  $result->content .= html_writer::tag('p', get_string('duedate', 'assign') . ': ' . userdate($assignment->duedate));
                  $result->content .= html_writer::end_tag('div');
          

          which would translate to this if the due date was today...
           

          <div class="due-date">
          <p>Due date: Monday, 1 April 2014, 12:00 AM</p>
          </div>
          
          Show
          Mary Evans added a comment - I did not see any problem in the way it was styled Gareth, however if I had been doing this I would have enclosed it in a div with class="due-date" just so you can target it easily and style it accordingly.   $result->content .= html_writer::start_tag('div', array('class'=> 'due-date')); $result->content .= html_writer::tag('p', get_string('duedate', 'assign') . ': ' . userdate($assignment->duedate)); $result->content .= html_writer::end_tag('div'); which would translate to this if the due date was today...   <div class= "due-date" > <p>Due date: Monday, 1 April 2014, 12:00 AM</p> </div>

            People

            • Votes:
              12 Vote for this issue
              Watchers:
              11 Start watching this issue

              Dates

              • Created:
                Updated: