Moodle
  1. Moodle
  2. MDL-32137

When a messaging output is set to forced, users should not be able to change that

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.2
    • Fix Version/s: 2.2.3
    • Component/s: Messages
    • Labels:
    • Testing Instructions:
      Hide
      1. Go to Site administration > Plugins > Message outputs > Default message outputs, and set something to forced and something else to disallowed.
      2. As user go to My profile settings > Messaging - note that for the things you set to forced and disallowed now have corresponding text and do not have checkboxes.
      Show
      Go to Site administration > Plugins > Message outputs > Default message outputs, and set something to forced and something else to disallowed. As user go to My profile settings > Messaging - note that for the things you set to forced and disallowed now have corresponding text and do not have checkboxes.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32137-master-1
    • Rank:
      38859

      Description

      Steps to reproduce:

      1. Go to Site administration > Plugins > Message outputs > Default message outputs, and set something to forced.

      2. Go to My profile settings > Messaging - note that for the thing you set to forced, there are two checkboxes, and you appear to be able to un-tick them.

      What was expected. For the forced setting there should be some text like "Is always used" instead of the checkboxes, similar to the "Not permitted" text for things that are Disallowed on Default message outputs.

        Activity

        Tim Hunt created issue -
        Hide
        Tim Hunt added a comment -

        Adding Ruslan and Andrew Nichols as watches, since they implemented this.

        Show
        Tim Hunt added a comment - Adding Ruslan and Andrew Nichols as watches, since they implemented this.
        Hide
        Iurii Kucherov added a comment - - edited

        Please have a look at it
        https://github.com/yuyokk/moodle/compare/master...MDL-32137-master

        I've added text Forced instead of the checkboxes

        Show
        Iurii Kucherov added a comment - - edited Please have a look at it https://github.com/yuyokk/moodle/compare/master...MDL-32137-master I've added text Forced instead of the checkboxes
        Hide
        Tim Hunt added a comment -

        It looks to me like the code that was already there was nearly right, and was trying to do something different (better). Why didn't you just fix the existing code so it worked?

        Show
        Tim Hunt added a comment - It looks to me like the code that was already there was nearly right, and was trying to do something different (better). Why didn't you just fix the existing code so it worked?
        Hide
        Iurii Kucherov added a comment -

        Actually I changed few lines only.
        Also I changed indention to look more readable so it looks like I changed a lot of lines.
        Or what did mean ?

        Show
        Iurii Kucherov added a comment - Actually I changed few lines only. Also I changed indention to look more readable so it looks like I changed a lot of lines. Or what did mean ?
        Hide
        Iurii Kucherov added a comment -

        I looked how it behaved with "Not permitted" and did almost the same with "Forced"

        Show
        Iurii Kucherov added a comment - I looked how it behaved with "Not permitted" and did almost the same with "Forced"
        Hide
        Tim Hunt added a comment -

        Read the code for 'forced' choices as it was before your changes (the bit in red at https://github.com/yuyokk/moodle/compare/master...MDL-32137-master). What was that code trying to do?

        Show
        Tim Hunt added a comment - Read the code for 'forced' choices as it was before your changes (the bit in red at https://github.com/yuyokk/moodle/compare/master...MDL-32137-master ). What was that code trying to do?
        Hide
        Iurii Kucherov added a comment - - edited

        It is set always to checked.
        After user have unchecked it and press Update it becomes checked again.
        So I shouldn't delete that lines.
        I'll try to add new patch.

        Show
        Iurii Kucherov added a comment - - edited It is set always to checked. After user have unchecked it and press Update it becomes checked again. So I shouldn't delete that lines. I'll try to add new patch.
        Dan Marsden made changes -
        Field Original Value New Value
        Labels gsoc2012
        Hide
        Michael de Raadt added a comment -

        Thanks for spotting this, Tim. And thanks for working on this, Iurii.

        Iurri: if you can add a new patch, that would be great.

        Show
        Michael de Raadt added a comment - Thanks for spotting this, Tim. And thanks for working on this, Iurii. Iurri: if you can add a new patch, that would be great.
        Michael de Raadt made changes -
        Fix Version/s STABLE backlog [ 10463 ]
        Labels gsoc2012 gsoc2012 triaged
        Difficulty Easy [ 10023 ]
        Hide
        Tim Hunt added a comment -

        Iurii still has not got it.

        The actual bug seems to be the line

        $disabled['disabled'] = 1;

        It looks to me like that is trying to make the 'forced' checkboxes disabled, but it not working. I think the fix is as simple as changing it to

        $disabled['disabled'] = 'disabled';

        but someone needs to test that and make a patch.

        Show
        Tim Hunt added a comment - Iurii still has not got it. The actual bug seems to be the line $disabled ['disabled'] = 1; It looks to me like that is trying to make the 'forced' checkboxes disabled, but it not working. I think the fix is as simple as changing it to $disabled ['disabled'] = 'disabled'; but someone needs to test that and make a patch.
        Hide
        Ruslan Kabalin added a comment - - edited

        Strange thing Tim is that it definitely worked with $disabled['disabled'] = 1; it has been tested dozen of times before integration. I will have a look in a moment.

        Show
        Ruslan Kabalin added a comment - - edited Strange thing Tim is that it definitely worked with $disabled ['disabled'] = 1; it has been tested dozen of times before integration. I will have a look in a moment.
        Hide
        Tim Hunt added a comment -

        Ruslan, I was sure this used to work too, but it is definitely broken now. (I am on FF 11, if that matters. FF11 seems to have messed up TinyMCE in 2.1 a bit too.) Weird.

        Show
        Tim Hunt added a comment - Ruslan, I was sure this used to work too, but it is definitely broken now. (I am on FF 11, if that matters. FF11 seems to have messed up TinyMCE in 2.1 a bit too.) Weird.
        Hide
        Ruslan Kabalin added a comment -

        Found. This is nothing about $disabled['disabled'] a pure JS issue - regression caused by bb3546f3 (MDL-22232)

        Show
        Ruslan Kabalin added a comment - Found. This is nothing about $disabled ['disabled'] a pure JS issue - regression caused by bb3546f3 (MDL-22232)
        Hide
        Ruslan Kabalin added a comment -

        So, M.core_message.init_editsettings should be aware beforehand which are 'permitted' and only touch those.

        Show
        Ruslan Kabalin added a comment - So, M.core_message.init_editsettings should be aware beforehand which are 'permitted' and only touch those.
        Hide
        Tim Hunt added a comment -

        Right. Good detective work.

        So, the question is, do we try to fix the existing code, or do we just change to code like Iurii's and replace the checkboxes by a textual message in this case?

        Show
        Tim Hunt added a comment - Right. Good detective work. So, the question is, do we try to fix the existing code, or do we just change to code like Iurii's and replace the checkboxes by a textual message in this case?
        Hide
        Ruslan Kabalin added a comment -

        Iurii's code sounds like an easier solution here, since JS will not need to change what it is not supposed to. However, Iurii's existing code requires some refactoring, there is no need to duplicate the code from Disallowed bit, existing 'disallowed' condition block can be just modified to accommodate both Disallowed and Forced, and 'forced' bit can be removed from 'else' block completely.

        Show
        Ruslan Kabalin added a comment - Iurii's code sounds like an easier solution here, since JS will not need to change what it is not supposed to. However, Iurii's existing code requires some refactoring, there is no need to duplicate the code from Disallowed bit, existing 'disallowed' condition block can be just modified to accommodate both Disallowed and Forced, and 'forced' bit can be removed from 'else' block completely.
        Hide
        Iurii Kucherov added a comment - - edited

        I didn't understand why $disabled['disabled'] = 1 didn't work
        I've got no idea how to fix that
        So my variant was to replace checkbox by a 'Forced' text

        Show
        Iurii Kucherov added a comment - - edited I didn't understand why $disabled ['disabled'] = 1 didn't work I've got no idea how to fix that So my variant was to replace checkbox by a 'Forced' text
        Andrew Nicols made changes -
        Link This issue is a regression caused by MDL-22232 [ MDL-22232 ]
        Hide
        Ruslan Kabalin added a comment -

        <off-topic> In addition, there is a room for refactoring bb3546f3 , ListNode object already has methods setAttribute/removeAttribute, so in this simple scenario, there is no need to iterate through each node and modify them one by one.</off-topic>

        Show
        Ruslan Kabalin added a comment - <off-topic> In addition, there is a room for refactoring bb3546f3 , ListNode object already has methods setAttribute/removeAttribute, so in this simple scenario, there is no need to iterate through each node and modify them one by one.</off-topic>
        Hide
        Saquib Alam added a comment -

        bug fixed.
        Mentor : how do i show u fixed code?

        Show
        Saquib Alam added a comment - bug fixed. Mentor : how do i show u fixed code?
        Hide
        Saquib Alam added a comment -

        does changing the content of 'value' attribute of checkbox have any effect???

        Show
        Saquib Alam added a comment - does changing the content of 'value' attribute of checkbox have any effect???
        Hide
        Ruslan Kabalin added a comment - - edited

        Mentor : how do i show u fixed code?

        Post a link to diff on public git repository or just attach the diff file.

        does changing the content of 'value' attribute of checkbox have any effect???

        No, read through comments above, the issue is related to JavaScript that overrides disabled checkboxes and make them enabled. One of solution I have posted in this comment.

        Show
        Ruslan Kabalin added a comment - - edited Mentor : how do i show u fixed code? Post a link to diff on public git repository or just attach the diff file. does changing the content of 'value' attribute of checkbox have any effect??? No, read through comments above, the issue is related to JavaScript that overrides disabled checkboxes and make them enabled. One of solution I have posted in this comment.
        Hide
        Saquib Alam added a comment - - edited

        yup i got the point of over ridding..
        actually i used the 'value' to define condition in javascript.

        >to make M.core_message.init_editsettings aware of permissions.

        so i was bothered that this might have some impact somewhere..

        well, thanks.

        Show
        Saquib Alam added a comment - - edited yup i got the point of over ridding.. actually i used the 'value' to define condition in javascript. >to make M.core_message.init_editsettings aware of permissions. so i was bothered that this might have some impact somewhere.. well, thanks.
        Hide
        Ruslan Kabalin added a comment -

        Saquib Alam: looking forward to seeing your code

        Show
        Ruslan Kabalin added a comment - Saquib Alam: looking forward to seeing your code
        Hide
        Saquib Alam added a comment - - edited

        In renderer.php

        // determine user preferences and use then unless we force
        // the preferences.
        $disabled = array();
        $value=2;
        if ($permitted == 'forced') {
        $checked = true;
        $disabled['disabled'] = 1;

        • $value=1;* //ALL STUFFS

        $label = get_string('sendingviawhen', 'message', $labelparams);
        $cellcontent = html_writer::label($label, $elementname, true, array('class' => 'accesshide'));
        $cellcontent .= html_writer::checkbox($elementname, $value, $checked, '', array_merge(array('id' => $elementname, 'class' => 'notificationpreference'), $disabled));

        // ALL OTHER STUFFS

        Show
        Saquib Alam added a comment - - edited In renderer.php // determine user preferences and use then unless we force // the preferences. $disabled = array(); $value=2; if ($permitted == 'forced') { $checked = true; $disabled ['disabled'] = 1; $value=1;* //ALL STUFFS $label = get_string('sendingviawhen', 'message', $labelparams); $cellcontent = html_writer::label($label, $elementname, true, array('class' => 'accesshide')); $cellcontent .= html_writer::checkbox($elementname, $value , $checked, '', array_merge(array('id' => $elementname, 'class' => 'notificationpreference'), $disabled)); // ALL OTHER STUFFS
        Hide
        Saquib Alam added a comment -

        in module.js in (M.core_message.init_editsettings)

        var disabled = e.target.get('checked');
        if(node.get('value')==2)
        { node.removeAttribute('disabled');
        if (disabled)

        { node.setAttribute('disabled', 1) }

        }

        //REST SAME

        Show
        Saquib Alam added a comment - in module.js in (M.core_message.init_editsettings) var disabled = e.target.get('checked'); if(node.get('value')==2) { node.removeAttribute('disabled'); if (disabled) { node.setAttribute('disabled', 1) } } //REST SAME
        Hide
        Ruslan Kabalin added a comment -

        Hi Saquib, it is difficult to see what you have changed, please provide a diff if you can (this doc may help you http://docs.moodle.org/dev/How_to_create_a_patch#Creating_a_patch_using_Git).

        In general, using value for tracking items in JS does not sound right to me, it might look very creative, but in terms of future support it makes the life of programmer more difficult and breaks the consistency of defining checked items as '1' in the config table, which might have unexpected implications.

        Show
        Ruslan Kabalin added a comment - Hi Saquib, it is difficult to see what you have changed, please provide a diff if you can (this doc may help you http://docs.moodle.org/dev/How_to_create_a_patch#Creating_a_patch_using_Git ). In general, using value for tracking items in JS does not sound right to me, it might look very creative, but in terms of future support it makes the life of programmer more difficult and breaks the consistency of defining checked items as '1' in the config table, which might have unexpected implications.
        Hide
        Ruslan Kabalin added a comment -

        As Tim said, the easiest probably will be replace checkboxes with text notification. The suggested solution also replaces "not permitted" message with one corresponding to default settings (e.g. Forced or Disallowed) to provide user with more intuition why preference is not permitted. 'Forced' condition has been removed from 'else' block and merged together with initial check of whether setting is overridden. Added the branch with a fix.

        Show
        Ruslan Kabalin added a comment - As Tim said, the easiest probably will be replace checkboxes with text notification. The suggested solution also replaces "not permitted" message with one corresponding to default settings (e.g. Forced or Disallowed) to provide user with more intuition why preference is not permitted. 'Forced' condition has been removed from 'else' block and merged together with initial check of whether setting is overridden. Added the branch with a fix.
        Ruslan Kabalin made changes -
        Ruslan Kabalin made changes -
        Assignee Andrew Davis [ andyjdavis ] Ruslan Kabalin [ kabalin ]
        Ruslan Kabalin made changes -
        Status Open [ 1 ] Waiting for peer review [ 10012 ]
        Hide
        Saquib Alam added a comment -

        ya better solution.

        Show
        Saquib Alam added a comment - ya better solution.
        Hide
        Saquib Alam added a comment -

        what actually "Temporarily disable notifications" is doing??
        is it only restricting user from making changes or blocking all notification?

        Show
        Saquib Alam added a comment - what actually "Temporarily disable notifications" is doing?? is it only restricting user from making changes or blocking all notification?
        Hide
        Ruslan Kabalin added a comment -

        is it only restricting user from making changes or blocking all notification?

        To be frank, I did not look deep into it, but it seems it is disabling all notifications for particular user. See MDL-22232 for details.

        Show
        Ruslan Kabalin added a comment - is it only restricting user from making changes or blocking all notification? To be frank, I did not look deep into it, but it seems it is disabling all notifications for particular user. See MDL-22232 for details.
        Hide
        Saquib Alam added a comment -

        By looking at M.core_message.init_editsettings is seem like its only restricting changes.
        but user should still recieve notifications because its not making them unchecked..

        Show
        Saquib Alam added a comment - By looking at M.core_message.init_editsettings is seem like its only restricting changes. but user should still recieve notifications because its not making them unchecked..
        Hide
        Saquib Alam added a comment -

        no link in MDL-22232

        Show
        Saquib Alam added a comment - no link in MDL-22232
        Dan Poltawski made changes -
        Labels gsoc2012 triaged triaged
        Dan Poltawski made changes -
        Original Estimate 0 minutes [ 0 ]
        Remaining Estimate 0 minutes [ 0 ]
        Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
        Peer reviewer poltawski
        Hide
        Dan Poltawski added a comment -

        Looks good to me - this should be in 2.2 also.

        Thanks Ruslan

        Show
        Dan Poltawski added a comment - Looks good to me - this should be in 2.2 also. Thanks Ruslan
        Dan Poltawski made changes -
        Status Peer review in progress [ 10013 ] Development in progress [ 3 ]
        Hide
        Ruslan Kabalin added a comment -

        Ready for integration, can be cleanly cherry-picked to 2.2

        Show
        Ruslan Kabalin added a comment - Ready for integration, can be cleanly cherry-picked to 2.2
        Ruslan Kabalin made changes -
        Status Development in progress [ 3 ] Waiting for integration review [ 10010 ]
        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
        Sam Hemelryk made changes -
        Currently in integration Yes [ 10041 ]
        Dan Marsden made changes -
        Labels triaged gsoc2012 triaged
        Sam Hemelryk made changes -
        Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
        Integrator samhemelryk
        Hide
        Sam Hemelryk added a comment -

        Hi Ruslan,

        I've just been looking at this now, there are a couple of very minor things that need to happen before this gets integrated.

        1. We can't backport it at the moment as we never remove strings from stable branches. Could you please produce a 22 branch that doesn't remove the messaging string.
        2. Can you please add testing instructions to this issue.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi Ruslan, I've just been looking at this now, there are a couple of very minor things that need to happen before this gets integrated. We can't backport it at the moment as we never remove strings from stable branches. Could you please produce a 22 branch that doesn't remove the messaging string. Can you please add testing instructions to this issue. Cheers Sam
        Hide
        Sam Hemelryk added a comment - - edited

        Forgot to mention if you don't have time to look at it no probs let me know and I'll make the changes.
        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - - edited Forgot to mention if you don't have time to look at it no probs let me know and I'll make the changes. Cheers Sam
        Ruslan Kabalin made changes -
        Ruslan Kabalin made changes -
        Testing Instructions # Go to Site administration > Plugins > Message outputs > Default message outputs, and set something to forced and something else to disallowed.
        # As user go to My profile settings > Messaging - note that for the things you set to forced and disallowed now have corresponding text and do not have checkboxes.
        Hide
        Ruslan Kabalin added a comment -

        Thanks Sam for comments, I have done everything you requested.

        Show
        Ruslan Kabalin added a comment - Thanks Sam for comments, I have done everything you requested.
        Hide
        Sam Hemelryk added a comment -

        Thanks for tiding that up Ruslan, this has been integrated now.

        Show
        Sam Hemelryk added a comment - Thanks for tiding that up Ruslan, this has been integrated now.
        Sam Hemelryk made changes -
        Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
        Fix Version/s 2.2.3 [ 12053 ]
        Fix Version/s STABLE backlog [ 10463 ]
        Michael de Raadt made changes -
        Tester andyjdavis
        Andrew Davis made changes -
        Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
        Hide
        Andrew Davis added a comment -

        Works as described in both master and 2.2 stable.

        Show
        Andrew Davis added a comment - Works as described in both master and 2.2 stable.
        Andrew Davis made changes -
        Status Testing in progress [ 10011 ] Tested [ 10006 ]
        Hide
        Aparup Banerjee added a comment -

        The code here has been spread to upstream moodle repositories and mirrors for anyone to use .

        Closing, have a good weekend!

        Show
        Aparup Banerjee added a comment - The code here has been spread to upstream moodle repositories and mirrors for anyone to use . Closing, have a good weekend!
        Aparup Banerjee made changes -
        Status Tested [ 10006 ] Closed [ 6 ]
        Resolution Fixed [ 1 ]
        Currently in integration Yes [ 10041 ]
        Integration date 05/Apr/12

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: