Moodle
  1. Moodle
  2. MDL-29239

Users without moodle/course:changesummary permission can not edit other course fields

    Details

    • Testing Instructions:
      Hide

      As admin

      1. create a course and assign a teacher.
      2. go to site admin > users > permissions > define roles
      3. edit teacher (editingteacher)role, set moodle/course:changesummary to prevent and save

      login as teacher

      1. access the course and edit the course settings.
      2. change the number of topics to some numbers and save it.

      Make sure, there's no error occurs for the submit.

      Show
      As admin create a course and assign a teacher. go to site admin > users > permissions > define roles edit teacher (editingteacher)role, set moodle/course:changesummary to prevent and save login as teacher access the course and edit the course settings. change the number of topics to some numbers and save it. Make sure, there's no error occurs for the submit.
    • Workaround:
      Hide

      One possible workaround which seems to have no secondary issues is to include the following lines in the file course/edit.php, which manually sets previous summary text and format.

      } else {
      // Save any changes to the files used in the editor
      if (!empty($course->id) and !has_capability('moodle/course:changesummary', $coursecontext))

      { $data->summary_editor["text"]=$course->summary; $data->summary_editor["format"]=1; }

      update_course($data, $editoroptions);
      }

      Show
      One possible workaround which seems to have no secondary issues is to include the following lines in the file course/edit.php, which manually sets previous summary text and format. } else { // Save any changes to the files used in the editor if (!empty($course->id) and !has_capability('moodle/course:changesummary', $coursecontext)) { $data->summary_editor["text"]=$course->summary; $data->summary_editor["format"]=1; } update_course($data, $editoroptions); }
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      If one removes moodle/course:changesummary permission the user can not edit other course fields, such as, for example, the number of topics or the format of the course because the form always requires summary and summaryformat to be specified. However, the user can not edit them since they are invisible as expected.

      Replication instructions:

      1. remove moodle/course:changesummary from teacher role
      2. edit a course configuration
      3. summary / summaryformat fields are not displayed
      4. change number of topics of the course and save the form

      Moodle throws an error writing to the database.

      The reason is:

      Debug info: Column 'summaryformat' cannot be null
      UPDATE course SET category = ?,fullname = ?,shortname = ?,idnumber = ?,format = ?,numsections = ?,startdate = ?,hiddensections = ?,newsitems = ?,showgrades = ?,showreports = ?,maxbytes = ?,groupmode = ?,groupmodeforce = ?,defaultgroupingid = ?,visible = ?,lang = ?,enablecompletion = ?,completionstartonenrol = ?,restrictmodules = ?,timemodified = ?,summary = ?,summaryformat = ? WHERE id=?
      [array (
      0 => '10',
      1 => 'COURSE',
      2 => 'SHORTNAME',
      3 => '',
      4 => 'topics',
      5 => '12',
      6 => 1314831600,
      7 => '0',
      8 => '5',
      9 => '1',
      10 => '0',
      11 => '33554432',
      12 => '0',
      13 => '0',
      14 => '0',
      15 => '1',
      16 => '',
      17 => 0,
      18 => 0,
      19 => 0,
      20 => 1315255126,
      21 => NULL,
      22 => NULL,
      23 => 32,
      )]

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks for reporting this.

            Could you please provide details of your DBMS? This may be relevant.

            I've put it on our backlog and we'll try to get to it as soon as we can.

            In the meantime adding more information, such as screenshots, a workaround or even a code solution, will help us and other users. If you do add a solution, please add a 'patch' label.

            Show
            Michael de Raadt added a comment - Thanks for reporting this. Could you please provide details of your DBMS? This may be relevant. I've put it on our backlog and we'll try to get to it as soon as we can. In the meantime adding more information, such as screenshots, a workaround or even a code solution, will help us and other users. If you do add a solution, please add a 'patch' label.
            Hide
            Jozas Nhial added a comment - - edited
            • MYSQL

            I was not able to find a workaround yet, but I think the problem is related to the hardFreeze of the summary_editor:

            if (!empty($course->id) and !has_capability('moodle/course:changesummary', $coursecontext))

            { $mform->hardFreeze('summary_editor'); }

            All frozen fields due to the lack of permissions to change them (for example, fullname) are displayed in the form, with no possibility of edition. However, the summary and its format are not shown at all.

            The other fields have a setConstant function associated, so I thought on setting:

            if (!empty($course->id) and !has_capability('moodle/course:changesummary', $coursecontext))

            { $mform->hardFreeze('summary_editor'); $mform->setConstant('summary_editor'); }

            However upon call of $element->onQuickFormEvent('updateValue', null, $this);

            The values are not set:

            ["_values":protected]=> array(3)

            { ["text"]=> NULL ["format"]=> NULL ["itemid"]=> NULL }

            Finally, there is a comment in lib/form/editor.php stating:

            //TODO:
            // * locking
            // * freezing
            // * ajax format conversion

            class MoodleQuickForm_editor extends HTML_QuickForm_element {

            Show
            Jozas Nhial added a comment - - edited MYSQL I was not able to find a workaround yet, but I think the problem is related to the hardFreeze of the summary_editor: if (!empty($course->id) and !has_capability('moodle/course:changesummary', $coursecontext)) { $mform->hardFreeze('summary_editor'); } All frozen fields due to the lack of permissions to change them (for example, fullname) are displayed in the form, with no possibility of edition. However, the summary and its format are not shown at all. The other fields have a setConstant function associated, so I thought on setting: if (!empty($course->id) and !has_capability('moodle/course:changesummary', $coursecontext)) { $mform->hardFreeze('summary_editor'); $mform->setConstant('summary_editor'); } However upon call of $element->onQuickFormEvent('updateValue', null, $this); The values are not set: ["_values":protected] => array(3) { ["text"]=> NULL ["format"]=> NULL ["itemid"]=> NULL } Finally, there is a comment in lib/form/editor.php stating: //TODO: // * locking // * freezing // * ajax format conversion class MoodleQuickForm_editor extends HTML_QuickForm_element {
            Hide
            Chris Follin added a comment -

            I took Jozas' suggestion and made a patch for it with just a few small changes. This may or may not be the best way to fix this (I honestly don't know) but it does work.

            Show
            Chris Follin added a comment - I took Jozas' suggestion and made a patch for it with just a few small changes. This may or may not be the best way to fix this (I honestly don't know) but it does work.
            Hide
            Rossiani Wijaya added a comment -

            Thanks Chris for creating patch. Your patch works for addressing the error. However, I think it should be fixed through form editor.

            I created a patch to fix this issue.

            Assigning Ankit as peer reviewer.

            Show
            Rossiani Wijaya added a comment - Thanks Chris for creating patch. Your patch works for addressing the error. However, I think it should be fixed through form editor. I created a patch to fix this issue. Assigning Ankit as peer reviewer.
            Hide
            Rossiani Wijaya added a comment -

            Assigning Sam as peer reviewer.

            Show
            Rossiani Wijaya added a comment - Assigning Sam as peer reviewer.
            Hide
            Sam Hemelryk added a comment -

            Hi Rossi,
            I've just been looking at this now.
            It's taken me a long time to study up on how the editor element works but I think I've managed to wrap my head around it enough to get it to work.
            I think the change to getValue is spot on.
            However the change to getFrozenHtml I don't like.
            If you did want to call format_text you need to do so providing the format the text uses ($this->_values['format']) as well as the context provided ($this->options['context']).
            Also no need for the $field argument?
            However I don't think that is at all necessary as the element template used when rendering a frozen editor is 'nodisplay' so even if you do format it (and it is called) it is never actually displayed.
            Because of this editors are not displayed at all when frozen, unlike most other elements.

            As this issue isn't about the display of a frozen editor I think that you could simply fix this by having getFrozenHtml returning and empty string, or something like that... it would be enough I think.

            We could then create another issue to properly fix the display of a frozen editor. No reason that I can see why we can't display a frozen editor.

            I would perhaps recommend getting Petr to take a peak at this, he was the brains behind the editor element originally and may have some good ideas.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Rossi, I've just been looking at this now. It's taken me a long time to study up on how the editor element works but I think I've managed to wrap my head around it enough to get it to work. I think the change to getValue is spot on. However the change to getFrozenHtml I don't like. If you did want to call format_text you need to do so providing the format the text uses ($this->_values ['format'] ) as well as the context provided ($this->options ['context'] ). Also no need for the $field argument? However I don't think that is at all necessary as the element template used when rendering a frozen editor is 'nodisplay' so even if you do format it (and it is called) it is never actually displayed. Because of this editors are not displayed at all when frozen, unlike most other elements. As this issue isn't about the display of a frozen editor I think that you could simply fix this by having getFrozenHtml returning and empty string, or something like that... it would be enough I think. We could then create another issue to properly fix the display of a frozen editor. No reason that I can see why we can't display a frozen editor. I would perhaps recommend getting Petr to take a peak at this, he was the brains behind the editor element originally and may have some good ideas. Cheers Sam
            Hide
            Rossiani Wijaya added a comment -

            Adding Petr to watcher list.

            Hi Sam,

            Thanks for the review. I updated the patch according to your suggestion.

            Hi Petr,

            when you have a chance, could you take a look the above patch?

            Thanks.
            Rosie

            Show
            Rossiani Wijaya added a comment - Adding Petr to watcher list. Hi Sam, Thanks for the review. I updated the patch according to your suggestion. Hi Petr, when you have a chance, could you take a look the above patch? Thanks. Rosie
            Hide
            Rossiani Wijaya added a comment -

            Ping Petr

            Show
            Rossiani Wijaya added a comment - Ping Petr
            Hide
            Rossiani Wijaya added a comment -

            Can't get any feedback from Petr.

            I'm submitting this for intergration.

            Show
            Rossiani Wijaya added a comment - Can't get any feedback from Petr. I'm submitting this for intergration.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I'm passing this to Sam after chatting with him. I think the getValue() change is the one solving the problem and, the getFrozenHTML() is just an unrelated visualization thing (aka can be empty, or can be the format_text() output, or whatever we want, but always unrelated with the DB error).

            So, as commented above, I'd create one new issue (master-only) about to know why "empty/hidden" was decided to be used by default and to decide if it's correct/safe to change that default to display the contents instead.

            Anyway, Sam has the last word, here. I just +0.4 current patch.

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - I'm passing this to Sam after chatting with him. I think the getValue() change is the one solving the problem and, the getFrozenHTML() is just an unrelated visualization thing (aka can be empty, or can be the format_text() output, or whatever we want, but always unrelated with the DB error). So, as commented above, I'd create one new issue (master-only) about to know why "empty/hidden" was decided to be used by default and to decide if it's correct/safe to change that default to display the contents instead. Anyway, Sam has the last word, here. I just +0.4 current patch. Ciao
            Hide
            Sam Hemelryk added a comment -

            Thanks guys this has been integrated now.

            Show
            Sam Hemelryk added a comment - Thanks guys this has been integrated now.
            Hide
            Ankit Agarwal added a comment -

            Hi,
            This is working great!
            Passing
            Thanks

            Show
            Ankit Agarwal added a comment - Hi, This is working great! Passing Thanks
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

            Now... disconnect, relax and enjoy the next days, yay!

            Closing...ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: