Moodle
  1. Moodle
  2. MDL-30438

Password field is not disabled even if password protect is set to NO in a Lesson

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.5, 2.1.3, 2.3.6, 2.4.3
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Lesson
    • Labels:
    • Testing Instructions:
      Hide
      1. Login as a teacher, update a lesson.
      2. Set 'Password protected lesson' to 'NO'
        Make sure the password and unmask fields are disable.
        Enter some text in the password field and toggle unmask password.
      1. Set 'Password protected lesson' to 'Yes'
        Make sure the password and unmask fields are enabled.
        Enter some text in the password field and toggle unmask password.

      It is very important that this is tested with IE 9+ as well as other browsers.

      Show
      Login as a teacher, update a lesson. Set 'Password protected lesson' to 'NO' Make sure the password and unmask fields are disable. Enter some text in the password field and toggle unmask password. Set 'Password protected lesson' to 'Yes' Make sure the password and unmask fields are enabled. Enter some text in the password field and toggle unmask password. It is very important that this is tested with IE 9+ as well as other browsers.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      MDL-30438_b_m24
    • Pull Master Branch:
    • Rank:
      33101

      Description

      Copied for linked issue:-
      Not sure if this a major issue, but observed following:
      If Password protection lesson is set to No, password field is disabled. Although unmask checkbox is enabled, and checking that enables password field. As password is enabled, any value in that actually gets saved as well.

        Issue Links

          Activity

          Hide
          Adrian Greeve added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [*] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Hi Rossie,

          I like the patch, I think that the solution that you have provided is good, just a couple of things:

          • Perhaps in the testing instructions you could mention clicking the 'Show advanced' button to unhide the password protection section.
          • The 'Unmask' checkbox now becomes unselectable when the password protected lesson is set as 'No', and the password text area will also be disabled, until you change the password protected lesson to 'Yes', and then check the unmask area. Now changing the password protected lesson to 'No' will not disable the password area. - It might be worth considering if it is worthwhile fixing up that problem.

          Thanks.

          Show
          Adrian Greeve added a comment - [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [*] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Hi Rossie, I like the patch, I think that the solution that you have provided is good, just a couple of things: Perhaps in the testing instructions you could mention clicking the 'Show advanced' button to unhide the password protection section. The 'Unmask' checkbox now becomes unselectable when the password protected lesson is set as 'No', and the password text area will also be disabled, until you change the password protected lesson to 'Yes', and then check the unmask area. Now changing the password protected lesson to 'No' will not disable the password area. - It might be worth considering if it is worthwhile fixing up that problem. Thanks.
          Hide
          Michael de Raadt added a comment -

          Shifting to the next sprint.

          Show
          Michael de Raadt added a comment - Shifting to the next sprint.
          Hide
          Rossiani Wijaya added a comment -

          Updated patch to fix the bug that Adrian mentioned above.

          Changing the input type is not supported by IE8 or lower. Due to this I created a patch to check for the browser type and version, specifically for IE.

          I created 3 different patch for (note: for lib/javascript-static.js):

          • master: the minimum support for IE is 9, the patch is simply just changing the input type.
          • 2.4: the minimum support for IE is 8, the browser check for its type and version is done through YUI. (SimpleYUI is supported from 2.4 branch and on).
          • 2.3: the minimum support for IE is 8, the browser check for its type and version id done through navigator.userAgent.
          Show
          Rossiani Wijaya added a comment - Updated patch to fix the bug that Adrian mentioned above. Changing the input type is not supported by IE8 or lower. Due to this I created a patch to check for the browser type and version, specifically for IE. I created 3 different patch for (note: for lib/javascript-static.js): master: the minimum support for IE is 9, the patch is simply just changing the input type. 2.4: the minimum support for IE is 8, the browser check for its type and version is done through YUI. (SimpleYUI is supported from 2.4 branch and on). 2.3: the minimum support for IE is 8, the browser check for its type and version id done through navigator.userAgent.
          Hide
          Adrian Greeve added a comment -

          Hi,

          I think that this patch is good. Rossie has taken into consideration what is available and created solutions for the different branches. I tested this on firefox on each of the branches and it fixes the problem that I mentioned before.

          The only minor problem (if that) is that a couple of the comments are not started with a capital letter and are missing full stops at the end of the sentence. Rossie is now away for a couple of weeks. If the integrators are happy with the general code, could they please add a commit to fix up these coding style issues.

          Thanks.

          Show
          Adrian Greeve added a comment - Hi, I think that this patch is good. Rossie has taken into consideration what is available and created solutions for the different branches. I tested this on firefox on each of the branches and it fixes the problem that I mentioned before. The only minor problem (if that) is that a couple of the comments are not started with a capital letter and are missing full stops at the end of the sentence. Rossie is now away for a couple of weeks. If the integrators are happy with the general code, could they please add a commit to fix up these coding style issues. Thanks.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (sorry getting late in the cycle, I give up this)

          Show
          Eloy Lafuente (stronk7) added a comment - (sorry getting late in the cycle, I give up this)
          Hide
          Damyon Wiese added a comment -

          Hi Rosie and Adrian,

          Just to clarify our browser support position - while we don't support IE8 for new features on 25 - we should not be actively breaking things for those users either. If we already have working code for the 24 branch we should use that for 25 and not strip it down.

          Could you take a look at this Adrian while Rosie is away?

          Thanks!

          Show
          Damyon Wiese added a comment - Hi Rosie and Adrian, Just to clarify our browser support position - while we don't support IE8 for new features on 25 - we should not be actively breaking things for those users either. If we already have working code for the 24 branch we should use that for 25 and not strip it down. Could you take a look at this Adrian while Rosie is away? Thanks!
          Hide
          Damyon Wiese added a comment -

          Pulling this out of integration for this week as it's getting a bit late in the cycle.

          Show
          Damyon Wiese added a comment - Pulling this out of integration for this week as it's getting a bit late in the cycle.
          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
          Rossiani Wijaya added a comment -

          Hi Damyon,

          I updated the patch for master to add support for IE8 as well.

          Re-submit for integration review.

          Show
          Rossiani Wijaya added a comment - Hi Damyon, I updated the patch for master to add support for IE8 as well. Re-submit for integration review.
          Hide
          Damyon Wiese added a comment -

          Thanks Rosie,

          This has been integrated now to 23, 24 and master branches. I tested all branches with ie8 in integration and everything worked well.

          One note is that the commit message was quite long - it would be preferred if the first line of your git message could be kept a bit shorter in future (around 72 chars is great!).

          Show
          Damyon Wiese added a comment - Thanks Rosie, This has been integrated now to 23, 24 and master branches. I tested all branches with ie8 in integration and everything worked well. One note is that the commit message was quite long - it would be preferred if the first line of your git message could be kept a bit shorter in future (around 72 chars is great!).
          Hide
          Rossiani Wijaya added a comment -

          Thanks Damyon.

          Sure, will keep it shorter for my next commit.

          Show
          Rossiani Wijaya added a comment - Thanks Damyon. Sure, will keep it shorter for my next commit.
          Hide
          Jason Fowler added a comment -

          All good

          Show
          Jason Fowler added a comment - All good
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Show
          Damyon Wiese added a comment - Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

            People

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

              Dates

              • Created:
                Updated:
                Resolved: