Moodle
  1. Moodle
  2. MDL-34791

Activity quick title edit doesn't update name in gradebook

    Details

    • Testing Instructions:
      Hide

      For each of the following affected modules:

      Modules

      • assign
      • assignment
      • data
      • forum
      • glossary
      • lesson
      • lti
      • quiz
      • scorm
      • workshop

      Instructions

      • Create a graded activity and name it "MDL-34791 $activitytype"
      • In a separate tab, open the course gradebook
        • Confirm that the activity 'MDL-34791 $activitytype' is shown in the gradebook
      • Using the quick edit tool, rename it to "MDL-34791 $activitytype updated"
        • Confirm that the activity 'MDL-34791 $activitytype updated' is now shown

      For each of the following unaffected modules:

      Modules

      • book
      • chat
      • choice
      • feedback
      • folder
      • glossary
      • imscp
      • label
      • page
      • resource
      • survey
      • url
      • wiki

      Instructions

      • Create a graded activity and name it "MDL-34791 $activitytype"
      • In a separate tab, open the course gradebook
        • Confirm that the activity is not shown in the gradebook
      • Using the quick edit tool, rename it to "MDL-34791 $activitytype updated"
        • Confirm that no errors were thrown and that the returned data from the AJAX call was valid
      Show
      For each of the following affected modules: Modules assign assignment data forum glossary lesson lti quiz scorm workshop Instructions Create a graded activity and name it " MDL-34791 $activitytype" In a separate tab, open the course gradebook Confirm that the activity ' MDL-34791 $activitytype' is shown in the gradebook Using the quick edit tool, rename it to " MDL-34791 $activitytype updated" Confirm that the activity ' MDL-34791 $activitytype updated' is now shown For each of the following unaffected modules: Modules book chat choice feedback folder glossary imscp label page resource survey url wiki Instructions Create a graded activity and name it " MDL-34791 $activitytype" In a separate tab, open the course gradebook Confirm that the activity is not shown in the gradebook Using the quick edit tool, rename it to " MDL-34791 $activitytype updated" Confirm that no errors were thrown and that the returned data from the AJAX call was valid
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-34791-master
    • Rank:
      43284

      Description

      When you use the quick name editing feature (AJAX course front page tool) to rename an activity, the name change is not reflected in the gradebook. You have to go into the activity and resave.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that. I can confirm this is happening.

          Show
          Michael de Raadt added a comment - Thanks for reporting that. I can confirm this is happening.
          Hide
          Michael de Raadt added a comment -

          I'm increasing the priority of this issue as it seems to be affecting other dependent systems.

          Andrew, if you are able to work on this, please do. Feel free to reassign this to moodle.com.

          Show
          Michael de Raadt added a comment - I'm increasing the priority of this issue as it seems to be affecting other dependent systems. Andrew, if you are able to work on this, please do. Feel free to reassign this to moodle.com.
          Hide
          Andrew Nicols added a comment -

          I've just had a quick look at this - it's not going to be easy.

          At present, each module can choose how it handles updating of a module. They all have a MODULE_update_instance() and dates the form data and the form. Some of these functions then handle things themselves, whilst others handle it with another function.

          Unfortunately we don't have the form data or the form when handling an AJAX update of the module.

          I'm not sure of the best way forward on this issue. I guess there are several options:

          • only allow a rename for modules which define a MODULE_update_instancename() function
          • only allow a rename if a module has FEATURE_RENAME in MODULE_supports()
          • allow a rename for any module, but call a MODULE_update_instancename() if it exists - perhaps combined with MODULE_supports too
          • drop the feature altogether

          I suspect that the third options is probably best but suggestions as to an alternative solution would be welcome.

          Show
          Andrew Nicols added a comment - I've just had a quick look at this - it's not going to be easy. At present, each module can choose how it handles updating of a module. They all have a MODULE_update_instance() and dates the form data and the form. Some of these functions then handle things themselves, whilst others handle it with another function. Unfortunately we don't have the form data or the form when handling an AJAX update of the module. I'm not sure of the best way forward on this issue. I guess there are several options: only allow a rename for modules which define a MODULE_update_instancename() function only allow a rename if a module has FEATURE_RENAME in MODULE_supports() allow a rename for any module, but call a MODULE_update_instancename() if it exists - perhaps combined with MODULE_supports too drop the feature altogether I suspect that the third options is probably best but suggestions as to an alternative solution would be welcome.
          Hide
          Tõnis Tartes added a comment -

          Each gradable module has MODULE_grade_item_update() function, maybe just call that?

          Show
          Tõnis Tartes added a comment - Each gradable module has MODULE_grade_item_update() function, maybe just call that?
          Hide
          Andrew Nicols added a comment -

          Hi Tonis,

          Yes - that looks like it should work. I'll try and work on a patch for that as soon as possible.

          Andrew

          Show
          Andrew Nicols added a comment - Hi Tonis, Yes - that looks like it should work. I'll try and work on a patch for that as soon as possible. Andrew
          Hide
          Andrew Nicols added a comment -

          David, I've added you as a watcher here as maintainer of the Workshop module.

          Whilst all other core modules require that the MODNAME_grade_item_update function be provided with the relevant record from the MODNAME table, and the cmidnumber, there's a comment in mod/workshop/lib.php on this function stating that:

           * @param stdClass $workshop instance object with extra cmidnumber and modname property
          

          However, I can't see any use of the modname property. I don't know whether this is a bug with the documentation (which I suspect) or whether I'm missing something with mod_workshop. The function in mod_workshop never uses the modname property, and never passes the $workshop variable to another function.

          I'm going to put this up for Peer Review now anyway, but when you have a moment I'd appreciate it if you could just sanity check my assessment and comment before I put it up for integration.

          Cheers!

          Andrew

          Show
          Andrew Nicols added a comment - David, I've added you as a watcher here as maintainer of the Workshop module. Whilst all other core modules require that the MODNAME_grade_item_update function be provided with the relevant record from the MODNAME table, and the cmidnumber, there's a comment in mod/workshop/lib.php on this function stating that: * @param stdClass $workshop instance object with extra cmidnumber and modname property However, I can't see any use of the modname property. I don't know whether this is a bug with the documentation (which I suspect) or whether I'm missing something with mod_workshop. The function in mod_workshop never uses the modname property, and never passes the $workshop variable to another function. I'm going to put this up for Peer Review now anyway, but when you have a moment I'd appreciate it if you could just sanity check my assessment and comment before I put it up for integration. Cheers! Andrew
          Hide
          David Mudrak added a comment -

          Hi Andrew

          Sorry I can't remember details now. Most probably it is there copied from the phpDoc block of grade_update_mod_grades() in /lib/gradelib.php. That function calls workshop_grade_item_update() with the instance object like that. So the @param statement actually describes how the incoming data look like if the function is called as a callback from the gradebook. What happened then probably is that I had forgotten about this and had started to consider that @param as a requirement declaration - see for example added workshop_update_grades() call in 10bc4bce2bea574505150317f8cb62f815398c59.

          So I believe is is actually a bug in documentation of that function as at least the 'modname' property is used by the grade_update_mod_grades(). On the other hand, function call chains get pretty complex sometimes so this might need a more detailed review and/or an input from the Gradebook Department guys.

          Show
          David Mudrak added a comment - Hi Andrew Sorry I can't remember details now. Most probably it is there copied from the phpDoc block of grade_update_mod_grades() in /lib/gradelib.php. That function calls workshop_grade_item_update() with the instance object like that. So the @param statement actually describes how the incoming data look like if the function is called as a callback from the gradebook. What happened then probably is that I had forgotten about this and had started to consider that @param as a requirement declaration - see for example added workshop_update_grades() call in 10bc4bce2bea574505150317f8cb62f815398c59. So I believe is is actually a bug in documentation of that function as at least the 'modname' property is used by the grade_update_mod_grades(). On the other hand, function call chains get pretty complex sometimes so this might need a more detailed review and/or an input from the Gradebook Department guys.
          Hide
          Andrew Nicols added a comment -

          I agree with your assessment - I think that it is just a bug in the phpDoc block.

          Since workshop_grade_item_update never passes the whole $workshop variable to another function and never uses $workshop->modname itself, I can't see any potential issues with chaining.

          I've created a issue to remove this from the docs in MDL-36805.

          Show
          Andrew Nicols added a comment - I agree with your assessment - I think that it is just a bug in the phpDoc block. Since workshop_grade_item_update never passes the whole $workshop variable to another function and never uses $workshop->modname itself, I can't see any potential issues with chaining. I've created a issue to remove this from the docs in MDL-36805 .
          Hide
          Damyon Wiese added a comment - - edited

          Peer review checklist:

          [N] Syntax - Coding guidelines specify comments should end with one of .?!
          [-] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git - Although it would be better if the commit message included the component name (e.g. MDL-34791 AJAX, Gradebook: blah)
          [N] Sanity check - You should be calling "grade_update_mod_grades"

          Hi Andrew, Thanks for the patch - the main comment I have is that you should be calling:

          // Attempt to update the grade item.                                                          
          $grademodule = $DB->get_record($cm->modname, array('id' => $cm->instance));                                 
          // The grade_update_mod_grades function requires the course module's idnumber and modname too.                         
          $grademodule->cmidnumber = $cm->idnumber;                                                                   
          $grademodule->modname = $cm->modname;                                                                       
          grade_update_mod_grades($grademodule);     
          

          Instead of checking if the function exists manually - this will do the include for the module lib.php and check for the function. This will also allow us to change this centrally later if we change the api. (You will have to add require_once($CFG->libdir.'/gradelib.php'); to the top of rest.php)

          I'll be happy to take another look at this once you've made these tweaks.

          Thanks - Damyon

          Show
          Damyon Wiese added a comment - - edited Peer review checklist: [N] Syntax - Coding guidelines specify comments should end with one of .?! [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git - Although it would be better if the commit message included the component name (e.g. MDL-34791 AJAX, Gradebook: blah) [N] Sanity check - You should be calling "grade_update_mod_grades" Hi Andrew, Thanks for the patch - the main comment I have is that you should be calling: // Attempt to update the grade item. $grademodule = $DB->get_record($cm->modname, array('id' => $cm->instance)); // The grade_update_mod_grades function requires the course module's idnumber and modname too. $grademodule->cmidnumber = $cm->idnumber; $grademodule->modname = $cm->modname; grade_update_mod_grades($grademodule); Instead of checking if the function exists manually - this will do the include for the module lib.php and check for the function. This will also allow us to change this centrally later if we change the api. (You will have to add require_once($CFG->libdir.'/gradelib.php'); to the top of rest.php) I'll be happy to take another look at this once you've made these tweaks. Thanks - Damyon
          Hide
          Andrew Nicols added a comment -

          Thanks for the review Damyon,

          I've updated the patches with your suggestions. I've put the require_once at the update_title stage rather than the top of the file because it's the only place that course/rest.php requires it.

          Cheers

          Show
          Andrew Nicols added a comment - Thanks for the review Damyon, I've updated the patches with your suggestions. I've put the require_once at the update_title stage rather than the top of the file because it's the only place that course/rest.php requires it. Cheers
          Hide
          Damyon Wiese added a comment -

          Thanks Andrew - this looks OK now (except for a warning about a missing full stop at the end of your comment).

          I think this is fine - but Sam was looking at the same issue on a duplicate ticket (I linked it). I'll check with Sam which solution he prefers.

          • Damyon
          Show
          Damyon Wiese added a comment - Thanks Andrew - this looks OK now (except for a warning about a missing full stop at the end of your comment). I think this is fine - but Sam was looking at the same issue on a duplicate ticket (I linked it). I'll check with Sam which solution he prefers. Damyon
          Hide
          Damyon Wiese added a comment -

          Sam I just added you as a watcher - can you take a look and say which of this ticket or MDL-37094 you prefer?

          Thanks

          Show
          Damyon Wiese added a comment - Sam I just added you as a watcher - can you take a look and say which of this ticket or MDL-37094 you prefer? Thanks
          Hide
          Sam Hemelryk added a comment -

          Best use this one, its older and work has already started

          Show
          Sam Hemelryk added a comment - Best use this one, its older and work has already started
          Hide
          Damyon Wiese added a comment -

          Thanks Sam, this looks OK from me then.

          Show
          Damyon Wiese added a comment - Thanks Sam, this looks OK from me then.
          Hide
          Dan Poltawski added a comment -

          Thanks guys, i've integrated this to 23, 24 and master.

          One comment was that maybe it would be worth making that get_record call MUST_EXIST. But I thought it would be best to get this integrated and have that debate elsewhere.

          Show
          Dan Poltawski added a comment - Thanks guys, i've integrated this to 23, 24 and master. One comment was that maybe it would be worth making that get_record call MUST_EXIST. But I thought it would be best to get this integrated and have that debate elsewhere.
          Hide
          David Monllaó added a comment -

          All works as expected except the LTI activity, the name is not updated in the gradebook. Same problem in master and in 23

          Show
          David Monllaó added a comment - All works as expected except the LTI activity, the name is not updated in the gradebook. Same problem in master and in 23
          Hide
          Dan Poltawski added a comment -

          Ping, Andrew?

          Show
          Dan Poltawski added a comment - Ping, Andrew?
          Hide
          Andrew Nicols added a comment -

          This is a bug in the LTI module (or arguably lib/gradelib.php::grade_update_mod_grades()).

          grade_update_mod_grades() only calls the grade_item_update function if both:

          • MODNAME_grade_item_update; and
          • MODNAME_update_grades
            exist.

          If one of them doesn't exist, then neither is called.

          In the case of the LTI module only the _grade_item_update function exists; and _updatee_grades does not so neither get called.

          Show
          Andrew Nicols added a comment - This is a bug in the LTI module (or arguably lib/gradelib.php::grade_update_mod_grades()). grade_update_mod_grades() only calls the grade_item_update function if both: MODNAME_grade_item_update; and MODNAME_update_grades exist. If one of them doesn't exist, then neither is called. In the case of the LTI module only the _grade_item_update function exists; and _updatee_grades does not so neither get called.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks' for the promptly research Andrew!

          I've created MDL-37168 as followup for the LTI case, so this can be considered passed.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks' for the promptly research Andrew! I've created MDL-37168 as followup for the LTI case, so this can be considered passed. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

            People

            • Votes:
              10 Vote for this issue
              Watchers:
              13 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: