Moodle
  1. Moodle
  2. MDL-34285

User profile field conditions missing a "is not empty" option.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Conditional activities
    • Labels:
      None
    • Testing Instructions:
      Hide
      1. Log in as an admin
      2. Enable conditional activities if you haven't already.
      3. Enter a course and enable conditional activities.
      4. Add a new forum and in the settings:
        • Check that you can add user conditions and that all the conditions strings are fine (equals, is empty, contains etc).
        • Check you see an "is not empty" option
      5. Set a single new user condition for the "Yahoo ID" field and set the condition to "is not empty"
      6. Log out and back in as a student.
      7. Make sure the user hasn't got a yahoo id set (who would right!)
      8. Browse to the course
      9. Make sure the forum is shown as inaccessible with a message about yahoo id being empty.
      Show
      Log in as an admin Enable conditional activities if you haven't already. Enter a course and enable conditional activities. Add a new forum and in the settings: Check that you can add user conditions and that all the conditions strings are fine (equals, is empty, contains etc). Check you see an "is not empty" option Set a single new user condition for the "Yahoo ID" field and set the condition to "is not empty" Log out and back in as a student. Make sure the user hasn't got a yahoo id set (who would right!) Browse to the course Make sure the forum is shown as inaccessible with a message about yahoo id being empty.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-34285-m24
    • Rank:
      42649

      Description

      The user profile field conditions are all in order but there is one obvious candidate missing "is not empty"
      Should be a simple change but will involve a new string and the checking of old strings.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Linked parent issue MDL-29538.

          Show
          Sam Hemelryk added a comment - Linked parent issue MDL-29538 .
          Hide
          Sam Hemelryk added a comment -

          Ok this is up for peer-review now.

          I've done three things here:

          1. Added an is not empty condition for user fields.
          2. Changed the way in which requirement errors are generated. Each condition now has its own so that they can be written properly.
          3. Copied strings being used in the filters lang file to the condition lang file and updated uses in condition code.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok this is up for peer-review now. I've done three things here: Added an is not empty condition for user fields. Changed the way in which requirement errors are generated. Each condition now has its own so that they can be written properly. Copied strings being used in the filters lang file to the condition lang file and updated uses in condition code. Cheers Sam
          Hide
          Frédéric Massart added a comment - - edited

          Hi Sam,

          code looks good. But you might want to consider those two minor things:

          • When get_full_information() to display the restrictions on a course page, a string error occurs. I guess you just forgot to add $details->operator on line 811.
          • And there is this endless empty() debate. I'd rather go for a !empty() or is_numeric() test, but I know that's not really how it has been done everywhere else. However, considering "How many siblings do you have?", 0 is a correct answer.

          Could you write some test instructions and push for integration whenever you're ready? Thanks!

          Show
          Frédéric Massart added a comment - - edited Hi Sam, code looks good. But you might want to consider those two minor things: When get_full_information() to display the restrictions on a course page, a string error occurs. I guess you just forgot to add $details->operator on line 811. And there is this endless empty() debate. I'd rather go for a !empty() or is_numeric() test, but I know that's not really how it has been done everywhere else. However, considering "How many siblings do you have?", 0 is a correct answer. Could you write some test instructions and push for integration whenever you're ready? Thanks!
          Hide
          Sam Hemelryk added a comment -

          Thanks for looking at this Fred.
          I've fixed up the borked string and have pushed this up for integration review now.
          In regards to the empty || is_numeric thing in this case empty is intentional.
          The whole area needs cleaning up, however in order to do that properly we would have to add a new feature for conditions to allow them to specify the operators applicable to them.
          There is an issue for that already as well.
          The reason being that checkboxes for example are empty when they are equal to 0.
          Anyway, fingers crossed it all gets cleaned up properly in the future.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for looking at this Fred. I've fixed up the borked string and have pushed this up for integration review now. In regards to the empty || is_numeric thing in this case empty is intentional. The whole area needs cleaning up, however in order to do that properly we would have to add a new feature for conditions to allow them to specify the operators applicable to them. There is an issue for that already as well. The reason being that checkboxes for example are empty when they are equal to 0. Anyway, fingers crossed it all gets cleaned up properly in the future. Cheers Sam
          Hide
          Aparup Banerjee added a comment -

          thanks, thats integrated into master now.

          Show
          Aparup Banerjee added a comment - thanks, thats integrated into master now.
          Hide
          Andrew Davis added a comment -

          "Not available if your yahoo is empty." LOL

          This technically fails but only because the testing instructions are incorrect. There are actually 4 different behaviours here.

          1) Student doesn't have the requirement, activity is set to be hidden entirely. (this seems to be the default so step #9 doesnt work until you further modify the activity settings)

          2) Student doesn't have the requirement, activity is set to be shown but greyed out.

          3) Student has the requirement, activity is available.

          4) User doesn't have the requirement but is a teacher/admin so sees the activity greyed out regardless of the activity setting.

          Passing. In future, please try to have the testing instructions cover more of the possible paths through the code

          Show
          Andrew Davis added a comment - "Not available if your yahoo is empty." LOL This technically fails but only because the testing instructions are incorrect. There are actually 4 different behaviours here. 1) Student doesn't have the requirement, activity is set to be hidden entirely. (this seems to be the default so step #9 doesnt work until you further modify the activity settings) 2) Student doesn't have the requirement, activity is set to be shown but greyed out. 3) Student has the requirement, activity is available. 4) User doesn't have the requirement but is a teacher/admin so sees the activity greyed out regardless of the activity setting. Passing. In future, please try to have the testing instructions cover more of the possible paths through the code
          Hide
          Aparup Banerjee added a comment -

          yay, it works!

          This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

          Thank you all for taking the time to get us here.

          cheers!

          Show
          Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: