Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Attachments

            Activity

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

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

            Show
            timhunt Tim Hunt added a comment - Adding Ruslan and Andrew Nichols as watches, since they implemented this.
            Hide
            yuyokk 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
            yuyokk 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
            timhunt 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
            timhunt 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
            yuyokk 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
            yuyokk 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
            yuyokk Iurii Kucherov added a comment -

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

            Show
            yuyokk Iurii Kucherov added a comment - I looked how it behaved with "Not permitted" and did almost the same with "Forced"
            Hide
            timhunt 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
            timhunt 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
            yuyokk 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
            yuyokk 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.
            danmarsden Dan Marsden made changes -
            Field Original Value New Value
            Labels gsoc2012
            Hide
            salvetore 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
            salvetore 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.
            salvetore Michael de Raadt made changes -
            Fix Version/s STABLE backlog [ 10463 ]
            Labels gsoc2012 gsoc2012 triaged
            Difficulty Easy [ 10023 ]
            Hide
            timhunt 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
            timhunt 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
            kabalin 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
            kabalin 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
            timhunt 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
            timhunt 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
            kabalin Ruslan Kabalin added a comment -

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

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

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

            Show
            kabalin Ruslan Kabalin added a comment - So, M.core_message.init_editsettings should be aware beforehand which are 'permitted' and only touch those.
            Hide
            timhunt 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
            timhunt 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
            kabalin 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
            kabalin 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
            yuyokk 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
            yuyokk 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
            dobedobedoh Andrew Nicols made changes -
            Link This issue is a regression caused by MDL-22232 [ MDL-22232 ]
            Hide
            kabalin 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
            kabalin 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
            saquib764 Saquib Alam added a comment -

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

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

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

            Show
            saquib764 Saquib Alam added a comment - does changing the content of 'value' attribute of checkbox have any effect???
            Hide
            kabalin 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
            kabalin 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
            saquib764 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
            saquib764 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
            kabalin Ruslan Kabalin added a comment -

            Saquib Alam: looking forward to seeing your code

            Show
            kabalin Ruslan Kabalin added a comment - Saquib Alam: looking forward to seeing your code
            Hide
            saquib764 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
            saquib764 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
            saquib764 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
            saquib764 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
            kabalin 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
            kabalin 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
            kabalin 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
            kabalin 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.
            kabalin Ruslan Kabalin made changes -
            kabalin Ruslan Kabalin made changes -
            Assignee Andrew Davis [ andyjdavis ] Ruslan Kabalin [ kabalin ]
            kabalin Ruslan Kabalin made changes -
            Status Open [ 1 ] Waiting for peer review [ 10012 ]
            Hide
            saquib764 Saquib Alam added a comment -

            ya better solution.

            Show
            saquib764 Saquib Alam added a comment - ya better solution.
            Hide
            saquib764 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
            saquib764 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
            kabalin 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
            kabalin 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
            saquib764 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
            saquib764 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
            saquib764 Saquib Alam added a comment -

            no link in MDL-22232

            Show
            saquib764 Saquib Alam added a comment - no link in MDL-22232
            poltawski Dan Poltawski made changes -
            Labels gsoc2012 triaged triaged
            poltawski 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
            poltawski Dan Poltawski added a comment -

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

            Thanks Ruslan

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

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

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

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

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

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

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks for tiding that up Ruslan, this has been integrated now.
            samhemelryk 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 ]
            salvetore Michael de Raadt made changes -
            Tester andyjdavis
            andyjdavis Andrew Davis made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Hide
            andyjdavis Andrew Davis added a comment -

            Works as described in both master and 2.2 stable.

            Show
            andyjdavis Andrew Davis added a comment - Works as described in both master and 2.2 stable.
            andyjdavis Andrew Davis made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            nebgor 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
            nebgor 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!
            nebgor 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:
                  Fix Release Date:
                  14/May/12